kubeadm: set the kubelet cgroup driver to "systemd" during "init" #99471
Conversation
k8s-ci-robot
added
release-note-action-required
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
k8s-ci-robot
added
priority/important-soon
|
/assign @SataQiu @fabriziopandini |
k8s-ci-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/cc |
There was a problem hiding this comment.
@neolit123 thanks for working on this!
Few nits, but overall lgtm
|
|
||
| // MutateCgroupDriver can be called to set the KubeletConfiguration cgroup driver to systemd | ||
| // Currently this cannot be as part of Default() because the function is called for | ||
| // upgrades too, which can break existing nodes after a kubelet restart. |
There was a problem hiding this comment.
Do we plan to move this back to default in next releases or do we think that a different upgrade path will be always required given that we are seeing users upgrading very old clusters?
Also, for sake of future memory, what about adding a note on Default about this field being defaulted out of band?
There was a problem hiding this comment.
as i've mentioned, the defaulting being the same for all commands is problematic.
we have a similar case for the "docker cgroup driver detection" code, that i like to move to dockershim.
since i really don't like keeping MutateCgroupDriver() forever i wonder what is best here...
i don't wish to do add this change in Default() for this release because it would break upgrading users in just one release, but we could say that in 1 more release kubeadm will start doing that in all of its commands, which means we are going to merge the code in Default() and users have 1 release to adapt their nodes / CRs (move to the systemd driver).
i like this approach better, because this code separation is not ideal.
if you agree to that i can add a comment over Default() about MutateCgroupDriver().
..and update the release note too.
There was a problem hiding this comment.
Given that this could be potentially disruptive, I think we should:
- advertise the change through usual channels
- have a warning in upgrade plan for at least one cycle (you are running with two cgroup drivers, see the doc to migrate)
- (finally) make this our default
There was a problem hiding this comment.
advertise the change through usual channels
i can send a notification to k-dev and #kubeadm
have a warning in upgrade plan for at least one cycle (you are running with two cgroup drivers, see the doc to migrate)
i can see where in upgrade plan this can be added for this PR.
the migration docs are waiting for review and i will like to them from the release note too.
kubernetes/website#26786
There was a problem hiding this comment.
@fabriziopandini added a warning during "plan".
neolit123
force-pushed
the
1.21-default-systemd-cgroup-driver
branch
from
5200c55
to
74da3dc
Compare
|
@fabriziopandini
appreciate comments on the wording of the release note too. |
| // upgrades too, which can break existing nodes after a kubelet restart. | ||
| // TODO: https://github.com/kubernetes/kubeadm/issues/2376 | ||
| func MutateCgroupDriver(cfg *kubeadmapi.ClusterConfiguration) { | ||
| const driver = "systemd" |
There was a problem hiding this comment.
Shall we consider using global constant instead of redefining the driver?
There was a problem hiding this comment.
i prefer to contain it here.
actually there is already a constant in preflight for the driver:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/preflight/checks_linux.go#L35
but i'd like to remove that preflight and docker detection code, if we get this in 1.21:
#98222
|
@neolit123 Few nits, but overall LGTM! |
|
updated release note to:
after i prepared the new documentation page: |
The kubeadm documentation instructs users to set the container runtime driver to "systemd", since kubeadm manages a kubelet via the systemd init system. The kubelet default however is "cgroupfs". For new clusters set the driver to "systemd" unless the user is explicit about it. The same defaulting would not happen during "upgrade".
neolit123
force-pushed
the
1.21-default-systemd-cgroup-driver
branch
from
74da3dc
to
b6ff320
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
The kubeadm documentation instructs users to set the container
runtime driver to "systemd", since kubeadm manages a kubelet via
the systemd init system. The kubelet default however is "cgroupfs".
For new clusters set the driver to "systemd" unless the user
is explicit about it. The same defaulting would not happen
during "upgrade".
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#2376
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: