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
service controller: update LB nodes updates - update nodes if providerID has changed #120492
service controller: update LB nodes updates - update nodes if providerID has changed #120492
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/assign @thockin |
staging/src/k8s.io/cloud-provider/controllers/service/controller.go
Outdated
Show resolved
Hide resolved
expectedUpdateCalls: []fakecloud.UpdateBalancerCall{}, | ||
}, | ||
{ | ||
desc: "Change node with empty empty providerID", |
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.
NIt: empty
is mentioned twice
staging/src/k8s.io/cloud-provider/controllers/service/controller.go
Outdated
Show resolved
Hide resolved
return retSuccess | ||
|
||
if nodeNames(newNodes).Equal(nodeNames(oldNodes)) && providerIDsEqual(oldNodes, newNodes) { | ||
return retNeedRetry |
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.
Why did this return code change? I feel like I am missing something...
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.
yyeah, sames nodes and with the same providersids looks like means retSuccess
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.
do we want to be paranoid and and check the node.uid too ? kubernetes/cloud-provider-gcp#589 , VMs can be registered with the same name and providerID but be different 🤷
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.
it should be retSuccess
here
@alexanderConstantinescu @thockin
I think it's worth to check node UID as Antonio suggested
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.
OTOH the shouldSyncUpdatedNode dos not check for UID change
maybe it should...
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.
WDYT?
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.
Wouldn't keying the nodes by their uid instead of their names, address that? So: nodeUID(newNodes).Equal(nodeUID(oldNodes))
.
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 like that (with a big comment why :)
staging/src/k8s.io/cloud-provider/controllers/service/controller.go
Outdated
Show resolved
Hide resolved
64bb24a
to
56e47a1
Compare
f13e77c
to
58c7095
Compare
…providerID has changed Signed-off-by: czawadka <czawadka@google.com>
58c7095
to
349e3eb
Compare
/retest |
This LGTM. I'll approve and hold. We can either fix this to use UID instead of name, followup to change name to UID, or just use name and put UID in protoNode. @cezarygerard you can clear the hold if you want to merge this as-is. @alexanderConstantinescu - OK with that? /lgtm |
LGTM label has been added. Git tree hash: b3d0f0208f1ecc27fef96e42f8370447ea6dda4b
|
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cezarygerard, thockin 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 |
I'm OK with that |
I vote for "just use name and put UID in protoNode." to have all the information, so we can add a klog.V2() message logging the difference in |
/hold cancel Seems there is consensus, so I think is good to merge, the node uid problem was not reproduced or at least I didn't see it in the wild neither had time to probe it , so we can move that discussion to #120630 and proceed with this, that is an issue we hit in production Thanks |
/retest |
1.26 Cherry pick of #120492: service controller: improve node lifecycle updates - update
1.27 Cherry pick of #120492: service controller: improve node lifecycle updates - update
…-#120492-upstream-release-1.28 Automated cherry pick of #120492: service controller: improve node lifecycle updates - update
service controller: update LB nodes updates - update nodes if providerID has changed
Current implementation will NOT update nodes if Provider ID has changed
Provider ID is added by node controller fraction of the second after the service controller can pick up node changes.
/kind bug
What this PR does / why we need it:
Lack of update after Provider ID was added to the node can result in not adding node to load balancer
Special notes for your reviewer:
This PR should drop the #120311
Please see reviewers' comments in 805d39f
[didn't create PR before amendment changed the commit id]
Does this PR introduce a user-facing change?
NONE