Add CEL rules to Workload #2008

IrvingMg · 2024-04-18T14:39:36Z

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 Workload type with CRD validation rules.

Which issue(s) this PR fixes:

Fixes #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?

Added CRD validation rules to Workload.

ACTION REQUIRED: Requires Kubernetes 1.25 or newer

k8s-ci-robot · 2024-04-18T14:39:45Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

netlify · 2024-04-18T14:39:51Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`415523a`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/662bfb2a0feede000804799f

trasc · 2024-04-19T07:23:57Z

/ok-to-test
/assign

pkg/webhooks/workload_webhook_test.go

test/integration/webhook/workload_test.go

trasc

/lgtm

k8s-ci-robot · 2024-04-23T09:39:05Z

LGTM label has been added.

Git tree hash: 7f9bd6f6022944ae873a72fa9ea393f4f55c5dab

test/integration/webhook/workload_test.go

alculquicondor · 2024-04-23T17:54:42Z

apis/kueue/v1beta1/workload_types.go

@@ -336,6 +359,11 @@ 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"


spec and status always exist. Apply the same for all rules.

.conditions.exist should already cover for has, I would think.

Suggested change

// +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="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"

Dropping has(self.status) check causes most of the workload tests to fail due to unexisting status field. For example this is one of the test cases which fails when skipping the status check.

An alternative could be using CEL optional types, that way we can leave just has(oldSelf.?status.conditions). However, optional types require Kubernetes 1.29.

Maybe we can just remove the spec checks for now and for status add this issue to #1986?

Interesting. I guess it's because of the omitempty for status. But yes, let's drop has(self.spec) and has (self.status.conditions)

I've removed has(self.spec) successfully, however, it seems we still need has (self.status.conditions) since self.status.conditions.exists() returns the error no such key: conditions when conditions are empty.

Ah gotcha.

I still see has(oldSelf.spec), however.

The updated rules without the spec check should be visible now :)

pkg/webhooks/workload_webhook.go

alculquicondor · 2024-04-25T14:52:31Z

apis/kueue/v1beta1/workload_types.go

@@ -336,6 +359,11 @@ 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"


Interesting. I guess it's because of the omitempty for status. But yes, let's drop has(self.spec) and has (self.status.conditions)

test/integration/webhook/workload_test.go

alculquicondor · 2024-04-25T15:01:19Z

test/integration/webhook/workload_test.go

+						PodSets(
+							*testing.MakePodSet("first", 1).Obj(),
+							*testing.MakePodSet("second", 1).Obj(),
+						).


Why not pass the admission check through the wrapper?

Since we're using the function workload.SetAdmissionCheckState(), I haven´t found a wrapper that returns a kueue.AdmissionCheckState. As I see in other cases, to set an admission check state, we pass the object directly as an argument.

apis/kueue/v1beta1/workload_types.go

IrvingMg · 2024-04-26T19:21:33Z

/test pull-kueue-test-e2e-main-1-29

alculquicondor

/lgtm
/approve

Thanks!

k8s-ci-robot · 2024-04-26T20:11:47Z

LGTM label has been added.

Git tree hash: 3eb6bcc3ce5cdd6e75f5292102b96fe8d69ba42a

k8s-ci-robot · 2024-04-26T20:11:48Z

[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:

~~OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from mimowo and trasc April 18, 2024 14:39

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2024

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 18, 2024

Add CEL rules to Workload

eb974eb

IrvingMg force-pushed the add-workload-cel-rules branch from 7e75cde to eb974eb Compare April 18, 2024 18:53

IrvingMg marked this pull request as ready for review April 18, 2024 18:55

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 18, 2024

k8s-ci-robot requested review from alculquicondor and kerthcet April 18, 2024 18:55

k8s-ci-robot assigned trasc Apr 19, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 19, 2024

Remove workload defaulter

6176fd1

trasc reviewed Apr 19, 2024

View reviewed changes

IrvingMg added 2 commits April 19, 2024 16:58

Remove test cases validated by CEL rules

3d7c27c

Update test descriptions

06a6910

trasc reviewed Apr 23, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024

alculquicondor reviewed Apr 23, 2024

View reviewed changes

alculquicondor reviewed Apr 24, 2024

View reviewed changes

pkg/webhooks/workload_webhook.go Show resolved Hide resolved

Refactoring integration tests

36e3078

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2024

k8s-ci-robot requested a review from trasc April 24, 2024 15:54

IrvingMg added 2 commits April 24, 2024 23:05

Restore validateAdmissionUpdate function

4cc0c19

Restore defaulting for minCount

8be7044

alculquicondor reviewed Apr 25, 2024

View reviewed changes

IrvingMg mentioned this pull request Apr 25, 2024

REQUEST: New membership for IrvingMg kubernetes/org#4907

Closed

11 tasks

IrvingMg added 2 commits April 26, 2024 17:16

Simplify error checks

c820847

Update workload cel rules

38d9750

IrvingMg force-pushed the add-workload-cel-rules branch from 0573521 to 38d9750 Compare April 26, 2024 16:57

alculquicondor reviewed Apr 26, 2024

View reviewed changes

apis/kueue/v1beta1/workload_types.go Outdated Show resolved Hide resolved

Restore podSets immutability validation via webhook

415523a

alculquicondor reviewed Apr 26, 2024

View reviewed changes

k8s-ci-robot assigned alculquicondor Apr 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2024

k8s-ci-robot merged commit 5401a3b into kubernetes-sigs:main Apr 26, 2024
15 checks passed

k8s-ci-robot added this to the v0.7 milestone Apr 26, 2024

IrvingMg mentioned this pull request Apr 29, 2024

Use quantity library and optional types once 1.28 reaches EoL #1986

Open

IrvingMg deleted the add-workload-cel-rules branch May 8, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CEL rules to Workload #2008

Add CEL rules to Workload #2008

IrvingMg commented Apr 18, 2024 •

edited

k8s-ci-robot commented Apr 18, 2024

netlify bot commented Apr 18, 2024 •

edited

trasc commented Apr 19, 2024

trasc left a comment

k8s-ci-robot commented Apr 23, 2024

alculquicondor Apr 23, 2024

IrvingMg Apr 25, 2024 •

edited

alculquicondor Apr 25, 2024

IrvingMg Apr 26, 2024

alculquicondor Apr 26, 2024

IrvingMg Apr 26, 2024

alculquicondor Apr 25, 2024

alculquicondor Apr 25, 2024

IrvingMg Apr 26, 2024

IrvingMg commented Apr 26, 2024

alculquicondor left a comment

k8s-ci-robot commented Apr 26, 2024

k8s-ci-robot commented Apr 26, 2024

	// +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="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"

Add CEL rules to Workload #2008

Add CEL rules to Workload #2008

Conversation

IrvingMg commented Apr 18, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 18, 2024

netlify bot commented Apr 18, 2024 • edited

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

trasc commented Apr 19, 2024

trasc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 23, 2024

Choose a reason for hiding this comment

IrvingMg Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IrvingMg commented Apr 26, 2024

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 26, 2024

k8s-ci-robot commented Apr 26, 2024

IrvingMg commented Apr 18, 2024 •

edited

netlify bot commented Apr 18, 2024 •

edited

IrvingMg Apr 25, 2024 •

edited