fix(agent): scan only root mount in is_container() cgroup-v2 fallback (#58135) - #58141
fix(agent): scan only root mount in is_container() cgroup-v2 fallback (#58135)#58141Bartok9 wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Fixes is_container() cgroup-v2 fallback to scan only the root mount (/), not all mounts. Follows the related fix in #58135.
Looks Good
- Uses
Path('/').iterdir()to scan root mount only — avoids traversing all mounts any()with generator expression is efficient- No security or performance concerns
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Supplementary review: confirms LGTM. cgroup-v2 is_container() root mount scan is well-scoped. Prior COMMENT found no blocking issues. No additional concerns.
Reviewed by Hermes Agent
|
Thanks for the focused regression fix. The premise still holds on current main: The linked PR #58145 is a duplicate; its maintainer comment identifies this earlier PR as canonical. The affected production hunk remains unchanged on current main, so this should be mechanically salvageable. Automated hermes-sweeper review. |
Closes NousResearch#58135 On a cgroup-v2 host with Docker's containerd image store, is_container() false-positives whenever any container is running. The mountinfo fallback scanned the entire file for containerd/crio/kubepods substrings, but each running container contributes an overlay mount whose option string carries lowerdir=/var/lib/containerd/..., so a plain host was misclassified as a container. The result is cached per-process, making it depend on whether a container happened to be running at first call — which then flipped subprocess HOME and broke browser tool launches (Chrome not found). Fix: only inspect the root ('/') mount line. Inside a container the root mount is the runtime's overlay/snapshot and carries the marker; on a host the root is a real block device and container overlays live at non-root mount points. Adds a regression test reproducing the host-running-containers case.
c363137 to
588f551
Compare
|
Thanks @teknium1 — confirmed and salvaged.
Ready for re-review / merge when convenient. |
|
Independent confirmation from another affected host:
In this case the user-visible failure was GitHub CLI authentication rather than browser discovery: default This supports the PR's root-mount-only boundary: the containerd markers observed here are on non-root host mounts and should not classify Hermes itself as containerized. Thanks for the focused fix. |
|
Thanks @HaiderSultanArc — that's a great independent reproduction, and the Your host shows exactly the boundary this PR targets: Worth calling out that your Rebased and green on current |
Summary
is_container()no longer false-positives on a Linux host just because it happens to be running containers.Motivation
Closes #58135.
On a cgroup-v2 host with Docker's containerd image store (Docker Desktop default),
/proc/1/cgroupis just0::/, sois_container()falls back to scanning/proc/self/mountinfoforcontainerd/crio/kubepodssubstrings. But every running container contributes an overlay mount whose option string containslowerdir=/var/lib/containerd/..., so a plain host gets classified as "inside a container". The result is cached per-process, so the answer permanently depends on whether a container happened to be running at first call — which then flips subprocessHOMEviaget_subprocess_home()and breaks browser calls.Root cause
The marker only ever appears on non-root mount lines (other containers' overlay rootfs). Inside a real container the marker appears on the root (
/) mount, because root is the runtime overlay/snapshot.Fix
Only inspect the root (
/) mount line (mountinfo field 5). Host root is a real block device; container root carries the runtime marker. cgroup-v1 markers,/.dockerenv,/run/.containerenv, andKUBERNETES_SERVICE_HOSTdetection are unchanged.Verification
python3 -m pytest tests/test_hermes_constants.py -k container— 9 passed (incl. new regression).test_host_running_containers_not_false_positivefails on current main (reproduces the bug) and passes with the fix.