From f160352682f4518c8356ba394e5f147822a1eb74 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 13 May 2020 16:18:46 -0700 Subject: [PATCH 1/2] libct/cgroup: prep to rm GetClosestMountpointAncestor This function is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Prepare to remove it by replacing with the implementation based on the parser from github.com/moby/sys/mountinfo parser. This commit is here to make sure the proposed replacement passes the unit test. Funny, but the unit test need to be slightly modified since it supplies the wrong mountinfo (space as the first character, empty line at the end). Validated by $ go test -v -run Ance === RUN TestGetClosestMountpointAncestor --- PASS: TestGetClosestMountpointAncestor (0.00s) PASS ok github.com/opencontainers/runc/libcontainer/cgroups 0.002s Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/cpuset.go | 6 +++++- libcontainer/cgroups/utils.go | 29 ++++++++++++++++++----------- libcontainer/cgroups/utils_test.go | 11 +++++++---- 3 files changed, 30 insertions(+), 16 deletions(-) diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index bfc900e3f6c..a0c3c5f68bc 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -62,7 +62,11 @@ func (s *CpusetGroup) ApplyDir(dir string, cgroup *configs.Cgroup, pid int) erro if err != nil { return err } - root := filepath.Dir(cgroups.GetClosestMountpointAncestor(dir, string(mountInfo))) + root, err := cgroups.GetClosestMountpointAncestor(dir, string(mountInfo)) + if err != nil { + return err + } + root = filepath.Dir(root) // 'ensureParent' start with parent because we don't want to // explicitly inherit from parent, it could conflict with // 'cpuset.cpu_exclusive'. diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 8cdd6b7eda6..9859557a724 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -16,6 +16,7 @@ import ( "time" units "github.com/docker/go-units" + "github.com/moby/sys/mountinfo" "golang.org/x/sys/unix" ) @@ -126,19 +127,25 @@ func isSubsystemAvailable(subsystem string) bool { return avail } -func GetClosestMountpointAncestor(dir, mountinfo string) string { - deepestMountPoint := "" - for _, mountInfoEntry := range strings.Split(mountinfo, "\n") { - mountInfoParts := strings.Fields(mountInfoEntry) - if len(mountInfoParts) < 5 { - continue - } - mountPoint := mountInfoParts[4] - if strings.HasPrefix(mountPoint, deepestMountPoint) && strings.HasPrefix(dir, mountPoint) { - deepestMountPoint = mountPoint +func GetClosestMountpointAncestor(dir, data string) (string, error) { + mi, err := mountinfo.GetMountsFromReader(strings.NewReader(data), mountinfo.ParentsFilter(dir)) + if err != nil { + return "", err + } + if len(mi) < 1 { + return "", err + } + + // find the longest mount point + var idx, maxlen int + for i := range mi { + if len(mi[i].Mountpoint) > maxlen { + maxlen = len(mi[i].Mountpoint) + idx = i } } - return deepestMountPoint + + return mi[idx].Mountpoint, nil } func FindCgroupMountpointDir() (string, error) { diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index adf266858bf..a3be724435d 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -374,7 +374,7 @@ func TestIgnoreCgroup2Mount(t *testing.T) { } func TestGetClosestMountpointAncestor(t *testing.T) { - fakeMountInfo := ` 18 24 0:17 / /sys rw,nosuid,nodev,noexec,relatime - sysfs sysfs rw + const fakeMountInfo = `18 24 0:17 / /sys rw,nosuid,nodev,noexec,relatime - sysfs sysfs rw 100 99 1:31 / /foo/bar rw,relatime - fake fake rw,fake 100 99 1:31 / /foo/bar/baz2 rw,relatime - fake fake rw,fake 100 99 1:31 / /foo/bar/baz rw,relatime - fake fake rw,fake @@ -382,8 +382,8 @@ func TestGetClosestMountpointAncestor(t *testing.T) { 100 99 1:31 / /foo/bar/baz3 rw,relatime - fake fake rw,fake 100 99 1:31 / /foo rw,relatime - fake fake rw,fake 100 99 1:31 / /unrelated rw,relatime - fake fake rw,fake -100 99 1:31 / / rw,relatime - fake fake rw,fake -` +100 99 1:31 / / rw,relatime - fake fake rw,fake` + testCases := []struct { input string output string @@ -395,7 +395,10 @@ func TestGetClosestMountpointAncestor(t *testing.T) { } for _, c := range testCases { - mountpoint := GetClosestMountpointAncestor(c.input, fakeMountInfo) + mountpoint, err := GetClosestMountpointAncestor(c.input, fakeMountInfo) + if err != nil { + t.Fatal(err) + } if mountpoint != c.output { t.Errorf("expected %s, got %s", c.output, mountpoint) } From 2db3240f35b0f082d5129b52a0622b303cb4ccd1 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Wed, 13 May 2020 17:32:06 -0700 Subject: [PATCH 2/2] libct/cgroups: rm GetClosestMountpointAncestor The function GetClosestMountpointAncestor is not very efficient, does not really belong to cgroup package, and is only used once (from fs/cpuset.go). Remove it, replacing with the implementation based on moby/sys/mountinfo parser. Signed-off-by: Kir Kolyshkin --- libcontainer/cgroups/fs/cpuset.go | 29 ++++++++++++++++++++++----- libcontainer/cgroups/utils.go | 22 -------------------- libcontainer/cgroups/utils_test.go | 32 ------------------------------ 3 files changed, 24 insertions(+), 59 deletions(-) diff --git a/libcontainer/cgroups/fs/cpuset.go b/libcontainer/cgroups/fs/cpuset.go index a0c3c5f68bc..6081abd1a16 100644 --- a/libcontainer/cgroups/fs/cpuset.go +++ b/libcontainer/cgroups/fs/cpuset.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" + "github.com/moby/sys/mountinfo" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/fscommon" "github.com/opencontainers/runc/libcontainer/configs" @@ -52,17 +53,35 @@ func (s *CpusetGroup) GetStats(path string, stats *cgroups.Stats) error { return nil } +// Get the source mount point of directory passed in as argument. +func getMount(dir string) (string, error) { + mi, err := mountinfo.GetMounts(mountinfo.ParentsFilter(dir)) + if err != nil { + return "", err + } + if len(mi) < 1 { + return "", fmt.Errorf("Can't find mount point of %s", dir) + } + + // find the longest mount point + var idx, maxlen int + for i := range mi { + if len(mi[i].Mountpoint) > maxlen { + maxlen = len(mi[i].Mountpoint) + idx = i + } + } + + return mi[idx].Mountpoint, nil +} + func (s *CpusetGroup) ApplyDir(dir string, cgroup *configs.Cgroup, pid int) error { // This might happen if we have no cpuset cgroup mounted. // Just do nothing and don't fail. if dir == "" { return nil } - mountInfo, err := ioutil.ReadFile("/proc/self/mountinfo") - if err != nil { - return err - } - root, err := cgroups.GetClosestMountpointAncestor(dir, string(mountInfo)) + root, err := getMount(dir) if err != nil { return err } diff --git a/libcontainer/cgroups/utils.go b/libcontainer/cgroups/utils.go index 9859557a724..3b6ef4af998 100644 --- a/libcontainer/cgroups/utils.go +++ b/libcontainer/cgroups/utils.go @@ -16,7 +16,6 @@ import ( "time" units "github.com/docker/go-units" - "github.com/moby/sys/mountinfo" "golang.org/x/sys/unix" ) @@ -127,27 +126,6 @@ func isSubsystemAvailable(subsystem string) bool { return avail } -func GetClosestMountpointAncestor(dir, data string) (string, error) { - mi, err := mountinfo.GetMountsFromReader(strings.NewReader(data), mountinfo.ParentsFilter(dir)) - if err != nil { - return "", err - } - if len(mi) < 1 { - return "", err - } - - // find the longest mount point - var idx, maxlen int - for i := range mi { - if len(mi[i].Mountpoint) > maxlen { - maxlen = len(mi[i].Mountpoint) - idx = i - } - } - - return mi[idx].Mountpoint, nil -} - func FindCgroupMountpointDir() (string, error) { f, err := os.Open("/proc/self/mountinfo") if err != nil { diff --git a/libcontainer/cgroups/utils_test.go b/libcontainer/cgroups/utils_test.go index a3be724435d..f4d6b4e7eb3 100644 --- a/libcontainer/cgroups/utils_test.go +++ b/libcontainer/cgroups/utils_test.go @@ -373,38 +373,6 @@ func TestIgnoreCgroup2Mount(t *testing.T) { } } -func TestGetClosestMountpointAncestor(t *testing.T) { - const fakeMountInfo = `18 24 0:17 / /sys rw,nosuid,nodev,noexec,relatime - sysfs sysfs rw -100 99 1:31 / /foo/bar rw,relatime - fake fake rw,fake -100 99 1:31 / /foo/bar/baz2 rw,relatime - fake fake rw,fake -100 99 1:31 / /foo/bar/baz rw,relatime - fake fake rw,fake -100 99 1:31 / /foo/bar/bazza rw,relatime - fake fake rw,fake -100 99 1:31 / /foo/bar/baz3 rw,relatime - fake fake rw,fake -100 99 1:31 / /foo rw,relatime - fake fake rw,fake -100 99 1:31 / /unrelated rw,relatime - fake fake rw,fake -100 99 1:31 / / rw,relatime - fake fake rw,fake` - - testCases := []struct { - input string - output string - }{ - {input: "/foo/bar/baz/a/b/c", output: "/foo/bar/baz"}, - {input: "/foo/bar/baz", output: "/foo/bar/baz"}, - {input: "/foo/bar/bazza", output: "/foo/bar/bazza"}, - {input: "/a/b/c/d", output: "/"}, - } - - for _, c := range testCases { - mountpoint, err := GetClosestMountpointAncestor(c.input, fakeMountInfo) - if err != nil { - t.Fatal(err) - } - if mountpoint != c.output { - t.Errorf("expected %s, got %s", c.output, mountpoint) - } - } -} - func TestFindCgroupMountpointAndRoot(t *testing.T) { fakeMountInfo := ` 35 27 0:29 / /foo rw,nosuid,nodev,noexec,relatime shared:18 - cgroup cgroup rw,devices