Skip to content

refactor: upgrade health check config from env vars to top-level options #2151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shellyco-code wants to merge 1 commit into from
Closed

refactor: upgrade health check config from env vars to top-level options #2151

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ package plugin
import (
"context"
"fmt"
"os"

"github.com/NVIDIA/go-nvlib/pkg/nvlib/device"
"github.com/NVIDIA/go-nvlib/pkg/nvlib/info"
Expand Down Expand Up @@ -128,9 +129,14 @@ func (o *options) getResourceManagers() ([]rm.ResourceManager, error) {
_ = o.nvmllib.Shutdown()
}()

return rm.NewNVMLResourceManagers(o.infolib, o.nvmllib, o.devicelib, o.config.Config)
disableHealthChecks := os.Getenv("DP_DISABLE_HEALTHCHECKS")

@mesutoezdil mesutoezdil Jul 28, 2026

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.

the env names are hardcoded here (4 times) while envDisableHealthChecks/envEnableHealthChecks in rm/health.go are now dead, can u export those consts and read the env once before the switch?

@shellyco-code shellyco-code Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I exported EnvDisableHealthChecks and EnvEnableHealthChecks in health.go and refactored getResourceManagers to read them just once at the top before the switch statement

enableHealthChecks := os.Getenv("DP_ENABLE_HEALTHCHECKS")

return rm.NewNVMLResourceManagers(o.infolib, o.nvmllib, o.devicelib, o.config.Config, disableHealthChecks, enableHealthChecks)
case "tegra":
return rm.NewTegraResourceManagers(o.config.Config)
disableHealthChecks := os.Getenv("DP_DISABLE_HEALTHCHECKS")
enableHealthChecks := os.Getenv("DP_ENABLE_HEALTHCHECKS")
return rm.NewTegraResourceManagers(o.config.Config, disableHealthChecks, enableHealthChecks)
default:
klog.Errorf("Incompatible strategy detected %v", strategy)
klog.Error("If this is a GPU node, did you configure the NVIDIA Container Toolkit?")
Expand Down
25 changes: 10 additions & 15 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/health.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

import (
"fmt"
"os"

Check failure on line 37 in pkg/device-plugin/nvidiadevice/nvinternal/rm/health.go

View workflow job for this annotation

GitHub Actions / lint 

"os" imported and not used (typecheck)
"strconv"
"strings"

Expand All @@ -58,7 +58,7 @@

// CheckHealth performs health checks on a set of devices, writing to the 'unhealthy' channel with any unhealthy devices
func (r *nvmlResourceManager) checkHealth(stop <-chan interface{}, devices Devices, unhealthy chan<- *Device, disableNVML <-chan bool) error {
xids := getHealthCheckXids()
xids := getHealthCheckXids(r.disableHealthChecks, r.enableHealthChecks)
Comment thread
shellyco-code marked this conversation as resolved.
if xids.IsAllDisabled() {
return nil
}
Expand Down Expand Up @@ -228,23 +228,18 @@
}

// getHealthCheckXids returns the XIDs that are considered fatal.
// Here we combine the following (in order of precedence):
// * A list of explicitly disabled XIDs (including all XIDs)
// * A list of hardcoded disabled XIDs
// * A list of explicitly enabled XIDs (including all XIDs)
//
// Note that if an XID is explicitly enabled, this takes precedence over it
// having been disabled either explicitly or implicitly.
func getHealthCheckXids() disabledXIDs {
// Healthchecks can be disabled by specifying an explicit list of XIDs to ignore.
// This list is defined by the DP_DISABLE_HEALTHCHECKS and DP_ENABLE_HEALTHCHECKS configuration options.
// If DP_DISABLE_HEALTHCHECKS="all", all healthchecks are disabled.
// An XID that is present in the "enabled" list will override the "disabled" list.
// The list of ignored XIDs returned contains the predefined list of application errors
// in addition to the specified list, minus the XIDs from the "enabled" list.
func getHealthCheckXids(disableHealthChecks, enableHealthChecks string) disabledXIDs {
disabled := newHealthCheckXIDs(
// TODO: We should not read the envvar here directly, but instead
// "upgrade" this to a top-level config option.
strings.Split(strings.ToLower(os.Getenv(envDisableHealthChecks)), ",")...,
strings.Split(strings.ToLower(disableHealthChecks), ",")...,

@mesutoezdil mesutoezdil Jul 28, 2026

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.

the os import at the top is unused now that both getenv calls are gone so the pkg doesnt compile, can u drop it?

@shellyco-code shellyco-code Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good catch! Dropped the unused os import in health.go

)
enabled := newHealthCheckXIDs(
// TODO: We should not read the envvar here directly, but instead
// "upgrade" this to a top-level config option.
strings.Split(strings.ToLower(os.Getenv(envEnableHealthChecks)), ",")...,
strings.Split(strings.ToLower(enableHealthChecks), ",")...,
)

// Add the list of hardcoded disabled (ignored) XIDs:
Expand Down
8 changes: 5 additions & 3 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/health_checkhealth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@ import (
// example DP_DISABLE_HEALTHCHECKS=all). CheckHealth must not dereference that
// nil error.
func TestCheckHealthNilErrorDoesNotPanic(t *testing.T) {
t.Setenv(envDisableHealthChecks, "all")

r := &nvmlResourceManager{}
r := &nvmlResourceManager{
resourceManager: resourceManager{
disableHealthChecks: "all",
},
}
stop := make(chan interface{})
unhealthy := make(chan *Device, 1)
disableNVML := make(chan bool, 1)
Expand Down
5 changes: 1 addition & 4 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/health_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,10 +225,7 @@ func TestGetHealthCheckXids(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.description, func(t *testing.T) {
t.Setenv(envDisableHealthChecks, tc.disabled)
t.Setenv(envEnableHealthChecks, tc.enabled)

xids := getHealthCheckXids()
xids := getHealthCheckXids(tc.disabled, tc.enabled)
require.EqualValues(t, tc.expectedContents, xids)
require.Equal(t, tc.expectedAllDisabled, xids.IsAllDisabled())

Expand Down
10 changes: 6 additions & 4 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/nvml_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type nvmlResourceManager struct {
var _ ResourceManager = (*nvmlResourceManager)(nil)

// NewNVMLResourceManagers returns a set of ResourceManagers, one for each NVML resource in 'config'.
func NewNVMLResourceManagers(infolib info.Interface, nvmllib nvml.Interface, devicelib device.Interface, config *spec.Config) ([]ResourceManager, error) {
func NewNVMLResourceManagers(infolib info.Interface, nvmllib nvml.Interface, devicelib device.Interface, config *spec.Config, disableHealthChecks, enableHealthChecks string) ([]ResourceManager, error) {
ret := nvmllib.Init()
if ret != nvml.SUCCESS {
return nil, fmt.Errorf("failed to initialize NVML: %v", ret)
Expand Down Expand Up @@ -83,9 +83,11 @@ func NewNVMLResourceManagers(infolib info.Interface, nvmllib nvml.Interface, dev
}
r := &nvmlResourceManager{
resourceManager: resourceManager{
config: config,
resource: resourceName,
devices: devices,
config: config,
resource: resourceName,
devices: devices,
disableHealthChecks: disableHealthChecks,
enableHealthChecks: enableHealthChecks,
},
nvml: nvmllib,
}
Expand Down
8 changes: 5 additions & 3 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/rm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,11 @@ import (

// resourceManager forms the base type for specific resource manager implementations
type resourceManager struct {
config *spec.Config
resource spec.ResourceName
devices Devices
config *spec.Config
resource spec.ResourceName
devices Devices
disableHealthChecks string
enableHealthChecks string
}

// ResourceManager provides an interface for listing a set of Devices and checking health on them
Expand Down
12 changes: 7 additions & 5 deletions pkg/device-plugin/nvidiadevice/nvinternal/rm/tegra_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ type tegraResourceManager struct {

var _ ResourceManager = (*tegraResourceManager)(nil)

// NewTegraResourceManagers returns a set of ResourceManagers for tegra resources
func NewTegraResourceManagers(config *spec.Config) ([]ResourceManager, error) {
// NewTegraResourceManagers returns a set of ResourceManagers, one for each Tegra resource in 'config'.
func NewTegraResourceManagers(config *spec.Config, disableHealthChecks, enableHealthChecks string) ([]ResourceManager, error) {
deviceMap, err := buildTegraDeviceMap(config)
if err != nil {
return nil, fmt.Errorf("error building Tegra device map: %v", err)
Expand All @@ -63,9 +63,11 @@ func NewTegraResourceManagers(config *spec.Config) ([]ResourceManager, error) {
}
r := &tegraResourceManager{
resourceManager: resourceManager{
config: config,
resource: resourceName,
devices: devices,
config: config,
resource: resourceName,
devices: devices,
disableHealthChecks: disableHealthChecks,
enableHealthChecks: enableHealthChecks,
},
}
if len(devices) != 0 {
Expand Down
Loading