fix(onboard): skip CDI GPU mode on Docker Desktop WSL (#5512) - #5537
Conversation
|
@abhi-0906 can you add a DCO 'Signed-off-by' to the PR description, please? |
📝 WalkthroughWalkthroughAdds a ChangesDocker Desktop WSL CDI skip in GPU patch flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
prekshivyas
left a comment
There was a problem hiding this comment.
Correct and well-scoped. The `!dockerDesktopWsl` guard threads cleanly through the entire call chain, and the production path correctly defaults to `isDockerDesktopWslRuntime()` at `createDockerGpuSandboxCreatePatch`. Tests cover both skip-on-WSL and CDI-preferred-on-non-WSL paths directly against the exported function.
Minor gap: no test verifies that `createDockerGpuSandboxCreatePatch` picks up the `isDockerDesktopWslRuntime()` default when `dockerDesktopWsl` is omitted from options. Not a blocker.
…NVIDIA#5512) When the Docker GPU patch recreate `docker run` fails after the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed new container and left the backup orphaned — stranding the sandbox with no live original and colliding on the next retry. Reuse the existing rollback primitive on this path: remove the failed new container, rename the backup back to the original name, and start it, so onboarding restores the pre-patch sandbox instead of leaking a backup container. Adds rollbackDockerGpuPatchOnRecreateFailure to the finalize module (resolving the real docker start/rename defaults) and records context.rolledBack for diagnostics. Follow-up to NVIDIA#5537 (which makes the patch succeed on Docker Desktop WSL, so this path is no longer hit there) addressing the orphan-backup symptom noted in NVIDIA#5512. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
|
✨ Thanks for the proposed fix addressing the CDI GPU mode failure on Docker Desktop WSL2 at onboard step [6/8]. This proposes a way to skip CDI mode when the WSL distro exposes no usable nvidia.com/gpu spec, allowing the --gpus compatibility path to be used instead. Related open PRs: Related open issues: |
On Docker Desktop + WSL2, onboard's [6/8] Docker GPU patch recreates the sandbox with `--device nvidia.com/gpu=all` (CDI) and fails with "CDI device injection failed: unresolvable CDI devices nvidia.com/gpu=all", even though preflight already commits to the `--gpus` compatibility path. Docker Desktop advertises CDI spec directories, so dockerReportsNvidiaCdiDevices() returns true and buildDockerGpuModeCandidates offers CDI first; the create-only probe passes but the real recreate fails because the WSL distro exposes no usable nvidia.com/gpu spec. Thread the existing Docker Desktop WSL detection (isDockerDesktopWslRuntime, already used to gate the patch) through selectDockerGpuPatchMode into buildDockerGpuModeCandidates, and skip the CDI candidate when on Docker Desktop WSL so the patch uses `--gpus all`. Native Docker-CDI hosts are unaffected and still prefer CDI (preserving the NVIDIA#4948 gateway supervisor-wiring contract). Reached only after the [2/8] gateway-bind issue (NVIDIA#5513 / NVIDIA#5534). A follow-up is still needed for the orphaned `*-nemoclaw-gpu-backup-*` container left behind on an early patch failure. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
…NVIDIA#5512) When the Docker GPU patch recreate `docker run` fails after the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed new container and left the backup orphaned — stranding the sandbox with no live original and colliding on the next retry. Reuse the existing rollback primitive on this path: remove the failed new container, rename the backup back to the original name, and start it, so onboarding restores the pre-patch sandbox instead of leaking a backup container. Adds rollbackDockerGpuPatchOnRecreateFailure to the finalize module (resolving the real docker start/rename defaults) and records context.rolledBack for diagnostics. Follow-up to NVIDIA#5537 (which makes the patch succeed on Docker Desktop WSL, so this path is no longer hit there) addressing the orphan-backup symptom noted in NVIDIA#5512. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
ad6ffa2 to
c09ffa6
Compare
…NVIDIA#5512) When the Docker GPU patch recreate `docker run` fails after the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed new container and left the backup orphaned — stranding the sandbox with no live original and colliding on the next retry. Reuse the existing rollback primitive on this path: remove the failed new container, rename the backup back to the original name, and start it, so onboarding restores the pre-patch sandbox instead of leaking a backup container. Adds rollbackDockerGpuPatchOnRecreateFailure to the finalize module (resolving the real docker start/rename defaults) and records context.rolledBack for diagnostics. Follow-up to NVIDIA#5537 (which makes the patch succeed on Docker Desktop WSL, so this path is no longer hit there) addressing the orphan-backup symptom noted in NVIDIA#5512. Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
…#5512) (#5541) ## Summary Follow-up to #5537 addressing the orphan-backup symptom in #5512. When the Docker GPU patch's recreate `docker run` fails **after** the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed *new* container and left the backup orphaned — stranding the sandbox with no live original, and colliding with `*-nemoclaw-gpu-backup-*` on the next retry (as reported in #5512). The supervisor-reconnect failure path already rolls back to the backup; this path didn't. ## Fix Reuse the existing rollback primitive (`rollbackToBackupContainer`) on the early-failure path: remove the failed new container, rename the backup back to the original name, and start it — restoring the pre-patch sandbox instead of leaking a backup container. - Adds `rollbackDockerGpuPatchOnRecreateFailure(refs, deps)` to `docker-gpu-patch-finalize.ts`, which resolves the real `docker start` / `docker rename` defaults (the recreate call path only carries a deps subset, so `dockerStart` would otherwise be unset). - Records `context.rolledBack` for failure diagnostics, matching the reconnect-failure path. - No `onboard.ts` change; all edits are under `src/lib/onboard/`. ## Testing - New composed test in `docker-gpu-patch-rollback.test.ts`: when `dockerRunDetached` fails, the backup is renamed back to the original and started, and is never left as an orphaned container. - `tsc -p tsconfig.src.json` clean; rollback / finalize / sandbox-create suites pass (18/18). ## Relationship to the WSL Docker Desktop chain - #5534 — gateway bind at `[2/8]` - #5536 — gateway cleanup on probe failure - #5537 — skip CDI GPU mode at `[6/8]` (makes the patch succeed on Docker Desktop WSL, so this early-failure path is no longer hit there) - this PR — restore the pre-patch sandbox for any *other* early GPU-recreate failure Refs #5512. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery for Docker GPU patch recreation failures: if the GPU-enabled recreate step fails after the original container is renamed, the system now performs a reliable rollback to the pre-patch sandbox state and cleans up the failed recreate attempt. * **Tests** * Added a rollback-path test for a recreate-phase `docker run --detach` failure, verifying restoration of the original container name, restart behavior, and correct cleanup (including ignoring the failed recreated container). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> --------- Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
…VIDIA#5537) ## Summary On Docker Desktop + WSL2 with an NVIDIA GPU, onboard's `[6/8]` Docker GPU patch recreates the sandbox container with `--device nvidia.com/gpu=all` (CDI syntax) and fails: ``` CDI device injection failed: unresolvable CDI devices nvidia.com/gpu=all ``` even though preflight already logs that it will use the `--gpus` compatibility path. The only workaround today is `--no-gpu` / `NEMOCLAW_SANDBOX_GPU=0`, which disables GPU entirely. ## Root cause Docker Desktop advertises CDI spec **directories**, so `dockerReportsNvidiaCdiDevices()` returns true and `buildDockerGpuModeCandidates()` offers CDI as the first candidate. The create-only probe (`docker create … true`) passes, but the real recreate fails because the WSL distro exposes **no usable `nvidia.com/gpu` spec**. The Docker Desktop WSL status was detected at preflight but never reached the mode selector — `selectDockerGpuPatchMode` only received `{image, device, backend}`. PR NVIDIA#5198 (which closed NVIDIA#5180) added the CDI-injection failure classification, the `--no-gpu` recovery hint, and the warning that `NEMOCLAW_DOCKER_GPU_PATCH=0` is ignored on this runtime — but it did not change mode selection. This is the unaddressed root cause. ## Fix Thread the existing Docker Desktop WSL detection (`isDockerDesktopWslRuntime()`, already used to gate the patch) through `selectDockerGpuPatchMode` into `buildDockerGpuModeCandidates`, and skip the CDI candidate when on Docker Desktop WSL so the patch uses `--gpus all` — the path preflight already commits to. - Native Docker-CDI hosts are **unaffected**: they still prefer CDI, preserving the gateway supervisor-wiring contract from NVIDIA#4948. - The flag is resolved via the cached detector in `docker-gpu-sandbox-create.ts`, so no change to `onboard.ts` and no extra `docker info` calls. ## Testing - New unit tests in `docker-gpu-patch-wsl.test.ts`: CDI is skipped (first candidate is `--gpus all`) when `dockerDesktopWsl` is true even with CDI advertised, and CDI is still preferred otherwise. - `tsc -p tsconfig.src.json` clean; GPU-patch suites pass (remaining failures are pre-existing Windows-only `/etc/cdi` path tests, identical on `main`). ## Notes / follow-up - This step is only reached after the `[2/8]` gateway-bind issue (NVIDIA#5513, fix in NVIDIA#5534). - Separate latent bug still open: on an **early** patch failure the original sandbox is already renamed to `*-nemoclaw-gpu-backup-<timestamp>` before container creation, and only the new container is removed — leaving an orphan backup. Happy to follow up with a focused PR for that cleanup. Fixes NVIDIA#5512. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Enhanced GPU configuration for Docker Desktop on Windows Subsystem for Linux (WSL). The system now properly detects WSL runtime environments and automatically selects GPU acceleration modes that work reliably on Docker Desktop WSL, avoiding GPU modes that may not be available or incompatible within that specific environment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com>
…NVIDIA#5512) (NVIDIA#5541) ## Summary Follow-up to NVIDIA#5537 addressing the orphan-backup symptom in NVIDIA#5512. When the Docker GPU patch's recreate `docker run` fails **after** the original sandbox was already renamed to `*-nemoclaw-gpu-backup-<timestamp>`, the early-failure path removed only the failed *new* container and left the backup orphaned — stranding the sandbox with no live original, and colliding with `*-nemoclaw-gpu-backup-*` on the next retry (as reported in NVIDIA#5512). The supervisor-reconnect failure path already rolls back to the backup; this path didn't. ## Fix Reuse the existing rollback primitive (`rollbackToBackupContainer`) on the early-failure path: remove the failed new container, rename the backup back to the original name, and start it — restoring the pre-patch sandbox instead of leaking a backup container. - Adds `rollbackDockerGpuPatchOnRecreateFailure(refs, deps)` to `docker-gpu-patch-finalize.ts`, which resolves the real `docker start` / `docker rename` defaults (the recreate call path only carries a deps subset, so `dockerStart` would otherwise be unset). - Records `context.rolledBack` for failure diagnostics, matching the reconnect-failure path. - No `onboard.ts` change; all edits are under `src/lib/onboard/`. ## Testing - New composed test in `docker-gpu-patch-rollback.test.ts`: when `dockerRunDetached` fails, the backup is renamed back to the original and started, and is never left as an orphaned container. - `tsc -p tsconfig.src.json` clean; rollback / finalize / sandbox-create suites pass (18/18). ## Relationship to the WSL Docker Desktop chain - NVIDIA#5534 — gateway bind at `[2/8]` - NVIDIA#5536 — gateway cleanup on probe failure - NVIDIA#5537 — skip CDI GPU mode at `[6/8]` (makes the patch succeed on Docker Desktop WSL, so this early-failure path is no longer hit there) - this PR — restore the pre-patch sandbox for any *other* early GPU-recreate failure Refs NVIDIA#5512. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved recovery for Docker GPU patch recreation failures: if the GPU-enabled recreate step fails after the original container is renamed, the system now performs a reliable rollback to the pre-patch sandbox state and cleans up the failed recreate attempt. * **Tests** * Added a rollback-path test for a recreate-phase `docker run --detach` failure, verifying restoration of the original container name, restart behavior, and correct cleanup (including ignoring the failed recreated container). <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> --------- Signed-off-by: Abhimanyu Kumar <abhimanyukumar7290@gmail.com> Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
Summary
On Docker Desktop + WSL2 with an NVIDIA GPU, onboard's
[6/8]Docker GPU patch recreates the sandbox container with--device nvidia.com/gpu=all(CDI syntax) and fails:even though preflight already logs that it will use the
--gpuscompatibility path. The only workaround today is--no-gpu/NEMOCLAW_SANDBOX_GPU=0, which disables GPU entirely.Root cause
Docker Desktop advertises CDI spec directories, so
dockerReportsNvidiaCdiDevices()returns true andbuildDockerGpuModeCandidates()offers CDI as the first candidate. The create-only probe (docker create … true) passes, but the real recreate fails because the WSL distro exposes no usablenvidia.com/gpuspec. The Docker Desktop WSL status was detected at preflight but never reached the mode selector —selectDockerGpuPatchModeonly received{image, device, backend}.PR #5198 (which closed #5180) added the CDI-injection failure classification, the
--no-gpurecovery hint, and the warning thatNEMOCLAW_DOCKER_GPU_PATCH=0is ignored on this runtime — but it did not change mode selection. This is the unaddressed root cause.Fix
Thread the existing Docker Desktop WSL detection (
isDockerDesktopWslRuntime(), already used to gate the patch) throughselectDockerGpuPatchModeintobuildDockerGpuModeCandidates, and skip the CDI candidate when on Docker Desktop WSL so the patch uses--gpus all— the path preflight already commits to.docker-gpu-sandbox-create.ts, so no change toonboard.tsand no extradocker infocalls.Testing
docker-gpu-patch-wsl.test.ts: CDI is skipped (first candidate is--gpus all) whendockerDesktopWslis true even with CDI advertised, and CDI is still preferred otherwise.tsc -p tsconfig.src.jsonclean; GPU-patch suites pass (remaining failures are pre-existing Windows-only/etc/cdipath tests, identical onmain).Notes / follow-up
[2/8]gateway-bind issue ([WSL2][Policy&Network] OpenShell gateway unreachable from sandbox containers on Docker Desktop WSL (binds to 127.0.0.1:8080) #5513, fix in fix(onboard): bind gateway to 0.0.0.0 on Docker Desktop WSL (#5513) #5534).*-nemoclaw-gpu-backup-<timestamp>before container creation, and only the new container is removed — leaving an orphan backup. Happy to follow up with a focused PR for that cleanup.Fixes #5512.
Summary by CodeRabbit
Signed-off-by: Abhimanyu Kumar abhimanyukumar7290@gmail.com