Add defaulting webhook for `Workload` resource #220

knight42 · 2022-04-14T12:01:28Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add basic defaulting webhook for Workload resource.

Which issue(s) this PR fixes:

Part of #171

Special notes for your reviewer:

Run the command ``` kubebuilder create webhook --group kueue --version v1alpha1 --kind Workload --defaulting --programmatic-validation ``` And follow the instructions described in https://book.kubebuilder.io/cronjob-tutorial/running-webhook.html.

knight42 · 2022-04-14T12:01:47Z

/cc @ahg-g @alculquicondor

apis/kueue/v1alpha1/webhook_suite_test.go

apis/kueue/v1alpha1/workload_webhook.go

apis/kueue/v1alpha1/workload_webhook_test.go

apis/kueue/v1alpha1/workload_webhook.go

apis/kueue/v1alpha1/webhook_suite_test.go

kerthcet · 2022-04-15T02:45:45Z

apis/kueue/v1alpha1/workload_webhook.go

+var _ webhook.Validator = &Workload{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+func (r *Workload) ValidateCreate() error {


Let's add some simple validation here like PodSet.Count should be positive.

Could we resolve this in a follow-up PR?

I think this type of validation can be done using kubebuilder markers on the field;

// +kubebuilder:validation:Minimum=0

but I am not sure if it has the same limitations as // +kubebuilder:default=main;

knight42 · 2022-04-15T16:27:00Z

@ahg-g Hi! Thanks for your detailed review, I think your comments have been addressed, would you please take another look?

ahg-g

Thanks, this looks nice!

apis/kueue/v1alpha1/workload_webhook.go

test/integration/framework/framework.go

test/integration/webhook/v1alpha1/suite_test.go

test/integration/webhook/v1alpha1/workload_test.go

ahg-g

Thanks, this looks nice!

ahg-g

thanks, you can squash.

apis/kueue/v1alpha1/workload_webhook.go

ahg-g · 2022-04-17T18:24:40Z

apis/kueue/v1alpha1/workload_webhook.go

+var _ webhook.Validator = &Workload{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+func (r *Workload) ValidateCreate() error {


I think this type of validation can be done using kubebuilder markers on the field;

// +kubebuilder:validation:Minimum=0

but I am not sure if it has the same limitations as // +kubebuilder:default=main;

apis/kueue/v1alpha1/workload_webhook.go

ahg-g · 2022-04-19T16:14:31Z

last question, did you try to deploy this on a real cluster?

/lgtm
/approve
/hold

feel free to cancel the hold

k8s-ci-robot · 2022-04-19T16:14:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, 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~~ [ahg-g]

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

knight42 · 2022-04-20T02:49:01Z

last question, did you try to deploy this on a real cluster?

@ahg-g Yes, and it works well.

$ make deploy IMAGE_REGISTRY=knight42
/Users/knight/github/kueue/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
cd config/manager && /Users/knight/github/kueue/bin/kustomize edit set image controller=knight42/kueue:0.2.0-devel-9-g64b24d7
/Users/knight/github/kueue/bin/kustomize build config/default | kubectl apply -f -
namespace/kueue-system created
customresourcedefinition.apiextensions.k8s.io/clusterqueues.kueue.x-k8s.io created
customresourcedefinition.apiextensions.k8s.io/queues.kueue.x-k8s.io created
customresourcedefinition.apiextensions.k8s.io/resourceflavors.kueue.x-k8s.io created
customresourcedefinition.apiextensions.k8s.io/workloads.kueue.x-k8s.io created
serviceaccount/kueue-controller-manager created
role.rbac.authorization.k8s.io/kueue-leader-election-role created
clusterrole.rbac.authorization.k8s.io/kueue-batch-admin-role created
clusterrole.rbac.authorization.k8s.io/kueue-batch-user-role created
clusterrole.rbac.authorization.k8s.io/kueue-clusterqueue-editor-role created
clusterrole.rbac.authorization.k8s.io/kueue-clusterqueue-viewer-role created
clusterrole.rbac.authorization.k8s.io/kueue-job-editor-role created
clusterrole.rbac.authorization.k8s.io/kueue-job-viewer-role created
clusterrole.rbac.authorization.k8s.io/kueue-manager-role created
clusterrole.rbac.authorization.k8s.io/kueue-metrics-reader created
clusterrole.rbac.authorization.k8s.io/kueue-proxy-role created
clusterrole.rbac.authorization.k8s.io/kueue-queue-editor-role created
clusterrole.rbac.authorization.k8s.io/kueue-queue-viewer-role created
clusterrole.rbac.authorization.k8s.io/kueue-resourceflavor-editor-role created
clusterrole.rbac.authorization.k8s.io/kueue-resourceflavor-viewer-role created
clusterrole.rbac.authorization.k8s.io/kueue-workload-editor-role created
clusterrole.rbac.authorization.k8s.io/kueue-workload-viewer-role created
rolebinding.rbac.authorization.k8s.io/kueue-leader-election-rolebinding created
clusterrolebinding.rbac.authorization.k8s.io/kueue-manager-rolebinding created
clusterrolebinding.rbac.authorization.k8s.io/kueue-proxy-rolebinding created
configmap/kueue-manager-config created
service/kueue-controller-manager-metrics-service created
service/kueue-webhook-service created
deployment.apps/kueue-controller-manager created
certificate.cert-manager.io/kueue-serving-cert created
issuer.cert-manager.io/kueue-selfsigned-issuer created
mutatingwebhookconfiguration.admissionregistration.k8s.io/kueue-mutating-webhook-configuration created
validatingwebhookconfiguration.admissionregistration.k8s.io/kueue-validating-webhook-configuration created

$ cat <<EOF | kubectl create --validate=false -f -
apiVersion: kueue.x-k8s.io/v1alpha1
kind: Workload
metadata:
  namespace: default
  name: foo
spec:
  queueName: test
  podSets:
  - count: 1
    spec:
      containers:
      - image: nginx:alpine
        name: nginx
EOF

$ kubectl get workload foo -oyaml
apiVersion: kueue.x-k8s.io/v1alpha1
kind: Workload
metadata:
  creationTimestamp: "2022-04-20T02:45:34Z"
  generation: 1
  name: foo
  namespace: default
  resourceVersion: "1557103"
  uid: b74f5db6-ef73-4365-8e34-1d490ced82d2
spec:
  podSets:
  - count: 1
    name: main # <- The defaulting webhook has filled the default podSet name
    spec:
      containers:
      - image: nginx:alpine
        name: nginx
        resources: {}
  queueName: test
status:
  conditions:
  - lastProbeTime: "2022-04-20T02:45:34Z"
    lastTransitionTime: "2022-04-20T02:45:34Z"
    message: Queue test doesn't exist
    reason: Inadmissible
    status: "False"
    type: Admitted

knight42 · 2022-04-20T02:49:15Z

/hold cancel

alculquicondor · 2022-04-26T15:30:53Z

This now requires installing cert-manager. Could you add it to the documentation? Are there alternatives?

feat: add generate manifests

153bd07

Run the command ``` kubebuilder create webhook --group kueue --version v1alpha1 --kind Workload --defaulting --programmatic-validation ``` And follow the instructions described in https://book.kubebuilder.io/cronjob-tutorial/running-webhook.html.

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 14, 2022

k8s-ci-robot requested review from ArangoGutierrez and denkensk April 14, 2022 12:01

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 14, 2022

k8s-ci-robot requested review from ahg-g and alculquicondor April 14, 2022 12:01

knight42 changed the title ~~Feat/webhook~~ Add defaulting webhook for Workload resource Apr 14, 2022

ahg-g reviewed Apr 14, 2022

View reviewed changes

kerthcet reviewed Apr 15, 2022

View reviewed changes

knight42 requested a review from ahg-g April 15, 2022 16:25

knight42 force-pushed the feat/webhook branch from 0b02a70 to aac95d7 Compare April 15, 2022 17:40

ahg-g reviewed Apr 16, 2022

View reviewed changes

knight42 force-pushed the feat/webhook branch from bbb0d4c to ee052af Compare April 16, 2022 14:34

knight42 requested a review from ahg-g April 16, 2022 14:43

ahg-g reviewed Apr 17, 2022

View reviewed changes

feat: add generated webhook code

64b24d7

knight42 force-pushed the feat/webhook branch from e03f989 to 64b24d7 Compare April 18, 2022 08:30

knight42 requested a review from ahg-g April 18, 2022 08:31

kerthcet reviewed Apr 18, 2022

View reviewed changes

apis/kueue/v1alpha1/workload_webhook.go Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 19, 2022

k8s-ci-robot assigned ahg-g Apr 19, 2022

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

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 20, 2022

k8s-ci-robot merged commit 9968361 into kubernetes-sigs:main Apr 20, 2022

knight42 deleted the feat/webhook branch April 20, 2022 02:52

alculquicondor mentioned this pull request Apr 26, 2022

Manage webhook cert internally #232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add defaulting webhook for `Workload` resource #220

Add defaulting webhook for `Workload` resource #220

knight42 commented Apr 14, 2022 •

edited by ahg-g

Loading

knight42 commented Apr 14, 2022

kerthcet Apr 15, 2022

knight42 Apr 15, 2022

ahg-g Apr 17, 2022

knight42 commented Apr 15, 2022

ahg-g left a comment

ahg-g left a comment

ahg-g left a comment

ahg-g Apr 17, 2022

ahg-g commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

knight42 commented Apr 20, 2022

knight42 commented Apr 20, 2022

alculquicondor commented Apr 26, 2022

Add defaulting webhook for Workload resource #220

Add defaulting webhook for Workload resource #220

Conversation

knight42 commented Apr 14, 2022 • edited by ahg-g Loading

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 14, 2022

kerthcet Apr 15, 2022

Choose a reason for hiding this comment

knight42 Apr 15, 2022

Choose a reason for hiding this comment

ahg-g Apr 17, 2022

Choose a reason for hiding this comment

knight42 commented Apr 15, 2022

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

ahg-g Apr 17, 2022

Choose a reason for hiding this comment

ahg-g commented Apr 19, 2022

k8s-ci-robot commented Apr 19, 2022

knight42 commented Apr 20, 2022

knight42 commented Apr 20, 2022

alculquicondor commented Apr 26, 2022

Add defaulting webhook for `Workload` resource #220

Add defaulting webhook for `Workload` resource #220

knight42 commented Apr 14, 2022 •

edited by ahg-g

Loading