Repository: kubernetes-sigs/kueue

Issue:
PropagateResourceRequests always generates diff on CPU
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!

If the matter is security related, please disclose it privately via https://kubernetes.io/security/
-->


PropagateResourceRequests always generates a diff on CPU apparently, which was spamming reconciles

from added logging
```
Mismatch in resource requests for workload main: main != main, OR amounts: true
resources: v1.ResourceList{"cpu":resource.Quantity{i:resource.int64Amount{value:10, scale:0}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"10", Format:"DecimalSI"}}
  rlList: v1.ResourceList{"cpu":resource.Quantity{i:resource.int64Amount{value:10000, scale:-3}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"", Format:"DecimalSI"}}
```
okay, integral CPU vs millicpu

https://github.com/kubernetes-sigs/kueue/blob/3279d9c05817e465229fac6bdc64250c890ea7dd/pkg/resources/requests.go#L102-L104

let's try milli cpu with a non-integral value
```
resources: v1.ResourceList{"cpu":resource.Quantity{i:resource.int64Amount{value:9001, scale:-3}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"9001m", Format:"DecimalSI"}}
  rlList: v1.ResourceList{"cpu":resource.Quantity{i:resource.int64Amount{value:9001, scale:-3}, d:resource.infDecAmount{Dec:(*inf.Dec)(nil)}, s:"", Format:"DecimalSI"}}
```
oh...the internally generated private string field differs during reflection comparison...because maps.Equal just calls direct struct equality for the entries https://cs.opensource.google/go/go/+/refs/tags/go1.24.2:src/maps/maps.go;l=22

switching to cmp.Equal resolves that


**What happened**:

they are not equal

**What you expected to happen**:

equality should pass for semantically equal values

**How to reproduce it (as minimally and precisely as possible)**:

Create an integration test with workloads blocked on CPU quota using integral CPU requests (e.g. "10"), it will generate a diff. Add logs in code to see the value of private fields that are different.

**Anything else we need to know?**:

**Environment**:
- Kubernetes version (use `kubectl version`):
- Kueue version (use `git describe --tags --dirty --always`):
- Cloud provider or hardware configuration:
- OS (e.g: `cat /etc/os-release`):
- Kernel (e.g. `uname -a`):
- Install tools:
- Others:

Discussion:
@tenzen-y: Thank you for reporting this. Could you point out the place where you fase the noisy logs? Or could you pointing out the noisy log messages?

> switching to cmp.Equal resolves that

The `cmp.Equal` has high computing costs. So in k/k, actually, it is prohibited from being used in production code.
So, I guess we should use Quantity Equal function someting like

