Pod based webhooks should only be enabled if one wants to use pod integration

[kannon92]

ref: #4074

There are many cases where people do not want to enable pod based integrations as it can potentially cause an entire cluster to be quota limited by Kueue.

Even if one disables this, these webhooks can still be hit as they are added to all kueue deployments. In the above example, those webhooks caused a critical service in openshift to not create certificates.

We would like to explore ways to only enable the webhooks if pod-based integration is enabled.

Since Pod based workloads are not enabled by default, could we disable the webhooks by default?

[mimowo]

Im surprised, I thought they get enabled based on the enablement of the pod integration. similarly we are enabling the job webhooks only when the particular job integration is enabled. Can you double check the webhooks are enabled whilst the pod integration is disabled?

cc @mbobrovskyi @dgrove-oss

[mbobrovskyi]

By default all pod-based integrations disabled:

kueue/config/components/manager/controller_manager_config.yaml

Lines 55 to 58 in 418091a

	# - "pod"
	# - "deployment" # requires enabling pod integration
	# - "statefulset" # requires enabling pod integration
	# - "leaderworkerset.x-k8s.io/leaderworkerset" # requires enabling pod integration

When integration is disabled, we use NoopWebhook:

kueue/pkg/controller/jobframework/setup.go

Lines 101 to 103 in 418091a

	if err := setupNoopWebhook(mgr, cb.JobType); err != nil {
	return fmt.Errorf("%s: unable to create noop webhook: %w", fwkNamePrefix, err)
	}

I also rechecked it manually.

[mimowo]

hmmm, wondering if the no-op webhook might still require API server to send the object to the webhook server for mutations?

if so then instead of registering no-op it is probably better not to register it at all

[mimowo]

I synced with @mbobrovskyi we register the no-op webhooks, because we rely on the webhook registration generated by kubebuilder: https://github.com/kubernetes-sigs/kueue/blob/main/config/components/webhook/manifests.yaml.

I agree registering the no-op webhooks is not ideal but to avoid it we would need to either:

1. place the registration in a dedicated opt-in manifest file and request users to install them, when they forget we need a clear message what is missing
2. add the registrations from Kueue code when we detect the Pod integration is enabled - could work, but sending the patches by Kueue post installation might be surprising to users.
3. just guide users which are performance sensitive how to delete the registrations in docs

I don't have a clear opinion which one is best.

[kannon92]

I don’t either. We are working on an operator where we would remove these from the manifest based on frameworks before installation. That is probably difficult to do with kustomize.

I think docs may be important because the default namespace selector seems to be valid for Kind but other clusters would need to consider expanding the excludes. I’m not even sure if our helm chart customizes that..

[varshaprasad96]

Looks like #4141 is also a side-effect of applying webhook configurations for all the frameworks together. I'm leaning a bit towards (1) - having webhook configurations for each framework in separate Kustomize directories. Users would have to manually uncomment patches for the integrations they have enabled explicitly. This would be an additional step, but a one-off task required when modifying config.yaml.

The helm charts seem to handle it well already through templating and ignoring the admission checks if integrations are not enabled:

kueue/charts/kueue/templates/webhook/webhook.yaml

Lines 21 to 24 in d02a764

	{{- if has "pod" $integrationsConfig.frameworks }}
	failurePolicy: Fail
	{{- else }}
	failurePolicy: Ignore