OCPBUGS-60093: propagate nodeSelector removal to control plane workloads

support/controlplane-component/defaults.go

@@ -165,10 +165,18 @@ func (c *controlPlaneWorkload[T]) setDefaultOptions(cpContext ControlPlaneContex
 
 	// set PriorityClassName
 	podTemplateSpec.Spec.PriorityClassName = priorityClass(c.Name(), hcp)
-	// setNodeSelector sets a nodeSelector passed through the API.
-	// This is useful to e.g ensure control plane pods land in management cluster Infra Nodes.
+	// Merge HCP nodeSelector on top of the template's existing nodeSelector.
+	// This preserves entries from component YAML templates (e.g., kubernetes.io/os: linux)
+	// while allowing the HCP spec to add additional selectors. When hcp.Spec.NodeSelector
+	// is nil, only the template entries remain, so removing nodeSelector from the HCP
+	// correctly removes HCP-added entries without wiping template-defined entries.
+	// Removal is detected by ApplyManifest via getNodeSelectorCount, because the template
+	// is loaded fresh each reconciliation.
 	if hcp.Spec.NodeSelector != nil {
-		podTemplateSpec.Spec.NodeSelector = hcp.Spec.NodeSelector
+		if podTemplateSpec.Spec.NodeSelector == nil {
+			podTemplateSpec.Spec.NodeSelector = make(map[string]string)
+		}
+		maps.Copy(podTemplateSpec.Spec.NodeSelector, hcp.Spec.NodeSelector)
 	}
 
 	c.setLabels(podTemplateSpec, hcp)

support/controlplane-component/defaults_test.go

@@ -310,3 +310,88 @@ func TestSetDefaultOptions(t *testing.T) {
 	g.Expect(workloadObject.Spec.Template.Spec.SecurityContext.RunAsUser).To(Equal(ptr.To(int64(1002))))
 	g.Expect(workloadObject.Spec.Template.Spec.SecurityContext.FSGroup).To(Equal(ptr.To(int64(1002))))
 }
+
+func TestSetDefaultOptionsNodeSelector(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = hyperv1.AddToScheme(scheme)
+	_ = corev1.AddToScheme(scheme)
+	_ = appsv1.AddToScheme(scheme)
+
+	tests := []struct {
+		name                 string
+		hcpNodeSelector      map[string]string
+		templateNodeSelector map[string]string
+		expectedNodeSelector map[string]string
+	}{
+		{
+			name:                 "When HCP nodeSelector is set and template has no nodeSelector, it should apply HCP nodeSelector",
+			hcpNodeSelector:      map[string]string{"node-role": "infra"},
+			expectedNodeSelector: map[string]string{"node-role": "infra"},
+		},
+		{
+			name:                 "When HCP nodeSelector is nil and template has a nodeSelector, it should preserve the template nodeSelector",
+			hcpNodeSelector:      nil,
+			templateNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+			expectedNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+		},
+		{
+			name:                 "When HCP nodeSelector is nil and template has no nodeSelector, it should remain nil",
+			hcpNodeSelector:      nil,
+			templateNodeSelector: nil,
+			expectedNodeSelector: nil,
+		},
+		{
+			name:                 "When HCP nodeSelector is an empty map and template has a nodeSelector, it should preserve the template nodeSelector",
+			hcpNodeSelector:      map[string]string{},
+			templateNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+			expectedNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+		},
+		{
+			name:                 "When both HCP and template have nodeSelectors, it should merge them with HCP entries added",
+			hcpNodeSelector:      map[string]string{"node-role": "infra"},
+			templateNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+			expectedNodeSelector: map[string]string{"kubernetes.io/os": "linux", "node-role": "infra"},
+		},
+		{
+			name:                 "When HCP and template have overlapping keys, HCP value should take precedence",
+			hcpNodeSelector:      map[string]string{"kubernetes.io/os": "windows"},
+			templateNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+			expectedNodeSelector: map[string]string{"kubernetes.io/os": "windows"},
+		},
+		{
+			name:                 "When HCP nodeSelector is removed after being set, template nodeSelector should be preserved",
+			hcpNodeSelector:      nil,
+			templateNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+			expectedNodeSelector: map[string]string{"kubernetes.io/os": "linux"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			cpw := &controlPlaneWorkload[*appsv1.Deployment]{
+				name:             "test-component",
+				workloadProvider: &deploymentProvider{},
+				ComponentOptions: &testComponent{},
+			}
+			dep := &appsv1.Deployment{}
+			dep.Spec.Template.Spec.NodeSelector = tt.templateNodeSelector
+
+			hcp := &hyperv1.HostedControlPlane{
+				Spec: hyperv1.HostedControlPlaneSpec{
+					NodeSelector: tt.hcpNodeSelector,
+				},
+			}
+			err := cpw.setDefaultOptions(ControlPlaneContext{
+				HCP:    hcp,
+				Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
+			}, dep, nil)
+			g.Expect(err).ToNot(HaveOccurred())
+			if tt.expectedNodeSelector == nil {
+				g.Expect(dep.Spec.Template.Spec.NodeSelector).To(BeNil())
+			} else {
+				g.Expect(dep.Spec.Template.Spec.NodeSelector).To(Equal(tt.expectedNodeSelector))
+			}
+		})
+	}
+}

support/upsert/apply.go

@@ -104,9 +104,11 @@ func (p *applyProvider) ApplyManifest(ctx context.Context, c crclient.Client, ob
 func (p *applyProvider) update(ctx context.Context, c crclient.Client, obj crclient.Object, existing crclient.Object) (controllerutil.OperationResult, error) {
 	key := crclient.ObjectKeyFromObject(obj)
 
-	// Save original labels before preserveOriginalMetadata modifies them
+	// Save original labels and nodeSelector count before preserveOriginalMetadata
+	// modifies them, so removal detection compares against the true originals.
 	originalLabels := existing.GetLabels()
 	originalLabelCount := len(originalLabels)
+	originalNodeSelectorCount := getNodeSelectorCount(existing)
 
 	switch existingTyped := existing.(type) {
 	case *corev1.ServiceAccount:
@@ -141,6 +143,14 @@ func (p *applyProvider) update(ctx context.Context, c crclient.Client, obj crcli
 		isEqual = false
 	}
 
+	// Special handling for nodeSelector removal: DeepDerivative treats nil/empty
+	// nodeSelector as "don't care", so removing nodeSelector entries from a
+	// Deployment or StatefulSet would otherwise be silently ignored.
+	mutatedNodeSelectorCount := getNodeSelectorCount(obj)
+	if mutatedNodeSelectorCount < originalNodeSelectorCount {
+		isEqual = false
+	}
+
 	// If objects are equal (no changes needed), record no-op update and return early
 	if isEqual {
 		if p.loopDetector != nil {
@@ -192,7 +202,11 @@ func cleanupRemovalMarkers(obj crclient.Object) {
 }
 
 func preserveOriginalMetadata(original, mutated crclient.Object) {
-	labels := original.GetLabels()
+	// Clone the maps so we don't mutate the original object's metadata.
+	// Without cloning, the comparison in update() between 'existing' (serialized
+	// after this function runs) and 'obj' would see identical labels/annotations,
+	// hiding value changes when the key count stays the same.
+	labels := maps.Clone(original.GetLabels())
 	if labels == nil {
 		labels = make(map[string]string)
 	}
@@ -207,7 +221,7 @@ func preserveOriginalMetadata(original, mutated crclient.Object) {
 	}
 	mutated.SetLabels(labels)
 
-	annotations := original.GetAnnotations()
+	annotations := maps.Clone(original.GetAnnotations())
 	if annotations == nil {
 		annotations = make(map[string]string)
 	}
@@ -242,6 +256,22 @@ var (
 	}
 )
 
+// getNodeSelectorCount returns the number of nodeSelector entries for
+// Deployment and StatefulSet pod templates, or 0 for other types.
+// Only these two types are handled because they are the only workload kinds
+// managed by the cpov2 (control-plane-component) framework. Other kinds like
+// DaemonSet or Job are not used as control plane workloads.
+func getNodeSelectorCount(obj crclient.Object) int {
+	switch typed := obj.(type) {
+	case *appsv1.Deployment:
+		return len(typed.Spec.Template.Spec.NodeSelector)
+	case *appsv1.StatefulSet:
+		return len(typed.Spec.Template.Spec.NodeSelector)
+	default:
+		return 0
+	}
+}
+
 func toUnstructured(obj crclient.Object) (map[string]any, error) {
 	// Create a copy of the original object as well as converting that copy to
 	// unstructured data.