```go
var a, b resource.Quantity
a.Equal(b)
```@alexeldeib: oh sorry! the logs are my addition inside the func -- I was seeing [this func](https://github.com/kubernetes-sigs/kueue/blob/3279d9c05817e465229fac6bdc64250c890ea7dd/pkg/scheduler/scheduler.go#L657) return true when I expected no diff, so I added my own debug logs to validate

```diff
diff --git a/pkg/workload/workload.go b/pkg/workload/workload.go
index e646c9244..f2706b2f2 100644
--- a/pkg/workload/workload.go
+++ b/pkg/workload/workload.go
@@ -654,6 +654,8 @@ func PropagateResourceRequests(w *kueue.Workload, info *Info) bool {
                for idx := range w.Status.ResourceRequests {
                        if w.Status.ResourceRequests[idx].Name != info.TotalRequests[idx].Name ||
                                !cmp.Equal(w.Status.ResourceRequests[idx].Resources, info.TotalRequests[idx].Requests.ToResourceList()) {
+                               fmt.Printf("Mismatch in resource requests for workload %s: %s != %s, OR amounts: %t\n", w.Status.ResourceRequests[idx].Name, w.Status.ResourceRequests[idx].Name, info.TotalRequests[idx].Name, !maps.Equal(w.Status.ResourceRequests[idx].Resources, info.TotalRequests[idx].Requests.ToResourceList()))
+                               fmt.Printf("resources: %#+v\n  rlList: %#+v\n", w.Status.ResourceRequests[idx].Resources, info.TotalRequests[idx].Requests.ToResourceList())
                                match = false
                                break
                        }
@@ -661,6 +663,8 @@ func PropagateResourceRequests(w *kueue.Workload, info *Info) bool {
                if match {
                        return false
                }
+       } else {
+               fmt.Printf("Mismatch in resource requests for workload %s: %d != %d\n", w.Name, len(w.Status.ResourceRequests), len(info.TotalRequests))
        }
 
        res := make([]kueue.PodSetRequest, len(info.TotalRequests))
```

> The cmp.Equal has high computing costs. So in k/k, actually, it is prohibited from being used in production code.
So, I guess we should use Quantity Equal function someting like

TIL, makes sense

@alexeldeib: /assign

might roll a fix into https://github.com/kubernetes-sigs/kueue/pull/4935 but will tackle it either way@PBundyra: cc @gabesaba 

Solution patch:
diff --git a/charts/kueue/templates/manager/manager.yaml b/charts/kueue/templates/manager/manager.yaml
index 06dff5922..98d2c81b4 100644
--- a/charts/kueue/templates/manager/manager.yaml
+++ b/charts/kueue/templates/manager/manager.yaml
@@ -67,6 +67,8 @@ spec:
         - mountPath: /tmp/k8s-webhook-server/serving-certs
           name: cert
           readOnly: true
+        - mountPath: /visibility
+          name: visibility
         - mountPath: /controller_manager_config.yaml
           name: manager-config
           subPath: controller_manager_config.yaml
@@ -95,6 +97,8 @@ spec:
       - name: manager-config
         configMap:
           name: {{ include "kueue.fullname" . }}-manager-config
+      - name: visibility
+        emptyDir: {}
       {{- if .Values.enableCertManager }}
       - name: metrics-certs
         secret:
diff --git a/charts/kueue/values.yaml b/charts/kueue/values.yaml
index 0ccd09fd8..fd38f1b92 100644
--- a/charts/kueue/values.yaml
+++ b/charts/kueue/values.yaml
@@ -30,6 +30,7 @@ controllerManager:
       seccompProfile:
         type: RuntimeDefault
     containerSecurityContext:
+      readOnlyRootFilesystem: true
       allowPrivilegeEscalation: false
       capabilities:
         drop:
diff --git a/config/components/manager/manager.yaml b/config/components/manager/manager.yaml
index 7b3fd2779..ad15226d4 100644
--- a/config/components/manager/manager.yaml
+++ b/config/components/manager/manager.yaml
@@ -29,6 +29,7 @@ spec:
         name: manager
         securityContext:
           allowPrivilegeEscalation: false
+          readOnlyRootFilesystem: true
           capabilities:
             drop:
               - ALL
diff --git a/config/default/manager_visibility_patch.yaml b/config/default/manager_visibility_patch.yaml
index 7e0104927..3c66ad904 100644
--- a/config/default/manager_visibility_patch.yaml
+++ b/config/default/manager_visibility_patch.yaml
@@ -13,3 +13,11 @@ spec:
           - containerPort: 8082
             name: visibility
             protocol: TCP
+          # Visibility mounts certificates to this location
+          # this is hard coded.
+          volumeMounts:
+          - mountPath: /visibility
+            name: visibility
+      volumes:
+      - name: visibility
+        emptyDir: {}
diff --git a/pkg/visibility/server.go b/pkg/visibility/server.go
index b6a8623b6..91bf54811 100644
--- a/pkg/visibility/server.go
+++ b/pkg/visibility/server.go
@@ -52,6 +52,7 @@ var (
 		validatingwebhook.PluginName,
 		mutatingwebhook.PluginName,
 	}
+	certDir = "/visibility"
 )
 
 // +kubebuilder:rbac:groups=flowcontrol.apiserver.k8s.io,resources=prioritylevelconfigurations,verbs=list;watch
@@ -88,7 +89,7 @@ func applyVisibilityServerOptions(config *genericapiserver.RecommendedConfig) er
 	o.Etcd = nil
 	o.SecureServing.BindPort = 8082
 	// The directory where TLS certs will be created
