Fix CGroupsV2 test (knative#14355) (#449)

* Fix CGroupsV2 test

* Fix comment

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,17 +94,18 @@ 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 != "" {
 			t.Error("Error getting cgroup information:", cgroup.Error)
 			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,26 +114,55 @@ 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
 			t.Logf("%v cgroup returned, but not validated", cgroup.Name)
 			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)

test/test_images/runtime/handlers/cgroup.go

@@ -19,7 +19,6 @@ package handlers
 import (
 	"log"
 	"os"
-	"strconv"
 	"strings"
 
 	"knative.dev/serving/test/types"
@@ -69,25 +68,16 @@ 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
 		newValue := []byte{'9'}
 		// #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

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.