Skip to content

recovery: detect GPUs needing recovery from ResourceSlices - #109

Open
tkatila wants to merge 1 commit into
intel:mainfrom
tkatila:recovery-detection
Open

recovery: detect GPUs needing recovery from ResourceSlices#109
tkatila wants to merge 1 commit into
intel:mainfrom
tkatila:recovery-detection

Conversation

@tkatila

@tkatila tkatila commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

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.

@tkatila

tkatila commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

This PR starts implementation of the gpurecovery controller. It will be followed by other PRs that increase functionality (approve handling, recovery Job creation etc.)

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 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 GPURecoveryPlanReconciler that lists ResourceSlice objects, detects tainted GPUs matching spec.deviceId, and maintains status.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.

Comment thread internal/controller/gpurecoveryplan_controller.go Outdated
Comment thread internal/controller/gpurecoveryplan_controller.go
Comment thread internal/controller/gpurecoveryplan_controller.go Outdated
Comment thread internal/controller/gpurecoveryplan_const.go
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants