From 7f6eddd9f11e2f4b186fba29692cd3aa38821046 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Wed, 13 Nov 2024 20:59:53 +0000 Subject: [PATCH 1/3] Set nodeSelector on jobs and allow empty nodeSelector Switch to a pointer for nodeSelector to allow different logic for empty vs unset --- api/v1beta1/placementapi_types.go | 2 +- api/v1beta1/zz_generated.deepcopy.go | 10 +++++++--- pkg/placement/dbsync.go | 4 ++++ pkg/placement/deployment.go | 4 ++-- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/api/v1beta1/placementapi_types.go b/api/v1beta1/placementapi_types.go index fe1fbc07..e0ac84fd 100644 --- a/api/v1beta1/placementapi_types.go +++ b/api/v1beta1/placementapi_types.go @@ -84,7 +84,7 @@ type PlacementAPISpecCore 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 index c3399e6d..71f9678b 100644 --- a/api/v1beta1/zz_generated.deepcopy.go +++ b/api/v1beta1/zz_generated.deepcopy.go @@ -165,9 +165,13 @@ func (in *PlacementAPISpecCore) DeepCopyInto(out *PlacementAPISpecCore) { 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/pkg/placement/dbsync.go b/pkg/placement/dbsync.go index dc966487..8de09fab 100644 --- a/pkg/placement/dbsync.go +++ b/pkg/placement/dbsync.go @@ -83,5 +83,9 @@ func DbSyncJob( }, } + if instance.Spec.NodeSelector != nil && len(*instance.Spec.NodeSelector) > 0 { + job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector + } + return job } diff --git a/pkg/placement/deployment.go b/pkg/placement/deployment.go index 42dbfa9d..e839b564 100644 --- a/pkg/placement/deployment.go +++ b/pkg/placement/deployment.go @@ -174,8 +174,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 && len(*instance.Spec.NodeSelector) > 0 { + deployment.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector } return deployment, nil From a740897dd4606cd9850710e3e6fb199fde27f739 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Wed, 13 Nov 2024 21:00:06 +0000 Subject: [PATCH 2/3] Add nodeSelector functional tests --- .../placementapi_controller_test.go | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/tests/functional/placementapi_controller_test.go b/tests/functional/placementapi_controller_test.go index 9c7a1685..4890da78 100644 --- a/tests/functional/placementapi_controller_test.go +++ b/tests/functional/placementapi_controller_test.go @@ -875,6 +875,105 @@ var _ = Describe("PlacementAPI controller", func() { }) }) + When("A PlacementAPI is created with nodeSelector", func() { + BeforeEach(func() { + spec := GetDefaultPlacementAPISpec() + spec["nodeSelector"] = map[string]interface{}{ + "foo": "bar", + } + + placement := CreatePlacementAPI(names.PlacementAPIName, spec) + DeferCleanup(th.DeleteInstance, placement) + + DeferCleanup(keystone.DeleteKeystoneAPI, keystone.CreateKeystoneAPI(namespace)) + DeferCleanup(k8sClient.Delete, ctx, CreatePlacementAPISecret(namespace, SecretName)) + + serviceSpec := corev1.ServiceSpec{Ports: []corev1.ServicePort{{Port: 3306}}} + DeferCleanup( + mariadb.DeleteDBService, + mariadb.CreateDBService(namespace, "openstack", serviceSpec), + ) + mariadb.SimulateMariaDBDatabaseCompleted(names.MariaDBDatabaseName) + mariadb.SimulateMariaDBAccountCompleted(names.MariaDBAccount) + + th.SimulateJobSuccess(names.DBSyncJobName) + th.SimulateDeploymentReplicaReady(names.DeploymentName) + keystone.SimulateKeystoneServiceReady(names.KeystoneServiceName) + keystone.SimulateKeystoneEndpointReady(names.KeystoneEndpointName) + DeferCleanup(th.DeleteInstance, placement) + }) + + It("sets nodeSelector in resource specs", func() { + Eventually(func(g Gomega) { + g.Expect(th.GetDeployment(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + g.Expect(th.GetJob(names.DBSyncJobName).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(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + g.Expect(th.GetJob(names.DBSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + placement := GetPlacementAPI(names.PlacementAPIName) + newNodeSelector := map[string]string{ + "foo2": "bar2", + } + placement.Spec.NodeSelector = &newNodeSelector + g.Expect(k8sClient.Update(ctx, placement)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + th.SimulateJobSuccess(names.DBSyncJobName) + th.SimulateDeploymentReplicaReady(names.DeploymentName) + g.Expect(th.GetDeployment(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo2": "bar2"})) + g.Expect(th.GetJob(names.DBSyncJobName).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(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + g.Expect(th.GetJob(names.DBSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + placement := GetPlacementAPI(names.PlacementAPIName) + emptyNodeSelector := map[string]string{} + placement.Spec.NodeSelector = &emptyNodeSelector + g.Expect(k8sClient.Update(ctx, placement)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + th.SimulateJobSuccess(names.DBSyncJobName) + th.SimulateDeploymentReplicaReady(names.DeploymentName) + g.Expect(th.GetDeployment(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(BeNil()) + g.Expect(th.GetJob(names.DBSyncJobName).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(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + g.Expect(th.GetJob(names.DBSyncJobName).Spec.Template.Spec.NodeSelector).To(Equal(map[string]string{"foo": "bar"})) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + placement := GetPlacementAPI(names.PlacementAPIName) + placement.Spec.NodeSelector = nil + g.Expect(k8sClient.Update(ctx, placement)).Should(Succeed()) + }, timeout, interval).Should(Succeed()) + + Eventually(func(g Gomega) { + th.SimulateJobSuccess(names.DBSyncJobName) + th.SimulateDeploymentReplicaReady(names.DeploymentName) + g.Expect(th.GetDeployment(names.DeploymentName).Spec.Template.Spec.NodeSelector).To(BeNil()) + g.Expect(th.GetJob(names.DBSyncJobName).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. From 2c4e26653e505742ff793c57bc92eea2c09f0b34 Mon Sep 17 00:00:00 2001 From: Oliver Walsh Date: Mon, 18 Nov 2024 16:16:31 +0000 Subject: [PATCH 3/3] Pass-through empty nodeSelector to podSpecs There is no need drop empty nodeSelector maps, omitempty already does this --- pkg/placement/dbsync.go | 2 +- pkg/placement/deployment.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/placement/dbsync.go b/pkg/placement/dbsync.go index 8de09fab..667b1fc8 100644 --- a/pkg/placement/dbsync.go +++ b/pkg/placement/dbsync.go @@ -83,7 +83,7 @@ func DbSyncJob( }, } - if instance.Spec.NodeSelector != nil && len(*instance.Spec.NodeSelector) > 0 { + if instance.Spec.NodeSelector != nil { job.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector } diff --git a/pkg/placement/deployment.go b/pkg/placement/deployment.go index e839b564..179b0017 100644 --- a/pkg/placement/deployment.go +++ b/pkg/placement/deployment.go @@ -174,7 +174,7 @@ func Deployment( }, corev1.LabelHostname, ) - if instance.Spec.NodeSelector != nil && len(*instance.Spec.NodeSelector) > 0 { + if instance.Spec.NodeSelector != nil { deployment.Spec.Template.Spec.NodeSelector = *instance.Spec.NodeSelector }