Retry NVML init on ERROR_LIBRARY_NOT_FOUND to fix GKE startup race - #717
CodeBuildder wants to merge 3 commits into
Conversation
…VIDIA#523) On GKE, the exporter pod can start before the GPU driver installer finishes, so nvml.Init briefly returns ERROR_LIBRARY_NOT_FOUND. This previously either crashed the pod or, with --disable-startup-validate, silently ran with no GPU metrics. Wrap NVML initialization in a bounded retry with exponential backoff and jitter, retrying only on that specific transient error so a permanent failure still fails fast. Retry attempts/backoff are configurable via CLI flags/env vars following the existing disable-startup-validate pattern, with sane hardcoded defaults (5 attempts, 2s-20s backoff). Signed-off-by: kaushik-kumaran <kaushik.kumaran@ibm.com>
… overflow-safe Address review findings from the initial retry implementation: - InitializeWithRetry now takes a context.Context and interrupts the backoff sleep on cancellation, returning promptly instead of ignoring a shutdown signal for up to the full retry window. The pod-startup call in runDCGMExporter now passes lifecycleCtx. - attempts <= 0 (e.g. from a misconfigured CLI flag/env var) is clamped to 1 so at least one real init attempt always happens, instead of skipping initialization and returning a malformed error. - backoffDuration now computes the exponential doubling with a capped loop instead of a bit shift, so an unbounded, user-configurable attempts value can no longer overflow int64 and wrap into a bogus small duration that would defeat the maxWait cap. - The GPU bind/unbind hot-reload path now calls InitializeWithRetry with a single attempt rather than the full startup retry/backoff: that path already treats a failed reinit as an expected outcome when the GPU is unbound, and it runs on the serial reload coordinator, so retrying there would block /metrics recovery and any queued reload for the length of the backoff. Signed-off-by: kaushik-kumaran <kaushik.kumaran@ibm.com>
|
Confirmed no overlap with #578, that one's entirely in the DCGM collector init path (collector_factory.go), while this PR is scoped to NVML init specifically. Different subsystems, no shared files or functions, and neither one touches or redefines DisableStartupValidate, they both just respect it as the existing escape hatch. Also rebased against current main to be safe, it came back as a no-op, so this is already up to date and there's nothing new to reconcile. Let me know if there's anything else you'd like me to check before this gets reviewed. |
Four tests in this package called the real Initialize() and asserted it succeeds unconditionally, with no guard for an environment that has no real NVML library. Two sibling tests already handle this correctly, by checking the real Initialize() result and calling t.Skip when it fails. The four that didn't would either fail hard after burning through the full real retry backoff, or, in one case, needed to be handled a third way I want to call out separately below. Test_newNVMLProvider only exercises newNVMLProvider() and compares the resulting struct; it never calls another real NVML function. That one gets a proper mock instead of a skip, by stubbing nvmlInitFunc to succeed the way retry_test.go's tests already do, so it runs (and actually verifies something) in any environment. I first tried the same mock-and-proceed approach for the other three, since it seemed like the more thorough fix; verifying it caught the mistake. Those three go on to call real NVML functions beyond just Init: GetMIGDeviceInfoByID (nvml.DeviceGetHandleByUUID) and Cleanup (nvml.Shutdown). Those symbols only resolve when the real library is actually loaded, so stubbing just the init call doesn't help; the test binary crashes with a dynamic symbol lookup error instead of a Go error. Those three get the same skip guard as their two siblings. Verified in a plain golang:1.26 container with no NVML library present, which is what exposed this: 23 passed, 5 skipped, 0 failed, 0 crashed. Signed-off-by: kaushik-kumaran <kaushik.kumaran@ibm.com>
|
Update: I went back and fixed the pre-existing test gap I flagged in the PR description above, instead of just leaving a note about it. What I found: four tests in What I did about it:
Verified in a plain I'll update the stale note in the description above since it no longer applies. |
822aa1a to
5474400
Compare
|
This should be fixed in 4.8.4. Closing. Please re-open or open a new issue or PR if you find issues. |
2 participants
Summary
nvml.Initbriefly returnsERROR_LIBRARY_NOT_FOUND. This previously either crashed the pod or, with--disable-startup-validate, silently ran with no GPU metrics.nvmlprovider.InitializeWithRetryretries only on that specific transient error (any other NVML error fails immediately), using exponential backoff (base, 2x, 4x... capped at max-wait) plus up to 30% jitter so pods restarting together after a node reboot don't retry in lockstep against the same node.Initialize()now callsInitializeWithRetrywith sane defaults (5 attempts, 2s base wait, 20s max wait).--nvml-init-retry-attempts/--nvml-init-retry-base-wait/--nvml-init-retry-max-waitflags (and matching env vars), following the existing--disable-startup-validateconvention.nvml.Initis now indirected through a package-levelnvmlInitFuncvar so it can be faked in tests without real GPU hardware.Hardening from self-review
InitializeWithRetrytakes acontext.Contextand interrupts the backoff sleep on cancellation, so a shutdown signal during a slow retry isn't ignored until the retry window is exhausted (the pod-startup call site passeslifecycleCtx).attempts <= 0(e.g. a misconfigured flag/env var) is clamped to 1 so at least one real init attempt always happens.backoffDuration's exponential doubling is now computed with a capped loop instead of a bit shift, so an unbounded, user-configurableattemptsvalue can't overflowint64into a bogus small duration that defeats themaxWaitcap.InitializeWithRetrywith a single attempt instead of the full startup backoff: that path already treats a failed reinit as expected when the GPU is unbound, and since it runs on the serial reload coordinator, retrying there would block/metricsrecovery and any queued reload for the length of the backoff.Test plan
go build ./internal/pkg/nvmlprovider/...retry_test.gocovers: succeeds after transient failures (exact call count), gives up after exhausting attempts, fails fast on a non-transient error (ERROR_INSUFFICIENT_POWER), backoff cap/jitter/growth, attempts-clamping, context cancellation interrupting the backoff sleep, and large attempt counts not overflowing the backoff cap.go build ./...,go vet ./..., andgo test ./pkg/cmd/... ./internal/pkg/appconfig/... ./internal/pkg/nvmlprovider/...pass in a Linux container (this repo is Linux-only; native build isn't possible on macOS).golangci-lint runclean on touched packages (added a#nosec G404justification for the jitter RNG, matching existing repo convention).Fixed the pre-existing test gap noted in an earlier revision of this description:
Test_newNVMLProvider,TestGetMIGDeviceInfoByID_When_DriverVersion_Below_R470,TestCleanup_WhenInitialized, andTestPreCheckno longer require real NVML/GPU hardware to pass. See the comment thread below for what changed and how it was verified (23 passed, 5 skipped, 0 failed in a container with no NVML library).