OCPBUGS-86949: Guard HCCO KubeletConfig CM deletion against transient source absence - #8672
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-low
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-86949, 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. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
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:
📝 WalkthroughWalkthroughThis PR modifies kubelet-config reconciliation to delete immutable hosted-cluster kubelet-config ConfigMaps before recreating them and reinitializes the in-memory hostedClusterCM to avoid stale server-populated fields. Cleanup now skips ConfigMaps labeled as NTO-mirrored when the upstream source is absent. The immutable-delete helper was refactored to use a predicate-based delete that only targets immutable KubeletConfig-owned ConfigMaps and logs deletions. Tests were extended with cases for mirrored preservation, non-mirrored deletion, immutable recreation, and asserting reconciled ConfigMaps are mutable. Sequence Diagram(s)sequenceDiagram
participant reconcileKubeletConfig
participant deleteImmutableConfigMapIfNeeded
participant kubeAPIServer
reconcileKubeletConfig->>deleteImmutableConfigMapIfNeeded: invoke deleteImmutableConfigMapIfNeeded
deleteImmutableConfigMapIfNeeded->>kubeAPIServer: Delete immutable CM if KubeletConfig-owned
kubeAPIServer-->>deleteImmutableConfigMapIfNeeded: deletion result
deleteImmutableConfigMapIfNeeded-->>reconcileKubeletConfig: return (log deletion)
reconcileKubeletConfig->>reconcileKubeletConfig: reinitialize hostedClusterCM object
reconcileKubeletConfig->>kubeAPIServer: CreateOrUpdate with clean state
kubeAPIServer-->>reconcileKubeletConfig: create/update result
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@vsolanki12: This pull request references Jira Issue OCPBUGS-86949, 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. |
openshift-ci
Bot
added
area/control-plane-operator
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8672 +/- ##
=======================================
Coverage 41.84% 41.85%
=======================================
Files 759 759
Lines 94073 94083 +10
=======================================
+ Hits 39361 39374 +13
+ Misses 51956 51953 -3
Partials 2756 2756
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/uncc |
|
I have tried to reproduce and tested the fix in my test cluster as below: Environment: Custom CPO image deployed: Test scenario: Deleted source KubeletConfig CM from HCP namespace to simulate transient absence during immutable-to-mutable migration (OCPBUGS-85778). Before fix: guest-side CM immediately removed: After fix: guest side CM preserved: HCCO logs guard active: |
|
/cc @jparrill |
sdminonne
left a comment
sdminonne
left a comment
There was a problem hiding this comment.
I realy like this. I just need a couple of explanation and some words for reviewers and people after us 😃
Mind follow-up?
TY!
| return nil | ||
| } | ||
|
|
||
| // deleteImmutableConfigMapIfNeeded checks if a ConfigMap exists and is immutable, |
There was a problem hiding this comment.
Any reason to delete this comment?
There was a problem hiding this comment.
Thanks for pointing out. The old comment described the previous implementation which didn't have the ownership guard. Removed it during the refactor but missed adding the updated one. Will add it back with the updated description.
| if err := r.client.Get(ctx, client.ObjectKeyFromObject(cm), existingCM); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil | ||
| _, err := k8sutil.DeleteIfNeededWithPredicate(ctx, r.client, cm, func(existing *corev1.ConfigMap) bool { |
There was a problem hiding this comment.
The code being replaced is getting existingCM a fresh object from the server.
The new version is deleting directly the hostedClusterCM. It's true that it's resetting immutable and resourceVersion but what about other fields (if any)?
There was a problem hiding this comment.
Thank you for the suggestion, Instead of clearing individual fields, I now reinitialize hostedClusterCM as a fresh object after deleteImmutableConfigMapIfNeeded. DeleteIfNeededWithPredicate calls Get() which populates the passed object with all server side fields. Reinitializing discards all of them, so no stale fields can leak into the subsequent CreateOrUpdate.
| continue | ||
| } | ||
| if cm.Labels[nodepool.NTOMirroredConfigLabel] == "true" { | ||
| log.Info("skipping deletion of mirrored ConfigMap with transiently absent source", |
There was a problem hiding this comment.
I know that it''s unlikely it will happen but this is preventing deletion of ANY mirrored CM even the ones potentially orphaned.
May you add a comment to explain the deletion path in case it happens?
There was a problem hiding this comment.
Thanks your, Added a comment explaining the trade off. Mirrored CMs have a source in the HCP namespace managed by the NodePool controller. During delete+recreate migrations or transient API errors, the source can be briefly absent. Deleting the guest copy would cause NTO to regenerate MachineConfigs without it, triggering MCO node rollouts. If the source is permanently removed (e.g. NodePool deletion), the orphaned guest CM is harmless and will be cleaned up when the HostedCluster is deleted.
| return err | ||
| } | ||
| hostedClusterCM.SetResourceVersion("") | ||
| hostedClusterCM.Immutable = nil |
There was a problem hiding this comment.
Posterity needs some words to explain this. 😄
May you add a comment?
There was a problem hiding this comment.
Addressed together with #2. The reinitialize approach replaces the two field clears, and the inline comment explains why:
DeleteIfNeededWithPredicate populates the object via Get with all server-side fields, so we reinitialize to avoid leaking stale fields into CreateOrUpdate.
816742a to
cd14bae
Compare
|
/retest-required |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Dropped some comments. Thanks!
| "configMap", client.ObjectKeyFromObject(cm).String()) | ||
| continue | ||
| } | ||
| log.Info("delete mirror config ConfigMap", "config", client.ObjectKeyFromObject(cm).String()) |
There was a problem hiding this comment.
This log message says "transiently absent source" but mutateKubeletConfig always sets NTOMirroredConfigLabel: "true" on every guest CM it creates — so this guard matches all KubeletConfig CMs, including permanently orphaned ones after a NodePool is deleted. The message will appear in logs for CMs whose source is never coming back, which could mislead operators.
Suggestion: "skipping deletion of mirrored ConfigMap; source may be transiently absent or permanently removed after NodePool deletion"
There was a problem hiding this comment.
Updated the log message to "skipping deletion of mirrored ConfigMap; source may be transiently absent or permanently removed after NodePool deletion" to accurately reflect both scenarios.
| log.Info("skipping deletion of mirrored ConfigMap with transiently absent source", | ||
| "configMap", client.ObjectKeyFromObject(cm).String()) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Since mutateKubeletConfig at line 3056 always sets NTOMirroredConfigLabel: "true", this guard effectively disables orphan cleanup for all KubeletConfig CMs — not just ones with a transiently absent source. The trade-off is correct (stale CM < spurious MCO rollout), but worth a TODO for a future improvement: check whether the owning NodePool (via hyperv1.NodePoolLabel) still exists before unconditionally skipping.
There was a problem hiding this comment.
Added a TODO to check whether the owning NodePool (via NodePoolLabel) still exists before skipping, to allow cleanup of truly orphaned CMs in a future improvement.
There was a problem hiding this comment.
If I'm understanding properly once this PR get merged (if we merge as it is), orphaned KubeletConfig CMs will persist in the guest cluster until the HostedCluster is deleted. Right?
Unsure what is meant with TODO here. Please follow-up
There was a problem hiding this comment.
Yes, that's correct. Orphaned KubeletConfig CMs will persist in the guest cluster until the HostedCluster is deleted. The trade-off is: a stale CM in openshift-config-managed is harmless (NTO ignores CMs that don't match any active MachineConfigPool), while deleting it during a transient source absence triggers an MCO node rollout.
The TODO proposes a future improvement: before skipping deletion, check whether the owning NodePool (tracked via NodePoolLabel) still exists. If the NodePool is gone, the CM is truly orphaned and safe to delete. This requires a cross-namespace lookup so I'll raise a follow-up Jira to track it
| hostedControlPlaneObjects: []client.Object{}, | ||
| existHostedControlPlaneObjects: []client.Object{ | ||
| makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1), | ||
| }, |
There was a problem hiding this comment.
nit: This test exercises a path that can't happen in production — mutateKubeletConfig always sets both KubeletConfigConfigMapLabel and NTOMirroredConfigLabel. Worth a one-line comment like // Defensive: this path is only reachable for CMs created before NTOMirroredConfigLabel was introduced so future readers know why it exists.
There was a problem hiding this comment.
Added a comment: // Defensive: this path is only reachable for CMs created before NTOMirroredConfigLabel was introduced.
| existingCM := &corev1.ConfigMap{} | ||
| if err := r.client.Get(ctx, client.ObjectKeyFromObject(cm), existingCM); err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil |
There was a problem hiding this comment.
The refactor to DeleteIfNeededWithPredicate + ownership guard is clean. One test gap: there's no case for an immutable CM without KubeletConfigConfigMapLabel — i.e., the return false at line 3054. Could you add a test case with an immutable CM that lacks the label and verify it is NOT deleted?
There was a problem hiding this comment.
Added test case "When guest CM is immutable but not a KubeletConfig, it should not be deleted" covers the return false path when KubeletConfigConfigMapLabel is absent on an immutable CM.
|
/lgtm |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
439580d to
c2510fa
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
added
area/testing
…bsence HCCO's reconcileKubeletConfig deletes guest-side ConfigMaps whose source is absent from the HCP namespace. During the immutable-to-mutable migration (OCPBUGS-85778) or any transient API error, the source CM can be briefly absent, causing HCCO to delete the guest copy. NTO then regenerates MachineConfigs without it, triggering an MCO node rollout. Skip deletion of guest-side CMs that carry the NTOMirroredConfigLabel, since their source is expected to reappear on the next reconcile. Also refactor deleteImmutableConfigMapIfNeeded to use DeleteIfNeededWithPredicate with a KubeletConfigConfigMapLabel ownership guard, and clear ResourceVersion after the predicate-based delete to avoid stale-resourceVersion errors on the subsequent CreateOrUpdate. Signed-off-by: Vimal Solanki <vsolanki@redhat.com>
c2510fa to
1e0b1ba
Compare
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
@vsolanki12: The following test failed, say
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. |
|
I now have all the evidence needed. The failure is completely clear — this is a CI infrastructure issue, not a code issue. Let me output the report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThis failure is a CI infrastructure issue — not related to the PR code changes. The job failed because the CI build cluster ( Root CauseThe root cause is CI build cluster (
This is a transient CI infrastructure problem unrelated to PR #8672's code changes. Recommendations
Evidence
|
|
/retest |
|
/verified by @vsolanki12 Before Fix: After Fix: |
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. |
|
@vsolanki12: Jira Issue Verification Checks: Jira Issue OCPBUGS-86949 Jira Issue OCPBUGS-86949 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-06-23-223621 |
…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>
…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>
…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>
…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>
…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>
…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>
7 participants
What this PR does / why we need it:
HCCO's
reconcileKubeletConfigdeletes guest-side ConfigMaps whose source is absent from the HCP namespace. During the immutable-to-mutable migration (OCPBUGS-85778) or any transient API error, the source CM can be briefly absent, causing HCCO to delete the guest copy. NTO then regenerates MachineConfigs without it, triggering an MCO node rollout.This PR:
NTOMirroredConfigLabel, since their source is expected to reappear on the next reconcile cycledeleteImmutableConfigMapIfNeeded: Refactors to useDeleteIfNeededWithPredicatewith aKubeletConfigConfigMapLabelownership check, preventing accidental deletion of unrelated immutable ConfigMapsDeleteIfNeededWithPredicatepopulates the passed object via its internalGet, leaking server-side fields (ResourceVersion, Immutable, etc.). The object is reinitialized as a freshConfigMapwith only Name/Namespace to avoid stale field leakage into the subsequentCreateOrUpdateWhich issue(s) this PR fixes:
Fixes OCPBUGS-86949
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests