Skip to content

test(core): let e2e builds re-root the hardware probes - #510

Open
rominf wants to merge 9 commits into
mainfrom
test/e2e-host-root
Open

rominf wants to merge 9 commits into
mainfrom
test/e2e-host-root

Conversation

@rominf

@rominf rominf commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

First of a short series that lets the cucumber E2E suite describe the GPU machine a scenario needs, instead of depending on the runner's hardware. Today a scenario about eight Instinct GPUs, a WSL2 distribution or a host with no GPU can only run where such a machine happens to be. The CLI reads fixed host paths to learn what it is running on, so nothing else can stand in.

  • Adds rocm_core::host_path(). In a build with the new rocm-core/e2e-test-hooks feature, it re-roots absolute host paths under $ROCM_CLI_TEST_HOST_ROOT. Without the feature it is the identity and never reads the environment, so a release build always probes the real machine. This is the same model as the existing ComfyUI source-archive override.
  • Routes these probes through it:
    • the KFD topology, /dev/kfd, /dev/dri and /sys/class/drm
    • /sys/module/amdgpu
    • /dev/dxg, /proc/version and the WSL ROCDXG/dxcore checks
    • /etc/os-release for the distro name examine reports (detect_distro_name), plus /proc/cpuinfo, /proc/meminfo, /proc/cmdline and the modprobe directories
    • /proc/modules, read only when lsmod fails
  • The CLI keeps printing the logical path (/dev/kfd, /etc/modprobe.d/x.conf), never the re-rooted one.
  • Container detection (/.dockerenv, /run/.containerenv, /proc/1/cgroup) is routed too.
  • rocm-core gains an e2e-test-hooks feature. The E2E build (cargo build -p rocm -p rocmd --features rocm/e2e-test-hooks, as cargo xtask e2e runs it) turns it on for both binaries, because Cargo builds one shared rocm-core for that invocation and rocm's feature enables it. rocmd and the two engine crates also declare a forwarding feature, but that edge fires only when the feature is named on them, and the standalone rocm-engine-vllm/rocm-engine-lemonade binaries are never built with it. Neither engine nor rocmd calls a re-rooted probe today.
  • Documented beside the ComfyUI override in docs/release-trust.md.

Risk: low. The release build gets a PathBuf where it had a &Path, and nothing else changes; the identity is pinned by a unit test that really sets the variable.

Deliberately left on the real host, and known gaps:

  • ROCm install discovery reads the real host: which installs exist, and their paths and versions. The one re-rooted read under them is the WSL ROCDXG existence check, which looks for its files under /opt/rocm and under each discovered install's path inside the root.
  • Programs the probes execute, such as ldconfig, run from the real host. If a scenario needs to control the linker-cache answer, the follow-up is to inject the cache text, not to re-root the executable. lspci and uname are not re-rooted either.
  • The /usr/lib/wsl/lib loader entry, /proc/<pid> liveness and /dev/shm sizing stay on the real host.
  • Every package-manager plan reads the real /etc/os-release: the OpenMPI install hints in rocm-core, and the driver, OpenMPI and runtime-library installs the rocm binary runs. Their commands act on the real machine. Only the distro name examine reports follows a simulated root.
  • The dashboard's amd-smi pre-flight follows the simulated root only to hide a GPU: the real /dev/kfd keeps a veto, because the check gates a real process. On WSL, the reachability verdict still bypasses the device check, as before.
  • The host-root unit tests in rocm-core run in CI through a new feature-on nextest step in the affected-crates job, whenever rocm-core is affected. The rocm-binary test that read_os_release ignores the root runs only locally with --features e2e-test-hooks. Under a feature-on threaded cargo test, other tests that read the real host can overlap the window in which the root is set, which is why CI uses nextest.

Follow-ups in this series add an E2E_HARDWARE=simulated|real switch with a @requires-real-gpu tag, then the simulated-machine Given steps and fake host tools. Those move 19 scenarios that only need the CLI to see a GPU (detection, diagnose, WSL driver plans, --gpu refusals) onto every Linux lane, including the GitHub-hosted one. This PR changes no scenario. Follow-ups: #517 (simulated machines and the drift check), then #518 (narrowing the per-PR GPU lanes to a smoke test).

Scenario coverage (AGENTS.md §3)

No user-observable behaviour changes: release builds are unaffected, and the hook only exists under e2e-test-hooks. So there is no new scenario here. The re-rooting is exercised end to end by the follow-up PR's simulated-machine scenarios, which fail if any routed probe ignores the root.

