diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go index 122b46d8c1c8..8dee56532e6d 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources.go @@ -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) @@ -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 + } 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) @@ -3027,26 +3048,22 @@ func (r *reconciler) reconcileKubeletConfig(ctx context.Context) error { return nil } -// deleteImmutableConfigMapIfNeeded checks if a ConfigMap exists and is immutable, -// 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 + _, err := k8sutil.DeleteIfNeededWithPredicate(ctx, r.client, cm, func(existing *corev1.ConfigMap) bool { + 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 { diff --git a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go index 8e1c5de8e352..7d2f0c06c7dd 100644 --- a/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go +++ b/control-plane-operator/hostedclusterconfigoperator/controllers/resources/resources_test.go @@ -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", @@ -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), + }, + 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 { @@ -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), @@ -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" diff --git a/test/e2e/nodepool_mirrorconfigs_test.go b/test/e2e/nodepool_mirrorconfigs_test.go index d8f93cf5eee4..ebaa9eb93d78 100644 --- a/test/e2e/nodepool_mirrorconfigs_test.go +++ b/test/e2e/nodepool_mirrorconfigs_test.go @@ -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, diff --git a/test/e2e/v2/tests/nodepool_lifecycle_test.go b/test/e2e/v2/tests/nodepool_lifecycle_test.go index 42dc3d327e9e..3fd02bcfb266 100644 --- a/test/e2e/v2/tests/nodepool_lifecycle_test.go +++ b/test/e2e/v2/tests/nodepool_lifecycle_test.go @@ -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,