Adding Alpha criteria for KEP to support IPPR & Pod Level Resources

[ndixita]

• One-line PR description: Split KEP from Pod level resources #2837 to add the alpha criteria for In place pod-level resources resize

• Issue link: Pod Level Resources Support With In Place Pod Vertical Scaling #5419

[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

[ndixita]

/assign @tallclair
/assign @soltysh

[ndixita]

@tallclair I couldn't find your comment because I changed the origin of my branch. I tried to address your question about allowing adding pod-level resources as a part of resize. PTAL

[tallclair]

We are going to allow adding & removing container-level resources. I feel like the use case is pretty weak, so I'm not too worried about this constraint, but it is a minor inconsistency.

[ndixita]

/assign @mrunalp

[soltysh]

This is mostly ok, the two bits that I'd like to get clarified before approving are:

1. defaulting
2. Resoruces field in PodStatus.

[k8s-ci-robot]

@ndixita: Can not set label lead-opted-in: Must be member in one of these teams: [release-team-enhancements release-team-leads sig-api-machinery-leads sig-apps-leads sig-architecture-leads sig-auth-leads sig-autoscaling-leads sig-cli-leads sig-cloud-provider-leads sig-cluster-lifecycle-leads sig-contributor-experience-leads sig-docs-leads sig-instrumentation-leads sig-k8s-infra-leads sig-multicluster-leads sig-network-leads sig-node-leads sig-release-leads sig-scalability-leads sig-scheduling-leads sig-security-leads sig-storage-leads sig-testing-leads sig-windows-leads]

In response to this:

/label lead-opted-in

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.

[ndixita]

This is mostly ok, the two bits that I'd like to get clarified before approving are:

1. defaulting
2. Resoruces field in PodStatus.

@soltysh added some details in #### Surfacing Pod Resource Requirements to add some clarity. PTAL

[soltysh]

/approve
the PRR

[tallclair]

We are going to allow adding & removing container-level resources. I feel like the use case is pretty weak, so I'm not too worried about this constraint, but it is a minor inconsistency.

[tallclair]

There is a weird consequence of defaulting in the apiserver: if a pod is resized before it is scheduled, then the desired resources will be different. Do we care about this scenario?

[natasha41575]

This would only be temporary until it is updated by the kubelet in generateAPIPodStatus, which presumably should happen quickly right? I think that seems fine?

[tallclair]

Should happen quickly in most cases, but if the pod is unschedulable it could be long-lasting. It is also sort of a resize pending/in-progress state, but without the associated condition. The other question it raises is, should the scheduler schedule based on the desired, original, or max resources?

[natasha41575]

What's the alternative? Leaving it empty until the kubelet gets to it?

[ndixita]

Should happen quickly in most cases, but if the pod is unschedulable it could be long-lasting.

I think it is better to have the scheduler set PodStatus.Resources after it binds the node to a pod in that case. If a pod is resized before scheduler sets PodStatus.Resources, it will be nil and controllers that need the info about pod requests in that case could compute it from pod spec using the resource helper

[natasha41575]

I think I'm missing the context of the motivation of defaulting this value before it gets to the kubelet, so can't really speak to the consequences of not doing that.

But I think:

Should happen quickly in most cases, but if the pod is unschedulable it could be long-lasting. It is also sort of a resize pending/in-progress state, but without the associated condition.

We're already having conversations about putting the kubelet in the scheduling path to deal with other resize / scheduling race conditions. If we prioritize that for 1.35 and include this in that discussion, the problems here could become much easier to resolve.

The other question it raises is, should the scheduler schedule based on the desired, original, or max resources?

The kubelet admits based on desired and wouldn't see the original, so the scheduler probably would need to do the same?

If a pod is resized before scheduler sets PodStatus.Resources, it will be nil and controllers that need the info about pod requests in that case could compute it from pod spec using the resource helper

If controllers are anyways going to have to handle the case that PodStatus.Resources could be nil, then I'm not seeing the benefit of having the apiserver or scheduler set PodStatus.Resources.

[ndixita]

I think we are converging towards the same thing. If we want to address the other resizes/scheduling race conditions, it makes sense for kubelet to set PodStatus.Resources. The idea behind this field is to allow controllers to inspect scheduled pods to quickly determine their effective resource requirements.

For unscheduled pods, the controllers can rely on components resource helpers to determine the requests from the PodSpec object.

Agree with not much benefit of having scheduler set PodStatus.Resources in that case. It would just help users understand what defaults does API server apply to the PodSpec before it is scheduled in case that's needed, but then there are cons of this wrt to the other resizes/scheduling race conditions

[tallclair]

Leaving PodStatus.Resources unset for now sounds good. The Kubelet owns this field, so I don't think the scheduler should touch it (especially not after binding to the node). It's really only safe for another component to set it on create, after that the Kubelet should be the only writer.

The motivation for defaulting it is if any components want to use this field instead of the component-helper to calculate pod-level resource totals. If we don't set it in defaulting, that means that it can't be used for admission (e.g. quota controls), or any controllers that run before the pod is started.

[natasha41575]

LGTM

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, natasha41575, ndixita, soltysh

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 [soltysh]
• keps/sig-node/OWNERS [mrunalp]

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

[tallclair]

/lgtm