Skip to content

fix: size ristretto cache from GOMEMLIMIT instead of host RAM - #7568

Merged
ycombinator merged 4 commits into
elastic:mainfrom
ycombinator:fix-cache-container-memory
Aug 7, 2026
Merged

fix: size ristretto cache from GOMEMLIMIT instead of host RAM#7568
ycombinator merged 4 commits into
elastic:mainfrom
ycombinator:fix-cache-container-memory

Conversation

@ycombinator

Copy link
Copy Markdown
Contributor

What is the problem this PR solves?

memEnvLimits() in internal/pkg/config/env_defaults.go calls memory.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 a MaxCost of 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 via debug.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 to memory.TotalMemory(), preserving the existing behaviour for non-containerised deployments.

memMB remains a var pointing to containerMemoryMB, so existing tests that stub memMB are unaffected.

How to test this PR locally

Run the config package tests:

go test ./internal/pkg/config/... -run TestContainerMemoryMB -v

TestContainerMemoryMB covers 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 via server.runtime.memory_limit) and observe that the logged recommended_mb value is ≤ 128, not 256–512.

Design Checklist

  • I have ensured my design is stateless and will work when multiple fleet-server instances are behind a load balancer.
  • I have or intend to scale test my changes, ensuring it will work reliably with 100K+ agents connected.
  • I have included fail safe mechanisms to limit the load on fleet-server: rate limiting, circuit breakers, caching, load shedding, etc.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool

Related issues

  • Closes elastic/ingest-dev#8991

@ycombinator
ycombinator requested a review from a team as a code owner August 6, 2026 17:47
@mergify

mergify Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@ycombinator
ycombinator requested review from belimawr and removed request for samuelvl August 6, 2026 17:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

lorienhu
lorienhu previously approved these changes Aug 6, 2026

@lorienhu lorienhu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mergify

mergify Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

Comment thread internal/pkg/config/env_defaults.go Outdated
Copilot AI review requested due to automatic review settings August 6, 2026 23:14
@ycombinator

Copy link
Copy Markdown
Contributor Author

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.

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
	}

@ycombinator
ycombinator requested a review from lorienhu August 6, 2026 23:17
ycombinator and others added 4 commits August 6, 2026 16:17
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>
@ycombinator

Copy link
Copy Markdown
Contributor Author

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.

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.

#7573

@ycombinator
ycombinator enabled auto-merge (squash) August 6, 2026 23:21
@ycombinator ycombinator added Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team backport-active-all Automated backport with mergify to all the active branches labels Aug 6, 2026
ycombinator added a commit to ycombinator/fleet-server that referenced this pull request Aug 6, 2026
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>
@github-actions

This comment has been minimized.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

TL;DR

The failing :gcloud: Cloud e2e FIPS Test did not expose the actual failing command in the provided artifact; the log only contains teardown (terraform destroy) and final exit status 1.
Immediate action: re-run this Buildkite step with full command output retained for the first non-zero command (likely in mage ... test:cloudE2EUp or mage test:cloudE2ERun).

Remediation

  • Re-run Buildkite build 16135 and ensure the Cloud e2e FIPS job uploads the full step log (not only teardown tail).
  • In .buildkite/scripts/cloud_e2e_test.sh, the failing command is before cleanup (line 30 or line 48); capture and annotate that exact command stderr/stdout in the step output.
  • Once full logs are available, classify as test vs infra and either fix code/test or file/associate a flaky-test issue.
Investigation details

Root Cause

Current 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 .buildkite/scripts/cloud_e2e_test.sh has set -euo pipefail and trap cleanup EXIT (.buildkite/scripts/cloud_e2e_test.sh:3, .buildkite/scripts/cloud_e2e_test.sh:28), any earlier failure exits immediately and then runs cleanup, which is exactly what this log shows.

Evidence

  • Build: https://buildkite.com/elastic/fleet-server/builds/16135
  • Job/step: :gcloud: Cloud e2e FIPS Test
  • Script flow references:
    • .buildkite/scripts/cloud_e2e_test.sh:30 runs USER=fleetserverci mage docker:cover docker:customAgentImage docker:push test:cloudE2EUp
    • .buildkite/scripts/cloud_e2e_test.sh:48 runs mage test:cloudE2ERun
  • Provided log excerpt (/tmp/gh-aw/buildkite-logs/fleet-server-gcloud-cloud-e2e-fips-test.txt:93-138):
Plan: 0 to add, 0 to change, 1 to destroy.
...
ec_deployment.deployment: Destruction complete after 1m7s
Destroy complete! Resources: 1 destroyed.
🚨 Error: The command exited with status 1
user command error: exit status 1

No earlier error line is present in the artifact.

Verification

  • Not run: no reproducible failing command was present in the supplied log artifact.
  • Checked for matching flaky-test issues in elastic/fleet-server; none found for Cloud e2e FIPS failure signature.

Follow-up

After full logs are available, we can pinpoint whether the failure is:

  1. infrastructure/provisioning (test:cloudE2EUp), or
  2. test/runtime (test:cloudE2ERun)
    and then provide a concrete code/config fix.

What is this? | From workflow: PR Buildkite Detective

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

@ycombinator
ycombinator merged commit 7626dc8 into elastic:main Aug 7, 2026
14 checks passed
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

@Mergifyio backport 9.5 9.4 8.19

@mergify

mergify Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

ycombinator added a commit to ycombinator/fleet-server that referenced this pull request Aug 7, 2026
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>
@ycombinator
ycombinator deleted the fix-cache-container-memory branch August 7, 2026 14:57
ycombinator added a commit that referenced this pull request Aug 7, 2026
…#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>
ycombinator added a commit that referenced this pull request Aug 7, 2026
…#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>
ycombinator added a commit that referenced this pull request Aug 7, 2026
…#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-active-all Automated backport with mergify to all the active branches Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants