Add env var DP_ENABLE_HEALTHCHECKS #1335

robertdavidsmith · 2025-07-28T21:40:42Z

Make it possible to enable health checks for xids which are disabled by default via an env var DP_ENABLE_HEALTHCHECKS.

This is occasionally useful, e.g. we have seen examples where xid 45 is very correlated with genuine GPU issues.

Signed-off-by: Robert Smith <[email protected]>

copy-pr-bot · 2025-07-28T21:40:45Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

elezar · 2025-08-01T10:20:41Z

Thanks @robertdavidsmith. We have had some internal discussions around more flexibility here and I had the idea for #1340 created against an internal issue tracker. I think your proposal here aligns with this to some extent and as such your input there would be appreciated.

elezar · 2025-08-01T10:22:02Z

internal/rm/health.go

 	// disabled entirely. If set, the envvar is treated as a comma-separated list of Xids to ignore. Note that
 	// this is in addition to the Application errors that are already ignored.
 	envDisableHealthChecks = "DP_DISABLE_HEALTHCHECKS"
+	envEnableHealthChecks  = "DP_ENABLE_HEALTHCHECKS"


Can we rename this DP_FORCE_HEALTHCHECKS or CRITICAL_XIDS or FATAL_XIDS instead? (I would prefer the latter, but I could hear arguments for alternatives.

DP_DISABLE_HEALTHCHECKS is the exact opposite of DP_ENABLE_HEALTHCHECKS and IMO the naming should reflect this. So I'd favour renaming DP_ENABLE_HEALTHCHECKS to DP_FORCE_ENABLE_HEALTHCHECKS, that way you get the "force" but it's still very clear it's the exact opposite of DP_DISABLE_HEALTHCHECKS.

One question: What is the order of precedence of these? If DP_DISABLE_HEALTHCHECKS=all|xids does adding a specific XID to DP_FORCE_ENABLE_HEALTHCHECKS take precedence over this?

Then a comment on the naming. I don't think this envvar is the oposite of the existing one. While the existing one removes the predefined XIDs that we treat as fatal, this one adds ADDITIONAL ones.

On balance I'd make DP_ADDITIONAL_HEALTHCHECKS /DP_FORCE_ENABLE_HEALTHCHECKS support all, and have it override DP_DISABLE_HEALTHCHECKS. This matters more if we choose a name with FORCE in it, but is probably worth doing regardless of name. This would complicate the code a little, but would make the externally visible behavior to be what a reasonable user would expect.

elezar · 2025-08-01T10:22:56Z

internal/rm/health.go

 		return nil
 	}

+	enableHealthChecks := strings.ToLower(os.Getenv(envEnableHealthChecks))


Let's move this to wehre we actually use the value.

Fair comment, though if we add support for "all" this all changes anyway.

elezar · 2025-08-01T10:23:56Z

internal/rm/health.go

 		skippedXids[additionalXid] = true
 	}

