Allow mutating queue name in StatefulSet Webhook.

[mbobrovskyi]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Allow mutating queue name in StatefulSet Webhook.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Allow mutating queue name in StatefulSet Webhook.

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue ready!

Name	Link
🔨 Latest commit	b7dff32
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67482fc8bd42a10008dfec11
😎 Deploy Preview	https://deploy-preview-3520--kubernetes-sigs-kueue.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[mimowo]

/hold
I want to understand the flow e2e first from the user perspective.
In particular, how can user start such a StafulSet, will adding the label make it start?

IIRC for Jobs we start such a Job (but please double-check and confirm).

I synced with @mbobrovskyi that this is to align the behavior for Deployment, but another option is to simply reject such Deployments if they are not supported anyway.

I think it deserves e2e test.

[dgrove-oss]

I'd also like to understand how it interacts with the namespaceSelector on the pod integration. In the Pod webhook Default method, if the namespaceSelector doesn't match then we never get to the code that consults manageJobsWithoutQueueName.

We don't support namespaceSelectors to modify manageJobsWithoutQueueName for any other integration (discussed at length in #2119).

What is the intended semantics for a StatefulSet or Deployment that is deployed in a namespace that doesn't match the namespaceSelector for the Pod integration when manageJobsWithoutQueueName is true?

[mimowo]

In the Pod webhook Default method, if the namespaceSelector doesn't match then we never get to the code that consults manageJobsWithoutQueueName.

Yes, this is WAI. The intention was to have a mechanism to exclude pods (like static pods or DeamonSet pods) in kube-system and kueue-system. We made the mechanism more generic (to exclude arbitrary namespaces).

We don't support namespaceSelectors to modify manageJobsWithoutQueueName for any other integration (discussed at length in #2119).

Right, we don't do it for all other integrations. However, I think Deployments and StatefulSets need to be the other cases, first Deployments are used in kube-system and kueue-system so we better don't touch them. Second, the support is based on the PodGroup integration and so we inherit the lookup into namespaceSelector for the pod integration.

What is the intended semantics for a StatefulSet or Deployment that is deployed in a namespace that doesn't match the namespaceSelector for the Pod integration when manageJobsWithoutQueueName is true?

IIUC this means basically "for Deployments and StatefulSets in the kube-system or kueue-system". I think we should not manage them - no workload should be created. Since Deployments and StatefulSets are based on PodGroup integration this should happen "for free".

Let me know if this matches your expectations and understanding.

cc @mwielgus

[dgrove-oss]

It honestly feels a bit like our implementation is leaking through to the API. In particular, treating StatefulSets one way and Jobs another wrt manageJobsWithoutQueueName.

I think it could be less surprising / easier to explain if the boolean manageJobsWithoutQueueNames was replaced with a namespaceSelector across all integrations. I know this was discussed before, but maybe it is worth revisiting now that (a) we see what we need for Deployment and StatefulSet and (b) we are thinking about what a v1 API would look like and what perhaps should be improved between now and then.

[mimowo]

It honestly feels a bit like our implementation is leaking through to the API. In particular, treating StatefulSets one way and Jobs another wrt manageJobsWithoutQueueName.

Yeah, I see the point - so that it is not clear why StatefulSet or Deployment pods are controlled by podOptions.namespaceSelector, whilst for other Jobs this is not respected.

I think it could be less surprising / easier to explain if the boolean manageJobsWithoutQueueNames was replaced with a namespaceSelector across all integrations.

You mean "replaced"? Or something like "restricted" - so that we only manage workloads matching the namespaceSelector?

I know this was discussed before, but maybe it is worth revisiting now that (a) we see what we need for Deployment and StatefulSet and (b) we are thinking about what a v1 API would look like and what perhaps should be improved between now and then.

I would be in favor of that. The original intention of podOptions.namespaceSelector was to exclude "kube-system" and "kueue-system" from pods. Back then we didn't foresee the need to exclude managing for Jobs or other supported CRDs. However, as we now support Deployments it makes also sense to exclude "kube-system" and "kueue-system". Luckily this is for free by using Pod integration, but as you say it means leaking implementation details.

Let me also cc @mwielgus and @tenzen-y for their opinions, but +1 from me to decouple namespaceSelector from podOptions.

The remaining question from me: do we support both places, or we validate only one is set? We could consider supporting both places for v1beta1 and depracate the one in podOptions, but it would be good to have a KEP for that. Are you interested in driving this?

[dgrove-oss]

You mean "replaced"? Or something like "restricted" - so that we only manage workloads matching the namespaceSelector?

restricted is a better word :).

Yes, I'd propose that we do a uniform filtering by namespaceSelector for all integrations when manageJobsWithoutQueueName is true. I'll give people some time to comment, but if there is interest in exploring this I'd be happy to kick off a KEP and drive it.

[mbobrovskyi]

/reopen

[k8s-ci-robot]

@mbobrovskyi: Reopened this PR.

In response to this:

/reopen

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-sigs/prow repository.

[mbobrovskyi]

/test all

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mbobrovskyi
Once this PR has been reviewed and has the lgtm label, please assign kerthcet, trasc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/controller/OWNERS
• pkg/util/OWNERS
• test/OWNERS

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

[k8s-ci-robot]

PR needs rebase.

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-sigs/prow repository.