Repository: opencontainers/runc
Pull request #3933 (2023-07-10 13:51:46)

libct/cg/fs2: use file + anon + swap for usage
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 several hundred megabytes.

`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

mrunalp: @kolyshkin @cyphar ptal
AkihiroSuda: This is likely to break compatibility, so I'm not sure we can merge this.

alexeldeib: @AkihiroSuda could you elaborate on the problem?

Currently causes major pain as a behavioral change to downstream consumers for v1 vs v2. I would argue compatibility has _already_ been broken, and this restores previous behavior.

Can you clarify what you mean? Who breaks here with this change and in what way?


AkihiroSuda: > @AkihiroSuda could you elaborate on the problem?
> 
> Current code causes major pain as a behavioral change to downstream consumers for v1 vs v2. I would argue compatibility has _already_ been broken, and this restores previous behavior.
> 
> Can you clarify what you mean? Who breaks here with this change and in what way?

There might be third party programs that are already depending on the existing output for v2.

alexeldeib: I can understand that this is a change to v2 consumers. I am not sure I see how it leads to breakage. simple see a lower number for usage?

Alternative changes up the stack would lead to runc producing different stats for identical memory usage under v1 and v2, but cadvisor and Kubernetes would fudge it using the same calculation as this PR.

 It seems to me “more correct” to fix the actual meaning of usage. But if there is concern about this approach, then I would start the discussion at cadvisor level or potentially purely in k/k.

The same argument you’re suggesting applies to cadvisor and k/k too, but at least Kubernetes folks seem to agree this is a bug for v1/v2. So I hope folks at *some* layer are willing to adapt and recognize the calculation is wildly different.
alexeldeib: Reframing. I can understand that changing these numbers with no API change
has an effect on users. That’s exactly the scenario I hope to “fix” and the
one which runc “broke” with the existing calculation.

What I don’t understand, is what then runc’s contract actually is. Is it
implementation defined by “whatever we did first”? That leaves little room
for bug fixes.

Is it formally defined — “usage is rss + cache”? Then this PR makes sense
and repairs an existing API violation, regardless of existing behavior. I
don’t see a formal definition of this API.

Cadvisor and Kubernetes strike me as rather large 3p users as you put it. I
would be curious where runc puts the balance of practical usage and
behavioral consistency against “bug compatibility” and you think this

There is plenty of opportunity up the stack for fudging. However IMO it’s
extremely misleading machines with comparable usage will report differently
whether they are v1 or v2. If runc doesn’t consider than a bug, okay I
suppose, but I’d like to understand clearly for future reference when we
can or cannot make corrective behavioral changes like this and what runc’s
actual guarantees are. It’s a bit disappointing to me something like this
would differ from v1 since day 1 and may remain baked in — future 3p users
would have to know about this and fudge their logic too if they care about
getting the ”correct” data, otherwise they will perceive they lose memory
when they upgrade to v2.

On Sun, Jul 16, 2023 at 9:38 AM Akihiro Suda ***@***.***>
wrote:

> @AkihiroSuda <https://github.com/AkihiroSuda> could you elaborate on the
> problem?
>
> Current code causes major pain as a behavioral change to downstream
> consumers for v1 vs v2. I would argue compatibility has *already* been
> broken, and this restores previous behavior.
>
> Can you clarify what you mean? Who breaks here with this change and in
> what way?
>
> There might be third party programs that are already depending on the
> existing output for v2.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/opencontainers/runc/pull/3933#issuecomment-1637087001>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABT4LWLCWMUCJPNQUKX4HNLXQPVF5ANCNFSM6AAAAAA2ESQEVI>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>

mrunalp: >  I can understand that this is a change to v2 consumers. I am not sure I see how it leads to breakage. You’d simply see a lower number for usage? The contract is unchanged.
I agree. I am not seeing how this would be breaking if we are presenting a more accurate view of usage in line with v1. 

AkihiroSuda: I'm ok to merge this, but if something breaks we may have to revert this and maybe re-add it as another file like "Usage2".

AkihiroSuda: Could you update https://github.com/opencontainers/runc/blob/main/CHANGELOG.md to explain the change?

alexeldeib: > Could you update https://github.com/opencontainers/runc/blob/main/CHANGELOG.md to explain the change?

Wasn't sure, if I should mark this as "unreleased" or create a new unreleased 1.1.9 tag? marked it as "unreleased - change"
kolyshkin: I agree with the general direction of this PR; thank you for your contribution @alexeldeib.

 Just a few notes:
1. The logic of getting the root cgroup stats should better be contained in `statsFromMeminfo`, not spread between the two as it is now.
2. Maybe instead adding an extra argument to statMemory for the sake of unit test, we can test `statsFromMeminfo` directly? This will make the commit less invasive.

While at it, it might make sense to rename `statsFromMeminfo` to e.g. `rootStatsFromMeminfo` so it is more clear it is only used for root cgroup.
kolyshkin: @alexeldeib needs a rebase
alexeldeib: I think that might be better?

swap confused me a bit, but I think I get it. I do not think we should count swapcached (and it's the wrong number anyway).

v1 has `usage_in_bytes` from `mem_cgroup_read_u64` with `MEMFILE_PRIVATE(_MEM, RES_USAGE)` 
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L5016-L5020

In this case we use `memcg->memory` not `memcg->memsw` (which is v1 only? vs `memcg->swap` for v2 only?)
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L3705-L3706

which would mean we actually call `mem_cgroup_usage` with swap = false
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L3722-L3724

which makes sense semantically for us anyway - if usage were already usage with swap, then summing swap + usage for swap would be silly.

v2 has `current` with `memory_current_read` which seems to just read `memcg->memory`
https://github.com/torvalds/linux/blob/5d0c230f1de8c7515b6567d9afba1f196fb4e2f4/mm/memcontrol.c#L6714-L6715

so v2 is consistent at least there

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index b09d44c8..adcf084d 100644
--- a/CHANGELOG.md
+++ b/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
 
diff --git a/libcontainer/cgroups/fs2/memory.go b/libcontainer/cgroups/fs2/memory.go
index e3b857dc..85e96b1c 100644
--- a/libcontainer/cgroups/fs2/memory.go
+++ b/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
 }


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)
+	}
+}