-	o.SecureServing.ServerCert.CertDirectory = "/tmp"
+	o.SecureServing.ServerCert.CertDirectory = certDir
 	o.Admission.DisablePlugins = disabledPlugins
 	if err := o.SecureServing.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {
 		return fmt.Errorf("error creating self-signed certificates: %v", err)
diff --git a/pkg/workload/workload.go b/pkg/workload/workload.go
index f66e871d6..4181c3256 100644
--- a/pkg/workload/workload.go
+++ b/pkg/workload/workload.go
@@ -26,6 +26,7 @@ import (
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -652,7 +653,7 @@ func PropagateResourceRequests(w *kueue.Workload, info *Info) bool {
 		match := true
 		for idx := range w.Status.ResourceRequests {
 			if w.Status.ResourceRequests[idx].Name != info.TotalRequests[idx].Name ||
-				!maps.Equal(w.Status.ResourceRequests[idx].Resources, info.TotalRequests[idx].Requests.ToResourceList()) {
+				!equality.Semantic.DeepEqual(w.Status.ResourceRequests[idx].Resources, info.TotalRequests[idx].Requests.ToResourceList()) {
 				match = false
 				break
 			}



Test patch:
diff --git a/pkg/workload/workload_test.go b/pkg/workload/workload_test.go
index 41a32545a..2bc1b63c6 100644
--- a/pkg/workload/workload_test.go
+++ b/pkg/workload/workload_test.go
@@ -889,3 +889,199 @@ func TestAdmissionCheckStrategy(t *testing.T) {
 		})
 	}
 }
+
+func TestPropagateResourceRequests(t *testing.T) {
+	cases := map[string]struct {
+		wl   *kueue.Workload
+		info *Info
+		want bool
+	}{
+		"one podset, no diff": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Mi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{{
+					Name: "ps1",
+					Requests: resources.Requests{
+						corev1.ResourceCPU:    10000,
+						corev1.ResourceMemory: 10 * 1024 * 1024,
+						"nvidia.com/gpu":      1,
+					},
+				}},
+			},
+			want: false,
+		},
+		"one podset, memory missing diff": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Mi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{{
+					Name: "ps1",
+					Requests: resources.Requests{
+						corev1.ResourceCPU: 5000,
+						"nvidia.com/gpu":   1,
+					},
+				}},
+			},
+			want: true,
+		},
+		"one podset, cpu diff": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Mi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{{
+					Name: "ps1",
+					Requests: resources.Requests{
+						corev1.ResourceCPU:    5000,
+						corev1.ResourceMemory: 10 * 1024 * 1024,
+						"nvidia.com/gpu":      1,
+					},
+				}},
+			},
+			want: true,
+		},
+		"one podset, memory diff": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Gi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{{
+					Name: "ps1",
+					Requests: resources.Requests{
+						corev1.ResourceCPU:    10000,
+						corev1.ResourceMemory: 10 * 1024 * 1024,
+						"nvidia.com/gpu":      1,
+					},
+				}},
+			},
+			want: true,
+		},
+		"one podset, gpu (extended resource) diff": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Mi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{{
+					Name: "ps1",
+					Requests: resources.Requests{
+						corev1.ResourceCPU:    10000,
+						corev1.ResourceMemory: 10 * 1024 * 1024,
+						"nvidia.com/gpu":      2,
+					},
+				}},
+			},
+			want: true,
+		},
+		"two podset, no diff ": {
+			wl: &kueue.Workload{
+				Status: kueue.WorkloadStatus{
+					ResourceRequests: []kueue.PodSetRequest{
+						{
+							Name: "ps1",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("10"),
+								corev1.ResourceMemory: resource.MustParse("10Mi"),
+								"nvidia.com/gpu":      resource.MustParse("1"),
+							},
+						},
+						{
+							Name: "ps2",
+							Resources: corev1.ResourceList{
+								corev1.ResourceCPU:    resource.MustParse("20"),
+								corev1.ResourceMemory: resource.MustParse("20Mi"),
+								"nvidia.com/gpu":      resource.MustParse("2"),
+							},
+						},
+					},
+				},
+			},
+			info: &Info{
+				TotalRequests: []PodSetResources{
+					{
+						Name: "ps1",
+						Requests: resources.Requests{
+							corev1.ResourceCPU:    10000,
+							corev1.ResourceMemory: 10 * 1024 * 1024,
+							"nvidia.com/gpu":      1,
+						},
+					},
+					{
+						Name: "ps2",
+						Requests: resources.Requests{
+							corev1.ResourceCPU:    20000,
+							corev1.ResourceMemory: 20 * 1024 * 1024,
+							"nvidia.com/gpu":      2,
+						},
+					},
+				},
+			},
+			want: false,
+		},
+	}
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			got := PropagateResourceRequests(tc.wl, tc.info)
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("Unexpected PropagateResourceRequests() result (-want,+got):\n%s", diff)
+			}
+		})
+	}
+}
diff --git a/test/e2e/config/certmanager/cert_metrics_manager_patch.yaml b/test/e2e/config/certmanager/cert_metrics_manager_patch.yaml
index 16cfdd7e6..7859f8a56 100644
--- a/test/e2e/config/certmanager/cert_metrics_manager_patch.yaml
+++ b/test/e2e/config/certmanager/cert_metrics_manager_patch.yaml
@@ -11,6 +11,8 @@
       mountPath: /controller_manager_config.yaml
       subPath: controller_manager_config.yaml
       readOnly: true
+    - name: visibility
+      mountPath: /visibility
 
 - op: replace
   path: /spec/template/spec/volumes
@@ -29,6 +31,8 @@
         items:
           - key: controller_manager_config.yaml
             path: controller_manager_config.yaml
+    - name: visibility
+      emptyDir: {}
 
 - op: replace
   path: /spec/template/spec/containers/0/imagePullPolicy