Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow management of Kubelet configuration in CAPI #4464

Closed
fabriziopandini opened this issue Apr 12, 2021 · 17 comments
Closed

Allow management of Kubelet configuration in CAPI #4464

fabriziopandini opened this issue Apr 12, 2021 · 17 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@fabriziopandini
Copy link
Member

User Story

As a CAPI user I would like to manage the KubeletConfiguration in a declarative way

Detailed Description

This is a follow up of #1584.

As of today when initialising a cluster with CABPK, a default KubeletConfiguration is used and stored into kubelet-config-XX ConfigMap gets created, and the same config map is then carried away during upgrades.

The user can edit the kubelet-config-XX ConfigMap between upgrades, but this approach is not declarative and does not provide a good abstraction over this configuration detail in CAPI.

This issue is about discussing following options:

  • Allow the user to specify the KubeletConfiguration to be used for creating the cluster in KCP.
  • Use the KubeletConfiguration specified in KCP as a authoritative source of truth for the kubelet-config-XX ConfigMap (and thus reconcile it, preventing direct modification/drift from the authoritative source)
  • Allow the user to edit the KubeletConfiguration cluster in KCP, and use the new version for joining new nodes.

A few consideration should be addressed:

  • How to model the KubeletConfiguration into KCP or into the KubeadmConfig, given that
    • The KubeletConfiguration API is still evolving, and embedding a not stable API ultimately results in an unstable API surface for the users (it is also hard to maintain given that introduces hard dependency between two projects with different lifecycles)
    • The KubeletConfiguration makes sense at cluster level only because as of today it is a global configuration in kubeadm (Nb. node specific configuration are already supported using ExtraArgs into the NodeRegistration object)
  • How to manage following scenarios
    • Adding a user managed KubeletConfiguration to a cluster using a default one (seamless upgrade/adoption)
    • Changing a user managed KubeletConfiguration (Should this trigger rollout in KCP? what about other nodes in the cluster)
    • Removing a user managed KubeletConfiguration to a cluster using a user managed one (is this allowed? if yes, should we rollback to the default one or keep the last user provided one and carry over across versions/without reconciling?)

Anything else you would like to add:

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 12, 2021
@sbueringer
Copy link
Member

sbueringer commented Apr 12, 2021

If possible, I think it would be really nice to be able to use different KubeletConfiguration for control plane and worker nodes.

Our experience is that depending on the node size the eviction thresholds should be configured differently. E.g. control plane nodes with 4 cpu / 16 GI require different thresholds then worker nodes with 16 cpu / 64 Gi RAM.

On our internal platform we solved this by deploying KubeletConfigurations and assigning them to nodes after the kubeadm join is done.

This could work similarly to how kubeadmConfigSpec can be specified today either on the KubeadmControlPlane or via bootstrap.configRef on e.g. a MachineDeployment. I.e. we could add a kubeletConfiguration (or something similar) field to KubeadmControlPlane and MachineSpec

@detiber
Copy link
Member

detiber commented Apr 12, 2021

I agree with @sbueringer, if we allow this we should allow for a configuration on a more granular level than an entire cluster. I would argue probably allow for configuration at the KCP, MachinePool, and MachineDeployment/MachineSet level, that way one could get as granular as they wanted.

The one thing that I do worry about with this, though is that currently doesn't kubeadm manage the KubeletConfiguration at the version level? Are we going to end up having to handle any type of conversions or other automated mutations for users if they specify a KubeletConfiguration? Or are we going to get into odd situations where specifying the KubeletConfiguration will interfere with upgrade handling?

@sbueringer
Copy link
Member

sbueringer commented Apr 12, 2021

