Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add CEL rules to ClusterQueue #1972

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Add CEL rules to ClusterQueue #1972

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a09db62
Add CEL rules to ClusterQueue
IrvingMg Apr 10, 2024
7cbcd5d
Set default values with CRD validations
IrvingMg Apr 12, 2024
b0dc005
Update message for validation rule
IrvingMg Apr 12, 2024
66e3640
Remove unecessary checks from crd validations
IrvingMg Apr 16, 2024
e1000f4
Remove test case
IrvingMg Apr 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions apis/kueue/v1beta1/clusterqueue_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
IrvingMg marked this conversation as resolved.
Show resolved Hide resolved
type ClusterQueueSpec struct {
// resourceGroups describes groups of resources.
// Each resource group defines the list of resources and a list of flavors
Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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="!(has(self.reclaimWithinCohort) && self.reclaimWithinCohort == 'Never' && has(self.borrowWithinCohort) && self.borrowWithinCohort.policy != 'Never')", message="reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never"
IrvingMg marked this conversation as resolved.
Show resolved Hide resolved
type ClusterQueuePreemption struct {
// reclaimWithinCohort determines whether a pending Workload can preempt
// Workloads from other ClusterQueues in the cohort that are using more than
Expand Down
23 changes: 23 additions & 0 deletions charts/kueue/templates/crd/kueue.x-k8s.io_clusterqueues.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
Expand Down Expand Up @@ -274,6 +276,11 @@ spec:
- LowerOrNewerEqualPriority
type: string
type: object
x-kubernetes-validations:
- message: reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never
rule: '!(has(self.reclaimWithinCohort) && self.reclaimWithinCohort
== ''Never'' && has(self.borrowWithinCohort) && self.borrowWithinCohort.policy
!= ''Never'')'
queueingStrategy:
default: BestEffortFIFO
description: |-
Expand Down Expand Up @@ -330,6 +337,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: |-
Expand Down Expand Up @@ -417,6 +426,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
Expand All @@ -439,6 +452,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:
Expand Down Expand Up @@ -531,6 +550,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
Expand Down Expand Up @@ -583,6 +604,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
Expand Down
4 changes: 4 additions & 0 deletions charts/kueue/templates/crd/kueue.x-k8s.io_localqueues.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions charts/kueue/templates/crd/kueue.x-k8s.io_workloads.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
23 changes: 23 additions & 0 deletions config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
Expand Down Expand Up @@ -259,6 +261,11 @@ spec:
- LowerOrNewerEqualPriority
type: string
type: object
x-kubernetes-validations:
- message: reclaimWithinCohort=Never and borrowWithinCohort.Policy!=Never
rule: '!(has(self.reclaimWithinCohort) && self.reclaimWithinCohort
== ''Never'' && has(self.borrowWithinCohort) && self.borrowWithinCohort.policy
!= ''Never'')'
queueingStrategy:
default: BestEffortFIFO
description: |-
Expand Down Expand Up @@ -315,6 +322,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: |-
Expand Down Expand Up @@ -402,6 +411,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
Expand All @@ -424,6 +437,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:
Expand Down Expand Up @@ -516,6 +535,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
Expand Down Expand Up @@ -568,6 +589,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
Expand Down
4 changes: 4 additions & 0 deletions config/components/crd/bases/kueue.x-k8s.io_localqueues.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions config/components/crd/bases/kueue.x-k8s.io_workloads.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
22 changes: 1 addition & 21 deletions pkg/webhooks/clusterqueue_webhook.go
View file
Open in desktop
IrvingMg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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]()
Expand Down Expand Up @@ -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) {
Expand All @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quantities can be validated via CEL too https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-quantity-library

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this table https://kubernetes.io/docs/reference/using-api/cel/#cel-options-language-features-and-libraries quantity library requires Kubernetes 1.29 -although then, in the quantity subsection states 1.28-, is it safe to replace the quantity validations with CEL? Wouldn't introduce breaking changes for the supported versions earlier than 1.29?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see. I missed that.
Can you leave an open issue to change this when 1.28 reaches EoL?

Expand Down
Loading