Set nodeSelector on jobs and allow empty nodeSelector

olliewalsh · olliewalsh · commit 09ce76cd3387 · 2024-11-22T11:12:22.000Z
Switch to a pointer for nodeSelector to allow different logic for empty vs unset
diff --git a/api/v1beta1/amphoracontroller_types.go b/api/v1beta1/amphoracontroller_types.go
@@ -89,7 +89,7 @@ type OctaviaAmphoraControllerSpecCore struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes running this service
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default="# add your customization here"
diff --git a/api/v1beta1/octavia_types.go b/api/v1beta1/octavia_types.go
@@ -145,7 +145,7 @@ type OctaviaSpecBase struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes running this service
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=false
diff --git a/api/v1beta1/octaviaapi_types.go b/api/v1beta1/octaviaapi_types.go
@@ -100,7 +100,7 @@ type OctaviaAPISpecCore struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes running this service
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=false
diff --git a/api/v1beta1/octaviarsyslog_types.go b/api/v1beta1/octaviarsyslog_types.go
@@ -45,7 +45,7 @@ type OctaviaRsyslogSpecCore struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes running this service
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// ConfigOverwrite - interface to overwrite default config files like e.g. logging.conf or policy.json.
diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/controllers/octavia_controller.go b/controllers/octavia_controller.go
@@ -1470,6 +1470,10 @@ func (r *OctaviaReconciler) apiDeploymentCreateOrUpdate(instance *octaviav1.Octa
 		},
 	}
 