I'm not sure about the impact of updates. Just some details about our current solution:

  • per Node
    • create server in IaaS
    • kubeadm init / join
    • deploy KubeletConfigurationConfigMap
    • set Node KubeletConfig on the Node (spec/configSource/ConfigMap/*)
    • wait until KubeletConfig active (i.e. Node.Status.Config.Active.ConfigMap.{Name,UID,ResourceVersion} matches to the CM deployed before)

We don't execute commands like kubeadm update, but afaik nor does ClusterAPI. We create the KubeletConfiguration ConfigMaps with distinct names to the one used by kubeadm. We explicitly wait until kubeadm is done before changing the KubeletConfiguration. Up until now we didn't have any problems with this approach, but if we want to use this approach we should definitely ensure that kubeadm does not overwrite the active KubeletConfiguration once we set it.

@neolit123
Copy link
Member

neolit123 commented Apr 12, 2021

i think the best option is to just allow passing KubeletConfiguration to the kubeadm join command and apply some sort of a marker (e.g. # some yaml comment) to indicate this kubelet config.yaml is no longer managed by kubeadm:
kubernetes/kubeadm#1682

as far as the kubelet CM naming goes, we really want kubeadm to stop versioning the CM name:
kubernetes/kubeadm#1582
but this would be a breaking change to a lot of users and we need to deprecate it and manage the two differently named CMs in parallel for 3+ releases.

@neolit123
Copy link
Member

neolit123 commented Apr 12, 2021

but to share, @fabriziopandini and me had a lengthy discussion about the state of kubeletconfiguration vs kubelet flags vs instance specific config vs component config upgrade and overall this space is quite messy and problematic.

@fabriziopandini preferred that we continue using the pattern on passing a single config to kubeadm and then users could apply node specific overrides with kubeletExtraArgs.

of course, this has the risk that if the kubelet suddenly removes a number of its deprecated flags, users of CAPI will be in trouble.
(same goes for kubeadm users or users of other tools, that are doing that).

@sbueringer
Copy link
Member

sbueringer commented Apr 12, 2021

@neolit123
So from a CAPI perspective:

  • deploy KubeletConfiguration ConfigMap
  • pass the name of the KubeletConfiguration to kubeadm, kubeadm then ensure that the KubeletConfiguration ConfigMap is used and we don't have to.

Possible problem:

  • we can only change the KubeletConfiguration when creating new nodes, we cannot dynamically change it (but maybe that was the plan anyway)

of course, this has the risk that if the kubelet suddenly removes a number of its deprecated flags, users of CAPI will be in trouble.
(same goes for kubeadm users or users of other tools).

That was also one of my concerns. kubeletExtraArgs only works as long as we can configure all relevant configurations there. So not sure if that's a good long-term solution.

@enxebre
Copy link
Member

enxebre commented Apr 13, 2021

Relates to #4444

@fabriziopandini
Copy link
Member Author

fabriziopandini commented Apr 13, 2021

TL;DR; this issue focuses on what is supported today by kubeadm, which is already an improvement on the current status.
more invasive steps requires to act somewhere else and some clarity at kubernetes level

Instance/group of instance specific KubeletConfigurations is something useful, but the change should be implemented in kubeadm first, and only after in CAPI. However as @detiber and @neolit123 was pointing out:

  • As of today there is not automatic conversion mechanism for the KubeletConfiguration
  • the state of kubeletconfiguration vs kubelet flags vs instance specific config vs component config upgrade and overall this space is quite messy and problematic.

Given that, we should be careful on changing the status quo in kubeadm unless SIG-node/SIG-architecture clarifies the roadmap in this area.
FYI with the help of some colleagues I'm trying to raise the point in the SIG-node agenda, but any help will be appreciated.

@vincepri
Copy link
Member

/milestone Next

@k8s-ci-robot k8s-ci-robot added this to the Next milestone May 25, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 23, 2021
@fabriziopandini
Copy link
Member Author

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 24, 2021
@sbueringer
Copy link
Member

Apparently DynamicKubeletConfig is deprecated and the plan is to remove it with 1.23.

Some more context in: kubernetes/enhancements#281

https://github.com/kubernetes/kubernetes/pull/102966/files:

fs.MarkDeprecated("dynamic-config-dir", "Feature DynamicKubeletConfig is deprecated in 1.22 and will not move to GA. It is planned to be removed from Kubernetes in the version 1.23. Please use alternative ways to update kubelet configuration.")

@chrischdi
Copy link
Member

chrischdi commented Oct 6, 2021

Jep, I think it is already possible to configure the kubelet via kubeletconfiguration via a cloud-init provided file.

Edit: for us we will do that and configure the flag in the kubelet.service .

@dlipovetsky
Copy link
Contributor

dlipovetsky commented Oct 26, 2021

Adding a use case:

When I bootstrap a Machine, I want to set the Node's Spec.ProviderID field. I could use the flag, but it appears deprecated:

# kubelet --help | grep provider-id
      --provider-id string                                       Unique identifier for identifying the node in a machine database, i.e cloudprovider (DEPRECATED: This parameter should be set via the config file specified by the Kubelet's --config flag. See https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/ for more information.)

That said, the kubeadm docs say to set node-specific fields using flags, not the configuration file.

For reference, kind v0.11 uses the nodeRegistration.kubeletExtraArgs field to set providerID: init and join.

(As an aside, does the out-of-tree cloud provider controller set this field? If it does, I assume it does so without changing kubelet's configuration.)

@sbueringer
Copy link
Member

fyi. If I understood it correctly it should be possible to customize/patch the KubeletConfiguration with kubeadm/Kubernetes v1.25:

@fabriziopandini fabriziopandini added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini fabriziopandini removed this from the Next milestone Jul 29, 2022
@fabriziopandini fabriziopandini removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini
Copy link
Member Author

/close

Given that the future of component config is not clear I would avoid extending its usage in CAPI. Let's collect use cases where current approach does not work before committing to a way forward

@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: Closing this issue.

In response to this:

/close

Given that the future of component config is not clear I would avoid extending its usage in CAPI. Let's collect use cases where current approach does not work before committing to a way forward

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

10 participants