Merge pull request #417 from olliewalsh/node_selectors

openshift-merge-bot[bot] · web-flow · commit bc64ab6db016 · 2024-11-21T09:39:35.000Z
Ensure nodeSelector logic is consistent for all operators
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
@@ -1476,6 +1476,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
@@ -1489,9 +1493,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
@@ -1537,6 +1538,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
@@ -1556,9 +1561,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
@@ -1618,13 +1620,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
@@ -193,8 +193,8 @@ func DaemonSet(
 		},
 		corev1.LabelHostname,
 	)
-	if 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
 	}
 
 	return daemonset
diff --git a/pkg/octavia/dbsync.go b/pkg/octavia/dbsync.go
@@ -97,5 +97,9 @@ func DbSyncJob(
 		},
 	}
 
+	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
@@ -212,8 +212,8 @@ func Deployment(
 		},
 		corev1.LabelHostname,
 	)
-	if 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
 	}
 
 	return deployment, nil
diff --git a/pkg/octaviarsyslog/daemonset.go b/pkg/octaviarsyslog/daemonset.go
@@ -152,8 +152,8 @@ func DaemonSet(
 		},
 		corev1.LabelHostname,
 	)
-	if 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
 	}
 
 	return daemonset
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
`@@ -193,8 +193,8 @@ func DaemonSet(`
`193`	`193`	`},`
`194`	`194`	`corev1.LabelHostname,`
`195`	`195`	`)`
`196`		`- if len(instance.Spec.NodeSelector) > 0 {`
`197`		`- daemonset.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector`
	`196`	`+ if instance.Spec.NodeSelector != nil {`
	`197`	`+ daemonset.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
`198`	`198`	`}`
`199`	`199`
`200`	`200`	`return daemonset`
Original file line number	Diff line number	Diff line change
`@@ -97,5 +97,9 @@ func DbSyncJob(`
`97`	`97`	`},`
`98`	`98`	`}`
`99`	`99`
	`100`	`+ if instance.Spec.NodeSelector != nil {`
	`101`	`+ job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
	`102`	`+ }`
	`103`	`+`
`100`	`104`	`return job`
`101`	`105`	`}`
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
`@@ -212,8 +212,8 @@ func Deployment(`
`212`	`212`	`},`
`213`	`213`	`corev1.LabelHostname,`
`214`	`214`	`)`
`215`		`- if len(instance.Spec.NodeSelector) > 0 {`
`216`		`- deployment.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector`
	`215`	`+ if instance.Spec.NodeSelector != nil {`
	`216`	`+ deployment.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector`
`217`	`217`	`}`
`218`	`218`
`219`	`219`	`return deployment, nil`