From 08434dbf32cd5c0856a466fb398e8e521f4e3f28 Mon Sep 17 00:00:00 2001 From: Martin Gencur Date: Wed, 13 Sep 2023 12:55:33 +0200 Subject: [PATCH] Fix CGroupsV2 test (#14355) (#449) * Fix CGroupsV2 test * Fix comment --- test/conformance/runtime/cgroup_test.go | 65 ++++++++++++++++----- test/test_images/runtime/handlers/cgroup.go | 14 +---- test/types/runtime.go | 4 +- 3 files changed, 53 insertions(+), 30 deletions(-) diff --git a/test/conformance/runtime/cgroup_test.go b/test/conformance/runtime/cgroup_test.go index 30842a0010d5..12641a19b0b1 100644 --- a/test/conformance/runtime/cgroup_test.go +++ b/test/conformance/runtime/cgroup_test.go @@ -22,6 +22,7 @@ package runtime import ( "fmt" "strconv" + "strings" "testing" corev1 "k8s.io/api/core/v1" @@ -70,14 +71,16 @@ func TestMustHaveCgroupConfigured(t *testing.T) { // It's important to make sure that the memory limit is divisible by common page // size (4k, 8k, 16k, 64k) as some environments apply rounding to the closest page // size multiple, see https://github.com/kubernetes/kubernetes/issues/82230. - var expectedCgroupsV1 = map[string]int{ - "/sys/fs/cgroup/memory/memory.limit_in_bytes": int(resources.Limits.Memory().Value()) &^ 4095, // floor() to 4K pages - "/sys/fs/cgroup/cpu/cpu.shares": int(resources.Requests.Cpu().MilliValue()) * 1024 / 1000} - - var expectedCgroupsV2 = map[string]int{ - "/sys/fs/cgroup/memory.max": int(resources.Limits.Memory().Value()) &^ 4095, // floor() to 4K pages - "/sys/fs/cgroup/cpu.weight": int(resources.Requests.Cpu().MilliValue()) * 1024 / 1000, - "/sys/fs/cgroup/cpu.max": int(resources.Limits.Cpu().MilliValue())} + var expectedCgroupsV1 = map[string]string{ + "/sys/fs/cgroup/memory/memory.limit_in_bytes": fmt.Sprintf("%d", resources.Limits.Memory().Value()&^4095), // floor() to 4K pages + "/sys/fs/cgroup/cpu/cpu.shares": fmt.Sprintf("%d", resources.Requests.Cpu().MilliValue()*1024/1000)} + + var expectedCgroupsV2 = map[string]string{ + "/sys/fs/cgroup/memory.max": fmt.Sprintf("%d", resources.Limits.Memory().Value()&^4095), // floor() to 4K pages + // Convert cgroup v1 cpu.shares value to cgroup v2 cpu.weight + // https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2254-cgroup-v2#phase-1-convert-from-cgroups-v1-settings-to-v2 + "/sys/fs/cgroup/cpu.weight": fmt.Sprintf("%d", ((((resources.Requests.Cpu().MilliValue()*1024/1000)-2)*9999)/262142)+1), + } cgroups := ri.Host.Cgroups cgroupV2, err := isCgroupsV2(ri.Host.Mounts) @@ -91,9 +94,10 @@ func TestMustHaveCgroupConfigured(t *testing.T) { expectedCgroups = expectedCgroupsV2 } - // These are used to check the ratio of 'period' to 'quota'. It needs to - // be equal to the 'cpuLimit (limit = period / quota) - var period, quota *int + // These are used to check the ratio of 'quota' to 'period'. It needs to + // be equal to the 'cpuLimit (limit = quota / period) + var period, quota *string + var maxV2, periodV2 string for _, cgroup := range cgroups { if cgroup.Error != "" { @@ -101,7 +105,7 @@ func TestMustHaveCgroupConfigured(t *testing.T) { continue } - // These two are special - just save their values and then continue + // These are special - just save their values and then continue if cgroup.Name == "/sys/fs/cgroup/cpu/cpu.cfs_period_us" { period = cgroup.Value continue @@ -110,6 +114,11 @@ func TestMustHaveCgroupConfigured(t *testing.T) { quota = cgroup.Value continue } + if cgroup.Name == "/sys/fs/cgroup/cpu.max" { + // The format is like 'max 100000'. + maxV2 = strings.Split(*cgroup.Value, " ")[0] + periodV2 = strings.Split(*cgroup.Value, " ")[1] + } if _, ok := expectedCgroups[cgroup.Name]; !ok { // Service returned a value we don't test @@ -117,19 +126,43 @@ func TestMustHaveCgroupConfigured(t *testing.T) { continue } if got, want := *cgroup.Value, expectedCgroups[cgroup.Name]; got != want { - t.Errorf("%s = %d, want: %d", cgroup.Name, *cgroup.Value, expectedCgroups[cgroup.Name]) + t.Errorf("%s = %s, want: %s", cgroup.Name, *cgroup.Value, expectedCgroups[cgroup.Name]) } } - if !cgroupV2 { - expectedCPULimit := int(resources.Limits.Cpu().MilliValue()) + expectedCPULimit := int(resources.Limits.Cpu().MilliValue()) + if cgroupV2 { + if maxV2 == "max" { + t.Errorf("The cpu is unlimited but should be %d MilliCPUs", expectedCPULimit) + } + m, err := strconv.Atoi(maxV2) + if err != nil { + t.Error(err) + } + p, err := strconv.Atoi(periodV2) + if err != nil { + t.Error(err) + } + milliCPU := (1000 * m) / p + if milliCPU != expectedCPULimit { + t.Errorf("MilliCPU (%v) is wrong should be %v. Max: %v Period: %v", milliCPU, expectedCPULimit, m, p) + } + } else { if period == nil { t.Error("Can't find the 'cpu.cfs_period_us' from cgroups") } else if quota == nil { t.Error("Can't find the 'cpu.cfs_quota_us' from cgroups") } else { + q, err := strconv.Atoi(*quota) + if err != nil { + t.Error(err) + } + p, err := strconv.Atoi(*period) + if err != nil { + t.Error(err) + } // CustomCpuLimits of a core e.g. 125m means 12,5% of a single CPU, 2 or 2000m means 200% of a single CPU - milliCPU := (1000 * (*quota)) / (*period) + milliCPU := (1000 * q) / p if milliCPU != expectedCPULimit { t.Errorf("MilliCPU (%v) is wrong should be %v. Period: %v Quota: %v", milliCPU, expectedCPULimit, period, quota) diff --git a/test/test_images/runtime/handlers/cgroup.go b/test/test_images/runtime/handlers/cgroup.go index 10f4cbae330b..a360b9e8c977 100644 --- a/test/test_images/runtime/handlers/cgroup.go +++ b/test/test_images/runtime/handlers/cgroup.go @@ -19,7 +19,6 @@ package handlers import ( "log" "os" - "strconv" "strings" "knative.dev/serving/test/types" @@ -69,15 +68,6 @@ func cgroups(paths ...string) []*types.Cgroup { } cs := strings.Trim(string(bc), "\n") - if path == "/sys/fs/cgroup/cpu.max" { - // The format is like 'max 100000' so trim the front "max". - cs = strings.Split(cs, " ")[1] - } - ic, err := strconv.Atoi(cs) - if err != nil { - cgroups = append(cgroups, &types.Cgroup{Name: path, Error: err.Error()}) - continue - } // Try to write to the Cgroup. We expect this to fail as a cheap // method for read-only validation @@ -85,9 +75,9 @@ func cgroups(paths ...string) []*types.Cgroup { // #nosec G306 err = os.WriteFile(path, newValue, 0644) if err != nil { - cgroups = append(cgroups, &types.Cgroup{Name: path, Value: &ic, ReadOnly: &yes}) + cgroups = append(cgroups, &types.Cgroup{Name: path, Value: &cs, ReadOnly: &yes}) } else { - cgroups = append(cgroups, &types.Cgroup{Name: path, Value: &ic, ReadOnly: &no}) + cgroups = append(cgroups, &types.Cgroup{Name: path, Value: &cs, ReadOnly: &no}) } } return cgroups diff --git a/test/types/runtime.go b/test/types/runtime.go index 58c69b09aadf..a2fad396cf6e 100644 --- a/test/types/runtime.go +++ b/test/types/runtime.go @@ -210,8 +210,8 @@ type FileAccessInfo struct { type Cgroup struct { // Name is the full path name of the cgroup. Name string `json:"name"` - // Value is the integer files in the cgroup file. - Value *int `json:"value,omitempty"` + // Value is the string value in the cgroup file. + Value *string `json:"value,omitempty"` // ReadOnly is true if the cgroup was not writable. ReadOnly *bool `json:"readOnly,omitempty"` // Error is the String representation of the error returned obtaining the information.