fix(cli/clistat): better handle cgroups with no limits (#8373)

cli/clistat/cgroup.go

@@ -61,7 +61,6 @@ func (s *Statter) ContainerCPU() (*Result, error) {
 	if err != nil {
 		return nil, xerrors.Errorf("get total cpu: %w", err)
 	}
-
 	used1, err := s.cGroupCPUUsed()
 	if err != nil {
 		return nil, xerrors.Errorf("get cgroup CPU usage: %w", err)
@@ -86,9 +85,12 @@ func (s *Statter) ContainerCPU() (*Result, error) {
 	r := &Result{
 		Unit:   "cores",
 		Used:   used2 - used1,
-		Total:  ptr.To(total),
 		Prefix: PrefixDefault,
 	}
+
+	if total > 0 {
+		r.Total = ptr.To(total)
+	}
 	return r, nil
 }
 
@@ -137,8 +139,12 @@ func (s *Statter) cGroupV2CPUTotal() (total float64, err error) {
 
 	quotaUs, err = readInt64SepIdx(s.fs, cgroupV2CPUMax, " ", 0)
 	if err != nil {
-		// Fall back to number of cores
+		if xerrors.Is(err, strconv.ErrSyntax) {
-		quotaUs = int64(s.nproc) * periodUs
+			// If the value is not a valid integer, assume it is the string
+			// 'max' and that there is no limit set.
+			return -1, nil
+		}
+		return 0, xerrors.Errorf("get cpu quota: %w", err)
 	}
 
 	return float64(quotaUs) / float64(periodUs), nil
@@ -156,8 +162,7 @@ func (s *Statter) cGroupV1CPUTotal() (float64, error) {
 	}
 
 	if quotaUs < 0 {
-		// Fall back to the number of cores
+		return -1, nil
-		quotaUs = int64(s.nproc) * periodUs
 	}
 
 	return float64(quotaUs) / float64(periodUs), nil
@@ -199,9 +204,19 @@ func (s *Statter) ContainerMemory(p Prefix) (*Result, error) {
 }
 
 func (s *Statter) cGroupV2Memory(p Prefix) (*Result, error) {
+	r := &Result{
+		Unit:   "B",
+		Prefix: p,
+	}
 	maxUsageBytes, err := readInt64(s.fs, cgroupV2MemoryMaxBytes)
 	if err != nil {
-		return nil, xerrors.Errorf("read memory total: %w", err)
+		if !xerrors.Is(err, strconv.ErrSyntax) {
+			return nil, xerrors.Errorf("read memory total: %w", err)
+		}
+		// If the value is not a valid integer, assume it is the string
+		// 'max' and that there is no limit set.
+	} else {
+		r.Total = ptr.To(float64(maxUsageBytes))
 	}
 
 	currUsageBytes, err := readInt64(s.fs, cgroupV2MemoryUsageBytes)
@@ -214,18 +229,23 @@ func (s *Statter) cGroupV2Memory(p Prefix) (*Result, error) {
 		return nil, xerrors.Errorf("read memory stats: %w", err)
 	}
 
-	return &Result{
+	r.Used = float64(currUsageBytes - inactiveFileBytes)
-		Total:  ptr.To(float64(maxUsageBytes)),
+	return r, nil
-		Used:   float64(currUsageBytes - inactiveFileBytes),
-		Unit:   "B",
-		Prefix: p,
-	}, nil
 }
 
 func (s *Statter) cGroupV1Memory(p Prefix) (*Result, error) {
+	r := &Result{
+		Unit:   "B",
+		Prefix: p,
+	}
 	maxUsageBytes, err := readInt64(s.fs, cgroupV1MemoryMaxUsageBytes)
 	if err != nil {
-		return nil, xerrors.Errorf("read memory total: %w", err)
+		if !xerrors.Is(err, strconv.ErrSyntax) {
+			return nil, xerrors.Errorf("read memory total: %w", err)
+		}
+		// I haven't found an instance where this isn't a valid integer.
+		// Nonetheless, if it is not, assume there is no limit set.
+		maxUsageBytes = -1
 	}
 
 	// need a space after total_rss so we don't hit something else
@@ -239,13 +259,15 @@ func (s *Statter) cGroupV1Memory(p Prefix) (*Result, error) {
 		return nil, xerrors.Errorf("read memory stats: %w", err)
 	}
 
+	// If max usage bytes is -1, there is no memory limit set.
+	if maxUsageBytes > 0 {
+		r.Total = ptr.To(float64(maxUsageBytes))
+	}
+
 	// Total memory used is usage - total_inactive_file
-	return &Result{
+	r.Used = float64(usageBytes - totalInactiveFileBytes)
-		Total:  ptr.To(float64(maxUsageBytes)),
+
-		Used:   float64(usageBytes - totalInactiveFileBytes),
+	return r, nil
-		Unit:   "B",
-		Prefix: p,
-	}, nil
 }
 
 // read an int64 value from path

cli/clistat/stat_internal_test.go

@@ -149,8 +149,7 @@ func TestStatter(t *testing.T) {
 			require.NoError(t, err)
 			require.NotNil(t, cpu)
 			assert.Equal(t, 1.0, cpu.Used)
-			require.NotNil(t, cpu.Total)
+			require.Nil(t, cpu.Total)
-			assert.Equal(t, 2.0, *cpu.Total)
 			assert.Equal(t, "cores", cpu.Unit)
 		})
 
@@ -167,6 +166,19 @@ func TestStatter(t *testing.T) {
 			assert.Equal(t, 1073741824.0, *mem.Total)
 			assert.Equal(t, "B", mem.Unit)
 		})
+
+		t.Run("ContainerMemory/NoLimit", func(t *testing.T) {
+			t.Parallel()
+			fs := initFS(t, fsContainerCgroupV1NoLimit)
+			s, err := New(WithFS(fs), withNoWait)
+			require.NoError(t, err)
+			mem, err := s.ContainerMemory(PrefixDefault)
+			require.NoError(t, err)
+			require.NotNil(t, mem)
+			assert.Equal(t, 268435456.0, mem.Used)
+			assert.Nil(t, mem.Total)
+			assert.Equal(t, "B", mem.Unit)
+		})
 	})
 
 	t.Run("CGroupV2", func(t *testing.T) {
@@ -201,12 +213,11 @@ func TestStatter(t *testing.T) {
 			require.NoError(t, err)
 			require.NotNil(t, cpu)
 			assert.Equal(t, 1.0, cpu.Used)
-			require.NotNil(t, cpu.Total)
+			require.Nil(t, cpu.Total)
-			assert.Equal(t, 2.0, *cpu.Total)
 			assert.Equal(t, "cores", cpu.Unit)
 		})
 
-		t.Run("ContainerMemory", func(t *testing.T) {
+		t.Run("ContainerMemory/Limit", func(t *testing.T) {
 			t.Parallel()
 			fs := initFS(t, fsContainerCgroupV2)
 			s, err := New(WithFS(fs), withNoWait)
@@ -219,6 +230,19 @@ func TestStatter(t *testing.T) {
 			assert.Equal(t, 1073741824.0, *mem.Total)
 			assert.Equal(t, "B", mem.Unit)
 		})
+
+		t.Run("ContainerMemory/NoLimit", func(t *testing.T) {
+			t.Parallel()
+			fs := initFS(t, fsContainerCgroupV2NoLimit)
+			s, err := New(WithFS(fs), withNoWait)
+			require.NoError(t, err)
+			mem, err := s.ContainerMemory(PrefixDefault)
+			require.NoError(t, err)
+			require.NotNil(t, mem)
+			assert.Equal(t, 268435456.0, mem.Used)
+			assert.Nil(t, mem.Total)
+			assert.Equal(t, "B", mem.Unit)
+		})
 	})
 }
 
@@ -316,7 +340,7 @@ proc /proc/sys proc ro,nosuid,nodev,noexec,relatime 0 0`,
 proc /proc/sys proc ro,nosuid,nodev,noexec,relatime 0 0`,
 		cgroupV2CPUMax:           "max 100000",
 		cgroupV2CPUStat:          "usage_usec 0",
-		cgroupV2MemoryMaxBytes:   "1073741824",
+		cgroupV2MemoryMaxBytes:   "max",
 		cgroupV2MemoryUsageBytes: "536870912",
 		cgroupV2MemoryStat:       "inactive_file 268435456",
 	}
@@ -338,7 +362,7 @@ proc /proc/sys proc ro,nosuid,nodev,noexec,relatime 0 0`,
 		cgroupV1CPUAcctUsage:        "0",
 		cgroupV1CFSQuotaUs:          "-1",
 		cgroupV1CFSPeriodUs:         "100000",
-		cgroupV1MemoryMaxUsageBytes: "1073741824",
+		cgroupV1MemoryMaxUsageBytes: "max", // I have never seen this in the wild
 		cgroupV1MemoryUsageBytes:    "536870912",
 		cgroupV1MemoryStat:          "total_inactive_file 268435456",
 	}