Merge pull request #3933 from alexeldeib/ace/v2root

libct/cg/fs2: use file + anon + swap for usage

CHANGELOG.md

@@ -26,6 +26,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
    (so that the container init is a child of that process) must now implement
    a proper child reaper in case a container does not have its own private PID
    namespace, as documented in `container.Signal`. (#3825)
+ * Sum `anon` and `file` from `memory.stat` for cgroupv2 root usage,
+   as the root does not have `memory.current` for cgroupv2.
+   This aligns cgroupv2 root usage more closely with cgroupv1 reporting.
+   Additionally, report root swap usage as sum of swap and memory usage,
+   aligned with v1 and existing non-root v2 reporting. (#3933)
 
 ### Fixed
 

libcontainer/cgroups/fs2/memory.go

@@ -106,8 +106,9 @@ func statMemory(dirPath string, stats *cgroups.Stats) error {
 	if err != nil {
 		if errors.Is(err, unix.ENOENT) && dirPath == UnifiedMountpoint {
 			// The root cgroup does not have memory.{current,max}
-			// so emulate those using data from /proc/meminfo.
-			return statsFromMeminfo(stats)
+			// so emulate those using data from /proc/meminfo and
+			// /sys/fs/cgroup/memory.stat
+			return rootStatsFromMeminfo(stats)
 		}
 		return err
 	}
@@ -159,7 +160,7 @@ func getMemoryDataV2(path, name string) (cgroups.MemoryData, error) {
 	return memoryData, nil
 }
 
-func statsFromMeminfo(stats *cgroups.Stats) error {
+func rootStatsFromMeminfo(stats *cgroups.Stats) error {
 	const file = "/proc/meminfo"
 	f, err := os.Open(file)
 	if err != nil {
@@ -171,14 +172,10 @@ func statsFromMeminfo(stats *cgroups.Stats) error {
 	var (
 		swap_free  uint64
 		swap_total uint64
-		main_total uint64
-		main_free  uint64
 	)
 	mem := map[string]*uint64{
 		"SwapFree":  &swap_free,
 		"SwapTotal": &swap_total,
-		"MemTotal":  &main_total,
-		"MemFree":   &main_free,
 	}
 
 	found := 0
@@ -211,11 +208,18 @@ func statsFromMeminfo(stats *cgroups.Stats) error {
 		return &parseError{Path: "", File: file, Err: err}
 	}
 
+	// cgroup v1 `usage_in_bytes` reports memory usage as the sum of
+	// - rss (NR_ANON_MAPPED)
+	// - cache (NR_FILE_PAGES)
+	// cgroup v1 reports SwapUsage values as mem+swap combined
+	// cgroup v2 reports rss and cache as anon and file.
+	// sum `anon` + `file` to report the same value as `usage_in_bytes` in v1.
+	// sum swap usage as combined mem+swap usage for consistency as well.
+	stats.MemoryStats.Usage.Usage = stats.MemoryStats.Stats["anon"] + stats.MemoryStats.Stats["file"]
+	stats.MemoryStats.Usage.Limit = math.MaxUint64
 	stats.MemoryStats.SwapUsage.Usage = (swap_total - swap_free) * 1024
 	stats.MemoryStats.SwapUsage.Limit = math.MaxUint64
-
-	stats.MemoryStats.Usage.Usage = (main_total - main_free) * 1024
-	stats.MemoryStats.Usage.Limit = math.MaxUint64
+	stats.MemoryStats.SwapUsage.Usage += stats.MemoryStats.Usage.Usage
 
 	return nil
 }

libcontainer/cgroups/fs2/memory_test.go

@@ -0,0 +1,139 @@
+package fs2
+
+import (
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/opencontainers/runc/libcontainer/cgroups"
+)
+
+const exampleMemoryStatData = `anon 790425600
+file 6502666240
+kernel_stack 7012352
+pagetables 8867840
+percpu 2445520
+sock 40960
+shmem 6721536
+file_mapped 656187392
+file_dirty 1122304
+file_writeback 0
+swapcached 10
+anon_thp 438304768
+file_thp 0
+shmem_thp 0
+inactive_anon 892223488
+active_anon 2973696
+inactive_file 5307346944
+active_file 1179316224
+unevictable 31477760
+slab_reclaimable 348866240
+slab_unreclaimable 10099808
+slab 358966048
+workingset_refault_anon 0
+workingset_refault_file 0
+workingset_activate_anon 0
+workingset_activate_file 0
+workingset_restore_anon 0
+workingset_restore_file 0
+workingset_nodereclaim 0
+pgfault 103216687
+pgmajfault 6879
+pgrefill 0
+pgscan 0
+pgsteal 0
+pgactivate 1110217
+pgdeactivate 292
+pglazyfree 267
+pglazyfreed 0
+thp_fault_alloc 57411
+thp_collapse_alloc 443`
+
+func TestStatMemoryPodCgroupNotFound(t *testing.T) {
+	// We're using a fake cgroupfs.
+	cgroups.TestMode = true
+	fakeCgroupDir := t.TempDir()
+
+	// only write memory.stat to ensure pod cgroup usage
+	// still reads memory.current.
+	statPath := filepath.Join(fakeCgroupDir, "memory.stat")
+	if err := os.WriteFile(statPath, []byte(exampleMemoryStatData), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	gotStats := cgroups.NewStats()
+
+	// use a fake root path to mismatch the file we wrote.
+	// this triggers the non-root path which should fail to find memory.current.
+	err := statMemory(fakeCgroupDir, gotStats)
+	if err == nil {
+		t.Errorf("expected error when statting memory for cgroupv2 root, but was nil")
+	}
+
+	if !strings.Contains(err.Error(), "memory.current: no such file or directory") {
+		t.Errorf("expected error to contain 'memory.current: no such file or directory', but was %s", err.Error())
+	}
+}
+
+func TestStatMemoryPodCgroup(t *testing.T) {
+	// We're using a fake cgroupfs.
+	cgroups.TestMode = true
+	fakeCgroupDir := t.TempDir()
+
+	statPath := filepath.Join(fakeCgroupDir, "memory.stat")
+	if err := os.WriteFile(statPath, []byte(exampleMemoryStatData), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := os.WriteFile(filepath.Join(fakeCgroupDir, "memory.current"), []byte("123456789"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := os.WriteFile(filepath.Join(fakeCgroupDir, "memory.max"), []byte("999999999"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	gotStats := cgroups.NewStats()
+
+	// use a fake root path to trigger the pod cgroup lookup.
+	err := statMemory(fakeCgroupDir, gotStats)
+	if err != nil {
+		t.Errorf("expected no error when statting memory for cgroupv2 root, but got %#+v", err)
+	}
+
+	// result should be "memory.current"
+	var expectedUsageBytes uint64 = 123456789
+	if gotStats.MemoryStats.Usage.Usage != expectedUsageBytes {
+		t.Errorf("parsed cgroupv2 memory.stat doesn't match expected result: \ngot %#v\nexpected %#v\n", gotStats.MemoryStats.Usage.Usage, expectedUsageBytes)
+	}
+}
+
+func TestRootStatsFromMeminfo(t *testing.T) {
+	stats := &cgroups.Stats{
+		MemoryStats: cgroups.MemoryStats{
+			Stats: map[string]uint64{
+				"anon": 790425600,
+				"file": 6502666240,
+			},
+		},
+	}
+
+	if err := rootStatsFromMeminfo(stats); err != nil {
+		t.Fatal(err)
+	}
+
+	// result is anon + file
+	var expectedUsageBytes uint64 = 7293091840
+	if stats.MemoryStats.Usage.Usage != expectedUsageBytes {
+		t.Errorf("parsed cgroupv2 memory.stat doesn't match expected result: \ngot %d\nexpected %d\n", stats.MemoryStats.Usage.Usage, expectedUsageBytes)
+	}
+
+	// swap is adjusted to mem+swap
+	if stats.MemoryStats.SwapUsage.Usage < stats.MemoryStats.Usage.Usage {
+		t.Errorf("swap usage %d should be at least mem usage %d", stats.MemoryStats.SwapUsage.Usage, stats.MemoryStats.Usage.Usage)
+	}
+	if stats.MemoryStats.SwapUsage.Limit < stats.MemoryStats.Usage.Limit {
+		t.Errorf("swap limit %d should be at least mem limit %d", stats.MemoryStats.SwapUsage.Limit, stats.MemoryStats.Usage.Limit)
+	}
+}