From 7e75cde287bdfe55769aa4db314e35158ab56c4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 18 Apr 2024 13:45:16 +0200 Subject: [PATCH] Add CEL rules to Workload --- apis/kueue/v1beta1/workload_types.go | 25 + .../crd/kueue.x-k8s.io_workloads.yaml | 61 ++ .../crd/bases/kueue.x-k8s.io_workloads.yaml | 61 ++ pkg/webhooks/common.go | 11 - pkg/webhooks/resourceflavor_webhook.go | 25 - pkg/webhooks/resourceflavor_webhook_test.go | 39 - pkg/webhooks/workload_webhook.go | 80 -- pkg/webhooks/workload_webhook_test.go | 158 +--- test/integration/webhook/workload_test.go | 791 +++++++++++++++++- 9 files changed, 934 insertions(+), 317 deletions(-) diff --git a/apis/kueue/v1beta1/workload_types.go b/apis/kueue/v1beta1/workload_types.go index fc8c48c95d..549c4ea0b1 100644 --- a/apis/kueue/v1beta1/workload_types.go +++ b/apis/kueue/v1beta1/workload_types.go @@ -22,6 +22,7 @@ import ( ) // WorkloadSpec defines the desired state of Workload +// +kubebuilder:validation:XValidation:rule="has(self.priorityClassName) ? has(self.priority) : true", message="priority should not be nil when priorityClassName is set" type WorkloadSpec struct { // podSets is a list of sets of homogeneous pods, each described by a Pod spec // and a count. @@ -36,6 +37,8 @@ type WorkloadSpec struct { // queueName is the name of the LocalQueue the Workload is associated with. // queueName cannot be changed while .status.admission is not null. + // +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])?)*$" QueueName string `json:"queueName,omitempty"` // If specified, indicates the workload's priority. @@ -44,6 +47,8 @@ type WorkloadSpec struct { // the highest priority. Any other name must be defined by creating a // PriorityClass object with that name. If not specified, the workload // priority will be default or zero if there is no default. + // +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])?)*$" PriorityClassName string `json:"priorityClassName,omitempty"` // Priority determines the order of access to the resources managed by the @@ -79,12 +84,15 @@ type Admission struct { // PodSetAssignments hold the admission results for each of the .spec.podSets entries. // +listType=map // +listMapKey=name + // +kubebuilder:validation:MaxItems=8 PodSetAssignments []PodSetAssignment `json:"podSetAssignments"` } type PodSetAssignment struct { // Name is the name of the podSet. It should match one of the names in .spec.podSets. // +kubebuilder:default=main + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" Name string `json:"name"` // Flavors are the flavors assigned to the workload for each resource. @@ -107,8 +115,11 @@ type PodSetAssignment struct { Count *int32 `json:"count,omitempty"` } +// +kubebuilder:validation:XValidation:rule="has(self.minCount) ? self.minCount <= self.count : true", message="minCount should be positive and less or equal to count" type PodSet struct { // name is the PodSet name. + // +kubebuilder:validation:MaxLength=63 + // +kubebuilder:validation:Pattern="^[a-z0-9]([-a-z0-9]*[a-z0-9])?$" Name string `json:"name"` // template is the Pod template. @@ -141,6 +152,7 @@ type PodSet struct { // This is an alpha field and requires enabling PartialAdmission feature gate. // // +optional + // +kubebuilder:validation:Minimum=1 MinCount *int32 `json:"minCount,omitempty"` } @@ -149,6 +161,7 @@ type WorkloadStatus struct { // admission holds the parameters of the admission of the workload by a // ClusterQueue. admission can be set back to null, but its fields cannot be // changed once set. + // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="field is immutable" Admission *Admission `json:"admission,omitempty"` // requeueState holds the re-queue state @@ -179,6 +192,7 @@ type WorkloadStatus struct { // +optional // +listType=map // +listMapKey=name + // +kubebuilder:validation:MaxItems=8 ReclaimablePods []ReclaimablePod `json:"reclaimablePods,omitempty"` // admissionChecks list all the admission checks required by the workload and the current status @@ -187,6 +201,7 @@ type WorkloadStatus struct { // +listMapKey=name // +patchStrategy=merge // +patchMergeKey=name + // +kubebuilder:validation:MaxItems=8 AdmissionChecks []AdmissionCheckState `json:"admissionChecks,omitempty" patchStrategy:"merge" patchMergeKey:"name"` } @@ -234,6 +249,7 @@ type AdmissionCheckState struct { // +optional // +listType=atomic + // +kubebuilder:validation:MaxItems=8 PodSetUpdates []PodSetUpdate `json:"podSetUpdates,omitempty"` } @@ -257,6 +273,12 @@ type PodSetUpdate struct { NodeSelector map[string]string `json:"nodeSelector,omitempty"` // +optional + // +kubebuilder:validation:MaxItems=8 + // +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.key) ? x.operator == 'Exists' : true)", message="operator must be Exists when 'key' is empty, which means 'match all values and all keys'" + // +kubebuilder:validation:XValidation:rule="self.all(x, has(x.tolerationSeconds) ? x.effect == 'NoExecute' : true)", message="effect must be 'NoExecute' when 'tolerationSeconds' is set" + // +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.operator) || x.operator in ['Equal', 'Exists'])", message="supported toleration values: 'Equal'(default), 'Exists'" + // +kubebuilder:validation:XValidation:rule="self.all(x, has(x.operator) && x.operator == 'Exists' ? !has(x.value) : true)", message="a value must be empty when 'operator' is 'Exists'" + // +kubebuilder:validation:XValidation:rule="self.all(x, !has(x.effect) || x.effect in ['NoSchedule', 'PreferNoSchedule', 'NoExecute'])", message="supported taint effect values: 'NoSchedule', 'PreferNoSchedule', 'NoExecute'" Tolerations []corev1.Toleration `json:"tolerations,omitempty"` } @@ -336,6 +358,9 @@ const ( // +kubebuilder:resource:shortName={wl} // Workload is the Schema for the workloads API +// +kubebuilder:validation:XValidation:rule="has(self.spec) && has(self.status) && has(self.status.conditions) && self.status.conditions.exists(c, c.type == 'QuotaReserved' && c.status == 'True') && has(self.status.admission) ? size(self.spec.podSets) == size(self.status.admission.podSetAssignments) : true", message="podSetAssignments must have the same number of podSets as the spec" +// +kubebuilder:validation:XValidation:rule="(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) && oldSelf.status.conditions.exists(c, c.type == 'QuotaReserved' && c.status == 'True')) ? (self.spec.podSets == oldSelf.spec.podSets && self.spec.priorityClassSource == oldSelf.spec.priorityClassSource && self.spec.priorityClassName == oldSelf.spec.priorityClassName) : true", message="field is immutable" +// +kubebuilder:validation:XValidation:rule="(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) && oldSelf.status.conditions.exists(c, c.type == 'QuotaReserved' && c.status == 'True')) && (has(self.spec) && has(self.status) && has(self.status.conditions) && self.status.conditions.exists(c, c.type == 'QuotaReserved' && c.status == 'True')) ? self.spec.queueName == oldSelf.spec.queueName : true", message="field is immutable" type Workload struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` 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 8697489777..c87686f1a0 100644 --- a/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml +++ b/charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml @@ -111,9 +111,12 @@ spec: This is an alpha field and requires enabling PartialAdmission feature gate. format: int32 + minimum: 1 type: integer name: description: name is the PodSet name. + maxLength: 63 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string template: description: |- @@ -7767,6 +7770,9 @@ spec: - name - template type: object + x-kubernetes-validations: + - message: minCount should be positive and less or equal to count + rule: 'has(self.minCount) ? self.minCount <= self.count : true' maxItems: 8 minItems: 1 type: array @@ -7790,6 +7796,8 @@ spec: the highest priority. Any other name must be defined by creating a PriorityClass object with that name. If not specified, the workload priority will be default or zero if there is no default. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string priorityClassSource: default: "" @@ -7806,10 +7814,15 @@ spec: description: |- queueName is the name of the LocalQueue the Workload is associated with. queueName cannot be changed while .status.admission is not null. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string required: - podSets type: object + x-kubernetes-validations: + - message: priority should not be nil when priorityClassName is set + rule: 'has(self.priorityClassName) ? has(self.priority) : true' status: description: WorkloadStatus defines the observed state of Workload properties: @@ -7853,6 +7866,8 @@ spec: default: main description: Name is the name of the podSet. It should match one of the names in .spec.podSets. + maxLength: 63 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string resourceUsage: additionalProperties: @@ -7872,6 +7887,7 @@ spec: required: - name type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -7880,6 +7896,9 @@ spec: - clusterQueue - podSetAssignments type: object + x-kubernetes-validations: + - message: field is immutable + rule: self == oldSelf admissionChecks: description: admissionChecks list all the admission checks required by the workload and the current status @@ -7962,10 +7981,32 @@ spec: If the operator is Exists, the value should be empty, otherwise just a regular string. type: string type: object + maxItems: 8 type: array + x-kubernetes-validations: + - message: operator must be Exists when 'key' is empty, + which means 'match all values and all keys' + rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' + : true)' + - message: effect must be 'NoExecute' when 'tolerationSeconds' + is set + rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect + == ''NoExecute'' : true)' + - message: 'supported toleration values: ''Equal''(default), + ''Exists''' + rule: self.all(x, !has(x.operator) || x.operator in + ['Equal', 'Exists']) + - message: a value must be empty when 'operator' is 'Exists' + rule: 'self.all(x, has(x.operator) && x.operator == + ''Exists'' ? !has(x.value) : true)' + - message: 'supported taint effect values: ''NoSchedule'', + ''PreferNoSchedule'', ''NoExecute''' + rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', + 'PreferNoSchedule', 'NoExecute']) required: - name type: object + maxItems: 8 type: array x-kubernetes-list-type: atomic state: @@ -7983,6 +8024,7 @@ spec: - name - state type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -8090,6 +8132,7 @@ spec: - count - name type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -8117,6 +8160,24 @@ spec: type: object type: object type: object + x-kubernetes-validations: + - message: podSetAssignments must have the same number of podSets as the spec + rule: 'has(self.spec) && has(self.status) && has(self.status.conditions) + && self.status.conditions.exists(c, c.type == ''QuotaReserved'' && c.status + == ''True'') && has(self.status.admission) ? size(self.spec.podSets) == + size(self.status.admission.podSetAssignments) : true' + - message: field is immutable + rule: '(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) + && oldSelf.status.conditions.exists(c, c.type == ''QuotaReserved'' && + c.status == ''True'')) ? (self.spec.podSets == oldSelf.spec.podSets && + self.spec.priorityClassSource == oldSelf.spec.priorityClassSource && self.spec.priorityClassName + == oldSelf.spec.priorityClassName) : true' + - message: field is immutable + rule: '(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) + && oldSelf.status.conditions.exists(c, c.type == ''QuotaReserved'' && + c.status == ''True'')) && (has(self.spec) && has(self.status) && has(self.status.conditions) + && self.status.conditions.exists(c, c.type == ''QuotaReserved'' && c.status + == ''True'')) ? self.spec.queueName == oldSelf.spec.queueName : true' served: true storage: true subresources: 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 095dcb1072..6ad04089b7 100644 --- a/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml +++ b/config/components/crd/bases/kueue.x-k8s.io_workloads.yaml @@ -96,9 +96,12 @@ spec: This is an alpha field and requires enabling PartialAdmission feature gate. format: int32 + minimum: 1 type: integer name: description: name is the PodSet name. + maxLength: 63 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string template: description: |- @@ -7752,6 +7755,9 @@ spec: - name - template type: object + x-kubernetes-validations: + - message: minCount should be positive and less or equal to count + rule: 'has(self.minCount) ? self.minCount <= self.count : true' maxItems: 8 minItems: 1 type: array @@ -7775,6 +7781,8 @@ spec: the highest priority. Any other name must be defined by creating a PriorityClass object with that name. If not specified, the workload priority will be default or zero if there is no default. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string priorityClassSource: default: "" @@ -7791,10 +7799,15 @@ spec: description: |- queueName is the name of the LocalQueue the Workload is associated with. queueName cannot be changed while .status.admission is not null. + maxLength: 253 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string required: - podSets type: object + x-kubernetes-validations: + - message: priority should not be nil when priorityClassName is set + rule: 'has(self.priorityClassName) ? has(self.priority) : true' status: description: WorkloadStatus defines the observed state of Workload properties: @@ -7838,6 +7851,8 @@ spec: default: main description: Name is the name of the podSet. It should match one of the names in .spec.podSets. + maxLength: 63 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ type: string resourceUsage: additionalProperties: @@ -7857,6 +7872,7 @@ spec: required: - name type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -7865,6 +7881,9 @@ spec: - clusterQueue - podSetAssignments type: object + x-kubernetes-validations: + - message: field is immutable + rule: self == oldSelf admissionChecks: description: admissionChecks list all the admission checks required by the workload and the current status @@ -7947,10 +7966,32 @@ spec: If the operator is Exists, the value should be empty, otherwise just a regular string. type: string type: object + maxItems: 8 type: array + x-kubernetes-validations: + - message: operator must be Exists when 'key' is empty, + which means 'match all values and all keys' + rule: 'self.all(x, !has(x.key) ? x.operator == ''Exists'' + : true)' + - message: effect must be 'NoExecute' when 'tolerationSeconds' + is set + rule: 'self.all(x, has(x.tolerationSeconds) ? x.effect + == ''NoExecute'' : true)' + - message: 'supported toleration values: ''Equal''(default), + ''Exists''' + rule: self.all(x, !has(x.operator) || x.operator in + ['Equal', 'Exists']) + - message: a value must be empty when 'operator' is 'Exists' + rule: 'self.all(x, has(x.operator) && x.operator == + ''Exists'' ? !has(x.value) : true)' + - message: 'supported taint effect values: ''NoSchedule'', + ''PreferNoSchedule'', ''NoExecute''' + rule: self.all(x, !has(x.effect) || x.effect in ['NoSchedule', + 'PreferNoSchedule', 'NoExecute']) required: - name type: object + maxItems: 8 type: array x-kubernetes-list-type: atomic state: @@ -7968,6 +8009,7 @@ spec: - name - state type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -8075,6 +8117,7 @@ spec: - count - name type: object + maxItems: 8 type: array x-kubernetes-list-map-keys: - name @@ -8102,6 +8145,24 @@ spec: type: object type: object type: object + x-kubernetes-validations: + - message: podSetAssignments must have the same number of podSets as the spec + rule: 'has(self.spec) && has(self.status) && has(self.status.conditions) + && self.status.conditions.exists(c, c.type == ''QuotaReserved'' && c.status + == ''True'') && has(self.status.admission) ? size(self.spec.podSets) == + size(self.status.admission.podSetAssignments) : true' + - message: field is immutable + rule: '(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) + && oldSelf.status.conditions.exists(c, c.type == ''QuotaReserved'' && + c.status == ''True'')) ? (self.spec.podSets == oldSelf.spec.podSets && + self.spec.priorityClassSource == oldSelf.spec.priorityClassSource && self.spec.priorityClassName + == oldSelf.spec.priorityClassName) : true' + - message: field is immutable + rule: '(has(oldSelf.spec) && has(oldSelf.status) && has(oldSelf.status.conditions) + && oldSelf.status.conditions.exists(c, c.type == ''QuotaReserved'' && + c.status == ''True'')) && (has(self.spec) && has(self.status) && has(self.status.conditions) + && self.status.conditions.exists(c, c.type == ''QuotaReserved'' && c.status + == ''True'')) ? self.spec.queueName == oldSelf.spec.queueName : true' served: true storage: true subresources: diff --git a/pkg/webhooks/common.go b/pkg/webhooks/common.go index 02d324c629..9554c241bd 100644 --- a/pkg/webhooks/common.go +++ b/pkg/webhooks/common.go @@ -13,14 +13,3 @@ func validateResourceName(name corev1.ResourceName, fldPath *field.Path) field.E } return allErrs } - -// validateNameReference is the same validation applied to name of an ObjectMeta. -func validateNameReference(name string, path *field.Path) field.ErrorList { - var allErrs field.ErrorList - if msgs := validation.IsDNS1123Subdomain(name); len(msgs) > 0 { - for _, msg := range msgs { - allErrs = append(allErrs, field.Invalid(path, name, msg)) - } - } - return allErrs -} diff --git a/pkg/webhooks/resourceflavor_webhook.go b/pkg/webhooks/resourceflavor_webhook.go index ffe6d4330c..9eeb3060ac 100644 --- a/pkg/webhooks/resourceflavor_webhook.go +++ b/pkg/webhooks/resourceflavor_webhook.go @@ -128,28 +128,3 @@ func validateNodeTaints(taints []corev1.Taint, fldPath *field.Path) field.ErrorL } return allErrors } - -// TODO(#463): Remove this function when CEL validations are added to workload type -// validateTaintEffect is extracted from git.k8s.io/kubernetes/pkg/apis/core/validation/validation.go -func validateTaintEffect(effect *corev1.TaintEffect, allowEmpty bool, fldPath *field.Path) field.ErrorList { - if !allowEmpty && len(*effect) == 0 { - return field.ErrorList{field.Required(fldPath, "")} - } - - allErrors := field.ErrorList{} - switch *effect { - // TODO: Replace next line with subsequent commented-out line when implement TaintEffectNoScheduleNoAdmit. - case corev1.TaintEffectNoSchedule, corev1.TaintEffectPreferNoSchedule, corev1.TaintEffectNoExecute: - // case core.TaintEffectNoSchedule, core.TaintEffectPreferNoSchedule, core.TaintEffectNoScheduleNoAdmit, core.TaintEffectNoExecute: - default: - validValues := []string{ - string(corev1.TaintEffectNoSchedule), - string(corev1.TaintEffectPreferNoSchedule), - string(corev1.TaintEffectNoExecute), - // TODO: Uncomment this block when implement TaintEffectNoScheduleNoAdmit. - // string(core.TaintEffectNoScheduleNoAdmit), - } - allErrors = append(allErrors, field.NotSupported(fldPath, *effect, validValues)) - } - return allErrors -} diff --git a/pkg/webhooks/resourceflavor_webhook_test.go b/pkg/webhooks/resourceflavor_webhook_test.go index 8e2983fca6..518b2c807e 100644 --- a/pkg/webhooks/resourceflavor_webhook_test.go +++ b/pkg/webhooks/resourceflavor_webhook_test.go @@ -63,45 +63,6 @@ func TestValidateResourceFlavor(t *testing.T) { field.Invalid(field.NewPath("spec", "nodeLabels"), "@abc", ""), }, }, - { - name: "bad tolerations", - rf: utiltesting.MakeResourceFlavor("resource-flavor"). - Toleration(corev1.Toleration{ - Key: "@abc", - Operator: corev1.TolerationOpEqual, - Value: "v", - Effect: corev1.TaintEffectNoSchedule, - }). - Toleration(corev1.Toleration{ - Key: "abc", - Operator: corev1.TolerationOpExists, - Value: "v", - Effect: corev1.TaintEffectNoSchedule, - }). - Toleration(corev1.Toleration{ - Key: "abc", - Operator: corev1.TolerationOpEqual, - Value: "v", - Effect: corev1.TaintEffect("not-valid"), - }). - Toleration(corev1.Toleration{ - Key: "abc", - Operator: corev1.TolerationOpEqual, - Value: "v", - Effect: corev1.TaintEffectNoSchedule, - }). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec", "tolerations").Index(0).Child("key"), "@abc", ""), - field.Invalid(field.NewPath("spec", "tolerations").Index(1).Child("operator"), corev1.Toleration{ - Key: "abc", - Operator: corev1.TolerationOpExists, - Value: "v", - Effect: corev1.TaintEffectNoSchedule, - }, ""), - field.NotSupported(field.NewPath("spec", "tolerations").Index(2).Child("effect"), corev1.TaintEffect("not-valid"), []corev1.TaintEffect{}), - }, - }, } for _, tc := range testcases { diff --git a/pkg/webhooks/workload_webhook.go b/pkg/webhooks/workload_webhook.go index a4527f693c..df324684f5 100644 --- a/pkg/webhooks/workload_webhook.go +++ b/pkg/webhooks/workload_webhook.go @@ -19,14 +19,12 @@ package webhooks import ( "context" "fmt" - "strings" corev1 "k8s.io/api/core/v1" apivalidation "k8s.io/apimachinery/pkg/api/validation" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" - "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/klog/v2" "k8s.io/utils/ptr" @@ -122,22 +120,6 @@ func ValidateWorkload(obj *kueue.Workload) field.ErrorList { allErrs = append(allErrs, field.Invalid(specPath.Child("podSets"), variableCountPosets, "at most one podSet can use minCount")) } - if len(obj.Spec.PriorityClassName) > 0 { - msgs := validation.IsDNS1123Subdomain(obj.Spec.PriorityClassName) - if len(msgs) > 0 { - for _, msg := range msgs { - allErrs = append(allErrs, field.Invalid(specPath.Child("priorityClassName"), obj.Spec.PriorityClassName, msg)) - } - } - if obj.Spec.Priority == nil { - allErrs = append(allErrs, field.Invalid(specPath.Child("priority"), obj.Spec.Priority, "priority should not be nil when priorityClassName is set")) - } - } - - if len(obj.Spec.QueueName) > 0 { - allErrs = append(allErrs, validateNameReference(obj.Spec.QueueName, specPath.Child("queueName"))...) - } - statusPath := field.NewPath("status") if workload.HasQuotaReservation(obj) { allErrs = append(allErrs, validateAdmission(obj, statusPath.Child("admission"))...) @@ -152,10 +134,6 @@ func ValidateWorkload(obj *kueue.Workload) field.ErrorList { func validatePodSet(ps *kueue.PodSet, path *field.Path) field.ErrorList { var allErrs field.ErrorList - // Apply the same validation as container names. - for _, msg := range validation.IsDNS1123Label(ps.Name) { - allErrs = append(allErrs, field.Invalid(path.Child("name"), ps.Name, msg)) - } // validate initContainers icPath := path.Child("template", "spec", "initContainers") @@ -168,10 +146,6 @@ func validatePodSet(ps *kueue.PodSet, path *field.Path) field.ErrorList { allErrs = append(allErrs, validateContainer(&ps.Template.Spec.Containers[ci], cPath.Index(ci))...) } - if min := ptr.Deref(ps.MinCount, ps.Count); min > ps.Count || min < 0 { - allErrs = append(allErrs, field.Forbidden(path.Child("minCount"), fmt.Sprintf("%d should be positive and less or equal to %d", min, ps.Count))) - } - return allErrs } @@ -247,38 +221,6 @@ func validateTolerations(tolerations []corev1.Toleration, fldPath *field.Path) f if len(toleration.Key) > 0 { allErrors = append(allErrors, metav1validation.ValidateLabelName(toleration.Key, idxPath.Child("key"))...) } - - // empty toleration key with Exists operator and empty value means match all taints - if len(toleration.Key) == 0 && toleration.Operator != corev1.TolerationOpExists { - allErrors = append(allErrors, field.Invalid(idxPath.Child("operator"), toleration.Operator, - "operator must be Exists when `key` is empty, which means \"match all values and all keys\"")) - } - - if toleration.TolerationSeconds != nil && toleration.Effect != corev1.TaintEffectNoExecute { - allErrors = append(allErrors, field.Invalid(idxPath.Child("effect"), toleration.Effect, - "effect must be 'NoExecute' when `tolerationSeconds` is set")) - } - - // validate toleration operator and value - switch toleration.Operator { - // empty operator means Equal - case corev1.TolerationOpEqual, "": - if errs := validation.IsValidLabelValue(toleration.Value); len(errs) != 0 { - allErrors = append(allErrors, field.Invalid(idxPath.Child("operator"), toleration.Value, strings.Join(errs, ";"))) - } - case corev1.TolerationOpExists: - if len(toleration.Value) > 0 { - allErrors = append(allErrors, field.Invalid(idxPath.Child("operator"), toleration, "value must be empty when `operator` is 'Exists'")) - } - default: - validValues := []string{string(corev1.TolerationOpEqual), string(corev1.TolerationOpExists)} - allErrors = append(allErrors, field.NotSupported(idxPath.Child("operator"), toleration.Operator, validValues)) - } - - // validate toleration effect, empty toleration effect means match all taint effects - if len(toleration.Effect) > 0 { - allErrors = append(allErrors, validateTaintEffect(&toleration.Effect, true, idxPath.Child("effect"))...) - } } return allErrors } @@ -286,17 +228,12 @@ func validateTolerations(tolerations []corev1.Toleration, fldPath *field.Path) f func validateAdmission(obj *kueue.Workload, path *field.Path) field.ErrorList { admission := obj.Status.Admission var allErrs field.ErrorList - allErrs = append(allErrs, validateNameReference(string(admission.ClusterQueue), path.Child("clusterQueue"))...) names := sets.New[string]() for _, ps := range obj.Spec.PodSets { names.Insert(ps.Name) } assignmentsPath := path.Child("podSetAssignments") - if names.Len() != len(admission.PodSetAssignments) { - allErrs = append(allErrs, field.Invalid(assignmentsPath, field.OmitValueType{}, "must have the same number of podSets as the spec")) - } - for i, ps := range admission.PodSetAssignments { psaPath := assignmentsPath.Index(i) if !names.Has(ps.Name) { @@ -342,34 +279,17 @@ func validateReclaimablePods(obj *kueue.Workload, basePath *field.Path) field.Er func ValidateWorkloadUpdate(newObj, oldObj *kueue.Workload) field.ErrorList { var allErrs field.ErrorList - specPath := field.NewPath("spec") statusPath := field.NewPath("status") allErrs = append(allErrs, ValidateWorkload(newObj)...) - if workload.HasQuotaReservation(oldObj) { - allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.PodSets, oldObj.Spec.PodSets, specPath.Child("podSets"))...) - allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.PriorityClassSource, oldObj.Spec.PriorityClassSource, specPath.Child("priorityClassSource"))...) - allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.PriorityClassName, oldObj.Spec.PriorityClassName, specPath.Child("priorityClassName"))...) - } if workload.HasQuotaReservation(newObj) && workload.HasQuotaReservation(oldObj) { - allErrs = append(allErrs, apivalidation.ValidateImmutableField(newObj.Spec.QueueName, oldObj.Spec.QueueName, specPath.Child("queueName"))...) allErrs = append(allErrs, validateReclaimablePodsUpdate(newObj, oldObj, field.NewPath("status", "reclaimablePods"))...) } - allErrs = append(allErrs, validateAdmissionUpdate(newObj.Status.Admission, oldObj.Status.Admission, field.NewPath("status", "admission"))...) allErrs = append(allErrs, validateImmutablePodSetUpdates(newObj, oldObj, statusPath.Child("admissionChecks"))...) return allErrs } -// validateAdmissionUpdate validates that admission can be set or unset, but the -// fields within can't change. -func validateAdmissionUpdate(new, old *kueue.Admission, path *field.Path) field.ErrorList { - if old == nil || new == nil { - return nil - } - return apivalidation.ValidateImmutableField(new, old, path) -} - // validateReclaimablePodsUpdate validates that the reclaimable counts do not decrease, this should be checked // while the workload is admitted. func validateReclaimablePodsUpdate(newObj, oldObj *kueue.Workload, basePath *field.Path) field.ErrorList { diff --git a/pkg/webhooks/workload_webhook_test.go b/pkg/webhooks/workload_webhook_test.go index fad58647b9..b3c4d5b6e4 100644 --- a/pkg/webhooks/workload_webhook_test.go +++ b/pkg/webhooks/workload_webhook_test.go @@ -117,52 +117,10 @@ func TestValidateWorkload(t *testing.T) { }, ).Obj(), }, - "should have a valid podSet name": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets( - kueue.PodSet{ - Name: "@driver", - Count: 1, - }, - ).Obj(), - wantErr: field.ErrorList{field.Invalid(podSetsPath.Index(0).Child("name"), nil, "")}, - }, - "should have valid priorityClassName": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - PriorityClass("invalid_class"). - Priority(0). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(specPath.Child("priorityClassName"), nil, ""), - }, - }, "should pass validation when priorityClassName is empty": { workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Obj(), wantErr: nil, }, - "should have priority once priorityClassName is set": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - PriorityClass("priority"). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(specPath.Child("priority"), nil, ""), - }, - }, - "should have a valid queueName": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - Queue("@invalid"). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(specPath.Child("queueName"), nil, ""), - }, - }, - "should have a valid clusterQueue name": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - ReserveQuota(testingutil.MakeAdmission("@invalid").Obj()). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(statusPath.Child("admission", "clusterQueue"), nil, ""), - }, - }, "should have a valid podSet name in status assignment": { workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). ReserveQuota(testingutil.MakeAdmission("cluster-queue", "@invalid").Obj()). @@ -171,25 +129,6 @@ func TestValidateWorkload(t *testing.T) { field.NotFound(statusPath.Child("admission", "podSetAssignments").Index(0).Child("name"), nil), }, }, - "should have same podSets in admission": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - PodSets( - kueue.PodSet{ - Name: "main2", - Count: 1, - }, - kueue.PodSet{ - Name: "main1", - Count: 1, - }, - ). - ReserveQuota(testingutil.MakeAdmission("cluster-queue", "main1", "main2", "main3").Obj()). - Obj(), - wantErr: field.ErrorList{ - field.Invalid(statusPath.Child("admission", "podSetAssignments"), nil, ""), - field.NotFound(statusPath.Child("admission", "podSetAssignments").Index(2).Child("name"), nil), - }, - }, "assignment usage should be divisible by count": { workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). PodSets(*testingutil.MakePodSet("main", 3). @@ -350,26 +289,7 @@ func TestValidateWorkload(t *testing.T) { field.NotSupported(statusPath.Child("reclaimablePods").Key("ps2").Child("name"), nil, []string{}), }, }, - "invalid podSet minCount (negative)": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - PodSets( - *testingutil.MakePodSet("ps1", 3).SetMinimumCount(-1).Obj(), - ). - Obj(), - wantErr: field.ErrorList{ - field.Forbidden(podSetsPath.Index(0).Child("minCount"), ""), - }, - }, - "invalid podSet minCount (too big)": { - workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). - PodSets( - *testingutil.MakePodSet("ps1", 3).SetMinimumCount(4).Obj(), - ). - Obj(), - wantErr: field.ErrorList{ - field.Forbidden(podSetsPath.Index(0).Child("minCount"), ""), - }, - }, + "too many variable count podSets": { workload: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). PodSets( @@ -397,39 +317,6 @@ func TestValidateWorkloadUpdate(t *testing.T) { before, after *kueue.Workload wantErr field.ErrorList }{ - "podSets should not be updated when has quota reservation: count": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets( - *testingutil.MakePodSet("main", 2).Obj(), - ).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("podSets"), nil, ""), - }, - }, - "podSets should not be updated: podSpec": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets( - kueue.PodSet{ - Name: "main", - Count: 1, - Template: corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "c-after", - Resources: corev1.ResourceRequirements{ - Requests: make(corev1.ResourceList), - }, - }, - }, - }, - }, - }, - ).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("podSets"), nil, ""), - }, - }, "queueName can be updated when not admitted": { before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q1").Obj(), after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q2").Obj(), @@ -440,15 +327,6 @@ func TestValidateWorkloadUpdate(t *testing.T) { after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q"). ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), }, - "queueName should not be updated once admitted": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q1"). - ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q2"). - ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("queueName"), nil, ""), - }, - }, "queueName can be updated when admission is reset": { before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q1"). ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), @@ -468,18 +346,6 @@ func TestValidateWorkloadUpdate(t *testing.T) { after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Obj(), wantErr: nil, }, - "admission should not be updated once set": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).ReserveQuota( - testingutil.MakeAdmission("cluster-queue").Obj(), - ).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).ReserveQuota( - testingutil.MakeAdmission("cluster-queue").Assignment("on-demand", "5", "1").Obj(), - ).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("status", "admission"), nil, ""), - }, - }, - "reclaimable pod count can change up": { before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace). PodSets( @@ -579,28 +445,6 @@ func TestValidateWorkloadUpdate(t *testing.T) { Obj(), wantErr: nil, }, - "priorityClassSource should not be updated": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q"). - PriorityClass("test-class").PriorityClassSource(constants.PodPriorityClassSource). - Priority(10).ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q"). - PriorityClass("test-class").PriorityClassSource(constants.WorkloadPriorityClassSource). - Priority(10).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("priorityClassSource"), nil, ""), - }, - }, - "priorityClassName should not be updated": { - before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q"). - PriorityClass("test-class-1").PriorityClassSource(constants.PodPriorityClassSource). - Priority(10).ReserveQuota(testingutil.MakeAdmission("cq").Obj()).Obj(), - after: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).Queue("q"). - PriorityClass("test-class-2").PriorityClassSource(constants.PodPriorityClassSource). - Priority(10).Obj(), - wantErr: field.ErrorList{ - field.Invalid(field.NewPath("spec").Child("priorityClassName"), nil, ""), - }, - }, "podSetUpdates should be immutable when state is ready": { before: testingutil.MakeWorkload(testWorkloadName, testWorkloadNamespace).PodSets( *testingutil.MakePodSet("first", 1).Obj(), diff --git a/test/integration/webhook/workload_test.go b/test/integration/webhook/workload_test.go index bf49b6a365..625337ab3b 100644 --- a/test/integration/webhook/workload_test.go +++ b/test/integration/webhook/workload_test.go @@ -15,12 +15,14 @@ package webhook import ( "fmt" + "time" "github.com/onsi/ginkgo/v2" "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" schedulingv1 "k8s.io/api/scheduling/v1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" @@ -29,6 +31,7 @@ import ( kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" "sigs.k8s.io/kueue/pkg/constants" "sigs.k8s.io/kueue/pkg/util/testing" + "sigs.k8s.io/kueue/pkg/workload" "sigs.k8s.io/kueue/test/util" ) @@ -82,6 +85,36 @@ var _ = ginkgo.Describe("Workload defaulting webhook", func() { gomega.Expect(created.Spec.PodSets[0].Name).Should(gomega.Equal(kueue.DefaultPodSetName)) }) + + ginkgo.It("Shouldn't set podSet name if multiple", func() { + ginkgo.By("Creating a new Workload") + // Not using the wrappers to avoid hiding any defaulting. + workload := kueue.Workload{ + ObjectMeta: metav1.ObjectMeta{Name: workloadName, Namespace: ns.Name}, + Spec: kueue.WorkloadSpec{ + PodSets: []kueue.PodSet{ + { + Count: 1, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{}, + }, + }, + }, + { + Count: 1, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{}, + }, + }, + }, + }, + }, + } + gomega.Expect(k8sClient.Create(ctx, &workload)).Should(testing.BeAPIError(testing.InvalidError)) + }) + }) }) @@ -95,7 +128,7 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { Obj() err := k8sClient.Create(ctx, workload) gomega.Expect(err).Should(gomega.HaveOccurred()) - gomega.Expect(errors.IsForbidden(err)).Should(gomega.BeTrue(), "error: %v", err) + gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err) }) ginkgo.DescribeTable("Should have valid PodSet when creating", func(podSetsCapacity int, podSetCount int, isInvalid bool) { @@ -117,6 +150,325 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { ginkgo.Entry("invalid podSet.Count", 3, 0, true), ginkgo.Entry("valid podSet", 3, 3, false), ) + + ginkgo.DescribeTable("Should have valid values when creating", func(w func() *kueue.Workload, errorType int) { + err := k8sClient.Create(ctx, w()) + switch errorType { + case isForbidden: + gomega.Expect(err).Should(gomega.HaveOccurred()) + gomega.Expect(err).Should(testing.BeAPIError(testing.ForbiddenError), "error: %v", err) + case isInvalid: + gomega.Expect(err).Should(gomega.HaveOccurred()) + gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err) + default: + gomega.Expect(err).ShouldNot(gomega.HaveOccurred()) + } + }, + ginkgo.Entry("workload", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name).PodSets( + *testing.MakePodSet("driver", 1).Obj(), + *testing.MakePodSet("workers", 100).Obj(), + ).Obj() + }, + isValid), + ginkgo.Entry("podSet name", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name).PodSets( + *testing.MakePodSet("@driver", 1).Obj(), + ).Obj() + }, + isInvalid), + ginkgo.Entry("priorityClassName", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PriorityClass("invalid_class"). + Priority(0). + Obj() + }, + isInvalid), + ginkgo.Entry("priorityClassName is empty", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + isValid), + ginkgo.Entry("priorityClassName is set", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PriorityClass("priority"). + Obj() + }, + isInvalid), + ginkgo.Entry("queueName", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Queue("@invalid"). + Obj() + }, + isInvalid), + + ginkgo.Entry("should not request num-pods resource", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets(kueue.PodSet{ + Name: "bad", + Count: 1, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("1"), + }, + }, + }, + }, + Containers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("1"), + }, + }, + }, + }, + }, + }, + }). + Obj() + }, + isForbidden), + ginkgo.Entry("empty podSetUpdates", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + AdmissionChecks(kueue.AdmissionCheckState{}). + Obj() + }, + isValid), + ginkgo.Entry("matched names in podSetUpdates with names in podSets", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("first", 1).Obj(), + *testing.MakePodSet("second", 1).Obj(), + ). + AdmissionChecks( + kueue.AdmissionCheckState{ + PodSetUpdates: []kueue.PodSetUpdate{ + { + Name: "first", + Labels: map[string]string{"l1": "first"}, + Annotations: map[string]string{"foo": "bar"}, + Tolerations: []corev1.Toleration{ + { + Key: "t1", + Operator: corev1.TolerationOpEqual, + Value: "t1v", + Effect: corev1.TaintEffectNoExecute, + TolerationSeconds: ptr.To[int64](5), + }, + }, + NodeSelector: map[string]string{"type": "first"}, + }, + { + Name: "second", + Labels: map[string]string{"l2": "second"}, + Annotations: map[string]string{"foo": "baz"}, + Tolerations: []corev1.Toleration{ + { + Key: "t2", + Operator: corev1.TolerationOpEqual, + Value: "t2v", + Effect: corev1.TaintEffectNoExecute, + TolerationSeconds: ptr.To[int64](10), + }, + }, + NodeSelector: map[string]string{"type": "second"}, + }, + }, + }, + ). + Obj() + }, + isValid), + ginkgo.Entry("podSet minCount (negative)", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).SetMinimumCount(-1).Obj(), + ). + Obj() + }, + isInvalid), + ginkgo.Entry("podSet minCount (too big)", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).SetMinimumCount(4).Obj(), + ). + Obj() + }, + isInvalid), + ginkgo.Entry("too many variable count podSets", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).SetMinimumCount(2).Obj(), + *testing.MakePodSet("ps2", 3).SetMinimumCount(1).Obj(), + ). + Obj() + }, + isForbidden), + ) + + ginkgo.DescribeTable("Should have valid values when creating", func(w func() *kueue.Workload, a *kueue.Admission, errorType int) { + workload := w() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + err := util.SetQuotaReservation(ctx, k8sClient, workload, a) + switch errorType { + case isForbidden: + gomega.Expect(err).Should(gomega.HaveOccurred()) + gomega.Expect(err).Should(testing.BeAPIError(testing.ForbiddenError), "error: %v", err) + case isInvalid: + gomega.Expect(err).Should(gomega.HaveOccurred()) + gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err) + default: + gomega.Expect(err).ShouldNot(gomega.HaveOccurred()) + } + }, + ginkgo.Entry("clusterQueue name", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + testing.MakeAdmission("@invalid").Obj(), + isInvalid), + ginkgo.Entry("podSet name in status assignment", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + testing.MakeAdmission("cluster-queue", "@invalid").Obj(), + isInvalid), + ginkgo.Entry("podSets in admission", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("main2", 1).Obj(), + *testing.MakePodSet("main1", 1).Obj(), + ). + Obj() + }, + testing.MakeAdmission("cluster-queue", "main1", "main2", "main3").Obj(), + isInvalid), + ginkgo.Entry("assignment usage should be divisible by count", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("main", 3). + Request(corev1.ResourceCPU, "1"). + Obj(), + ). + Obj() + }, + testing.MakeAdmission("cluster-queue"). + Assignment(corev1.ResourceCPU, "flv", "1"). + AssignmentPodCount(3). + Obj(), + isForbidden), + ) + + ginkgo.DescribeTable("Should have valid values", func(w func() *kueue.Workload, acs kueue.AdmissionCheckState) { + wl := w() + gomega.Expect(k8sClient.Create(ctx, wl)).Should(gomega.Succeed()) + + gomega.Eventually(func() error { + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl), wl)).To(gomega.Succeed()) + workload.SetAdmissionCheckState(&wl.Status.AdmissionChecks, acs) + return k8sClient.Status().Update(ctx, wl) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.ForbiddenError)) + + }, + ginkgo.Entry("podSetUpdates have the same number of podSets", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("first", 1).Obj(), + *testing.MakePodSet("second", 1).Obj(), + ). + Obj() + }, + kueue.AdmissionCheckState{ + Name: "check", + State: kueue.CheckStateReady, + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first"}}}, + ), + ginkgo.Entry("mismatched names in podSetUpdates with names in podSets", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("first", 1).Obj(), + *testing.MakePodSet("second", 1).Obj(), + ). + Obj() + }, + kueue.AdmissionCheckState{ + Name: "check", + State: kueue.CheckStateReady, + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first"}, {Name: "third"}}}, + ), + ginkgo.Entry("invalid label name of podSetUpdate", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + kueue.AdmissionCheckState{ + Name: "check", + State: kueue.CheckStateReady, + PodSetUpdates: []kueue.PodSetUpdate{{Name: "main", Labels: map[string]string{"@abc": "foo"}}}}, + ), + ginkgo.Entry("invalid node selector name of podSetUpdate", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + kueue.AdmissionCheckState{ + Name: "check", + State: kueue.CheckStateReady, + PodSetUpdates: []kueue.PodSetUpdate{{Name: "main", NodeSelector: map[string]string{"@abc": "foo"}}}}, + ), + ginkgo.Entry("invalid label value of podSetUpdate", + func() *kueue.Workload { + return testing.MakeWorkload(workloadName, ns.Name). + Obj() + }, + kueue.AdmissionCheckState{ + Name: "check", + State: kueue.CheckStateReady, + PodSetUpdates: []kueue.PodSetUpdate{{Name: "main", Labels: map[string]string{"foo": "@abc"}}}}, + ), + ) + + ginkgo.It("invalid reclaimablePods", func() { + ginkgo.By("Creating a new Workload") + wl := testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).Obj(), + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, wl)).Should(gomega.Succeed()) + + err := workload.UpdateReclaimablePods(ctx, k8sClient, wl, []kueue.ReclaimablePod{ + {Name: "ps1", Count: 4}, + {Name: "ps2", Count: 1}, + }) + gomega.Expect(err).Should(gomega.HaveOccurred()) + gomega.Expect(err).Should(testing.BeAPIError(testing.ForbiddenError), "error: %v", err) + }) + }) ginkgo.Context("When updating a Workload", func() { @@ -165,7 +517,7 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) newWL.Spec.PodSets[0].Count = 10 return k8sClient.Update(ctx, &newWL) - }, util.Timeout, util.Interval).Should(testing.BeForbiddenError()) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) }) ginkgo.It("Should forbid the change of spec.queueName of an admitted workload", func() { @@ -183,7 +535,7 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) newWL.Spec.QueueName = "queue2" return k8sClient.Update(ctx, &newWL) - }, util.Timeout, util.Interval).Should(testing.BeForbiddenError()) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) }) ginkgo.It("Should forbid the change of spec.admission", func() { @@ -205,7 +557,7 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) newWL.Status.Admission.ClusterQueue = "foo-cluster-queue" return k8sClient.Status().Update(ctx, &newWL) - }, util.Timeout, util.Interval).Should(testing.BeForbiddenError()) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) }) @@ -214,7 +566,7 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { workload := testing.MakeWorkload(workloadName, ns.Name).PriorityClass("priority").Obj() err := k8sClient.Create(ctx, workload) gomega.Expect(err).Should(gomega.HaveOccurred()) - gomega.Expect(errors.IsForbidden(err)).Should(gomega.BeTrue(), "error: %v", err) + gomega.Expect(err).Should(testing.BeAPIError(testing.InvalidError), "error: %v", err) }) ginkgo.It("workload's priority should be mutable when referencing WorkloadPriorityClass", func() { @@ -260,5 +612,434 @@ var _ = ginkgo.Describe("Workload validating webhook", func() { return finalQueueWorkload.Spec.Priority }, util.Timeout, util.Interval).Should(gomega.Equal(&updatedPriority)) }) + + ginkgo.It("Should not be updated when has quota reservation: count", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cq").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status = kueue.WorkloadStatus{} + newWL.Spec.PodSets = []kueue.PodSet{*testing.MakePodSet("main", 2).Obj()} + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("Should not be updated: podSpec", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cq").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PodSets = []kueue.PodSet{{ + Name: "main", + Count: 1, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "c-after", + Resources: corev1.ResourceRequirements{ + Requests: make(corev1.ResourceList), + }, + }, + }, + }, + }, + }} + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("Should be updated when not admitted: queueName", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Queue("q1").Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.QueueName = "q2" + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("Should be updated when admitting: queueName", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.QueueName = "q" + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("Should not be updated once admitted: queueName", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Queue("q1").Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cq").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.QueueName = "q2" + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("Should be updated when admission is reset: queueName", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Queue("q1"). + ReserveQuota(testing.MakeAdmission("cq").Obj()).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.QueueName = "q2" + newWL.Status = kueue.WorkloadStatus{} + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("admission can be set", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status = kueue.WorkloadStatus{ + Admission: testing.MakeAdmission("cluster-queue").Assignment("on-demand", "5", "1").Obj(), + Conditions: []metav1.Condition{{ + Type: kueue.WorkloadQuotaReserved, + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Now()), + Reason: "AdmittedByTest", + Message: "Admitted by ClusterQueue cluster-queue", + }}, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("admission can be unset", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).ReserveQuota( + testing.MakeAdmission("cluster-queue").Assignment("on-demand", "5", "1").Obj(), + ).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status = kueue.WorkloadStatus{} + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("admission should not be updated once set", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cluster-queue").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status.Admission = testing.MakeAdmission("cluster-queue").Assignment("on-demand", "5", "1").Obj() + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("reclaimable pod count can change up", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).PodSets( + *testing.MakePodSet("ps1", 3).Obj(), + *testing.MakePodSet("ps2", 3).Obj(), + ). + ReserveQuota( + testing.MakeAdmission("cluster-queue"). + PodSets(kueue.PodSetAssignment{Name: "ps1"}, kueue.PodSetAssignment{Name: "ps2"}). + Obj(), + ). + ReclaimablePods( + kueue.ReclaimablePod{Name: "ps1", Count: 1}, + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status.ReclaimablePods = []kueue.ReclaimablePod{ + {Name: "ps1", Count: 2}, + {Name: "ps2", Count: 1}, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("reclaimable pod count cannot change down", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).Obj(), + *testing.MakePodSet("ps2", 3).Obj(), + ). + ReclaimablePods( + kueue.ReclaimablePod{Name: "ps1", Count: 2}, + kueue.ReclaimablePod{Name: "ps2", Count: 1}, + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cluster-queue"). + PodSets( + kueue.PodSetAssignment{Name: "ps1"}, + kueue.PodSetAssignment{Name: "ps2"}). + Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status.ReclaimablePods = []kueue.ReclaimablePod{ + {Name: "ps1", Count: 0}, + {Name: "ps2", Count: 1}, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("reclaimable pod count can go to 0 if the job is suspended", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("ps1", 3).Obj(), + *testing.MakePodSet("ps2", 3).Obj(), + ). + ReserveQuota( + testing.MakeAdmission("cluster-queue"). + PodSets(kueue.PodSetAssignment{Name: "ps1"}, kueue.PodSetAssignment{Name: "ps2"}). + Obj(), + ). + ReclaimablePods( + kueue.ReclaimablePod{Name: "ps1", Count: 2}, + kueue.ReclaimablePod{Name: "ps2", Count: 1}, + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status.AdmissionChecks = []kueue.AdmissionCheckState{ + { + PodSetUpdates: []kueue.PodSetUpdate{{Name: "ps1"}, {Name: "ps2"}}, + State: kueue.CheckStateReady, + }, + } + newWL.Status.ReclaimablePods = []kueue.ReclaimablePod{ + {Name: "ps1", Count: 0}, + {Name: "ps2", Count: 1}, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("priorityClassSource should not be updated", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + Queue("q"). + PriorityClass("test-class").PriorityClassSource(constants.PodPriorityClassSource). + Priority(10). + Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cq").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PriorityClassSource = constants.WorkloadPriorityClassSource + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("priorityClassName should not be updated", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + Queue("q"). + PriorityClass("test-class-1").PriorityClassSource(constants.PodPriorityClassSource). + Priority(10). + Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + gomega.Expect(util.SetQuotaReservation(ctx, k8sClient, workload, testing.MakeAdmission("cq").Obj())).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PriorityClassName = "test-class-2" + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.InvalidError)) + }) + + ginkgo.It("podSetUpdates should be immutable when state is ready", func() { + ginkgo.By("Creating a new Workload") + wl := testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("first", 1).Obj(), + *testing.MakePodSet("second", 1).Obj(), + ). + Obj() + gomega.Expect(k8sClient.Create(ctx, wl)).Should(gomega.Succeed()) + + ginkgo.By("Setting admission check state") + gomega.Eventually(func() error { + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl), wl)).To(gomega.Succeed()) + workload.SetAdmissionCheckState(&wl.Status.AdmissionChecks, kueue.AdmissionCheckState{ + Name: "ac1", + Message: "old", + LastTransitionTime: metav1.NewTime(time.Now()), + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first", Labels: map[string]string{"foo": "bar"}}, {Name: "second"}}, + State: kueue.CheckStateReady, + }) + return k8sClient.Status().Update(ctx, wl) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + + ginkgo.By("Updating admission check state") + gomega.Eventually(func() error { + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(wl), wl)).To(gomega.Succeed()) + workload.SetAdmissionCheckState(&wl.Status.AdmissionChecks, kueue.AdmissionCheckState{ + Name: "ac1", + Message: "new", + LastTransitionTime: metav1.NewTime(time.Now()), + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first", Labels: map[string]string{"foo": "baz"}}, {Name: "second"}}, + State: kueue.CheckStateReady, + }) + return k8sClient.Status().Update(ctx, wl) + }, util.Timeout, util.Interval).Should(testing.BeAPIError(testing.ForbiddenError)) + }) + + ginkgo.It("should change other fields of admissionchecks when podSetUpdates is immutable", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + PodSets( + *testing.MakePodSet("first", 1).Obj(), + *testing.MakePodSet("second", 1).Obj(), + ).AdmissionChecks( + kueue.AdmissionCheckState{ + Name: "ac1", + Message: "old", + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first", Labels: map[string]string{"foo": "bar"}}, {Name: "second"}}, + State: kueue.CheckStateReady, + }).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Status.AdmissionChecks = []kueue.AdmissionCheckState{ + { + Name: "ac1", + Message: "new", + LastTransitionTime: metav1.NewTime(time.Now()), + PodSetUpdates: []kueue.PodSetUpdate{{Name: "first", Labels: map[string]string{"foo": "bar"}}, {Name: "second"}}, + State: kueue.CheckStateReady, + }, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("updating priorityClassName before setting reserve quota for workload", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + Queue("q"). + PriorityClass("test-class-1").PriorityClassSource(constants.PodPriorityClassSource). + Priority(10).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PriorityClassName = "test-class-2" + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("updating priorityClassSource before setting reserve quota for workload", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name). + Queue("q"). + PriorityClass("test-class").PriorityClassSource(constants.PodPriorityClassSource). + Priority(10).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PriorityClassSource = constants.WorkloadPriorityClassSource + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + + ginkgo.It("updating podSets before setting reserve quota for workload", func() { + ginkgo.By("Creating a new Workload") + workload := testing.MakeWorkload(workloadName, ns.Name).Obj() + gomega.Expect(k8sClient.Create(ctx, workload)).Should(gomega.Succeed()) + + ginkgo.By("Updating the workload") + gomega.Eventually(func() error { + var newWL kueue.Workload + gomega.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(workload), &newWL)).To(gomega.Succeed()) + newWL.Spec.PodSets = []kueue.PodSet{ + { + Name: "main", + Count: 1, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "c-after", + Resources: corev1.ResourceRequirements{ + Requests: make(corev1.ResourceList), + }, + }, + }, + }, + }, + }, + } + return k8sClient.Update(ctx, &newWL) + }, util.Timeout, util.Interval).Should(gomega.Succeed()) + }) + }) })