-
Notifications
You must be signed in to change notification settings - Fork 249
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.
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
Add CEL rules to ClusterQueue #1972
Add CEL rules to ClusterQueue #1972
Conversation
Hi @IrvingMg. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/assign |
8cd72b7
to
a09db62
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit.
LGTM otherwise
@@ -74,6 +77,7 @@ type ClusterQueueSpec struct { | |||
|
|||
// flavorFungibility defines whether a workload should try the next flavor | |||
// before borrowing or preempting in the flavor being evaluated. | |||
// +kubebuilder:default={} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Just making the field not a pointer will do this and we can remove some nil checks in the rest of the code-base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this change be applied to every field where +kubebuilder:default={}
was added? Because we could change the pointers on spec.preemption
and spec.preemption.borrowWithinCohort
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes if we'll default them to empty, in my opinion.
@alculquicondor WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can make it a non-pointer when we migrate to v1beta2 or v1.
@@ -74,6 +77,7 @@ type ClusterQueueSpec struct { | |||
|
|||
// flavorFungibility defines whether a workload should try the next flavor | |||
// before borrowing or preempting in the flavor being evaluated. | |||
// +kubebuilder:default={} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can make it a non-pointer when we migrate to v1beta2 or v1.
@@ -191,7 +155,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") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
/lgtm |
LGTM label has been added. Git tree hash: 3ef7bb460335de62e6fabb58ff75d10b51bc64e7
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, IrvingMg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Add CEL rules to ClusterQueue * Set default values with CRD validations * Update message for validation rule * Remove unecessary checks from crd validations * Remove test case
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
It replaces some of the validations executed by webhooks for the
ClusterQueue
type with CRD validation rules.Which issue(s) this PR fixes:
Relates to #463
Special notes for your reviewer:
Only validation rules with a cost within the API server limits have been added.
Does this PR introduce a user-facing change?