Add validation for Workload via webhook #225

knight42 · 2022-04-20T03:55:58Z

Signed-off-by: Jian Zeng [email protected]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add basic validating webhook for Workload.

Which issue(s) this PR fixes:

Part of #171

Special notes for your reviewer:

knight42 · 2022-04-20T03:56:26Z

/cc @ahg-g @kerthcet

k8s-ci-robot · 2022-04-20T03:56:28Z

@knight42: GitHub didn't allow me to request PR reviews from the following users: kerthcet.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ahg-g @kerthcet

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.

kerthcet · 2022-04-20T06:28:29Z

apis/kueue/v1alpha1/workload_webhook.go

 }

 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateDelete() error {
 	return nil
 }
+
+func validateWorkload(obj *Workload) field.ErrorList {


We should also validate the PriorityClassName, it should be a DNS subdomain, although we ensured this in JobController, but we will have multi implements in the future, it can prevent the problem as early as possible.

Should we also make sure the queue name shouldn't be empty as I asked before. I think so if we don't have default ones. @ahg-g

Should we also make sure the queue name shouldn't be empty as I asked before. I think so if we don't have default ones.

How about if the user create the default queue after the workload created ?

I think we should not fail the Workload creation if the queue name is missing. This is because a user could create a Job without a queue annotation and decide to add it later. We need the Workload object to post an Event that indicates why the workload is not schedulable.

test/integration/webhook/v1alpha1/workload_test.go

denkensk

Thanks @knight42
some comments.

denkensk · 2022-04-20T11:37:25Z

apis/kueue/v1alpha1/workload_webhook.go

 }

 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateDelete() error {
 	return nil
 }
