fix: read cgroup memory limit for cache sizing when GOMEMLIMIT is not set - #7573
fix: read cgroup memory limit for cache sizing when GOMEMLIMIT is not set#7573ycombinator wants to merge 7 commits into
Conversation
|
This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
|
There was a problem hiding this comment.
Pull request overview
This PR improves memory-based sizing decisions in Fleet Server’s config defaults by making containerMemoryMB() cgroup-aware when GOMEMLIMIT is not explicitly set, so cache sizing better reflects the effective container limit rather than host RAM.
Changes:
- Extend
containerMemoryMB()to prefer:GOMEMLIMIT→ cgroup v2 → cgroup v1 → host RAM. - Add unit tests for cgroup limit file parsing and for
containerMemoryMB()selection behavior. - Add changelog fragments documenting the behavior change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/pkg/config/env_defaults.go | Adds cgroup-based memory limit detection and updates logging around detected memory. |
| internal/pkg/config/env_defaults_test.go | Adds tests for containerMemoryMB() and cgroup file parsing behavior. |
| changelog/fragments/1786058255-fix-cgroup-aware-cache-memory.yaml | Documents the new cgroup fallback tier when GOMEMLIMIT is unset. |
| changelog/fragments/1786038287-fix-cache-container-memory.yaml | Adds a changelog entry whose description should be updated to match the new cgroup-aware behavior. |
Suppressed comments (1)
internal/pkg/config/env_defaults.go:375
- To make containerMemoryMB() tests hermetic, expose the cgroup lookup via a package-level var (similar to memMB) so tests can temporarily override it.
// memMB returns available memory in MiB.
// It is a var so that unit tests can replace it.
var memMB func() uint64 = containerMemoryMB
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
internal/pkg/config/env_defaults_test.go:111
- This test assumes that clearing GOMEMLIMIT always forces a host-RAM fallback, but containerMemoryMB() now checks cgroup limits before host RAM. On Linux CI/container environments where a cgroup limit is set, this assertion can fail even though the production behavior is correct. Make the expected value conditional on whether a cgroup limit is detected so the test is environment-independent.
prev := debug.SetMemoryLimit(math.MaxInt64)
t.Cleanup(func() { debug.SetMemoryLimit(prev) })
got := containerMemoryMB()
assert.Equal(t, memory.TotalMemory()/1024/1024, got)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
TL;DRThe failing Buildkite unit-test step is inconclusive from the available log snippet: Remediation
Investigation detailsRoot CauseThe log available at
Because no failing test name, panic, or Evidence
Verification
Follow-up
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
… set Extend containerMemoryMB() to try the cgroup memory limit (v2 at /sys/fs/cgroup/memory.max, then v1 at /sys/fs/cgroup/memory/memory.limit_in_bytes) before falling back to host RAM. This means the ristretto cache is correctly sized even in deployments that do not explicitly set GOMEMLIMIT. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
containerMemoryMB is used for general memory-based sizing in fleet-server, not solely for the ristretto cache tier. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This fragment belongs to PR elastic#7568. Removing it here so it doesn't appear twice in the diff against main. It will re-enter via main once elastic#7568 merges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This pull request is now in conflicts. Could you fix it @ycombinator? 🙏 |
79ae421 to
c21d6ad
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
internal/pkg/config/env_defaults_test.go:112
- This subtest still asserts a direct fallback to host RAM when GOMEMLIMIT is unset, but containerMemoryMB() now checks cgroup limits before host RAM. That makes this assertion incorrect and potentially environment-dependent (it will fail when tests run in a container with a finite cgroup limit). Update the expectation to follow the same priority order as the production code.
assert.Equal(t, memory.TotalMemory()/1024/1024, got)
})
changelog/fragments/1786058255-fix-cgroup-aware-cache-memory.yaml:1
- This change is described and implemented as a fix for incorrect cache sizing that can lead to OOMKills; other similar fixes in this repo use
kind: bug-fix. Consider usingbug-fixhere so release notes categorize it correctly.
kind: enhancement
| return 0, false | ||
| } | ||
| n, err := strconv.ParseUint(s, 10, 64) | ||
| if err != nil || n >= math.MaxInt64 { |
| // cgroupMemoryLimitMB reads the container memory limit from cgroup files. | ||
| // It tries cgroup v2 first, then cgroup v1. Returns (0, false) when no | ||
| // applicable limit is found (unlimited, missing file, or parse error). | ||
| func cgroupMemoryLimitMB() (uint64, bool) { |
There was a problem hiding this comment.
Might want to add a comment here that this doesn't work with nested cgroups.
What is the problem this PR solves?
When
GOMEMLIMITis not explicitly set,containerMemoryMB()(introduced in #7568) falls back directly to host RAM. A container with a cgroup memory limit but no explicitGOMEMLIMITwould use the node's full RAM for memory-based sizing decisions in fleet-server.How does this PR solve the problem?
Extends
containerMemoryMB()with a cgroup tier between GOMEMLIMIT and host RAM:/sys/fs/cgroup/memory.max; skips if value is"max"(unlimited)/sys/fs/cgroup/memory/memory.limit_in_bytes; skips if value ≥MaxInt64(unlimited sentinel)On non-Linux hosts the cgroup files simply don't exist, so the reads fail silently and the fallback chain continues — no platform-specific build tags needed.
How to test this PR locally
TestReadCgroupMemoryFilecovers: valid byte-limit,"max"unlimited sentinel,MaxInt64unlimited sentinel (cgroup v1), missing file, and invalid content.Design Checklist
Checklist
./changelog/fragmentsusing the changelog toolRelated issues