fix(docker): handle cgroup v2 "max" memory limit in container memory guard - #2154
Open
Battleplus wants to merge 2 commits into
Open
fix(docker): handle cgroup v2 "max" memory limit in container memory guard#2154Battleplus wants to merge 2 commits into
Battleplus wants to merge 2 commits into
Conversation
…guard Closes unclecode#2123 cgroup v2 reports an unset memory limit as the literal string "max". int("max") raised a ValueError that the bare except swallowed, so the guard silently fell back to the HOST's usage percent instead of the container's usage vs host total (the intent of the existing v1 > 1e18 "unlimited" branch, which was unreachable on v2). Normalize "max" to the same unlimited sentinel so memory_threshold_percent guards the container even when no -m / mem_limit is set. Adds dependency-free unit tests covering v2 "max", v2/v1 numeric limits, the v1 unlimited sentinel and the non-container host-percent fallback. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
test_utils_memory.py (added for unclecode#2123) was not matched by the existing 'test_security_*.py' glob, so the regression coverage would never execute in CI. Add a 'test_utils_*.py' run to the offline security job — the stub-import tests are pure-function, need no Docker/network, and run in milliseconds. 🤖 Generated with Codebuff Co-Authored-By: Codebuff <noreply@codebuff.com>
Author
|
Follow-up commit: the new test file |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2123
Problem
get_container_memory_percent()indeploy/docker/utils.pyreads the cgroup memory limit withint(limit_path.read_text()). On cgroup v2 with no container memory limit set (no-m/mem_limit),/sys/fs/cgroup/memory.maxcontains the literal stringmax, soint("max")raisesValueError. The bareexceptswallows it and the function returnspsutil.virtual_memory().percent— the host's usage percentage.The existing
if limit > 1e18"unlimited" branch (whose comment even names the v2"max"form) was unreachable on v2 because the exception fired two lines earlier.Effects:
memory_threshold_percentno longer guards the container; on a 16 GB host the guard effectively watched host-wide usage (~15.2 GB at the 95% default) while the container itself could OOM.except, no log), so a defeated guard looks like a working one.Fix
Normalize the cgroup v2
maxsentinel to the same "unlimited" value the v1> 1e18branch already handled, so the guard reports container usage against host total when no limit is set — the behavior the docstring and the existing branch already intended. Also.strip()s the raw file contents.Tests
Adds
deploy/docker/tests/test_utils_memory.py— dependency-free unit tests (stub the module-level third-party imports so they run without the Docker dev env) covering:max(unlimited) → usage/host-total (regression for this issue)> 1e18unlimited sentinel → usage/host-total (existing intent preserved)Files changed
deploy/docker/utils.py— normalize the cgroup v2maxsentinel to the same "unlimited" value the v1> 1e18branch already handled.deploy/docker/tests/test_utils_memory.py— new dependency-free unit tests for the memory guard..github/workflows/security.yml— run the docker utility unit tests in the security workflow. The tests use atest_utils_*.pyprefix, which the existingtest_security_*.pyglob did not match, so they would never have run in CI without this.