+	for _, additionalXid := range getAdditionalXids(enableHealthChecks) {


Does it make sense to allow something like all to be specified?

That's reasonable, I'm happy to do this

robertdavidsmith · 2025-08-04T10:13:51Z

Thanks @robertdavidsmith. We have had some internal discussions around more flexibility here and I had the idea for #1340 created against an internal issue tracker. I think your proposal here aligns with this to some extent and as such your input there would be appreciated.

Noted, I've commented on #1340

Do you see that issue as a replacement for this PR (in which case I'll close it), or in addition to it (in which case I'll update the PR to incorporate your feedback)?

elezar · 2025-08-13T09:59:12Z

Thanks @robertdavidsmith. We have had some internal discussions around more flexibility here and I had the idea for #1340 created against an internal issue tracker. I think your proposal here aligns with this to some extent and as such your input there would be appreciated.

Noted, I've commented on #1340

Do you see that issue as a replacement for this PR (in which case I'll close it), or in addition to it (in which case I'll update the PR to incorporate your feedback)?

I think the core logic of what we're trying to achieve here is covered by #1340, but this PR calls out an omission in that issue which is that the set of "Critical" / "Forced" XIDs also need to be settable by an environment variable and not only in a config file. With that in mind, I think we can continue with your PR and then all that remains for #1340 would be to define the actual config format and map these to the relevant envvars using CLI flags like we do for other options.

Using our CLI handling to specify envvars also give us more flexibility to rename these envvars while falling back to the original. For the longest time, the DP_ENABLE_HEALTHCHECKS envvar has been considered an internal-only / undocumented features and if we pull this up to a more public configuration option (with support in helm etc.) a clearer name would be useful.

robertdavidsmith · 2025-08-14T14:25:42Z

Ok shall I make a PR to add DP_ADDITIONAL_HEALTHCHECKS that

Will support DP_ADDITIONAL_HEALTHCHECKS=all
DP_ADDITIONAL_HEALTHCHECKS always beats DP_DISABLE_HEALTHCHECKS
Given increased complexity, I'll separate the enable/disable logic from the rest of checkHealth() so it can be properly unit-tested

You can then do #1340

Thoughts?

robertdavidsmith · 2025-08-15T10:06:27Z

Just seen https://github.com/NVIDIA/k8s-device-plugin/pull/1360/files, that PR is largely complete and should be merged first.

elezar · 2025-08-28T11:03:11Z

Just seen https://github.com/NVIDIA/k8s-device-plugin/pull/1360/files, that PR is largely complete and should be merged first.

While #1360 does have some overlap with what you're doing here, I think that it could be seen as a follow up. I have created a branch https://github.com/elezar/k8s-device-plugin/tree/robertdavidsmith/main with a minor refactoring on top of your changes which should be compatible with what we want to do in 1360. What do you think?

robertdavidsmith · 2025-08-29T13:16:39Z

Just seen https://github.com/NVIDIA/k8s-device-plugin/pull/1360/files, that PR is largely complete and should be merged first.

While #1360 does have some overlap with what you're doing here, I think that it could be seen as a follow up. I have created a branch https://github.com/elezar/k8s-device-plugin/tree/robertdavidsmith/main with a minor refactoring on top of your changes which should be compatible with what we want to do in 1360. What do you think?

Thank you @elezar

I've commented on your commit here, basically I think this makes a lot of sense, but see comments.

robertdavidsmith · 2025-09-08T10:04:10Z

Hi @elezar,

How do you want to proceed from here?

Shall I copy your commit here onto my branch, make the changes I suggested (see comments on that commit), and then send you this PR for final review?

Thanks,

Rob

elezar · 2025-09-16T18:20:50Z

Sorry for the delay here. If you think the changes proposed in the commit are reasonable, feel free to pull them in here so that we can continue reviewing your PR. It would be good for you to get the credit.

robertdavidsmith · 2025-09-22T16:43:06Z

Sorry for the delay here. If you think the changes proposed in the commit are reasonable, feel free to pull them in here so that we can continue reviewing your PR. It would be good for you to get the credit.

Ok, I'm happy to do so

elezar · 2025-10-07T11:42:30Z

In order to close this out I have created #1443. Please take a look.

Signed-off-by: Robert Smith <[email protected]>

robertdavidsmith · 2025-10-08T10:24:34Z

@elezar PR #1443 LGTM, thank you. I will close this PR

elezar · 2025-10-09T12:00:05Z

@elezar PR #1443 LGTM, thank you. I will close this PR

Thanks again for your work here!

Add env var DP_ENABLE_HEALTHCHECKS

7e2a880

Signed-off-by: Robert Smith <[email protected]>

Merge branch 'main' into main

abff1c9

elezar reviewed Aug 1, 2025

View reviewed changes

elezar mentioned this pull request Aug 13, 2025

Add advanced health check configuration to config file #1360

Open

elezar mentioned this pull request Oct 7, 2025

Add support for explicitly enabling XIDs in health checks #1443

Merged

pr feedback

e2691c1

Signed-off-by: Robert Smith <[email protected]>

robertdavidsmith marked this pull request as draft October 7, 2025 15:36

robertdavidsmith closed this Oct 8, 2025

Add env var DP_ENABLE_HEALTHCHECKS #1335

Add env var DP_ENABLE_HEALTHCHECKS #1335

Uh oh!

Conversation

robertdavidsmith commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copy-pr-bot bot commented Jul 28, 2025

Uh oh!

elezar commented Aug 1, 2025

Uh oh!

elezar Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

robertdavidsmith Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

robertdavidsmith Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elezar Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

robertdavidsmith Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

elezar Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

robertdavidsmith Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

robertdavidsmith commented Aug 4, 2025

Uh oh!

elezar commented Aug 13, 2025

Uh oh!

robertdavidsmith commented Aug 14, 2025

Uh oh!

robertdavidsmith commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elezar commented Aug 28, 2025

Uh oh!

robertdavidsmith commented Aug 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertdavidsmith commented Sep 8, 2025

Uh oh!

elezar commented Sep 16, 2025

Uh oh!

robertdavidsmith commented Sep 22, 2025

Uh oh!

elezar commented Oct 7, 2025

Uh oh!

robertdavidsmith commented Oct 8, 2025

Uh oh!

elezar commented Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robertdavidsmith commented Jul 28, 2025 •

edited

Loading

robertdavidsmith Aug 4, 2025 •

edited

Loading

robertdavidsmith Aug 14, 2025 •

edited

Loading

robertdavidsmith commented Aug 15, 2025 •

edited

Loading

robertdavidsmith commented Aug 29, 2025 •

edited

Loading