Test plan

  • cargo test -p rocm-core --lib hardware_root passes both with and without --features e2e-test-hooks. The non-feature test sets ROCM_CLI_TEST_HOST_ROOT and asserts the path is unchanged; the feature test asserts re-rooting and that an empty value counts as unset.

  • cargo clippy --workspace --all-targets -- -D warnings is clean both with and without --features rocm/e2e-test-hooks.

  • Manual check on a WSL2 machine with a planted root holding an MI300X KFD node, /dev/kfd and a non-WSL /proc/version:

    • a hooks build of rocm examine reports wsl: false, detected_gfx_target: gfx942, driver_status: amdgpu_available and default_engine: vllm
    • a release build given the same variable still reports the real WSL2 machine
  • Not run locally: the full cargo test --workspace. Two proc_lifecycle tree tests fail on WSL2 against unmodified main. CI runs the full suite.

  • If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (n/a, no bug fix)

  • If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (n/a, no subcommand; the helper is its own module)

  • Every new or changed user-facing message was read against the code path that runs after it. (n/a, no user-facing message changes)

@volen-silo volen-silo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I went through this one carefully, because a seam that re-points the hardware probes is the kind of change that either leaks into release builds or quietly leaves half the probes reading the real machine.

The release-build half holds up. hardware_root() under #[cfg(not(feature = "e2e-test-hooks"))] is a const fn returning None and genuinely cannot reach the environment, no crate enables the feature by default, nothing in CI uses --all-features, and the release build passes no --features at all. The identity test that pins this does run. I also checked the two string reconstructions in probe_modules and probe_devices and they are byte-for-byte what the old entry.path() produced, so default output is unchanged.

What I would want addressed is the other half: consistency, and one conversion I think is actively wrong.

The ldconfig change routes an executable path through a seam the module's own docs say is for reads only, and I think it makes the function worse in both directions rather than better. Details inline.

Two probes were left outside the seam where I cannot see a reason. Container detection in examine.rs, and the dash collectors' own /dev/kfd check, which the new rocmd feature flag does not reach. Both produce exactly the mixed answer this change exists to prevent: a simulated host that reports the runner's real containerization, or the runner's real GPU presence, while every other fact is simulated.

I would also push back gently on the PR text's claim that the feature is forwarded from rocm, rocmd and both engines so the existing build turns it on everywhere. It does end up on for rocm and rocmd, but through Cargo unifying the single shared rocm-core build in that one combined invocation, not through the forwarding edges, and the standalone engine binaries never get it at all. Harmless today, but the sentence promises a guarantee the build does not give.

Finally, the new tests cover host_path and nothing downstream of it. None of the roughly thirty converted call sites is exercised, and as you note, no CI job enables the feature for unit tests, so the one test proving re-rooting actually happens never runs automatically. The two missed probes above are what that gap looks like in practice.

None of this is a release-safety problem, and the overall design reads well to me. I am asking for changes because the two unrouted probes will silently undermine the follow-up's simulated scenarios, and because the ldconfig conversion looks like it should be reverted rather than refined.

A few concerns I chased that did not hold: no double re-rooting in rocm_relative_file_exists, strip_prefix("/") is component-based so the path algebra is right, GpuProbeSources::host's new lifetime has no missed callers, the logical-versus-physical reporting contract holds everywhere I looked in lib.rs and main.rs, and the e2e lanes all build with the same feature set so there is no mixed-binary run.

Comment thread crates/rocm-core/src/lib.rs Outdated
for program in ["ldconfig", "/sbin/ldconfig", "/usr/sbin/ldconfig"] {
if let Some(text) = capture_optional_command(program, &["-p"]) {
let program = host_path(program);
if let Some(text) = capture_optional_command(&program.to_string_lossy(), &["-p"]) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This routes an exec path through the seam, which the host_path module docs specifically argue against: "anything a real process acts on rather than reads to describe the machine" is the stated exclusion, and capture_optional_command bottoms out in Command::new(program).spawn().

I think it also makes the function worse in both directions.

Where ldconfig is on PATH, the bare relative candidate is tried first and passes through host_path unchanged, so the real host's linker cache answers and the re-rooting never happens at all.

Where it is not on PATH — the exact case the doc comment directly above says these absolute fallbacks exist for, a non-root user on Debian and derivatives — the fallbacks now resolve to <root>/sbin/ldconfig and <root>/usr/sbin/ldconfig. A fixture root populated with sysfs and procfs text will not have a working linker binary there, so the loop falls through to None, ldconfig_lists_librocdxg() degrades to "could not ask", and the ROCDXG verdict changes under simulation on precisely the hosts the fallback was added to rescue.

I would leave these three candidates off the seam entirely. If the e2e suite needs to control this answer, injecting the cache text (or an explicit None) is a far smaller seam than making the probe executable-from-fixture.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, reverted in e22d8726. The three candidates are back off the seam, with a doc line saying why, and executed programs are now in the hardware_root exclusion list. If the suite later needs to control this answer, I'll inject the cache text as you suggest, in a follow-up.


/// [`read_text`] for an absolute host path, read where [`crate::host_path`]
/// says the host is.
fn read_host_text(path: &str) -> String {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Anchoring here because the real location is not in the diff: probe_container, at crates/rocm-core/src/examine.rs:2201-2218, still reads /.dockerenv and /run/.containerenv through Path::new(...).exists() and /proc/1/cgroup through read_text, so it never reaches this new helper.

It sits in the same Linux probe sequence as the probes you did convert — probe_env immediately before it, probe_shared_memory immediately after — and e.in_container / e.container_kind feed the diagnose scoring directly. CI runners very often run inside a container themselves, so a scenario describing a bare-metal machine picks up the runner's real containerization alongside otherwise-simulated facts. That is the mixed answer this change exists to remove, and it is the same shape as WSL detection, which you did route.

It also appears in neither list in the hardware_root module docs: not in the routed examples, not in the deliberate exclusions. So it reads as an oversight rather than a decision. If it is deliberate, the exclusion list is the place to say why. Otherwise the two existence checks want crate::host_path and the cgroup read wants read_host_text.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

An oversight, thanks. Fixed in 52c56b1d: both marker checks go through crate::host_path and the cgroup read through read_host_text, and they're now listed among the routed probes in the module docs. ffe5386e covers them, planting the container once by marker and once by cgroup alone.

Comment thread apps/rocmd/Cargo.toml
workspace = true

[features]
e2e-test-hooks = ["rocm-core/e2e-test-hooks"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Worth checking what this flag actually buys rocmd today.

I could not find a call from apps/rocmd/src/ into any probe this PR converted, and the GPU-presence check the daemon's dashboard path depends on lives outside the seam: crates/rocm-dash-collectors/src/amd_smi.rs opens the literal /dev/kfd in device_accessible (KFD_DEVICE at line 23, reached from line 84 via preflight_passes). That crate deliberately has no rocm-core dependency.

The fallback is reached in exactly the case this feature targets. gpu_reachable_for_preflight(is_wsl_host, has_usable_gpu) in apps/rocm/src/dash.rs is false on any non-WSL host by design, so on a simulated bare-metal machine preflight_passes falls straight through to device_accessible(Path::new(KFD_DEVICE)) against the runner's real device. This one is not in the known-gaps list in the PR description.

preflight_passes already takes an injectable &Path, so this should not need a new crate edge — dash.rs already depends on rocm-core, and could resolve the path through host_path and thread it in the same way it already threads the WSL verdict. Failing that, it belongs in the known gaps, because as written rocmd gains a feature flag that does nothing for its own GPU probe.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Routed in 894d5cbc, with one constraint. This pre-flight decides whether a real amd-smi process may start, so re-rooting /dev/kfd alone isn't safe: a simulated host that plants /dev/kfd on a GPU-less runner would launch amd-smi into the D-state hang the check exists to prevent. Instead dash.rs resolves the host-root path through rocm_core::host_path and passes it through RunnerOptions, the same way as the WSL verdict, and amd-smi runs only if both the real node and the host-root one are readable. A simulated GPU-less host now hides the runner's GPU, and a simulated GPU can't start amd-smi on a machine without one. A new four-case test pins this: dropping either half of the rule fails it. Without a root the two paths are equal and the second open is skipped, so release behaviour is unchanged, and no new crate edge is needed. On WSL, the reachability verdict still stands in for the device check, as before.

On the rocmd half: you're right that its feature edge does nothing today. rocmd calls no converted probe and doesn't depend on the dash crates; the dashboard daemon runs inside rocm. The description is corrected.

Comment thread engines/vllm/Cargo.toml
workspace = true

[features]
e2e-test-hooks = ["rocm-core/e2e-test-hooks"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An accuracy point on the PR description rather than on this line itself.

"The feature is forwarded from rocm, rocmd and both engines, so the existing cargo build -p rocm -p rocmd --features rocm/e2e-test-hooks ... turns it on everywhere" is not quite how this resolves.

--features rocm/e2e-test-hooks is scoped to the rocm package and never names rocmd, so rocmd's own edge does not fire. It gets the feature because Cargo unifies the single shared rocm-core build across that one combined invocation. Build rocmd by itself and the edge still will not fire.

These engine edges likewise apply to the engine libraries that rocm links, not to the standalone rocm-engine-vllm and rocm-engine-lemonade binaries. Those are built only in the ci.yml lane that compiles -p rocm -p rocmd -p rocm-engine-lemonade -p rocm-engine-vllm -p xtask, which passes no features at all. The e2e lanes never build them.

Nothing breaks today, since neither engine calls host_path and rocmd calls none of the converted probes. But the sentence asserts a guarantee the build does not provide, and whoever later adds a probe behind one of those entry points gets the real host silently. Worth either narrowing the claim or having the e2e build name the packages explicitly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, and the description is narrowed. rocmd gets the feature because Cargo builds one shared rocm-core for the combined invocation, not through its own edge, and the standalone engine binaries never get it. Nothing in those entry points calls a re-rooted probe today.

Comment thread docs/release-trust.md Outdated
the DRM cards under `/sys/class/drm`, `/sys/module/amdgpu`, `/proc/version`,
`/proc/cpuinfo`, `/proc/meminfo`, `/proc/cmdline`, `/proc/modules`,
`/etc/os-release`, the modprobe configuration directories, and the WSL plumbing
under `/usr/lib/wsl` and `/opt/rocm`. The E2E suite can point those reads at a

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This list and the paragraph a dozen lines below contradict each other on /opt/rocm. Here it is named among the paths that can be pointed at a simulated host; below, "discovered ROCm installs ... are never re-rooted."

Both describe the same literal string, and only the narrow case is true. The hardcoded /opt/rocm existence check in rocm_relative_file_exists is re-rooted, but discover_rocm_installs() — which produces the "ROCm install detected" fact and version that examine reports — uses raw fs::read_dir, fs::canonicalize and fs::read_to_string with no host_path anywhere. This is a trust document, so someone auditing it would reasonably come away believing the reported install version is simulatable. It is not.

I would name the specific check that is re-rooted (the WSL ROCDXG capability probe) rather than the bare path. The /usr/lib/wsl entry has the same shape, but there the closing paragraph's "loader entry an engine is launched with" does disambiguate adequately.

While you are in here: the list does not carry the /etc/os-release carve-out you mention in the PR description, where the install-hint helpers in crates/rocm-core/src/openmpi.rs still read the real file. That is invisible to someone reading only this document.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in a200d963. The list now names the WSL ROCDXG file check under /opt/rocm instead of the bare path. It says plainly that install discovery, and the version examine reports, always read the real host, and it carries the /etc/os-release install-hint carve-out. 894d5cbc adds the dash pre-flight as routed, with the real device keeping a veto.

}

#[cfg(test)]
mod tests {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These tests pin host_path_under's algebra and the env-var honouring, all against the single literal /dev/kfd. That is the plumbing, not what the plumbing was installed for.

Nothing exercises any of the roughly thirty converted call sites. The existing _in-suffixed seams (detect_kfd_gfx_target_in, linux_kfd_gpu_node_count_in) take a directory directly and bypass host_path, so even the thin wrappers that do call it are uncovered. Reverting any single conversion back to a raw literal would compile and pass the entire suite.

Combined with the gap you already flag — no unit-test, nextest or clippy job enables rocm-core/e2e-test-hooks — the one test proving re-rooting actually happens never runs anywhere automated. The feature-off identity test does run, which is the safety-critical direction, so this is not a release risk. It is that the seam's usefulness currently rests entirely on review.

I am not asking for a test per call site. One test that plants a root holding a KFD node and a /dev/kfd, runs the probes against it and asserts the simulated answer would have caught both missed probes I flagged elsewhere in this review, and would keep catching the next one.

Separately and minor: host_path_under does not guard ... An input containing .. after the leading slash walks back out of the root, because join does not normalise. Not reachable through any current call site, since they are all fixed literals, so I would just state the constraint in the doc comment rather than add validation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added in ffe5386e. One test plants a root with a KFD topology, /dev/kfd, a non-WSL /proc/version and container markers, runs the real converted probes with the feature on, and asserts the simulated answers. Reverting the container, cgroup, KFD or /proc/version conversions each make it fail. A /dev/dxg revert only went undetected on our host because it has no real /dev/dxg. host_path_under's doc now states the no-.. precondition. The test runs only with --features e2e-test-hooks, and the description says so.

@rominf
rominf dismissed volen-silo’s stale review October 5, 2026 11:27

Dismissed as stale: this review is of 2daa432. Every finding is addressed, with a reply in each thread: ldconfig back off the seam (e22d872), container detection routed (52c56b1), a planted-root test of the converted probes (ffe5386), release-trust accuracy (a200d96), and the dash /dev/kfd pre-flight routed with the real device keeping a veto (894d5cb). The description is corrected. Please re-review the current head.

rominf added 6 commits October 5, 2026 11:28
The E2E suite can only exercise GPU behaviour on the self-hosted lanes,
because every hardware probe reads fixed host paths: /dev/kfd, the KFD
topology, /sys/module/amdgpu, /dev/dxg, /proc/version, /etc/os-release
and /usr/lib/wsl. A scenario cannot describe a host it is not running on.

Route those reads through rocm_core::host_path. In a build with the new
rocm-core e2e-test-hooks feature it re-roots absolute paths under
ROCM_CLI_TEST_HOST_ROOT; without the feature it is the identity and never
reads the environment, so a release build always probes the real machine.
The CLI keeps printing the logical path, so the root changes what the
probes find, not what users read.

The feature is forwarded from rocm, rocmd and both engines so the
existing `--features rocm/e2e-test-hooks` build turns it on everywhere.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
ldconfig_cache executes the program it names, so routing its candidates
through host_path pointed the absolute fallbacks at a fixture root that
holds no linker binary. On Debian-style hosts, where ldconfig is not on a
non-root PATH, the probe then fell through to "could not ask" and the
ROCDXG verdict changed under simulation; where ldconfig is on PATH the
bare name was never re-rooted anyway. Executed programs belong with the
paths the hardware_root module already leaves on the real host.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
probe_container sits in the same Linux probe sequence as the probes
already routed through host_path, and its answer feeds the diagnose
scoring directly, yet it still read /.dockerenv, /run/.containerenv and
/proc/1/cgroup from the real machine. A simulated bare-metal host
therefore picked up a containerised runner's real containerisation.

Route all three, and list them in the hardware_root module docs next to
the other routed probes. The exclusion list now also names executed
programs such as ldconfig and the install hints that still read the
real /etc/os-release.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The hardware_root tests pinned host_path's path algebra but nothing
downstream of it, so reverting any converted probe to a raw literal
compiled and passed. Plant a host root holding a KFD topology with one
GPU node, a /dev/kfd, a non-WSL /proc/version and container markers,
run the real probes with the e2e-test-hooks feature on, and assert they
describe the planted machine. The container is planted once by marker
and once by cgroup alone, so the test fails whether the runner is bare,
under Docker or under Podman.

The tests that set the root now share one lock, and host_path_under
documents that its input must not contain "..".

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The simulated-host-root list named /opt/rocm as re-rootable while the
paragraph below said discovered ROCm installs are never re-rooted. Only
the WSL ROCDXG capability check's file lookups are re-rooted; install
discovery, and the version examine reports, always read the real host.
Name that check instead of the bare path, add the container markers, and
list what stays on the real machine: install discovery, the WSL loader
entry and executed programs such as ldconfig, and the install hints that
read the real /etc/os-release.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The rocm dash amd-smi pre-flight opened the literal /dev/kfd, so on a
simulated GPU-less host the dashboard still found the runner's real GPU
while every other probe reported none. Re-rooting that path alone would
be unsafe: the check decides whether a real amd-smi process may start,
and a planted /dev/kfd on a machine without one would launch amd-smi
into the uninterruptible hang the check exists to prevent.

Thread the host-root view of /dev/kfd from apps/rocm (which resolves it
through rocm_core::host_path) into RunnerOptions, the detect plan and the
collector, and run amd-smi only when both it and the real /dev/kfd are
readable. A simulated host can now hide the GPU but never start amd-smi
on a host without one. Without a root the two paths are equal and the
second open is skipped, so the check is unchanged. The WSL reachability
verdict still substitutes for the device check as before. The dash crates
keep no rocm-core dependency.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf

rominf commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased onto current main (5f72ac6a). The only conflict was additive: #407 added pub mod model_readiness; in the same spot as hardware_root, and both are kept. The new module reads no host paths directly, so there was nothing new to route. Verified locally: rocm-core lib tests without (476) and with (477) e2e-test-hooks, xtask (255), check-crate-edges, dash collectors (108), dash:: (24), e2e-cucumber lib and feature_naming, and clippy -D warnings on the workspace with and without the feature, plus --test e2e. No code change.

@rominf
rominf force-pushed the test/e2e-host-root branch from 894d5cb to f7ef476 Compare October 5, 2026 11:46

@volen-silo volen-silo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Five commits against six points, and most of them are properly closed. Going through them in order, because you did enough work here that it should be said explicitly rather than inferred from which comments I did not repeat.

Closed.

  1. ldconfig_cache — reverted. The three candidates are off the seam and "programs the probes execute" is now in the exclusion list, which is the right place for it. Injecting the cache text later, if the suite ever needs to control that answer, is the smaller change it always was.

  2. probe_container — both marker checks and the cgroup read now go through the seam, and planting the container twice (once by marker, once by cgroup alone) is a better test than I asked for. I reverted each of the two conversions by hand and the test caught both.

  3. The dash pre-flight — this is the one I expected to be awkward and the answer is better than the one I suggested. Requiring both the real node and the host-root node, rather than substituting one for the other, keeps the D-state hazard the check exists for while still letting a simulated GPU-less host hide the runner's GPU. I traced it rather than taking it on trust: dash.rs is the only construction site that should re-root and the only one that does, every other RunnerOptions comes from ..Default::default() and gets the real device, and the two test-only entry points pass the real literal deliberately. With no root the two paths compare equal, the second open is short-circuited, and the expression collapses to exactly the old single check. device_accessible is a bare read-only open, so a planted regular file and a real character device behave the same for it. No new crate edge, as you said.

  4. The forwarding claim — narrowed, and the narrowed version matches what the build actually does. The combined -p rocm -p rocmd invocation is the only reason the daemon gets it, and the standalone engine binaries are built in a lane that passes no features at all.

  5. docs/release-trust.md — the /opt/rocm contradiction is gone and the install-version point is now stated plainly enough that nobody auditing it would come away believing the reported version is simulatable. Two smaller accuracy gaps left, in a comment below.

  6. The test — real, and it does the job. I did not take the "reverting X makes it fail" claim at face value; I reverted six conversions one at a time and ran it. Four fail immediately. The two WSL ones do not, and the /proc/version one needs more than the /dev/dxg caveat you already wrote down — details inline.

Still open. Three things, in descending order of how much they matter.

The first is mine as much as yours: /etc/os-release is re-rooted into the three package-manager install plans, which then run real apt/dnf/amdgpu-install against the machine. That was in the first push and I missed it; the new prose asserting the opposite is what made it visible. It is the one case where extending the seam reached something a real process acts on, which is exactly the rule the module docs state.

The second is that the new test, good as it is, still is not run by anything. No unit-test or clippy job passes the feature, so the test is not even compiled in CI. The thing I was worried about last round — that reverting a conversion compiles and passes everything automated — is still true, and now the test itself can rot unnoticed too.

The third is two small doc inaccuracies left over from the rewrite.

I also re-derived the release-build containment from scratch rather than trusting my earlier pass, since extending a seam is how that sort of property usually gets breached. It holds: the non-feature hardware_root() is a const fn returning None with no env read, hardware_root() is private, the feature is in no default list and no crate enables it transitively, nothing in the repo passes --all-features, and the release lane builds with no --features at all. The new unconditional rocm_core::host_path call in dash.rs is a pure identity call without the feature, and there is already a contract test keeping the Windows lifecycle lane off the hooks build. Nothing the five commits added reaches the seam from a shipping binary.

Comment thread apps/rocm/src/main.rs Outdated

fn read_os_release() -> Result<String> {
fs::read_to_string("/etc/os-release").context("failed to read /etc/os-release")
fs::read_to_string(rocm_core::host_path("/etc/os-release"))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This one is at least as much mine as yours — it was in the first push, I did not flag it, and what made it visible is the new prose asserting the opposite.

read_os_release() is re-rooted, and all three of its callers build a package-manager plan that then acts on the real machine: install_driver (main.rs:3419) feeds build_driver_install_plan and executes it when --yes is given and the plan is mutating, and ensure_openmpi_for_vllm (main.rs:9834) and ensure_torch_runtime_dep (main.rs:10009) do the same for the OpenMPI and libatomic/libnuma installs that run before serving. There is no fourth caller that merely describes the host; examine's distro name comes from detect_distro_name in rocm-core, which has its own re-rooted read and is the one that belongs on the seam.

So in a hook build with a root set, rocm install driver --yes takes its distro, codename and package manager from the fixture and then runs apt or dnf or amdgpu-install against the real runner. That is precisely what the module's own rule excludes — "anything a real process acts on rather than reads to describe the machine" — and the self-hosted and nightly lanes do build release binaries with the feature on real hardware, so it is reachable as soon as a scenario sets a root.

It also turns three statements from incomplete into wrong: the release-trust.md bullet "The package-manager install hints, which read the real /etc/os-release", the same sentence at hardware_root.rs:23, and the matching line in the PR description. All three cite openmpi.rs, which is accurate for openmpi.rs's own helpers — it is these three call sites that are not covered by it.

Smallest fix is to put read_os_release back on the real file and leave the seam to detect_distro_name. A simulated distro in examine already works through that one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, and confirmed: all three callers build package-manager plans that run on the real machine. Fixed in 48755993. read_os_release reads the real /etc/os-release again, and the seam stays with detect_distro_name for the distro name examine reports. A feature-on test plants a different os-release under a root and asserts the real file comes back. Routing the read through host_path again fails that test. The three statements that were wrong (release-trust.md, the hardware_root docs and the PR description) are corrected in 68bf43aa, and the description now names all three plans. One related path: under a simulated WSL root, install_driver picks the ROCDXG plan, but its execute phase checks the real /dev/dxg and libdxcore.so before installing anything, so a bare-metal runner stops there.

/// or cgroup answers differently whether the test runs bare, under Docker
/// or under Podman. The GPU is a gfx1101, and `/proc/version` names no WSL
/// kernel.
#[cfg(all(feature = "e2e-test-hooks", target_os = "linux"))]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This does what I asked for, and I checked rather than assumed — I reverted each conversion in turn and ran the test.

Caught: the container markers, the cgroup read, the KFD topology directory (both the detect_linux_kfd_gfx_target and linux_kfd_gpu_node_count wrappers), and stat_device's host_path. That is the useful half and it is genuinely useful.

Not caught: both halves of the WSL check. You already wrote down /dev/dxg. /proc/version has the same hole for a different reason, and the reason is the assertion rather than the host. assert!(!wsl) is the answer a bare-metal runner gives anyway, so an unrouted read agrees with a routed one and the assertion passes either way. It failed for you (and for me) only because the machine underneath is WSL2. I confirmed that by pointing the unrouted read at a path that does not exist — which is what a non-WSL runner looks like to this code — and the test passed with the conversion gone. Planting a WSL-naming /proc/version and asserting is_wsl_host() is true fails everywhere except a real WSL host, which is the direction that holds. Worth doing, because "a /dev/dxg that makes a bare-metal runner read as WSL" is the example the module doc leads with.

The other half is that nothing runs this. cargo nextest run and cargo test --workspace --all-targets are the only unit-test invocations in CI and neither passes the feature; cargo clippy --locked --workspace --all-targets does not either. So the test is not run, not linted, and not even compiled anywhere automated. The nightly and self-hosted lanes do compile with --features rocm/e2e-test-hooks, but they build, they do not test. Two consequences: reverting a conversion still compiles and passes everything CI does, which is the gap I raised last round unchanged; and this test can now break or rot without anyone finding out. A single cargo test -p rocm-core --features e2e-test-hooks step closes both.

Minor, while you are in here: fix.rs's current_os_reports_wsl_exactly_when_is_wsl_host_does reaches crate::is_wsl_host() twice without taking HOST_ROOT_TEST_LOCK, so with the feature on it can straddle this test's root and compare two different answers. I ran the suite with the feature three times without reproducing it, so the window is small — but it is the same race the *_TEST_LOCK convention exists for, seen from the reader's side rather than the writer's.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

All three addressed in 7e00663a.

  • The test now plants a WSL kernel, then a /dev/dxg alone, and asserts that each reads as WSL. Pointing either unrouted read at a nonexistent path, as you did, now fails it.
  • CI: a new step in the affected-crates test job runs rocm-core's lib tests with e2e-test-hooks whenever rocm-core is affected. It uses nextest, like the step before it, so the test that sets the root can't leak it into another test that probes the host.
  • current_os_reports_wsl_exactly_when_is_wsl_host_does now holds HOST_ROOT_TEST_LOCK.

The one feature-on test in the rocm binary, the os-release one above, still runs only locally. Running it in CI would need a feature-on rocm build in that job, and the description says so.

Comment thread docs/release-trust.md Outdated
`/proc/1/cgroup`, the container markers `/.dockerenv` and `/run/.containerenv`,
`/etc/os-release`, the modprobe configuration directories, the WSL plumbing
under `/usr/lib/wsl`, and the WSL ROCDXG capability check, which looks for
`lib/librocdxg.so` and `share/rocdxg/dids.conf` under `/opt/rocm`. The E2E

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Two things the rewrite left slightly off. Both small, but this is the document people audit.

rocm_relative_file_exists (lib.rs:2628-2636) checks host_path("/opt/rocm").join(relative) and then host_path(&install.path).join(relative) for every discovered install, so the re-rooted part is not only the hardcoded /opt/rocm — a simulated root can answer the ROCDXG question for a versioned install too. As written, this sentence and the "ROCm install discovery ... always the real ones" bullet below describe a cleaner split than the code makes.

That discovered-install branch is also the one your new host_path_under doc comment names as the sole non-literal caller, so the .. precondition and this sentence are describing the same path from two directions. Worth having them say the same thing.

Second, hardware_root.rs's own routed list (lines 9-12) never mentions the ROCDXG check or /opt/rocm at all, and its exclusion paragraph carves out only install discovery. So the module doc still reads as though nothing under /opt is re-rooted — the same contradiction I flagged in the trust document, now living one file over.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 68bf43aa. release-trust.md and the hardware_root module docs now both say that discovery itself reads the real host, and that the ROCDXG check in rocm_relative_file_exists looks under /opt/rocm and under each discovered install's path inside the root. The routed list names that check, and the exclusion paragraph carves out only discovery itself. The host_path_under .. note now names that same caller, and notes that $ROCM_PATH is taken verbatim there.

rominf added 3 commits October 5, 2026 12:52
read_os_release was routed through host_path, but all three callers --
the driver install, and the OpenMPI and runtime-library installs before
serving -- build apt/dnf/amdgpu-install plans that run against the real
machine. Under a simulated host root, `rocm install driver --yes` would
take its distro from the fixture and then run the resulting commands on
the runner, which is exactly what the hardware_root rule excludes.

Read the real file again. The descriptive distro name examine reports
comes from rocm_core::detect_distro_name, which keeps following the root.
A feature-on test plants a different os-release under a root and checks
read_os_release still returns the real one.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The planted-root test only asserted that a non-WSL root reads as "not
WSL", which a bare-metal runner answers anyway, so an unrouted
/proc/version or /dev/dxg read passed everywhere but a real WSL host.
Plant a WSL kernel, then a /dev/dxg alone, and assert both read as WSL.

No CI job enabled e2e-test-hooks for unit tests, so these tests were not
even compiled. Add a step to the affected-crates test job that runs
rocm-core's lib tests with the feature whenever rocm-core is in the
selection. It uses nextest like the step before it, so the test that
sets the root cannot leak it into another test probing the host.

fix.rs's is_wsl_host comparison now holds the host-root lock, so in a
hook build it cannot compare answers from either side of a root change.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
The ROCDXG check in rocm_relative_file_exists re-roots /opt/rocm and
every discovered install path, so "install discovery is always real"
was a cleaner split than the code makes. Say that discovery itself is
real and that this one existence check looks under the discovered paths
inside the root, in both the hardware_root module docs and
release-trust.md, and make the host_path_under ".." note describe the
same caller.

Also correct the /etc/os-release statements: the reported distro name
follows the root, and every package-manager plan -- the OpenMPI install
hints and the rocm binary's driver, OpenMPI and runtime-library installs
-- reads the real file.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf dismissed volen-silo’s stale review October 5, 2026 13:05

Dismissed as stale: this review is of f7ef476. All three remaining points are addressed, with a reply in each thread: package-manager plans read the real /etc/os-release (4875599); the WSL halves of the planted-root test can now fail, the host-root tests run in CI, and the fix.rs test holds HOST_ROOT_TEST_LOCK (7e00663); and the docs match the code (68bf43a). Please re-review the current head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants