-
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
[KCCM]: have providerID trigger re-sync, but not be required for load balancer syncs #117602
[KCCM]: have providerID trigger re-sync, but not be required for load balancer syncs #117602
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. |
/sig network |
// This predicate just validates if the providerID has been set and triggers a | ||
// node sync. It is _not_ used when determining which nodes to use when | ||
// configuring the load balancer's backend pool. | ||
func nodeHasProviderIDPredicate(node *v1.Node) bool { |
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.
We only use this in shouldSyncUpdatedNode()
now, and we test whether the predicate changed. IOW we handle "" to "value" and "value" to "", but not "foo" to "bar". Should we just compare that ?
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.
Can the providerID even change? I was under the impression it can't.
EDIT: it technically can obviously. And yeah: we cover more ground by just changing the logic to react to that. I'll update
/assign @nckturner @andrewsykim |
Thanks! /lgtm |
LGTM label has been added. Git tree hash: 0a158f33766466206d4d0aceebd7d32e16b27c6f
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexanderConstantinescu, 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 |
@alexanderConstantinescu
So after all there's no UpdateLoadBalancer after setting node's providerID |
Which version of Kube is this? You might be using a version which is missing: #120943 |
indeed, mine is before 1.28.4, thanks |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
#117388 left some open ended questions around "are we sure all cloud providers even set a providerID at all?"...we are not. Should a cloud provider not set the providerID but still expect the service controller to provision LBs: then this won't work for them, because #117388 enforces that the providerID is defined on all nodes that we pass to the cloud provider when configuring load balancers.
This PR revises that logic, and instead triggers a resync when any node gets the providerID assigned, but does not filter nodes based on it.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/assign @thockin