Skip to content

[GPU Reset 1/3] Refactor health checker to go-nvml with Stop/Start lifecycle - #642

Open
ankitjoju-google wants to merge 3 commits into
GoogleCloudPlatform:masterfrom
ankitjoju-google:gpu-reset-1-healthcheck
Open

ankitjoju-google wants to merge 3 commits into
GoogleCloudPlatform:masterfrom
ankitjoju-google:gpu-reset-1-healthcheck

Conversation

@ankitjoju-google

Copy link
Copy Markdown
Collaborator

GPU Reset, PR 1/3. Part of in-process GPU reset for the NVIDIA device plugin (PR 2/3 and 3/3 build on this).

Why

Resetting a GPU requires every process, including the device plugin itself, to release its NVML handles first. The health checker holds NVML event sets for its whole lifetime and can't be stopped and restarted, and it still uses the deprecated gpu-monitoring-tools bindings while the rest of the plugin uses go-nvml.

What

  • Move pkg/gpu/nvidia/health_check to github.com/NVIDIA/go-nvml, behind small internal interfaces so it can be unit tested without hardware.
  • Add idempotent Stop() / restartable Start(): goroutines are joined before the event set is freed, one long-lived event broadcaster is kept, and Stop() can't deadlock on a full health channel.
  • Keep existing MIG behaviour (modern and legacy MIG UUIDs); register events once per physical GPU.

Testing

  • go vet ./..., go test -race ./pkg/gpu/nvidia/... ./cmd/... ./partition_gpu/...
  • New unit tests for Stop/Start cycles, Start failure cleanup, backpressure, and MIG UUID handling.
  • Exercised on A4 (8x B200) and A4X Max (4x GB300) GKE nodes, stopped and restarted around each GPU reset.

Replaced the deprecated gpu-monitoring-tools library with the official go-nvml library.
Updated method signatures to use raw types for easier testability and mocking, avoiding issues with non-mockable nvml.Device struct.
Updated test cases to match new signatures and smaller mock instance IDs.
- Make Stop() idempotent, synchronize Start()/Stop() with a mutex, and
  wait for background goroutines (listenToEvents, setXIDheartbeat,
  resetXIDConditionWithBackoff) via sync.WaitGroup before freeing the
  NVML event set.
- Keep a single long-lived EventBroadcaster across Stop()/Start() cycles
  instead of attempting an invalid interface assertion in Stop() or
  leaking broadcasters on restart.
- Spawn background goroutines only after NVML device registration
  succeeds, and clean up the allocated eventSet on early Start() failure.
- Restore upstream/master MIG behavior: treat nvml.ERROR_NOT_SUPPORTED
  from GetMigMode() as MIG disabled, verify currentMode == pendingMode,
  and resolve MIG parent UUID and GI/CI IDs via go-nvml handles for
  modern R470+ MIG-<uuid> format with fallback to legacy regex parsing.
- Add unit tests covering idempotent Stop(), Stop()/Start() restart
  without broadcaster leaks, Start() error cleanup, and modern/legacy
  MIG UUID handling.
…el and optimize MIG

- Select on hc.stop when sending to hc.health in catchError so Stop()
  cannot deadlock if the health channel is full or unconsumed during
  teardown.
- Deduplicate physDevice.RegisterEvents calls per physical GPU UUID when
  multiple MIG instances share a parent GPU.
- Skip MIG NVML handle lookups in resolveMigDeviceInfo and
  GPUDevice.parseMigDeviceUUID when the device UUID does not start with
  "MIG-".
- Add TestStopUnderHealthChannelBackpressure and verify single
  RegisterEvents per physical GPU in TestMIGSupportModernAndLegacyUUIDs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant