From fdeb8f18a743966aad1239dd7b22d8a6655d2b6a Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Tue, 12 Nov 2024 22:29:52 +0000 Subject: [PATCH] Set nodeSelector on jobs and allow empty nodeSelector Switch to a pointer for nodeSelector to allow different logic for empty vs unset --- api/v1beta1/common_types.go | 2 +- api/v1beta1/heat_types.go | 2 +- api/v1beta1/zz_generated.deepcopy.go | 20 +++-- controllers/heat_controller.go | 21 +++-- pkg/heat/dbsync.go | 4 + pkg/heatapi/deployment.go | 4 +- pkg/heatcfnapi/deployment.go | 4 +- pkg/heatengine/deployment.go | 4 +- tests/functional/heat_controller_test.go | 104 ++++++++++++++++++++++- 9 files changed, 141 insertions(+), 24 deletions(-) diff --git a/api/v1beta1/common_types.go b/api/v1beta1/common_types.go index 0ba386fa..2acb4e8d 100644 --- a/api/v1beta1/common_types.go +++ b/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, diff --git a/api/v1beta1/heat_types.go b/api/v1beta1/heat_types.go index 5da2c184..9ed445f7 100644 --- a/api/v1beta1/heat_types.go +++ b/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 diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go index 405219fa..f48cf861 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -556,9 +556,13 @@ func (in *HeatServiceTemplate) DeepCopyInto(out *HeatServiceTemplate) { } if in.NodeSelector != nil { in, out := &in.NodeSelector, &out.NodeSelector - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val + *out = new(map[string]string) + if **in != nil { + in, out := *in, *out + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } } } if in.CustomServiceConfigSecrets != nil { @@ -615,9 +619,13 @@ func (in *HeatSpecBase) DeepCopyInto(out *HeatSpecBase) { } if in.NodeSelector != nil { in, out := &in.NodeSelector, &out.NodeSelector - *out = make(map[string]string, len(*in)) - for key, val := range *in { - (*out)[key] = val + *out = new(map[string]string) + if **in != nil { + in, out := *in, *out + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } } } out.HeatTemplate = in.HeatTemplate diff --git a/controllers/heat_controller.go b/controllers/heat_controller.go index 2b66b0db..7d2ddf3e 100644 --- a/controllers/heat_controller.go +++ b/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) }) diff --git a/pkg/heat/dbsync.go b/pkg/heat/dbsync.go index 0f2cf7e0..d79a03cf 100644 --- a/pkg/heat/dbsync.go +++ b/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 } diff --git a/pkg/heatapi/deployment.go b/pkg/heatapi/deployment.go index 7827ea24..0688c65f 100644 --- a/pkg/heatapi/deployment.go +++ b/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 diff --git a/pkg/heatcfnapi/deployment.go b/pkg/heatcfnapi/deployment.go index c758dd12..c293b8fb 100644 --- a/pkg/heatcfnapi/deployment.go +++ b/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 diff --git a/pkg/heatengine/deployment.go b/pkg/heatengine/deployment.go index 9e6b6372..20b328b0 100644 --- a/pkg/heatengine/deployment.go +++ b/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 diff --git a/tests/functional/heat_controller_test.go b/tests/functional/heat_controller_test.go index 6692eea3..ac1816f4 100644 --- a/tests/functional/heat_controller_test.go +++ b/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.