OCPBUGS-86494: fix(nodepool): preserve KubeVirt userdata Secrets during NodePool rollout - #8581
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@amasolov: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughThis PR extends the existing user-data Secret retention logic to support KubeVirt platform NodePools in addition to AWS. The change adds an unconditional early return in 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
area/hypershift-operator
|
Hi @amasolov. Thanks for your PR. I'm waiting for a openshift 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hypershift-operator/controllers/nodepool/token.go`:
- Around line 181-187: The current guard unconditionally preserves KubeVirt
userdata Secrets; change it to preserve only while the old NodePool generation
is still referenced by existing VMs/Machines and delete once rollout completes.
Replace the permanent-platform check (t.nodePool.Spec.Platform.Type !=
hyperv1.KubevirtPlatform) with a rollout-aware condition that calls a helper
(e.g., add/use a function like isOldGenerationReferenced(nodePool) or
t.oldGenerationStillReferenced()) which inspects current Machine/VM objects or
NodePool status to detect references to the outdated generation, and only skip
deletion when that helper returns true; otherwise proceed to remove the
outdatedUserDataSecret() as normal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 67ae5b4b-b88a-4532-a142-fe3cf9cabca4
📒 Files selected for processing (4)
hypershift-operator/controllers/nodepool/secret_janitor.gohypershift-operator/controllers/nodepool/secret_janitor_test.gohypershift-operator/controllers/nodepool/token.gohypershift-operator/controllers/nodepool/token_test.go
| // For AWS and KubeVirt, we keep the old userdata Secret so old Machines during rollout can be deleted. | ||
| // AWS: deletion fails because of https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/3805. | ||
| // KubeVirt: the Secret is shared by all VMs in the NodePool generation and must survive until | ||
| // the rollout completes and all old VMs are gone. | ||
| // TODO (Alberto): enable back deletion when the PR above gets merged. | ||
| if t.nodePool.Spec.Platform.Type != hyperv1.AWSPlatform { | ||
| if t.nodePool.Spec.Platform.Type != hyperv1.AWSPlatform && t.nodePool.Spec.Platform.Type != hyperv1.KubevirtPlatform { | ||
| userDataSecret := t.outdatedUserDataSecret() |
There was a problem hiding this comment.
Make KubeVirt retention rollout-aware instead of permanent.
This guard keeps outdated KubeVirt userdata Secrets forever, not just during rollout. That diverges from the stated behavior (“preserve during rollout until old VMs are gone”) and can accumulate stale Secrets across upgrades. Please gate retention on an explicit “old generation still referenced” condition, then delete once rollout is complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hypershift-operator/controllers/nodepool/token.go` around lines 181 - 187,
The current guard unconditionally preserves KubeVirt userdata Secrets; change it
to preserve only while the old NodePool generation is still referenced by
existing VMs/Machines and delete once rollout completes. Replace the
permanent-platform check (t.nodePool.Spec.Platform.Type !=
hyperv1.KubevirtPlatform) with a rollout-aware condition that calls a helper
(e.g., add/use a function like isOldGenerationReferenced(nodePool) or
t.oldGenerationStillReferenced()) which inspects current Machine/VM objects or
NodePool status to detect references to the outdated generation, and only skip
deletion when that helper returns true; otherwise proceed to remove the
outdatedUserDataSecret() as normal.
There was a problem hiding this comment.
This intentionally follows the same unconditional platform guard that already exists for AWS. A rollout-aware check would be a larger refactor (both platforms would benefit) and is better suited as a follow-up.
There was a problem hiding this comment.
@amasolov, fair point — keeping parity with the existing AWS unconditional guard is a reasonable approach for this minimal fix, and a rollout-aware cleanup that benefits both platforms is a sensible follow-up scope. Would you like me to open a GitHub issue to track that improvement (rollout-aware Secret cleanup for AWS and KubeVirt once old-generation VMs/Machines are gone)?
🧠 Learnings used
Learnt from: twolff-gh
Repo: openshift/hypershift PR: 8472
File: hypershift-operator/controllers/nodepool/azure.go:0-0
Timestamp: 2026-05-13T15:17:58.584Z
Learning: When constructing CAPZ `UserAssignedIdentity.ProviderID` values (used under `AzureMachineSpec.UserAssignedIdentities`), use the documented canonical `azure:///`-prefixed format: `azure:///subscriptions/{subscriptionId}/resourceGroups/{rg}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/{name}`. CAPZ internally trims the prefix before calling the Azure SDK, so adding the prefix is still required by the contract. Use `capzutil.ProviderIDPrefix` ("azure:///") when building the ProviderID, and do not recommend removing or stripping this prefix in code review.
amasolov
changed the title
openshift-ci-robot
added
jira/severity-important
|
@amasolov: This pull request references Jira Issue OCPBUGS-86494, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@amasolov: This pull request references Jira Issue OCPBUGS-86494, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8581 +/- ##
=======================================
Coverage 44.50% 44.51%
=======================================
Files 774 774
Lines 96980 96986 +6
=======================================
+ Hits 43164 43170 +6
Misses 50828 50828
Partials 2988 2988
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
cd5f18b to
1edbe53
Compare
|
Rebase result: succeeded locally but push failed. The rebase onto The rebase pulled in upstream commits that modify To resolve this, the PR author can rebase locally and push: git fetch upstream main
git rebase upstream/main
git push --force-with-lease origin fix/kubevirt-keep-old-userdata-during-rolloutAlternatively, enable "Allow edits from maintainers" on this PR and retry |
…lout On KubeVirt, the bootstrap userdata Secret in the HCP namespace is shared by all VMs in a given NodePool generation. Premature deletion of this Secret during a rolling update causes CAPK to fail reconciliation for VMs that still reference it. Extend the existing AWS platform guard in both shouldKeepOldUserData() and cleanupOutdated() to also cover KubevirtPlatform, deferring Secret cleanup until the rollout is complete. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Assisted-by: Claude Opus 4.6 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
Address review feedback: distinguish the AWS temporary workaround (CAPA bug, removable when OCP < 4.16 support is dropped) from the KubeVirt architectural requirement in the code comments. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…erData Replace the if-chain with a switch statement and extract the AWS version check into its own method for clarity. Also align the doc comments with the style already used in token.go (distinguishing the AWS temporary workaround from the KubeVirt architectural requirement). Signed-off-by: Alexey Masolov <amasolov@redhat.com> Assisted-by: Claude Opus 4.6 (via Cursor) Co-authored-by: Cursor <cursoragent@cursor.com>
6aba3d8 to
a2945ea
Compare
openshift-ci-robot
removed
the
verified
|
@bryan-cox I've rebased this branch onto the latest |
|
/ok-to-test |
|
/lgtm Putting back on from rebase |
|
Scheduling tests matching the |
|
/ok-to-test |
|
@bryan-cox failing on the flaky tests again |
@amasolov please take a look here and follow the workflow if you find the flaky tests are not related to your PR - https://hypershift.pages.dev/how-to/ci/triage/presubmit-failures/ |
|
@bryan-cox followed the triage workflow: https://hypershift.pages.dev/how-to/ci/triage/presubmit-failures/ Failing required jobs (commit a2945ea):
Relation to this PR: none. Diff is only KubeVirt userdata Secret retention in Job history: both jobs are mostly red across other PRs right now (~14/20 failures each), so per the flowchart this looks like an infra/flake issue to escalate rather than a PR code change. Will escalate in #forum-ocp-hypershift. Also noting |
|
/retest-required |
|
/test e2e-aws |
|
/verified by @amasolov |
|
@amasolov: Jira verification commands are restricted to collaborators for this repo. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/verified by @amasolov See #8581 (comment) |
openshift-ci-robot
added
the
verified
|
@bryan-cox: This PR has been marked as verified by 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/ok-to-test |
|
@amasolov: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@amasolov: Jira Issue Verification Checks: Jira Issue OCPBUGS-86494 Jira Issue OCPBUGS-86494 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Fix included in release 5.0.0-0.nightly-2026-07-28-081944 |
7 participants
What this PR does / why we need it:
On KubeVirt, the bootstrap userdata Secret in the HCP namespace (
user-data-<nodepool>-<hash>) is shared by all VMs in a given NodePool generation. During a rolling update, HyperShift'ssecretJanitorandToken.cleanupOutdated()eagerly delete the old userdata Secret as soon as a new config version is computed. Any VMs from the previous generation that are still running will then fail CAPK reconciliation because their referenced Secret no longer exists.This PR extends the existing AWS platform guard (which already preserves old userdata Secrets for a different reason) to also cover KubeVirt. With this change, old userdata Secrets are retained until the rollout finishes and old VMs are cleaned up.
Which issue(s) this PR fixes:
Fixes premature garbage collection of shared userdata Secrets on the KubeVirt provider during NodePool rolling updates.
Special notes for your reviewer:
Manual testing
Verified on a real KubeVirt HostedCluster (OCP 4.20 management cluster with CNV 4.20.15, guest release 4.22.3, 2-replica NodePool). Triggered a rolling update by changing VM compute cores. The old userdata Secret was preserved throughout the rollout, both old machines were deleted cleanly, and the rollout completed with no errors. Full test evidence: #8581 (comment)
Checklist:
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Tests