diff --git a/src/lib/onboard/docker-gpu-patch-mode-selection.test.ts b/src/lib/onboard/docker-gpu-patch-mode-selection.test.ts new file mode 100644 index 00000000000..182278e6ddf --- /dev/null +++ b/src/lib/onboard/docker-gpu-patch-mode-selection.test.ts @@ -0,0 +1,147 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it, vi } from "vitest"; + +import { + buildDockerGpuModeCandidates, + type DockerContainerInspect, + type DockerGpuPatchDeps, + recreateOpenShellDockerSandboxWithGpu, + selectDockerGpuPatchMode, +} from "../../../dist/lib/onboard/docker-gpu-patch"; + +// Deps that surface an NVIDIA CDI spec at /etc/cdi/nvidia.yaml so +// `dockerReportsNvidiaCdiDevices` reports CDI as available (the #4948 host +// shape). Probe behavior is supplied per-test via `dockerRun`. +function cdiHostDeps(): DockerGpuPatchDeps { + return { + dockerCapture: vi.fn(() => JSON.stringify(["/etc/cdi"])), + readDir: (dir: string) => (dir === "/etc/cdi" ? ["nvidia.yaml"] : null), + readFile: (file: string) => + file === "/etc/cdi/nvidia.yaml" + ? "cdiVersion: 0.6.0\nkind: nvidia.com/gpu\ndevices:\n - name: all\n" + : null, + dockerRm: vi.fn(() => ({ status: 0 })), + }; +} + +function inspectFixture(): DockerContainerInspect { + return { + Id: "old-container-id", + Name: "/openshell-alpha", + Config: { + Image: "openshell/sandbox:abc", + Labels: { + "openshell.ai/managed-by": "openshell", + "openshell.ai/sandbox-name": "alpha", + }, + }, + HostConfig: { NetworkMode: "openshell-docker" }, + }; +} + +describe("docker-gpu-patch CDI-first mode selection (#4948)", () => { + it("prefers CDI over --gpus when the host advertises an NVIDIA CDI spec", () => { + // Repro for #4948: on a Docker-CDI GPU host (e.g. Ubuntu 24.04 with + // /etc/cdi/nvidia.yaml), `docker create --gpus all` is *accepted* so the + // create-only probe passes and `--gpus all` was selected. OpenShell's + // gateway injects GPUs via the CDI spec, so the legacy --gpus injection + // path diverges from how the supervisor expects the container to be wired + // and never reconnects. When a CDI spec is present we must select the CDI + // mode (`--device nvidia.com/gpu=all`) ahead of --gpus. + expect(buildDockerGpuModeCandidates("all", { cdiAvailable: true }).map((m) => m.kind)).toEqual([ + "cdi", + "gpus", + "nvidia-runtime", + ]); + + // Every probe (including --gpus) would succeed on this host, yet CDI wins. + const dockerRun = vi.fn(() => ({ status: 0, stdout: "probe-id" })); + const selected = selectDockerGpuPatchMode( + { image: "openshell/sandbox:abc" }, + { ...cdiHostDeps(), dockerRun }, + ); + + expect(selected.mode?.kind).toBe("cdi"); + expect(selected.attempts[0].mode.kind).toBe("cdi"); + }); + + it("falls back to --gpus when the CDI probe fails on a CDI host", () => { + // CDI is preferred first, but if `docker create --device nvidia.com/gpu=all` + // is rejected the selection must continue down the fallback chain rather + // than leaving the host with no usable GPU mode. + const dockerRun = vi.fn((args: readonly string[]) => + args.includes("--device") + ? { status: 1, stderr: "could not select device driver" } + : { status: 0, stdout: "probe-id" }, + ); + const selected = selectDockerGpuPatchMode( + { image: "openshell/sandbox:abc" }, + { ...cdiHostDeps(), dockerRun }, + ); + + expect(selected.mode?.kind).toBe("gpus"); + expect(selected.attempts.map((attempt) => attempt.mode.kind)).toEqual(["cdi", "gpus"]); + expect(selected.attempts[0].ok).toBe(false); + }); + + it("falls back to the NVIDIA runtime when both CDI and --gpus probes fail", () => { + const dockerRun = vi.fn((args: readonly string[]) => + args.includes("--device") || args.includes("--gpus") + ? { status: 1, stderr: "probe rejected" } + : { status: 0, stdout: "probe-id" }, + ); + const selected = selectDockerGpuPatchMode( + { image: "openshell/sandbox:abc" }, + { ...cdiHostDeps(), dockerRun }, + ); + + expect(selected.mode?.kind).toBe("nvidia-runtime"); + expect(selected.attempts.map((attempt) => attempt.mode.kind)).toEqual([ + "cdi", + "gpus", + "nvidia-runtime", + ]); + }); + + it("passes the CDI --device flag to docker run when recreating on a CDI host", () => { + // Proves the selected CDI mode propagates into the actual recreate command + // (`dockerRunDetached`), not just the selection result. This is the create + // option that the issue's product log surfaces as `patched_create_option`. + const dockerCapture = vi.fn((args: readonly string[]) => { + if (args[0] === "ps") return "old-container-id\n"; + if (args[0] === "inspect") return JSON.stringify([inspectFixture()]); + if (args[0] === "info") return JSON.stringify(["/etc/cdi"]); + return ""; + }); + const dockerRunDetached = vi.fn(() => ({ status: 0, stdout: "new-container-id\n" })); + const host = cdiHostDeps(); + + const result = recreateOpenShellDockerSandboxWithGpu( + { sandboxName: "alpha", timeoutSecs: 1 }, + { + dockerCapture, + readDir: host.readDir, + readFile: host.readFile, + dockerRun: vi.fn(() => ({ status: 0, stdout: "probe-id\n" })), + dockerRunDetached, + dockerRename: vi.fn(() => ({ status: 0 })), + dockerStop: vi.fn(() => ({ status: 0 })), + dockerRm: vi.fn(() => ({ status: 0 })), + runOpenshell: vi.fn(() => ({ status: 0 })), + sleep: vi.fn(), + now: () => new Date("2026-05-12T00:00:00Z"), + }, + ); + + expect(result.mode.kind).toBe("cdi"); + expect(dockerRunDetached).toHaveBeenCalledWith( + expect.arrayContaining(["--name", "openshell-alpha", "--device", "nvidia.com/gpu=all"]), + expect.objectContaining({ ignoreError: true }), + ); + // The legacy --gpus flag must NOT appear on a CDI host recreate. + const detachedArgs = (dockerRunDetached.mock.calls[0] as unknown[])[0] as string[]; + expect(detachedArgs).not.toContain("--gpus"); + }); +}); diff --git a/src/lib/onboard/docker-gpu-patch.test.ts b/src/lib/onboard/docker-gpu-patch.test.ts index f0b3b1b974d..b52ade24a9e 100644 --- a/src/lib/onboard/docker-gpu-patch.test.ts +++ b/src/lib/onboard/docker-gpu-patch.test.ts @@ -385,6 +385,8 @@ describe("docker-gpu-patch", () => { dockerCapture: vi.fn(() => ""), dockerRun, dockerRm: vi.fn(() => ({ status: 0 })), + readDir: vi.fn(() => null), + readFile: vi.fn(() => null), }, ); @@ -423,14 +425,16 @@ describe("docker-gpu-patch", () => { expect(dockerCapture).not.toHaveBeenCalled(); }); - it("tries CDI only when Docker reports readable NVIDIA CDI specs", () => { + it("prefers CDI only when Docker reports readable NVIDIA CDI specs", () => { expect(buildDockerGpuModeCandidates("all", { cdiAvailable: false }).map((m) => m.kind)).toEqual( ["gpus", "nvidia-runtime"], ); + // When a CDI spec is present, CDI is preferred first (see #4948); --gpus + // and the NVIDIA runtime remain as fallbacks. expect(buildDockerGpuModeCandidates("all", { cdiAvailable: true }).map((m) => m.kind)).toEqual([ + "cdi", "gpus", "nvidia-runtime", - "cdi", ]); const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-docker-cdi-")); @@ -454,8 +458,7 @@ describe("docker-gpu-patch", () => { // case: `docker info` returns an empty CDISpecDirs list, but Docker is // still reading specs from its well-known default /etc/cdi. The detector // should mirror Docker's behavior and surface cdi as available so the - // candidate list keeps `cdi` for fallback after `--gpus all` trips the - // AMD-CDI bug. + // candidate list prefers `cdi` ahead of `--gpus all` on CDI hosts (#4948). const readDir = vi.fn((dirPath: string) => (dirPath === "/etc/cdi" ? ["nvidia.yaml"] : null)); const readFile = vi.fn((filePath: string) => filePath === "/etc/cdi/nvidia.yaml" @@ -535,6 +538,8 @@ describe("docker-gpu-patch", () => { runOpenshell, sleep: vi.fn(), now: () => new Date("2026-05-12T00:00:00Z"), + readDir: vi.fn(() => null), + readFile: vi.fn(() => null), }, ); diff --git a/src/lib/onboard/docker-gpu-patch.ts b/src/lib/onboard/docker-gpu-patch.ts index a3e3e75ac9f..c32551004bf 100644 --- a/src/lib/onboard/docker-gpu-patch.ts +++ b/src/lib/onboard/docker-gpu-patch.ts @@ -16,13 +16,15 @@ import { } from "../adapters/docker"; import { reconcileSupervisorReconnect } from "./docker-gpu-patch-finalize"; import { - type DockerGpuSupervisorReconnectDeps, DOCKER_GPU_SUPERVISOR_RECONNECT_ERROR_DEBOUNCE_ENV, DOCKER_GPU_SUPERVISOR_RECONNECT_TIMEOUT_ENV, + type DockerGpuSupervisorReconnectDeps, getDockerGpuSupervisorReconnectErrorDebouncePolls, getDockerGpuSupervisorReconnectTimeoutSecs, waitForOpenShellSupervisorReconnect, } from "./docker-gpu-supervisor-reconnect"; + +export type { DockerGpuSupervisorReconnectDeps }; export { DOCKER_GPU_SUPERVISOR_RECONNECT_ERROR_DEBOUNCE_ENV, DOCKER_GPU_SUPERVISOR_RECONNECT_TIMEOUT_ENV, @@ -30,7 +32,6 @@ export { getDockerGpuSupervisorReconnectTimeoutSecs, waitForOpenShellSupervisorReconnect, }; -export type { DockerGpuSupervisorReconnectDeps }; export const OPENSHELL_MANAGED_BY_LABEL = "openshell.ai/managed-by"; export const OPENSHELL_MANAGED_BY_VALUE = "openshell"; @@ -500,11 +501,18 @@ export function buildDockerGpuModeCandidates( if (options.backend === "jetson") { return [buildDockerGpuMode("nvidia-runtime", device, { backend: "jetson" })]; } - const candidates = [ - buildDockerGpuMode("gpus", device), - buildDockerGpuMode("nvidia-runtime", device), - ]; + // When the host advertises an NVIDIA CDI spec, prefer the CDI mode + // (`--device nvidia.com/gpu=all`) ahead of --gpus. OpenShell's gateway owns + // supervisor GPU injection and wires Docker-CDI hosts from that spec; this + // NemoClaw patch only chooses the recreate mode while matching that source + // boundary. On Docker-CDI hosts `docker create --gpus all` is accepted (the + // create-only probe passes), but the legacy --gpus injection diverges from + // gateway wiring and the supervisor never reconnects (#4948). Keep --gpus + // and the NVIDIA runtime as fallbacks until OpenShell exposes an + // authoritative GPU mode contract that can replace CDI-spec probing. + const candidates: DockerGpuPatchMode[] = []; if (options.cdiAvailable) candidates.push(buildDockerGpuMode("cdi", device)); + candidates.push(buildDockerGpuMode("gpus", device), buildDockerGpuMode("nvidia-runtime", device)); return candidates; } diff --git a/src/lib/onboard/docker-gpu-supervisor-reconnect.test.ts b/src/lib/onboard/docker-gpu-supervisor-reconnect.test.ts index 33995ab2c80..b9b50907ff0 100644 --- a/src/lib/onboard/docker-gpu-supervisor-reconnect.test.ts +++ b/src/lib/onboard/docker-gpu-supervisor-reconnect.test.ts @@ -96,8 +96,34 @@ describe("docker-gpu-supervisor-reconnect Error-phase debounce", () => { expect(runOpenshell).toHaveBeenCalledTimes(6); }); - it("defaults the debounce to 15 polls and honors the env override", () => { - expect(getDockerGpuSupervisorReconnectErrorDebouncePolls({})).toBe(15); + it("absorbs a Docker-CDI Error phase longer than the old 30s window", () => { + // #4948 runtime validation on the Docker-CDI GPU runner showed the + // sandbox-list row can remain Error for roughly a minute after the CDI + // recreate (`--device nvidia.com/gpu=all`) while the supervisor is still + // reconnecting. The default debounce must therefore outlive the old + // 15-poll / ~30s fast-fail window. + let polls = 0; + const runOpenshell = vi.fn(() => { + polls += 1; + return polls <= 30 ? { status: 1, stderr: "sandbox not ready" } : { status: 0 }; + }); + const runCaptureOpenshell = vi.fn(() => + polls <= 30 ? "alpha Error 1s ago" : "alpha Ready 65s ago", + ); + const sleep = vi.fn(); + + const ok = waitForOpenShellSupervisorReconnect("alpha", 600, { + runOpenshell, + runCaptureOpenshell, + sleep, + }); + + expect(ok).toBe(true); + expect(runOpenshell).toHaveBeenCalledTimes(31); + }); + + it("defaults the debounce to 60 polls and honors the env override", () => { + expect(getDockerGpuSupervisorReconnectErrorDebouncePolls({})).toBe(60); expect( getDockerGpuSupervisorReconnectErrorDebouncePolls({ NEMOCLAW_DOCKER_GPU_SUPERVISOR_RECONNECT_ERROR_DEBOUNCE: "2", @@ -149,9 +175,9 @@ describe("docker-gpu-supervisor-reconnect Error-phase debounce", () => { }); expect(ok).toBe(false); - // Default K=15 from the env-backed helper: 15 polls + 14 sleeps before fast-fail. - expect(runOpenshell).toHaveBeenCalledTimes(15); - expect(sleep).toHaveBeenCalledTimes(14); + // Default K=60 from the env-backed helper: 60 polls + 59 sleeps before fast-fail. + expect(runOpenshell).toHaveBeenCalledTimes(60); + expect(sleep).toHaveBeenCalledTimes(59); } }); }); diff --git a/src/lib/onboard/docker-gpu-supervisor-reconnect.ts b/src/lib/onboard/docker-gpu-supervisor-reconnect.ts index 3527d087632..abd59a8c1d3 100644 --- a/src/lib/onboard/docker-gpu-supervisor-reconnect.ts +++ b/src/lib/onboard/docker-gpu-supervisor-reconnect.ts @@ -29,11 +29,11 @@ import { envInt } from "./env"; const DOCKER_GPU_PATCH_TIMEOUT_MS = 30_000; const DOCKER_GPU_SUPERVISOR_RECONNECT_MIN_SECS = 900; // Default consecutive Error-phase polls required before fast-fail. With a -// 2-second poll interval this is ~30s of sustained Error, leaving headroom -// for slower hosts (Docker Desktop on WSL2, DGX Spark cached re-onboard) -// whose sandbox list cache divergence was observed at ~12s in the original -// repro. Hosts that genuinely crashed on startup hit the rollback path in -// applyDockerGpuPatch rather than waiting out the full window. +// 2-second poll interval this is ~2 minutes of sustained Error, leaving +// headroom for Docker-CDI GPU runners whose OpenShell sandbox-list row can +// stay Error for longer than the original ~30s window while the recreated +// container is still reconnecting (#4948). Hosts that genuinely crashed on +// startup still hit the rollback path well before the full reconnect timeout. // // Alternative considered: branching on Docker State.Status + Health.Status // to keep retrying when the patched container reports Status=running plus @@ -44,10 +44,10 @@ const DOCKER_GPU_SUPERVISOR_RECONNECT_MIN_SECS = 900; // the user keeps the pre-patch CPU sandbox on reconnect failure, which a // Health-aware retry alone would not provide. If a future repro shows // Status=running + Health=starting genuinely failing reconnect after this -// 30s window, switch to a Health-aware retry, but extract Docker health +// default window, switch to a Health-aware retry, but extract Docker health // probing into a separate observation channel first rather than overloading // this one. -const DOCKER_GPU_SUPERVISOR_RECONNECT_ERROR_PHASE_DEFAULT_DEBOUNCE_POLLS = 15; +const DOCKER_GPU_SUPERVISOR_RECONNECT_ERROR_PHASE_DEFAULT_DEBOUNCE_POLLS = 60; export const DOCKER_GPU_SUPERVISOR_RECONNECT_TIMEOUT_ENV = "NEMOCLAW_DOCKER_GPU_SUPERVISOR_RECONNECT_TIMEOUT";