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 #3279

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 canceled.

Name	Link
🔨 Latest commit	1489600
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67781642aa0cb90008fc7e28

[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

[mbobrovskyi]

/unhold

[mbobrovskyi]

/cc @mimowo

[mimowo]

This PR seems to be changing much more than needed. Let me hold until the scope and purpose of the PR is clarified.
/hold

[mimowo]

Why is this changed? Please don't if not necessary.

[mbobrovskyi]

Sometimes, a Pod is updated, but the StatefulSet isn't aware of it, causing the reconcile process to not work as expected (e.g., the finalizer isn't removed). Thats why it is better to reconcile Pod instead of StatefulSet.

[mimowo]

I don't understand this case - can you elaborate? For example in the core k8s Job we also have finalizers on Job pods, but the reconciler is at the Job level, reference. I would like to first document such problematic scenarios in some form of tests and change the implementation in a dedicated PR (if needed).

[mimowo]

This change does not seem releated to changing queue-name. Please revert if not necessary.