fix: size ristretto cache from GOMEMLIMIT instead of host RAM - #7568
Conversation
|
This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
|
lorienhu
left a comment
There was a problem hiding this comment.
Just a suggestion but since GOMEMLIMIT isn't currently automatically set, might be worth looking at reading it from cgroup limits similar to https://github.com/KimMachineGun/automemlimit.
|
Tick the box to add this pull request to the merge queue (same as
|
It's a good idea. Let's do it in a follow up PR since this PR here addresses an immediate need in Serverless Production. |
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.go:333
- containerMemoryMB() ignores a GOMEMLIMIT of 0 (valid via debug.SetMemoryLimit(0)) because the condition requires limit > 0; in that case it will incorrectly fall back to host RAM and may oversize the cache. Consider treating any non-MaxInt64 limit (including 0) as "set".
limit := debug.SetMemoryLimit(-1)
if limit > 0 && limit != math.MaxInt64 {
return uint64(limit) / 1024 / 1024
}
memEnvLimits() called memory.TotalMemory() which returns host-node RAM, not the container cgroup limit. On a K8s node with >=16 GB RAM this sized the ristretto MaxCost at 256-512 MB -- 2-4x the pod's GOMEMLIMIT -- causing OOMKills at modest agent counts. Fix: read the current GOMEMLIMIT via debug.SetMemoryLimit(-1) and use it to select the cache tier. Falls back to memory.TotalMemory() when GOMEMLIMIT is unset (math.MaxInt64), preserving behaviour for non-containerised deployments.
The agent-count path in loadLimits was still calling memory.TotalMemory() directly, so the low-RAM warning would fire against host node RAM rather than the container's available memory. Switch to memMB() to stay consistent with memEnvLimits(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
be781d3 to
4d6de64
Compare
|
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 comment has been minimized.
This comment has been minimized.
TL;DRThe failing Remediation
Investigation detailsRoot CauseCurrent evidence is insufficient to attribute this to PR code changes. The supplied job log starts during cleanup and shows only successful teardown plus a generic non-zero exit. Because Evidence
No earlier error line is present in the artifact. Verification
Follow-upAfter full logs are available, we can pinpoint whether the failure is:
What is this? | From workflow: PR Buildkite Detective Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not. |
|
@Mergifyio backport 9.5 9.4 8.19 |
✅ Backports have been createdDetails
|
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>
…#7577) * fix: size ristretto cache from GOMEMLIMIT instead of host RAM memEnvLimits() called memory.TotalMemory() which returns host-node RAM, not the container cgroup limit. On a K8s node with >=16 GB RAM this sized the ristretto MaxCost at 256-512 MB -- 2-4x the pod's GOMEMLIMIT -- causing OOMKills at modest agent counts. Fix: read the current GOMEMLIMIT via debug.SetMemoryLimit(-1) and use it to select the cache tier. Falls back to memory.TotalMemory() when GOMEMLIMIT is unset (math.MaxInt64), preserving behaviour for non-containerised deployments. * test: add TestContainerMemoryMB covering GOMEMLIMIT and host-RAM paths * changelog: add fragment for ristretto cache container-memory fix * fix: use memMB() in loadLimits to respect GOMEMLIMIT for RAM warning The agent-count path in loadLimits was still calling memory.TotalMemory() directly, so the low-RAM warning would fire against host node RAM rather than the container's available memory. Switch to memMB() to stay consistent with memEnvLimits(). --------- (cherry picked from commit 7626dc8) Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#7578) * fix: size ristretto cache from GOMEMLIMIT instead of host RAM memEnvLimits() called memory.TotalMemory() which returns host-node RAM, not the container cgroup limit. On a K8s node with >=16 GB RAM this sized the ristretto MaxCost at 256-512 MB -- 2-4x the pod's GOMEMLIMIT -- causing OOMKills at modest agent counts. Fix: read the current GOMEMLIMIT via debug.SetMemoryLimit(-1) and use it to select the cache tier. Falls back to memory.TotalMemory() when GOMEMLIMIT is unset (math.MaxInt64), preserving behaviour for non-containerised deployments. * test: add TestContainerMemoryMB covering GOMEMLIMIT and host-RAM paths * changelog: add fragment for ristretto cache container-memory fix * fix: use memMB() in loadLimits to respect GOMEMLIMIT for RAM warning The agent-count path in loadLimits was still calling memory.TotalMemory() directly, so the low-RAM warning would fire against host node RAM rather than the container's available memory. Switch to memMB() to stay consistent with memEnvLimits(). --------- (cherry picked from commit 7626dc8) Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…#7579) * fix: size ristretto cache from GOMEMLIMIT instead of host RAM memEnvLimits() called memory.TotalMemory() which returns host-node RAM, not the container cgroup limit. On a K8s node with >=16 GB RAM this sized the ristretto MaxCost at 256-512 MB -- 2-4x the pod's GOMEMLIMIT -- causing OOMKills at modest agent counts. Fix: read the current GOMEMLIMIT via debug.SetMemoryLimit(-1) and use it to select the cache tier. Falls back to memory.TotalMemory() when GOMEMLIMIT is unset (math.MaxInt64), preserving behaviour for non-containerised deployments. * test: add TestContainerMemoryMB covering GOMEMLIMIT and host-RAM paths * changelog: add fragment for ristretto cache container-memory fix * fix: use memMB() in loadLimits to respect GOMEMLIMIT for RAM warning The agent-count path in loadLimits was still calling memory.TotalMemory() directly, so the low-RAM warning would fire against host node RAM rather than the container's available memory. Switch to memMB() to stay consistent with memEnvLimits(). --------- (cherry picked from commit 7626dc8) Co-authored-by: Shaunak Kashyap <ycombinator@gmail.com> 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>
… set (#7573) * fix: read cgroup memory limit for cache sizing when GOMEMLIMIT is 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> * chore: remove ristretto-specific wording from comments and changelog 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> * chore: drop #7568 changelog fragment from this branch This fragment belongs to PR #7568. Removing it here so it doesn't appear twice in the diff against main. It will re-enter via main once #7568 merges. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
What is the problem this PR solves?
memEnvLimits()ininternal/pkg/config/env_defaults.gocallsmemory.TotalMemory()to select the ristretto cache tier.memory.TotalMemory()returns the host node's total physical RAM, not the container's cgroup memory limit. On a Kubernetes node with ≥16 GB of RAM, this selects aMaxCostof 256–512 MB — 2–4× the pod's GOMEMLIMIT (128 MB at the 256 M default pod limit). The cache is allowed to grow past the GOMEMLIMIT, triggering OOMKills even when live application heap is within budget.This was the primary driver of the OOMKills observed for a high-volume serverless project in elastic/ingest-dev#8991.
How does this PR solve the problem?
Introduces
containerMemoryMB(), which reads the current GOMEMLIMIT viadebug.SetMemoryLimit(-1)and converts it to MiB. When GOMEMLIMIT is set (i.e. ≠math.MaxInt64), that value drives cache tier selection — so a pod with a 256 M limit and GOMEMLIMIT = 128 M will select a cache sized for 128 MB of available memory, not 16+ GB of node RAM.When GOMEMLIMIT is unset (
math.MaxInt64), the function falls back tomemory.TotalMemory(), preserving the existing behaviour for non-containerised deployments.memMBremains avarpointing tocontainerMemoryMB, so existing tests that stubmemMBare unaffected.How to test this PR locally
Run the config package tests:
TestContainerMemoryMBcovers both the GOMEMLIMIT path (sets a 256 MiB limit and asserts the function returns 256) and the fallback path (clears the limit and asserts the function returns host RAM in MiB).To verify the end-to-end effect: start fleet-server with
GOMEMLIMIT=128MiB(or viaserver.runtime.memory_limit) and observe that the loggedrecommended_mbvalue is ≤ 128, not 256–512.Design Checklist
Checklist
./changelog/fragmentsusing the changelog toolRelated issues