Repository: opencontainers/runc
Pull request #3977 (2023-08-09 18:18:23)

[1.1] libct/cg/fs2: use `file` + `anon` + `swap` for usage
Backport of #3933 by @alexeldeib to release-1.1 branch. Original description follows. 

----

This aligns v2 usage calculations more closely with v1. Current node-level reporting for v1 vs v2 on the same machine under similar load may differ by ~250-750Mi.

Also return usage as combined swap + memory usage, aligned with v1 and non-root v2 cgroups.

`mem_cgroup_usage` in the kernel counts NR_FILE_PAGES
+ NR_ANON_MAPPED + `nr_swap_pages` (if swap enabled) [^0].

Using total - free results in higher "usage" numbers. This is likely due to various types of reclaimable memory technically counted as in use (e.g. inactive anon).

See also https://github.com/kubernetes/kubernetes/issues/118916 for more context

[^0]: https://github.com/torvalds/linux/blob/06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5/mm/memcontrol.c#L3673-L3680

haircommander: cc @mrunalp @kolyshkin 
haircommander: @AkihiroSuda I know you had hesitation on the other PR, but I think the differences qualify as a bug and that justifies including it in 1.1 (especially so kubernetes can vendor it before 1.2 is released)
kolyshkin: Let's merge it after #3976 so we can run CI on go 1.21.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index ad7e4d39..65ebce91 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased 1.1.z]
 
+### Changed
+
+ * 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)
+
 ## [1.1.8] - 2023-07-20
 
 > 海纳百川 有容乃大
diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go
index adbc4b23..9cca98c4 100644
--- a/libcontainer/cgroups/fs2/memory.go
+++ b/libcontainer/cgroups/fs2/memory.go
@@ -101,8 +101,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
 	}
@@ -154,7 +155,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 {
@@ -166,14 +167,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
@@ -206,11 +203,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
 }


Test patch:
diff --git a/libcontainer/cgroups/fs2/memory_test.go b/libcontainer/cgroups/fs2/memory_test.go
new file mode 100644
index 00000000..2e2713c2
--- /dev/null
+++ b/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)
+	}
+}