kep-3695-beta update

[guptaNswati]

• One-line PR description: Update KEP to prepare for beta in 1.34

• Issue link: DRA: Extend PodResources to include resources from Dynamic Resource Allocation #3695

/wg device-management
/assign @moshe010
/assign @klueska
/assign @@johnbelamaric for PRR

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: guptaNswati / name: Swati Gupta (6e540a5, 64302a3, 72432d5, 3d1512b, ae728ef, 430351f, 2402b80)

[k8s-ci-robot]

Welcome @guptaNswati!

It looks like this is your first PR to kubernetes/enhancements 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/enhancements has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @guptaNswati. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[kannon92]

/ok-to-test

[guptaNswati]

@soltysh @SergeyKanzhelev what else is needed to get this merged. I see its set to At risk enhancements freeze

[SergeyKanzhelev]

Please update to the latest template according to #3695 (comment)

My understanding @soltysh haven't stazrted reviews as the metadata was not correct. Now it should be correct, need to work on the document

[guptaNswati]

@SergeyKanzhelev

Please update to the latest template according to #3695 (comment)

It is using the latest template. See
KEP readme using the [latest template](https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template) has been merged into the k/enhancements repo.

Sorry, i am confused if you are referring to something else.

[ffromani]

@SergeyKanzhelev

Please update to the latest template according to #3695 (comment)

It is using the latest template. See KEP readme using the [latest template](https://github.com/kubernetes/enhancements/tree/master/keps/NNNN-kep-template) has been merged into the k/enhancements repo.

Sorry, i am confused if you are referring to something else.

You need to check the KEP layout of this PR matches the layout in the latest KEP template available. When folks update the KEP template some sections may move/be created/merged/removed, some new PRR questions can arise, and so forth

[SergeyKanzhelev]

As @ffromani mentioned some template sections might have moved or were added. I am relying on this comment: #3695 (comment) that tells the the KEP readme is not following the last template.

[guptaNswati]

Got it. thanks both. I see i updated the implementation history but that is within the format.

- Kubernetes 1.27: Alpha version of the KEP.

- Kubernetes 1.34: Beta version of the KEP.

[soltysh]

There are multiple questions not answered, which are requirement for beta promotion.

[soltysh]

Earlier in the doc:

1. Please make sure to check appropriate boxes in the ## Release Signoff Checklist.
2. Missing links in the integration tests section, see template, and in the e2e section as well, see template. Either of the two is required for beta promotion, and it looks like you had a requirement for e2e during alpha, so I expect those to be completed.

[soltysh]

• Please make sure to check appropriate boxes in the ## Release Signoff Checklist.

This was addressed - thank you.

• Missing links in the integration tests section, see template, and in the e2e section as well, see template. Either of the two is required for beta promotion, and it looks like you had a requirement for e2e during alpha, so I expect those to be completed.

This one still holds. We need links for integration and e2e based on the template in the appropriate section. I believe e2es were added in kubernetes/kubernetes#116846 so you should be able to quickly fill those in. Not sure if there are others.

[soltysh]

This still holds, I see Francesco mentioned several tests that were added, can we make sure they are explicitly linked in this document?

[soltysh]

I believe this holds.

[soltysh]

Further in the document:

1. In ###### Are there any tests for feature enablement/disablement? can you please link to those tests?
2. In ###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested? why that question is not applicable, can you explain?
3. In ###### What are the reasonable SLOs (Service Level Objectives) for the enhancement? N/A is not correct. You're using explicit metrics pod_resources_endpoint_requests_total, pod_resources_endpoint_requests_list and pod_resources_endpoint_requests_get. What is the SLO for those?
4. In ###### Will enabling / using this feature result in increasing size or count of the existing API objects? No is not correct. You're expanding the return types from Get call by additional information coming from DRA, so I'd expect that information be expressed in that answer.
5. In ###### Will enabling / using this feature result in non-negligible increase of resource usage (CPU, RAM, disk, IO, ...) in any components? your answer is not one I'd expect. What you're writing there is probably more suited to Troubleshooting or Risks and Mitigations sections. In your particular case I'm expecting a rough estimation if CPU/RAM/disk/IO will be affected by this feature. Based on my understanding you're likely increase the memory consumption to hold the DRA information, no?
6. You're missing ###### Can enabling / using this feature result in resource exhaustion of some node resources (PIDs, sockets, inodes, etc.)? question, from the template
7. In ###### How does this feature react if the API server and/or etcd is unavailable? I'm expecting a concrete answer.
8. In ###### What are other known failure modes? can you please use the proposed template? IE. Detection -> Mitigations -> Diagnostic, etc.
9. In ###### What steps should be taken if SLOs are not being met to determine the problem? you're missing answer, N/A is not acceptable.
10. What about Drawbacks and alternatives considered? I'm missing both sections, have we not considered any?

[guptaNswati]

@soltysh Most of these have the similar answers in this KEP and this KEP is an extension of it https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2043-pod-resource-concrete-assigments

Are these blocking updates?

And for this particular KEP, nothing has changed in the API from Alpha to Beta.
@ffromani where can i find the e2e test. The test PR is this: kubernetes/kubernetes#116846

[guptaNswati]

I do see some test here https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/directory/pull-kubernetes-node-kubelet-serial-containerd/1932604229172596736

but that doesnot seem to be the right link

[ffromani]

we need to review on which lane the podresources api (not pod-level resources) tests run, which changed multiple time over the months. Let's find and fix the configuration if needed.

[ffromani]

looking at the config in k/test-infra the tests should indeed run in the lanes
pull-kubernetes-node-kubelet-serial-containerd, but it's high time we add an explicit lane and/or we call out the tests explicitly in some lane. We can do something similar to the somehow confusing pull-kubernetes-node-kubelet-serial-podresources lane

[soltysh]

@soltysh Most of these have the similar answers in this KEP and this KEP is an extension of it https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2043-pod-resource-concrete-assigments

Are these blocking updates?

Yes, those are blocking changes. It doesn't matter the answers are similar, we need them in this document. If they are matching, it's fine to just copy them over.

[ffromani]

sig-node review: provisional LGTM. Two outstanding items from my POV.

1. (nonblocking, in scope for Beta, "just" extra work) turns out the CI config is obsolete, and the podresources api tests run incidentally. We should add them explicitly (like we do for example for cpumanager) and we should have a presubmit lane (like we do for example for pod level resources). I think it is unfair to account all this work into this KEP, but some of it make sense to be done in the context of the beta graduation. Already begun, PRs linked. We may need to fix or add some e2e tests, but I think this is totally fair in the context of the beta graduation.
2. (blocking) clarify the Get semantics as discussed. Because of how the API works, there is a TOCTOU unavoidable issue. There is no atomicity or consistency guarantee between List or Get, nor a concept of a session (which I would push back, btw). So it is possible, albeit probably unlikely, that Get is called with stale information (e.g pod becomes terminated between calls). We need to clarify and document the error paths

Once the above are addressed, LGTM.

[soltysh]

All of my previous comments still hold, and are blocking to merging this KEP.

[soltysh]

• Please make sure to check appropriate boxes in the ## Release Signoff Checklist.

This was addressed - thank you.

• Missing links in the integration tests section, see template, and in the e2e section as well, see template. Either of the two is required for beta promotion, and it looks like you had a requirement for e2e during alpha, so I expect those to be completed.

This one still holds. We need links for integration and e2e based on the template in the appropriate section. I believe e2es were added in kubernetes/kubernetes#116846 so you should be able to quickly fill those in. Not sure if there are others.

[soltysh]

The linked PR isn't testing feature enablement/disablement, or am I misreading it? The closest place where you test this feature gate is https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/podresources/server_v1_test.go but there you only turn this on, but I don't see the requested on/off test.

[ffromani]

we have a on/off test scattered across the existing tests: https://github.com/kubernetes/kubernetes/blob/v1.34.0-alpha.1/test/e2e_node/podresources_test.go#L977 and https://github.com/kubernetes/kubernetes/blob/v1.34.0-alpha.1/test/e2e_node/podresources_test.go#L1066
We can use a PR to make the tests more explicit and some changes are needed if the FG goes to default on: the FG status should be set explicitly.

[soltysh]

Great, can we make sure this is listed here?

[soltysh]

@soltysh Most of these have the similar answers in this KEP and this KEP is an extension of it https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2043-pod-resource-concrete-assigments

Are these blocking updates?

Yes, those are blocking changes. It doesn't matter the answers are similar, we need them in this document. If they are matching, it's fine to just copy them over.

[ffromani]

thanks, we need one more update please

[ffromani]

I would omit this part, is not relevant for this KEP. I will submit a separate update to both the KEP and the docs later on.

[guptaNswati]

okay. I was also not sure if this is the right place. But still wanted to add a bit of context of Get() is based on List()

[ffromani]

Once this and the comment right below are addressed (basically once this section is rephrased), LGTM

note that even the current most common usage pattern is List + a series of call to Get, the two calls are independent from each other. Is perfectly legal for a client to call Get with a namespace+name pair taken in a different way than the output of List.

[ffromani]

we need to fully specify the behavior of Get in this case, or if, because a mistake or a race or any issue, it is fed with inconsistent data.
Currently the code does this:

	pod, exist := p.podsProvider.GetPodByName(req.PodNamespace, req.PodName)
	if !exist {
		metrics.PodResourcesEndpointErrorsGetCount.WithLabelValues("v1").Inc()
		return nil, fmt.Errorf("pod %s in namespace %s not found", req.PodName, req.PodNamespace)
	}

It seems to me the most logical extension is to take into account the phase of the pod either

1. return empty data and swallow the error
2. return error like pod not found

The API in general (we determined during the fixing and review process of 132028) wants to return data pertaining to running pods which thus actively hold resources.

Therefore, my take is that Get should also error out in the unlikely but possible case it is asked about non-running pods.

I'm open to the other approach or to any other sufficiently motivated approach.

[guptaNswati]

@ffromani
How about this:

Get() returns a valid GetPodResourcesResponse with its current PodResources when the pod is in Running state. But due to some race condition or other inconsistency, it’s possible (though uncommon) for a specified pod to transition to terminated or its Status.Phase is not Running, the call must return a NotFound error.

Do you think, adding an explicit check about the Pod Status.Phase can help? Something like:

pod, exist := p.podsProvider.GetPodByName(req.PodNamespace, req.PodName)
if !exist || pod.Status.Phase != v1.PodRunning {
    metrics.PodResourcesEndpointErrorsGetCount.WithLabelValues("v1").Inc()
    return nil, status.Errorf(codes.NotFound, 
        "pod %s in namespace %s not found or not running", 
        req.PodName, req.PodNamespace)
} 

[guptaNswati]

Or we may be overthinking this. As you pointed out, its possible that Get() mostly (not 100%) returns an error when asked about a non running pod. I think just calling it out in the spec should be enough. Digging bit deeper in the code, Get() should not report a removed pod except a rare race condition https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/pod/pod_manager.go#L198

Your fix in List() to filterOutInactivePods() make sense https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods.go#L1065

If you still think, its better to improve Get(), we can similar check like I proposed above.

[ffromani]

I surely don't want to overcomplicate. The problem here is the insufficient validation and lack of checking, albeit in a corner case. TL;DR: we need to decide what Get does when the phase is not Running. A one-liner is sufficient for me, but it has to be there :)
e.g.
"Get() never fails if the kubelet knows about the pods" (but note this likely misreports resources of terminated pods)
"Get() returns error if the pod is known to the kubelet, but is terminated"

that's it.

[guptaNswati]

@soltysh PTAL, i tried to answers these #5346 (comment).

If these are satisfactory, then the only pending items:

• adding e2e test link. I am still not sure what is an acceptable answer for this which unblocks this PR.
• update Get() based on @ffromani feedback.

[ffromani]

/lgtm

sig-node review: my comment about Get was addressed. We may need to review for the nect cycle (beta2? GA?), but the current changes are sufficient.

[ffromani]

my 2c about e2e tests (k/k/test/e2e_node/podresources_test.go):

• we have FG on/off test dispersed among others, should be clear and have its own block (Context in ginkgo lingo)
• I would like to see more tests for Get to cover e.g. multi-container pods, pods not having exclusive resources
• I would like tests to exercise the expected happy path (List + Get on each returned pod)
• I would like to see a test which compares the output of List and Get in the happy path
• I would like more tests to exercise both the error paths, e.g. pod found but bogus container; asking about terminated pod.

My PR kubernetes/kubernetes#132028 does a bit of cleanup in the e2e tests so it can be wise to wait for it to merge (it's a fix we need anyway) and base the work on top of it

Fixing the test-infra lane to better handle the e2e tests is already ongoing: kubernetes/kubernetes#132345 and can proceed in parallel

My understanding is that doing all the above is fair in the same cycle on which we attempt to graduate to beta.

[soltysh]

There are still answers missing inside the document itself, but since they were provided as comments, I'm willing to conditionally approve as is. Please make sure to either add the missing links before merging this PR, or shortly after.

/approve
the PRR section

[soltysh]

Thank you, this clarifies this a bit more.

[soltysh]

Great, can we make sure this is listed here?

[soltysh]

This still holds, I see Francesco mentioned several tests that were added, can we make sure they are explicitly linked in this document?

[klueska]

There are still answers missing inside the document itself, but since they were provided as comments, I'm willing to conditionally approve as is. Please make sure to either add the missing links before merging this PR, or shortly after.

/approve

the PRR section

Thanks @soltysh. Given the holiday in the US today I don't think @guptaNswati will have time to make the update before tomorrow. I will work with her early next week to ensure all of the information in the comments gets incorporated.

@mrunalp, @haircommander, @SergeyKanzhelev can we please get a sig-node approval to move this forward.

Thanks!

[ffromani]

@mrunalp, @haircommander, @SergeyKanzhelev can we please get a sig-node approval to move this forward.

I think they are all based in the US. I'd be supporting (FWIW) an exception if the KEP slips just because there is no time left for an approval review because of the holidays.

[mrunalp]

Approving based on @klueska review.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guptaNswati, mrunalp, 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