Fix device plugin recovery from Unhealthy state - #2201
Conversation
Signed-off-by: itsvishalyadav <vishalyadav87695@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: itsvishalyadav 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 |
|
Welcome @itsvishalyadav! It looks like this is your first PR to Project-HAMi/HAMi 🎉 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe NVIDIA resource manager renames its health-reporting channel from ChangesNVIDIA health channel
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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`:
- Around line 127-133: Move the unhealthyDevices map initialization before the
device registration loop, and when registration marks a device Unhealthy in the
setup path, add it to this map. Preserve the existing recovery and timeout
polling behavior so setup-time failures can later transition back to Healthy.
- Around line 60-61: Update nvmlResourceManager.checkHealth so every device
health result send uses the renamed health channel parameter instead of the
undefined unhealthy channel, including the sends at the referenced branches,
while preserving the existing send conditions and values.
In `@pkg/device-plugin/nvidiadevice/nvinternal/rm/nvml_manager.go`:
- Line 116: Update the comment for CheckHealth to state that the health channel
reports device health updates or transitions, including both healthy and
unhealthy devices, instead of referring only to unhealthy devices.
In `@pkg/device-plugin/nvidiadevice/nvinternal/rm/rm_mock.go`:
- Around line 139-144: Update the mock call-record type used by
mock.calls.CheckHealth to consistently use the Health field instead of
Unhealthy, including its stored field, accessor return type, and related
comments; ensure the append involving callInfo compiles without changing the
surrounding 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: aca90f67-cc17-40ff-ae74-38234aa1c668
📒 Files selected for processing (6)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.gopkg/device-plugin/nvidiadevice/nvinternal/rm/health.gopkg/device-plugin/nvidiadevice/nvinternal/rm/nvml_manager.gopkg/device-plugin/nvidiadevice/nvinternal/rm/rm.gopkg/device-plugin/nvidiadevice/nvinternal/rm/rm_mock.gopkg/device-plugin/nvidiadevice/nvinternal/rm/tegra_manager.go
Signed-off-by: itsvishalyadav <vishalyadav87695@gmail.com>
|
Hi @DSFans2014, @FouoF and @ouyangluwei163 |
There was a problem hiding this comment.
could you add a unit test for the recovery path? nothing covers it rn.
and no ai assistance disclosure is present. if any ai tool was used, it must be disclosed per CONTRIBUTING.md: https://github.com/Project-HAMi/HAMi/blob/master/CONTRIBUTING.md#ai-assistance-notice
| // 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 { | ||
| // CheckHealth performs health checks on a set of devices, writing to the 'health' channel with any unhealthy devices | ||
| func (r *nvmlResourceManager) checkHealth(stop <-chan interface{}, devices Devices, health chan<- *Device, disableNVML <-chan bool) error { |
There was a problem hiding this comment.
the rename stops at the sig, l.102/112/119 still send to unhealthy so this pkg doesn't compile.
| _ = eventSet.Free() | ||
| }() | ||
|
|
||
| unhealthyDevices := make(map[string]*Device) |
There was a problem hiding this comment.
devices marked unhealthy in the registration loop below never land in this map, so they can never recover.
| if ret != nvml.SUCCESS { | ||
| klog.Infof("Marking device %v as unhealthy: %v", d.ID, ret) | ||
| unhealthy <- d | ||
| d.Health = kubeletdevicepluginv1beta1.Unhealthy |
There was a problem hiding this comment.
we now write d.Health from the health goroutine while listandwatch reads the same *Device through apiDevices(), that's a data race afaict.
| e, ret := eventSet.Wait(5000) | ||
| if ret == nvml.ERROR_TIMEOUT { | ||
| for id, d := range unhealthyDevices { | ||
| uuid, _, _, err := r.getDevicePlacement(d) |
There was a problem hiding this comment.
for a mig device getDevicePlacement gives back the parent gpu uuid, so the probe below says nothing abt the mig instance itself.
| if ret != nvml.SUCCESS { | ||
| continue | ||
| } | ||
| _, ret = gpu.GetMemoryInfo() |
There was a problem hiding this comment.
GetMemoryInfo still succeeds on a gpu that just hit an xid, so imo this flips nearly every device back to healthy after 5s and we lose the signal for real faults.
| Stop <-chan interface{} | ||
| Health chan<- *Device | ||
| } { | ||
| var calls []struct { |
There was a problem hiding this comment.
this local still says Unhealthy while the return type above says Health, so it doesn't compile.
Signed-off-by: itsvishalyadav <vishalyadav87695@gmail.com>
382df9b to
4f39770
Compare
|
Thank you for the detailed review @mesutoezdil! You raised excellent points. I have pushed a new commit that addresses your feedback:
This PR now serves purely as a refactoring groundwork to change the internal channel from \unhealthy\ to \health\ (supporting bidirectional signaling). Since automatic recovery via polling is unsafe with our current signals, what is the project's preferred approach to resolving the original \FIXME? Should we provide an API endpoint or SIGHUP signal to allow cluster admins to manually clear the Unhealthy state without a full pod restart? |
Reminder: Answers must be written by human being. You can view the relevant rule here. |
My bad for the previous comment. I have made the requested changes in a new commit. Since the condition on GetMemoryInfo was not sufficient as a condition for the GPU recovery from XID error (since it might conceal a problem with the hardware), I have completely deleted the polling mechanism, thus solving the data race problem with ListAndWatch. Additionally, I have fixed the mock compilation errors. The safe polling scheme will not work for the recovery of the devices, so do you think it would make sense to tackle the original FIXME? Can we proceed with the idea that I expose an API or a signal for the administrators of the cluster to mark the device as healthy without having to restart the pod? |
|
@itsvishalyadav Actually these codes are forked form https://github.com/NVIDIA/k8s-device-plugin and they will be synced periodically. We'd better not change them if not necessary to avoid potential conflict. |
3 participants
What type of PR is this?
/kind bug
/kind cleanup
What this PR does / why we need it:
This PR refactors the device plugin's health signaling mechanism to support bidirectional health reporting (both
HealthyandUnhealthyevents) as groundwork for recovering devices from anUnhealthystate.Note: Initially, this PR attempted to implement an automatic recovery mechanism by polling
GetMemoryInfoevery 5 seconds. However, as noted in review by @mesutoezdil,GetMemoryInfocan still succeed on a GPU that just hit an XID fault, which would unsafely hide real hardware faults. Therefore, the automatic polling mechanism has been reverted. This PR now focuses purely on the API rename and mock fixes to lay the foundation for a safe (likely manual/endpoint-driven) recovery mechanism in the future.AI Assistance Notice:
Which issue(s) this PR fixes:
Fixes #2200
Special notes for your reviewer:
N/A
Does this PR introduce a user-facing change?:
No
Summary by CodeRabbit