recovery: add the GPURecoveryPlan webhook - #107
Conversation
There was a problem hiding this comment.
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
GPURecoveryPlandefaulter/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.
pfl
left a comment
There was a problem hiding this comment.
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.
927244e to
7c48262
Compare
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>
7c48262 to
f2ffab4
Compare
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.