recovery: detect GPUs needing recovery from ResourceSlices - #109
Conversation
|
This PR starts implementation of the gpurecovery controller. It will be followed by other PRs that increase functionality (approve handling, recovery Job creation etc.) |
There was a problem hiding this comment.
🟡 Changes recommended
The controller currently accepts ResourceSlices with missing spec.nodeName (leading to empty-node events/key collisions) and should tighten behavior/RBAC markers and documentation before it can be safely approved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the initial (detection-only) reconciliation phase for GPURecoveryPlan, mirroring recovery-relevant DRA device taints from ResourceSlice objects into GPURecoveryPlan.status.events and deriving status.state from those events.
Changes:
- Introduces
GPURecoveryPlanReconcilerthat listsResourceSliceobjects, detects tainted GPUs matchingspec.deviceId, and maintainsstatus.events(including escalation reset → reflash). - Adds helpers/constants for taint→recovery mapping and deterministic, bounded event/job naming.
- Adds a comprehensive controller test suite and updates envtest feature gates to preserve device taints.
File summaries
| File | Description |
|---|---|
| internal/controller/suite_test.go | Enables DRADeviceTaints in envtest so device taints are preserved for controller tests. |
| internal/controller/gpurecoveryplan_helpers.go | Helper functions for attribute access, taint mapping, priority, and deterministic ID/job-name generation. |
| internal/controller/gpurecoveryplan_controller.go | New reconciler implementing detection: scan ResourceSlices, manage status.events, compute status.state, and watch mapping. |
| internal/controller/gpurecoveryplan_controller_test.go | Extensive Ginkgo/Gomega tests covering detection, escalation, persistence, naming, and state derivation. |
| internal/controller/gpurecoveryplan_const.go | New constants for DRA attribute/taint keys, reasons, caps, and naming budgets. |
| config/rbac/role.yaml | Grants the operator RBAC access to gpurecoveryplans and gpurecoveryplans/status. |
| cmd/main.go | Registers the new GPURecoveryPlan controller with the manager. |
| charts/gpu-base-operator/templates/role.yaml | Mirrors RBAC additions in the Helm chart role template. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Add the GPURecoveryPlan reconciler's detection phase. On every reconcile it lists ResourceSlices, picks the devices matching spec.deviceId, and mirrors the ones carrying a recovery-related device taint into status.events; events whose taint has cleared are dropped again, since nothing has been done to the GPU yet and something else evidently healed it. status.state is derived from the resulting event states. Three taint keys are recognised. health-xpumd-gpu.wedged means the GPU needs a reset, and which reset that is comes from spec.defaultResetType because the DRA driver cannot tell whether the platform's PCIe slots support hot-plug. health-Survivability and health-xpumd-gpu.survivability mean it needs a firmware reflash, which outranks any reset: a card in survivability mode also reports wedged, and no reset revives it. A device whose taints escalate that way has its existing event re-typed in place rather than gaining a second one, so a reset and a reflash cannot race on the same hardware. Escalation regenerates the event ID, which invalidates any approval naming the old one -- approving a slot reset is not approving a firmware reflash. Event IDs stay readable (evt-<node>-<type>-<bdf>) while fitting 63 bytes, because the ID becomes a recovery Job name and the Job controller copies that name into a label value. Long node names keep a prefix and gain a hash of the full name, so two nodes sharing a prefix cannot alias onto one event. This is detection only: the events it creates wait for an approval that no phase yet consumes, and no cluster resources are created. The controller test suite now runs with DRADeviceTaints=true, without which the API server prunes the device taints the specs are built on. Signed-off-by: Tuomas Katila <tuomas.katila@intel.com>
62c9bfb to
61ede4e
Compare
Add the GPURecoveryPlan reconciler's detection phase. On every reconcile it lists ResourceSlices, picks the devices matching spec.deviceId, and mirrors the ones carrying a recovery-related device taint into status.events; events whose taint has cleared are dropped again, since nothing has been done to the GPU yet and something else evidently healed it. status.state is derived from the resulting event states.
Three taint keys are recognised. health-xpumd-gpu.wedged means the GPU needs a reset, and which reset that is comes from spec.defaultResetType because the DRA driver cannot tell whether the platform's PCIe slots support hot-plug. health-Survivability and
health-xpumd-gpu.survivability mean it needs a firmware reflash, which outranks any reset: a card in survivability mode also reports wedged, and no reset revives it. A device whose taints escalate that way has its existing event re-typed in place rather than gaining a second one, so a reset and a reflash cannot race on the same hardware. Escalation regenerates the event ID, which invalidates any approval naming the old one -- approving a slot reset is not approving a firmware reflash.
Event IDs stay readable (evt---) while fitting 63 bytes, because the ID becomes a recovery Job name and the Job controller copies that name into a label value. Long node names keep a prefix and gain a hash of the full name, so two nodes sharing a prefix cannot alias onto one event.
This is detection only: the events it creates wait for an approval that no phase yet consumes, and no cluster resources are created.
The controller test suite now runs with DRADeviceTaints=true, without which the API server prunes the device taints the specs are built on.