sandbox/cgroup: improve lookup for memory controller support with v2 (#…

…14766) * sandbox/cgroup: improve lookup for memory controller support with v2 With cgroup v2, we should not be looking at /proc/cgroups looking for memory controller, but rather at the list of availble controllers at the root of the hierarchy. Signed-off-by: Maciej Borzecki <[email protected]> * sandbox/cgroup: scan with word splitting Signed-off-by: Maciej Borzecki <[email protected]> * sandbox/cgroup: tweak errors and comments Signed-off-by: Maciej Borzecki <[email protected]> * tests/core/mem-cgroup-disabled: account for changed error message Signed-off-by: Maciej Borzecki <[email protected]> * sandbox/cgroup: use specific error value, wrap errors Signed-off-by: Maciej Borzecki <[email protected]> --------- Signed-off-by: Maciej Borzecki <[email protected]>
canonical · Nov 29, 2024 · b3e5b64 · b3e5b64
1 parent 47a8559
commit b3e5b64
Show file tree

Hide file tree

Showing 4 changed files with 142 additions and 30 deletions.
diff --git a/sandbox/cgroup/export_test.go b/sandbox/cgroup/export_test.go
@@ -108,12 +108,6 @@ func MockCreateScopeJobTimeout(d time.Duration) (restore func()) {
 	}
 }
 
-func MockCgroupsFilePath(path string) (restore func()) {
-	r := testutil.Backup(&cgroupsFilePath)
-	cgroupsFilePath = path
-	return r
-}
-
 func MonitorDelete(folders []string, name string, channel chan string) error {
 	return currentWatcher.monitorDelete(folders, name, channel)
 }

diff --git a/sandbox/cgroup/memory.go b/sandbox/cgroup/memory.go
@@ -21,12 +21,25 @@ package cgroup
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 )
 
-var cgroupsFilePath = "/proc/cgroups"
+var (
+	// path where v1 controllers are listed, see
+	// https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v1/index.html
+	// and cgroups(7)
+	cgroupV1ControllersPath = "/proc/cgroups"
+
+	// path where v2 controllers are listed, at the root of the hierarchy tree,
+	// see https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html
+	cgroupV2ControllersPath = filepath.Join(cgroupMountPoint, "cgroup.controllers")
+
+	errMemoryControllerDisabled = errors.New("cgroup memory controller is disabled on this system")
+)
 
 // CheckMemoryCgroup checks if the memory cgroup is enabled. It will return
 // an error if not.
@@ -38,9 +51,30 @@ var cgroupsFilePath = "/proc/cgroups"
 // 0 => false => disabled
 // 1 => true => enabled
 func CheckMemoryCgroup() error {
-	cgroupsFile, err := os.Open(cgroupsFilePath)
+	var supp bool
+	var err error
+	if IsUnified() {
+		supp, err = checkV2CgroupMemoryController()
+	} else {
+		supp, err = checkV1CgroupMemoryController()
+	}
+
 	if err != nil {
-		return fmt.Errorf("cannot open cgroups file: %v", err)
+		return err
+	}
+
+	if supp {
+		return nil
+	}
+
+	// no errors so far but found no evidence of memory controller to be enabled
+	return errMemoryControllerDisabled
+}
+
+func checkV1CgroupMemoryController() (bool, error) {
+	cgroupsFile, err := os.Open(filepath.Join(rootPath, cgroupV1ControllersPath))
+	if err != nil {
+		return false, fmt.Errorf("cannot open cgroups file: %w", err)
 	}
 	defer cgroupsFile.Close()
 
@@ -51,20 +85,40 @@ func CheckMemoryCgroup() error {
 			memoryCgroupValues := strings.Fields(line)
 			if len(memoryCgroupValues) < 4 {
 				// change in size, should investigate the new structure
-				return fmt.Errorf("cannot parse cgroups file: invalid line %q", line)
+				return false, fmt.Errorf("cannot parse cgroups file: invalid line %q", line)
 			}
 			isMemoryEnabled := memoryCgroupValues[3] == "1"
-			if !isMemoryEnabled {
-				return fmt.Errorf("memory cgroup is disabled on this system")
-			}
-			return nil
+			return isMemoryEnabled, nil
+		}
+	}
+
+	if err := scanner.Err(); err != nil {
+		return false, fmt.Errorf("cannot read %s contents: %w", cgroupV1ControllersPath, err)
+	}
+
+	return false, nil
+}
+
+func checkV2CgroupMemoryController() (bool, error) {
+	// check at the root controller
+	f, err := os.Open(filepath.Join(rootPath, cgroupV2ControllersPath))
+	if err != nil {
+		return false, err
+	}
+	defer f.Close()
+
+	scanner := bufio.NewScanner(f)
+	scanner.Split(bufio.ScanWords)
+	// expecting a single line
+	for scanner.Scan() {
+		if ctrl := scanner.Text(); ctrl == "memory" {
+			return true, nil
 		}
 	}
 
 	if err := scanner.Err(); err != nil {
-		return fmt.Errorf("cannot read %s contents: %v", cgroupsFilePath, err)
+		return false, err
 	}
 
-	// no errors so far but the only path here is the cgroups file without the memory line
-	return fmt.Errorf("cannot find memory cgroup in %s", cgroupsFilePath)
+	return false, nil
 }
diff --git a/sandbox/cgroup/memory_test.go b/sandbox/cgroup/memory_test.go
@@ -24,17 +24,28 @@ import (
 
 	. "gopkg.in/check.v1"
 
+	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/sandbox/cgroup"
 	"github.com/snapcore/snapd/testutil"
 )
 
-type memorySuite struct {
+type memorySuiteBase struct {
 	testutil.BaseTest
 
+	rootDir         string
 	mockCgroupsFile string
 }
 
-var _ = Suite(&memorySuite{})
+type memoryCgroupV1Suite struct {
+	memorySuiteBase
+}
+
+type memoryCgroupV2Suite struct {
+	memorySuiteBase
+}
+
+var _ = Suite(&memoryCgroupV1Suite{})
+var _ = Suite(&memoryCgroupV2Suite{})
 
 var (
 	cgroupContentCommon = `#subsys_name	hierarchy	num_cgroups	enabled
@@ -43,14 +54,28 @@ cpu	3	133	1
 devices	10	135	1`
 )
 
-func (s *memorySuite) SetUpTest(c *C) {
+func (s *memorySuiteBase) SetUpTest(c *C) {
 	s.BaseTest.SetUpTest(c)
 
-	s.mockCgroupsFile = filepath.Join(c.MkDir(), "cgroups")
-	s.AddCleanup(cgroup.MockCgroupsFilePath(s.mockCgroupsFile))
+	s.rootDir = c.MkDir()
+	dirs.SetRootDir(s.rootDir)
+	s.AddCleanup(func() { dirs.SetRootDir("/") })
+
+	s.AddCleanup(cgroup.MockVersion(cgroup.V1, nil))
+
+	s.mockCgroupsFile = filepath.Join(s.rootDir, "/proc/cgroups")
+	c.Assert(os.MkdirAll(filepath.Dir(s.mockCgroupsFile), 0755), IsNil)
+
+	c.Assert(os.MkdirAll(filepath.Join(s.rootDir, "/sys/fs/cgroup"), 0755), IsNil)
+}
+
+func (s *memoryCgroupV1Suite) SetUpTest(c *C) {
+	s.memorySuiteBase.SetUpTest(c)
+
+	s.AddCleanup(cgroup.MockVersion(cgroup.V1, nil))
 }
 
-func (s *memorySuite) TestCheckMemoryCgroupHappy(c *C) {
+func (s *memoryCgroupV1Suite) TestCheckMemoryCgroupV1_Happy(c *C) {
 	extra := "memory	2	223	1"
 	content := cgroupContentCommon + "\n" + extra
 
@@ -60,23 +85,23 @@ func (s *memorySuite) TestCheckMemoryCgroupHappy(c *C) {
 	c.Assert(err, IsNil)
 }
 
-func (s *memorySuite) TestCheckMemoryCgroupMissing(c *C) {
+func (s *memoryCgroupV1Suite) TestCheckMemoryCgroupV1_Missing(c *C) {
 	// note there is no file created for s.mockCgroupsFile
 
 	err := cgroup.CheckMemoryCgroup()
 	c.Assert(err, ErrorMatches, "cannot open cgroups file: open .*/cgroups: no such file or directory")
 }
 
-func (s *memorySuite) TestCheckMemoryCgroupNoMemoryEntry(c *C) {
+func (s *memoryCgroupV1Suite) TestCheckMemoryCgroupV1_NoMemoryEntry(c *C) {
 	content := cgroupContentCommon
 
 	err := os.WriteFile(s.mockCgroupsFile, []byte(content), 0644)
 	c.Assert(err, IsNil)
 	err = cgroup.CheckMemoryCgroup()
-	c.Assert(err, ErrorMatches, "cannot find memory cgroup in .*/cgroups")
+	c.Assert(err, ErrorMatches, "cgroup memory controller is disabled on this system")
 }
 
-func (s *memorySuite) TestCheckMemoryCgroupInvalidMemoryEntry(c *C) {
+func (s *memoryCgroupV1Suite) TestCheckMemoryCgroupV1_InvalidMemoryEntry(c *C) {
 	extra := "memory	invalid line"
 	content := cgroupContentCommon + "\n" + extra
 
@@ -86,12 +111,51 @@ func (s *memorySuite) TestCheckMemoryCgroupInvalidMemoryEntry(c *C) {
 	c.Assert(err, ErrorMatches, `cannot parse cgroups file: invalid line "memory\\tinvalid line"`)
 }
 
-func (s *memorySuite) TestCheckMemoryCgroupDisabled(c *C) {
+func (s *memoryCgroupV1Suite) TestCheckMemoryCgroupV1_Disabled(c *C) {
 	extra := "memory	2	223	0"
 	content := cgroupContentCommon + "\n" + extra
 
 	err := os.WriteFile(s.mockCgroupsFile, []byte(content), 0644)
 	c.Assert(err, IsNil)
 	err = cgroup.CheckMemoryCgroup()
-	c.Assert(err, ErrorMatches, "memory cgroup is disabled on this system")
+	c.Assert(err, ErrorMatches, "cgroup memory controller is disabled on this system")
+}
+
+func (s *memoryCgroupV2Suite) SetUpTest(c *C) {
+	s.memorySuiteBase.SetUpTest(c)
+
+	s.AddCleanup(cgroup.MockVersion(cgroup.V2, nil))
+}
+
+func (s *memoryCgroupV2Suite) TestCheckMemoryCgroupV2_Disabled(c *C) {
+	defer cgroup.MockVersion(cgroup.V2, nil)()
+
+	content := cgroupContentCommon + "\n"
+
+	// memory not mentioned in /proc/cgroups at all (like on 6.12+ kernels)
+	err := os.WriteFile(s.mockCgroupsFile, []byte(content), 0644)
+	c.Assert(err, IsNil)
+
+	v2ControllersFile := filepath.Join(s.rootDir, "/sys/fs/cgroup/cgroup.controllers")
+	// no memory
+	c.Assert(os.WriteFile(v2ControllersFile, []byte("foo bar baz other\n"), 0644), IsNil)
+
+	err = cgroup.CheckMemoryCgroup()
+	c.Assert(err, ErrorMatches, "cgroup memory controller is disabled on this system")
+}
+
+func (s *memoryCgroupV2Suite) TestCheckMemoryCgroupV2_Happy(c *C) {
+	defer cgroup.MockVersion(cgroup.V2, nil)()
+
+	content := cgroupContentCommon + "\n"
+
+	// memory not mentioned in /proc/cgroups at all (like on 6.12+ kernels)
+	err := os.WriteFile(s.mockCgroupsFile, []byte(content), 0644)
+	c.Assert(err, IsNil)
+
+	v2ControllersFile := filepath.Join(s.rootDir, "/sys/fs/cgroup/cgroup.controllers")
+	c.Assert(os.WriteFile(v2ControllersFile, []byte("foo bar baz memory other\n"), 0644), IsNil)
+
+	err = cgroup.CheckMemoryCgroup()
+	c.Assert(err, IsNil)
 }
diff --git a/tests/core/mem-cgroup-disabled/task.yaml b/tests/core/mem-cgroup-disabled/task.yaml
@@ -101,7 +101,7 @@ execute: |
       snap quota grp 2>&1 | MATCH "error: memory usage unavailable"
 
       # updating quota groups does not work and gives a useful error message
-      snap set-quota --memory=200MB grp 2>&1 | tr -s "\n " " " | MATCH 'error: cannot update quota group: cannot update group "grp": cannot use memory quota: memory cgroup is disabled on this system'
+      snap set-quota --memory=200MB grp 2>&1 | tr -s "\n " " " | MATCH 'error: cannot update quota group: cannot update group "grp": cannot use memory quota: cgroup memory controller is disabled on this system'
 
       # make sure our snap still has the Slice setting
       MATCH Slice=snap.grp.slice < "$SVC_UNIT"