Collect DinD metrics from the container cgroup root - #4272
Closed
pentaoa wants to merge 1 commit into
Closed
Conversation
Contributor
|
@pentaoa we don't accept PRs if the implementation is not discussed first and the author that submits the PR is not actively using dstack. |
Author
|
Thanks for clarifying, and apologies for not discussing the implementation with you first. I’m not currently an active dstack user, so I understand these PRs don’t meet your contribution requirements. I’ll hold off on further submissions. |
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.
Summary
Fixes #4267.
Prefer the cgroup mount root for runner CPU and memory accounting when its
memory.currentexists. In a container cgroup namespace this includes the whole run, including nested containers under the siblingdocker/group afterstart-dockerdmoves the runner intodind/.When the mount exposes the host root, which has no
memory.current, retain the existing/proc/self/cgroupresolution. Resolve this on each collection, preserving support for process cgroup changes. A failure to inspect the root other than a missing file is reported rather than silently selecting a different accounting scope.The change follows the approach suggested in the issue. It does not enable controllers, modify the Docker startup script, change GPU collection, or restore cgroup v1 support. The namespace behavior addressed by #3402 is retained; the separate privileged Kubernetes limitation described in #4267 is not claimed as fixed.
Validation
Filesystem-fixture regression cases cover:
/;An additional test exercises
GetSystemMetricsusing root CPU, memory usage, and cache counters and checks the resulting working set.With the original path-selection behavior extracted unchanged, both DinD cases failed because they selected
/dindinstead of the mount root. After the fix:Changed Go files are gofmt-formatted and
git diff --checkpasses. The metrics test package also cross-compiles for Linux/amd64 (compile check only, not Linux execution).Validation ran on macOS using temporary proc/cgroup fixtures. I did not launch nested Docker containers, run GPU workloads, execute the full runner suite (existing collector-constructor tests require Linux procfs), or run golangci-lint locally.
AI-assisted implementation and tests; I inspected the DinD startup code, previous cgroup-path fix, and current collector, and ran the focused validation above.