Skip to content

OCPBUGS-86949: Guard HCCO KubeletConfig CM deletion against transient source absence #8672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 1 commit into from
Jun 23, 2026
Merged

OCPBUGS-86949: Guard HCCO KubeletConfig CM deletion against transient source absence #8672

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 34 additions & 17 deletions control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2997,6 +2997,14 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error {
if err := r.deleteImmutableConfigMapIfNeeded(ctx, log, hostedClusterCM); err != nil {
return err
}
// DeleteIfNeededWithPredicate populates hostedClusterCM via Get with all server-side
// fields. Reinitialize to avoid leaking stale fields into the subsequent CreateOrUpdate.
hostedClusterCM = &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: cm.Name,
Namespace: ConfigManagedNamespace,
},
}

if result, err := r.CreateOrUpdate(ctx, r.client, hostedClusterCM, func() error {
return mutateKubeletConfig(&cm, hostedClusterCM)
Expand All @@ -3019,6 +3027,19 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error {
if want.Has(cm.Name) {
continue
}
// 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 here 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.
// TODO(OCPBUGS-88738): check whether the owning NodePool (via NodePoolLabel) still exists
// before unconditionally skipping, to allow cleanup of truly orphaned CMs.
if cm.Labels[nodepool.NTOMirroredConfigLabel] == "true" {
log.Info("skipping deletion of mirrored ConfigMap; source may be transiently absent or permanently removed after NodePool deletion",
"configMap", client.ObjectKeyFromObject(cm).String())
continue
}

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vsolanki12 vsolanki12 Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sdminonne sdminonne Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vsolanki12 vsolanki12 Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

log.Info("delete mirror config ConfigMap", "config", client.ObjectKeyFromObject(cm).String())

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@vsolanki12 vsolanki12 Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if _, err := k8sutil.DeleteIfNeeded(ctx, r.client, cm); err != nil {
return fmt.Errorf("failed to delete ConfigMap %s: %w", client.ObjectKeyFromObject(cm).String(), err)
Expand All @@ -3027,26 +3048,22 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error {
return nil
}

// deleteImmutableConfigMapIfNeeded checks if a ConfigMap exists and is immutable,

@sdminonne sdminonne Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to delete this comment?

@vsolanki12 vsolanki12 Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// and deletes it if necessary to allow recreation as a mutable ConfigMap.
// This handles migration from immutable ConfigMaps to mutable ones.
// deleteImmutableConfigMapIfNeeded deletes an existing immutable ConfigMap only if it
// carries the KubeletConfigConfigMapLabel ownership label, allowing it to be recreated
// as mutable by the subsequent CreateOrUpdate.
func (r *reconciler) deleteImmutableConfigMapIfNeeded(ctx context.Context, log logr.Logger, cm *corev1.ConfigMap) error {
existingCM := &corev1.ConfigMap{}
if err := r.client.Get(ctx, client.ObjectKeyFromObject(cm), existingCM); err != nil {
if apierrors.IsNotFound(err) {
return nil

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@vsolanki12 vsolanki12 Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_, err := k8sutil.DeleteIfNeededWithPredicate(ctx, r.client, cm, func(existing *corev1.ConfigMap) bool {

@sdminonne sdminonne Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

@vsolanki12 vsolanki12 Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if existing.Labels[nodepool.KubeletConfigConfigMapLabel] != "true" {
return false
}
return fmt.Errorf("failed to get ConfigMap %s: %w", client.ObjectKeyFromObject(cm).String(), err)
}

if existingCM.Immutable != nil && *existingCM.Immutable {
log.Info("deleting immutable KubeletConfig ConfigMap to recreate as mutable", "configMap", client.ObjectKeyFromObject(existingCM).String())
if _, err := k8sutil.DeleteIfNeeded(ctx, r.client, existingCM); err != nil {
return fmt.Errorf("failed to delete immutable ConfigMap %s: %w", client.ObjectKeyFromObject(existingCM).String(), err)
if existing.Immutable != nil && *existing.Immutable {
log.Info("deleting immutable KubeletConfig ConfigMap to recreate as mutable",
"configMap", client.ObjectKeyFromObject(existing).String())
return true
}
}

return nil
return false
})
return err
}

func mutateKubeletConfig(controlPlaneConfigMap, hostedClusterConfigMap *corev1.ConfigMap) error {
Expand Down
103 changes: 102 additions & 1 deletion control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1610,6 +1610,7 @@ func TestReconcileKubeletConfig(t *testing.T) {
hostedControlPlaneObjects []client.Object
existHostedControlPlaneObjects []client.Object
expectedHostedClusterObjects []client.Object
preservedObjects []client.Object
}{
{
name: "copy kubelet config from control plane NS",
Expand Down Expand Up @@ -1647,6 +1648,66 @@ func TestReconcileKubeletConfig(t *testing.T) {
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1),
},
},
{
name: "When source CM is transiently absent, it should not delete the mirrored guest-side CM",
hostedControlPlaneObjects: []client.Object{},
existHostedControlPlaneObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
},
expectedHostedClusterObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
},
},
{
// Defensive: this path is only reachable for CMs created before NTOMirroredConfigLabel was introduced.
name: "When source CM is absent and guest CM is not mirrored, it should be deleted",
hostedControlPlaneObjects: []client.Object{},
existHostedControlPlaneObjects: []client.Object{
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1),
},

@jparrill jparrill Jun 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vsolanki12 vsolanki12 Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment: // Defensive: this path is only reachable for CMs created before NTOMirroredConfigLabel was introduced.

expectedHostedClusterObjects: []client.Object{},
},
{
name: "When guest CM is immutable but not a KubeletConfig, it should not be deleted",
hostedControlPlaneObjects: []client.Object{
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcpNamespace, kubeletConfig1),
},
existHostedControlPlaneObjects: []client.Object{
// Immutable CM without KubeletConfigConfigMapLabel — should be left alone.
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "unrelated-immutable-cm",
Namespace: hcNamespace,
Labels: map[string]string{"some-other-label": "true"},
},
Immutable: ptr.To(true),
Data: map[string]string{"key": "value"},
},
},
expectedHostedClusterObjects: []client.Object{
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1),
},
preservedObjects: []client.Object{
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "unrelated-immutable-cm",
Namespace: hcNamespace,
},
},
},
},
{
name: "When guest CM is immutable, it should be deleted and recreated as mutable",
hostedControlPlaneObjects: []client.Object{
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcpNamespace, kubeletConfig1),
},
existHostedControlPlaneObjects: []client.Object{
makeImmutableKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1),
},
expectedHostedClusterObjects: []client.Object{
makeKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, kubeletConfig1),
},
},
}

for _, tc := range testCases {
Expand All @@ -1661,7 +1722,14 @@ func TestReconcileKubeletConfig(t *testing.T) {
}
g.Expect(r.reconcileKubeletConfig(t.Context())).To(Succeed())
for _, obj := range tc.expectedHostedClusterObjects {
g.Expect(r.client.Get(t.Context(), client.ObjectKeyFromObject(obj), obj)).To(Succeed(), "failed to get %s", client.ObjectKeyFromObject(obj))
actual := &corev1.ConfigMap{}
g.Expect(r.client.Get(t.Context(), client.ObjectKeyFromObject(obj), actual)).To(Succeed(), "failed to get %s", client.ObjectKeyFromObject(obj))
g.Expect(actual.Immutable).To(BeNil(), "recreated ConfigMap %s should be mutable", client.ObjectKeyFromObject(obj))
}
for _, obj := range tc.preservedObjects {
actual := &corev1.ConfigMap{}
g.Expect(r.client.Get(t.Context(), client.ObjectKeyFromObject(obj), actual)).To(Succeed(),
"preserved object %s should still exist after reconcile", client.ObjectKeyFromObject(obj))
}
listOpts := []client.ListOption{
client.InNamespace(hcNamespace),
Expand Down Expand Up @@ -1750,6 +1818,39 @@ func makeKubeletConfigConfigMap(name, namespace, data string) *corev1.ConfigMap
}
}

func makeMirroredKubeletConfigConfigMap(name, namespace, nodePoolName, data string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Labels: map[string]string{
nodepool.KubeletConfigConfigMapLabel: "true",
nodepool.NTOMirroredConfigLabel: "true",
hyperv1.NodePoolLabel: nodePoolName,
},
},
Data: map[string]string{
"config": data,
},
}
}

func makeImmutableKubeletConfigConfigMap(name, namespace, data string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: namespace,
Labels: map[string]string{
nodepool.KubeletConfigConfigMapLabel: "true",
},
},
Immutable: ptr.To(true),
Data: map[string]string{
"config": data,
},
}
}

func TestReconcileAuthOIDC(t *testing.T) {
testNamespace := "master-cluster1"
testHCPName := "cluster1"
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/nodepool_mirrorconfigs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ func (mc *MirrorConfigsTest) Run(t *testing.T, nodePool hyperv1.NodePool, nodes
},
[]e2eutil.Predicate[[]*corev1.ConfigMap]{
func(configMaps []*corev1.ConfigMap) (done bool, reasons string, err error) {
want, got := 0, len(configMaps)
want, got := 1, len(configMaps)
return want == got, fmt.Sprintf("expected %d KubeletConfig configmap, got %d", want, got), nil
},
}, nil,
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/v2/tests/nodepool_lifecycle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ func NodePoolMirrorConfigsTest(getTestCtx internal.TestContextGetter) {
},
[]e2eutil.Predicate[[]*corev1.ConfigMap]{
func(configMaps []*corev1.ConfigMap) (done bool, reasons string, err error) {
want, got := 0, len(configMaps)
want, got := 1, len(configMaps)
return want == got, fmt.Sprintf("expected %d KubeletConfig ConfigMaps, got %d", want, got), nil
},
}, nil,
Expand Down