From f7043aef7513cbb95b99610754a089e8f5d765f5 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Fri, 7 Jul 2023 15:07:46 +0100 Subject: [PATCH] fix(cli/clistat): better handle cgroups with no limits --- cli/clistat/cgroup.go | 62 +++++++++++++++++++++---------- cli/clistat/stat_internal_test.go | 38 +++++++++++++++---- 2 files changed, 73 insertions(+), 27 deletions(-) diff --git a/cli/clistat/cgroup.go b/cli/clistat/cgroup.go index f94e14405f1ff..d4fc9976550ce 100644 --- a/cli/clistat/cgroup.go +++ b/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 - quotaUs = int64(s.nproc) * periodUs + if xerrors.Is(err, strconv.ErrSyntax) { + // 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 - quotaUs = int64(s.nproc) * periodUs + return -1, nil } 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{ - Total: ptr.To(float64(maxUsageBytes)), - Used: float64(currUsageBytes - inactiveFileBytes), - Unit: "B", - Prefix: p, - }, nil + r.Used = float64(currUsageBytes - inactiveFileBytes) + return r, 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{ - Total: ptr.To(float64(maxUsageBytes)), - Used: float64(usageBytes - totalInactiveFileBytes), - Unit: "B", - Prefix: p, - }, nil + r.Used = float64(usageBytes - totalInactiveFileBytes) + + return r, nil } // read an int64 value from path diff --git a/cli/clistat/stat_internal_test.go b/cli/clistat/stat_internal_test.go index d52ef28557d59..25cb9e099f682 100644 --- a/cli/clistat/stat_internal_test.go +++ b/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) - assert.Equal(t, 2.0, *cpu.Total) + require.Nil(t, 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) - assert.Equal(t, 2.0, *cpu.Total) + require.Nil(t, 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", }