fix: fallback to API server on missing Node topology key - #1485
Conversation
|
Welcome @ryan-mist! |
|
Hi @ryan-mist. Thanks for your PR. I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
This PR addresses a topology-provisioning failure mode where stale/incomplete cached Node labels can cause repeated provisioning errors even after the Node later gains the missing topology labels, by falling back to re-reading Node labels when cached labels are missing required topology keys.
Changes:
- Adjust
getTopologyFromNodeNameto fall back to fetching Node labels (via the lister) when cached Node labels are missing required topology keys. - Update
extractTopologyTermto return the available label map on missing-key errors (improves error context). - Add a regression test that reproduces “initially missing label, later added label” behavior and validates successful provisioning on retry.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/controller/topology.go |
Adds cache-miss-on-missing-keys fallback and preserves label context when topology keys are missing. |
pkg/controller/topology_test.go |
Adds a unit test covering the missing-label-then-added-label retry scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fc52847 to
427f790
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, ryan-mist The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-6.2 |
|
@msau42: new pull request created: #1516 DetailsIn response to this:
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-sigs/prow repository. |
|
/cherry-pick release-6.1 |
|
@msau42: new pull request created: #1517 DetailsIn response to this:
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-sigs/prow repository. |
|
/cherry-pick release-6.0 |
|
@msau42: new pull request created: #1518 DetailsIn response to this:
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-sigs/prow repository. |
What type of PR is this?
/kind bug
What this PR does / why we need it:e
This PR adds a fallback to the API server in Node Topology if the topology labels on the Node do not match the labels on the CSINode.
This fixes a bug where during provisioning, if the Node does not yet have all topology labels then provisioning continually fails. Even when the Node eventually does have all the labels.
Testing After Fix
In this case, I use EKS Auto to launch the Node. There are 4 required topology labels are:
Looking at the PVC events
Before this new version of external provisioner, PVCs would remain unbound with provisioning failures of:
Which issue(s) this PR fixes:
Fixes #1478
Special notes for your reviewer:
Does this PR introduce a user-facing change?: