fix(hardware): nvidia-smi probe backs off instead of giving up for good #62
thisilike
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/nvidia-smi-backoff"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
startGpuCollectorsgated the NVIDIA probe onnvidiaFailures < 3. The counter only resets on a success, and once it reaches 3 the probe never runs again, so there is no success to reset it: three consecutive failures end NVIDIA sampling until the container is restarted. The comment above it said the opposite ("so a transient error doesn't kill the metrics until restart").Three ticks is 30 s at the default interval. Anything that takes
nvidia-smiaway for longer than that is enough: a driver upgrade or a module reload on the host, a GPU that falls off the bus and is rescanned, anvidia-smithat hangs into the 8 s timeout three times under load.Found by reading the code, not from a failure in production. It came up while investigating an unattended driver upgrade on epyclab (610.43.02 → 610.57.04) where the kernel module was unloaded for the upgrade. The probe did not fail there — the opsdeck log shows it running every 10 s throughout, with no warning — because of the second point under "Not changed". Had the module actually stayed unloaded for the ~80 s the upgrade took, the metrics would have been gone until the next restart.
Change
nvidiaBackoff(intervalMs)(pure) replaces the bare counter:NVIDIA_BACKOFF_AFTER(3) consecutive failures: probe every tick, as beforeNVIDIA_RETRY_MAX_MS(60 s), never below two. At the default interval that is 20 s, 40 s, then one attempt per minute. A probe that merely timed out under load is back within a few ticks; a host that keeps failing settles at one attempt per minute. AnINTERVAL_MSabove 60 s does not turn the backoff into a no-op.due()is called exactly once per tick and counts down. No clock is involved, so neither an NTP step nor a resume from suspend can move the next attempt, and there is no drift against thesetIntervalgrid. A tick the scheduler skips for overlap is not counted and lengthens that gap by one.nvidia-smi is answering againis logged at infoMissing binary. If
ctx.execthrowsDeno.errors.NotFound, the container has nonvidia-smiat all. Only the NVIDIA container toolkit puts it there, at container creation, so nothing short of recreating the container changes that: the probe stops for the lifetime of the process (stop()). It logs a warning if an NVIDIA DRM card is present (the "run the container with the NVIDIA runtime" hint), otherwise one debug line. This is every VM and GPU-less host, and they are back to the old cost: one spawn, not one a minute. A driver upgrade does not hit this path — the bind-mounted binary stays and fails with a non-zero exit, which gets the backoff.Any other failure (non-zero exit, timeout) backs off and keeps retrying. The runtime warning fires once per outage (
failures === NVIDIA_BACKOFF_AFTER, NVIDIA card present). Each retry produces two log lines: the unconditionalexecline at info that everyctx.execcall emits (server/src/util/exec.ts, kept as the audit trail for host commands), and the failure itself at debug. On a host wherenvidia-smiexists but keeps failing that is ~1440 info lines a day, next to the ~8640 a working probe already produces.Not changed
Stale samples are not cleared while the probe is failing; the last written values stay as they did before. With the backoff that means frozen values for the length of the outage with nothing marking them stale. What the GPU page should show during an outage is a separate decision: #70.
The probe is gated on a one-time DRM snapshot (
detectGpusruns once). If the process starts while the nvidia modules are unloaded and the host has another DRM card (the ASPEED BMC on a server board), the probe never runs and the backoff never ticks. That predates this PR and is the other half of the same problem, but fixing it changes behaviour on non-NVIDIA hosts: #69.Worth knowing, though not something this PR addresses: on a host where the nvidia module is unloaded on purpose, the probe is what loads it again — the container is privileged, and opening
/dev/nvidiactltriggers the kernel'schar-major-195-*alias. On epyclab the probe at 04:16:43.249 lines up with the kernel loading the old module at 04:16:43.45, two seconds after the upgrade script had unloaded it, which is what broke that upgrade. The script now blocks module autoload and stops the opsdeck container for the duration, so nothing is needed here — but it does mean the container will see exactly the kind of outage this PR is about on every future driver upgrade.Tests
gpu_test.ts, againstnvidiaBackoffdirectly: per-tick probing until the threshold; the exact probed ticks over 40 all-failing ticks (0, 1, 2, 4, 8, 14, 20, …— gaps 2, 4, then 6 at the cap); an interval above the cap still waiting two ticks; recovery reported once with the count reset;stop()ending probing.Against
startGpuCollectorswith a fakectx(no DRM cards, scriptedexec, ticks driven by hand):NotFoundon the first tick:execcalled once in 50 ticks, a single debug line0, 1, 2, 4, 8, 14, 15, …,nvidia-smi is answering againlogged once, no warning without an NVIDIA card,nv0published and samples writtenNot exercised against a real GPU outage.
Review round 1 (
ae7f323)performance.now()default (since replaced by tick counting, round 2)execinfo line stays as the audit trailok()/fail()mutate-and-return: left as isReview round 2 (
98be265)NotFoundstops the probe for good (stop(), kept separate so the backoff stays error-agnostic); other errors back offstartGpuCollectorstests with a fakectx🤖 Generated with Claude Code
Review of
ae7f323. I re-ran everything rather than taking the description's word for it.Verified
nvidiaFailures < 3with a reset only inside thetrymeans the gate closes permanently. The old comment did claim the opposite.deno test --allow-read packages/modules/hardware/backend/gpu_test.ts→ 11 passed;deno lint,deno fmt --check,deno task check:backendall clean, run from a clean worktree of the branch.mod.ts:20,INTERVAL_MS ?? 10_000. Correct.failuresonly resets inok(), sofailures === NVIDIA_BACKOFF_AFTERis reached once per outage.due()/fail():scheduler.ts:25skips overlapping ticks.1. The backoff cannot run on the host state this PR is about (
gpu.ts:366)cardsis assigned exactly once (gpu.ts:347);detectGpusis never called again, and it reads/sys/class/drm/card*, where an NVIDIA node exists only whilenvidia-drmis bound.So if the process starts while the NVIDIA modules are unloaded and the host has any other DRM card — on a server board that is normally the ASPEED BMC (
0x1a03, mapped to"other", bound byast) — thencards.length !== 0and nothing incardsisnvidia.wantNvidiais false for the entire lifetime of the process: nvidia-smi is never invoked once, the backoff never ticks, and the GPU metrics never appear. That is the same "gone until restart" outcome, except a restart does not fix it either.The PR body makes this more reachable, not less: the upgrade script now stops the container for the duration, so whether GPU metrics exist afterwards depends on whether the module is back before the container is. The same hole applies to a compute-only host that never loads
nvidia-drm.This predates the PR. I am raising it here because the backoff is what makes cheap perpetual probing affordable, which removes the reason the DRM snapshot gate existed. Worth either a driver-independent presence check (
/sys/bus/pci/devices/*/vendor == 0x10dewith PCI class0x0300) or a periodic re-detect. Either one changes behaviour on non-NVIDIA hosts, so it is a separate decision, but without it the fix only covers outages that start after a successful detection.2. The retry deadline is wall-clock (
gpu.ts:213,gpu.ts:229)nvidiaBackoff(now: () => number = Date.now)withretryAt = now() + NVIDIA_RETRY_MS.Date.now()is not monotonic. A backward NTP step (container up before the first sync), a host resuming from suspend, or a VM migration pushesretryAtinto the future by however far the clock moved, and the probe stays quiet for exactly that long — the failure mode this PR removes, reintroduced through a different door.performance.now()is monotonic and needs no permission in Deno 2. Since the clock is already injected, only the default changes and no test moves:I confirmed the suite, lint, fmt and
check:backendstay green with that change.3. "the retries log at debug like every other failure" is not accurate
execCommandlogs unconditionally at info before spawning (exec.ts:27:log.info("exec", { cmd, args })), and the default minimum level isinfo(logger.ts:13). Every retry emits an INFO line in addition to the debug failure line. On a host without the NVIDIA runtime that is ~1440 info lines a day where the current code emits none. Small next to the ~8640/day a working probe already produces, but the description says something the code does not do.4. The retry window is unrelated to the tick interval (
gpu.ts:204)The threshold counts failures, the window is a fixed 60 s. The scenario the description names — nvidia-smi hitting the 8 s timeout three times under load — now costs up to 60 s without samples, against 10 s while the count is still below the threshold. Still strictly better than the current "never again", but the constant is arbitrary:
Math.max(NVIDIA_RETRY_MS, intervalMs), or an exponential window starting at2 * intervalMsand capped at 60 s, ties it to the collector it paces. Note also thatINTERVAL_MS > 60_000makes the backoff a no-op, which is harmless but unintended.Nits
ok()andfail()both mutate and return, soif (nvidia.ok())reads like a query at the call site. The doc comments carry it; not worth churn.Net: the change is correct and the tests are honest. #2 is a one-line fix I would take before merging; #1 is the larger half of the same problem and should at least get an issue.
@thisilike addressed in
98be265, description updated.performance.now()now; initial deadline-Infinityso a clock starting near 0 is due at once.max(60 s, 2 ticks). Default interval: 20 s, 40 s, then once a minute.INTERVAL_MS > 60_000no longer makes it a no-op. Tests cover the doubling, the cap and the above-cap interval.execinfo line stays since it is the audit trail for host commands.Re-review requested.
Re-review at
98be265.Verified
performance.now()default with a-Infinityinitial deadline, window from two ticks doubling tomax(60 s, 2 ticks), description corrected, #69 / #70 filed. All as described.gpu_test.ts13 passed;deno lint,deno fmt --checkonhardware/backend/clean.2 ** retriesoverflowing toInfinityafter a long outage is harmless:Math.min(cap, Infinity)is the cap.Blocking
1. Every host without a GPU now runs nvidia-smi once a minute, forever (
gpu.ts:380).wantNvidiais also true whencards.length === 0, which is every VM, most VPSes and any host without/sys/class/drm/card*. That is the most common deployment, not the "NVIDIA card but no runtime" case the description prices in. Before: threeexecattempts in the process lifetime. After: 1440execINFO lines a day plus 1440 spawns, on hosts that have nothing to probe.For most of these hosts the failure is permanent.
ctx.execruns inside the container (host.ts:553, plainDeno.Command), andnvidia-smiis only there if the NVIDIA container toolkit injected it when the container was created. If the binary is missing,Deno.CommandthrowsDeno.errors.NotFound, and that cannot change until the container is recreated. A driver upgrade does not cause it either: the bind-mounted binary keeps its inode, so it fails with a non-zero exit (NVML version mismatch or no devices), not with NotFound.So the distinction the backoff needs is already in the error type.
NotFoundmeans give up for the lifetime of the process (log once at debug). Anything else is transient and gets the backoff. This keeps the fix for the outage this PR is about and restores the old cost on GPU-less hosts. Add a test that feedsfail()a NotFound. Or, if the API stays error-agnostic, add a separatestop().Should fix
2. The real gaps are one tick longer than documented and tested.
fail()anchorsretryAtat the time the failed exec returned, but the scheduler fires on a fixedsetIntervalgrid (scheduler.ts:40). The tick that is nominally due runs atstart + W + timer lateness, and the deadline isstart + exec duration + W. Exec duration (a process spawn, or the 8 s timeout in the "hangs under load" case) is almost always longer than timer lateness, so that tick misses by a few ms and the probe waits one more tick.I checked this against the real scheduler shape:
setIntervalat 100 ms, 5 ms of simulated exec cost, the PR'snvidiaBackoff. The probe ran at ticks1, 2, 3, 6, 11, 20, 36, so the gaps after the threshold were 3, 5, 9, 16 ticks instead of 2, 4, 8, 16. At the default interval that is 30 s, 50 s, then 70 s, not 20 s, 40 s, then 60 s. The fake-clock tests can't see this becausefail()anddue()read the same instant.Fix: anchor the deadline to the tick, not the end of the exec. For example,
due()stores thenow()it read andfail()computesretryAtfrom that value. Or count skipped ticks instead of milliseconds, which also removes the clock question entirely.Nits
startGpuCollectors:due(),ok()andfail()are only covered as a pure unit. A test with a fakectx.execthat fails N times and then succeeds would have caught #2 and would cover the recovery log line.Net: the backoff logic is correct and the tests are honest. #1 changes what every GPU-less install pays, so I'd like that fixed before it lands. #2 is small.
Re-review at
0e1a7b7.Verified
ctx.execisexecCommand→ plainDeno.Command(...).output()with no catch, so a missing binary reaches the collector as-is. Checked on Deno directly: missing binary →Deno.errors.NotFound; the 8 s timeout resolves with code 143 /SIGTERM(so it hits theout.code !== 0throw and gets the backoff); a non-executable file →PermissionDenied(backoff, not stop). The image doesn't shipnvidia-smiitself (only the compose comment for the toolkit), soNotFoundreally does mean "no toolkit injection" for the life of the container.nvidia.due()is the left operand ofwantNvidia, so it is evaluated exactly once per tick regardless of the DRM gate. Gap math:skip = min(capTicks, 2·2^retries) − 1gives 2, 4, 6, 6, … at 10 s, matching0, 1, 2, 4, 8, 14, 20, …;2 ** retriesgoing toInfinitystill clamps to the cap.capTicks = max(2, ceil(60 s / interval))rounds up (e.g. 9 ticks = 63 s at 7 s), which is what "ticks spanning" says.ctxdrives the realstartGpuCollectors, the last scripted step repeats, and the NotFound case asserts oneexecin 50 ticks.gpu_test.ts15 passed;deno lint,deno fmt --checkonhardware/backend/,deno task check:backendclean;deno task test604 passed, 1 ignored — from a clean worktree of the branch.Please fix before merging
The backoff-path warning now gives the wrong hint (
gpu.ts:455). That branch is only reached when the binary exists, i.e. the toolkit already injected it. So on an NVIDIA host during a driver upgrade — the scenario this PR is for — the third failure logs "run the container with the NVIDIA runtime (--gpus all, …)", which is not the problem. Something likenvidia-smi failing, backing offwith the error, and keephintfor theNotFoundwarning only. The comment above it ("or no NVIDIA runtime") is stale for the same reason.Small, but it is the exact log line an operator reads during the outage this PR handles, and it points them at the wrong fix. Approving so this doesn't need another round, but please push that change before you merge.
Nit (optional)
nvidiaBackoffdoc comment explains why a millisecond deadline was dropped. That is review history; it reads better in the commit message, and the comment can just say the gap is counted in ticks anddue()must be called once per tick.Net: both round-2 points are fixed properly and the tests cover the wiring now. Approved on the condition that the warning hint above is fixed before merge; no re-review needed for that.