/sig node

/triage accepted

/assign

I was thinking about this issue for a bit, and the root cause is probably that there is not a (good enough) formal spec for the API itself so the implementation is pretty much the reference.
We should probably improve here but not sure how (e.g. where to describe more formally the API, perhaps just in the docs?)

That said, my thoughts about the ideal semantics are:

1. Considering the docs as the most accurate, possibly the only, spec "The List endpoint provides information on resources of running pods, with details such as the id of exclusively allocated CPUs, device id as it was reported by device plugins and id of the NUMA node where these devices are allocated. Also, for NUMA-based machines, it contains the information about memory and hugepages reserved for a container." This alone should be sufficient to exclude pods in terminal phase from the running set.
2. I need to doublecheck the actual behavior of the resource managers. If resources assigned to containers are NOT released until the containers are actually cleared by the system, then the API is returning the correct data because these resources can't be re-allocated to newly admitted containers, and the accounting must reflect that
3. But this brings us to the question: is the allocation behavior of the resource manager documented? should it?

I wonder if pod resources API may report the same set of resources used by two pods - one succeeded and one newly scheduled in a single response?

@ffromani should we try to summarize shortcomings at the page you listed as a starting point? We can start with the list of edge cases like:

• "You cannot rely on data completeness while kubelet is starting"
• "Succeeded or Failed pods may be listed"
• "When container has failed, can resources still be allocated?"
• "Is pod information guaranteed visible when NRI plugins are being called for this pod?"
• "what about init containers resources? When it will be returned and when not?"

I like the idea that podresources API is driven by the fact whether resources are actually in use, not by the Pod status" Thus the resource manager behavior ideally needs to be documented.

@SergeyKanzhelev I totally agree that the behavior should be documented in a more precse way. I'm just not sure that page is the best place for reference docs. I'll check with SIG-docs and get advice.

I wonder if pod resources API may report the same set of resources used by two pods - one succeeded and one newly scheduled in a single response?

From top of my head, I'm not sure we have a code path which prevents this. It should be extremely unlikely (at very least you will need a very limited amount of resources, bypass the scheduler entirely and hit a race within the kubelet) but possible. Should also be transient - the next query should return the expected consistent result. But still, I'm not sure there's prevention for this flow. Also depends on the specifics of resource managers.

The gist is that the podresources API is a thin layer which reformats and exposes the internal data of resource managers. This makes the code very simple, but leaks pretty badly the internal implementation of the resource managers allocation process.

This in turns makes the API not really formally specified and with perhaps poor stability guarantees.

I think the best approach would be extending https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubelet/pkg/apis/podresources/v1/api.proto#L36 to enable the client side to filter out or not pod in terminal phase. This way we fix the issue AND we preserve backward compatible.

This of course assuming we can extend the message in a backward compatible way. It should be the case, but we need to doublecheck.

+1 I like the ability to allow the client to filter the result based on pod phase or in general exposing the pod phase allowing the client to filter out the pods themselves.

The next question would be if such a change deserves a KEP or not. I quite strongly feel that this would still fall under the "bugfix" category - we had a rich history about "invasive" and sometimes behavior-changing fixes, and this change hardly look as a RFE.
But before to move forward with that we need a better spec of the API behavior and more formalized guarantees. That would be the next step.