Introducing isExclusive field as part of ContainerResources in PodResource API

[swatisehgal]

Rationale behind this change is explained in detail here: #102190

A pod with same non-integral CPU request and limit belongs to Guaranteed
QoS class but obtains CPUs from the shared pool. Currently, in podresource API
there is no way to distinguish such pods from the pods which have been exclusively
allocated CPUs.

One of the primary goals of recent enhancements of PodResource API is to allow it to
enable node monitoring agents to know the allocatable compute resources on a node,
in order to calculate the node compute resource utilization. However, not being
able to determine if a pod has been exclusively allocated CPUs or CPUs belong to
the shared pool means that the goal cannot be realized.

We need to enhance the podresource API to give it the ability to determine if a pod
belongs to the shared pool or exclusive pool to be able to do proper accounting in
the node monitoring agents.

Signed-off-by: Swati Sehgal swsehgal@redhat.com

What type of PR is this?

/kind bug
/kind api-change

What this PR does / why we need it:

This PR introduces a new boolean field called isExclusive as part of ContainerResources in PodResource API to distinguish if the CPUs are exclusively allocated to a container or are allocated from shared pool.

Which issue(s) this PR fixes:

Fixes # #102190

Documentation update: kubernetes/website#28488

Does this PR introduce a user-facing change?

Kubelet podresources API provides information if a pod has been exclusively allocated CPUs.

Special notes for your reviewer:

This issue was discussed in SIG Node on 1st June 2021 and the SIG Node chair (Dawn Chen) agreed to treat this as a bug to be fixed in 1.22 release timeframe.

[swatisehgal]

/sig node

[ffromani]

This looks like a leftover of a past revision

[swatisehgal]

Good catch! Thanks, fixed it.

[fejta-bot]

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

[ehashman]

/triage accepted
/priority important-soon

[SergeyKanzhelev]

@ruiwen-zhao can you please take a look?

[ruiwen-zhao]

can we also add to the comment what the boolean is?

[swatisehgal]

Sure, done!

[ruiwen-zhao]

just trying to understand why we are changing to use millis here? The tests dont seem to use fractional CPU request?

[swatisehgal]

In the commit here: 739f11b I have added a test case with fractional CPU request, which demonstrates the case of guaranteed pods with fractional CPU request thereby obtaining CPUs from the shared pool.

[swatisehgal]

@ruiwen-zhao I have addressed the questions/comments you had. Could you take another look?

[ruiwen-zhao]

/lgtm
/approve

Thanks!

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ruiwen-zhao, swatisehgal
To complete the pull request process, please assign mrunalp after the PR has been reviewed.
You can assign the PR to them by writing /assign @mrunalp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/kubelet/OWNERS
• staging/src/k8s.io/kubelet/OWNERS
• test/e2e_node/OWNERS

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

[marquiz]

Sorry to be blunt, but to me this API change looks very kludge-ish without enough long-term perspective 😸 My main concerns are

• this change addresses one specific use case
• this change too heavily reflects the internal state/implementation of kubelet in the API

My suggestion would be to extend the API with full information of the resource requests and limits. This would allow the consumer to derive isExclusive, and much more.

     map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> requests = 5;
     map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> limits = 6;

[swatisehgal]

The solution proposed here is to handle a corner case of guaranteed pods requesting non-integral CPUs and not being able to determine what comprises the shared pool. This issue was also raised by @derekwaynecarr during the enhancements made to the podresource API here.

IMO, the solution proposed here is a non-invasive backward compatible solution that does not alter the original goal of the podresource API.

this change too heavily reflects the internal state/implementation of kubelet in the API

Reflecting the internal state of kubelet in terms of the resources allocated is essentially the goal of pod resource API. It returns information about the kubelet's assignment of concrete resources (devices with their numa ID, cpuids and recently memory) allocated to the containers. Please refer to kubernetes/enhancements#1884, #93243 and #95734. Podresource API allows monitoring agents to gain visibility into how those resources are allocated and the allocated resources for use cases that can be seen here. The information remains local to the node where the endpoint can be accessed by a monitoring agent running on the node and not exposed outside of the node so not sure what the cause of concern is.

My suggestion would be to extend the API with full information of the resource requests and limits. This would allow the consumer to derive isExclusive, and much more.

 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> requests = 5;
 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> limits = 6;

Having put some thought into this here are some of my concerns with this:

1. This appears to be a way to determine what the pod spec looked like from resource point of view (something which can be obtained by querying the API server) and doesn’t seem to align with the original intent of providing a way to node local clients to determine info on Kubelet's concrete resource assignment.
2. Kubelet already evaluates a pod's QoS and whether it was allocated exclusive CPUs or not based on its resource requests and limits. This evaluation is not trivial and expecting the client to perform this evaluation and essentially duplicating what Kubelet does, makes the consumption harder for the clients.
3. This edge case we have encountered is only in case of CPUs. The only reason we run into this issue is because guaranteed pods requesting CPUs can still belong to shared pool (in case of non-integral CPUs). However, this is not a case with any other resource. So exposing the request and limit of all the possible resources requested by pod as part of podresource API seems like an overkill to me.
4. Based on my experience expanding the scope of the API without concrete use cases just to make it future proof can be a recipe for disaster. Could you please provide use cases that would benefit from this change so we can understand the motivation a bit better?

[marquiz]

IMO, the solution proposed here is a non-invasive backward compatible solution

I agree that it's non-invasive and backward compatible. But I think it's short-sighted, addressing the only nearest bump in front of our front tire.

this change too heavily reflects the internal state/implementation of kubelet in the API

Reflecting the internal state of kubelet in terms of the resources allocated is essentially the goal of pod resource API.

I don't believe the API should be too tightly coupled with the (current) internal implementation details of kubelet. Just from the top of my head what if kubelet starts to support "mixed allocations" with some exclusive and some shared cpus per container(?)

My suggestion would be to extend the API with full information of the resource requests and limits. This would allow the consumer to derive isExclusive, and much more.

 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> requests = 5;
 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> limits = 6;

Having put some thought into this here are some of my concerns with this:

1. This appears to be a way to determine what the pod spec looked like from resource point of view (something which can be obtained by querying the API server) and doesn’t seem to align with the original intent of providing a way to node local clients to determine info on Kubelet's concrete resource assignment.

We have the cpu ids.

Second, a bit unrelated, you can't do proper accounting of the cpus just by based on the IDs. I.e. with the isExclusive flag added we still fall short on e.g. burstable containers.

2. Kubelet already evaluates a pod's QoS and whether it was allocated exclusive CPUs or not based on its resource requests and limits. This evaluation is not trivial and expecting the client to perform this evaluation and essentially duplicating what Kubelet does, makes the consumption harder for the clients.

Maybe the pod qos class should be added into PodResources message instead, then(?) I don't know if this is a good idea (either 😉 ) but at least the pod qos class is somewhat de-facto standardized property of the pod. Another way would be to have library function for evaluating this from requests and limits so that not all consumers need to re-implement it.

3. This edge case we have encountered is only in case of CPUs. The only reason we run into this issue is because guaranteed pods requesting CPUs can still belong to shared pool (in case of non-integral CPUs). However, this is not a case with any other resource. So exposing the request and limit of all the possible resources requested by pod as part of podresource API seems like an overkill to me.

For now, it's only cpus, future has might bring surprises, who knows. OTOH, I can't see what harm including the complete information about resource requests would do.

4. Based on my experience expanding the scope of the API without concrete use cases just to make it future proof can be a recipe for disaster. Could you please provide use cases that would benefit from this change so we can understand the motivation a bit better?

I wouldn't see it as expanding the scope. Rather making it more complete. Maybe it's about perspective 😄 I think overall we should learn something from the way Linux kernel community designs apis.

One use case is tracking burstable containers (which I already mentioned above).

[AlexeyPerevalov]

IMO, the solution proposed here is a non-invasive backward compatible solution

I agree that it's non-invasive and backward compatible. But I think it's short-sighted, addressing the only nearest bump in front of our front tire.

this change too heavily reflects the internal state/implementation of kubelet in the API

Reflecting the internal state of kubelet in terms of the resources allocated is essentially the goal of pod resource API.

I don't believe the API should be too tightly coupled with the (current) internal implementation details of kubelet. Just from the top of my head what if kubelet starts to support "mixed allocations" with some exclusive and some shared cpus per container(?)

My suggestion would be to extend the API with full information of the resource requests and limits. This would allow the consumer to derive isExclusive, and much more.

 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> requests = 5;
 map<string, k8s.io.apimachinery.pkg.api.resource.Quantity> limits = 6;

Having put some thought into this here are some of my concerns with this:

1. This appears to be a way to determine what the pod spec looked like from resource point of view (something which can be obtained by querying the API server) and doesn’t seem to align with the original intent of providing a way to node local clients to determine info on Kubelet's concrete resource assignment.

We have the cpu ids.

Second, a bit unrelated, you can't do proper accounting of the cpus just by based on the IDs. I.e. with the isExclusive flag added we still fall short on e.g. burstable containers.

1. Kubelet already evaluates a pod's QoS and whether it was allocated exclusive CPUs or not based on its resource requests and limits. This evaluation is not trivial and expecting the client to perform this evaluation and essentially duplicating what Kubelet does, makes the consumption harder for the clients.

Maybe the pod qos class should be added into PodResources message instead, then(?) I don't know if this is a good idea (either 😉 ) but at least the pod qos class is somewhat de-facto standardized property of the pod. Another way would be to have library function for evaluating this from requests and limits so that not all consumers need to re-implement it.

1. This edge case we have encountered is only in case of CPUs. The only reason we run into this issue is because guaranteed pods requesting CPUs can still belong to shared pool (in case of non-integral CPUs). However, this is not a case with any other resource. So exposing the request and limit of all the possible resources requested by pod as part of podresource API seems like an overkill to me.

For now, it's only cpus, future has might bring surprises, who knows. OTOH, I can't see what harm including the complete information about resource requests would do.

1. Based on my experience expanding the scope of the API without concrete use cases just to make it future proof can be a recipe for disaster. Could you please provide use cases that would benefit from this change so we can understand the motivation a bit better?

I wouldn't see it as expanding the scope. Rather making it more complete. Maybe it's about perspective 😄 I think overall we should learn something from the way Linux kernel community designs apis.

One use case is tracking burstable containers (which I already mentioned above).

I found that providing requests and limits as well as qos for podresources api is controversial.
It adds redundancy into podresources api.
Now we can identify whether cpu was exclusively allocated indirectly - get the appropriate pod by client-go (or have prefetched list, it doesn't matter) here we could know QoS and integral amount of request and limits
and we can get KubeletConfiguration to make sure CPUManager uses static policy.
The original motivation of this commit was to simplify topology exporter, to reduce data flows.
If we want to generalize podersouces interface, it mostly the task for the next version (v2).

[swatisehgal]

Second, a bit unrelated, you can't do proper accounting of the cpus just by based on the IDs. I.e. with the isExclusive flag added we still fall short on e.g. burstable containers.

If we are talking about accounting in the context of Topology aware scheduling, considering burstable pods for accounting does not makes sense as burstable pods (just like best effort pods and guaranteed pod with integral CPUs) obtain CPUs from the shared pool. Because CPU manager only allocates exclusive CPUs to guaranteed pods with integral CPU requests (and limit), the rest (non-integral guaranteed, best effort, burstable pods) obtain CPUs from a shared pool. When CPUs are exclusively allocated we know the CPUIds allocated to the pod and can therefore determine the corresponding NUMA but how do you account for CPUs allocated (and therefore) available at a per NUMA level when the CPUs are in the shared pool? All we know is the CPUIds corresponding to the shared pool and even if we knew the amount of CPU requested (as per the approach you are suggesting) it does not help us because we still can't determine NUMA distribution of CPUs for a pool that is being shared by all burstable, best-effort and guaranteed non-integral pods.

Maybe the pod qos class should be added into PodResources message instead, then(?) I don't know if this is a good idea (either ) but at least the pod qos class is somewhat de-facto standardized property of the pod. Another way would be to have library function for evaluating this from requests and limits so that not all consumers need to re-implement it.

Adding QoS to Podresource does not solve the problem at hand here as we are trying to differentiate a pod which is exclusively allocated CPUs and the ones that belong to the shared pool. A guaranteed pod with non-integral CPUs can belong to the shared pool and those are the ones we should not account for the reasons I explained above.

I wouldn't see it as expanding the scope. Rather making it more complete. Maybe it's about perspective I think overall we should learn something from the way Linux kernel community designs apis.

It is indeed about perspective :)

[marquiz]

I found that providing requests and limits as well as qos for podresources api is controversial.
It adds redundancy into podresources api.

In what way? You cannot derive that information from the other data.

The original motivation of this commit was to simplify topology exporter, to reduce data flows.

I can see that, and that is a good motivation 😸 But I think it falls short, why be so short-sighted and only address the very problem at our hands just now? 🤔

[marquiz]

If we are talking about accounting in the context of Topology aware scheduling

I think an API shouldn't be designed for one specific consumer/usage scenario alone. There might be other users to the API than just TAS. With all the work going on there is probably interest for other scheduler extensions, too.

Adding QoS to Podresource does not solve the problem at hand here as we are trying to differentiate a pod which is exclusively allocated CPUs and the ones that belong to the shared pool. A guaranteed pod with non-integral CPUs can belong to the shared pool and those are the ones we should not account for the reasons

True, It possibly wouldn't bring much value.

But what about "mixed allocation" that I mentioned earlier (i.e. if in the future kubelet would allocate exclusive plus shared cpus for the same container)? Perhaps having e.g. something like

repeated int64 exclusive_cpu_ids

would be more future-proof?

It is indeed about perspective :)

What is your perspective on "sustainable" API design? 😉

[swatisehgal]

I think an API shouldn't be designed for one specific consumer/usage scenario alone. There might be other users to the API than just TAS.

Right, we shouldn't be defining the API based on a very specific use case. Let's put Topology aware scheduling aside, you mentioned tracking burstable pods as an example but I double checked the List endpoint and noted that burstable pods are already exposed as part of List response. At the moment it is not clear to me what we are trying to achieve by introducing request and limit information in the podresource API itself? Unfortunately, the value proposition is not very clear to me yet.

With all the work going on there is probably interest for other scheduler extensions, too.

Can you elaborate on what Scheduling extensions you are referring to here? If we have a specific use case there is no problem extending the API but like I said from client perspective it seems to be adding a lot of complexity trying to solve a problem which can be solved in more cleaner and simpler manner.

But what about "mixed allocation" that I mentioned earlier (i.e. if in the future kubelet would allocate exclusive plus shared cpus for the same container)? Perhaps having e.g. something like

repeated int64 exclusive_cpu_ids

would be more future-proof?

I am okay with addition of exclusive_cpu_ids field instead of isExclusive. From client perspective, this is simple and all they have to do is to compare this field with the cpu_ids field to determine if all the CPUs allocated are exclusive!

What is your perspective on "sustainable" API design?

I am all for "sustainable" API but we have to strike a balance here to make sure that we introduce a change that A) aligns with the goals and intents of the API and B) has a strong use case to back the API change. Changing the API for something we "might" need is not ideal.

[swatisehgal]

Perhaps having e.g. something like

repeated int64 exclusive_cpu_ids

would be more future-proof?

I am okay with addition of exclusive_cpu_ids field instead of isExclusive. From client perspective, this is simple and all they have to do is to compare this field with the cpu_ids field to determine if all the CPUs allocated are exclusive!

I will create a patch with the addition of exclusive_cpu_ids field instead of isExclusive and link it here.

[askervin]

From the original description:

One of the primary goals of recent enhancements of PodResource API is to allow it to
enable node monitoring agents to know the allocatable compute resources on a node,

This sounds good!

However, not being able to determine if a pod has been exclusively allocated
CPUs or CPUs belong to the shared pool means that the goal cannot be realized.

I see the point, but I also see a danger. In this API design there is a built-in assumption on the nature of CPU pools. The assumption is that CPUs would be either from the shared CPU pool, or they would be exclusive to a container. In use cases where this assumption is true, I think it is perfectly fine that a monitoring agent makes this assumption, too. But if you make this assumption in the API design, you cannot build sensible monitoring agents for different use cases using that API.

Please consider the podpools as a real-life example of a different use case. This policy partitions CPUs of a node into pools whose capacity can be configured in the number of pods that are running in the same pool. This is very useful for efficient resource usage in certain cases where you know what kind of workloads will fill your node.

For instance, there can be following pool split on the node:

| CPU |  Pool
+-----+-------------------------
|cpu0 | Reserved
|cpu1 | (kube-system pods)
+-----+-------------------------
|cpu2 | Single-pod pool A
|cpu3 | - runs all containers
|cpu4 |   of a single pod
|cpu5 |
+-----+-------------------------
|cpu6 | Single-pod pool B
|cpu7 | - runs all containers
|cpu8 |   of a single pod
|cpu9 |
+-----+-------------------------
:     :
+-----+-------------------------
|cpuK | Quad-pod pool A
|cpuL | - runs all containers
|cpuM |   of at most 4 pods
|cpuN |
+-----+-------------------------
:     :
+-----+-------------------------
|cpuX | Shared pool
|cpuY | - runs non-pooled
|cpuZ |   workloads
+-----+-------------------------

Therefore, I would prefer an API design that exposes the resource reservation information in the format that is independent of the use case. In other words, I would prefer a design that makes minimal or no assumptions on what the reservation algorithm might have thought when making reservations that are reported.

[swatisehgal]

@marquiz @askervin Thanks for your input and all the discussion on this PR. Given that you both have objections on the proposed API changes as part of this PR, I am closing this PR. We'd be looking at alternative solutions to solve this issue.