+
+func validateWorkload(obj *Workload) field.ErrorList {


Should we also make sure the queue name shouldn't be empty as I asked before. I think so if we don't have default ones.

How about if the user create the default queue after the workload created ?

test/integration/webhook/v1alpha1/workload_test.go

denkensk · 2022-04-20T11:43:17Z

apis/kueue/v1alpha1/workload_webhook.go

+	for i, podSet := range obj.Spec.PodSets {
+		if podSet.Count <= 0 {
+			allErrs = append(allErrs, field.Invalid(
+				field.NewPath("spec").Child("podSets").Index(i).Child("count"),


Can you also check the priority in workload?

Yeah of course, the highest priority for user defined priority class is 1 biliion, should we take predefined system priority classes into account as well?

test/integration/webhook/v1alpha1/workload_test.go

apis/kueue/v1alpha1/workload_webhook.go

alculquicondor · 2022-04-25T20:25:34Z

/assign

alculquicondor · 2022-04-25T20:31:47Z

/retitle Add validation for Workload via webhook

alculquicondor · 2022-04-25T20:37:46Z

apis/kueue/v1alpha1/workload_webhook.go

 }

 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
 func (r *Workload) ValidateDelete() error {
 	return nil
 }
+
+func validateWorkload(obj *Workload) field.ErrorList {


I think we should not fail the Workload creation if the queue name is missing. This is because a user could create a Job without a queue annotation and decide to add it later. We need the Workload object to post an Event that indicates why the workload is not schedulable.

apis/kueue/v1alpha1/workload_webhook.go

test/integration/webhook/v1alpha1/workload_test.go

apis/kueue/v1alpha1/workload_webhook.go

test/integration/webhook/v1alpha1/workload_test.go

alculquicondor · 2022-04-27T19:20:50Z

apis/kueue/v1alpha1/workload_webhook_test.go

+)
+
+// makeWorkload is copied from "sigs.k8s.io/kueue/pkg/util/testing"
+// NOTE: we could not import "sigs.k8s.io/kueue/pkg/util/testing" because of cyclic import.


I think you can avoid the circular dependency doing this: https://github.com/golang/go/wiki/CodeReviewComments#import-dot

@alculquicondor Hi, I have switched to dot import, but it seems a workaround to me, should me move the validation logic to a separate package, such as pkg/validation?

yeah, that's actually a better idea.

Let's do it in apis/kueue/v1alpha1/validation

hmm I just realize this won't work, the circular dependency still persists: the package apis/kueue/v1alpha1/validation imports the Worload struct from apis/kueue/v1alpha1, and apis/kueue/v1alpha1 imports apis/kueue/v1alpha1/validation for the validator. The root cause is that the validator function is called inside apis/kueue/v1alpha1 package.

Workload doesn't have to implement the Defaulter or Validator interface. You could create a new type for it that lives in a webhooks package.

Regarding of the definition of Defaulter or Validator interface:

type Defaulter interface { runtime.Object Default() } type Validator interface { runtime.Object ValidateCreate() error ValidateUpdate(old runtime.Object) error ValidateDelete() error }

it seems these interfaces could only be implements on the Workload struct, otherwise we could not access the fields.

ahhh, I missed that they depend on runtime.Object 😞

I guess we have to go the dotimport route then.

apis/kueue/v1alpha1/workload_webhook_test.go

test/integration/webhook/v1alpha1/workload_test.go

ahg-g · 2022-05-05T01:26:06Z

@knight42 do you need help moving forward with this PR?

knight42 · 2022-05-05T15:30:40Z

@ahg-g Hi! Sorry for the late response, I just return from vacation, I am discussing with @alculquicondor about the implmenetation details: #225 (comment). It would be nice if I could hear your advice!

ahg-g · 2022-05-05T15:33:15Z

@ahg-g Hi! Sorry for the late response, I just return from vacation, I am discussing with @alculquicondor about the implmenetation details: #225 (comment).

Thank a lot @knight42 , I was just wondering if there is something you needed help with, I hope you had a great vacation :)

It would be nice if I could hear your advice!

I will leave this to Aldo just so you get consistent feedback.

alculquicondor · 2022-05-05T15:35:56Z

apis/kueue/v1alpha1/workload_webhook_test.go

@@ -0,0 +1,57 @@
+// Rename the package to avoid circular dependencies which is caused by "sigs.k8s.io/kueue/pkg/util/testing".
+// See also: https://github.com/golang/go/wiki/CodeReviewComments#import-dot


This file is missing the copyright notice.

Also, a comment just up from the package name makes it part of the package doc. Can you move it bellow?

alculquicondor · 2022-05-05T15:38:18Z

pkg/util/testing/wrappers.go

+	return w
+}
+
+func (w *WorkloadWrapper) PodSetCount(count int32) *WorkloadWrapper {


This is way too specific. Just use PodSets to pass a list with the necessary count.

alculquicondor · 2022-05-05T15:40:22Z

test/integration/webhook/v1alpha1/workload_test.go

+			err := k8sClient.Create(ctx, workload)
+			gomega.Expect(err).Should(gomega.HaveOccurred())
+			status := err.(*errors.StatusError).Status()
+			gomega.Expect(status).Should(testing.Equal(metav1.Status{


It would be clearer to just use errors.IsInvalid.

alculquicondor

Awesome

/approve

Can you please squash the commits?

k8s-ci-robot · 2022-05-05T18:11:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, knight42

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

Signed-off-by: Jian Zeng <[email protected]>

alculquicondor

/lgtm

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 20, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk April 20, 2022 03:56

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 20, 2022

k8s-ci-robot requested a review from ahg-g April 20, 2022 03:56

kerthcet reviewed Apr 20, 2022

View reviewed changes

denkensk reviewed Apr 20, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 20, 2022

knight42 requested a review from denkensk April 20, 2022 15:32

kerthcet reviewed Apr 21, 2022

View reviewed changes

k8s-ci-robot assigned alculquicondor Apr 25, 2022

k8s-ci-robot changed the title ~~feat: add validating webhook for Workload~~ Add validation for Workload via webhook Apr 25, 2022

alculquicondor reviewed Apr 25, 2022

View reviewed changes

knight42 requested a review from alculquicondor April 27, 2022 17:07

alculquicondor reviewed Apr 27, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2022

knight42 force-pushed the feat/validate-webhook branch from 8335f02 to 977d6b7 Compare April 30, 2022 12:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2022

knight42 force-pushed the feat/validate-webhook branch from 977d6b7 to 9409e57 Compare April 30, 2022 12:15

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2022

knight42 force-pushed the feat/validate-webhook branch from 9409e57 to 0053275 Compare May 1, 2022 12:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 1, 2022

alculquicondor reviewed May 5, 2022

View reviewed changes

knight42 requested a review from alculquicondor May 5, 2022 16:11

alculquicondor reviewed May 5, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2022

feat: add validating webhook for Workload

e5d79ab

Signed-off-by: Jian Zeng <[email protected]>

knight42 force-pushed the feat/validate-webhook branch from e8d2cc3 to e5d79ab Compare May 6, 2022 04:21

knight42 requested a review from alculquicondor May 6, 2022 04:24

alculquicondor reviewed May 6, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 6, 2022

k8s-ci-robot merged commit 4bd0feb into kubernetes-sigs:main May 6, 2022

knight42 deleted the feat/validate-webhook branch May 15, 2022 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation for Workload via webhook #225

Add validation for Workload via webhook #225

knight42 commented Apr 20, 2022

knight42 commented Apr 20, 2022

k8s-ci-robot commented Apr 20, 2022

kerthcet Apr 20, 2022

kerthcet Apr 20, 2022

denkensk Apr 20, 2022

alculquicondor Apr 25, 2022

denkensk left a comment

denkensk Apr 20, 2022

denkensk Apr 20, 2022

knight42 Apr 20, 2022

alculquicondor commented Apr 25, 2022

alculquicondor commented Apr 25, 2022

alculquicondor Apr 25, 2022

alculquicondor Apr 27, 2022

knight42 Apr 30, 2022

alculquicondor May 2, 2022

knight42 May 2, 2022

alculquicondor May 2, 2022

knight42 May 5, 2022

alculquicondor May 5, 2022

ahg-g commented May 5, 2022

knight42 commented May 5, 2022

ahg-g commented May 5, 2022

alculquicondor May 5, 2022

alculquicondor May 5, 2022

alculquicondor May 5, 2022

alculquicondor left a comment

k8s-ci-robot commented May 5, 2022

alculquicondor left a comment

		@@ -0,0 +1,57 @@
		// Rename the package to avoid circular dependencies which is caused by "sigs.k8s.io/kueue/pkg/util/testing".
		// See also: https://github.com/golang/go/wiki/CodeReviewComments#import-dot

Add validation for Workload via webhook #225

Add validation for Workload via webhook #225

Conversation

knight42 commented Apr 20, 2022

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:

knight42 commented Apr 20, 2022

k8s-ci-robot commented Apr 20, 2022

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

denkensk left a 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

alculquicondor commented Apr 25, 2022

alculquicondor commented Apr 25, 2022

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

ahg-g commented May 5, 2022

knight42 commented May 5, 2022

ahg-g commented May 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alculquicondor left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 5, 2022

alculquicondor left a comment

Choose a reason for hiding this comment