Repository navigation
test(features): add CDI injection scenarios for Docker and Podman - #4461
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
🌿 Preview your docs: https://nvidia-preview-pr-4461.docs.buildwithfern.com/openshell |
Compatibility review (advisory)Needs review No source-level SDK, CLI, configuration, policy, Helm, or state contract changes were found. Qualification remains incomplete because release VM drivers embed newly hashed Linux runtime archives whose semantic delta is unavailable in the supplied source. Not inspected: api-behavior 1. api-behavior — NOT ACKEDThe pinned x86_64 and aarch64 Linux VM runtime archive hashes changed, but the archive contents and semantic differences are not present in the supplied source.
Review details
Advisory review; publication gates are unchanged. |
SDAChess
left a comment
There was a problem hiding this comment.
Some questions, I'm surprised there are changes related to the VM driver, are these on purpose?
844e44f to
e9fa277
Compare
360f6d5 to
01b0b55
Compare
e9fa277 to
fb39ac1
Compare
|
This pull request has had no activity for 14 days and is now marked stale. |
1228858 to
22db4b6
Compare
fb39ac1 to
82fed66
Compare
82fed66 to
3f0a69c
Compare
|
Label |
|
/ok-to-test 3f0a69c |
SDAChess
left a comment
There was a problem hiding this comment.
Keeping the shell is fine with me. One small fix for the echo fixture and a non-blocking parser nit below.
3f0a69c to
2f2907b
Compare
matthewgrossman
left a comment
There was a problem hiding this comment.
Thanks for this, Evan. The layering (withSetup/features), one crate per feature, explicit CI rows, and expect_compute_driver all fit how we're structuring features suites. My comments are about how inputs reach the tests, not the scenarios themselves.
1. Scenario expectations should come through OPENSHELL_TEST_* and test-env.yaml.
The suite reads a hard-coded guest path (/var/lib/openshell-test-inputs/cdi/scenarios.json), with an override named OPENSHELL_CDI_SCENARIOS. The pattern from the Keycloak example is that setup includes playbooks/test-env.yaml to write OPENSHELL_TEST_* into /etc/openshell-test/env.d/<name>.env, the runner sources those into the test environment, and the suite reads plain variables. Reading a fixed guest path means the suite doesn't run unchanged against any deployment that supplies the same variables, and the openshell-test-inputs file-path style is the one we're moving away from (OPENSHELL_TEST_INPUT_DIR in the old Podman playbooks). Could the cdi_fixtures role publish something like OPENSHELL_TEST_CDI_SCENARIOS (and the device class, if useful) this way?
2. /etc/openshell-test/driver.toml is a new general mechanism.
It's slurped in detect.yaml and templated into the Docker and Podman gateway.toml. It fits "one environment per gateway config", but it isn't one of the channels we've settled on, and other features will likely start using it. Is this meant to be the shared way for an environment to add driver settings? If so I'd like to agree on the name/contract and document it in TESTING.md; if it's CDI-specific, it may be better scoped that way.
3. Packaged gateway override.
openshell_packaged_gateway now writes a fresh operator gateway.toml containing only the extra driver settings. Please confirm nothing the generated config previously supplied for the rpm Podman rootless path (socket path, openshell_gateway_podman_driver_settings, etc.) is dropped when this file exists. The PR notes validation was arm64-only, so CI on x86_64 will be the first real check.
4. Hidden perl dependency.
echo-socket needs perl in the workload image. Fine for the default image, but it'd be good for that to be explicit rather than inferred. For example, tie it to OPENSHELL_TEST_BASE_IMAGE, or state it in the suite docs.
Smaller notes, non-blocking:
- The
writeprobe uses: >> path, so on a path that doesn't exist yet (.../directory/probe,.../new-file) it creates a file on the host if the write is allowed. The comment says nothing is written. rejectedisn't exercised by any scenario afterdevice-node-with-gidwas dropped, and an emptyrejectedstring matches any failure.- The expectations record current behavior (e.g.
read: deniedon read-only mounts). A short "characterization, not spec" note would keep them from being read as desired behavior.
0bf9937 to
06125b4
Compare
|
Thanks for the review. I've reworked some things here to address things. This includes stacking this PR on #4289 -- making the driver config changes there since that would be required anyway for a switch to
I've updated this. The path to the scenarios file is now published, and the CDI device kind is included in the scenario instead of as a separate constant / variable.
This is what is addressed in #4289. I changed the implementation there to define a gateway config override as this would be required for non-driver configs at some point. I also created #5446 to track adding "merged" configs / overrides as a first-class entity.
The new merge logic should maintain existing keys since we're doing a deep merge.
Documented. For what it's worth, I chose
|
Define the base environments once and build the Keycloak and user-namespace variants from them with a shared helper, instead of listing the Ubuntu Docker variants by hand. Add a features table so a feature's setup playbooks and inputs apply to each base it supports. The Ubuntu Docker Keycloak environment now also gets the base's test-tools playbook, as the Fedora Keycloak environments already do. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Add the cdi_fixtures role, which defines one CDI device per scenario of the tmachine.example/cdi-scenario class together with the host files each references, so CDI injection can be tested without GPU hardware. Run it in a cdi feature environment on each Docker and Podman base, such as ubuntu-docker-rootful-cdi, instead of the base environments. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Add the cdi features package, which creates a sandbox that selects one CDI scenario device on the gateway's Docker or Podman compute driver and checks what the workload sees against the scenario's published expectations. The first test covers a mount under a directory the sandbox already makes read-only, and the merge of spec and device edits. Run the suite in ubuntu-docker-rootful-cdi with the deb installer, and in fedora-podman-rootful-cdi and fedora-podman-rootless-cdi with the rpm installer. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Add the scenarios for read-only and read-write mounts, library parent derivation, a device node, and a unix socket, with one test per scenario in the cdi suite. Each test delegates to the scenario wrapper, which checks the workload against the expectations recorded in the spec. Add the fixture machinery the scenarios need: a pipe fixture type, a host device node that a systemd service creates and labels on every boot, and the echo socket, which a systemd service serves on every boot. The node has the SELinux type of /dev/null so that a rootless runtime, which bind-mounts it, can use it. Signed-off-by: Evan Lezar <elezar@nvidia.com>
06125b4 to
e7776d2
Compare
Summary
Add hardware-free CDI injection tests to the features suite and tmachine environments that provide the fixtures. Each test creates a sandbox that selects one scenario device through the Docker or Podman driver and checks what the workload observes against expectations recorded in a spec. This PR builds on #4289, which has merged.
Related Issue
Related to #4460. This PR supplies the initial CDI test infrastructure; the issue remains open to track follow-up coverage.
Changes
featurestable so a feature's setup playbooks and inputs apply to each base it supports. The Keycloak and Podman user-namespace variants are built the same way. The Ubuntu Docker Keycloak environment now also runs the base'stest-toolsplaybook.cdi_fixturesrole writes[openshell.drivers.<driver>] allow_driver_config = trueto/etc/openshell-test/gateway-overrides.toml, and every installer merges it into the gateway's own configuration, so the rpm keeps its seeded defaults. Each base intests/tmachine/config.nixpasses its compute driver to setup playbooks asenvironment_driver.cdifeature with environmentsubuntu-docker-rootful-cdi,fedora-podman-rootful-cdiandfedora-podman-rootless-cdi. Thecdi_fixturesrole creates one CDI device per scenario of thetmachine.example/cdi-scenariokind, installs the spec in/etc/cdi, publishes the expectations, and enables caller driver config, which the gateway requires to accept a per-sandbox CDI device./usr/local/share, whose default type containers can read, and the socket's directory is labeledcontainer_file_t. A systemd service serves the echo socket withsystemd-socket-activate --inetd. It opens the socket to any user, because the process creates it root-only, and runs ascontainer_ton SELinux hosts, because a container may not connect to a socket owned by a service's defaultunconfined_service_t. The playbook connects to the socket after starting the service and fails if it does not echo. Another service creates the host node/dev/tmachine-cdi-nullon every boot with the SELinux type of/dev/null, so a rootless runtime that bind-mounts it can use it.OPENSHELL_TEST_CDI_SCENARIOSthroughplaybooks/test-env.yaml. The expectations name the device kind, so the suite assumes no kind or path of its own, fails when the variable is unset, and runs in any environment that creates matching devices and sets it.cdifeatures package with a scenario wrapper (env,exists,contents,filetype,read,write,non-root,in-group-ofandecho-socketchecks) and 10 tests, one per scenario.echo-socketconnects to a unix socket from the workload and expects its payload back; it needsperlin the workload image, which the default image has, and reportsno-clientwithout it. Thewritecheck applies to a file that exists: it opens it for append without writing, and a path that does not exist reportsmissing, so a check on the wrong path fails. Nothing creates files on the host. There is no check for creating an entry in a mounted directory yet, so the scenarios no longer cover that. Run it frome2e-suites.ymlin the three environments above, with thedebinstaller for Docker and therpminstaller for Podman.Removed scenario:
device-node-with-gidThis scenario is not in this PR. The Podman driver rejects a CDI-added supplementary group at sandbox startup (#5305), and the scenario is recorded there and below so it can be restored once the driver is fixed. Its original definition in the
cdi_fixturesrole spec (vars/scenarios.yaml), with a groupopenshell-cdi-fixturecreated in the guest and the node made withmknod:device-node-with-gid scenario (verbatim from fb39ac1)
The
group-ofcheck was later renamedin-group-of. The role'stasks/scenarios.yamlcreates the system groupopenshell-cdi-fixture(registered ascdi_fixture_group) and makes the node withmknod ... c 1 3.Release Notes
Testing
cdipackage cover check parsing, comparison and the generated probe shell, including a probe against a real unix socket and the device kind read from the expectations.cdipackage passed all 24 tests (the 14 unit tests and the 10 scenarios) inubuntu-docker-rootful-cdi(deb) andfedora-podman-rootless-cdi(rpm) on this stack.fedora-podman-rootful-cdi(rpm) passed 22 tests (12 unit and 10 scenarios) at an earlier revision and was not re-run locally; CI runs all three. Each ran withnix run .#openshell-test-runner -- --environment <environment> --installer <installer> --family features --package cdion a local Apple Silicon Mac, using locally built arm64 artifacts.The expectations in the spec record the sandbox's behavior today, so they will change as CDI grant derivation changes. x86_64 was not run locally. Rootless Podman cannot create device nodes, so the
device-nodescenario uses a host node at the same path as the container path.Checklist