From 3e1f03da34c8ee2f2f1ac72de4d0e95cf10e9245 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Wed, 20 Nov 2024 22:14:16 +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/amphoracontroller_types.go | 2 +- api/v1beta1/octavia_types.go | 2 +- api/v1beta1/octaviaapi_types.go | 2 +- api/v1beta1/octaviarsyslog_types.go | 2 +- api/v1beta1/zz_generated.deepcopy.go | 40 +++++--- controllers/octavia_controller.go | 21 ++-- pkg/amphoracontrollers/daemonset.go | 4 +- pkg/octavia/dbsync.go | 4 + pkg/octavia/image_upload_deployment.go | 4 + pkg/octaviaapi/deployment.go | 4 +- pkg/octaviarsyslog/daemonset.go | 4 +- tests/functional/octavia_controller_test.go | 107 ++++++++++++++++++-- 12 files changed, 159 insertions(+), 37 deletions(-) diff --git a/api/v1beta1/amphoracontroller_types.go b/api/v1beta1/amphoracontroller_types.go index 5522400f..681716cc 100644 --- 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 index e7ed3219..92912217 100644 --- 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 index a4137e8e..f38b8b63 100644 --- 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 index 59e07652..7cd35f3e 100644 --- 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 index 5561710a..2eddbe60 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -162,9 +162,13 @@ func (in *OctaviaAPISpecCore) DeepCopyInto(out *OctaviaAPISpecCore) { out.PasswordSelectors = in.PasswordSelectors 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.DefaultConfigOverwrite != nil { @@ -319,9 +323,13 @@ func (in *OctaviaAmphoraControllerSpecCore) DeepCopyInto(out *OctaviaAmphoraCont out.PasswordSelectors = in.PasswordSelectors 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.DefaultConfigOverwrite != nil { @@ -591,9 +599,13 @@ func (in *OctaviaRsyslogSpecCore) DeepCopyInto(out *OctaviaRsyslogSpecCore) { *out = *in 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.DefaultConfigOverwrite != nil { @@ -717,9 +729,13 @@ func (in *OctaviaSpecBase) DeepCopyInto(out *OctaviaSpecBase) { out.PasswordSelectors = in.PasswordSelectors 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.DefaultConfigOverwrite != nil { diff --git a/controllers/octavia_controller.go b/controllers/octavia_controller.go index 03c5586f..81574ae1 100644 --- 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 index 63b9f8d6..f1ab0046 100644 --- 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 index c69c1ec2..c167b9ae 100644 --- 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 index 794f4d7f..81752635 100644 --- 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 index 0efeacbe..e8d6e078 100644 --- 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 index a311c83d..a5ede1e5 100644 --- 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 index 734224bb..cf26ced9 100644 --- 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