From 8cd72b7e783431e2c645b9bc25e190932dbc8952 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Wed, 10 Apr 2024 23:57:13 +0200 Subject: [PATCH] Add CEL rules to ClusterQueue --- apis/kueue/v1beta1/clusterqueue_types.go | 7 + .../crd/kueue.x-k8s.io_clusterqueues.yaml | 22 ++ .../crd/kueue.x-k8s.io_localqueues.yaml | 4 + .../crd/kueue.x-k8s.io_workloads.yaml | 2 + .../bases/kueue.x-k8s.io_clusterqueues.yaml | 22 ++ .../crd/bases/kueue.x-k8s.io_localqueues.yaml | 4 + .../crd/bases/kueue.x-k8s.io_workloads.yaml | 2 + pkg/webhooks/clusterqueue_webhook.go | 22 +- pkg/webhooks/clusterqueue_webhook_test.go | 118 ------- test/integration/webhook/clusterqueue_test.go | 303 +++++++++++++++++- 10 files changed, 364 insertions(+), 142 deletions(-) diff --git a/apis/kueue/v1beta1/clusterqueue_types.go b/apis/kueue/v1beta1/clusterqueue_types.go index 830099391b..4f029f2c09 100644 --- a/apis/kueue/v1beta1/clusterqueue_types.go +++ b/apis/kueue/v1beta1/clusterqueue_types.go @@ -23,6 +23,7 @@ import ( ) // ClusterQueueSpec defines the desired state of ClusterQueue +// +kubebuilder:validation:XValidation:rule="(!has(self.cohort) || size(self.cohort) == 0) && has(self.resourceGroups) ? self.resourceGroups.all(rg, rg.flavors.all(f, f.resources.all(r, !has(r.borrowingLimit)))) : true", message="resourceGroups[0].flavors[0].resources[0].borrowingLimit must be nil when cohort is empty" type ClusterQueueSpec struct { // resourceGroups describes groups of resources. // Each resource group defines the list of resources and a list of flavors @@ -48,6 +49,8 @@ type ClusterQueueSpec struct { // // Validation of a cohort name is equivalent to that of object names: // subdomain in DNS (RFC 1123). + // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$" Cohort string `json:"cohort,omitempty"` // QueueingStrategy indicates the queueing strategy of the workloads @@ -134,6 +137,7 @@ const ( Hold StopPolicy = "Hold" ) +// +kubebuilder:validation:XValidation:rule="self.flavors.all(x, size(x.resources) == size(self.coveredResources))", message="flavors must have the same number of resources as the coveredResources" type ResourceGroup struct { // coveredResources is the list of resources covered by the flavors in this // group. @@ -217,6 +221,8 @@ type ResourceQuota struct { } // ResourceFlavorReference is the name of the ResourceFlavor. +// +kubebuilder:validation:MaxLength=253 +// +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$" type ResourceFlavorReference string // ClusterQueueStatus defines the observed state of ClusterQueue @@ -362,6 +368,7 @@ type FlavorFungibility struct { // ClusterQueuePreemption contains policies to preempt Workloads from this // ClusterQueue or the ClusterQueue's cohort. +// +kubebuilder:validation:XValidation:rule="!(self.reclaimWithinCohort == 'Never' && self.borrowWithinCohort != null && self.borrowWithinCohort.policy != 'Never')", message="reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never" type ClusterQueuePreemption struct { // reclaimWithinCohort determines whether a pending Workload can preempt // Workloads from other ClusterQueues in the cohort that are using more than diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml index e4aceaaa07..871d0bf0d2 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml @@ -99,6 +99,8 @@ spec: Validation of a cohort name is equivalent to that of object names: subdomain in DNS (RFC 1123). + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string flavorFungibility: description: |- @@ -274,6 +276,10 @@ spec: - LowerOrNewerEqualPriority type: string type: object + x-kubernetes-validations: + - message: reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never + rule: '!(self.reclaimWithinCohort == ''Never'' && self.borrowWithinCohort + != null && self.borrowWithinCohort.policy != ''Never'')' queueingStrategy: default: BestEffortFIFO description: |- @@ -330,6 +336,8 @@ spec: name of this flavor. The name should match the .metadata.name of a ResourceFlavor. If a matching ResourceFlavor does not exist, the ClusterQueue will have an Active condition set to False. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: |- @@ -417,6 +425,10 @@ spec: - coveredResources - flavors type: object + x-kubernetes-validations: + - message: flavors must have the same number of resources as the + coveredResources + rule: self.flavors.all(x, size(x.resources) == size(self.coveredResources)) maxItems: 16 type: array x-kubernetes-list-type: atomic @@ -439,6 +451,12 @@ spec: - HoldAndDrain type: string type: object + x-kubernetes-validations: + - message: resourceGroups[0].flavors[0].resources[0].borrowingLimit must + be nil when cohort is empty + rule: '(!has(self.cohort) || size(self.cohort) == 0) && has(self.resourceGroups) + ? self.resourceGroups.all(rg, rg.flavors.all(f, f.resources.all(r, + !has(r.borrowingLimit)))) : true' status: description: ClusterQueueStatus defines the observed state of ClusterQueue properties: @@ -531,6 +549,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources @@ -583,6 +603,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml index 76559bc9e8..d944b7334b 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml @@ -172,6 +172,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources @@ -213,6 +215,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources diff --git a/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml b/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml index a6d85f6841..8697489777 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml @@ -7843,6 +7843,8 @@ spec: additionalProperties: description: ResourceFlavorReference is the name of the ResourceFlavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string description: Flavors are the flavors assigned to the workload for each resource. diff --git a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml index b98091c84f..4fcd6f1ecd 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml @@ -84,6 +84,8 @@ spec: Validation of a cohort name is equivalent to that of object names: subdomain in DNS (RFC 1123). + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string flavorFungibility: description: |- @@ -259,6 +261,10 @@ spec: - LowerOrNewerEqualPriority type: string type: object + x-kubernetes-validations: + - message: reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never + rule: '!(self.reclaimWithinCohort == ''Never'' && self.borrowWithinCohort + != null && self.borrowWithinCohort.policy != ''Never'')' queueingStrategy: default: BestEffortFIFO description: |- @@ -315,6 +321,8 @@ spec: name of this flavor. The name should match the .metadata.name of a ResourceFlavor. If a matching ResourceFlavor does not exist, the ClusterQueue will have an Active condition set to False. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: |- @@ -402,6 +410,10 @@ spec: - coveredResources - flavors type: object + x-kubernetes-validations: + - message: flavors must have the same number of resources as the + coveredResources + rule: self.flavors.all(x, size(x.resources) == size(self.coveredResources)) maxItems: 16 type: array x-kubernetes-list-type: atomic @@ -424,6 +436,12 @@ spec: - HoldAndDrain type: string type: object + x-kubernetes-validations: + - message: resourceGroups[0].flavors[0].resources[0].borrowingLimit must + be nil when cohort is empty + rule: '(!has(self.cohort) || size(self.cohort) == 0) && has(self.resourceGroups) + ? self.resourceGroups.all(rg, rg.flavors.all(f, f.resources.all(r, + !has(r.borrowingLimit)))) : true' status: description: ClusterQueueStatus defines the observed state of ClusterQueue properties: @@ -516,6 +534,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources @@ -568,6 +588,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources diff --git a/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml b/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml index af64fbf32e..5edff1cac7 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml @@ -157,6 +157,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources @@ -198,6 +200,8 @@ spec: properties: name: description: name of the flavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string resources: description: resources lists the quota usage for the resources diff --git a/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml b/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml index 56a156e8bf..095dcb1072 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml @@ -7828,6 +7828,8 @@ spec: additionalProperties: description: ResourceFlavorReference is the name of the ResourceFlavor. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string description: Flavors are the flavors assigned to the workload for each resource. diff --git a/pkg/webhooks/clusterqueue_webhook.go b/pkg/webhooks/clusterqueue_webhook.go index da59185b97..c35afcd5a2 100644 --- a/pkg/webhooks/clusterqueue_webhook.go +++ b/pkg/webhooks/clusterqueue_webhook.go @@ -116,15 +116,9 @@ func ValidateClusterQueue(cq *kueue.ClusterQueue) field.ErrorList { path := field.NewPath("spec") var allErrs field.ErrorList - if len(cq.Spec.Cohort) != 0 { - allErrs = append(allErrs, validateNameReference(cq.Spec.Cohort, path.Child("cohort"))...) - } allErrs = append(allErrs, validateResourceGroups(cq.Spec.ResourceGroups, cq.Spec.Cohort, path.Child("resourceGroups"))...) allErrs = append(allErrs, validation.ValidateLabelSelector(cq.Spec.NamespaceSelector, validation.LabelSelectorValidationOptions{}, path.Child("namespaceSelector"))...) - if cq.Spec.Preemption != nil { - allErrs = append(allErrs, validatePreemption(cq.Spec.Preemption, path.Child("preemption"))...) - } return allErrs } @@ -134,16 +128,6 @@ func ValidateClusterQueueUpdate(newObj, oldObj *kueue.ClusterQueue) field.ErrorL return allErrs } -func validatePreemption(preemption *kueue.ClusterQueuePreemption, path *field.Path) field.ErrorList { - var allErrs field.ErrorList - if preemption.ReclaimWithinCohort == kueue.PreemptionPolicyNever && - preemption.BorrowWithinCohort != nil && - preemption.BorrowWithinCohort.Policy != kueue.BorrowWithinCohortPolicyNever { - allErrs = append(allErrs, field.Invalid(path, preemption, "reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never")) - } - return allErrs -} - func validateResourceGroups(resourceGroups []kueue.ResourceGroup, cohort string, path *field.Path) field.ErrorList { var allErrs field.ErrorList seenResources := sets.New[corev1.ResourceName]() @@ -174,10 +158,7 @@ func validateResourceGroups(resourceGroups []kueue.ResourceGroup, cohort string, } func validateFlavorQuotas(flavorQuotas kueue.FlavorQuotas, coveredResources []corev1.ResourceName, cohort string, path *field.Path) field.ErrorList { - allErrs := validateNameReference(string(flavorQuotas.Name), path.Child("name")) - if len(flavorQuotas.Resources) != len(coveredResources) { - allErrs = append(allErrs, field.Invalid(path.Child("resources"), field.OmitValueType{}, "must have the same number of resources as the coveredResources")) - } + var allErrs field.ErrorList for i, rq := range flavorQuotas.Resources { if i >= len(coveredResources) { @@ -191,7 +172,6 @@ func validateFlavorQuotas(flavorQuotas kueue.FlavorQuotas, coveredResources []co if rq.BorrowingLimit != nil { borrowingLimitPath := path.Child("borrowingLimit") allErrs = append(allErrs, validateResourceQuantity(*rq.BorrowingLimit, borrowingLimitPath)...) - allErrs = append(allErrs, validateLimit(*rq.BorrowingLimit, cohort, borrowingLimitPath)...) } if features.Enabled(features.LendingLimit) && rq.LendingLimit != nil { lendingLimitPath := path.Child("lendingLimit") diff --git a/pkg/webhooks/clusterqueue_webhook_test.go b/pkg/webhooks/clusterqueue_webhook_test.go index f4a6a8ae7e..a8470951ad 100644 --- a/pkg/webhooks/clusterqueue_webhook_test.go +++ b/pkg/webhooks/clusterqueue_webhook_test.go @@ -34,7 +34,6 @@ import ( func TestValidateClusterQueue(t *testing.T) { specPath := field.NewPath("spec") resourceGroupsPath := specPath.Child("resourceGroups") - preemptionPath := specPath.Child("preemption") testcases := []struct { name string @@ -61,13 +60,6 @@ func TestValidateClusterQueue(t *testing.T) { name: "in cohort", clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Cohort("prod").Obj(), }, - { - name: "invalid cohort", - clusterQueue: testingutil.MakeClusterQueue("cluster-queue").Cohort("@prod").Obj(), - wantErr: field.ErrorList{ - field.Invalid(specPath.Child("cohort"), "@prod", ""), - }, - }, { name: "extended resources with qualified names", clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). @@ -80,15 +72,6 @@ func TestValidateClusterQueue(t *testing.T) { ResourceGroup(*testingutil.MakeFlavorQuotas("x86").Obj()). Obj(), }, - { - name: "flavor with unqualified names", - clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). - ResourceGroup(*testingutil.MakeFlavorQuotas("invalid_name").Obj()). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("name"), "invalid_name", ""), - }, - }, { name: "flavor quota with negative value", clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). @@ -125,16 +108,6 @@ func TestValidateClusterQueue(t *testing.T) { field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources").Index(0).Child("borrowingLimit"), "-1", ""), }, }, - { - name: "flavor quota with borrowingLimit and empty cohort", - clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). - ResourceGroup( - *testingutil.MakeFlavorQuotas("x86").Resource("cpu", "1", "1").Obj()). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources").Index(0).Child("borrowingLimit"), "1", limitIsEmptyErrorMsg), - }, - }, { name: "flavor quota with lendingLimit 0", clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). @@ -260,78 +233,6 @@ func TestValidateClusterQueue(t *testing.T) { field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(1).Child("resources").Index(1).Child("name"), nil, ""), }, }, - { - name: "missing resources in a flavor", - clusterQueue: &kueue.ClusterQueue{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-queue", - }, - Spec: kueue.ClusterQueueSpec{ - ResourceGroups: []kueue.ResourceGroup{ - { - CoveredResources: []corev1.ResourceName{"cpu", "memory"}, - Flavors: []kueue.FlavorQuotas{ - *testingutil.MakeFlavorQuotas("alpha"). - Resource("cpu", "0"). - Obj(), - }, - }, - }, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources"), nil, ""), - }, - }, - { - name: "missing resources in a flavor", - clusterQueue: &kueue.ClusterQueue{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-queue", - }, - Spec: kueue.ClusterQueueSpec{ - ResourceGroups: []kueue.ResourceGroup{ - { - CoveredResources: []corev1.ResourceName{"cpu"}, - Flavors: []kueue.FlavorQuotas{ - *testingutil.MakeFlavorQuotas("alpha"). - Resource("cpu", "0"). - Resource("memory", "0"). - Obj(), - }, - }, - }, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources"), nil, ""), - }, - }, - { - name: "missing resources in a flavor and mismatch", - clusterQueue: &kueue.ClusterQueue{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-queue", - }, - Spec: kueue.ClusterQueueSpec{ - ResourceGroups: []kueue.ResourceGroup{ - { - CoveredResources: []corev1.ResourceName{"blah"}, - Flavors: []kueue.FlavorQuotas{ - *testingutil.MakeFlavorQuotas("alpha"). - Resource("cpu", "0"). - Resource("memory", "0"). - Obj(), - }, - }, - }, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources"), nil, ""), - field.Invalid(resourceGroupsPath.Index(0).Child("flavors").Index(0).Child("resources").Index(0).Child("name"), nil, ""), - }, - }, { name: "resource in more than one resource group", clusterQueue: testingutil.MakeClusterQueue("cluster-queue"). @@ -366,25 +267,6 @@ func TestValidateClusterQueue(t *testing.T) { field.Duplicate(resourceGroupsPath.Index(1).Child("flavors").Index(0).Child("name"), nil), }, }, - { - name: "invalid preemption due to reclaimWithinCohort=Never, while borrowWithinCohort!=nil", - clusterQueue: &kueue.ClusterQueue{ - ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-queue", - }, - Spec: kueue.ClusterQueueSpec{ - Preemption: &kueue.ClusterQueuePreemption{ - ReclaimWithinCohort: kueue.PreemptionPolicyNever, - BorrowWithinCohort: &kueue.BorrowWithinCohort{ - Policy: kueue.BorrowWithinCohortPolicyLowerPriority, - }, - }, - }, - }, - wantErr: field.ErrorList{ - field.Invalid(preemptionPath, nil, ""), - }, - }, { name: "valid preemption with borrowWithinCohort", clusterQueue: &kueue.ClusterQueue{ diff --git a/test/integration/webhook/clusterqueue_test.go b/test/integration/webhook/clusterqueue_test.go index 2524699b09..b9d7ac2dc7 100644 --- a/test/integration/webhook/clusterqueue_test.go +++ b/test/integration/webhook/clusterqueue_test.go @@ -152,7 +152,47 @@ var _ = ginkgo.Describe("ClusterQueue Webhook", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cq), &updateCQ)).Should(gomega.Succeed()) updateCQ.Spec.ResourceGroups[0].Flavors[0].Name = "@x86" return k8sClient.Update(ctx, &updateCQ) - }, util.Timeout, util.Interval).Should(testing.BeForbiddenError()) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("Should allow to update queueingStrategy with different value", func() { + ginkgo.By("Creating a new clusterQueue") + cq := testing.MakeClusterQueue("cluster-queue"). + QueueingStrategy(kueue.StrictFIFO). + ResourceGroup(*testing.MakeFlavorQuotas("x86").Resource(corev1.ResourceMemory).Obj()). + Obj() + gomega.Expect(k8sClient.Create(ctx, cq)).Should(gomega.Succeed()) + + defer func() { + util.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true) + }() + + gomega.Eventually(func() error { + var updateCQ kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cq), &updateCQ)).Should(gomega.Succeed()) + updateCQ.Spec.QueueingStrategy = kueue.BestEffortFIFO + return k8sClient.Update(ctx, &updateCQ) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("Should allow to update queueingStrategy with same value", func() { + ginkgo.By("Creating a new clusterQueue") + cq := testing.MakeClusterQueue("cluster-queue"). + QueueingStrategy(kueue.BestEffortFIFO). + ResourceGroup(*testing.MakeFlavorQuotas("x86").Resource(corev1.ResourceMemory).Obj()). + Obj() + gomega.Expect(k8sClient.Create(ctx, cq)).Should(gomega.Succeed()) + + defer func() { + util.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true) + }() + + gomega.Eventually(func() error { + var updateCQ kueue.ClusterQueue + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cq), &updateCQ)).Should(gomega.Succeed()) + updateCQ.Spec.QueueingStrategy = kueue.BestEffortFIFO + return k8sClient.Update(ctx, &updateCQ) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) }) ginkgo.DescribeTable("Validate ClusterQueue on creation", func(cq *kueue.ClusterQueue, errorType int) { @@ -168,7 +208,7 @@ var _ = ginkgo.Describe("ClusterQueue Webhook", func() { gomega.Expect(errors.IsForbidden(err)).Should(gomega.BeTrue(), "error: %v", err) case isInvalid: gomega.Expect(err).Should(gomega.HaveOccurred()) - gomega.Expect(errors.IsInvalid(err)).Should(gomega.BeTrue(), "error: %v", err) + gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err) default: gomega.Expect(err).ShouldNot(gomega.HaveOccurred()) // Validating that defaults are set. @@ -203,7 +243,7 @@ var _ = ginkgo.Describe("ClusterQueue Webhook", func() { testing.MakeClusterQueue("cluster-queue"). ResourceGroup(*testing.MakeFlavorQuotas("invalid_name").Resource(corev1.ResourceCPU, "5").Obj()). Obj(), - isForbidden), + isInvalid), ginkgo.Entry("Should have qualified resource name", testing.MakeClusterQueue("cluster-queue"). ResourceGroup(*testing.MakeFlavorQuotas("x86").Resource("@cpu", "5").Obj()). @@ -255,6 +295,263 @@ var _ = ginkgo.Describe("ClusterQueue Webhook", func() { ginkgo.Entry("Should forbid to create clusterQueue with unknown preemption.withinClusterQueue", testing.MakeClusterQueue("cluster-queue").Preemption(kueue.ClusterQueuePreemption{WithinClusterQueue: "unknown"}).Obj(), isInvalid), + ginkgo.Entry("Should allow to create clusterQueue with built-in resources with qualified names", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup(*testing.MakeFlavorQuotas("default").Resource("cpu").Obj()). + Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with invalid resource name", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup(*testing.MakeFlavorQuotas("default").Resource("@cpu").Obj()). + Obj(), + isForbidden), + ginkgo.Entry("Should allow to create clusterQueue with valid cohort", + testing.MakeClusterQueue("cluster-queue").Cohort("prod").Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with invalid cohort", + testing.MakeClusterQueue("cluster-queue").Cohort("@prod").Obj(), + isInvalid), + ginkgo.Entry("Should allow to create clusterQueue with extended resources with qualified names", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup(*testing.MakeFlavorQuotas("default").Resource("example.com/gpu").Obj()). + Obj(), + isValid), + ginkgo.Entry("Should allow to create clusterQueue with flavor with qualified names", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup(*testing.MakeFlavorQuotas("x86").Resource("cpu").Obj()). + Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with flavor with unqualified names", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup(*testing.MakeFlavorQuotas("invalid_name").Obj()). + Obj(), + isInvalid), + ginkgo.Entry("Should forbid to create clusterQueue with flavor quota with negative value", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("x86").Resource("cpu", "-1").Obj()). + Obj(), + isForbidden), + ginkgo.Entry("Should allow to create clusterQueue with flavor quota with zero values", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("x86").Resource("cpu", "0").Obj()). + Obj(), + isValid), + ginkgo.Entry("Should allow to create clusterQueue with flavor quota with borrowingLimit 0", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("x86").Resource("cpu", "1", "0").Obj()). + Cohort("cohort"). + Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with flavor quota with negative borrowingLimit", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("x86").Resource("cpu", "1", "-1").Obj()). + Cohort("cohort"). + Obj(), + isForbidden), + ginkgo.Entry("Should forbid to create clusterQueue with flavor quota with borrowingLimit and empty cohort", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("x86").Resource("cpu", "1", "1").Obj()). + Obj(), + isInvalid), + ginkgo.Entry("Should allow to create clusterQueue with empty queueing strategy", + testing.MakeClusterQueue("cluster-queue"). + QueueingStrategy(""). + Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with namespaceSelector with invalid labels", + testing.MakeClusterQueue("cluster-queue").NamespaceSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{"nospecialchars^=@": "bar"}, + }).Obj(), + isForbidden), + ginkgo.Entry("Should forbid to create clusterQueue with namespaceSelector with invalid expressions", + testing.MakeClusterQueue("cluster-queue").NamespaceSelector(&metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "key", + Operator: "In", + }, + }, + }).Obj(), + isForbidden), + ginkgo.Entry("Should allow to create clusterQueue with multiple resource groups", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + *testing.MakeFlavorQuotas("beta"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + ). + ResourceGroup( + *testing.MakeFlavorQuotas("gamma"). + Resource("example.com/gpu", "0"). + Obj(), + *testing.MakeFlavorQuotas("omega"). + Resource("example.com/gpu", "0"). + Obj(), + ). + Obj(), + isValid), + ginkgo.Entry("Should forbid to create clusterQueue with resources in a flavor in different order", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + ResourceGroups: []kueue.ResourceGroup{ + { + CoveredResources: []corev1.ResourceName{"cpu", "memory"}, + Flavors: []kueue.FlavorQuotas{ + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + *testing.MakeFlavorQuotas("beta"). + Resource("memory", "0"). + Resource("cpu", "0"). + Obj(), + }, + }, + }, + }, + }, + isForbidden), + ginkgo.Entry("Should forbid to create clusterQueue missing resources in a flavor", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + ResourceGroups: []kueue.ResourceGroup{ + { + CoveredResources: []corev1.ResourceName{"cpu", "memory"}, + Flavors: []kueue.FlavorQuotas{ + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Obj(), + }, + }, + }, + }, + }, + isInvalid), + ginkgo.Entry("Should forbid to create clusterQueue missing resources in a flavor", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + ResourceGroups: []kueue.ResourceGroup{ + { + CoveredResources: []corev1.ResourceName{"cpu"}, + Flavors: []kueue.FlavorQuotas{ + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + }, + }, + }, + }, + }, + isInvalid), + ginkgo.Entry("Should forbid to create clusterQueue missing resources in a flavor and mismatch", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + ResourceGroups: []kueue.ResourceGroup{ + { + CoveredResources: []corev1.ResourceName{"blah"}, + Flavors: []kueue.FlavorQuotas{ + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + }, + }, + }, + }, + }, + isInvalid), + ginkgo.Entry("Should forbid to create clusterQueue with resource in more than one resource group", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("alpha"). + Resource("cpu", "0"). + Resource("memory", "0"). + Obj(), + ). + ResourceGroup( + *testing.MakeFlavorQuotas("beta"). + Resource("memory", "0"). + Obj(), + ). + Obj(), + isForbidden), + ginkgo.Entry("Should forbid to create clusterQueue with flavor in more than one resource group", + testing.MakeClusterQueue("cluster-queue"). + ResourceGroup( + *testing.MakeFlavorQuotas("alpha").Resource("cpu").Obj(), + *testing.MakeFlavorQuotas("beta").Resource("cpu").Obj(), + ). + ResourceGroup( + *testing.MakeFlavorQuotas("beta").Resource("memory").Obj(), + ). + Obj(), + isForbidden), + ginkgo.Entry("Should forbid to create clusterQueue missing with invalid preemption due to reclaimWithinCohort=Never, while borrowWithinCohort!=nil", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + Preemption: &kueue.ClusterQueuePreemption{ + ReclaimWithinCohort: kueue.PreemptionPolicyNever, + BorrowWithinCohort: &kueue.BorrowWithinCohort{ + Policy: kueue.BorrowWithinCohortPolicyLowerPriority, + }, + }, + }, + }, + isInvalid), + ginkgo.Entry("Should allow to create clusterQueue with valid preemption with borrowWithinCohort", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + Preemption: &kueue.ClusterQueuePreemption{ + ReclaimWithinCohort: kueue.PreemptionPolicyLowerPriority, + BorrowWithinCohort: &kueue.BorrowWithinCohort{ + Policy: kueue.BorrowWithinCohortPolicyLowerPriority, + MaxPriorityThreshold: ptr.To[int32](10), + }, + }, + }, + }, + isValid), + ginkgo.Entry("Should allow to create clusterQueue with existing cluster queue created with older Kueue version that has a nil borrowWithinCohort field", + &kueue.ClusterQueue{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-queue", + }, + Spec: kueue.ClusterQueueSpec{ + Preemption: &kueue.ClusterQueuePreemption{ + ReclaimWithinCohort: kueue.PreemptionPolicyNever, + }, + }, + }, + isValid), ) }) })