Skip to content

Refactor GPU Health Checker to use go-nvml - #622

Open
ankitjoju-google wants to merge 1 commit into
GoogleCloudPlatform:masterfrom
ankitjoju-google:refactor-healthcheck
Open

Refactor GPU Health Checker to use go-nvml#622
ankitjoju-google wants to merge 1 commit into
GoogleCloudPlatform:masterfrom
ankitjoju-google:refactor-healthcheck

Conversation

@ankitjoju-google

Copy link
Copy Markdown
Collaborator

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.

Required for potential implementation of GPU Reset.

Tested with Unit Tests & Real GKE nodes w/ Accelerators

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.

"cloud.google.com/go/compute/metadata"
"github.com/GoogleCloudPlatform/container-engine-accelerators/pkg/gpu/nvidia/util"
"github.com/NVIDIA/gpu-monitoring-tools/bindings/go/nvml"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you ran go mod vendor; go mod tidy? if gpu-monitoring-tools/bindings/go/nvml is not used it should be cleaned up from vendor/

}
type GPUDevice struct{}

var migUUIDRegex = regexp.MustCompile(`^MIG-GPU-([^/]+)/([0-9]+)/([0-9]+)$`)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex looks a little off to me. From my memory the MIG device UUID is MIG-UUID not MIG-GPU-UUID.

have you tested this new code manually and does this work

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.

3 participants