Repository: opencontainers/runc
Pull request #3234 (2021-10-06 00:29:29)

libct/cg: refactor/improve/rename GetHugePageSize -> HugePageSizes
~~_(this is based on and currently includes PR #3233)_~~

1. Since GetHugePageSize do not have any external users (checked by
       sourcegraph), and no internal user ever uses its second return value
       (the error), let's drop it.
    
2. Rename GetHugePageSize -> HugePageSizes (drop the Get prefix as per
       Go guidelines, add suffix since we return many sizes).

3.  Implement lazy init for `HugePageSizes`
    
    I have noticed that libct/cg/fs allocates 8K during init on every runc
    execution:
    
    > init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.028 ms clock, **8512 bytes, 13 allocs**
    
    Apparently this is caused by the global `HugePageSizes` variable initialization,
    which is only used from `GetStat`s (i.e. it is never used by the runc binary itself).
    
    Remove it, and use `HugePageSizes()` directly instead. Make it init-once,
    so that `GetStats` (which, I guess, is periodically called by kubernetes)
    does not re-read huge page sizes over and over.
    
    This also removes 12 allocs and 8K from libct/cg/fs init section:
    
    > $ time GODEBUG=inittrace=1 ./runc --help 2>&1 | grep cgroups/fs
    > init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.003 ms clock, **16 bytes, 1 allocs**

4. libct/cg: TestGetHugePageSizeImpl: use t.Run
    
    Move test case comments to doc strings, and use t.Run.


kolyshkin: This is definitely post-1.1 material
kolyshkin: Rebased on top of current git tip to repro https://github.com/opencontainers/runc/issues/3266

Patch:
diff --git a/libcontainer/cgroups/fs/fs.go b/libcontainer/cgroups/fs/fs.go
index b574d3a0..fb4fcc7f 100644
--- a/libcontainer/cgroups/fs/fs.go
+++ b/libcontainer/cgroups/fs/fs.go
@@ -13,25 +13,22 @@ import (
 	"github.com/opencontainers/runc/libcontainer/configs"
 )
 
-var (
-	subsystems = []subsystem{
-		&CpusetGroup{},
-		&DevicesGroup{},
-		&MemoryGroup{},
-		&CpuGroup{},
-		&CpuacctGroup{},
-		&PidsGroup{},
-		&BlkioGroup{},
-		&HugetlbGroup{},
-		&NetClsGroup{},
-		&NetPrioGroup{},
-		&PerfEventGroup{},
-		&FreezerGroup{},
-		&RdmaGroup{},
-		&NameGroup{GroupName: "name=systemd", Join: true},
-	}
-	HugePageSizes, _ = cgroups.GetHugePageSize()
-)
+var subsystems = []subsystem{
+	&CpusetGroup{},
+	&DevicesGroup{},
+	&MemoryGroup{},
+	&CpuGroup{},
+	&CpuacctGroup{},
+	&PidsGroup{},
+	&BlkioGroup{},
+	&HugetlbGroup{},
+	&NetClsGroup{},
+	&NetPrioGroup{},
+	&PerfEventGroup{},
+	&FreezerGroup{},
+	&RdmaGroup{},
+	&NameGroup{GroupName: "name=systemd", Join: true},
+}
 
 var errSubsystemDoesNotExist = errors.New("cgroup: subsystem does not exist")
 
diff --git a/libcontainer/cgroups/fs/hugetlb.go b/libcontainer/cgroups/fs/hugetlb.go
index 86650128..8ddd6fdd 100644
--- a/libcontainer/cgroups/fs/hugetlb.go
+++ b/libcontainer/cgroups/fs/hugetlb.go
@@ -29,11 +29,11 @@ func (s *HugetlbGroup) Set(path string, r *configs.Resources) error {
 }
 
 func (s *HugetlbGroup) GetStats(path string, stats *cgroups.Stats) error {
-	hugetlbStats := cgroups.HugetlbStats{}
 	if !cgroups.PathExists(path) {
 		return nil
 	}
-	for _, pageSize := range HugePageSizes {
+	hugetlbStats := cgroups.HugetlbStats{}
+	for _, pageSize := range cgroups.HugePageSizes() {
 		usage := "hugetlb." + pageSize + ".usage_in_bytes"
 		value, err := fscommon.GetCgroupParamUint(path, usage)
 		if err != nil {
diff --git a/libcontainer/cgroups/fs2/hugetlb.go b/libcontainer/cgroups/fs2/hugetlb.go
index a228d3f2..c92a7e64 100644
--- a/libcontainer/cgroups/fs2/hugetlb.go
+++ b/libcontainer/cgroups/fs2/hugetlb.go
@@ -26,10 +26,8 @@ func setHugeTlb(dirPath string, r *configs.Resources) error {
 }
 
 func statHugeTlb(dirPath string, stats *cgroups.Stats) error {
-	hugePageSizes, _ := cgroups.GetHugePageSize()
 	hugetlbStats := cgroups.HugetlbStats{}
-
-	for _, pagesize := range hugePageSizes {
+	for _, pagesize := range cgroups.HugePageSizes() {
 		value, err := fscommon.GetCgroupParamUint(dirPath, "hugetlb."+pagesize+".current")
 		if err != nil {
 			return err
diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go
index 6c96882c..13ebf52a 100644
--- a/libcontainer/cgroups/utils.go
+++ b/libcontainer/cgroups/utils.go
@@ -302,40 +302,61 @@ func RemovePaths(paths map[string]string) (err error) {
 	return fmt.Errorf("Failed to remove paths: %v", paths)
 }
 
-func GetHugePageSize() ([]string, error) {
-	dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0)
-	if err != nil {
-		return nil, err
-	}
-	files, err := dir.Readdirnames(0)
-	dir.Close()
-	if err != nil {
-		return nil, err
-	}
+var (
+	hugePageSizes []string
+	initHPSOnce   sync.Once
+)
 
-	return getHugePageSizeFromFilenames(files)
+func HugePageSizes() []string {
+	initHPSOnce.Do(func() {
+		dir, err := os.OpenFile("/sys/kernel/mm/hugepages", unix.O_DIRECTORY|unix.O_RDONLY, 0)
+		if err != nil {
+			return
+		}
+		files, err := dir.Readdirnames(0)
+		dir.Close()
+		if err != nil {
+			return
+		}
+
+		hugePageSizes, err = getHugePageSizeFromFilenames(files)
+		if err != nil {
+			logrus.Warn("HugePageSizes: ", err)
+		}
+	})
+
+	return hugePageSizes
 }
 
 func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
 	pageSizes := make([]string, 0, len(fileNames))
+	var warn error
 
 	for _, file := range fileNames {
 		// example: hugepages-1048576kB
 		val := strings.TrimPrefix(file, "hugepages-")
 		if len(val) == len(file) {
-			// unexpected file name: no prefix found
+			// Unexpected file name: no prefix found, ignore it.
 			continue
 		}
-		// The suffix is always "kB" (as of Linux 5.9)
+		// The suffix is always "kB" (as of Linux 5.13). If we find
+		// something else, produce an error but keep going.
 		eLen := len(val) - 2
 		val = strings.TrimSuffix(val, "kB")
 		if len(val) != eLen {
-			logrus.Warnf("GetHugePageSize: %s: invalid filename suffix (expected \"kB\")", file)
+			// Highly unlikely.
+			if warn == nil {
+				warn = errors.New(file + `: invalid suffix (expected "kB")`)
+			}
 			continue
 		}
 		size, err := strconv.Atoi(val)
 		if err != nil {
-			return nil, err
+			// Highly unlikely.
+			if warn == nil {
+				warn = fmt.Errorf("%s: %w", file, err)
+			}
+			continue
 		}
 		// Model after https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/hugetlb_cgroup.c?id=eff48ddeab782e35e58ccc8853f7386bbae9dec4#n574
 		// but in our case the size is in KB already.
@@ -349,7 +370,7 @@ func getHugePageSizeFromFilenames(fileNames []string) ([]string, error) {
 		pageSizes = append(pageSizes, val)
 	}
 
-	return pageSizes, nil
+	return pageSizes, warn
 }
 
 // GetPids returns all pids, that were added to cgroup at path.
diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go
index 4c56582f..1632cce0 100644
--- a/libcontainer/container_linux.go
+++ b/libcontainer/container_linux.go
@@ -2195,9 +2195,6 @@ func (c *linuxContainer) bootstrapData(cloneFlags uintptr, nsMaps map[configs.Na
 		var mounts []byte
 		for _, m := range c.config.Mounts {
 			if m.IsBind() {
-				if strings.IndexByte(m.Source, 0) >= 0 {
-					return nil, fmt.Errorf("mount source string contains null byte: %q", m.Source)
-				}
 				mounts = append(mounts, []byte(m.Source)...)
 			}
 			mounts = append(mounts, byte(0))
diff --git a/libcontainer/specconv/spec_linux.go b/libcontainer/specconv/spec_linux.go
index cf54cf3d..9e39a70b 100644
--- a/libcontainer/specconv/spec_linux.go
+++ b/libcontainer/specconv/spec_linux.go
@@ -407,18 +407,6 @@ func createLibcontainerMount(cwd string, m specs.Mount) (*configs.Mount, error)
 			mnt.Source = filepath.Join(cwd, m.Source)
 		}
 	}
-
-	// None of the mount arguments can contain a null byte. Normally such
-	// strings would either cause some other failure or would just be truncated
-	// when we hit the null byte, but because we serialise these strings as
-	// netlink messages (which don't have special null-byte handling) we need
-	// to block this as early as possible.
-	if strings.IndexByte(mnt.Source, 0) >= 0 ||
-		strings.IndexByte(mnt.Destination, 0) >= 0 ||
-		strings.IndexByte(mnt.Device, 0) >= 0 {
-		return nil, errors.New("mount field contains null byte")
-	}
-
 	return mnt, nil
 }
 


Test patch:
diff --git a/libcontainer/cgroups/fs/hugetlb_test.go b/libcontainer/cgroups/fs/hugetlb_test.go
index ba54f35a..f4aea7eb 100644
--- a/libcontainer/cgroups/fs/hugetlb_test.go
+++ b/libcontainer/cgroups/fs/hugetlb_test.go
@@ -31,14 +31,14 @@ func TestHugetlbSetHugetlb(t *testing.T) {
 		hugetlbAfter  = 512
 	)
 
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		writeFileContents(t, path, map[string]string{
 			fmt.Sprintf(limit, pageSize): strconv.Itoa(hugetlbBefore),
 		})
 	}
 
 	r := &configs.Resources{}
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		r.HugetlbLimit = []*configs.HugepageLimit{
 			{
 				Pagesize: pageSize,
@@ -51,7 +51,7 @@ func TestHugetlbSetHugetlb(t *testing.T) {
 		}
 	}
 
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		limit := fmt.Sprintf(limit, pageSize)
 		value, err := fscommon.GetCgroupParamUint(path, limit)
 		if err != nil {
@@ -65,7 +65,7 @@ func TestHugetlbSetHugetlb(t *testing.T) {
 
 func TestHugetlbStats(t *testing.T) {
 	path := tempDir(t, "hugetlb")
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		writeFileContents(t, path, map[string]string{
 			fmt.Sprintf(usage, pageSize):    hugetlbUsageContents,
 			fmt.Sprintf(maxUsage, pageSize): hugetlbMaxUsageContents,
@@ -80,7 +80,7 @@ func TestHugetlbStats(t *testing.T) {
 		t.Fatal(err)
 	}
 	expectedStats := cgroups.HugetlbStats{Usage: 128, MaxUsage: 256, Failcnt: 100}
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		expectHugetlbStatEquals(t, expectedStats, actualStats.HugetlbStats[pageSize])
 	}
 }
@@ -101,7 +101,7 @@ func TestHugetlbStatsNoUsageFile(t *testing.T) {
 
 func TestHugetlbStatsNoMaxUsageFile(t *testing.T) {
 	path := tempDir(t, "hugetlb")
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		writeFileContents(t, path, map[string]string{
 			fmt.Sprintf(usage, pageSize): hugetlbUsageContents,
 		})
@@ -117,7 +117,7 @@ func TestHugetlbStatsNoMaxUsageFile(t *testing.T) {
 
 func TestHugetlbStatsBadUsageFile(t *testing.T) {
 	path := tempDir(t, "hugetlb")
-	for _, pageSize := range HugePageSizes {
+	for _, pageSize := range cgroups.HugePageSizes() {
 		writeFileContents(t, path, map[string]string{
 			fmt.Sprintf(usage, pageSize): "bad",
 			maxUsage:                     hugetlbMaxUsageContents,
diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go
index 929cf511..c9feb844 100644
--- a/libcontainer/cgroups/utils_test.go
+++ b/libcontainer/cgroups/utils_test.go
@@ -8,7 +8,6 @@ import (
 	"testing"
 
 	"github.com/moby/sys/mountinfo"
-	"github.com/sirupsen/logrus"
 )
 
 const fedoraMountinfo = `15 35 0:3 / /proc rw,nosuid,nodev,noexec,relatime shared:5 - proc proc rw
@@ -448,19 +447,6 @@ func TestFindCgroupMountpointAndRoot(t *testing.T) {
 	}
 }
 
-func BenchmarkGetHugePageSize(b *testing.B) {
-	var (
-		output []string
-		err    error
-	)
-	for i := 0; i < b.N; i++ {
-		output, err = GetHugePageSize()
-	}
-	if err != nil || len(output) == 0 {
-		b.Fatal("unexpected results")
-	}
-}
-
 func BenchmarkGetHugePageSizeImpl(b *testing.B) {
 	var (
 		input  = []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"}
@@ -477,71 +463,78 @@ func BenchmarkGetHugePageSizeImpl(b *testing.B) {
 
 func TestGetHugePageSizeImpl(t *testing.T) {
 	testCases := []struct {
+		doc    string
 		input  []string
 		output []string
 		isErr  bool
-		isWarn bool
 	}{
 		{
+			doc:    "normal input",
 			input:  []string{"hugepages-1048576kB", "hugepages-2048kB", "hugepages-32768kB", "hugepages-64kB"},
 			output: []string{"1GB", "2MB", "32MB", "64KB"},
 		},
 		{
+			doc:    "empty input",
 			input:  []string{},
 			output: []string{},
 		},
-		{ // not a number
+		{
+			doc:   "not a number",
 			input: []string{"hugepages-akB"},
 			isErr: true,
 		},
-		{ // no prefix (silently skipped)
+		{
+			doc:   "no prefix (silently skipped)",
 			input: []string{"1024kB"},
 		},
-		{ // invalid prefix (silently skipped)
+		{
+			doc:   "invalid prefix (silently skipped)",
 			input: []string{"whatever-1024kB"},
 		},
-		{ // invalid suffix (skipped with a warning)
-			input:  []string{"hugepages-1024gB"},
-			isWarn: true,
+		{
+			doc:   "invalid suffix",
+			input: []string{"hugepages-1024gB"},
+			isErr: true,
 		},
-		{ // no suffix (skipped with a warning)
-			input:  []string{"hugepages-1024"},
-			isWarn: true,
+		{
+			doc:   "no suffix",
+			input: []string{"hugepages-1024"},
+			isErr: true,
+		},
+		{
+			doc:    "mixed valid and invalid entries",
+			input:  []string{"hugepages-4194304kB", "hugepages-2048kB", "hugepages-akB", "hugepages-64kB"},
+			output: []string{"4GB", "2MB", "64KB"},
+			isErr:  true,
+		},
+		{
+			doc:    "more mixed valid and invalid entries",
+			input:  []string{"hugepages-2048kB", "hugepages-kB", "hugepages-64kB"},
+			output: []string{"2MB", "64KB"},
+			isErr:  true,
 		},
 	}
 
-	// Need to catch warnings.
-	savedOut := logrus.StandardLogger().Out
-	defer logrus.SetOutput(savedOut)
-	warns := new(bytes.Buffer)
-	logrus.SetOutput(warns)
-
 	for _, c := range testCases {
-		warns.Reset()
-		output, err := getHugePageSizeFromFilenames(c.input)
-		if err != nil {
-			if !c.isErr {
-				t.Errorf("input %v, expected nil, got error: %v", c.input, err)
+		c := c
+		t.Run(c.doc, func(t *testing.T) {
+			output, err := getHugePageSizeFromFilenames(c.input)
+			t.Log("input:", c.input, "; output:", output, "; err:", err)
+			if err != nil {
+				if !c.isErr {
+					t.Errorf("input %v, expected nil, got error: %v", c.input, err)
+				}
+				// no more checks
+				return
 			}
-			// no more checks
-			continue
-		}
-		if c.isErr {
-			t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output)
-			// no more checks
-			continue
-		}
-		// check for warnings
-		if c.isWarn && warns.Len() == 0 {
-			t.Errorf("input %v, expected a warning, got none", c.input)
-		}
-		if !c.isWarn && warns.Len() > 0 {
-			t.Errorf("input %v, unexpected warning: %s", c.input, warns.String())
-		}
-		// check output
-		if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) {
-			t.Errorf("input %v, expected %v, got %v", c.input, c.output, output)
-		}
+			if c.isErr {
+				t.Errorf("input %v, expected error, got error: nil, output: %v", c.input, output)
+			}
+			// check output
+			if len(output) != len(c.output) || (len(output) > 0 && !reflect.DeepEqual(output, c.output)) {
+				t.Errorf("input %v, expected %v, got %v", c.input, c.output, output)
+			}
+		})
 	}
 }