Retry LB sync while nodes are still cloud-provider initializing - #939
Retry LB sync while nodes are still cloud-provider initializing#939pyadagiri-do wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a race where nodes still marked as cloud-provider-uninitialized (and therefore missing required public addressing) could cause partial backend updates for REGIONAL_NETWORK EXTERNAL load balancers, potentially preventing later convergence when node addresses appear. It does so by returning api.RetryError in that scenario and ensuring the retry error is preserved through error wrapping, with unit tests covering transient vs permanent “private-only” cases.
Changes:
- Return
api.RetryError(with a short retry delay) when filteredpublicNetUnreadynodes are still cloud-provider-uninitialized, avoiding partial backend updates. - Wrap
buildLoadBalancerRequesterrors using%wso upstream can detectRetryErrorviaerrors.As. - Extend unit tests to assert retry behavior and distinguish transient initialization races from permanently ineligible nodes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cloud-controller-manager/do/loadbalancers.go | Adds initialization-aware retry behavior for node filtering and preserves retry semantics via %w error wrapping. |
| cloud-controller-manager/do/loadbalancers_test.go | Adds helpers and test coverage to verify RetryError behavior and retry delay for initialization vs permanent filtering cases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if classification == nodeClassPublicNetUnready { | ||
| klog.V(4).Infof("Node %s filtered: no external IP addresses (required for REGIONAL_NETWORK EXTERNAL load balancer)", node.Name) | ||
| state.filteredCount++ | ||
| state.publicNetUnreadyCount++ | ||
| state.publicNetUnreadyNodes = append(state.publicNetUnreadyNodes, node) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cloud-controller-manager/do/loadbalancers_test.go:7416
- These assertions only exercise
buildLoadBalancerRequestdirectly, so they do not verify the behavior this PR depends on:RetryErrorsurviving the new wrappers inEnsureLoadBalancerandUpdateLoadBalancer. A regression back to%sin either public path would leave these tests green while upstream resumes exponential backoff. Add public-method cases with an initializing private-only node and asserterrors.As(err, &retryErr)(and the retry delay) for both paths.
var retryErr *api.RetryError
gotRetry := errors.As(err, &retryErr)
|
The guard treats 'taint present + no ExternalIP' as "still initializing, address is coming." That makes sense for the race in ESC-23605. But what happens if a node ends up stuck with the taint indefinitely? |
skumarc-do
left a comment
skumarc-do
left a comment
There was a problem hiding this comment.
What happens if we are scaling the nodes and one of the node is stuck / in the retry phase, will the other nodes get processed successfully?
Yup, if a node is stuck with the taint, we will keep retrying. So anyway as the node is in broken, so should be fine I guess ? |
Good catch. brought it up on the related thread for discussion |
updated the approach now. to call updateLB with ready nodes. And later if there return retry error if there are nodes initializing or stuck. This should unblock the other nodes processing |
Summary
api.RetryErrorso upstream retries for the rest.api.RetryErrorthrough service annotation patching so upstream does not fall back to exponential backoff.