From 6dc673f6cea82626aa394565b5cdeb1aa4b35968 Mon Sep 17 00:00:00 2001 From: Mykhailo Bobrovskyi Date: Tue, 26 Nov 2024 11:46:59 +0200 Subject: [PATCH] Allow to delete queue-name on deployment. --- .../jobs/deployment/deployment_webhook.go | 16 +++++++---- .../deployment/deployment_webhook_test.go | 20 +++++++++++-- pkg/util/testingjobs/deployment/wrappers.go | 12 ++++++-- test/e2e/singlecluster/deployment_test.go | 2 +- .../webhook/jobs/deployment_webhook_test.go | 28 ++++++++++++++++++- 5 files changed, 65 insertions(+), 13 deletions(-) diff --git a/pkg/controller/jobs/deployment/deployment_webhook.go b/pkg/controller/jobs/deployment/deployment_webhook.go index 4489d0b3d9..8d3409185b 100644 --- a/pkg/controller/jobs/deployment/deployment_webhook.go +++ b/pkg/controller/jobs/deployment/deployment_webhook.go @@ -27,7 +27,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" - "sigs.k8s.io/kueue/pkg/controller/constants" + "sigs.k8s.io/kueue/pkg/constants" + controllerconsts "sigs.k8s.io/kueue/pkg/controller/constants" "sigs.k8s.io/kueue/pkg/controller/jobframework" "sigs.k8s.io/kueue/pkg/controller/jobframework/webhook" ) @@ -62,7 +63,11 @@ func (wh *Webhook) Default(ctx context.Context, obj runtime.Object) error { if deployment.Spec.Template.Labels == nil { deployment.Spec.Template.Labels = make(map[string]string, 1) } - deployment.Spec.Template.Labels[constants.QueueLabel] = queueName + deployment.Labels[constants.ManagedByKueueLabel] = "true" + deployment.Spec.Template.Labels[controllerconsts.QueueLabel] = queueName + } else if deployment.Labels[constants.ManagedByKueueLabel] == "true" { + delete(deployment.Labels, constants.ManagedByKueueLabel) + delete(deployment.Spec.Template.Labels, controllerconsts.QueueLabel) } return nil @@ -85,7 +90,7 @@ func (wh *Webhook) ValidateCreate(ctx context.Context, obj runtime.Object) (warn var ( labelsPath = field.NewPath("metadata", "labels") - queueNameLabelPath = labelsPath.Key(constants.QueueLabel) + queueNameLabelPath = labelsPath.Key(controllerconsts.QueueLabel) ) func (wh *Webhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (warnings admission.Warnings, err error) { @@ -101,9 +106,8 @@ func (wh *Webhook) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Ob allErrs := field.ErrorList{} allErrs = append(allErrs, jobframework.ValidateQueueName(newDeployment.Object())...) - // Prevents updating the queue-name if at least one Pod is not suspended - // or if the queue-name has been deleted. - if oldDeployment.Status.ReadyReplicas > 0 || newQueueName == "" { + // Prevents updating the queue-name if at least one Pod is not suspended. + if oldDeployment.Status.ReadyReplicas > 0 { allErrs = append(allErrs, apivalidation.ValidateImmutableField(oldQueueName, newQueueName, queueNameLabelPath)...) } diff --git a/pkg/controller/jobs/deployment/deployment_webhook_test.go b/pkg/controller/jobs/deployment/deployment_webhook_test.go index e2371ba0e7..10f561514b 100644 --- a/pkg/controller/jobs/deployment/deployment_webhook_test.go +++ b/pkg/controller/jobs/deployment/deployment_webhook_test.go @@ -45,6 +45,7 @@ func TestDefault(t *testing.T) { Obj(), want: testingdeployment.MakeDeployment("test-pod", ""). Queue("test-queue"). + ManagedByKueue(). PodTemplateSpecQueue("test-queue"). Obj(), }, @@ -55,6 +56,7 @@ func TestDefault(t *testing.T) { Obj(), want: testingdeployment.MakeDeployment("test-pod", ""). Queue("new-test-queue"). + ManagedByKueue(). PodTemplateSpecQueue("new-test-queue"). Obj(), }, @@ -62,6 +64,13 @@ func TestDefault(t *testing.T) { deployment: testingdeployment.MakeDeployment("test-pod", "").PodTemplateSpecQueue("test-queue").Obj(), want: testingdeployment.MakeDeployment("test-pod", "").PodTemplateSpecQueue("test-queue").Obj(), }, + "deployment without queue with pod template spec queue and managed by kueue label": { + deployment: testingdeployment.MakeDeployment("test-pod", ""). + ManagedByKueue(). + PodTemplateSpecQueue("test-queue"). + Obj(), + want: testingdeployment.MakeDeployment("test-pod", "").Obj(), + }, } for name, tc := range testCases { @@ -79,7 +88,7 @@ func TestDefault(t *testing.T) { if err := w.Default(ctx, tc.deployment); err != nil { t.Errorf("failed to set defaults for v1/deployment: %s", err) } - if diff := cmp.Diff(tc.want, tc.deployment); len(diff) != 0 { + if diff := cmp.Diff(tc.want, tc.deployment, cmpopts.EquateEmpty()); len(diff) != 0 { t.Errorf("Default() mismatch (-want,+got):\n%s", diff) } }) @@ -146,11 +155,18 @@ func TestValidateUpdate(t *testing.T) { oldDeployment: testingdeployment.MakeDeployment("test-pod", "").Obj(), newDeployment: testingdeployment.MakeDeployment("test-pod", "").Obj(), }, - "without queue": { + "without queue and ReadyReplicas = 0": { oldDeployment: testingdeployment.MakeDeployment("test-pod", ""). Queue("test-queue"). Obj(), newDeployment: testingdeployment.MakeDeployment("test-pod", "").Obj(), + }, + "without queue and ReadyReplicas > 0": { + oldDeployment: testingdeployment.MakeDeployment("test-pod", ""). + ReadyReplicas(1). + Queue("test-queue"). + Obj(), + newDeployment: testingdeployment.MakeDeployment("test-pod", "").Obj(), wantErr: field.ErrorList{ &field.Error{ Type: field.ErrorTypeInvalid, diff --git a/pkg/util/testingjobs/deployment/wrappers.go b/pkg/util/testingjobs/deployment/wrappers.go index 04ac119991..6aad417de2 100644 --- a/pkg/util/testingjobs/deployment/wrappers.go +++ b/pkg/util/testingjobs/deployment/wrappers.go @@ -24,7 +24,8 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/kueue/pkg/controller/constants" + "sigs.k8s.io/kueue/pkg/constants" + controllerconstants "sigs.k8s.io/kueue/pkg/controller/constants" ) // DeploymentWrapper wraps a Deployment. @@ -82,7 +83,12 @@ func (d *DeploymentWrapper) Label(k, v string) *DeploymentWrapper { // Queue updates the queue name of the Deployment func (d *DeploymentWrapper) Queue(q string) *DeploymentWrapper { - return d.Label(constants.QueueLabel, q) + return d.Label(controllerconstants.QueueLabel, q) +} + +// ManagedByKueue updates the queue name of the Deployment +func (d *DeploymentWrapper) ManagedByKueue() *DeploymentWrapper { + return d.Label(constants.ManagedByKueueLabel, "true") } // Name updated the name of the Deployment @@ -130,5 +136,5 @@ func (d *DeploymentWrapper) PodTemplateSpecLabel(k, v string) *DeploymentWrapper // PodTemplateSpecQueue updates the queue name of the pod template spec of the Deployment func (d *DeploymentWrapper) PodTemplateSpecQueue(q string) *DeploymentWrapper { - return d.PodTemplateSpecLabel(constants.QueueLabel, q) + return d.PodTemplateSpecLabel(controllerconstants.QueueLabel, q) } diff --git a/test/e2e/singlecluster/deployment_test.go b/test/e2e/singlecluster/deployment_test.go index cf6059bb72..7a1734ca39 100644 --- a/test/e2e/singlecluster/deployment_test.go +++ b/test/e2e/singlecluster/deployment_test.go @@ -149,7 +149,7 @@ var _ = ginkgo.Describe("Deployment", func() { g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), createdDeployment)).To(gomega.Succeed()) g.Expect(createdDeployment.Status.Replicas).To(gomega.Equal(int32(3))) g.Expect(createdDeployment.Status.UnavailableReplicas).To(gomega.Equal(int32(3))) - g.Expect(createdDeployment.Status.AvailableReplicas).To(gomega.Equal(int32(0))) + g.Expect(createdDeployment.Status.ReadyReplicas).To(gomega.Equal(int32(0))) }, util.Timeout, util.Interval).Should(gomega.Succeed()) }) diff --git a/test/integration/webhook/jobs/deployment_webhook_test.go b/test/integration/webhook/jobs/deployment_webhook_test.go index a4d50bd11a..b7ec2bb9b6 100644 --- a/test/integration/webhook/jobs/deployment_webhook_test.go +++ b/test/integration/webhook/jobs/deployment_webhook_test.go @@ -114,9 +114,18 @@ var _ = ginkgo.Describe("Deployment Webhook", ginkgo.Ordered, ginkgo.ContinueOnF }) }) - ginkgo.It("shouldn't allow to remove the queue label", func() { + ginkgo.It("shouldn't allow to remove the queue label (ReadyReplicas > 0)", func() { createdDeployment := &appsv1.Deployment{} + ginkgo.By("Update status ReadyReplicas = 1", func() { + gomega.Eventually(func(g gomega.Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), createdDeployment)).Should(gomega.Succeed()) + createdDeployment.Status.Replicas = 1 + createdDeployment.Status.ReadyReplicas = 1 + gomega.Expect(k8sClient.Status().Update(ctx, createdDeployment)).To(gomega.Succeed()) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + ginkgo.By("Try to remove queue label", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), createdDeployment)).Should(gomega.Succeed()) delete(createdDeployment.Labels, constants.QueueLabel) @@ -131,6 +140,23 @@ var _ = ginkgo.Describe("Deployment Webhook", ginkgo.Ordered, ginkgo.ContinueOnF }) }) + ginkgo.It("should allow to remove the queue label (ReadyReplicas = 0)", func() { + createdDeployment := &appsv1.Deployment{} + + ginkgo.By("Try to remove queue label", func() { + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), createdDeployment)).Should(gomega.Succeed()) + delete(createdDeployment.Labels, constants.QueueLabel) + gomega.Expect(k8sClient.Update(ctx, createdDeployment)).To(gomega.Succeed()) + }) + + ginkgo.By("Check that queue label deleted from pod template spec", func() { + gomega.Eventually(func(g gomega.Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(deployment), createdDeployment)).Should(gomega.Succeed()) + g.Expect(createdDeployment.Spec.Template.Labels).ShouldNot(gomega.HaveKey(constants.QueueLabel)) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + }) + ginkgo.It("shouldn't allow to change the queue name (ReadyReplicas > 0)", func() { createdDeployment := &appsv1.Deployment{}