KEP-3521: Graduate PodSchedulingReadiness to beta

[Huang-Wei]

• One-line PR description: graduate PodSchedulingReadiness to beta

• Issue link: Pod Scheduling Readiness #3521

[Huang-Wei]

/assign @ahg-g @wojtek-t

[ahg-g]

nits

[ahg-g]

this already exists since alpha, correct?

[Huang-Wei]

correct.

[Huang-Wei]

@wojtek-t not sure if you have cycles to review the PRR portion? Thanks in advance. 🙏

[ahg-g]

/approve

[Huang-Wei]

@wojtek-t not sure if you have cycles to review the PRR portion? Thanks in advance. 🙏

Kindly ping @wojtek-t in case it doesn't get slipped.

[cici37]

Thanks for the PR! PRR shadow here for PRR review. Just have a couple comments:

1. Integration tests and e2e test seems to be added during alpha, would you have time to update the KEP to reflect the changes? :)
2. Seems like how can a rollout or rollback fail section hasn't been filled since it is required to be filled for feature going to beta. It might be mentioned in Risks and Mitigations section. Appreciate if you could also add info inside PRR section. Thanks
3. Couple questions in scalability section.
   Thank you :)

/cc @wojtek-t for PRR approval

[cici37]

Since a new field is introduced, the API object could potentially be longer(even not expected to be huge difference)?

[Huang-Wei]

I thought this question is asking about a possible size increase of existing API objects? Given this field is optional, if a pod does not opt-in this feature, when it gets roundtripped, its size won't change. But I'm updating it to clarify for both cases.

[Huang-Wei]

@cici37 Thanks for the review! Your comments have been addressed.

[wojtek-t]

Kindly ping @wojtek-t in case it doesn't get slipped.

I was waiting for PRR shadower to look first - looking now
[Sorry could have written that it's in my queue more explicitly.]

[wojtek-t]

@cici37 - I added some more comments - please follow up with the @Huang-Wei on having them incorporated into KEP and ping me when it will look fine from your perspective.

[wojtek-t]

I would follow up on the callout from @cici37 - instead of just saying "added in Alpha", can you please add links to k8s-triage (as mentioned in the comment in lin 511 above).

[Huang-Wei]

updated with k8s-triage link, and pruned a dated item.

[wojtek-t]

Commenting here as I can't comment on unchanged lines:

L623: you have the following beta criteria:

Permission control on individual schedulingGate is applicable

From what I know this at best will reach Alpha in 1.27, and we can't rely on Alpha in Beta feature.
What are your plans here?

[I would be ok with skipping that Beta graduation criteria, but if that's the plan let's mention it explicitly as a "future work"]

[wojtek-t]

Another thing (it seems I missed that for Alpha - or it was changed in the meantime)

for Downgrade section vs the section from Notes/Constraints/... above:

New field disabled in Alpha but not scheduler extension: In Alpha, the new Pod field is disabled by default. However, the scheduler's extension point is activated no matter the feature gate is enabled or not. This enables scheduler plugin developers to tryout the feature even in Alpha, by crafting different enqueue plugins and wire with custom fields or conditions.

That doesn't play well with downgrade. Because where we get is actually:

• I first upgraded to a release when the feature is enabled
• I created a pod with schedulingGate
• I downgrade the pod - the schedulingGate field remains set as expected
• but... on scheduler the feature is still enabled, and this pod will keep pending forever...

That seems wrong to me. Or was that properly solved on scheduler side?
That requires a more detailed discussion in the doc.

[Huang-Wei]

From what I know this at best will reach Alpha in 1.27, and we can't rely on Alpha in Beta feature.
What are your plans here?

Fine-grained control on k8s fields is a general topic. So I'd like to move it to future work to not block beta.

That doesn't play well with downgrade. Because where we get is actually:

• I first upgraded to a release when the feature is enabled
• I created a pod with schedulingGate
• I downgrade the pod - the schedulingGate field remains set as expected
• but... on scheduler the feature is still enabled, and this pod will keep pending forever...

If we downgrade the cluster, scheduler has to be restarted, and during its initialization, the scheduler plugin (to back this feature) will not be enabled at all. So if the downgrade is successful, no matter the previously-created pod carries schedulingGates or not, it will be bypassed.

Actually the section "New field disabled in Alpha but not scheduler extension" refers to another scenario: users don't opt-in to this feature (i.e., always leave .spec.schedulingGates empty), instead, they (usually advanced scheduler plugin developers) choose to apply custom fields (labels/annotations), and craft their own custom plugin to back those fields.

[wojtek-t]

If we downgrade the cluster, scheduler has to be restarted, and during its initialization, the scheduler plugin (to back this feature) will not be enabled at all. So if the downgrade is successful, no matter the previously-created pod carries schedulingGates or not, it will be bypassed.

Ok - so that seems correct.
But then I don't really understand this part:
https://github.com/kubernetes/enhancements/pull/3739/files#diff-e8d9aae23eeca866dc6f8a2914e984dd8a74c22c05b7d6bb2800d88b047ce0b1R316-R319

In Alpha, the new Pod field is disabled
by default. However, the scheduler's extension point is activated no matter the feature gate is enabled
or not. This enables scheduler plugin developers to tryout the feature even in Alpha, by crafting
different enqueue plugins and wire with custom fields or conditions

What does the extension point do then? I though it's resposible for handling the pods with schedulingGates set. If not, then what that is?

instead, they (usually advanced scheduler plugin developers) choose to apply custom fields (labels/annotations), and craft their own custom plugin to back those fields.

I'm sorry, I think I'm not following. Can you please clarify?

[Huang-Wei]

sure, this section is more for plugin developers. In a high level, the entire feature contains the following parts:

• a. Pod's spec, feature gate, and other misc bits describing the field schedulingGates
• b. The SchedulingGates scheduler plugin
• c. The underlying scheduler framework, i.e., the new PreEnqueue extension point

So, what this section tries to say is: a plugin developer can use part c) only, and craft their own a' and b' to accomplish their own feature set.

[Huang-Wei]

What does the extension point do then? I though it's resposible for handling the pods with schedulingGates set. If not, then what that is?

The extension point is a lower-level core layer, which is agnostic to any pod field. As explained above, the SchedulingGates plugin aims to handle the pods with schedulingGates. When we disable this feature, it means the SchedulingGates plugin along with the upper-level API machinery bits are disabled, but the extension point (like an outlet in the wall) is still there.

[wojtek-t]

So, what this section tries to say is: a plugin developer can use part c) only, and craft their own a' and b' to accomplish their own feature set.

OK - now that makes sense to me.
Would you mind adding something along those lines (describe the parts and add this sentence) to the KEP itself so that it is clear for other potential readers?

[wojtek-t]

This is fine, but please update the KEP with the scenario and results of that test (and cc me on it) before switching the feature gate value to true in k/k.

[Huang-Wei]

Leaving a <<UNRESOLVED>> section here to not block KEP's freeze date.

[wojtek-t]

Please explain "this" - (I'm assuming you're talking about pod-startup-time which contains scheduling).

Also - I would say that this actually doesn't affect it, because the SLO explicitly talks about "schedulable" pods - if schedulingGate is set it's unclear if this pod is actually schedulable ...

[Huang-Wei]

Thanks. I took a look at https://github.com/kubernetes/community/blob/master/sig-scalability/slos/slos.md#kubernetes-slisslos, a SLO about the time from pod creation to pod scheduled is not defined officially yet, so the answer is No here.

[wojtek-t]

If we downgrade the cluster, scheduler has to be restarted, and during its initialization, the scheduler plugin (to back this feature) will not be enabled at all. So if the downgrade is successful, no matter the previously-created pod carries schedulingGates or not, it will be bypassed.

Ok - so that seems correct.
But then I don't really understand this part:
https://github.com/kubernetes/enhancements/pull/3739/files#diff-e8d9aae23eeca866dc6f8a2914e984dd8a74c22c05b7d6bb2800d88b047ce0b1R316-R319

In Alpha, the new Pod field is disabled
by default. However, the scheduler's extension point is activated no matter the feature gate is enabled
or not. This enables scheduler plugin developers to tryout the feature even in Alpha, by crafting
different enqueue plugins and wire with custom fields or conditions

What does the extension point do then? I though it's resposible for handling the pods with schedulingGates set. If not, then what that is?

instead, they (usually advanced scheduler plugin developers) choose to apply custom fields (labels/annotations), and craft their own custom plugin to back those fields.

I'm sorry, I think I'm not following. Can you please clarify?

[wojtek-t]

Thanks. I took a look at https://github.com/kubernetes/community/blob/master/sig-scalability/slos/slos.md#kubernetes-slisslos, a SLO about the time from pod creation to pod scheduled is not defined officially yet, so the answer is No here.

That's not entirely true, because the SLO is from time a pod is created to when it's observed as ready. Which actually includes scheduling time.

But as I mentioned previously, it depends how we define "pod is schedulable" here. Currently it's defined as

[1]By schedulable pod we mean a pod that can be scheduled in the cluster without causing any preemption.

Which is too weak. We should update it, because e.g. low-priority pod will not be able to preempt anything, but may not be schedulable due to lack of space in the cluster.

So we have two options basically:
(a) say that pod has to be immediately schedulable (without actions from any other components), [which would directly exclude pods with schedulingGates]
(b) say that pods with schedulingGates may be schedulable, but then we're taking a dependency on components we may not control, which I don't like.

So I'm in favor of (a). This will indeed mean the answer is "no" here, but then requires updating:
https://github.com/kubernetes/community/blob/master/sig-scalability/slos/pod_startup_latency.md

Would you mind sending me a PR to update that along with what I mentioned in (a)?

[cici37]

This was the confusion I had earlier. Thank you for the clarification! :)

[Huang-Wei]

Would you mind sending me a PR to update that along with what I mentioned in (a)?

Sure, here it is: kubernetes/community#7086

[wojtek-t]

@Huang-Wei - ok #3739 (comment) is my last comment, once applied this will be ready from PRR perspective too.

[Huang-Wei]

@Huang-Wei - ok #3739 (comment) is my last comment, once applied this will be ready from PRR perspective too.

Thanks. That section gets reworded.

[wojtek-t]

/lgtm
/approve PRR

Thanks! Let's make this happen!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Huang-Wei, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-scheduling/OWNERS [Huang-Wei,ahg-g]

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