Skip to content

Ignore faulty GPUs when listing GPUs - #66

Open
geobeau wants to merge 1 commit into
NVIDIA:mainfrom
geobeau:main
Open

Ignore faulty GPUs when listing GPUs#66
geobeau wants to merge 1 commit into
NVIDIA:mainfrom
geobeau:main

Conversation

@geobeau

@geobeau geobeau commented May 18, 2026

Copy link
Copy Markdown

We have the case where a server has some of the GPUs in a broken state, however we want to continue using the other GPUs. While the nvidia operator is aware that these GPUs are out, it fails when computing GPU topology.
The goal of this PR is to skip broken GPUs when computing the topology

On servers with some of the GPUs broken, we want
to continue using the other GPUs. However, when
computing the topology, it fails on GPU Lost state.

Signed-off-by: g.beausire <g.beausire@criteo.com>
@geobeau

geobeau commented May 20, 2026

Copy link
Copy Markdown
Author

cc @rajatchopra, sorry for the ping but you are the last one that merged something on the repo, do you mind taking a look?

@kvalliyurnatt kvalliyurnatt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for your contribution ! I think we need more changes to actually be able to ignore faulty GPUs, I have left a couple of comments. Sorry i misread the goal of the PR, I am taking another look, dismissed my earlier review

@kvalliyurnatt
kvalliyurnatt dismissed their stale review September 2, 2026 18:35

I reread the goal of the PR, operator already knows about the broken GPUs. OS my comments are not valid in this case

Comment thread internal/links/device.go
for i := 0; i < nvml.NVLINK_MAX_LINKS; i++ {
state, ret := dev.GetNvLinkState(i)
if ret == nvml.ERROR_NOT_SUPPORTED || ret == nvml.ERROR_INVALID_ARGUMENT {
if ret == nvml.ERROR_NOT_SUPPORTED || ret == nvml.ERROR_INVALID_ARGUMENT || ret == nvml.ERROR_GPU_IS_LOST {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we add a higher-level test demonstrating the expected operator flow: topology discovery tolerates ERROR_GPU_IS_LOST, the known-broken GPU is excluded from the allocation set, and BestEffort successfully evaluates the remaining healthy GPUs? This would document the caller-side filtering assumption and why incomplete links involving the broken GPU are safe.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please see the nvml.h documentation for the C symbol nvmlDeviceGetNvLinkState


/**
 * Retrieves the state of the device's NvLink for the link specified
 *
 * For Pascal &tm; or newer fully supported devices.
 *
 * @param device                               The identifier of the target device
 * @param link                                 Specifies the NvLink link to be queried
 * @param isActive                             \a nvmlEnableState_t where NVML_FEATURE_ENABLED indicates that
 *                                             the link is active and NVML_FEATURE_DISABLED indicates it
 *                                             is inactive
 *
 * @return
 *         - \ref NVML_SUCCESS                 if \a isActive has been set
 *         - \ref NVML_ERROR_UNINITIALIZED     if the library has not been successfully initialized
 *         - \ref NVML_ERROR_INVALID_ARGUMENT  if \a device or \a link is invalid or \a isActive is NULL
 *         - \ref NVML_ERROR_NOT_SUPPORTED     if the device doesn't support this feature
 *         - \ref NVML_ERROR_UNKNOWN           on any unexpected error
 */
nvmlReturn_t DECLDIR nvmlDeviceGetNvLinkState(nvmlDevice_t device, unsigned int link, nvmlEnableState_t *isActive);

NVML_ERROR_GPU_IS_LOST is not in the list of possible errors returned.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can also refer to this page.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks I completely missed this.

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.

4 participants