Fast admission annotation

[vladikkuzn]

What type of PR is this?

/kind feature

What this PR does / why we need it:

The PodGroup integration waits until all pods are created, but StatefulSet creates second pod only after all previous pods are running, so there was a deadlock. The solution to this is to introduce the kueue.x-k8s.io/fast-pod-group-admission annotation, when true, then the Pod group does not need to wait for all pods with ungating. Part of #2717

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Introduce the "kueue.x-k8s.io/pod-group-fast-admission" annotation to Plain Pod integration.

If the PlainPod has the annotation and is part of the Plain PodGroup, the Kueue will admit the Plain Pod regardless of whether all PodGroup Pods are created.

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	c4ca698
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6706629858800c0008fab048

[vladikkuzn]

/ok-to-test

[trasc]

/test pull-kueue-test-integration-main

[vladikkuzn]

/cc @trasc

[trasc]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 6dd114a245a01672959984db22bb09f7a4c1d280

[mimowo]

/lgtm
/approve
/hold
in case @tenzen-y has some extra comments

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 7ef7fa94527d727a61d9dcb6990f6c1b1d8c25b9

[tenzen-y]

Yeah, the PodGroup feature seems like a better fit to base the StatefulSet integration. If we base the integration on individual pods, then in case of preemption, the StatefulSet could lose Pod-0 which might be problematic, because StatefulSets assume continuity of indexes. For Deployments it is fair to use individual pods, because we don't care which pod is preempted. WDYT?

cc @mwielgus who also was thinking about it

@mimowo The use case sounds reasonable, but shouldn't we implement the StatefulSet integration as the dedicated Job?
Actually, we implemented some of dedicated Jobs like batch/job, RayJob, and ... so that we can support such a complex cases.

[mimowo]

Yes, we are considering it, but it seems to be significantly more work. The idea is to provide the MVP functionality so that Kueue can be used for Inference in 0.9. I consider this as "experimental" support to gain user feedback.

I would go for a dedicated integration when we hit some use cases which cannot be achieved this way. Maybe it is needed for efficient scaling, but I'm not sure. Maybe it is achievable by scaling support for PodGroups also.

[tenzen-y]

Yes, we are considering it, but it seems to be significantly more work. The idea is to provide the MVP functionality so that Kueue can be used for Inference in 0.9. I consider this as "experimental" support to gain user feedback.

I would go for a dedicated integration when we hit some use cases which cannot be achieved this way. Maybe it is needed for efficient scaling, but I'm not sure. Maybe it is achievable by scaling support for PodGroups also.

That makes sense. In that case, we may would like to avoid using "StatefulSet" as an integration name and use alternative name something like "StatefulSetWithPodGroup" since we are planning to implement the dedicated integration for the StetefulSet and we can assume the "StatefulSetWithPodGroup" will be deprecated in the future release.

Anyway, this PR can be moved forward.

[tenzen-y]

@vladikkuzn Could you open a PR to update the PodGroup documentation?

https://kueue.sigs.k8s.io/docs/tasks/run/plain_pods/#running-a-group-of-pods-to-be-admitted-together

[tenzen-y]

Oh, sorry. I accidentally closed this PR.

[tenzen-y]

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, tenzen-y, vladikkuzn

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:

• pkg/controller/OWNERS [mimowo,tenzen-y]
• test/OWNERS [mimowo,tenzen-y]

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

[mimowo]

That makes sense. In that case, we may would like to avoid using "StatefulSet" as an integration name and use
alternative name something like "StatefulSetWithPodGroup" since we are planning to implement the dedicated ?
integration for the StetefulSet and we can assume the "StatefulSetWithPodGroup" will be deprecated in the future release.

TBH I would prefer to use "StatefulSet" and handle the underlying mechanism as an implementation detail. Users creating StatefulSets don't need to be aware of that. The "fast-admission" annotation will be added by a webhook.

One particularly challenging aspect is that StatefulSet as such does not support "suspend" operation, similarly as Deployment. Adding such fields to the upstream k8s will be challenging, because the usage is rather niche for now. So, we will likely stay with PodGroups as the underlying integration mechanism for a while.

[tenzen-y]

That makes sense. In that case, we may would like to avoid using "StatefulSet" as an integration name and use
alternative name something like "StatefulSetWithPodGroup" since we are planning to implement the dedicated ?
integration for the StetefulSet and we can assume the "StatefulSetWithPodGroup" will be deprecated in the future release.

TBH I would prefer to use "StatefulSet" and handle the underlying mechanism as an implementation detail. Users creating StatefulSets don't need to be aware of that. The "fast-admission" annotation will be added by a webhook.

One particularly challenging aspect is that StatefulSet as such does not support "suspend" operation, similarly as Deployment. Adding such fields to the upstream k8s will be challenging, because the usage is rather niche for now. So, we will likely stay with PodGroups as the underlying integration mechanism for a while.

I discussed this with @mimowo offline. Throughout valuable discussions, we found that the framework integration migration potentially could happen in any integration, not only for StatefulSet. So, we decided to use the "StatefulSet" as a integration name as a today's StatefulSet integration. Then, we will seek out how we can support migration to new integration.

[tenzen-y]

It seems that this is a user facing new feature since this can be used by batch users.

/release-note-edit

Introduce the "kueue.x-k8s.io/retriable-in-group" annotation to Plain Pod integration.

If the PlainPod has the annotation and is part of the Plain PodGroup, the Kueue will admit the Plain Pod regardless of whether all PodGroup Pods are created.

[vladikkuzn]

@tenzen-y are you sure about this release note^? "kueue.x-k8s.io/pod-group-fast-admission" annotation was added, not "kueue.x-k8s.io/retriable-in-group"

[tenzen-y]

Oops, sorry. You're right.

/release-note-edit

Introduce the "kueue.x-k8s.io/pod-group-fast-admission" annotation to Plain Pod integration.

If the PlainPod has the annotation and is part of the Plain PodGroup, the Kueue will admit the Plain Pod regardless of whether all PodGroup Pods are created.