From a5fd5015e9ff8ed89c5037d1e1c4b99409e4a1ed Mon Sep 17 00:00:00 2001
From: Fabricio Voznika <fvoznika@google.com>
Date: Mon, 24 Jul 2023 14:17:46 -0700
Subject: [PATCH] Better memory reporting for multi-container

Right now, the entire sandbox memory is reported per-container, confusing
users and tools that aggregate per-container memory to compute sandbox/pod
memory. So instead, split memory usage amoung all containers in the
system, except for the root container which is ignored by K8s. This way
pod memory usage is shown correctly in graphs.

Updates #172

PiperOrigin-RevId: 550670618
---
 runsc/boot/events.go                    | 29 +++++++++++++++++++++----
 runsc/boot/loader.go                    | 17 ++++++++++++++-
 runsc/container/multi_container_test.go | 13 ++++++++++-
 runsc/sandbox/sandbox.go                |  5 +----
 4 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/runsc/boot/events.go b/runsc/boot/events.go
index 65137de8a..c1a8595d6 100644
--- a/runsc/boot/events.go
+++ b/runsc/boot/events.go
@@ -15,6 +15,8 @@
 package boot
 
 import (
+	"errors"
+
 	"gvisor.dev/gvisor/pkg/sentry/control"
 	"gvisor.dev/gvisor/pkg/sentry/usage"
 )
@@ -81,22 +83,41 @@ type CPUUsage struct {
 }
 
 // Event gets the events from the container.
-func (cm *containerManager) Event(_ *struct{}, out *EventOut) error {
+func (cm *containerManager) Event(cid *string, out *EventOut) error {
 	*out = EventOut{
 		Event: Event{
+			ID:   *cid,
 			Type: "stats",
 		},
 	}
 
 	// Memory usage.
-	// TODO(gvisor.dev/issue/172): Per-container accounting.
 	mem := cm.l.k.MemoryFile()
 	_ = mem.UpdateUsage() // best effort to update.
 	_, totalUsage := usage.MemoryAccounting.Copy()
-	out.Event.Data.Memory.Usage = MemoryEntry{
-		Usage: totalUsage,
+	switch containers := cm.l.containerCount(); containers {
+	case 0:
+		return errors.New("no container was found")
+
+	case 1:
+		// There is a single container, so total usage can only come from it.
+
+	default:
+		// In the multi-container case, reports 0 for the root (pause) container,
+		// since it's small and idle. Then equally split the usage to the other
+		// containers. At least the sum of all containers will correctly account
+		// for the memory used by the sandbox.
+		//
+		// TODO(gvisor.dev/issue/172): Proper per-container accounting.
+		if *cid == cm.l.sandboxID {
+			totalUsage = 0
+		} else {
+			totalUsage /= uint64(containers - 1)
+		}
 	}
 
+	out.Event.Data.Memory.Usage.Usage = totalUsage
+
 	// PIDs.
 	// TODO(gvisor.dev/issue/172): Per-container accounting.
 	out.Event.Data.Pids.Current = uint64(len(cm.l.k.TaskSet().Root.ThreadGroups()))
diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go
index 9949fb09a..3ebe7f9f2 100644
--- a/runsc/boot/loader.go
+++ b/runsc/boot/loader.go
@@ -1009,7 +1009,7 @@ func (l *Loader) startGoferMonitor(cid string, rootfsGoferFD int32) {
 			},
 		}
 		_, _, err := specutils.RetryEintr(func() (uintptr, uintptr, error) {
-			// Use ppoll instead of poll because it's already whilelisted in seccomp.
+			// Use ppoll instead of poll because it's already allowed in seccomp.
 			n, err := unix.Ppoll(events, nil, nil)
 			return uintptr(n), 0, err
 		})
@@ -1608,3 +1608,18 @@ func (l *Loader) importFD(ctx context.Context, f *os.File) (*vfs.FileDescription
 	hostFD.Release()
 	return fd, nil
 }
+
+func (l *Loader) containerCount() int {
+	l.mu.Lock()
+	defer l.mu.Unlock()
+
+	containers := 0
+	for id := range l.processes {
+		if id.pid == 0 {
+			// pid==0 represents the init process of a container. There is
+			// only one of such process per container.
+			containers++
+		}
+	}
+	return containers
+}
diff --git a/runsc/container/multi_container_test.go b/runsc/container/multi_container_test.go
index 15d4f6500..c4c2764ee 100644
--- a/runsc/container/multi_container_test.go
+++ b/runsc/container/multi_container_test.go
@@ -2106,7 +2106,7 @@ func TestMultiContainerEvent(t *testing.T) {
 	}
 
 	// Check events for running containers.
-	for _, cont := range containers[:2] {
+	for i, cont := range containers[:2] {
 		ret, err := cont.Event()
 		if err != nil {
 			t.Errorf("Container.Event(%q): %v", cont.ID, err)
@@ -2123,6 +2123,17 @@ func TestMultiContainerEvent(t *testing.T) {
 			t.Errorf("Wrong number of PIDs, cid: %q, want: %d, got: %d", cont.ID, want, got)
 		}
 
+		switch i {
+		case 0:
+			if evt.Data.Memory.Usage.Usage != uint64(0) {
+				t.Errorf("root container should report 0 memory usage, got: %v", evt.Data.Memory.Usage.Usage)
+			}
+		case 1:
+			if evt.Data.Memory.Usage.Usage == uint64(0) {
+				t.Error("sub-container should report non-zero memory usage")
+			}
+		}
+
 		// The exited container should always have a usage of zero.
 		if exited := ret.ContainerUsage[containers[2].ID]; exited != 0 {
 			t.Errorf("Exited container should report 0 CPU usage, got: %d", exited)
diff --git a/runsc/sandbox/sandbox.go b/runsc/sandbox/sandbox.go
index eac90c992..d67577334 100644
--- a/runsc/sandbox/sandbox.go
+++ b/runsc/sandbox/sandbox.go
@@ -577,12 +577,9 @@ func (s *Sandbox) Execute(conf *config.Config, args *control.ExecArgs) (int32, e
 func (s *Sandbox) Event(cid string) (*boot.EventOut, error) {
 	log.Debugf("Getting events for container %q in sandbox %q", cid, s.ID)
 	var e boot.EventOut
-	// TODO(b/129292330): Pass in the container id (cid) here. The sandbox
-	// should return events only for that container.
-	if err := s.call(boot.ContMgrEvent, nil, &e); err != nil {
+	if err := s.call(boot.ContMgrEvent, &cid, &e); err != nil {
 		return nil, fmt.Errorf("retrieving event data from sandbox: %w", err)
 	}
-	e.Event.ID = cid
 	return &e, nil
 }