In-Place Pod Vertical Scaling KEP API review

[Vinay Kulkarni]

Hello Jordan, API reviewers,

We have the In-Place Pod Vertical Scaling KEP that has been merged as provisional, and is ready for API review.

There is a PR that addresses the open questions for the above provisional KEP, adds a new mini-KEP for Kubelet-CRI changes (if that is of interest to API review process), and requesting to get the above KEP to implementable state. Please see: https://github.com/kubernetes/enhancements/pull/1342

I have created a short document that provides our rationale behind the proposed API changes, and I hope it helps the API review process. Please review: https://docs.google.com/presentation/d/16-R9gEpKF6nyzDNtA5JDsKzQzr_vF_ZevN0ezZVYHpk/edit#slide=id.p1

I and David are attending the K8s contributor summit  + KubeCon next week. I'm wondering if we can find a reviewer and go over this f2f next week if possible. Ideally, if there is time, we can do this in Jordan's Live API review session in the contributor summit :)

Alternately, is this something we can put up for an Unconference session vote? If yes, how do I go about proposing it for unconference?

Thanks,

Vinay

[Jordan Liggitt]

I think this would be great to go over for part of the API review session tomorrow. I'm less familiar with the CRI APIs, so I'd probably defer review of those to Tim or Clayton, but we can certainly work through some of the implications of the Pod API proposal.

[Vinay Kulkarni]

Sounds great! I'll plan on presenting the changes, if needed.

[Vinay Kulkarni]

+Tim.

Hi Jordan,

Thanks for taking up this KEP for API review today! 

To summarize our discussion today:

• Adding Pod.Spec.Containers[i].ResourcesAllocated (v1.ResourceList) and Pod.Status.ContainerStatuses[i].Resources (v1.ResourceRequirements) is reasonable.
• We should use list of named sub-objects for ResizePolicy for future extensibility. I'll update the KEP to reflect this.
• Avoid subresource if possible.
• David suggested an alternative: SIG-Auth can allow kubelet to update PodSpec. Is that a reasonable approach?
• When we resize down, are we enforcing ResourceQuota if we used Spec-Resources?
• We initially planned to use max(Spec.Containers[i].Resources, Status.ContainerStatuses[i].Resources) as Status is updated by kubelet after requests/limits have converged to desired.
• However, we felt that Kubelet should always admit downward resize for requests, and while we may lag with limits, excess usage will result in eviction. Eventually system converges to desired ResourceQuota bounds.
• Link to discussion about Requests and RQ on the KEP: https://github.com/kubernetes/enhancements/pull/686#discussion_r324809149
• Link to discussion about Limits and RQ on the KEP: https://github.com/kubernetes/enhancements/pull/686#discussion_r324806067
  

Please add if I missed something.

Thanks,

Vinay

[Vinay Kulkarni]

Hi Jordan,

Happy New Year! 

Tim took the time to review the KEP over the holidays and felt it looks reasonable overall, and approved it. Thanks Tim!

I've pushed one update to the KEP, i.e: use named sub-objects instead of maps as per our discussion during the API review.

Regarding your concern about adding a new subresource, I experimented with the alternative option of using Admission controller instead, and still think we should use subresource as this concerns system resource accounting, and no-one (even admins/superusers) besides the system node account should have control over it or be able to disable it. If you still feel strongly about adding a new subresource, do you think it is reasonable to extend/overload the binding subresource? I prefer to have this new subresouce as it seems much cleaner.

Thanks,

Vinay

[Clayton Coleman]

On Jan 5, 2020, at 6:06 PM, Vinay Kulkarni <kulkar...@gmail.com> wrote:

Hi Jordan,

Happy New Year! 

Tim took the time to review the KEP over the holidays and felt it looks reasonable overall, and approved it. Thanks Tim!

I've pushed one update to the KEP, i.e: use named sub-objects instead of maps as per our discussion during the API review.

Regarding your concern about adding a new subresource, I experimented with the alternative option of using Admission controller instead, and still think we should use subresource as this concerns system resource accounting, and no-one (even admins/superusers) besides the system node account should have control over it or be able to disable it.

I don’t understand this comment.  A Kubelet is just a particular actor.  We don’t generally treat “system actors” as special, and super users should *always* be able to edit spec/status fields that any particular user can edit.

We have trended away from “subresource for access control” in recent years, and instead encouraged control over field level modifications.  Jordan’s comment seems more in line with that.

--
You received this message because you are subscribed to the Google Groups "kubernetes-api-reviewers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-api-rev...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAG6HisP4K5g%2BOpX3_ngsric5GQmNa1CNeyEdCma2hkZkimGvOg%40mail.gmail.com.

[Vinay Kulkarni]

Thanks Clayton. Let' do this with admission control hooks, I'll update the KEP and send a PR - that should resolve this last outstanding issue.

Best,

Vinay

[Vinay Kulkarni]

Hi Jordan,

I've updated the KEP to use a new admission controller instead of subresource, and use list of named subobjects instead of maps for policy.

Please review, and let me know if this is close to what you were looking for.

Thanks again for the API review!

Vinay

[Vinay Kulkarni]

Hi Jordan,

I have preview code for the proposed API change in the KEP - please see PR https://github.com/vinaykul/kubernetes/pull/1 , and specifically commit https://github.com/vinaykul/kubernetes/commit/2a1aedd48c4ba5490e130bba36946b7c17f0fc2e which is the actual struct changes plus admission controller, validation & defaults, minus all the generated code and testdata update stuff.

Please let me know if this is close to what you are looking for.

Thanks,

Vinay

[Vinay Kulkarni]

The PR is in my fork as it contains only the API change and not the implementation yet, so it is not ready to merge. But I've added you guys as collaborators, I think that should allow you to comment on the PR.

Thanks,

Vinay