Add webhook for APIs defaulting and validation

[ahg-g]

A webhook for APIs defaulting and validation; see example: https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation.html

[knight42]

@ahg-g Hi! I would like to volunteer!
/assign

Could you please tell me the defaulting rule and the validation rule, such as which values needs defaulting?

[alculquicondor]

In terms of defaulting, we are using CRD defaults, like this one:

kueue/apis/core/v1alpha1/workload_types.go

Line 66 in f8e104a

// +kubebuilder:default=main

That works if you don't set it through a yaml. But I think it doesn't work if you submit the object through the client, like we do in

kueue/pkg/controller/workload/job/job_controller.go

Line 330 in 425bf7f

if err = r.client.Create(ctx, wl); err != nil {

So you could start adding those defaults.

Next, we can look into validation. We don't have an exhaustive list yet, but you could start validating all the enums and integers (most of them should only be positive). You can go through all the APIs and check what makes sense to validate.

[knight42]

@alculquicondor Hi! Thanks for your comments, that would definitely be a good start!

I am trying to follow the steps described in https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation.html to scaffold the webhook, but I failed:

$ git diff PROJECT
diff --git PROJECT PROJECT
index bab428b..93389ef 100644
--- PROJECT
+++ PROJECT
@@ -12,7 +12,7 @@ resources:
   domain: x-k8s.io
   group: kueue
   kind: Queue
-  path: sigs.k8s.io/kueue/api/v1alpha1
+  path: sigs.k8s.io/kueue/apis/core/v1alpha1
   version: v1alpha1
 - api:
     crdVersion: v1

$ kubebuilder create webhook --group kueue --version v1alpha1 --kind Queue --defaulting --programmatic-validation
Writing kustomize manifests for you to edit...
Error: failed to create webhook: unable to scaffold with "kustomize.common.kubebuilder.io/v1": error updating resource: unable to update Resource (Path "sigs.k8s.io/kueue/apis/core/v1alpha1") with another with non-matching Path "sigs.k8s.io/kueue/apis/kueue/v1alpha1"
...

It seems the current project layout confused kubebuilder, shall we rename apis/core to apis/kueue? Or am I missing something?

[alculquicondor]

It seems that we didn't do a good job keeping the PROJECT file in-sync with the renames. It looks like you have to change:

group: core

[ahg-g]

Changing to the group to core is going to produce core.x-k8s.io as the group name for future apis (including crd file names) instead of kueue.x-k8s.io; so I don't think we should change the group.

perhaps only update the paths.

[ahg-g]

I think for kubebuilder tool to work smoothly we indeed need to move core to kueue, which sounds redundant, but might unfortunately be necessary if we want to continue to rely on the kubebuilder tool

[alculquicondor]

Opened #219

[ahg-g]

Now that we started with Workload, do we believe we are done with defaulting and validation for that resource? Should some fields be marked immutable (e.g., PodSets)?

[alculquicondor]

as a hole? Are we sold on the idea of not adding the node affinities to the Workload, but add them directly to the Job?

[ahg-g]

Are we sold on the idea of not adding the node affinities to the Workload

Is there a setup where we would consider adding affinities to the workload itself?

[alculquicondor]

I'm trying to think of ways to avoid the custom controllers to look at the ResourceFlavors and only look at the Workload.

Although, just directly modifying the Workload's pod specs is probably not the best idea. Maybe at admission time we can add the node labels somewhere in the .spec.admission struct.

So, you are right, making the PodSpecs immutable makes sense.

[ahg-g]

Adding the labels to spec.admission makes sense I think.

ok, lets round up the workload validation with making the spec immutable and then perhaps we focus on the next one, queue is probably the easier?

@knight42 would you like to send a PR validating that the PodSet is immutable?

[knight42]

@knight42 would you like to send a PR validating that the PodSet is immutable?

Yeah I am glad to do that!

[alculquicondor]

Did you already add a webhook for Queue? It would be useful to make .spec.clusterQueue immutable.

[knight42]

Did you already add a webhook for Queue? It would be useful to make .spec.clusterQueue immutable.

Nope, now there is only webhook for Workload. I will add it in upcoming days.

[alculquicondor]

We should also validate Workload updates. We probably want to make most of the fields immutable. Also, we should disallow changing the queue of an Admitted workload.

[ahg-g]

To track what is left, lets try and list the validations we want to do:

• Workload validation

1. Queue and Admitted fields are immutable on update once set.

• Queue validation feat: implement validation webhook for Queue #264

1. ClusterQueue is a valid name.
2. ClusterQueue is immutable

• ClusterQueue validation

1. Two resources can either have completely different flavors or they must share all flavors and their order.
2. Resource name is a valid name
3. NamespaceSelector is a valid selector
4. Quota: max > min
5. Resource flavor name is a valid name

• ResourceFlavor validation Add the finalizer via webhook when creating resourceFlavor #299

1. Validate labels

[ahg-g]

@alculquicondor anything else?

[alculquicondor]

edited the comment above

[kerthcet]

/cc

[ahg-g]

@kerthcet do you want to send a PR for CQ validation?

[kerthcet]

Yes, I'll finish it soon.

[ahg-g]

Almost there, only one left is validating CQ

[kerthcet]

FYI: We only leave one validation now.

Two resources can either have completely different flavors or they must share all flavors and their order.

[ahg-g]

FYI: We only leave one validation now.

Two resources can either have completely different flavors or they must share all flavors and their order.

We need to do that as well, we have scheduling logic assuming that this is validated.

[ahg-g]

Done