refactor: upgrade health check config from env vars to top-level options - #2151
refactor: upgrade health check config from env vars to top-level options#2151shellyco-code wants to merge 1 commit into
Conversation
Signed-off-by: shellyco-code <shellychahar57@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shellyco-code The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe NVIDIA plugin now reads health-check environment variables during startup, passes them into NVML and Tegra resource managers, stores them in manager state, and uses them during health-check XID selection. Tests now provide configuration directly instead of modifying process-wide environment state. ChangesNVIDIA health-check configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/rm/health.go`:
- Line 61: Move XID parsing out of checkHealth and into resource-manager
construction, creating and storing a parsed health-check configuration alongside
each manager. Update checkHealth and getHealthCheckXids usage to reuse that
stored configuration without lowercasing, splitting, or parsing raw environment
values at runtime, while preserving the existing enable/disable behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82b5a7e1-e0db-465a-8d95-f334a4f039ff
📒 Files selected for processing (7)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health_checkhealth_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health_test.gopkg/device-plugin/nvidiadevice/nvinternal/rm/nvml_manager.gopkg/device-plugin/nvidiadevice/nvinternal/rm/rm.gopkg/device-plugin/nvidiadevice/nvinternal/rm/tegra_manager.go
| // 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), ",")..., |
There was a problem hiding this comment.
the os import at the top is unused now that both getenv calls are gone so the pkg doesnt compile, can u drop it?
There was a problem hiding this comment.
Good catch! Dropped the unused os import in health.go
| }() | ||
|
|
||
| return rm.NewNVMLResourceManagers(o.infolib, o.nvmllib, o.devicelib, o.config.Config) | ||
| disableHealthChecks := os.Getenv("DP_DISABLE_HEALTHCHECKS") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I exported EnvDisableHealthChecks and EnvEnableHealthChecks in health.go and refactored getResourceManagers to read them just once at the top before the switch statement
|
Thanks for the configuration refactor. This branch does not address a demonstrated production failure, and the exact head still fails lint because of an unused os import, which prevents the remaining checks from running. Replies indicate that fixes were made, but those fixes are not present in the reviewable head. We are closing this stale branch. If the refactor is still needed, please first confirm the scope in an issue and then submit one minimal, clean, fully passing change from current master. |
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
This PR resolves the
// TODOtechnical debt inpkg/device-plugin/nvidiadevice/nvinternal/rm/health.goby upgrading the health check configuration so it no longer dynamically reads environment variables (DP_DISABLE_HEALTHCHECKSandDP_ENABLE_HEALTHCHECKS) on the fly viaos.Getenv().Instead, the configuration is now parsed exactly once at startup during plugin initialization (
pkg/device-plugin/nvidiadevice/nvinternal/plugin/factory.go) and explicitly passed down into therm.ResourceManagerinstances as state variables. This eliminates hidden dependencies onos.Getenv()deep within the business logic and makes testing and mocking significantly cleaner. The test suite (health_test.goandhealth_checkhealth_test.go) has also been refactored to inject the configuration into the mock structs rather than relying ont.Setenv().Which issue(s) this PR fixes:
Fixes #2150
Special notes for your reviewer:
The commit is signed-off (DCO) and all associated tests have been successfully updated to natively test the new configuration injection logic without environment variable mocking!
Does this PR introduce a user-facing change?: