fix(linux-introspection): report cgroup limits on v1, v2 and hybrid hosts, under both cgroup namespace modes - #637
fix(linux-introspection): report cgroup limits on v1, v2 and hybrid hosts, under both cgroup namespace modes#637bburda wants to merge 3 commits into
Conversation
…pace modes read_cgroup_path() matched only the unified "0::<path>" line, so on a pure cgroup v1 host no line matched and read_cgroup_info() gave up. Parse the "<id>:<controllers>:<path>" form as well, read memory.limit_in_bytes and cpu.cfs_quota_us / cpu.cfs_period_us for the legacy controllers, and tolerate the hybrid layout that mounts both hierarchies. The interface files were looked for only under the mount point joined with the reported path, which holds under cgroupns=host. Under cgroupns=private the process's own cgroup is the mount point itself and the reported path resolves nowhere. Both locations are tried now, most specific first, and merged by how informative the answer is, so the root of a v1 hierarchy - which always reads as unlimited - cannot mask a limit set on the container's cgroup below it. A limit that could not be read was reported the same way as no limit at all, which is indistinguishable from an unconstrained container. Each limit now carries a LimitState (limited, unlimited, unreadable, unavailable) alongside its value, surfaced as memory_limit_state and cpu_quota_state. The numeric fields keep their meaning, so existing clients are unaffected. Under cgroupns=private the cgroup path is the namespace root and carries no container id, so the endpoint answered 404 even though the limits were readable. Containerization is now decided by the id when the path has one and by the runtime's own markers otherwise - /.dockerenv, /run/.containerenv, or an overlay root filesystem - and the container is reported with an empty id. The CPU quota is the CFS bandwidth limit only and does not reflect --cpuset-cpus; that is recorded as a contract at the field and in the docs. Also in ros2_medkit_linux_introspection: read_file_contents() built its string from a pair of istreambuf_iterators, which makes GCC report a null dereference inside <streambuf>, and the node FQN was assembled with a chain of temporary strings. The Docker introspection harness could not build or start: cgroupns is not a Compose key, the build context pointed one level below the repository root, the image copied a cmake/ directory that does not exist while omitting four packages from the dependency closure, rosdep ran after the apt lists had been removed and resolved test-only dependencies for an image built with BUILD_TESTING=OFF, --symlink-install left the runtime stage with dangling links, and the suites picked the gateway's own node, whose process is not in the PID cache. The container compose file now runs the same image under both cgroup namespace modes so the limits can be compared against the ones Docker was given.
There was a problem hiding this comment.
Pull request overview
This PR extends Linux container introspection across cgroup v1/v2, hybrid layouts, and host/private namespaces, with explicit limit states, updated tests/docs, and repaired Docker harnesses.
Changes:
- Added namespace-aware cgroup parsing and container detection.
- Added limited, unlimited, unreadable, and unavailable limit states.
- Updated serialization, documentation, tests, and proc-reader handling.
Review findings:
- Critical (2 votes): Empty
container_idaggregation can merge multiple private-namespace containers. - Moderate (3 votes): Unlimited fallback can mask an unreadable specific cgroup.
- Moderate (4 votes):
cpu.maxaccepts trailing tokens that should be rejected. - Moderate (2 votes): Non-
ENOENTopen failures are reported as unavailable. - Critical (3 votes): Container IDs are not searched across all hierarchy paths.
- Moderate (3 votes): v1 mount paths are assumed rather than derived from mount information.
- Critical (3 votes): Container markers are checked in the gateway root instead of the target process root.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/ros2_medkit_integration_tests/test/docker/introspection/test_container_introspection.py |
Container endpoint integration tests |
src/ros2_medkit_integration_tests/test/docker/introspection/test_container_cgroup_layouts.py |
Host/private namespace integration tests |
src/ros2_medkit_integration_tests/test/docker/introspection/run_docker_tests.sh |
Runs both container configurations |
src/ros2_medkit_integration_tests/test/docker/introspection/Dockerfile.systemd |
Systemd test image build |
src/ros2_medkit_integration_tests/test/docker/introspection/Dockerfile.container |
Container test image build |
src/ros2_medkit_integration_tests/test/docker/introspection/docker-compose.systemd.yml |
Systemd Docker configuration |
src/ros2_medkit_integration_tests/test/docker/introspection/docker-compose.container.yml |
Private and host cgroup services |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/test/test_container_plugin.cpp |
Limit-state JSON tests |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/test/test_cgroup_reader.cpp |
Cgroup layout and parsing tests |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/src/linux_utils/proc_reader.cpp |
File-reading and FQN fixes |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/src/linux_utils/cgroup_reader.cpp |
Cgroup parsing, detection, and limit-state handling |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/src/container_plugin.cpp |
Container handling and aggregation |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/README.md |
Plugin behavior documentation |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/include/ros2_medkit_linux_introspection/container_utils.hpp |
Limit-state serialization |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/include/ros2_medkit_linux_introspection/cgroup_reader.hpp |
Cgroup data models and APIs |
src/ros2_medkit_discovery_plugins/ros2_medkit_linux_introspection/design/index.rst |
Design documentation |
docs/tutorials/linux-introspection.rst |
Linux introspection documentation |
docs/api/rest.rst |
REST API documentation |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Deduplicate by container_id. Under cgroupns=private every app reports an | ||
| // empty id and they all collapse into one entry, which is what they are: | ||
| // the single container whose id the namespace hides. | ||
| std::map<std::string, nlohmann::json> containers; |
| return 2; | ||
| case LimitState::kUnreadable: | ||
| return 1; | ||
| case LimitState::kUnavailable: |
| if (!(ss >> quota_text) || !(ss >> period_text)) { | ||
| return {CgroupLimit<int64_t>::unreadable(), std::nullopt}; | ||
| } |
| if (!f.is_open()) { | ||
| return std::nullopt; |
| std::string select_reported_path(const CgroupPaths & paths) { | ||
| if (!paths.unified.empty() && paths.unified != "/") { | ||
| return paths.unified; | ||
| } | ||
| for (const auto * preferred : {"memory", "cpu", "cpuacct"}) { | ||
| if (auto path = controller_path(paths, preferred); path && *path != "/") { | ||
| return *path; |
| std::vector<std::string> v1_mounts(const std::string & root, const std::string & controller) { | ||
| std::vector<std::string> mounts{root + "/sys/fs/cgroup/" + controller}; | ||
| if (controller == "cpu") { | ||
| // Distributions co-mount cpu with cpuacct and leave "cpu" as a symlink to | ||
| // that directory, which not every image preserves. | ||
| mounts.push_back(root + "/sys/fs/cgroup/cpu,cpuacct"); | ||
| } | ||
| return mounts; |
| ContainerMarker detect_container_marker(pid_t pid, const std::string & root) { | ||
| std::error_code ec; | ||
| if (std::filesystem::exists(root + "/.dockerenv", ec)) { | ||
| return {true, "docker"}; | ||
| } | ||
| if (std::filesystem::exists(root + "/run/.containerenv", ec)) { | ||
| return {true, "podman"}; | ||
| } | ||
| if (has_overlay_root(pid, root)) { |
…ry cgroup layout in ctest The container marker files were read from the reader's own root while the mount check was per process, so a gateway running in a container reported every process it could see as containerized, with a fabricated runtime. The markers now go through /proc/<pid>/root, systemd-nspawn is recognised, and an overlay root only counts together with a cgroup path of "/", which a process outside a cgroup namespace never reports. Stacked mounts at "/" take the last entry. The component endpoint grouped by container id alone. An empty id is not an identity: two apps in different containers merged into one entry and one set of limits was dropped. Grouping falls back to the process's mount namespace. test_combined_introspection asserted that the container endpoint answers 404. That only held while a private cgroup namespace defeated detection. The tests run inside a container, where the gateway really is containerized and 200 is the right answer, so it now points the plugin at a synthetic tree describing a host process and no longer depends on where it runs. Every cgroup layout is now covered by a ctest-registered launch test that drives the real gateway over the real HTTP endpoint, with proc_root pointing at a synthetic tree: cgroup v1 in both namespace layouts, a v1 hierarchy root that must not mask the container's own limit, the hybrid layout, unlimited, an unparsable limit file, a plain host process, and component grouping across two containers. A unified-only kernel has no v1 controllers to mount, so this is the only way those layouts can be reached at all. Also corrected: rest.rst claimed cpu_period_us appears only when a limit was read, while it is reported whenever the CPU limit was read at all, and the package README documented a plugin name and config keys that do not exist, with a proc_root value that resolves to /proc/proc/<pid>.
An unlimited reading from a fallback location could overwrite an unreadable one from the process's own cgroup, so a limit nobody managed to parse was reported as no limit at all. On cgroup v1 that is reachable on any host, because the root of a v1 hierarchy always reads as unlimited. Candidates are visited most specific first and the first one that has a file now decides, which is both simpler than ranking states and free of the masking. A limit file that exists but will not open was reported as unavailable, the state documented as "no file exists". It is a failed read and is reported as unreadable. cpu.max holds exactly two fields, but only the first two were read, so "50000 100000 garbage" passed as a valid quota while the memory reader rejects trailing input. A third token now makes the file unreadable. The container id was only looked for on the unified, memory, cpu and cpuacct paths. A host that names the container on another hierarchy, say pids, was treated as bare metal. Every reported path is searched for an id now. cgroup v1 does not require a controller to be mounted at a directory named after it, and a mount can expose only part of the hierarchy. The mount points are read from the process's own mountinfo, and the exposed subtree is stripped off the front of the cgroup path. The conventional locations are still tried first, so a gateway that does not share the target's mount namespace keeps working and the derived paths simply do not match.
|
|
||
| auto & cid = cgroup_info->container_id; | ||
| auto cid = cgroup_info->container_id; | ||
| if (cid.empty()) { |
There was a problem hiding this comment.
The cid.empty() -> mnt-<ns> / unidentified-N fallback still has no automated coverage: every ctest layout node carries a 64-hex id, and cgroup_fixtures.write_process never creates /proc/<pid>/ns/mnt, so mount_namespace_of returns empty and this branch only runs in the manual docker harness. A regression that merged all hidden-id containers into one entry would ship green. Suggest a ctest node with a hidden id plus a ns/mnt symlink, asserting different-ns nodes stay separate and same-ns merge.
Summary
cgroup_readerreported container limits for only one of the three common cgrouplayouts. This makes it work on the unified hierarchy (v2), the legacy one (v1) and
hybrid hosts, under both
--cgroupns=hostand--cgroupns=private, and stops a failedread from looking like a container with no limits.
cgroup v1 is parsed.
read_cgroup_path()matched only0::<path>, so on a v1 hostno line matched and the read gave up. The
<id>:<controllers>:<path>form is parsed now,with
memory.limit_in_bytesandcpu.cfs_quota_us/cpu.cfs_period_usfor the legacycontrollers. Hosts that mount both hierarchies work too.
Both namespace layouts are searched. The path was built only as mount point plus
reported path. That is right for
cgroupns=host. Undercgroupns=privatethe process'sown cgroup is the mount point itself, and the reported path leads nowhere. Both places
are tried now. The more specific one is preferred, and a location that reports no limit
does not stop the search. This matters on v1: the root of a v1 hierarchy always reads as
unlimited, so it would otherwise hide the limit set on the container's own cgroup.
"No limit" and "could not read" are now different answers. Each limit carries a state
next to its value:
limited,unlimited,unreadableorunavailable. On the wirethese are
memory_limit_stateandcpu_quota_state, always present.A container is recognised when the namespace hides its ID. Under
cgroupns=privatethe cgroup path is the namespace root and has no container ID, so the endpoint answered
404 even though the limits were readable. Detection now falls back to the markers a
runtime leaves behind, read through the inspected process's own root
(
/proc/<pid>/root):/.dockerenv,/run/.containerenv,/run/systemd/container, or anoverlay root filesystem together with a cgroup path of
/. Reading them through theprocess's own root matters, otherwise a gateway that is itself in a container would report
every process it can see as containerized. An overlay root alone is not enough either,
because some distributions boot that way.
cpu_quota_usis the CFS bandwidth limit. It does not cover--cpuset-cpus, which onlysched_getaffinity()can see. That is written down at the field and in the docs, notimplemented.
Behaviour changes for existing clients
cgroupns=private, and on hybrid hosts, it went from 404 to 200.container_idis empty in that case, because there is no ID to report.
memory_limit_stateandcpu_quota_state.container_idalone. An empty ID is not anidentity, so two apps in different containers were merged into one entry and one set of
limits was dropped. Grouping now falls back to the process's mount namespace.
Known limits of the detection
A runtime that leaves none of those markers and does not use an overlay root, for example
containerd or CRI-O on a btrfs or ZFS snapshotter, is not detected under a private cgroup
namespace. Its apps are reported as non-containerized. This is in the docs.
Issue
Type
Testing
New integration test, registered with ctest so CI runs it.
test/features/test_container_cgroup_layouts.test.pystarts a real gateway, loads thereal plugin and asserts over the real HTTP endpoint. The plugin's
proc_rootpoints at asynthetic tree, so one gateway covers seven layouts, one per demo node:
unlimited, no numbers, period still presentmemory_limit_stateisunreadable, no numberx-medkit-not-containerizedPlus the component endpoint: three apps in one container are one entry with three
node_ids, a second container stays a separate entry, and the host process appears inneither.
This is the only way to reach v1 and hybrid at all. A unified-only kernel has no v1
controllers to mount, and every supported distribution boots unified. I checked on this
machine:
/proc/cgroupsputs every controller on hierarchy 0, and mounting the v1 memorycontroller fails with
wrong fs typeeven in a privileged container.Live run on real containers, cgroup v2, both namespace modes. The container compose
file now starts the same image twice, once with
cgroup: privateand once withcgroup: host, both withmem_limit: 512mandcpus: 1.0:/proc/self/cgroup0::/0::/docker/c46f96b3...c46f96b3...536870912 bytes is exactly the configured 512 MiB and 100000/100000 us is exactly 1.0 CPU,
under both modes.
pytest test_container_introspection.py test_container_cgroup_layouts.pygives 22 passed. Note this Docker harness is not run by any workflow, so these numbers come
from a manual run, unlike the ctest test above.
Unit tests:
colcon test --packages-select ros2_medkit_linux_introspectiongives 84tests, 0 failures. They cover the layouts above plus malformed contents: empty file,
cpu.maxwithout a period, zero period, a value past the type range, a negative bytecount, trailing garbage, and both sides of the v1 unlimited sentinel.
The unit suite was mutation-checked. Reverting to the joined path only fails 2 tests,
dropping v1 parsing fails 11, and collapsing
unreadableintounavailablefails 10.That shows the tests tell those behaviours apart. It does not show the synthetic trees
match a real v1 host, which is why the ctest test above matters more.
Full integration suite for
ros2_medkit_integration_testsruns clean.test_combined_introspectionasserted a 404 that only held because container detectionwas broken under a private namespace. CI jobs run inside containers, where the gateway
really is containerized, so that test now points the container plugin at a synthetic host
tree and no longer depends on where it runs.
Zero build warnings under
-Wall -Wextra -Wpedantic -Wshadow -Wconversion, andrun-clang-tidyis clean on every changed source.Not covered by a real system
The v1 and hybrid layouts, the Podman and overlay markers, and every state other than
limitedare covered by the ctest test against a synthetic tree, not against a kernelthat produced those files. The v2 layouts under both namespace modes are the only ones
verified against real containers.
Docker test harness
The Docker introspection harness could not build or start, and no workflow runs it. It is
repaired here because it produces the live numbers above:
cgroupnsis not a Compose key,the build context pointed one level below the repository root, the image copied a
cmake/directory that does not exist while missing three packages from the dependency closure,
rosdepran after the apt lists had been deleted and resolved test-only dependencies foran image built with
BUILD_TESTING=OFF,--symlink-installleft the runtime stage withdangling links, and both test files picked the gateway's own node, whose process is not in
the PID cache.
Checklist