Merge pull request #473 from olliewalsh/node_selectors

Ensure nodeSelector logic is consistent for all operators

api/v1beta1/common_types.go

@@ -61,7 +61,7 @@ type HeatServiceTemplate struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes for running the service
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// CustomServiceConfig - customize the service config using this parameter to change service defaults,

api/v1beta1/heat_types.go

@@ -119,7 +119,7 @@ type HeatSpecBase struct {
 
 	// +kubebuilder:validation:Optional
 	// NodeSelector to target subset of worker nodes for running the Heat services
-	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
+	NodeSelector *map[string]string `json:"nodeSelector,omitempty"`
 
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=600

controllers/heat_controller.go

@@ -835,11 +835,12 @@ func (r *HeatReconciler) apiDeploymentCreateOrUpdate(
 		},
 	}
 
+	if heatAPISpec.NodeSelector == nil {
+		heatAPISpec.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, deployment, func() error {
 		deployment.Spec = heatAPISpec
-		if len(deployment.Spec.NodeSelector) == 0 {
-			deployment.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		return controllerutil.SetControllerReference(instance, deployment, r.Scheme)
 	})
 
@@ -858,6 +859,10 @@ func (r *HeatReconciler) cfnapiDeploymentCreateOrUpdate(
 		ServiceAccount:     instance.RbacResourceName(),
 	}
 
+	if heatCfnAPISpec.NodeSelector == nil {
+		heatCfnAPISpec.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	deployment := &heatv1beta1.HeatCfnAPI{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("%s-cfnapi", instance.Name),
@@ -867,9 +872,6 @@ func (r *HeatReconciler) cfnapiDeploymentCreateOrUpdate(
 
 	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, deployment, func() error {
 		deployment.Spec = heatCfnAPISpec
-		if len(deployment.Spec.NodeSelector) == 0 {
-			deployment.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		return controllerutil.SetControllerReference(instance, deployment, r.Scheme)
 	})
 
@@ -889,6 +891,10 @@ func (r *HeatReconciler) engineDeploymentCreateOrUpdate(
 		TLS:                instance.Spec.HeatAPI.TLS.Ca,
 	}
 
+	if heatEngineSpec.NodeSelector == nil {
+		heatEngineSpec.NodeSelector = instance.Spec.NodeSelector
+	}
+
 	deployment := &heatv1beta1.HeatEngine{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("%s-engine", instance.Name),
@@ -898,9 +904,6 @@ func (r *HeatReconciler) engineDeploymentCreateOrUpdate(
 
 	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, deployment, func() error {
 		deployment.Spec = heatEngineSpec
-		if len(deployment.Spec.NodeSelector) == 0 {
-			deployment.Spec.NodeSelector = instance.Spec.NodeSelector
-		}
 		return controllerutil.SetControllerReference(instance, deployment, r.Scheme)
 	})
 

pkg/heat/dbsync.go

@@ -82,5 +82,9 @@ func DBSyncJob(
 		},
 	}
 
+	if instance.Spec.NodeSelector != nil {
+		job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector
+	}
+
 	return job
 }

pkg/heatapi/deployment.go

@@ -169,8 +169,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
 	}
 
 	return deployment, nil

pkg/heatcfnapi/deployment.go

@@ -169,8 +169,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
 	}
 
 	return deployment, nil

pkg/heatengine/deployment.go

@@ -139,8 +139,8 @@ func Deployment(instance *heatv1beta1.HeatEngine, configHash string, labels map[
 		},
 		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
 	}
 
 	return deployment

tests/functional/heat_controller_test.go

@@ -46,6 +46,7 @@ var _ = Describe("Heat controller", func() {
 	var heatConfigSecretName types.NamespacedName
 	var memcachedSpec memcachedv1.MemcachedSpec
 	var keystoneAPI *keystonev1.KeystoneAPI
+	var heatDbSyncName types.NamespacedName
 
 	BeforeEach(func() {
 
@@ -66,7 +67,10 @@ var _ = Describe("Heat controller", func() {
 				Replicas: ptr.To[int32](3),
 			},
 		}
-
+		heatDbSyncName = types.NamespacedName{
+			Name:      "heat-db-sync",
+			Namespace: namespace,
+		}
 		err := os.Setenv("OPERATOR_TEMPLATES", "../../templates")
 		Expect(err).NotTo(HaveOccurred())
 	})
@@ -542,6 +546,104 @@ var _ = Describe("Heat controller", func() {
 		})
 	})
 
+	When("heatAPI is created with nodeSelector", func() {
+		BeforeEach(func() {
+			spec := GetDefaultHeatSpec()
+			spec["nodeSelector"] = map[string]interface{}{
+				"foo": "bar",
+			}
+			heatAPI := GetDefaultHeatAPISpec()
+			spec["heatAPI"] = heatAPI
+			DeferCleanup(th.DeleteInstance, CreateHeat(heatName, spec))
+
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateHeatSecret(namespace, SecretName))
+			DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec))
+			infra.SimulateMemcachedReady(types.NamespacedName{
+				Name:      "memcached",
+				Namespace: namespace,
+			})
+			DeferCleanup(
+				k8sClient.Delete, ctx, CreateHeatMessageBusSecret(namespace, HeatMessageBusSecretName))
+			infra.SimulateTransportURLReady(heatTransportURLName)
+			keystoneAPIName := keystone.CreateKeystoneAPI(namespace)
+			keystoneAPI = keystone.GetKeystoneAPI(keystoneAPIName)
+			DeferCleanup(keystone.DeleteKeystoneAPI, keystoneAPIName)
+			DeferCleanup(
+				mariadb.DeleteDBService,
+				mariadb.CreateDBService(
+					namespace,
+					GetHeat(heatName).Spec.DatabaseInstance,
+					corev1.ServiceSpec{
+						Ports: []corev1.ServicePort{{Port: 3306}},
+					},
+				),
+			)
+			mariadb.SimulateMariaDBAccountCompleted(types.NamespacedName{Namespace: namespace, Name: GetHeat(heatName).Spec.DatabaseAccount})
+			mariadb.SimulateMariaDBDatabaseCompleted(types.NamespacedName{Namespace: namespace, Name: heat.DatabaseCRName})
+			th.SimulateJobSuccess(heatDbSyncName)
+			// TODO: assert deployment once it's supported in the tests
+		})
+
+		It("sets nodeSelector in resource specs", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(heatDbSyncName).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(heatDbSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				heat := GetHeat(heatName)
+				newNodeSelector := map[string]string{
+					"foo2": "bar2",
+				}
+				heat.Spec.NodeSelector = &newNodeSelector
+				g.Expect(k8sClient.Update(ctx, heat)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(heatDbSyncName).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(heatDbSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				heat := GetHeat(heatName)
+				emptyNodeSelector := map[string]string{}
+				heat.Spec.NodeSelector = &emptyNodeSelector
+				g.Expect(k8sClient.Update(ctx, heat)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(heatDbSyncName).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(heatDbSyncName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"}))
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				heat := GetHeat(heatName)
+				heat.Spec.NodeSelector = nil
+				g.Expect(k8sClient.Update(ctx, heat)).Should(Succeed())
+			}, timeout, interval).Should(Succeed())
+
+			Eventually(func(g Gomega) {
+				g.Expect(th.GetJob(heatDbSyncName).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.