diff --git a/changelog/fragments/1786038287-fix-cache-container-memory.yaml b/changelog/fragments/1786038287-fix-cache-container-memory.yaml deleted file mode 100644 index cdbbc1733f..0000000000 --- a/changelog/fragments/1786038287-fix-cache-container-memory.yaml +++ /dev/null @@ -1,13 +0,0 @@ -kind: bug-fix - -summary: Fix ristretto cache sized from host RAM instead of container memory limit - -description: > - memEnvLimits() called memory.TotalMemory() to select the ristretto cache tier. - On a Kubernetes node with >=16 GB of RAM this picked a MaxCost of 256-512 MB -- - 2-4x the pod's GOMEMLIMIT -- causing OOMKills even at modest agent counts. - The fix reads GOMEMLIMIT via debug.SetMemoryLimit(-1) and uses that value to - select the cache tier, falling back to memory.TotalMemory() only when GOMEMLIMIT - is unset (non-containerised deployments). - -component: fleet-server diff --git a/changelog/fragments/1786058255-fix-cgroup-aware-cache-memory.yaml b/changelog/fragments/1786058255-fix-cgroup-aware-cache-memory.yaml new file mode 100644 index 0000000000..b655081ccc --- /dev/null +++ b/changelog/fragments/1786058255-fix-cgroup-aware-cache-memory.yaml @@ -0,0 +1,10 @@ +kind: bug-fix + +summary: Read cgroup memory limit for cache sizing when GOMEMLIMIT is not set + +description: > + containerMemoryMB() now falls back to the cgroup memory limit (v2 then v1) + before host RAM. This ensures fleet-server is correctly sized for the + container even in deployments that do not explicitly set GOMEMLIMIT. + +component: fleet-server diff --git a/internal/pkg/config/env_defaults.go b/internal/pkg/config/env_defaults.go index ceb3790ab0..ce7ec1a8a7 100644 --- a/internal/pkg/config/env_defaults.go +++ b/internal/pkg/config/env_defaults.go @@ -10,8 +10,10 @@ import ( "io" "io/fs" "math" + "os" "runtime" "runtime/debug" + "strconv" "strings" "time" @@ -314,7 +316,7 @@ func loadLimits(log *zerolog.Logger, agentLimit int) *envLimits { log.Info().Msgf("Using system limits for %d to %d agents for a configured value of %d agents", l.Agents.Min, l.Agents.Max, agentLimit) ramSize := memMB() if ramSize < l.RecommendedRAM { - log.Warn().Msgf("Detected %d MB of available memory, which is lower than the recommended amount (%d MB) for the configured agent limit", ramSize, l.RecommendedRAM) + log.Warn().Msgf("Detected %d MiB of available memory, which is lower than the recommended amount (%d MiB) for the configured agent limit", ramSize, l.RecommendedRAM) } return l } @@ -323,17 +325,64 @@ func loadLimits(log *zerolog.Logger, agentLimit int) *envLimits { return defaultEnvLimits() } -// containerMemoryMB returns available memory in MiB, preferring the GOMEMLIMIT -// runtime setting over host RAM so the ristretto cache is sized for the container, -// not the node. Falls back to memory.TotalMemory() when GOMEMLIMIT is unset. +// cgroupMemMB returns the cgroup memory limit in MiB. +// It is a var so that unit tests can replace it. +var cgroupMemMB func() (uint64, bool) = cgroupMemoryLimitMB + +// containerMemoryMB returns available memory in MiB using this priority order: +// 1. GOMEMLIMIT, if explicitly set +// 2. cgroup memory limit (v2, then v1), for containers without an explicit GOMEMLIMIT +// 3. host total RAM, for non-containerised deployments func containerMemoryMB() uint64 { limit := debug.SetMemoryLimit(-1) if limit > 0 && limit != math.MaxInt64 { return uint64(limit) / 1024 / 1024 } + if mb, ok := cgroupMemMB(); ok { + return mb + } return memory.TotalMemory() / 1024 / 1024 } +// cgroupMemoryLimitMB reads the container memory limit from cgroup files. +// It tries cgroup v2 first, then cgroup v1. This only checks the cgroup mount +// root, so it does not account for limits imposed by nested cgroups. Returns +// (0, false) when no applicable limit is found (unlimited, missing file, or +// parse error). +func cgroupMemoryLimitMB() (uint64, bool) { + if mb, ok := readCgroupMemoryFile("/sys/fs/cgroup/memory.max"); ok { + return mb, true + } + return readCgroupMemoryFile("/sys/fs/cgroup/memory/memory.limit_in_bytes") +} + +// readCgroupMemoryFile parses a cgroup memory limit file and returns the +// value in MiB. Returns (0, false) when the file doesn't exist, contains +// "max" (unlimited), or the value is at or above cgroup v1's page-aligned +// unlimited sentinel. +func readCgroupMemoryFile(path string) (uint64, bool) { + data, err := os.ReadFile(path) + if err != nil { + return 0, false + } + s := strings.TrimSpace(string(data)) + if s == "max" { + return 0, false + } + n, err := strconv.ParseUint(s, 10, 64) + if err != nil || n >= cgroupV1UnlimitedMemoryLimit() { + return 0, false + } + return n / 1024 / 1024, true +} + +// cgroupV1UnlimitedMemoryLimit is the value cgroup v1 exposes for an +// unrestricted memory limit: MaxInt64 rounded down to the system page size. +func cgroupV1UnlimitedMemoryLimit() uint64 { + pageSize := int64(os.Getpagesize()) + return uint64(math.MaxInt64 / pageSize * pageSize) //nolint:gosec // the page-aligned result is always non-negative +} + // memMB returns available memory in MiB. // It is a var so that unit tests can replace it. var memMB func() uint64 = containerMemoryMB diff --git a/internal/pkg/config/env_defaults_test.go b/internal/pkg/config/env_defaults_test.go index 3ec8794def..48e6eb68a7 100644 --- a/internal/pkg/config/env_defaults_test.go +++ b/internal/pkg/config/env_defaults_test.go @@ -8,8 +8,10 @@ import ( "io" "io/fs" "math" + "os" "reflect" "runtime/debug" + "strconv" "strings" "testing" @@ -91,7 +93,7 @@ func TestDefaultLimitsYAMLKeys(t *testing.T) { } // TestContainerMemoryMB verifies that containerMemoryMB prefers GOMEMLIMIT over -// host RAM so the ristretto cache is sized for the container, not the node. +// host RAM so fleet-server is correctly sized for the container, not the node. func TestContainerMemoryMB(t *testing.T) { t.Run("uses GOMEMLIMIT when set", func(t *testing.T) { const setLimit = int64(256 * 1024 * 1024) // 256 MiB @@ -102,11 +104,67 @@ func TestContainerMemoryMB(t *testing.T) { assert.Equal(t, uint64(256), got) }) - t.Run("falls back to host RAM when GOMEMLIMIT is unset", func(t *testing.T) { + t.Run("uses cgroup limit when GOMEMLIMIT is unset", func(t *testing.T) { prev := debug.SetMemoryLimit(math.MaxInt64) t.Cleanup(func() { debug.SetMemoryLimit(prev) }) + prevCgroup := cgroupMemMB + cgroupMemMB = func() (uint64, bool) { return 128, true } + t.Cleanup(func() { cgroupMemMB = prevCgroup }) + + got := containerMemoryMB() + assert.Equal(t, uint64(128), got) + }) + + t.Run("falls back to host RAM when GOMEMLIMIT and cgroup are both unset", func(t *testing.T) { + prev := debug.SetMemoryLimit(math.MaxInt64) + t.Cleanup(func() { debug.SetMemoryLimit(prev) }) + prevCgroup := cgroupMemMB + cgroupMemMB = func() (uint64, bool) { return 0, false } + t.Cleanup(func() { cgroupMemMB = prevCgroup }) got := containerMemoryMB() assert.Equal(t, memory.TotalMemory()/1024/1024, got) }) } + +func TestReadCgroupMemoryFile(t *testing.T) { + writeFile := func(t *testing.T, content string) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "cgroup-memory-*") + require.NoError(t, err) + _, err = f.WriteString(content) + require.NoError(t, err) + require.NoError(t, f.Close()) + return f.Name() + } + + t.Run("returns MiB for a valid byte limit", func(t *testing.T) { + path := writeFile(t, "134217728\n") // 128 MiB + mb, ok := readCgroupMemoryFile(path) + assert.True(t, ok) + assert.Equal(t, uint64(128), mb) + }) + + t.Run("returns false for 'max' (unlimited)", func(t *testing.T) { + path := writeFile(t, "max\n") + _, ok := readCgroupMemoryFile(path) + assert.False(t, ok) + }) + + t.Run("returns false for the cgroup v1 unlimited sentinel", func(t *testing.T) { + path := writeFile(t, strconv.FormatUint(cgroupV1UnlimitedMemoryLimit(), 10)+"\n") + _, ok := readCgroupMemoryFile(path) + assert.False(t, ok) + }) + + t.Run("returns false when file does not exist", func(t *testing.T) { + _, ok := readCgroupMemoryFile("/nonexistent/cgroup/memory.max") + assert.False(t, ok) + }) + + t.Run("returns false for invalid content", func(t *testing.T) { + path := writeFile(t, "not-a-number\n") + _, ok := readCgroupMemoryFile(path) + assert.False(t, ok) + }) +}