Merge pull request #502 from olliewalsh/node_selectors

Ensure nodeSelector logic is consistent for all operators
openstack-k8s-operators · Nov 19, 2024 · 18b7410 · 18b7410
2 parents 6d9f1d4 + 1a349e3
commit 18b7410
Show file tree

Hide file tree

Showing 7 changed files with 150 additions and 18 deletions.
diff --git a/api/v1beta1/keystoneapi_types.go b/api/v1beta1/keystoneapi_types.go
@@ -138,7 +138,7 @@ type KeystoneAPISpecCore 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/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/pkg/keystone/bootstrap.go b/pkg/keystone/bootstrap.go
@@ -117,8 +117,8 @@ func BootstrapJob(
 	}
 	job.Spec.Template.Spec.Containers[0].Env = env.MergeEnvs(job.Spec.Template.Spec.Containers[0].Env, envVars)
 
-	if len(instance.Spec.NodeSelector) > 0 {
-		job.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
 	return job

diff --git a/pkg/keystone/cronjob.go b/pkg/keystone/cronjob.go
@@ -98,9 +98,8 @@ func CronJob(
 			},
 		},
 	}
-	if len(instance.Spec.NodeSelector) > 0 {
-		cronjob.Spec.JobTemplate.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		cronjob.Spec.JobTemplate.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
-
 	return cronjob
 }
diff --git a/pkg/keystone/dbsync.go b/pkg/keystone/dbsync.go
@@ -90,8 +90,8 @@ func DbSyncJob(
 		},
 	}
 
-	if len(instance.Spec.NodeSelector) > 0 {
-		job.Spec.Template.Spec.NodeSelector = instance.Spec.NodeSelector
+	if instance.Spec.NodeSelector != nil {
+		job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
 	}
 
 	return job

diff --git a/pkg/keystone/deployment.go b/pkg/keystone/deployment.go
@@ -158,8 +158,9 @@ 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/tests/functional/keystoneapi_controller_test.go b/tests/functional/keystoneapi_controller_test.go
@@ -51,6 +51,7 @@ var _ = Describe("Keystone controller", func() {
 	var internalCertSecretName types.NamespacedName
 	var publicCertSecretName types.NamespacedName
 	var memcachedSpec memcachedv1.MemcachedSpec
+	var cronJobName types.NamespacedName
 
 	BeforeEach(func() {
 
@@ -99,6 +100,10 @@ var _ = Describe("Keystone controller", func() {
 				Replicas: ptr.To(int32(3)),
 			},
 		}
+		cronJobName = types.NamespacedName{
+			Namespace: keystoneAPIName.Namespace,
+			Name:      "keystone-cron",
+		}
 
 		err := os.Setenv("OPERATOR_TEMPLATES", "../../templates")
 		Expect(err).NotTo(HaveOccurred())
@@ -609,11 +614,7 @@ var _ = Describe("Keystone controller", func() {
 		})
 
 		It("should create a CronJob for trust flush", func() {
-			cronJob := types.NamespacedName{
-				Namespace: keystoneAPIName.Namespace,
-				Name:      fmt.Sprintf("%s-%s", keystoneAPIName.Name, "cron"),
-			}
-			GetCronJob(cronJob)
+			GetCronJob(cronJobName)
 		})
 
 		It("should create a ConfigMap and Secret for client config", func() {
@@ -1189,6 +1190,133 @@ var _ = Describe("Keystone controller", func() {
 		})
 	})
 
+	When("A KeystoneAPI is created with nodeSelector", func() {
+		BeforeEach(func() {
+			spec := GetDefaultKeystoneAPISpec()
+			spec["nodeSelector"] = map[string]interface{}{
+				"foo": "bar",
+			}
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateKeystoneMessageBusSecret(namespace, "rabbitmq-secret"))
+			keystone := CreateKeystoneAPI(keystoneAPIName, spec)
+			DeferCleanup(th.DeleteInstance, keystone)
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateKeystoneAPISecret(namespace, SecretName))
+			DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec))
+			DeferCleanup(
+				mariadb.DeleteDBService,
+				mariadb.CreateDBService(
+					namespace,
+					GetKeystoneAPI(keystoneAPIName).Spec.DatabaseInstance,
+					corev1.ServiceSpec{
+						Ports: []corev1.ServicePort{{Port: 3306}},
+					},
+				),
+			)
+			mariadb.SimulateMariaDBAccountCompleted(keystoneAccountName)
+			mariadb.SimulateMariaDBDatabaseCompleted(keystoneDatabaseName)
+			infra.SimulateTransportURLReady(types.NamespacedName{
+				Name:      fmt.Sprintf("%s-keystone-transport", keystoneAPIName.Name),
+				Namespace: namespace,
+			})
+			infra.SimulateMemcachedReady(types.NamespacedName{
+				Name:      "memcached",
+				Namespace: namespace,
+			})
+			th.SimulateJobSuccess(dbSyncJobName)
+			th.SimulateJobSuccess(bootstrapJobName)
+			th.SimulateDeploymentReplicaReady(deploymentName)
+		})
+
+		It("sets nodeSelector in resource specs", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.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.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				keystone := GetKeystoneAPI(keystoneAPIName)
+				newNodeSelector := map[string]string{
+					"foo2": "bar2",
+				}
+				keystone.Spec.NodeSelector = &newNodeSelector
+				g.Expect(k8sClient.Update(ctx, keystone)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(dbSyncJobName)
+				th.SimulateJobSuccess(bootstrapJobName)
+				th.SimulateDeploymentReplicaReady(deploymentName)
+				g.Expect(th.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo2": "bar2"}))
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo2": "bar2"}))
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo2": "bar2"}))
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.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.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				keystone := GetKeystoneAPI(keystoneAPIName)
+				emptyNodeSelector := map[string]string{}
+				keystone.Spec.NodeSelector = &emptyNodeSelector
+				g.Expect(k8sClient.Update(ctx, keystone)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(dbSyncJobName)
+				th.SimulateJobSuccess(bootstrapJobName)
+				th.SimulateDeploymentReplicaReady(deploymentName)
+				g.Expect(th.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.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.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				keystone := GetKeystoneAPI(keystoneAPIName)
+				keystone.Spec.NodeSelector = nil
+				g.Expect(k8sClient.Update(ctx, keystone)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				th.SimulateJobSuccess(dbSyncJobName)
+				th.SimulateJobSuccess(bootstrapJobName)
+				th.SimulateDeploymentReplicaReady(deploymentName)
+				g.Expect(th.GetDeployment(deploymentName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(th.GetJob(bootstrapJobName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(th.GetJob(dbSyncJobName).Spec.Template.Spec.NodeSelector).To(BeNil())
+				g.Expect(GetCronJob(cronJobName).Spec.JobTemplate.Spec.Template.Spec.NodeSelector).To(BeNil())
+			}, timeout, interval).Should(Succeed())
+		})
+	})
+
 	// Run MariaDBAccount suite tests.  these are pre-packaged ginkgo tests
 	// that exercise standard account create / update patterns that should be
 	// common to all controllers that ensure MariaDBAccount CRs.