code review updates

Signed-off-by: Blake <blaketastic2@gmail.com>
feast-dev · franciscojavierarceo · Oct 21, 2025 · Oct 16, 2025 · Oct 17, 2025 · Oct 20, 2025
commit 621ffeab329d7fcb342889d5dfc6f61bf72947aa
@@ -46,7 +46,7 @@ _Appears in:_
 | `envFrom` _[EnvFromSource](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#envfromsource-v1-core)_ |  |
 | `imagePullPolicy` _[PullPolicy](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#pullpolicy-v1-core)_ |  |
 | `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#resourcerequirements-v1-core)_ |  |
-| `nodeSelector` _map[string]string_ |  |
+| `nodeSelector` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, all selectors are merged together. Operator node selectors are merged with any existing selectors (e.g., from ops team), with operator values taking precedence for conflicting keys. |
 
 
 #### CronJobContainerConfigs
@@ -65,7 +65,7 @@ _Appears in:_
 | `envFrom` _[EnvFromSource](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#envfromsource-v1-core)_ |  |
 | `imagePullPolicy` _[PullPolicy](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#pullpolicy-v1-core)_ |  |
 | `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#resourcerequirements-v1-core)_ |  |
-| `nodeSelector` _map[string]string_ |  |
+| `nodeSelector` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, all selectors are merged together. Operator node selectors are merged with any existing selectors (e.g., from ops team), with operator values taking precedence for conflicting keys. |
 | `commands` _string array_ | Array of commands to be executed (in order) against a Feature Store deployment.
 Defaults to "feast apply" & "feast materialize-incremental $(date -u +'%Y-%m-%dT%H:%M:%S')" |
 
@@ -568,7 +568,7 @@ _Appears in:_
 | `envFrom` _[EnvFromSource](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#envfromsource-v1-core)_ |  |
 | `imagePullPolicy` _[PullPolicy](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#pullpolicy-v1-core)_ |  |
 | `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#resourcerequirements-v1-core)_ |  |
-| `nodeSelector` _map[string]string_ |  |
+| `nodeSelector` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, all selectors are merged together. Operator node selectors are merged with any existing selectors (e.g., from ops team), with operator values taking precedence for conflicting keys. |
 
 
 #### PvcConfig
@@ -657,7 +657,7 @@ _Appears in:_
 | --- | --- |
 | `path` _string_ |  |
 | `pvc` _[PvcConfig](#pvcconfig)_ |  |
-| `s3_additional_kwargs` _map[string]string_ |  |
+| `s3_additional_kwargs` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, the last service's configuration takes precedence. |
 
 
 #### RegistryPersistence
@@ -691,7 +691,7 @@ _Appears in:_
 | `envFrom` _[EnvFromSource](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#envfromsource-v1-core)_ |  |
 | `imagePullPolicy` _[PullPolicy](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#pullpolicy-v1-core)_ |  |
 | `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#resourcerequirements-v1-core)_ |  |
-| `nodeSelector` _map[string]string_ |  |
+| `nodeSelector` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, all selectors are merged together. Operator node selectors are merged with any existing selectors (e.g., from ops team), with operator values taking precedence for conflicting keys. |
 | `tls` _[TlsConfigs](#tlsconfigs)_ |  |
 | `logLevel` _string_ | LogLevel sets the logging level for the server
 Allowed values: "debug", "info", "warning", "error", "critical". |
@@ -754,7 +754,7 @@ _Appears in:_
 | `envFrom` _[EnvFromSource](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#envfromsource-v1-core)_ |  |
 | `imagePullPolicy` _[PullPolicy](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#pullpolicy-v1-core)_ |  |
 | `resources` _[ResourceRequirements](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#resourcerequirements-v1-core)_ |  |
-| `nodeSelector` _map[string]string_ |  |
+| `nodeSelector` _map[string]string_ | NodeSelector constrains pod scheduling to nodes with matching labels. When multiple services define nodeSelector, all selectors are merged together. Operator node selectors are merged with any existing selectors (e.g., from ops team), with operator values taking precedence for conflicting keys. |
 | `tls` _[TlsConfigs](#tlsconfigs)_ |  |
 | `logLevel` _string_ | LogLevel sets the logging level for the server
 Allowed values: "debug", "info", "warning", "error", "critical". |

@@ -802,30 +802,46 @@ func (feast *FeastServices) getNodeSelectorForType(feastType FeastServiceType) *
 }
 
 func (feast *FeastServices) applyNodeSelector(podSpec *corev1.PodSpec) {
-	var selectedNodeSelector map[string]string
-	var selectedService FeastServiceType
+	// Merge node selectors from all services
+	mergedNodeSelector := make(map[string]string)
 
 	// Check all service types for node selector configuration
 	allServiceTypes := append(feastServerTypes, UIFeastType)
 	for _, feastType := range allServiceTypes {
 		if selector := feast.getNodeSelectorForType(feastType); selector != nil && len(*selector) > 0 {
-			selectedNodeSelector = *selector
-			selectedService = feastType
+			for k, v := range *selector {
+				mergedNodeSelector[k] = v
+			}
 		}
 	}
 
 	// If no service has node selector configured, we're done
-	if len(selectedNodeSelector) == 0 {
+	if len(mergedNodeSelector) == 0 {
 		return
 	}
 
-	// Apply the simple node selector to the pod spec
-	podSpec.NodeSelector = selectedNodeSelector
+	// Merge with any existing node selectors (from ops team or other sources)
+	// This preserves pre-existing selectors while adding operator requirements
+	finalNodeSelector := feast.mergeNodeSelectors(podSpec.NodeSelector, mergedNodeSelector)
+	podSpec.NodeSelector = finalNodeSelector
+}
+
+// mergeNodeSelectors merges existing and operator node selectors
+// Existing selectors are preserved, operator selectors can override existing keys
+func (feast *FeastServices) mergeNodeSelectors(existing, operator map[string]string) map[string]string {
+	merged := make(map[string]string)
+
+	// Start with existing selectors (from ops team or other sources)
+	for k, v := range existing {
+		merged[k] = v
+	}
+
+	// Add/override with operator selectors
+	for k, v := range operator {
+		merged[k] = v
+	}
 
-	// Log which service's node selector was applied for debugging
-	log.FromContext(feast.Handler.Context).V(1).Info("Applied node selector",
-		"serviceType", selectedService,
-		"selector", selectedNodeSelector)
+	return merged
 }
 
 // GetObjectMeta returns the feast k8s object metadata with type

@@ -230,7 +230,7 @@ var _ = Describe("Registry Service", func() {
 			Expect(deployment.Spec.Template.Spec.NodeSelector).To(Equal(expectedNodeSelector))
 		})
 
-		It("should apply NodeSelector from last service when multiple services have selectors", func() {
+		It("should merge NodeSelectors from multiple services", func() {
 			// Set NodeSelector for registry service
 			registryNodeSelector := map[string]string{
 				"kubernetes.io/os": "linux",
@@ -266,10 +266,53 @@ var _ = Describe("Registry Service", func() {
 			Expect(deployment).NotTo(BeNil())
 			Expect(feast.setDeployment(deployment)).To(Succeed())
 
-			// Verify NodeSelector is applied with the last service's selector (Online service wins)
+			// Verify NodeSelector merges all service selectors (online overrides registry for node-type)
 			expectedNodeSelector := map[string]string{
-				"node-type": "online",
-				"zone":      "us-west-1a",
+				"kubernetes.io/os": "linux",
+				"node-type":        "online",
+				"zone":             "us-west-1a",
+			}
+			Expect(deployment.Spec.Template.Spec.NodeSelector).To(Equal(expectedNodeSelector))
+		})
+
+		It("should merge operator NodeSelector with existing selectors (ops team scenario)", func() {
+			// Simulate existing node selector from ops team
+			existingNodeSelector := map[string]string{
+				"team":        "ml",
+				"environment": "prod",
+			}
+
+			// Set NodeSelector for UI service
+			uiNodeSelector := map[string]string{
+				"node-type": "ui",
+			}
+			featureStore.Spec.Services.UI = &feastdevv1alpha1.ServerConfigs{
+				ContainerConfigs: feastdevv1alpha1.ContainerConfigs{
+					DefaultCtrConfigs: feastdevv1alpha1.DefaultCtrConfigs{
+						Image: ptr("test-image"),
+					},
+					OptionalCtrConfigs: feastdevv1alpha1.OptionalCtrConfigs{
+						NodeSelector: &uiNodeSelector,
+					},
+				},
+			}
+
+			Expect(k8sClient.Update(ctx, featureStore)).To(Succeed())
+			Expect(feast.ApplyDefaults()).To(Succeed())
+			applySpecToStatus(featureStore)
+			feast.refreshFeatureStore(ctx, typeNamespacedName)
+
+			// Create deployment and simulate existing node selector
+			deployment := feast.initFeastDeploy()
+			deployment.Spec.Template.Spec.NodeSelector = existingNodeSelector
+			Expect(deployment).NotTo(BeNil())
+			Expect(feast.setDeployment(deployment)).To(Succeed())
+
+			// Verify NodeSelector merges existing and operator selectors
+			expectedNodeSelector := map[string]string{
+				"team":        "ml",
+				"environment": "prod",
+				"node-type":   "ui",
 			}
 			Expect(deployment.Spec.Template.Spec.NodeSelector).To(Equal(expectedNodeSelector))
 		})