Skip to content

recovery: add the GPURecoveryPlan webhook - #107

Merged
pfl merged 1 commit into
intel:mainfrom
tkatila:recovery-webhooks
Sep 2, 2026
Merged

recovery: add the GPURecoveryPlan webhook#107
pfl merged 1 commit into
intel:mainfrom
tkatila:recovery-webhooks

Conversation

@tkatila

@tkatila tkatila commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Add the defaulting and validating admission webhooks for GPURecoveryPlan.

The defaulter generates an "app-XXXX" ID for every spec.approvals entry that does not carry one, and fills in xpuSmi.pullPolicy. defaultResetType is deliberately left alone: neither accepted value can be inferred from the cluster, and a wrong guess is silent - the recovery Job runs a reset the platform cannot perform and exits 0 - so the validator rejects the omission instead.

The validator checks the PCI ID formats, restricts defaultResetType to the two platform resets, and rejects approval entries that can never match (both or neither of eventId/selector, duplicate IDs, persistent without a selector, node selectors that are not valid label sets). Two checks are load-bearing rather than cosmetic: spec.drain.namespacesToSkip entries that are not DNS labels would silently protect nothing, and spec.firmware.file is interpolated into the shell command a privileged reflash Job runs, so it is restricted to a character allow-list with no path components.

On update, spec.firmware is immutable while a reflash event is in-progress or blocked - both are approved and about to flash. It stays editable in the missing-firmware and waiting-approval states, where the operator is asking the admin to correct that very field.

@tkatila
tkatila requested a review from pfl as a code owner September 1, 2026 13:30
@tkatila
tkatila requested a lite review from Copilot September 1, 2026 13:30

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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds admission webhooks (defaulting + validation) for the GPURecoveryPlan CRD to enforce safety/consistency constraints and ensure required default fields are set, including firmware immutability rules during active reflash.

Changes:

  • Register new mutating and validating webhooks for GPURecoveryPlan (manifests + manager wiring).
  • Implement GPURecoveryPlan defaulter/validator logic (ID generation, spec validation, firmware immutability on update).
  • Add a comprehensive Ginkgo test suite covering defaulting and validation behaviors.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
config/webhook/manifests.yaml Adds mutating/validating webhook configuration entries for gpurecoveryplans.
cmd/main.go Registers the new GPURecoveryPlan webhook with the controller manager.
api/v1alpha1/webhook_suite_test.go Wires the new webhook into the envtest webhook suite.
api/v1alpha1/gpurecoveryplan_webhook.go Implements the webhook defaulter/validator and related validation helpers.
api/v1alpha1/gpurecoveryplan_webhook_test.go Adds unit tests for defaulting/validation/update immutability behaviors.
PROJECT Enables defaulting/validation webhooks for the GPURecoveryPlan resource.
Files not reviewed (1)
  • api/v1alpha1/zz_generated.deepcopy.go: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread api/v1alpha1/gpurecoveryplan_webhook.go
Comment thread api/v1alpha1/gpurecoveryplan_webhook.go Outdated
Comment thread api/v1alpha1/gpurecoveryplan_webhook.go Outdated
Comment thread api/v1alpha1/gpurecoveryplan_webhook_test.go
Comment thread api/v1alpha1/gpurecoveryplan_webhook.go Outdated

@pfl pfl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot had a point of 64k ids, but that is probably not a real concern. Although it would perhaps be better to err out than in theory keep looping forever.

@tkatila
tkatila force-pushed the recovery-webhooks branch 2 times, most recently from 927244e to 7c48262 Compare September 2, 2026 06:37
Add the defaulting and validating admission webhooks for GPURecoveryPlan.

The defaulter generates an "app-XXXXXXXX" ID for every spec.approvals entry
that does not carry one, and fills in xpuSmi.pullPolicy. defaultResetType is
deliberately left alone: neither accepted value can be inferred from the
cluster, and a wrong guess is silent - the recovery Job runs a reset the
platform cannot perform and exits 0 - so the validator rejects the omission
instead.

The validator checks the PCI ID formats, restricts defaultResetType to the two
platform resets, and rejects approval entries that can never match (both or
neither of eventId/selector, duplicate IDs, persistent without a selector,
node selectors that are not valid label sets). Two checks are load-bearing
rather than cosmetic: spec.drain.namespacesToSkip entries that are not DNS
labels would silently protect nothing, and spec.firmware.file is interpolated
into the shell command a privileged reflash Job runs, so it is restricted to a
character allow-list with no path components.

On update, spec.firmware is immutable while a reflash event is in-progress or
blocked - both are approved and about to flash. It stays editable in the
missing-firmware and waiting-approval states, where the operator is asking the
admin to correct that very field.

Signed-off-by: Tuomas Katila <tuomas.katila@intel.com>
@pfl
pfl merged commit 6db4df0 into intel:main Sep 2, 2026
11 checks passed
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.

3 participants