Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory limit check bytes to GB conversion #13838

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

1pkg
Copy link
Member

@1pkg 1pkg commented Aug 6, 2024

Motivation/summary

This PR closed the issue #13837, by fixing the bytes to GB conversion in the fallback memory check.

Checklist

Update CHANGELOG.asciidoc

  • Documentation has been updated

For functional changes, consider:

  • Is it observable through the addition of either logging or metrics?
  • Is its use being published in telemetry to enable product improvement?
  • Have system tests been added to avoid regression?

How to test these changes

The easiest way to test this change is to create a custom cgroup with a memory limit that is larger than the total system memory, then start apm-server binary with the created cgroup. Observer APM Server logs for "cgroup memory limit exceed available memory, falling back to the total system memory".

cgcreate  -g memory:apm_test
cgget -r memory.limit_in_bytes=10737418240 /apm_test
cgexec -g memory:apm_test ./apm-server run

Related issues

Changelog

==== Bug fixes

  • Fix memory limit fallback check bytes to GB conversion {pull}13838[13838]

@1pkg 1pkg added the backport-8.15 Automated backport with mergify label Aug 6, 2024
@1pkg 1pkg self-assigned this Aug 6, 2024
@1pkg 1pkg requested a review from a team as a code owner August 6, 2024 19:12
internal/beater/beater.go Outdated Show resolved Hide resolved
kruskall
kruskall previously approved these changes Aug 6, 2024
Copy link
Member

@kruskall kruskall left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix! 🙇

marclop
marclop previously approved these changes Aug 7, 2024
Copy link
Contributor

@marclop marclop left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and tidying the code!

Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

nice catch!
non-blocker: is this something that can be covered by tests? Otherwise, the PR test instructions are clear enough should be fine.

internal/beater/beater.go Outdated Show resolved Hide resolved
changelogs/8.15.asciidoc Outdated Show resolved Hide resolved
@1pkg 1pkg dismissed stale reviews from marclop and kruskall via c914b9a August 13, 2024 00:29
@1pkg 1pkg force-pushed the fix-memory-limit-fallback-check branch 2 times, most recently from b9b884f to d7b2f12 Compare August 13, 2024 00:38
@1pkg 1pkg force-pushed the fix-memory-limit-fallback-check branch from d7b2f12 to d05e2de Compare August 13, 2024 00:39
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

changes lgtm. Thanks for the tests.

Could you remove the changelog? It will be added back later in the release process. Sorry if I didn't express that clearly in the earlier comment.

carsonip
carsonip previously approved these changes Aug 13, 2024
Copy link
Member

@carsonip carsonip left a comment

Choose a reason for hiding this comment

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

I'll approve it first so you can merge it right away after removing the changelog (if github allows you to do that)

@1pkg 1pkg enabled auto-merge (squash) August 13, 2024 17:52
@1pkg 1pkg merged commit 6ad5821 into main Aug 13, 2024
10 checks passed
@1pkg 1pkg deleted the fix-memory-limit-fallback-check branch August 13, 2024 17:54
mergify bot pushed a commit that referenced this pull request Aug 13, 2024
Fixes the memory limit auto check conversion in beater runner, when cgroups memory limit was scaled to gigabytes while the total memory limit stayed in bytes. Additional tests coverage is added to better validate memory conversion logic.

(cherry picked from commit 6ad5821)
mergify bot added a commit that referenced this pull request Aug 13, 2024
…13876)

Fixes the memory limit auto check conversion in beater runner, when cgroups memory limit was scaled to gigabytes while the total memory limit stayed in bytes. Additional tests coverage is added to better validate memory conversion logic.

(cherry picked from commit 6ad5821)

Co-authored-by: Kostiantyn Masliuk <[email protected]>
@inge4pres inge4pres mentioned this pull request Sep 2, 2024
8 tasks
@kruskall
Copy link
Member

kruskall commented Sep 2, 2024

Tested with systemd-run --scope -p MemoryMax=200G --use ./apm-server -e

{"log.level":"info","log.logger":"beater","log.origin":{"function":"github.com/elastic/apm-server/internal/beater.processMemoryLimit","file.name":"beater/beater.go","file.line":1027},"message":"cgroup memory limit exceed available memory, falling back to the total system memory","service.name":"apm-server","ecs.version":"1.6.0"}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.15 Automated backport with mergify test-plan-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants