Skip to content

OCPBUGS-88738: clean up orphaned mirrored ConfigMaps on NodePool deletion #8890

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
Jul 16, 2026
Merged

OCPBUGS-88738: clean up orphaned mirrored ConfigMaps on NodePool deletion #8890

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
38 changes: 28 additions & 10 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 @@ -3013,8 +3013,20 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error {
return fmt.Errorf("failed to list KubeletConfig ConfigMaps from controlplane namespace %s: %w", r.hcpNamespace, err)
}
want := set.Set[string]{}
// Derive which NodePools are still active from CMs in 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.
// Note: a narrow race exists for a NodePool with exactly one immutable kubelet-config CM
// during the one-time immutable-to-mutable migration — the NodePool controller briefly
// deletes the CM before recreating it. If HCCO reconciles in that window the NodePool
// appears inactive. This is acceptable: the window is milliseconds, the migration is a
// one-time event, and the guest CM would be recreated on the next reconcile.
activeNodePools := set.Set[string]{}
for _, cm := range wantCMList.Items {
want.Insert(cm.Name)
if npName := cm.Labels[hyperv1.NodePoolLabel]; npName != "" {
activeNodePools.Insert(npName)
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
for _, cm := range wantCMList.Items {
hostedClusterCM := &corev1.ConfigMap{
Expand Down Expand Up @@ -3059,18 +3071,24 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error {
}
// 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.
// absent. Deleting the guest copy would cause NTO to regenerate MachineConfigs
// without it, triggering MCO node rollouts. However, if the owning NodePool has been
// deleted, its finalizer has already removed all its CMs from the HCP namespace, so
// the guest copy is orphaned and safe to delete.
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
npName := cm.Labels[hyperv1.NodePoolLabel]
// Defensive: if the CM has no NodePoolLabel, we cannot determine whether

@jparrill jparrill Jul 14, 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: The deletion path logs both "configMap" and "nodePool", but the skip path only logs "configMap". Adding "nodePool", npName here would help operators trace which NodePool a preserved CM belongs to.

@vsolanki12 vsolanki12 Jul 14, 2026 •
edited
Loading

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.

Done. Added "nodePool", npName to the skip log line for traceability.

// its owning NodePool still exists; preserve it to avoid spurious rollouts.
if npName == "" || activeNodePools.Has(npName) {
log.Info("skipping deletion of mirrored ConfigMap; source transiently absent but owning NodePool still active",
"configMap", client.ObjectKeyFromObject(cm).String(), "nodePool", npName)
continue
}

@jparrill jparrill Jul 14, 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.

question: The message says "owning NodePool no longer exists", but there's a scenario where the NodePool is still active but the user removed all kubelet configs from its spec — zero CMs in HCP namespace, but the NodePool is alive. The deletion is correct either way, but the log could be misleading. Something like "owning NodePool has no remaining kubelet-config CMs in HCP namespace" would be accurate for both cases. Up to you — the behavior is right regardless.

@vsolanki12 vsolanki12 Jul 14, 2026 •
edited
Loading

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.

Good point. Updated the log message to "owning NodePool has no remaining kubelet-config CMs in HCP namespace" which is accurate for both the NodePool-deleted and config-removed-from-spec cases.

log.Info("deleting orphaned mirrored ConfigMap; owning NodePool has no remaining kubelet-config CMs in HCP namespace",
"configMap", client.ObjectKeyFromObject(cm).String(), "nodePool", npName)
} else {

@jparrill jparrill Jul 14, 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 (pre-existing): This log uses "config" as the structured key while the two new log lines above use "configMap". Since you're already restructuring this block, would be nice to normalize to "configMap" for consistency.

@vsolanki12 vsolanki12 Jul 14, 2026 •
edited
Loading

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.

Done. Normalized to "configMap" for consistency.

log.Info("delete mirror config ConfigMap", "configMap", client.ObjectKeyFromObject(cm).String())
}
log.Info("delete mirror config ConfigMap", "config", client.ObjectKeyFromObject(cm).String())
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 Down
69 changes: 67 additions & 2 deletions 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 @@ -1649,13 +1649,21 @@ func TestReconcileKubeletConfig(t *testing.T) {
},
},
{
name: "When source CM is transiently absent, it should not delete the mirrored guest-side CM",
hostedControlPlaneObjects: []client.Object{},
// NodePool still active (another CM exists in HCP namespace with the same NodePoolLabel),
// but this particular source CM is transiently absent — preserve the guest copy.
name: "When source CM is transiently absent, it should not delete the mirrored guest-side CM",
hostedControlPlaneObjects: []client.Object{
// Another CM for the same NodePool proves it is still active.
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("baz", npName1, validation.LabelValueMaxLength), hcpNamespace, npName1, kubeletConfig1),
},
existHostedControlPlaneObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
// The "baz" CM is expected on the guest side too (reconciled from the HCP-namespace source above).
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("baz", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
},
expectedHostedClusterObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("baz", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
},
},
{
Expand All @@ -1667,6 +1675,63 @@ func TestReconcileKubeletConfig(t *testing.T) {
},
expectedHostedClusterObjects: []client.Object{},
},
{

@cblecker cblecker Jul 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@vsolanki12 vsolanki12 Jul 4, 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, 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.

// NodePool deleted: its finalizer has removed all CMs from the HCP namespace,
// so the orphaned guest-side mirrored CM should be cleaned up.
name: "When NodePool is deleted, it should delete orphaned mirrored guest-side CM",
hostedControlPlaneObjects: []client.Object{},
existHostedControlPlaneObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
},
expectedHostedClusterObjects: []client.Object{},
},
{
// Two NodePools: npName1 deleted (zero CMs in HCP namespace), npName2 still active.
// Only npName1's orphaned guest CM should be deleted; npName2's should be preserved.
name: "When one NodePool is deleted and another is active, only the deleted NodePool's orphaned CM is removed",
hostedControlPlaneObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("foo", npName2, validation.LabelValueMaxLength), hcpNamespace, npName2, kubeletConfig1),
},
existHostedControlPlaneObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("bar", npName1, validation.LabelValueMaxLength), hcNamespace, npName1, kubeletConfig1),
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("foo", npName2, validation.LabelValueMaxLength), hcNamespace, npName2, kubeletConfig1),
},
expectedHostedClusterObjects: []client.Object{
makeMirroredKubeletConfigConfigMap(netutil.ShortenName("foo", npName2, validation.LabelValueMaxLength), hcNamespace, npName2, kubeletConfig1),
},
},
{
// Defensive: mirrored CM without NodePoolLabel cannot be attributed to any NodePool,
// so preserve it to avoid spurious MCO rollouts.
name: "When mirrored CM has no NodePoolLabel, it should be preserved",
hostedControlPlaneObjects: []client.Object{},
existHostedControlPlaneObjects: []client.Object{
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "orphan-no-np-label",
Namespace: hcNamespace,
Labels: map[string]string{
nodepool.KubeletConfigConfigMapLabel: "true",
nodepool.NTOMirroredConfigLabel: "true",
},
},
Data: map[string]string{"config": kubeletConfig1},
},
},
expectedHostedClusterObjects: []client.Object{
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "orphan-no-np-label",
Namespace: hcNamespace,
Labels: map[string]string{
nodepool.KubeletConfigConfigMapLabel: "true",
nodepool.NTOMirroredConfigLabel: "true",
},
},
Data: map[string]string{"config": kubeletConfig1},
},
},
},
{
name: "When guest CM is immutable but not a KubeletConfig, it should not be deleted",
hostedControlPlaneObjects: []client.Object{
Expand Down
3 changes: 3 additions & 0 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,9 @@ func (r *NodePoolReconciler) delete(ctx context.Context, nodePool *hyperv1.NodeP
}

// Delete any ConfigMap belonging to this NodePool i.e. TunedConfig ConfigMaps.
// NOTE: HCCO's reconcileKubeletConfig infers NodePool liveness from the
// presence of CMs in this namespace. This cleanup must complete before
// the finalizer is removed.
err = r.DeleteAllOf(ctx, &corev1.ConfigMap{},
client.InNamespace(controlPlaneNamespace),
client.MatchingLabels{nodePoolAnnotation: nodePool.GetName()},
Expand Down