diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 22b24db8f99d..0b14124da340 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -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) + } } for _, cm := range wantCMList.Items { hostedClusterCM := &corev1.ConfigMap{ @@ -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 + // 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 + } + 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 { + 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) } diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index a0933df30e16..280733e28f8b 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -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), }, }, { @@ -1667,6 +1675,63 @@ func TestReconcileKubeletConfig(t *testing.T) { }, expectedHostedClusterObjects: []client.Object{}, }, + { + // 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{ diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index 4da04354e472..cfa62e28d4ce 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -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()},