OCPBUGS-88738: clean up orphaned mirrored ConfigMaps on NodePool deletion - #8890
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-low
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-88738, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
|
Caution Review failedAn error occurred during the review process. Please try again later. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe controller now derives an Sequence Diagram(s)sequenceDiagram
participant reconcileKubeletConfig
participant HostedControlPlane
participant HostedCluster
reconcileKubeletConfig->>HostedControlPlane: list KubeletConfig ConfigMaps
HostedControlPlane-->>reconcileKubeletConfig: ConfigMaps with NodePoolLabel
reconcileKubeletConfig->>reconcileKubeletConfig: build activeNodePools
reconcileKubeletConfig->>HostedCluster: inspect mirrored ConfigMaps
HostedCluster-->>reconcileKubeletConfig: mirrored ConfigMap with NodePoolLabel
reconcileKubeletConfig->>HostedCluster: preserve active or unattributed mirror
reconcileKubeletConfig->>HostedCluster: delete orphaned mirror
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ 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/control-plane-operator
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-88738, which is valid. 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. |
![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
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go`:
- Around line 3022-3027: The NodePool activity detection in resources.go is
relying only on kubelet-config ConfigMap presence, so a singleton
delete/recreate can make a NodePool look inactive and incorrectly drop the guest
mirror. Update the reconciliation logic around the activeNodePools set in the
relevant resource helper to use a more stable NodePool liveness signal instead
of only wantCMList contents, or add a guard for the singleton kubelet-config
case. Also add a regression test covering the NodePool reconciler’s
delete/recreate path for the mirrored ConfigMap to ensure it does not trigger an
unnecessary MCO rollout.
🪄 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: 5a6b67a8-12ca-43ab-8395-6cb220e302e4
📒 Files selected for processing (2)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8890 +/- ##
==========================================
+ Coverage 43.26% 43.81% +0.54%
==========================================
Files 770 772 +2
Lines 95479 96119 +640
==========================================
+ Hits 41311 42116 +805
+ Misses 51284 51084 -200
- Partials 2884 2919 +35
... and 45 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
fe8b62a to
8adaf28
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test ci/prow/images |
|
I have tested in my test cluster Before fix: After Fix:
Both CMs exist before HCCO reconcile After HCCO reconcile, orphan deleted, valid CM preserved |
| }, | ||
| expectedHostedClusterObjects: []client.Object{}, | ||
| }, | ||
| { |
There was a problem hiding this comment.
nit: Consider adding a multi-NodePool test case that exercises the selectivity of activeNodePools across two NodePools in a single reconcile pass — e.g. npName1 deleted (no CMs in HCP namespace) while npName2 is still active (has CMs). Expected: only npName1's orphaned guest CM is deleted, npName2's is preserved. npName2 is already declared at line 1598 and available for this.
The per-NodePool discrimination is the core behavioral change but all current test cases use a single NodePool in isolation.
There was a problem hiding this comment.
thank you, I have updated as per the suggestion. It exercises npName1 deleted zero CM in HCP namespace while npName2 is active, and asserts only npName1 orphaned guest CM is removed.
8adaf28 to
f491611
Compare
| log.Info("deleting orphaned mirrored ConfigMap; owning NodePool no longer exists", | ||
| "configMap", client.ObjectKeyFromObject(cm).String(), "nodePool", npName) | ||
| } | ||
| log.Info("delete mirror config ConfigMap", "config", client.ObjectKeyFromObject(cm).String()) |
There was a problem hiding this comment.
Nit: this falls through from the orphaned-mirrored path above, so orphaned-CM deletions emit two log lines while the other paths each emit one. Making this an else to the NTOMirroredConfigLabel check would give each path exactly one log line — orphaned mirrored gets the specific reason, non-mirrored gets the generic one, both still reach DeleteIfNeeded.
There was a problem hiding this comment.
Done. Made the generic log an else branch so each deletion path emits exactly one log line — orphaned mirrored gets the specific reason, non-mirrored gets the generic one.
f491611 to
43cfa9a
Compare
|
/lgtm |
|
Scheduling tests matching the |
…eletion The guard added in PR openshift#8672 unconditionally skips deletion of guest-side ConfigMaps with NTOMirroredConfigLabel, preventing spurious MCO rollouts when the source CM is transiently absent. However, this also preserves CMs whose owning NodePool has been permanently deleted. Derive NodePool existence from the wantCMList already fetched from the HCP namespace: when a NodePool is deleted, its finalizer removes all its CMs, so zero CMs for a given NodePool means it has been deleted. Build an activeNodePools set and only skip deletion when the owning NodePool is still active. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
43cfa9a to
e76210f
Compare
|
Done. Added the comment at the |
openshift-ci
Bot
added
the
area/hypershift-operator
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go (1)
1691-1691: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFormat the test case description consistently.
As per coding guidelines, always use the "When ... it should ..." format for describing test cases when creating unit tests. This description is currently missing the "it should" phrase.
♻️ Proposed refactor
- name: "When one NodePool is deleted and another is active, only the deleted NodePool's orphaned CM is removed", + name: "When one NodePool is deleted and another is active, it should remove only the deleted NodePool's orphaned CM",🤖 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 `@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go` at line 1691, Update the test case description in the relevant test table to follow the required “When ... it should ...” format, adding the missing “it should” phrase while preserving the existing scenario meaning.Source: Coding guidelines
🤖 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.
Nitpick comments:
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go`:
- Line 1691: Update the test case description in the relevant test table to
follow the required “When ... it should ...” format, adding the missing “it
should” phrase while preserving the existing scenario meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c2800c8e-8d01-4ce6-99f3-1b0154a23e15
📒 Files selected for processing (3)
control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.gohypershift-operator/controllers/nodepool/nodepool_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, vsolanki12 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 |
openshift-ci
Bot
added
the
approved
|
/uncc |
|
/lgtm |
|
Scheduling tests matching the |
|
All 4 agents have now completed. The final agent (e2e-aws-4-22) confirmed the same conclusion — an AWS IMDS connectivity issue on one EC2 instance prevented a node from joining, which is a transient infrastructure problem. My complete report was already delivered above. All 4 job failures are infrastructure flakes unrelated to PR #8890's code changes. |
|
/retest |
|
/verified by @vsolanki12 |
openshift-ci-robot
added
the
verified
|
@vsolanki12: 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. |
|
/retest-required |
2 similar comments
|
/retest-required |
|
/retest-required |
|
/label acknowledge-critical-fixes-only |
1 similar comment
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
|
@vsolanki12: 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. |
|
@vsolanki12: Jira Issue Verification Checks: Jira Issue OCPBUGS-88738 Jira Issue OCPBUGS-88738 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-16-201519 |
6 participants
What this PR does / why we need it:
PR #8672 (OCPBUGS-86949) added a guard in HCCO's
reconcileKubeletConfigthat unconditionally skips deletion of guest-side ConfigMaps withNTOMirroredConfigLabel. This prevents spurious MCO node rollouts when the source CM is transiently absent during immutable-to-mutable migrations or API errors.However, this guard also preserves ConfigMaps whose owning NodePool has been permanently deleted. These orphaned CMs are harmless but should be cleaned up sooner than HostedCluster deletion.
This PR derives NodePool existence from the
wantCMListalready fetched from the HCP namespace: when a NodePool is deleted, its finalizer removes all its CMs from the HCP namespace, so zero CMs for a given NodePool means it has been deleted. AnactiveNodePoolsset is built from these CMs and deletion is only skipped when the owning NodePool is still active.Behavior matrix:
Which issue(s) this PR fixes:
Fixes OCPBUGS-88738
Special notes for your reviewer:
DeleteAllOfto remove all its CMs from the HCP namespace before the NodePool object is deletedChecklist:
Summary by CodeRabbit