From 40870106ef8823280db7f45d52d7c6bb44dc35ae Mon Sep 17 00:00:00 2001 From: utam0k Date: Thu, 24 Feb 2022 03:53:17 +0000 Subject: [PATCH 1/7] ws-daemon: Add a simple unit test for cputlimit. --- components/ws-daemon/pkg/cpulimit/cfs_test.go | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 components/ws-daemon/pkg/cpulimit/cfs_test.go diff --git a/components/ws-daemon/pkg/cpulimit/cfs_test.go b/components/ws-daemon/pkg/cpulimit/cfs_test.go new file mode 100644 index 00000000000000..565fbe0f1b7ba5 --- /dev/null +++ b/components/ws-daemon/pkg/cpulimit/cfs_test.go @@ -0,0 +1,88 @@ +// Copyright (c) 2022 Gitpod GmbH. All rights reserved. +// Licensed under the GNU Affero General Public License (AGPL). +// See License-AGPL.txt in the project root for license information. + +package cpulimit + +import ( + "os" + "path/filepath" + "strconv" + "testing" + + "github.com/opencontainers/runc/libcontainer/cgroups" +) + +func init() { + cgroups.TestMode = true +} + +func createTempDir(t *testing.T, subsystem string) string { + path := filepath.Join(t.TempDir(), subsystem) + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatal(err) + } + return path +} + +func TestCfsSetLimit(t *testing.T) { + type test struct { + beforeCfsPeriodUs int + beforeCfsQuotaUs int + bandWidth Bandwidth + cfsQuotaUs int + changed bool + } + tests := []test{ + { + beforeCfsPeriodUs: 10000, + beforeCfsQuotaUs: -1, + bandWidth: Bandwidth(6000), + cfsQuotaUs: 60000, + changed: true, + }, + { + beforeCfsPeriodUs: 5000, + beforeCfsQuotaUs: -1, + bandWidth: Bandwidth(6000), + cfsQuotaUs: 30000, + changed: true, + }, + { + beforeCfsPeriodUs: 10000, + beforeCfsQuotaUs: 60000, + bandWidth: Bandwidth(6000), + cfsQuotaUs: 60000, + changed: false, + }, + } + for _, tc := range tests { + + tempdir := createTempDir(t, "cpu") + err := cgroups.WriteFile(tempdir, "cpu.cfs_period_us", strconv.Itoa(tc.beforeCfsPeriodUs)) + if err != nil { + t.Fatal(err) + } + err = cgroups.WriteFile(tempdir, "cpu.cfs_quota_us", strconv.Itoa(tc.beforeCfsQuotaUs)) + if err != nil { + t.Fatal(err) + } + + cfs := CgroupCFSController(tempdir) + changed, err := cfs.SetLimit(tc.bandWidth) + if err != nil { + t.Fatal(err) + } + if changed != tc.changed { + t.Fatalf("unexpected error: changed is '%v' but expected '%v'", changed, tc.changed) + } + cfsQuotaUs, err := cgroups.ReadFile(tempdir, "cpu.cfs_quota_us") + if err != nil { + t.Fatal(err) + } + if cfsQuotaUs != strconv.Itoa(tc.cfsQuotaUs) { + t.Fatalf("unexpected error: cfsQuotaUs is '%v' but expected '%v'", cfsQuotaUs, tc.cfsQuotaUs) + } + } + +} From 660cfd96c5df37525c4fd4fc1d7fef18d87da95e Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 25 Feb 2022 05:20:59 +0000 Subject: [PATCH 2/7] Deal with when cpu.cfs_quota_us is negative --- components/ws-daemon/pkg/cpulimit/cfs.go | 33 ++++++++++++++++--- components/ws-daemon/pkg/cpulimit/cfs_test.go | 1 - 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/components/ws-daemon/pkg/cpulimit/cfs.go b/components/ws-daemon/pkg/cpulimit/cfs.go index d09ae1d5b4f8cb..52260e203c726f 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs.go +++ b/components/ws-daemon/pkg/cpulimit/cfs.go @@ -39,7 +39,7 @@ func (basePath CgroupCFSController) SetLimit(limit Bandwidth) (changed bool, err } period := time.Duration(p) * time.Microsecond - q, err := basePath.readUint64("cpu.cfs_quota_us") + q, err := basePath.readInt64("cpu.cfs_quota_us") if err != nil { err = xerrors.Errorf("cannot parse CFS quota: %w", err) return @@ -60,7 +60,7 @@ func (basePath CgroupCFSController) SetLimit(limit Bandwidth) (changed bool, err func (basePath CgroupCFSController) readParentQuota() time.Duration { parent := CgroupCFSController(filepath.Dir(string(basePath))) - pq, err := parent.readUint64("cpu.cfs_quota_us") + pq, err := parent.readInt64("cpu.cfs_quota_us") if err != nil { return time.Duration(0) } @@ -68,14 +68,22 @@ func (basePath CgroupCFSController) readParentQuota() time.Duration { return time.Duration(pq) * time.Microsecond } -func (basePath CgroupCFSController) readUint64(path string) (uint64, error) { +func (basePath CgroupCFSController) readString(path string) (string, error) { fn := filepath.Join(string(basePath), path) fc, err := os.ReadFile(fn) if err != nil { - return 0, err + return "", err } s := strings.TrimSpace(string(fc)) + return s, nil +} + +func (basePath CgroupCFSController) readUint64(path string) (uint64, error) { + s, err := basePath.readString(path) + if err != nil { + return 0, err + } if s == "max" { return math.MaxUint64, nil } @@ -87,6 +95,23 @@ func (basePath CgroupCFSController) readUint64(path string) (uint64, error) { return uint64(p), nil } +func (basePath CgroupCFSController) readInt64(path string) (int64, error) { + s, err := basePath.readString(path) + if err != nil { + return 0, err + } + + if s == "max" { + return math.MaxInt64, nil + } + + p, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, err + } + return int64(p), nil +} + // NrThrottled returns the number of CFS periods the cgroup was throttled in func (basePath CgroupCFSController) NrThrottled() (uint64, error) { f, err := os.Open(filepath.Join(string(basePath), "cpu.stat")) diff --git a/components/ws-daemon/pkg/cpulimit/cfs_test.go b/components/ws-daemon/pkg/cpulimit/cfs_test.go index 565fbe0f1b7ba5..ee3c8cabdf5711 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs_test.go +++ b/components/ws-daemon/pkg/cpulimit/cfs_test.go @@ -57,7 +57,6 @@ func TestCfsSetLimit(t *testing.T) { }, } for _, tc := range tests { - tempdir := createTempDir(t, "cpu") err := cgroups.WriteFile(tempdir, "cpu.cfs_period_us", strconv.Itoa(tc.beforeCfsPeriodUs)) if err != nil { From 14b5a4715bfb046b58c86e90bf6b9cf8b52ec286 Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 25 Feb 2022 06:21:58 +0000 Subject: [PATCH 3/7] Unify the values of each cgroup for units when reading them. --- components/ws-daemon/pkg/cpulimit/cfs.go | 42 ++++++++++++++++-------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/components/ws-daemon/pkg/cpulimit/cfs.go b/components/ws-daemon/pkg/cpulimit/cfs.go index 52260e203c726f..0053588ebad97d 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs.go +++ b/components/ws-daemon/pkg/cpulimit/cfs.go @@ -22,29 +22,27 @@ type CgroupCFSController string // Usage returns the cpuacct.usage value of the cgroup func (basePath CgroupCFSController) Usage() (usage CPUTime, err error) { - cpuTimeInNS, err := basePath.readUint64("cpuacct.usage") + cputime, err := basePath.readCpuUsage() if err != nil { return 0, xerrors.Errorf("cannot read cpuacct.usage: %w", err) } - return CPUTime(time.Duration(cpuTimeInNS) * time.Nanosecond), nil + return CPUTime(cputime), nil } // SetQuota sets a new CFS quota on the cgroup func (basePath CgroupCFSController) SetLimit(limit Bandwidth) (changed bool, err error) { - p, err := basePath.readUint64("cpu.cfs_period_us") + period, err := basePath.readCfsPeriod() if err != nil { err = xerrors.Errorf("cannot parse CFS period: %w", err) return } - period := time.Duration(p) * time.Microsecond - q, err := basePath.readInt64("cpu.cfs_quota_us") + quota, err := basePath.readCfsQuota() if err != nil { err = xerrors.Errorf("cannot parse CFS quota: %w", err) return } - quota := time.Duration(q) * time.Microsecond target := limit.Quota(period) if quota == target { return false, nil @@ -60,7 +58,7 @@ func (basePath CgroupCFSController) SetLimit(limit Bandwidth) (changed bool, err func (basePath CgroupCFSController) readParentQuota() time.Duration { parent := CgroupCFSController(filepath.Dir(string(basePath))) - pq, err := parent.readInt64("cpu.cfs_quota_us") + pq, err := parent.readCfsQuota() if err != nil { return time.Duration(0) } @@ -79,24 +77,24 @@ func (basePath CgroupCFSController) readString(path string) (string, error) { return s, nil } -func (basePath CgroupCFSController) readUint64(path string) (uint64, error) { - s, err := basePath.readString(path) +func (basePath CgroupCFSController) readCfsPeriod() (time.Duration, error) { + s, err := basePath.readString("cpu.cfs_period_us") if err != nil { return 0, err } if s == "max" { - return math.MaxUint64, nil + return time.Duration(math.MaxInt64), nil } p, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err } - return uint64(p), nil + return time.Duration(uint(p)) * time.Microsecond, nil } -func (basePath CgroupCFSController) readInt64(path string) (int64, error) { - s, err := basePath.readString(path) +func (basePath CgroupCFSController) readCfsQuota() (time.Duration, error) { + s, err := basePath.readString("cpu.cfs_quota_us") if err != nil { return 0, err } @@ -109,7 +107,23 @@ func (basePath CgroupCFSController) readInt64(path string) (int64, error) { if err != nil { return 0, err } - return int64(p), nil + return time.Duration(p) * time.Microsecond, nil +} + +func (basePath CgroupCFSController) readCpuUsage() (time.Duration, error) { + s, err := basePath.readString("cpuacct.usage") + if err != nil { + return 0, err + } + if s == "max" { + return time.Duration(math.MaxInt64), nil + } + + p, err := strconv.ParseInt(s, 10, 64) + if err != nil { + return 0, err + } + return time.Duration(uint64(p)) * time.Nanosecond, nil } // NrThrottled returns the number of CFS periods the cgroup was throttled in From 2475ba5a44b720e83b80d6a341122116aa2f21ab Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 25 Feb 2022 06:43:19 +0000 Subject: [PATCH 4/7] Add some unit tests for the cpu subsystem of cgroup. --- components/ws-daemon/pkg/cpulimit/cfs_test.go | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/components/ws-daemon/pkg/cpulimit/cfs_test.go b/components/ws-daemon/pkg/cpulimit/cfs_test.go index ee3c8cabdf5711..914e424a5cb2d6 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs_test.go +++ b/components/ws-daemon/pkg/cpulimit/cfs_test.go @@ -83,5 +83,72 @@ func TestCfsSetLimit(t *testing.T) { t.Fatalf("unexpected error: cfsQuotaUs is '%v' but expected '%v'", cfsQuotaUs, tc.cfsQuotaUs) } } +} + +func TestReadCfsQuota(t *testing.T) { + tests := []int{ + 100000, + -1, + } + for _, tc := range tests { + tempdir := createTempDir(t, "cpu") + err := cgroups.WriteFile(tempdir, "cpu.cfs_quota_us", strconv.Itoa(tc)) + if err != nil { + t.Fatal(err) + } + + cfs := CgroupCFSController(tempdir) + v, err := cfs.readCfsQuota() + if err != nil { + t.Fatal(err) + } + if v.Microseconds() != int64(tc) { + t.Fatalf("unexpected error: cfs quota is '%v' but expected '%v'", v, tc) + } + } +} + +func TestReadCfsPeriod(t *testing.T) { + tests := []int{ + 10000, + } + for _, tc := range tests { + tempdir := createTempDir(t, "cpu") + err := cgroups.WriteFile(tempdir, "cpu.cfs_period_us", strconv.Itoa(tc)) + if err != nil { + t.Fatal(err) + } + + cfs := CgroupCFSController(tempdir) + v, err := cfs.readCfsPeriod() + if err != nil { + t.Fatal(err) + } + if v.Microseconds() != int64(tc) { + t.Fatalf("unexpected error: cfs period is '%v' but expected '%v'", v, tc) + } + } +} + +func TestReadCpuUsage(t *testing.T) { + tests := []int{ + 0, + 100000, + } + for _, tc := range tests { + tempdir := createTempDir(t, "cpu") + err := cgroups.WriteFile(tempdir, "cpuacct.usage", strconv.Itoa(tc)) + if err != nil { + t.Fatal(err) + } + cfs := CgroupCFSController(tempdir) + v, err := cfs.readCpuUsage() + if err != nil { + t.Fatal(err) + } + if v.Nanoseconds() != int64(tc) { + t.Fatalf("unexpected error: cpu usage is '%v' but expected '%v'", v, tc) + } + } } From a33de5459cd68a26a2215a47eec60b7c58dabf9f Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 25 Feb 2022 06:44:25 +0000 Subject: [PATCH 5/7] Since `max` is not used in the cpu subsystem of cgroup, delete it. --- components/ws-daemon/pkg/cpulimit/cfs.go | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/components/ws-daemon/pkg/cpulimit/cfs.go b/components/ws-daemon/pkg/cpulimit/cfs.go index 0053588ebad97d..20f53ccc0830aa 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs.go +++ b/components/ws-daemon/pkg/cpulimit/cfs.go @@ -6,7 +6,6 @@ package cpulimit import ( "bufio" - "math" "os" "path/filepath" "strconv" @@ -82,9 +81,6 @@ func (basePath CgroupCFSController) readCfsPeriod() (time.Duration, error) { if err != nil { return 0, err } - if s == "max" { - return time.Duration(math.MaxInt64), nil - } p, err := strconv.ParseInt(s, 10, 64) if err != nil { @@ -99,10 +95,6 @@ func (basePath CgroupCFSController) readCfsQuota() (time.Duration, error) { return 0, err } - if s == "max" { - return math.MaxInt64, nil - } - p, err := strconv.ParseInt(s, 10, 64) if err != nil { return 0, err @@ -115,9 +107,6 @@ func (basePath CgroupCFSController) readCpuUsage() (time.Duration, error) { if err != nil { return 0, err } - if s == "max" { - return time.Duration(math.MaxInt64), nil - } p, err := strconv.ParseInt(s, 10, 64) if err != nil { From 119be45c8e3bf4f4c52e7f5477aaf29b189f369d Mon Sep 17 00:00:00 2001 From: utam0k Date: Fri, 25 Feb 2022 07:21:24 +0000 Subject: [PATCH 6/7] When `cpu.cfs_quota_us` is negative, it is always set to the maximum value. https://elixir.bootlin.com/linux/v5.16/source/kernel/sched/core.c#L10292 --- components/ws-daemon/pkg/cpulimit/cfs.go | 6 ++++- components/ws-daemon/pkg/cpulimit/cfs_test.go | 25 ++++++++++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/components/ws-daemon/pkg/cpulimit/cfs.go b/components/ws-daemon/pkg/cpulimit/cfs.go index 20f53ccc0830aa..6d1b8aa7a4fc98 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs.go +++ b/components/ws-daemon/pkg/cpulimit/cfs.go @@ -6,6 +6,7 @@ package cpulimit import ( "bufio" + "math" "os" "path/filepath" "strconv" @@ -20,7 +21,6 @@ type CgroupCFSController string // Usage returns the cpuacct.usage value of the cgroup func (basePath CgroupCFSController) Usage() (usage CPUTime, err error) { - cputime, err := basePath.readCpuUsage() if err != nil { return 0, xerrors.Errorf("cannot read cpuacct.usage: %w", err) @@ -99,6 +99,10 @@ func (basePath CgroupCFSController) readCfsQuota() (time.Duration, error) { if err != nil { return 0, err } + + if p < 0 { + return time.Duration(math.MaxInt64), nil + } return time.Duration(p) * time.Microsecond, nil } diff --git a/components/ws-daemon/pkg/cpulimit/cfs_test.go b/components/ws-daemon/pkg/cpulimit/cfs_test.go index 914e424a5cb2d6..b62e1e3da09c25 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs_test.go +++ b/components/ws-daemon/pkg/cpulimit/cfs_test.go @@ -5,10 +5,12 @@ package cpulimit import ( + "math" "os" "path/filepath" "strconv" "testing" + "time" "github.com/opencontainers/runc/libcontainer/cgroups" ) @@ -86,13 +88,24 @@ func TestCfsSetLimit(t *testing.T) { } func TestReadCfsQuota(t *testing.T) { - tests := []int{ - 100000, - -1, + type test struct { + value int + expect int } + tests := []test{ + { + value: 100000, + expect: 100000, + }, + { + value: -1, + expect: int(time.Duration(math.MaxInt64).Microseconds()), + }, + } + for _, tc := range tests { tempdir := createTempDir(t, "cpu") - err := cgroups.WriteFile(tempdir, "cpu.cfs_quota_us", strconv.Itoa(tc)) + err := cgroups.WriteFile(tempdir, "cpu.cfs_quota_us", strconv.Itoa(tc.value)) if err != nil { t.Fatal(err) } @@ -102,8 +115,8 @@ func TestReadCfsQuota(t *testing.T) { if err != nil { t.Fatal(err) } - if v.Microseconds() != int64(tc) { - t.Fatalf("unexpected error: cfs quota is '%v' but expected '%v'", v, tc) + if v.Microseconds() != int64(tc.expect) { + t.Fatalf("unexpected error: cfs quota is '%v' but expected '%v'", v, tc.expect) } } } From 78720fccdc6512bb27871735f3549a56fd7e30d9 Mon Sep 17 00:00:00 2001 From: utam0k Date: Mon, 28 Feb 2022 01:10:55 +0000 Subject: [PATCH 7/7] Clarify the size ot the cfs_period_us. --- components/ws-daemon/pkg/cpulimit/cfs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/ws-daemon/pkg/cpulimit/cfs.go b/components/ws-daemon/pkg/cpulimit/cfs.go index 6d1b8aa7a4fc98..e1ffb7f71e44ad 100644 --- a/components/ws-daemon/pkg/cpulimit/cfs.go +++ b/components/ws-daemon/pkg/cpulimit/cfs.go @@ -86,7 +86,7 @@ func (basePath CgroupCFSController) readCfsPeriod() (time.Duration, error) { if err != nil { return 0, err } - return time.Duration(uint(p)) * time.Microsecond, nil + return time.Duration(uint64(p)) * time.Microsecond, nil } func (basePath CgroupCFSController) readCfsQuota() (time.Duration, error) {