+	if instance.Spec.OctaviaAPI.NodeSelector == nil {
+		instance.Spec.OctaviaAPI.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	op, err := controllerutil.CreateOrUpdate(context.TODO(), r.Client, deployment, func() error {
 		deployment.Spec = instance.Spec.OctaviaAPI
 		deployment.Spec.DatabaseInstance = instance.Spec.DatabaseInstance
@@ -1483,9 +1487,6 @@ func (r *OctaviaReconciler) apiDeploymentCreateOrUpdate(instance *octaviav1.Octa
 		deployment.Spec.TLS = instance.Spec.OctaviaAPI.TLS
 		deployment.Spec.APITimeout = instance.Spec.APITimeout
 
-		if len(deployment.Spec.NodeSelector) == 0 {
-			deployment.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		err := controllerutil.SetControllerReference(instance, deployment, r.Scheme)
 		if err != nil {
 			return err
@@ -1531,6 +1532,10 @@ func (r *OctaviaReconciler) amphoraControllerDaemonSetCreateOrUpdate(
 		},
 	}
 
+	if controllerSpec.NodeSelector == nil {
+		controllerSpec.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	op, err := controllerutil.CreateOrUpdate(context.TODO(), r.Client, daemonset, func() error {
 		daemonset.Spec = controllerSpec
 		daemonset.Spec.Role = role
@@ -1550,9 +1555,6 @@ func (r *OctaviaReconciler) amphoraControllerDaemonSetCreateOrUpdate(
 		daemonset.Spec.OctaviaProviderSubnetGateway = networkInfo.ManagementSubnetGateway
 		daemonset.Spec.OctaviaProviderSubnetCIDR = networkInfo.ManagementSubnetCIDR
 		daemonset.Spec.OctaviaProviderSubnetExtraCIDRs = networkInfo.ManagementSubnetExtraCIDRs
-		if len(daemonset.Spec.NodeSelector) == 0 {
-			daemonset.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		err := controllerutil.SetControllerReference(instance, daemonset, r.Scheme)
 		if err != nil {
 			return err
@@ -1612,13 +1614,14 @@ func (r *OctaviaReconciler) octaviaRsyslogDaemonSetCreateOrUpdate(
 		},
 	}
 
+	if controllerSpec.NodeSelector == nil {
+		controllerSpec.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	op, err := controllerutil.CreateOrUpdate(context.TODO(), r.Client, daemonset, func() error {
 		daemonset.Spec = controllerSpec
 		daemonset.Spec.ServiceUser = instance.Spec.ServiceUser
 		daemonset.Spec.ServiceAccount = instance.RbacResourceName()
-		if len(daemonset.Spec.NodeSelector) == 0 {
-			daemonset.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		err := controllerutil.SetControllerReference(instance, daemonset, r.Scheme)
 		if err != nil {
 			return err
diff --git a/pkg/amphoracontrollers/daemonset.go b/pkg/amphoracontrollers/daemonset.go
@@ -163,8 +163,8 @@ func DaemonSet(
 		},
 		corev1.LabelHostname,
 	)
-	if instance.Spec.NodeSelector != nil && len(instance.Spec.NodeSelector) > 0 {
-		daemonset.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		daemonset.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
 	initContainerDetails := octavia.APIDetails{
diff --git a/pkg/octavia/dbsync.go b/pkg/octavia/dbsync.go
@@ -91,6 +91,9 @@ func DbSyncJob(
 		VolumeMounts:   initVolumeMounts,
 	}
 	job.Spec.Template.Spec.InitContainers = InitContainer(initContainerDetails)
+	if instance.Spec.NodeSelector != nil {
+		job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
+	}
 
 	return job
 }
diff --git a/pkg/octavia/image_upload_deployment.go b/pkg/octavia/image_upload_deployment.go
@@ -133,6 +133,10 @@ func ImageUploadDeployment(
 	}
 	depl.Spec.Template.Spec.InitContainers = initContainer(initContainerDetails)
 
+	if instance.Spec.NodeSelector != nil {
+		depl.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
+	}
+
 	return depl
 }
 
diff --git a/pkg/octaviaapi/deployment.go b/pkg/octaviaapi/deployment.go
@@ -192,8 +192,8 @@ func Deployment(
 		},
 		corev1.LabelHostname,
 	)
-	if instance.Spec.NodeSelector != nil && len(instance.Spec.NodeSelector) > 0 {
-		deployment.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		deployment.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
 	initContainerDetails := octavia.APIDetails{
diff --git a/pkg/octaviarsyslog/daemonset.go b/pkg/octaviarsyslog/daemonset.go
@@ -127,8 +127,8 @@ func DaemonSet(
 		},
 		corev1.LabelHostname,
 	)
-	if instance.Spec.NodeSelector != nil && len(instance.Spec.NodeSelector) > 0 {
-		daemonset.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		daemonset.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
 	initContainerDetails := APIDetails{
diff --git a/tests/functional/octavia_controller_test.go b/tests/functional/octavia_controller_test.go
@@ -109,6 +109,7 @@ var _ = Describe("Octavia controller", func() {
 	var octaviaName types.NamespacedName
 	var transportURLName types.NamespacedName
 	var transportURLSecretName types.NamespacedName
+	var octaviaDBSyncName types.NamespacedName
 
 	BeforeEach(func() {
 		name = fmt.Sprintf("octavia-%s", uuid.New().String())
@@ -128,6 +129,11 @@ var _ = Describe("Octavia controller", func() {
 			Namespace: namespace,
 			Name:      RabbitmqSecretName,
 		}
+
+		octaviaDBSyncName = types.NamespacedName{
+			Namespace: namespace,
+			Name:      octaviaName.Name + "-db-sync",
+		}
 	})
 
 	When("an Octavia instance is created", func() {
@@ -337,7 +343,7 @@ var _ = Describe("Octavia controller", func() {
 			mariadb.SimulateMariaDBDatabaseCompleted(types.NamespacedName{Namespace: namespace, Name: octavia.DatabaseCRName})
 			mariadb.SimulateMariaDBAccountCompleted(types.NamespacedName{Namespace: namespace, Name: GetOctavia(octaviaName).Spec.PersistenceDatabaseAccount})
 			mariadb.SimulateMariaDBDatabaseCompleted(types.NamespacedName{Namespace: namespace, Name: octavia.PersistenceDatabaseCRName})
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 			octavia := GetOctavia(octaviaName)
 			hostname := "hostname-for-" + octavia.Spec.DatabaseInstance + "." + namespace + ".svc"
 			Expect(octavia.Status.DatabaseHostname).To(Equal(hostname))
@@ -369,7 +375,7 @@ var _ = Describe("Octavia controller", func() {
 
 			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
 
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 		})
 
 		It("should set Service Config Ready Condition", func() {
@@ -499,7 +505,7 @@ var _ = Describe("Octavia controller", func() {
 
 			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
 
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 		})
 
 		It("should create appropriate resources in Neutron", func() {
@@ -662,7 +668,7 @@ var _ = Describe("Octavia controller", func() {
 			spec["lbMgmtNetwork"].(map[string]interface{})["createDefaultLbMgmtNetwork"] = false
 			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
 
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 		})
 
 		It("should create appropriate resources in Neutron", func() {
@@ -820,6 +826,95 @@ var _ = Describe("Octavia controller", func() {
 		})
 	})
 
+	When("Ocatavia is created with nodeSelector", func() {
+		BeforeEach(func() {
+			spec["nodeSelector"] = map[string]interface{}{
+				"foo": "bar",
+			}
+
+			createAndSimulateKeystone(octaviaName)
+
+			createAndSimulateOctaviaSecrets(octaviaName)
+			createAndSimulateTransportURL(transportURLName, transportURLSecretName)
+
+			createAndSimulateDB(spec)
+
+			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
+
+			th.SimulateJobSuccess(octaviaDBSyncName)
+
+			// TODO: assert nodeSelector on more resources when supported
+		})
+
+		It("sets nodeSelector in resource specs", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+		})
+
+		It("updates nodeSelector in resource specs when changed", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				octavia := GetOctavia(octaviaName)
+				newNodeSelector := map[string]string{
+					"foo2": "bar2",
+				}
+				octavia.Spec.NodeSelector = &newNodeSelector
+				g.Expect(k8sClient.Update(ctx, octavia)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(octaviaDBSyncName)
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo2": "bar2"}))
+			}, timeout, interval).Should(Succeed())
+		})
+
+		It("removes nodeSelector from resource specs when cleared", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				octavia := GetOctavia(octaviaName)
+				emptyNodeSelector := map[string]string{}
+				octavia.Spec.NodeSelector = &emptyNodeSelector
+				g.Expect(k8sClient.Update(ctx, octavia)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(octaviaDBSyncName)
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(BeNil())
+			}, timeout, interval).Should(Succeed())
+		})
+
+		It("removes nodeSelector from resource specs when nilled", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				octavia := GetOctavia(octaviaName)
+				octavia.Spec.NodeSelector = nil
+				g.Expect(k8sClient.Update(ctx, octavia)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(octaviaDBSyncName)
+				g.Expect(th.GetJob(octaviaDBSyncName).Spec.Template.Spec.NodeSelector).To(BeNil())
+			}, timeout, interval).Should(Succeed())
+		})
+
+		// It("allows nodeSelector service override", func() {
+		// })
+
+		// It("allows nodeSelector service override to empty", func() {
+		// })
+
+	})
+
 	// Predictable IPs
 
 	// Amphora Controller Daemonsets
@@ -852,7 +947,7 @@ var _ = Describe("Octavia controller", func() {
 
 			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
 
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 		})
 
 		It("should set OctaviaAmphoraSSHReady condition", func() {
@@ -913,7 +1008,7 @@ var _ = Describe("Octavia controller", func() {
 
 			DeferCleanup(th.DeleteInstance, CreateOctavia(octaviaName, spec))
 
-			th.SimulateJobSuccess(types.NamespacedName{Namespace: namespace, Name: octaviaName.Name + "-db-sync"})
+			th.SimulateJobSuccess(octaviaDBSyncName)
 		})
 
 		// PENDING https://issues.redhat.com/browse/OSPRH-10543

Original file line number	Diff line number	Diff line change
`@@ -163,8 +163,8 @@ func DaemonSet(`
`163`	`163`	`},`
`164`	`164`	`corev1.LabelHostname,`
`165`	`165`	`)`
`166`		`- if instance.Spec.NodeSelector != nil && len(instance.Spec.NodeSelector) > 0 {`
`167`		`- daemonset.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector`
	`166`	`+ if instance.Spec.NodeSelector != nil {`
	`167`	`+ daemonset.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
`168`	`168`	`}`
`169`	`169`
`170`	`170`	`initContainerDetails := octavia.APIDetails{`
Original file line number	Diff line number	Diff line change
`@@ -91,6 +91,9 @@ func DbSyncJob(`
`91`	`91`	`VolumeMounts: initVolumeMounts,`
`92`	`92`	`}`
`93`	`93`	`job.Spec.Template.Spec.InitContainers = InitContainer(initContainerDetails)`
	`94`	`+ if instance.Spec.NodeSelector != nil {`
	`95`	`+ job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
	`96`	`+ }`
`94`	`97`
`95`	`98`	`return job`
`96`	`99`	`}`
Original file line number	Diff line number	Diff line change
`@@ -133,6 +133,10 @@ func ImageUploadDeployment(`
`133`	`133`	`}`
`134`	`134`	`depl.Spec.Template.Spec.InitContainers = initContainer(initContainerDetails)`
`135`	`135`
	`136`	`+ if instance.Spec.NodeSelector != nil {`
	`137`	`+ depl.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
	`138`	`+ }`
	`139`	`+`
`136`	`140`	`return depl`
`137`	`141`	`}`
`138`	`142`
Original file line number	Diff line number	Diff line change
`@@ -192,8 +192,8 @@ func Deployment(`
`192`	`192`	`},`
`193`	`193`	`corev1.LabelHostname,`
`194`	`194`	`)`
`195`		`- if instance.Spec.NodeSelector != nil && len(instance.Spec.NodeSelector) > 0 {`
`196`		`- deployment.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector`
	`195`	`+ if instance.Spec.NodeSelector != nil {`
	`196`	`+ deployment.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
`197`	`197`	`}`
`198`	`198`
`199`	`199`	`initContainerDetails := octavia.APIDetails{`