properly configure appwrapper webhooks

[dgrove-oss]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Adds the kubebuilder directives to properly configure the AppWrapper webhooks.
This should have been part of #3953.

Which issue(s) this PR fixes:

Special notes for your reviewer:

No release notes because we haven't made a release with appwrapper. The release note from #3953 is sufficient.

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	fa1a7cf
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67a517049570bb0008d4bbb6

[tenzen-y]

Could you implement minimum test cases in https://github.com/kubernetes-sigs/kueue/tree/main/test/integration/singlecluster/webhook/jobs?

[tenzen-y]

/kind feature

This integration has not been released, yet. So we can consider this as just followup of #3953

[mimowo]

What is the user-facing change of the webhooks? It would be great to cover that in e2e or integration tests. I'm ok with a follow up if you tested manually.

[tenzen-y]

What is the user-facing change of the webhooks? It would be great to cover that in e2e or integration tests. I'm ok with a follow up if you tested manually.

I think that the integration tests are enough in this case. The objective for webhook tests aim to verify if webhooks are correctly configured (mostly manifests and kubebuilder markers). It seems that AppWrapper does not have specific webhook as I can see https://github.com/kubernetes-sigs/kueue/tree/main/pkg/controller/jobs/appwrapper

[dgrove-oss]

What is the user-facing change of the webhooks? It would be great to cover that in e2e or integration tests. I'm ok with a follow up if you tested manually.

I'm working on the integration test now as suggested by @tenzen-y . Should only take about an hour.
I didn't do it before because for its Kueue webhook AppWrapper just instantiates the BaseWebook (so the core functionality is tested already). But we do need to test exactly the thing I forgot to do in my original PR... that the webhook actually gets registered correctly.

For AppWrapper users, there is no change. With Kueue 0.10 this code runs in the external webhook (installed with AppWrapper controller). In Kueue 0.11 it runs in Kueue's webhook and is disabled by configuration in the AppWrapper controller's webhook.

[kannon92]

Could we follow a simple pattern that we do for pod/deployment/statefulset?

If we don't use AppWrapper in our framework, could we ignore the webhooks?

[dgrove-oss]

Could we follow a simple pattern that we do for pod/deployment/statefulset?

If we don't use AppWrapper in our framework, could we ignore the webhooks?

We should use the same pattern as all enabled-by-default frameworks that aren't Kinds that are part core Kubernetes. Ie, no difference between AppWrapper or RayCluster, or PyTorchJob.

[dgrove-oss]

integration test added.

it's not the most direct way to find it, but the reason I found this problem in the first place is my e2e multikueue tests in #3992 were failing because the webhook is responsible for defaulting managedBy and it wasn't running on the appwrapper the test created on the master node.

[mimowo]

Impl LGTM, left some comments to the test.

[tenzen-y]

Thank you!
/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f272a51a4b3ce64a56fff60169b74be7aeb94b3d

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgrove-oss, tenzen-y

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 [tenzen-y]

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