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: cleanup the "master" taint on CP nodes during upgrade #110095
kubeadm: cleanup the "master" taint on CP nodes during upgrade #110095
Conversation
- iniconfiguration.go: stop applying the "master" taint for new clusters; update related unit tests in _test.go - apply.go: Remove logic related to cleanup of the "master" label during upgrade - apply.go: Add cleanup of the "master" taint on CP nodes during upgrade - controlplane_nodes_test.go: remove test for old "master" taint on nodes (this needs backport to 1.24, because we have a kubeadm 1.25 vs kubernetes test suite 1.24 e2e test)
[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 |
/milestone v1.25 |
// TODO: remove the legacy taint check in 1.25: | ||
// https://github.com/kubernetes/kubeadm/issues/2200 | ||
for _, cp := range controlPlanes.Items { | ||
framework.ExpectNodeHasTaint(f.ClientSet, cp.GetName(), &corev1.Taint{Key: controlPlaneLabel, Effect: corev1.TaintEffectNoSchedule}) | ||
framework.ExpectNodeHasTaint(f.ClientSet, cp.GetName(), &corev1.Taint{Key: controlPlaneLabelLegacy, Effect: corev1.TaintEffectNoSchedule}) |
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.
controlplane_nodes_test.go: remove test for old "master" taint
on nodes (this needs backport to 1.24, because we have a kubeadm
1.25 vs kubernetes test suite 1.24 e2e test)
due to our skew tests we also have to make this change in release-1.24.
otherwise this test job will fail:
https://k8s-testgrid.appspot.com/sig-cluster-lifecycle-kubeadm#kubeadm-kinder-latest-on-1-24
the 1.24 test suite will try to find both old + new taint on the CP nodes, but kubeadm 1.25 will only add the new taint.
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 opened #110103 for this.
@@ -238,10 +238,10 @@ func RemoveOldControlPlaneLabel(client clientset.Interface) error { | |||
return nil |
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.
RemoveOldControlPlaneLabel
is not used anymore and can be removed as https://github.com/kubernetes/kubernetes/pull/110095/files#diff-fd7aac3195ba96f8f6beb0a559e48ebbc93ef87df4c197de70cacf9a97117c56L162.
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.
This is for just nit.
/lgtm
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 missed that. It has a TODO linking to the issue, at least.
// If the old taint is present and the new taint is missing, patch the node with the new taint. | ||
// When the old taint is missing, assume the user has manually untainted the node and take no action. | ||
if !hasNewTaint && hasOldTaint { | ||
// If the old taint is present remove it |
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.
If someone manually removes the new taint before they upgrade to v1.25, are we assured that the new taint is put back before we remove the old taint?
(I'd expected to see at least a warning that the old taint is being removed but the new taint isn't present).
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.
If someone manually removes the new taint before they upgrade to v1.25, are we assured that the new taint is put back before we remove the old taint?
I personally don't expect this use case. If someone manually removed the new taint we can assume they wanted it removed and we should not put it back in. There was a related bug report that kubeadm should not reintroduce removed taints - e.g. in single control plane node clusters.
(I'd expected to see at least a warning that the old taint is being removed but the new taint isn't present).
The output of "upgrade" will print a message that the old taint is being removed. Combined with the release note it should give sufficient awareness to users.
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.
Cool, makes sense.
What type of PR is this?
/kind cleanup
/kind deprecation
What this PR does / why we need it:
for new clusters; update related unit tests in _test.go
during upgrade
during upgrade
on nodes (this needs backport to 1.24, because we have a kubeadm
1.25 vs kubernetes test suite 1.24 e2e test)
Which issue(s) this PR fixes:
xref kubernetes/kubeadm#2200
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: