From 87adb1d0dd539726ebb658b7625eb9b2ef5b07ca Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Sun, 23 Mar 2025 19:35:07 +0000 Subject: [PATCH 1/6] chore: query container memory if available --- agent/proto/resourcesmonitor/fetcher.go | 22 ++++++++++++++++------ cli/clistat/container.go | 4 ++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/agent/proto/resourcesmonitor/fetcher.go b/agent/proto/resourcesmonitor/fetcher.go index 495a249fe9198..65945d1499fe9 100644 --- a/agent/proto/resourcesmonitor/fetcher.go +++ b/agent/proto/resourcesmonitor/fetcher.go @@ -13,19 +13,29 @@ type Fetcher interface { type fetcher struct { *clistat.Statter + isContainerized bool } //nolint:revive func NewFetcher(f *clistat.Statter) *fetcher { - return &fetcher{ - f, - } + isContainerized, _ := f.IsContainerized() + + return &fetcher{f, isContainerized} } func (f *fetcher) FetchMemory() (total int64, used int64, err error) { - mem, err := f.HostMemory(clistat.PrefixDefault) - if err != nil { - return 0, 0, xerrors.Errorf("failed to fetch memory: %w", err) + var mem *clistat.Result + + if f.isContainerized { + mem, err = f.ContainerMemory(clistat.PrefixDefault) + if err != nil { + return 0, 0, xerrors.Errorf("failed to fetch memory: %w", err) + } + } else { + mem, err = f.HostMemory(clistat.PrefixDefault) + if err != nil { + return 0, 0, xerrors.Errorf("failed to fetch memory: %w", err) + } } if mem.Total == nil { diff --git a/cli/clistat/container.go b/cli/clistat/container.go index b58d32591b907..cf64727d8b9c5 100644 --- a/cli/clistat/container.go +++ b/cli/clistat/container.go @@ -16,6 +16,10 @@ const ( kubernetesDefaultServiceAccountToken = "/var/run/secrets/kubernetes.io/serviceaccount/token" //nolint:gosec ) +func (s *Statter) IsContainerized() (ok bool, err error) { + return IsContainerized(s.fs) +} + // IsContainerized returns whether the host is containerized. // This is adapted from https://github.com/elastic/go-sysinfo/tree/main/providers/linux/container.go#L31 // with modifications to support Sysbox containers. From 1e28ae70d9b3dfd37f24dda858727c04f6e933fb Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 24 Mar 2025 09:24:14 +0000 Subject: [PATCH 2/6] chore: fallback to host memory limit A container may not have a memory limit set. If there is no memory limit set, we want to fallback to using the memory available on the underlying host. --- agent/proto/resourcesmonitor/fetcher.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/agent/proto/resourcesmonitor/fetcher.go b/agent/proto/resourcesmonitor/fetcher.go index 65945d1499fe9..59641b9d52ec2 100644 --- a/agent/proto/resourcesmonitor/fetcher.go +++ b/agent/proto/resourcesmonitor/fetcher.go @@ -29,12 +29,24 @@ func (f *fetcher) FetchMemory() (total int64, used int64, err error) { if f.isContainerized { mem, err = f.ContainerMemory(clistat.PrefixDefault) if err != nil { - return 0, 0, xerrors.Errorf("failed to fetch memory: %w", err) + return 0, 0, xerrors.Errorf("failed to fetch container memory: %w", err) + } + + // A container might not have a memory limit set. If this + // happens we want to fallback to querying the host's memory + // to know what the total memory is on the host. + if mem.Total == nil { + hostMem, err := f.HostMemory(clistat.PrefixDefault) + if err != nil { + return 0, 0, xerrors.Errorf("failed to host fetch memory: %w", err) + } + + mem.Total = hostMem.Total } } else { mem, err = f.HostMemory(clistat.PrefixDefault) if err != nil { - return 0, 0, xerrors.Errorf("failed to fetch memory: %w", err) + return 0, 0, xerrors.Errorf("failed to host fetch memory: %w", err) } } From 0a9681318d2de0c4d2d54533297fa8a59ed91bdb Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 24 Mar 2025 09:26:48 +0000 Subject: [PATCH 3/6] chore: add error handling --- agent/agent.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/agent/agent.go b/agent/agent.go index acd959582280f..6d7c1c8038daa 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -965,7 +965,10 @@ func (a *agent) run() (retErr error) { if err != nil { return xerrors.Errorf("failed to create resources fetcher: %w", err) } - resourcesFetcher := resourcesmonitor.NewFetcher(statfetcher) + resourcesFetcher, err := resourcesmonitor.NewFetcher(statfetcher) + if err != nil { + return xerrors.Errorf("new resource fetcher: %w", err) + } resourcesmonitor := resourcesmonitor.NewResourcesMonitor(logger, clk, config, resourcesFetcher, aAPI) return resourcesmonitor.Start(ctx) From f1dc9f7dea15d2e97fb0f9bf79914bd80d2eea73 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 24 Mar 2025 09:40:10 +0000 Subject: [PATCH 4/6] chore: error handling --- agent/proto/resourcesmonitor/fetcher.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/agent/proto/resourcesmonitor/fetcher.go b/agent/proto/resourcesmonitor/fetcher.go index 59641b9d52ec2..f269838437768 100644 --- a/agent/proto/resourcesmonitor/fetcher.go +++ b/agent/proto/resourcesmonitor/fetcher.go @@ -17,10 +17,13 @@ type fetcher struct { } //nolint:revive -func NewFetcher(f *clistat.Statter) *fetcher { - isContainerized, _ := f.IsContainerized() +func NewFetcher(f *clistat.Statter) (*fetcher, error) { + isContainerized, err := f.IsContainerized() + if err != nil { + return nil, xerrors.Errorf("check is containerized: %w", err) + } - return &fetcher{f, isContainerized} + return &fetcher{f, isContainerized}, nil } func (f *fetcher) FetchMemory() (total int64, used int64, err error) { @@ -29,7 +32,7 @@ func (f *fetcher) FetchMemory() (total int64, used int64, err error) { if f.isContainerized { mem, err = f.ContainerMemory(clistat.PrefixDefault) if err != nil { - return 0, 0, xerrors.Errorf("failed to fetch container memory: %w", err) + return 0, 0, xerrors.Errorf("fetch container memory: %w", err) } // A container might not have a memory limit set. If this @@ -38,7 +41,7 @@ func (f *fetcher) FetchMemory() (total int64, used int64, err error) { if mem.Total == nil { hostMem, err := f.HostMemory(clistat.PrefixDefault) if err != nil { - return 0, 0, xerrors.Errorf("failed to host fetch memory: %w", err) + return 0, 0, xerrors.Errorf("fetch host memory: %w", err) } mem.Total = hostMem.Total @@ -46,7 +49,7 @@ func (f *fetcher) FetchMemory() (total int64, used int64, err error) { } else { mem, err = f.HostMemory(clistat.PrefixDefault) if err != nil { - return 0, 0, xerrors.Errorf("failed to host fetch memory: %w", err) + return 0, 0, xerrors.Errorf("fetch host memory: %w", err) } } From a2182e1ff672726be39dc201741f9770acd7a64b Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 24 Mar 2025 10:15:11 +0000 Subject: [PATCH 5/6] chore: add tests --- agent/proto/resourcesmonitor/fetcher.go | 11 +- agent/proto/resourcesmonitor/fetcher_test.go | 108 +++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 agent/proto/resourcesmonitor/fetcher_test.go diff --git a/agent/proto/resourcesmonitor/fetcher.go b/agent/proto/resourcesmonitor/fetcher.go index f269838437768..8305ae571def3 100644 --- a/agent/proto/resourcesmonitor/fetcher.go +++ b/agent/proto/resourcesmonitor/fetcher.go @@ -6,18 +6,25 @@ import ( "github.com/coder/coder/v2/cli/clistat" ) +type Statter interface { + IsContainerized() (bool, error) + ContainerMemory(p clistat.Prefix) (*clistat.Result, error) + HostMemory(p clistat.Prefix) (*clistat.Result, error) + Disk(p clistat.Prefix, path string) (*clistat.Result, error) +} + type Fetcher interface { FetchMemory() (total int64, used int64, err error) FetchVolume(volume string) (total int64, used int64, err error) } type fetcher struct { - *clistat.Statter + Statter isContainerized bool } //nolint:revive -func NewFetcher(f *clistat.Statter) (*fetcher, error) { +func NewFetcher(f Statter) (*fetcher, error) { isContainerized, err := f.IsContainerized() if err != nil { return nil, xerrors.Errorf("check is containerized: %w", err) diff --git a/agent/proto/resourcesmonitor/fetcher_test.go b/agent/proto/resourcesmonitor/fetcher_test.go new file mode 100644 index 0000000000000..0edda5810edef --- /dev/null +++ b/agent/proto/resourcesmonitor/fetcher_test.go @@ -0,0 +1,108 @@ +package resourcesmonitor_test + +import ( + "errors" + "testing" + + "github.com/coder/coder/v2/agent/proto/resourcesmonitor" + "github.com/coder/coder/v2/cli/clistat" + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/stretchr/testify/require" +) + +type mockStatter struct { + isContainerized bool + containerMemory clistat.Result + hostMemory clistat.Result + disk map[string]clistat.Result +} + +func (s *mockStatter) IsContainerized() (bool, error) { + return s.isContainerized, nil +} + +func (s *mockStatter) ContainerMemory(_ clistat.Prefix) (*clistat.Result, error) { + return &s.containerMemory, nil +} + +func (s *mockStatter) HostMemory(_ clistat.Prefix) (*clistat.Result, error) { + return &s.hostMemory, nil +} + +func (s *mockStatter) Disk(_ clistat.Prefix, path string) (*clistat.Result, error) { + disk, ok := s.disk[path] + if !ok { + return nil, errors.New("path not found") + } + return &disk, nil +} + +func TestFetchMemory(t *testing.T) { + t.Parallel() + + t.Run("IsContainerized", func(t *testing.T) { + t.Parallel() + + t.Run("WithMemoryLimit", func(t *testing.T) { + t.Parallel() + + fetcher, err := resourcesmonitor.NewFetcher(&mockStatter{ + isContainerized: true, + containerMemory: clistat.Result{ + Used: 10.0, + Total: ptr.Ref(20.0), + }, + hostMemory: clistat.Result{ + Used: 20.0, + Total: ptr.Ref(30.0), + }, + }) + require.NoError(t, err) + + total, used, err := fetcher.FetchMemory() + require.NoError(t, err) + require.Equal(t, int64(10), used) + require.Equal(t, int64(20), total) + }) + + t.Run("WithoutMemoryLimit", func(t *testing.T) { + t.Parallel() + + fetcher, err := resourcesmonitor.NewFetcher(&mockStatter{ + isContainerized: true, + containerMemory: clistat.Result{ + Used: 10.0, + Total: nil, + }, + hostMemory: clistat.Result{ + Used: 20.0, + Total: ptr.Ref(30.0), + }, + }) + require.NoError(t, err) + + total, used, err := fetcher.FetchMemory() + require.NoError(t, err) + require.Equal(t, int64(10), used) + require.Equal(t, int64(30), total) + }) + }) + + t.Run("IsHost", func(t *testing.T) { + t.Parallel() + + fetcher, err := resourcesmonitor.NewFetcher(&mockStatter{ + isContainerized: false, + hostMemory: clistat.Result{ + Used: 20.0, + Total: ptr.Ref(30.0), + }, + }) + require.NoError(t, err) + + total, used, err := fetcher.FetchMemory() + require.NoError(t, err) + require.Equal(t, int64(20), used) + require.Equal(t, int64(30), total) + }) +} From 1df4073122c27a95b23c9a32f7de89d127d73cec Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Mon, 24 Mar 2025 10:24:14 +0000 Subject: [PATCH 6/6] chore: appease linter --- agent/proto/resourcesmonitor/fetcher_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/agent/proto/resourcesmonitor/fetcher_test.go b/agent/proto/resourcesmonitor/fetcher_test.go index 0edda5810edef..1b99023871a08 100644 --- a/agent/proto/resourcesmonitor/fetcher_test.go +++ b/agent/proto/resourcesmonitor/fetcher_test.go @@ -1,13 +1,14 @@ package resourcesmonitor_test import ( - "errors" "testing" + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + "github.com/coder/coder/v2/agent/proto/resourcesmonitor" "github.com/coder/coder/v2/cli/clistat" "github.com/coder/coder/v2/coderd/util/ptr" - "github.com/stretchr/testify/require" ) type mockStatter struct { @@ -32,7 +33,7 @@ func (s *mockStatter) HostMemory(_ clistat.Prefix) (*clistat.Result, error) { func (s *mockStatter) Disk(_ clistat.Prefix, path string) (*clistat.Result, error) { disk, ok := s.disk[path] if !ok { - return nil, errors.New("path not found") + return nil, xerrors.New("path not found") } return &disk, nil }