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
Make kubelet set alpha.kubernetes.io/provided-node-ip unconditionally #109794
Make kubelet set alpha.kubernetes.io/provided-node-ip unconditionally #109794
Conversation
pkg/kubelet/nodestatus/setters.go
Outdated
// node controller in the cluster but kubelet is still running | ||
// the in-tree provider. Adding this annotation in all cases | ||
// ensures that while Addresses flap between the competing | ||
// controllers, they at least flap consistently. |
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.
nice comment, thanks Matt
/assign @nckturner |
cc @cheftako |
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 am not super familiar with code here, but this generally makes sense to me
/lgtm
Just to make future references to this PR slightly easier to read, can you add a note to the "What this PR does / why we need it:" section, i.e.
|
/triage accepted |
/lgtm |
cc @kubernetes/sig-node-pr-reviews |
/assign @yujuhong |
ping @yujuhong @kubernetes/sig-node-bugs @kubernetes/sig-node-pr-reviews, who is the correct reviewer/approver from sig-node for this? |
Hi @mdbooth My name is Marky Jackson and I am one of the k8s 1.25 bug triage shadow assigned to track this body of work. Just checking in to see if this is still on track for k8s 1.25? |
I certainly hope so! Nothing to do from my side that I'm aware of beyond soliciting approvals. |
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.
One nit on the related doc. Otherwise LGTM
/lgtm |
/lgtm Who can approve? @dchen1107? |
I just found there is a KEP to handle this in/out tree cloud provider migration problem https://github.com/kubernetes/enhancements/tree/master/keps/sig-cloud-provider/2436-controller-manager-leader-migration , can not be used for this? |
Unfortunately not. We're effectively migrating the 'Node controller' from running locally in every kubelet to running centrally in a single Node controller. We'd need a separate leader election for each individual kubelet. If we wanted to add more annotations I suppose the kubelet could annotate the Node to indicate who is expected to manage it, and the Node controller could be taught to ignore kubelet-managed Nodes. However, this annotation would have an extremely limited useful lifetime. |
@aojea This PR involves the kubelet and the CCM, (the above KEP refers to a lock between the KCM and CCM) and is specific to the period of time when migration is taking place (old kubelets are still running and reconciling Node Addresses, and the new CCM also begins reconciling node addresses). In order for the above leader migration lock to be used for this, kubelet would need to take that migration lock before doing its node address management. Might be possible, but I think regardless we should still make the above change, because they are not mutually exclusive and it is a simple remediation to the flapping address behavior that is described above. |
@nckturner thanks for open pr for doc updates: kubernetes/website#35027 /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dchen1107, mdbooth 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 |
@nckturner I recommend adding a changelog entry (not |
agreed |
@mdbooth can you add a changelog entry like:
|
/release-note-edit |
…nconditionally Cherry-pick of upstream Kubernetes PR kubernetes#109794 (kubernetes#109794), which has not yet been approved and merged into upstream Kubernetes as of the latest EKS-D 1.23 release. If the PR is approved, Kubernetes currently intends to include it in the 1.25 release. This patch may change in future EKS-D releases as to stay aligned with any modification to the upstream PR on which it is based. This fixes a node addresses flapping bug related to legacy and external cloud controller manager migration, as described in upstream Kubernetes issue kubernetes#109793 (kubernetes#109793) From the original PR description: "The easier of two possible fixes for an issue where node addresses flap during an upgrade to an external CCM. This fix causes kubelet to apply the alpha.kubernetes.io/provided-node-ip annotation unconditionally (not only when --cloud-provider=external). This does not exclude a future fix involving the root cause of the issue, which is that kubelet and cloud-controller-manager both attempt to manage node addresses when --cloud-provider is not set to external." Signed-off-by: Davanum Srinivas <davanum@gmail.com>
Make kubelet set alpha.kubernetes.io/provided-node-ip unconditionally - kubernetes#109794
What type of PR is this?
/kind bug
What this PR does / why we need it:
The easier of two possible fixes for an issue where node addresses flap during an upgrade to an external CCM. This fix causes kubelet to apply the alpha.kubernetes.io/provided-node-ip annotation unconditionally (not only when --cloud-provider=external). This does not exclude a future fix involving the root cause of the issue, which is that kubelet and cloud-controller-manager both attempt to manage node addresses when --cloud-provider is not set to external.
Which issue(s) this PR fixes:
Fixes #109793
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: