Repository: intel/intel-device-plugins-for-kubernetes
Pull request #1819 (2024-08-28 11:09:31)

Add cpu/memory requests and limits
Operator maturity level 3 requires cpu/memory requests and limits for operands. Add them to all plugins deployed by operator.

hj-johannes-lee: dlb plugin
%CPU 0.7-3.3 (avg: 0.78) RES: 12544

dsa plugin
%CPU 1.0-4.2 (avg: 1.65) RES: 20796

fpga plugin
%CPU 0.3-7.4 (avg: 1.01) RES: 25716

gpu plugin
%CPU 1.7-3.3 (avg. 2.73) RES: 44556

iaa plugin
%CPU 1.0-5.2 (avg: 1.61) RES: 21260

qat plugin
%CPU 3.3-6.2 (avg: 3.48) RES: 14336

sgx plugin
%CPU 0.7-2.0 (avg: 0.89) RES: 12544

Based on this, I set the values in the yaml files. Please review.!
hj-johannes-lee: I set
2* the maximum figure that I see => requests
and 2* of reqeusts => limits

mythi: > I set 2* the maximum figure that I see => requests and 2* of reqeusts => limits
> 
> If you think it is fine, let me update the test files.!

@eero-t had this suggestion:
"If the values are really small, I'd round them up quite a bit. E.g. ~40m at least to 100m (0.1 CPU cores)."
hj-johannes-lee: I 'rounded them up" and made them to be minimum 40m. Didn't he mean that?
Do you think he meant to "add up" 40m-100m more?

eero-t: > Or.. if it is less than 40m, make it to be 100m? Sorry, that is not really clear to me.

I meant that if 2x CPU usage is <100m, just use 100m, because it's better for the limits to be (clearly) larger than needed, that process being needlessly throttled.

Memory throttling means paging, which means that perf of **all** processes in that node will suffer in effort to keep given container's memory usage within some artificial limit.
eero-t: > I meant that if 2x CPU usage is <100m, just use 100m, because it's better for the limits to be (clearly) larger than needed, that process being needlessly throttled.
> 
> Memory throttling means paging, which means that perf of **all** processes in that node will suffer in effort to keep given container's memory usage within some artificial limit.

Another reason for having higher limits is that plugins resource usage is not going to be measured and limits updated for every release.  Therefore there should be plenty of extra space in case plugin size / resource usage increases due to added features (GPU plugin is largest, so it can act as guideline how large they could grow).

Scheduler is not going to block additional workloads going to a node usage because it has couple of pods requesting smallish fraction of single core. :-)
mythi: > If you think it is fine, let me update the test files.!

It would be good to do this anyways to get the e2e tests going. It would also be useful to come up with a simple test that tries to stress the plugins a bit.
hj-johannes-lee: @eero-t Thanks for the explanation.!
I increased the CPU limits' minimum value as 100m for all plugins.

So, in conclusion:
- requests => 2 * maximum of the value I saw (min. 40m)
- limits => 2 * of reqeusts (min. 100m)

I thought increasing `limits` would be enough from the last commit.
eero-t: > I thought increasing `limits` would be enough from the last commit.

Not that whether `requests` match `limits`, changes Pod QoS: https://kubernetes.io/docs/concepts/workloads/pods/pod-qos/
hj-johannes-lee: Isn't Eero pointing out that limits and requests are different and they need to be equal for QoS?
eero-t: My point was about QoS is wanted for the plugin pods?

If it's "Guaranteed", then limits & requests should be equal (and CPU & memory requests should be specified for all containers, if Pod has multiple ones).
hj-johannes-lee: @eero-t Mikko said they do not need to be `guaranteed` but `burstable` would be enough. So, I am keeping them as now.
mythi: I thought about `guaranteed` a bit more. We don't give any possibilities for the users to update these values so we need to select the them carefully. Are we going to see cases where the kubelet starts evicting the plugin pods on overloaded nodes? AFAUI, it should be enough if the `requests` are set so that we don't exceed them in normal cases...
hj-johannes-lee: @mythi Umm, we are adding the cpu/memory `requests` and `limits` because of the maturity level 3 of the operator. So, if only `requests` are set, it still does not fit the criteria of the maturity level 3. Would we then go with guaranteed?
mythi: > So, if only `requests` are set,

I did not propose/mean this. I wrote that if we select this value carefully perhaps we are safe (but we would still set `limits` also). I prefer not to have these in the CRD API to keep it simple.
hj-johannes-lee: > it should be enough if the requests are set so that we don't exceed them in normal cases...

I thought you meant that we do not need limits since it is enough to set requests.

> I did not propose/mean this. I wrote that if we select this value carefully perhaps we are safe (but we would still set limits also).

I think I still do not get your point. We already set requests and limits safely, and you said about `guaranteed`. Are you suggesting to change to be `guaranteed`?

> I prefer not to have these in the CRD API to keep it simple.

'These' means about the part 'giving possibility to users', right?
mythi: > We already set requests and limits safely,

But the plugin pods can still be evicted, right? My question is how likely problem that is with the current settings. Also, have you tested what happens to running workloads that are using devices in case a plugin pod gets evicted. 
hj-johannes-lee: I have not tested about what happens to the workloads when the pod gets evicted.
Would it work like this?

- deploy a pluglin
- run workload
- delete the plugin while running (idk if it can be same as "getting evicted"

If not, what would be the way to make the pod evicted?

---
But to be honest, I do not see the matter that pods may get evicted is related to this PR...
My point is...
Of course, it is good to avoid such situations, but before this PR we did not have any requests/limits => so the plugins have been `BestEfforts` until now.
The current change makes them `Burstable`.

No one cared about the matter of pods getting evicted though they have been the least priority until now...
eero-t: Yes, according to pod k8s QoS doc, a _best-effort_ pod (earlier plugins) will be evicted before _burstable_ ones (this PR).  I assume this to be case even when latter pod **exceeds** it's resource _request_.

However, if pod exceeds its memory **limit**, it gets OOM killed by kernel & restarted by k8s scheduler (if its spec allows restart), and eventually scheduler backs off from restarting it.  Regardless of whether node is short on resources or not.   Something like that could happen if there's a condition that triggers plugin memory leak.
mythi: > but before this PR we did not have any requests/limits

they were still controllable through `LimitRange` for example. Now were are locking things down.
hj-johannes-lee: Thanks for the explanations.!

> However, if pod exceeds its memory limit, it gets OOM killed by kernel & restarted by k8s scheduler (if its spec allows restart), and eventually scheduler backs off from restarting it. Regardless of whether node is short on resources or not. Something like that could happen if there's a condition that triggers plugin memory leak, so it may be relevant to check.

Would this happen even when the pods QoS is guaranteed?

If yes... what do you suggest now?

I am lost. Just, let me know what I need to do next.!
mythi: > what do you suggest now?

I think we all agreed that additional testing is still needed, especially in node cpu/memory pressure cases. I could be implemented as an e2e test also as [suggested](https://github.com/intel/intel-device-plugins-for-kubernetes/pull/1819#issuecomment-2333496867) earlier.
eero-t: > Wouldn't this happen even when the pods QoS is guaranteed?

Yes.  But not for best-effort containers, *if* node has free resources.  Node running out of resources is a case, where many other things could also fail, so it's hard to predict what happens in that case with best-effort containers, so testing such condition was IMHO less of a priority.   Whereas with specified limits, it's clear what happens, when and to which pod, so testing that could make more sense.

> If yes... what do you suggest now?

Testing what happens when limits are crossed..

*Startup*

Issue when new version will cross limits already at startup, can be tested by setting limits too low.  But startup failures are not really interesting, as then there's no chance for workloads to request devices provided by the plugin.

*Run-time*

I do not think kernel allows lowering memory cgroup limits below container's current usage.  Which means that container resource utilization would need to be increased at run-time, instead of limits being lowered.

Because current containers include just static plugin binary, not any tools, their allocation cannot cannot be easily increased by `kubectl exec`ing more stuff in them.  Meaning that plugin or container content needs to be modified to support that.

This could be done either by adding some trivial tool for allocating + dirtying (writing to) given amount of memory, or by adding such option to plugin:
```
-test-alloc-and-use-cpu  Allocate given number of MBs, and busy loop, to simulate large resource usage for testing.
```
(That option should do just that, not any of the other plugin functionality, to avoid messing up things otherwise.)

...and using that at suitable moment in test:
```
kubectl exec -it <plugin pod> <container name> -- /path/to/plugin -test-alloc-and-use-cpu 200
```

mythi: > [1] Workloads that already got a device, still work

if this is the case, then my main concerns is covered. I was wondering if a `guaranteed` QoS class workload would be evicted if the plugin device resources go away.
eero-t: > > [1] Workloads that already got a device, still work
> 
> if this is the case, then my main concerns is covered. I was wondering if a `guaranteed` QoS class workload would be evicted if the plugin device resources go away.

AFAIK scheduler considers extendended resources only when pod is scheduled, and kubelet plugin can do something when the container is created (e.g. assign different device instead), but after that pod is completely on its own in this regard.
mythi: From my side, I'm fine merging once @hj-johannes-lee confirms enough (manual) testing is done  
tkatila: I verified that when a plugin is removed, the workload utilizing the resource will stay on. If another workload is scheduled, it will go into `Pending` state.

mythi: @tkatila thanks!

@hj-johannes-lee the two commits belong to one so please squash the commits and rebase

Patch:
diff --git a/deployments/dlb_plugin/base/intel-dlb-plugin.yaml b/deployments/dlb_plugin/base/intel-dlb-plugin.yaml
index 79d3ecf1..14b903df 100644
--- a/deployments/dlb_plugin/base/intel-dlb-plugin.yaml
+++ b/deployments/dlb_plugin/base/intel-dlb-plugin.yaml
@@ -32,6 +32,13 @@ spec:
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
         terminationMessagePath: /tmp/termination-log          
+        resources:
+          requests:
+            memory: "15Mi"
+            cpu: 40m
+          limits:
+            memory: "30Mi"
+            cpu: 100m
         volumeMounts:
         - name: devfs
           mountPath: /dev
diff --git a/deployments/dsa_plugin/base/intel-dsa-plugin.yaml b/deployments/dsa_plugin/base/intel-dsa-plugin.yaml
index 4e3acf78..4a2a7dae 100644
--- a/deployments/dsa_plugin/base/intel-dsa-plugin.yaml
+++ b/deployments/dsa_plugin/base/intel-dsa-plugin.yaml
@@ -33,6 +33,13 @@ spec:
             type: "container_device_plugin_t"
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
+        resources:
+          requests:
+            memory: "25Mi"
+            cpu: 50m
+          limits:
+            memory: "50Mi"
+            cpu: 100m
         volumeMounts:
         - name: devfs
           mountPath: /dev/dsa
diff --git a/deployments/fpga_plugin/base/intel-fpga-plugin-daemonset.yaml b/deployments/fpga_plugin/base/intel-fpga-plugin-daemonset.yaml
index 787d4add..2e946245 100644
--- a/deployments/fpga_plugin/base/intel-fpga-plugin-daemonset.yaml
+++ b/deployments/fpga_plugin/base/intel-fpga-plugin-daemonset.yaml
@@ -44,6 +44,13 @@ spec:
         securityContext:
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
+        resources:
+          requests:
+            memory: "30Mi"
+            cpu: 80m
+          limits:
+            memory: "60Mi"
+            cpu: 160m
         volumeMounts:
         - name: devfs
           mountPath: /dev
diff --git a/deployments/gpu_plugin/base/intel-gpu-plugin.yaml b/deployments/gpu_plugin/base/intel-gpu-plugin.yaml
index b64b5f10..5ea86975 100644
--- a/deployments/gpu_plugin/base/intel-gpu-plugin.yaml
+++ b/deployments/gpu_plugin/base/intel-gpu-plugin.yaml
@@ -36,6 +36,13 @@ spec:
             type: "container_device_plugin_t"
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
+        resources:
+          requests:
+            memory: "45Mi"
+            cpu: 40m
+          limits:
+            memory: "90Mi"
+            cpu: 100m
         volumeMounts:
         - name: devfs
           mountPath: /dev/dri
diff --git a/deployments/iaa_plugin/base/intel-iaa-plugin.yaml b/deployments/iaa_plugin/base/intel-iaa-plugin.yaml
index 3821a81a..037179a9 100644
--- a/deployments/iaa_plugin/base/intel-iaa-plugin.yaml
+++ b/deployments/iaa_plugin/base/intel-iaa-plugin.yaml
@@ -33,6 +33,13 @@ spec:
             type: "container_device_plugin_t"
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
+        resources:
+          requests:
+            memory: "25Mi"
+            cpu: 60m
+          limits:
+            memory: "50Mi"
+            cpu: 120m
         volumeMounts:
         - name: devfs
           mountPath: /dev/iax
diff --git a/deployments/qat_plugin/base/intel-qat-plugin.yaml b/deployments/qat_plugin/base/intel-qat-plugin.yaml
index 4f4dbf74..fd007510 100644
--- a/deployments/qat_plugin/base/intel-qat-plugin.yaml
+++ b/deployments/qat_plugin/base/intel-qat-plugin.yaml
@@ -37,6 +37,13 @@ spec:
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
         imagePullPolicy: IfNotPresent
+        resources:
+          requests:
+            memory: "15Mi"
+            cpu: 70m
+          limits:
+            memory: "30Mi"
+            cpu: 140m
         volumeMounts:
         - name: devdir
           mountPath: /dev/vfio
diff --git a/deployments/sgx_plugin/base/intel-sgx-plugin.yaml b/deployments/sgx_plugin/base/intel-sgx-plugin.yaml
index 078cbf3d..3ce4073e 100644
--- a/deployments/sgx_plugin/base/intel-sgx-plugin.yaml
+++ b/deployments/sgx_plugin/base/intel-sgx-plugin.yaml
@@ -28,6 +28,13 @@ spec:
           readOnlyRootFilesystem: true
           allowPrivilegeEscalation: false
         imagePullPolicy: IfNotPresent
+        resources:
+          requests:
+            memory: "15Mi"
+            cpu: 40m
+          limits:
+            memory: "30Mi"
+            cpu: 100m
         volumeMounts:
         - name: kubeletsockets
           mountPath: /var/lib/kubelet/device-plugins


Test patch:
diff --git a/pkg/controllers/dlb/controller_test.go b/pkg/controllers/dlb/controller_test.go
index 7a4ed796..1d318ed1 100644
--- a/pkg/controllers/dlb/controller_test.go
+++ b/pkg/controllers/dlb/controller_test.go
@@ -21,6 +21,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -92,6 +93,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("100m"),
+									v1.ResourceMemory: resource.MustParse("30Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("40m"),
+									v1.ResourceMemory: resource.MustParse("15Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "devfs",
diff --git a/pkg/controllers/dsa/controller_test.go b/pkg/controllers/dsa/controller_test.go
index 370107a8..3d8458d3 100644
--- a/pkg/controllers/dsa/controller_test.go
+++ b/pkg/controllers/dsa/controller_test.go
@@ -20,6 +20,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -96,6 +97,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("100m"),
+									v1.ResourceMemory: resource.MustParse("50Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("50m"),
+									v1.ResourceMemory: resource.MustParse("25Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "devfs",
diff --git a/pkg/controllers/fpga/controller_test.go b/pkg/controllers/fpga/controller_test.go
index 1471477b..a16ef0a9 100644
--- a/pkg/controllers/fpga/controller_test.go
+++ b/pkg/controllers/fpga/controller_test.go
@@ -21,6 +21,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -94,6 +95,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								AllowPrivilegeEscalation: &no,
 							},
 							TerminationMessagePath: "/tmp/termination-log",
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("160m"),
+									v1.ResourceMemory: resource.MustParse("60Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("80m"),
+									v1.ResourceMemory: resource.MustParse("30Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									MountPath: "/dev",
diff --git a/pkg/controllers/gpu/controller_test.go b/pkg/controllers/gpu/controller_test.go
index 9edda978..b06f707a 100644
--- a/pkg/controllers/gpu/controller_test.go
+++ b/pkg/controllers/gpu/controller_test.go
@@ -22,6 +22,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -106,6 +107,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("100m"),
+									v1.ResourceMemory: resource.MustParse("90Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("40m"),
+									v1.ResourceMemory: resource.MustParse("45Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "devfs",
diff --git a/pkg/controllers/iaa/controller_test.go b/pkg/controllers/iaa/controller_test.go
index fd4c9183..5d451d9e 100644
--- a/pkg/controllers/iaa/controller_test.go
+++ b/pkg/controllers/iaa/controller_test.go
@@ -20,6 +20,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -96,6 +97,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("120m"),
+									v1.ResourceMemory: resource.MustParse("50Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("60m"),
+									v1.ResourceMemory: resource.MustParse("25Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "devfs",
diff --git a/pkg/controllers/qat/controller_test.go b/pkg/controllers/qat/controller_test.go
index fed489d3..d9432340 100644
--- a/pkg/controllers/qat/controller_test.go
+++ b/pkg/controllers/qat/controller_test.go
@@ -21,6 +21,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -100,6 +101,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("140m"),
+									v1.ResourceMemory: resource.MustParse("30Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("70m"),
+									v1.ResourceMemory: resource.MustParse("15Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "devdir",
diff --git a/pkg/controllers/sgx/controller_test.go b/pkg/controllers/sgx/controller_test.go
index 971b5be8..97fcfbfc 100644
--- a/pkg/controllers/sgx/controller_test.go
+++ b/pkg/controllers/sgx/controller_test.go
@@ -21,6 +21,7 @@ import (
 
 	apps "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/diff"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -88,6 +89,16 @@ func (c *controller) newDaemonSetExpected(rawObj client.Object) *apps.DaemonSet
 								ReadOnlyRootFilesystem:   &yes,
 								AllowPrivilegeEscalation: &no,
 							},
+							Resources: v1.ResourceRequirements{
+								Limits: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("100m"),
+									v1.ResourceMemory: resource.MustParse("30Mi"),
+								},
+								Requests: v1.ResourceList{
+									v1.ResourceCPU:    resource.MustParse("40m"),
+									v1.ResourceMemory: resource.MustParse("15Mi"),
+								},
+							},
 							VolumeMounts: []v1.VolumeMount{
 								{
 									Name:      "kubeletsockets",