-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
kubeadm: adjust kubeadm skew policy for upgrades #120825
Conversation
Skipping CI for Draft Pull Request. |
I tested with 1.28 control-plane node with 1.26 kubelet node.
I need to double check later. |
/assign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/priority important-soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
for the above question
LGTM label has been added. Git tree hash: 42c5c65dfb0bdddc6055761d5e2f88c860aaa84f
|
After this change, the kubernetes/cmd/kubeadm/app/preflight/checks.go Lines 610 to 612 in 0786fcc
|
but the skew becomes N-3 of the kubeadm version, while the sig architecture change is N-3 of the control plane version. the current skew is documented here: if the user uses kubeadm 1.28, and applies a control-plane N-1 1.27, then if they use the N-3 kubelet skew it will be 1.25, but the skew between CP and kubelet will be N-2. |
i think this is still OK to do like so, i.e. we support the N-3 if the user doesn't use a custom control plane version.
this should not close the k/kubeadm ticket because we need e2e and docs changes too. |
proposing release note:
please double check my wording. |
It sounds OK as well. Use kubeadm to kubeadm can only upgrade to the kubeadm version or n-1 control plane.
|
@@ -453,7 +453,8 @@ var ( | |||
MinimumControlPlaneVersion = getSkewedKubernetesVersion(-1) | |||
|
|||
// MinimumKubeletVersion specifies the minimum version of kubelet which kubeadm supports | |||
MinimumKubeletVersion = getSkewedKubernetesVersion(-1) | |||
// Refer to https://kubernetes.io/releases/version-skew-policy/#kubelet-1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add some comments that kubelet version can be N-3 of the kubeadm version, as control-plane is n or n-1, this can meet N-3 of the control plane version.
// Allows the kubelet version to be N-3 of the kubeadm version, provided that the control plane version is either N or N-1, meeting the maximum N-3 requirement for the control plane version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i think we should drop this comment and just leave the old comment (line 455):
// Refer to https://kubernetes.io/releases/version-skew-policy/#kubelet-1
the constants.go is not a place where users should read what we have. it's for us / developers to manage how kubeadm works.
if they (users) visit the k8s.io website they can find the kubeadm and k8s skews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update it to https://kubernetes.io/releases/version-skew-policy/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's ok to not link to docs. these comments are for developers to understand.
bdd8cc2
to
4ff321c
Compare
4ff321c
to
7b1d873
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: 3f5df90ed307e59775cfed002a1a8266c71eda37
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacoxu, SataQiu 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 |
/hold cancel |
ci bot hangs /hold cancel |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#2924
Special notes for your reviewer:
Does this PR introduce a user-facing change?
The KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-architecture/3935-oldest-node-newest-control-plane