diff --git a/api/v1alpha1/gpurecoveryplan_webhook.go b/api/v1alpha1/gpurecoveryplan_webhook.go index b8b46c7..cfe9cc6 100644 --- a/api/v1alpha1/gpurecoveryplan_webhook.go +++ b/api/v1alpha1/gpurecoveryplan_webhook.go @@ -214,7 +214,9 @@ func validateRecoveryPlanSpec(spec *GPURecoveryPlanSpec) error { return err } - if spec.XpuSmi.Image != "" { + if spec.XpuSmi.Image == "" { + return fmt.Errorf("spec.xpuSmi.image is required") + } else { if _, err := reference.ParseAnyReference(spec.XpuSmi.Image); err != nil { return fmt.Errorf("spec.xpuSmi.image %q is not a valid image reference: %w", spec.XpuSmi.Image, err) } diff --git a/api/v1alpha1/gpurecoveryplan_webhook_test.go b/api/v1alpha1/gpurecoveryplan_webhook_test.go index 0bee830..cd03f63 100644 --- a/api/v1alpha1/gpurecoveryplan_webhook_test.go +++ b/api/v1alpha1/gpurecoveryplan_webhook_test.go @@ -28,6 +28,9 @@ func validPlan() *GPURecoveryPlan { Spec: GPURecoveryPlanSpec{ DeviceID: "0x1234", DefaultResetType: RecoveryTypeSlot, + XpuSmi: XpuSmiSpec{ + Image: "intel/xpusmi:devel", + }, }, } } @@ -394,6 +397,16 @@ var _ = Describe("GPURecoveryPlan Webhook", func() { Expect(err).NotTo(HaveOccurred()) }) + It("should reject missing xpuSmi.image", func() { + fw := validFW() + fw.Source = FirmwareSource{} + obj.Spec.Firmware = fw + obj.Spec.XpuSmi.Image = "" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("xpuSmi.image")) + }) + It("should reject missing source (no container and no volume)", func() { fw := validFW() fw.Source = FirmwareSource{} diff --git a/internal/controller/gpurecoveryplan_const.go b/internal/controller/gpurecoveryplan_const.go index a247a75..017758d 100644 --- a/internal/controller/gpurecoveryplan_const.go +++ b/internal/controller/gpurecoveryplan_const.go @@ -57,6 +57,17 @@ const ( reasonWedged = "gpu-wedged" reasonSurvivability = "survivability-mode" + // Container names for the recovery Jobs. + resetJobContainer = "resetter" + reflashJobContainer = "updater" + reflashCopyContainer = "fw-copy" + + // firmwareImageDir is the directory a firmware image is expected to carry its .bin files in. + firmwareImageDir = "/fwupdate" + + // reflashStagingDir is where the fw-copy initContainer stages the firmware inside the Job's emptyDir. + reflashStagingDir = "/update" + // maxStatusMessages is the maximum number of entries kept in status.messages. maxStatusMessages = 50 @@ -77,6 +88,11 @@ const ( // defaultDrainTimeout mirrors the CRD default for spec.drain.timeoutSeconds. defaultDrainTimeout = 300 * time.Second + // defaultResetJobTimeout and defaultReflashJobTimeout mirror the CRD defaults for + // spec.timeouts.resetSeconds and spec.timeouts.reflashSeconds + defaultResetJobTimeout int64 = 300 + defaultReflashJobTimeout int64 = 600 + // maxRecoveryNameLen is the hard ceiling on a recovery Job name, and therefore on the // event ID it is built from. // diff --git a/internal/controller/gpurecoveryplan_controller.go b/internal/controller/gpurecoveryplan_controller.go index cd450c7..a03a05b 100644 --- a/internal/controller/gpurecoveryplan_controller.go +++ b/internal/controller/gpurecoveryplan_controller.go @@ -34,6 +34,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" "k8s.io/klog/v2" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -48,6 +49,10 @@ type GPURecoveryPlanReconciler struct { client.Client Scheme *runtime.Scheme Opts ControllerOpts + + // imgVerify is the pre-flight registry check run before a recovery Job is created. An + // interface so tests can answer for a registry they do not have. + imgVerify ContentImageVerifier } // +kubebuilder:rbac:groups=intel.com,resources=gpurecoveryplans,verbs=get;list;watch;update;patch @@ -378,7 +383,8 @@ func (r *GPURecoveryPlanReconciler) processApprovals(ctx context.Context, plan * // blocked is admitted alongside waiting-approval if evt.State != intelv1a1.RecoveryEventStateWaitingApproval && - evt.State != intelv1a1.RecoveryEventStateBlocked { + evt.State != intelv1a1.RecoveryEventStateBlocked && + evt.State != intelv1a1.RecoveryEventStateMissingFirmware { continue } @@ -411,6 +417,11 @@ func (r *GPURecoveryPlanReconciler) processApprovals(ctx context.Context, plan * continue } + // Every image the Job is about to pull has to resolve in its registry first. + if !ensureImagesUsable(r.imgVerify, r.Opts.SecretName, ctx, plan, evt) { + continue + } + // A reset needs the node emptied first, which spans several reconciles; its Job is created // by processDrains once the node is clear. Anything that resets nothing goes straight to // the Job. @@ -428,7 +439,8 @@ func (r *GPURecoveryPlanReconciler) processApprovals(ctx context.Context, plan * } // Only consume a one-shot approval once the event has actually left waiting-approval — - // into draining, or straight to in-progress. + // into draining, or straight to in-progress. An event parked in missing-firmware never + // started, so its approval stays and fires again once spec.firmware is filled in. if !approval.Persistent && (evt.State == intelv1a1.RecoveryEventStateInProgress || evt.State == intelv1a1.RecoveryEventStateDraining) { @@ -679,6 +691,12 @@ func (r *GPURecoveryPlanReconciler) reconcileDrainTaints(ctx context.Context, pl wanted[plan.Status.Events[i].NodeName] = struct{}{} } } + + // An event held back by a failed image check is deliberately absent, unlike a blocked one. A + // block clears by itself, usually within minutes, so holding the node is cheap. An unpullable + // image waits on a person and may wait indefinitely, and a NoSchedule taint parked on a working + // node for the lifetime of a config typo takes real capacity out of the cluster. Such an event + // reads as plain waiting-approval here: no taint until an approval actually starts a recovery. } r.untaintNodesExcept(ctx, plan, wanted) @@ -872,6 +890,13 @@ func (r *GPURecoveryPlanReconciler) prepareRecoveryJob(job *batch.Job, plan *int klog.Warning(warning) } + // Bound how long the recovery may run. Without a deadline a Job whose pod never gets anywhere — + // an xpu-smi that hangs on a card that has stopped answering at all — holds the node's drain + // taint and the event's in-progress state indefinitely, and nothing else in the reconcile is + // watching a clock once the Job exists. Overwrites the template's own value, which is the same + // number as the CRD default and is here for objects that bypassed defaulting. + job.Spec.ActiveDeadlineSeconds = ptr.To(recoveryJobTimeout(plan, evt)) + // Pin the pod to the node hosting the affected GPU. job.Spec.Template.Spec.NodeName = evt.NodeName @@ -881,10 +906,6 @@ func (r *GPURecoveryPlanReconciler) prepareRecoveryJob(job *batch.Job, plan *int plan.Spec.Tolerations..., ) - if plan.Spec.XpuSmi.PullPolicy != "" { - job.Spec.Template.Spec.Containers[0].ImagePullPolicy = core.PullPolicy(plan.Spec.XpuSmi.PullPolicy) - } - if r.Opts.SecretName != "" { job.Spec.Template.Spec.ImagePullSecrets = []core.LocalObjectReference{{Name: r.Opts.SecretName}} } @@ -896,16 +917,7 @@ func (r *GPURecoveryPlanReconciler) prepareRecoveryJob(job *batch.Job, plan *int // in-progress. func (r *GPURecoveryPlanReconciler) createRecoveryJob(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) error { if evt.RecoveryType.IsReflash() { - // A reflash writes firmware over a card that is already in survivability mode rather than - // resetting the PCIe bus, so it is a different Job built from different inputs — a firmware - // image and a file within it — which this version does not assemble yet. The event keeps - // its state and its approval, so it starts as soon as the operator can carry it out. - setEventState(evt, evt.State, "a firmware reflash is not carried out by this version of the operator") - - klog.Warningf("GPURecoveryPlan %s: event %s calls for a firmware reflash, which is not implemented; leaving it in %s", - plan.Name, evt.ID, evt.State) - - return nil + return r.createReflashJob(ctx, plan, evt) } return r.createResetJob(ctx, plan, evt) @@ -926,16 +938,9 @@ func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *in jobName := r.prepareRecoveryJob(job, plan, evt) // Inject the xpu-smi image and the reset command from the plan and the event. - for i := range job.Spec.Template.Spec.Containers { - if job.Spec.Template.Spec.Containers[i].Name == "resetter" { - if plan.Spec.XpuSmi.Image != "" { - job.Spec.Template.Spec.Containers[i].Image = plan.Spec.XpuSmi.Image - } - - job.Spec.Template.Spec.Containers[i].Args = args - - break - } + if c := containerByName(job.Spec.Template.Spec.Containers, resetJobContainer); c != nil { + applyXpuSmiImage(c, plan) + c.Args = args } if err := r.Create(ctx, job); err != nil { @@ -962,6 +967,80 @@ func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *in return nil } +// createReflashJob creates the firmware-reflash Job for a reflash event and moves the event to +// in-progress. +func (r *GPURecoveryPlanReconciler) createReflashJob(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) error { + fw := plan.Spec.Firmware + if fw == nil || fw.File == "" { + // The state names the problem; the message names which part of spec.firmware is missing. + r.parkForFirmware(plan, evt, "spec.firmware is not configured, so a reflash cannot be attempted", + "spec.firmware not configured") + + return nil + } + + if fw.Source.ContainerSource == nil { + r.parkForFirmware(plan, evt, + "spec.firmware.source.containerSource is not set; a reflash copies the firmware from a "+ + "container image, and volumeSource is not supported yet", + "spec.firmware.source.containerSource not configured") + + return nil + } + + job := deployments.XpuManagerFWUpdateJob() + jobName := r.prepareRecoveryJob(job, plan, evt) + + // The firmware comes out of its own image, which the initContainer copies into the shared + // emptyDir the updater then flashes from. + if c := containerByName(job.Spec.Template.Spec.InitContainers, reflashCopyContainer); c != nil { + c.Image = fw.Source.ContainerSource.Name + } + + if c := containerByName(job.Spec.Template.Spec.Containers, reflashJobContainer); c != nil { + applyXpuSmiImage(c, plan) + + // The template's command is /bin/sh -c, so the flash is one argument: a command line, not an + // argv. Overwriting args rather than command keeps the shell, which the template needs + // anyway. + c.Args = buildFDOFlashCommand(evt.GPUBDF, fw.File) + } + + if err := r.Create(ctx, job); err != nil { + if !k8serrors.IsAlreadyExists(err) { + return fmt.Errorf("creating reflash Job %s: %w", jobName, err) + } + + // Adopted for the same reason createResetJob adopts: the name embeds the event ID and the + // attempt index, so this is the very Job this attempt wanted. + klog.V(2).Infof("GPURecoveryPlan %s: reflash Job %s already exists", plan.Name, jobName) + } + + evt.JobName = jobName + + // No message, as in createResetJob: the Job named on the event is where the detail is. + setEventState(evt, intelv1a1.RecoveryEventStateInProgress, "") + + appendMessage(plan, fmt.Sprintf("Event %s: FDO reflash Job %s created (node: %s, bdf: %s, file: %s)", + evt.ID, jobName, evt.NodeName, evt.GPUBDF, fw.File)) + + klog.Infof("GPURecoveryPlan %s: created reflash Job %s for event %s (node: %s, bdf: %s, file: %s)", + plan.Name, jobName, evt.ID, evt.NodeName, evt.GPUBDF, fw.File) + + return nil +} + +// parkForFirmware moves a reflash event to missing-firmware, recording on the event the sentence that +// says which part of spec.firmware is missing and on the plan the shorter form of the same. +func (r *GPURecoveryPlanReconciler) parkForFirmware(plan *intelv1a1.GPURecoveryPlan, + evt *intelv1a1.RecoveryEvent, stateMsg, planMsg string) { + setEventState(evt, intelv1a1.RecoveryEventStateMissingFirmware, "%s", stateMsg) + + klog.Warningf("GPURecoveryPlan %s: event %s parked in %s: %s", + plan.Name, evt.ID, intelv1a1.RecoveryEventStateMissingFirmware, stateMsg) + appendMessage(plan, fmt.Sprintf("Event %s: firmware reflash pending — %s", evt.ID, planMsg)) +} + // syncJobStatuses polls the Job of every in-progress event and moves the event to succeeded or // failed once the Job has finished. func (r *GPURecoveryPlanReconciler) syncJobStatuses(ctx context.Context, plan *intelv1a1.GPURecoveryPlan) error { // nolint:unparam @@ -1165,6 +1244,10 @@ func (r *GPURecoveryPlanReconciler) resourceSliceToPlans(ctx context.Context, ob func (r *GPURecoveryPlanReconciler) SetupWithManager(mgr ctrl.Manager, opts ControllerOpts) error { r.Opts = opts + // The API reader, not the cached client: the pull secret lives in the operator namespace but is + // read before any Job exists, and the manager cache is not started yet at Setup time. + r.imgVerify = newContentImageVerifier(mgr.GetAPIReader(), opts.Namespace) + return ctrl.NewControllerManagedBy(mgr). For(&intelv1a1.GPURecoveryPlan{}). Watches( diff --git a/internal/controller/gpurecoveryplan_controller_test.go b/internal/controller/gpurecoveryplan_controller_test.go index 8792a30..ba4b7a3 100644 --- a/internal/controller/gpurecoveryplan_controller_test.go +++ b/internal/controller/gpurecoveryplan_controller_test.go @@ -42,6 +42,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" intelv1a1 "github.com/intel/gpu-base-operator/api/v1alpha1" + "github.com/intel/gpu-base-operator/config/deployments" ) // makeTestJob builds a minimal batch.Job with the given condition pre-set, suitable for creating @@ -202,7 +203,17 @@ func createPlanForOwnerRef(p *intelv1a1.GPURecoveryPlan) { } // newTestReconciler builds a GPURecoveryPlanReconciler wired to the shared test client. +// +// The image verifier is a fake that approves everything, because there is no registry here and the +// pre-flight check gates every recovery Job: a real verifier would fail every spec below on an image +// reference that is only ever a string in a fixture. The specs that are about the check itself supply +// their own fake through newTestReconcilerVerifying. func newTestReconciler() *GPURecoveryPlanReconciler { + return newTestReconcilerVerifying(&fakeContentImageVerifier{}) +} + +// newTestReconcilerVerifying builds a reconciler whose pre-flight image check answers as given. +func newTestReconcilerVerifying(v ContentImageVerifier) *GPURecoveryPlanReconciler { return &GPURecoveryPlanReconciler{ Client: k8sClient, Scheme: k8sClient.Scheme(), @@ -210,6 +221,7 @@ func newTestReconciler() *GPURecoveryPlanReconciler { Namespace: "default", RequeueDelay: 2 * time.Second, }, + imgVerify: v, } } @@ -220,6 +232,13 @@ func reconcilePlan(ctx context.Context, name string) (reconcile.Result, error) { }) } +// reconcilePlanVerifying runs a single reconcile cycle with the given image verifier in place. +func reconcilePlanVerifying(ctx context.Context, name string, v ContentImageVerifier) (reconcile.Result, error) { + return newTestReconcilerVerifying(v).Reconcile(ctx, reconcile.Request{ + NamespacedName: types.NamespacedName{Name: name}, + }) +} + // putTaintedSlice creates or replaces a single-device ResourceSlice carrying the given taint // keys, and registers its deletion. Detection reads taints off real ResourceSlices, so the // specs drive it through the API server rather than through a hand-built object. @@ -1343,6 +1362,23 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(evt.ApprovalMatchedAt).To(BeNil()) }) + // A reset that could not pull xpu-smi says nothing about the firmware image the escalated + // reflash also needs. Carrying the recorded generation across would suppress the check on an + // image that has never been looked at, and the reflash would run — or not — on a guess. + It("should re-check the images of the escalated operation", func() { + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-esc-imgcheck", Generation: 7}, + } + evt := waitingEvent(intelv1a1.RecoveryTypeSlot) + evt.ImageVerifyGeneration = 7 + + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + + Expect(evt.ImageVerifyGeneration).To(BeZero()) + // Clearing the hold must not take the escalation message with it. + Expect(evt.StateMessage).To(ContainSubstring("escalated")) + }) + // The ID changing is invisible on the event itself, and it is the reason an approval an // admin already granted has stopped applying. Nothing else says so. It("should say on the event why the previous approval no longer applies", func() { @@ -1876,6 +1912,32 @@ var _ = Describe("GPURecoveryPlan Controller", func() { "nothing clears this but an operator filling in spec.firmware") }) + // An event whose images did not verify is parked in waiting-approval, which normally reads as + // active. Here it is not: the approval is already there and the plan is what is wrong, so + // nothing will move until an admin edits it. Reporting active would hide that behind a state + // that means "working on it". + It("should report error for an event held on an unpullable image", func() { + p := planWith(evt(intelv1a1.RecoveryEventStateWaitingApproval, 0)) + p.Generation = 4 + p.Status.Events[0].ImageVerifyGeneration = 4 + + updatePlanState(p) + + Expect(p.Status.State).To(Equal(intelv1a1.PlanStateError)) + }) + + // The recorded generation is only a verdict on the spec it was made against. Once the spec + // moves on the check has not been made yet, so the event is genuinely waiting again. + It("should report active again once the plan has moved past a held generation", func() { + p := planWith(evt(intelv1a1.RecoveryEventStateWaitingApproval, 0)) + p.Generation = 5 + p.Status.Events[0].ImageVerifyGeneration = 4 + + updatePlanState(p) + + Expect(p.Status.State).To(Equal(intelv1a1.PlanStateActive)) + }) + // The whole point of the field is answering "does this need me?" — one healthy event in // flight must not mask a stuck one. It("should let error outrank active", func() { @@ -2370,7 +2432,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", MaxRetries: 3, - Drain: intelv1a1.DrainSpec{Enable: ptr.To(false)}, + XpuSmi: intelv1a1.XpuSmiSpec{ + Image: "local/xpusmi:devel", + }, + Drain: intelv1a1.DrainSpec{Enable: ptr.To(false)}, Approvals: []intelv1a1.RecoveryApproval{ { ID: "sel-persistent", @@ -2405,10 +2470,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { "a persistent approval is standing policy and must survive firing") }) - // A reflash cannot be carried out yet, so its approval must stay unspent: consuming it - // would leave the admin's decision spent on nothing, and they would have to approve again - // once the operator can do the work. - It("should park an approved reflash without consuming the approval", func() { + // A reflash the operator cannot build a Job for must leave its approval unspent: consuming it + // would spend the admin's decision on nothing, and they would have to approve again once + // spec.firmware told the operator what to flash. + It("should park a reflash with no firmware configured without consuming the approval", func() { r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{ @@ -2438,9 +2503,9 @@ var _ = Describe("GPURecoveryPlan Controller", func() { r.processApprovals(ctx, p) evt := p.Status.Events[0] - Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateMissingFirmware)) Expect(evt.JobName).To(BeEmpty()) - Expect(evt.StateMessage).To(ContainSubstring("reflash")) + Expect(evt.StateMessage).To(ContainSubstring("spec.firmware")) Expect(p.Spec.Approvals[0].Consumed).To(BeFalse(), "an approval that produced no Job must stay available") @@ -2449,7 +2514,60 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Name: "recovery-evt-reflash-park-0", Namespace: "default", }, job) Expect(errors.IsNotFound(err)).To(BeTrue(), - "a reflash event must not be answered with a reset Job") + "a parked reflash must not leave a Job behind") + }) + + // The other half of the above: once spec.firmware says what to flash, the retained approval + // carries the reflash through on the next pass without a second admin action. + It("should consume the approval once the reflash Job is created", func() { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-reflash-resume"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, + Firmware: &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: "registry/fw:v2"}, + }, + File: "gfx.bin", + }, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-reflash-resume", EventID: "evt-reflash-resume"}, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-reflash-resume", NodeName: "node01", GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeReflash}, + Reason: reasonSurvivability, + // Where the spec above left the event: parked, approval still in place. + State: intelv1a1.RecoveryEventStateMissingFirmware, + }, + }, + }, + } + + createPlanForOwnerRef(p) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: "recovery-evt-reflash-resume-0", Namespace: "default", + }}) + }) + + r.processApprovals(ctx, p) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress), + "missing-firmware must be re-examined once the plan is corrected; messages: %v", p.Status.Messages) + Expect(evt.JobName).To(Equal("recovery-evt-reflash-resume-0")) + Expect(p.Spec.Approvals[0].Consumed).To(BeTrue(), + "the retained approval is spent by the reflash it eventually authorised") }) }) @@ -2464,6 +2582,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", MaxRetries: 2, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, // The drain is off so the re-approved event lands straight in in-progress; // what is under test is the retry counter and the approval, not the drain. Drain: intelv1a1.DrainSpec{Enable: ptr.To(false)}, @@ -2516,6 +2635,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", MaxRetries: 2, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, Approvals: []intelv1a1.RecoveryApproval{ { ID: "sel-approval", @@ -2725,6 +2845,106 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) + // The deadline is the only clock running once a recovery Job exists: nothing in the reconcile + // gives up on an in-progress event, so an xpu-smi that hangs on a card that has stopped answering + // would hold the node's drain taint and the event's state indefinitely. + Context("Recovery Job timeouts", func() { + timeoutFor := func(rt intelv1a1.RecoveryType, t intelv1a1.RecoveryTimeoutsSpec) int64 { + plan := &intelv1a1.GPURecoveryPlan{Spec: intelv1a1.GPURecoveryPlanSpec{Timeouts: t}} + evt := &intelv1a1.RecoveryEvent{RecoveryType: intelv1a1.RecoveryTypeSpec{Type: rt}} + + return recoveryJobTimeout(plan, evt) + } + + DescribeTable("should take the deadline for the kind of recovery being run", + func(rt intelv1a1.RecoveryType, t intelv1a1.RecoveryTimeoutsSpec, want int64) { + Expect(timeoutFor(rt, t)).To(Equal(want)) + }, + Entry("a reset from resetSeconds", intelv1a1.RecoveryTypeSlot, + intelv1a1.RecoveryTimeoutsSpec{ResetSeconds: 45, ReflashSeconds: 900}, int64(45)), + Entry("a reflash from reflashSeconds", intelv1a1.RecoveryTypeReflash, + intelv1a1.RecoveryTimeoutsSpec{ResetSeconds: 45, ReflashSeconds: 900}, int64(900)), + // Cross-wiring the two is the mistake worth pinning: a reflash cut off after a reset's + // deadline leaves the card part-written, which is worse than the state it started in. + Entry("a reflash when only resetSeconds is set", intelv1a1.RecoveryTypeReflash, + intelv1a1.RecoveryTimeoutsSpec{ResetSeconds: 45}, defaultReflashJobTimeout), + Entry("a reset when only reflashSeconds is set", intelv1a1.RecoveryTypeSlot, + intelv1a1.RecoveryTimeoutsSpec{ReflashSeconds: 900}, defaultResetJobTimeout), + // spec.timeouts is defaulted by the CRD, so an empty one means an object that never + // reached the API server. A Job with no deadline at all is the one outcome that must not + // happen: activeDeadlineSeconds is what ends a hung recovery. + Entry("a reset with no timeouts at all", intelv1a1.RecoveryTypeSlot, + intelv1a1.RecoveryTimeoutsSpec{}, defaultResetJobTimeout), + Entry("a reflash with no timeouts at all", intelv1a1.RecoveryTypeReflash, + intelv1a1.RecoveryTimeoutsSpec{}, defaultReflashJobTimeout), + ) + + // The fallbacks exist to reproduce what the object would have been given had it been + // defaulted, so three places have to agree: these constants, the CRD defaults, and the + // templates' own activeDeadlineSeconds. The CRD side is checked by the webhook suite; this is + // the templates. + DescribeTable("should fall back to the deadline the Job template carries", + func(tmpl *batch.Job, want int64) { + Expect(tmpl.Spec.ActiveDeadlineSeconds).To(HaveValue(Equal(want))) + }, + Entry("reset", deployments.XpuManagerResetJob(), defaultResetJobTimeout), + Entry("reflash", deployments.XpuManagerFWUpdateJob(), defaultReflashJobTimeout), + ) + + DescribeTable("should put the plan's deadline on the Job it creates", + func(planName, evtID string, rt intelv1a1.RecoveryType, want int64) { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: planName}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "local/xpusmi:devel"}, + Timeouts: intelv1a1.RecoveryTimeoutsSpec{ResetSeconds: 42, ReflashSeconds: 1200}, + Firmware: &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: "registry/fw:v2"}, + }, + File: "gfx.bin", + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{{ + ID: evtID, + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: rt}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + LastUpdated: ptr.To(metav1.Now()), + }}, + }, + } + + createPlanForOwnerRef(p) + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + job := &batch.Job{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: recoveryJobName(evtID, 0), Namespace: "default", + }, job)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + Expect(job.Spec.ActiveDeadlineSeconds).To(HaveValue(Equal(want)), + "the template's own deadline must not survive the plan's") + }, + Entry("a reset Job", "plan-deadline-reset", "evt-deadline-reset", + intelv1a1.RecoveryTypeSlot, int64(42)), + Entry("a reflash Job", "plan-deadline-reflash", "evt-deadline-reflash", + intelv1a1.RecoveryTypeReflash, int64(1200)), + ) + }) + // Jobs are kept for as long as the event they belong to, so an admin looking at a GPU can still // read the pod that touched it. That is only true if the terminal-Job handling moves the name // into pastJobs rather than deleting the object. @@ -2874,6 +3094,9 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{ + Image: "local/xpusmi:devel", + }, }, Status: intelv1a1.GPURecoveryPlanStatus{ Events: []intelv1a1.RecoveryEvent{ @@ -2951,7 +3174,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(job.Spec.Template.Spec.Tolerations).To(ContainElement( core.Toleration{Key: "extra", Operator: core.TolerationOpExists})) - resetter := findJobContainer(job, "resetter") + resetter := containerByName(job.Spec.Template.Spec.Containers, resetJobContainer) Expect(resetter).NotTo(BeNil()) Expect(resetter.Image).To(Equal("registry/xpu-smi:v1")) Expect(resetter.ImagePullPolicy).To(Equal(core.PullAlways)) @@ -2959,24 +3182,6 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(resetter.Args).To(Equal([]string{"config", "-d", "0000:02:00.0", "--coldreset"})) }) - // pullPolicy is defaulted by both the CRD and the webhook, so an empty one means an object - // that never reached the API server. Keeping the template's IfNotPresent then matters: - // leaving the field empty hands the choice to the kubelet, which picks Always for a - // ":latest" image — a pull the broken node may not be able to make. - It("should keep the template's pull policy when the plan states none", func() { - r := newTestReconciler() - p := planWithEvent("plan-own-nopolicy", "evt-own-nopolicy", intelv1a1.RecoveryTypeSBR) - p.Spec.XpuSmi = intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"} - - Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) - - job := expectOwned("recovery-evt-own-nopolicy-0", p) - - resetter := findJobContainer(job, "resetter") - Expect(resetter).NotTo(BeNil()) - Expect(resetter.ImagePullPolicy).To(Equal(core.PullIfNotPresent)) - }) - It("should give the Job the operator's own pull secret", func() { r := newTestReconciler() r.Opts.SecretName = "operator-pull-secret" @@ -3047,6 +3252,509 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) + // A card in survivability mode has firmware that no reset can fix, so the recovery is to write a + // known-good image over it. That is a different Job from a reset — a firmware image, an + // initContainer that stages it, and a shell command line rather than an argv — and every part the + // operator fills in fails silently inside a pod if it is filled in wrongly. + Context("Reflash Jobs", func() { + const ( + reflashBDF = "0000:4b:00.0" + reflashFile = "gfx_fw.bin" + fwImage = "registry.example.com/intel/gpu-fw:2026.1" + ) + + // reflashPlan creates a plan (in the API server, so the Job's owner reference has a UID) + // carrying one reflash event ready to be acted on, so createRecoveryJob is the whole of what + // these specs drive. + reflashPlan := func(planName, evtID string, fw *intelv1a1.FirmwareSpec) *intelv1a1.GPURecoveryPlan { + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: planName}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, + Firmware: fw, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{{ + ID: evtID, + NodeName: "node07", + GPUBDF: reflashBDF, + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeReflash}, + Reason: reasonSurvivability, + State: intelv1a1.RecoveryEventStateWaitingApproval, + LastUpdated: ptr.To(metav1.Now()), + }}, + }, + } + + createPlanForOwnerRef(p) + + return p + } + + containerFirmware := func() *intelv1a1.FirmwareSpec { + return &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: fwImage}, + }, + File: reflashFile, + } + } + + getJob := func(name string) *batch.Job { + job := &batch.Job{} + Expect(k8sClient.Get(ctx, + types.NamespacedName{Name: name, Namespace: "default"}, job)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + return job + } + + expectNoJob := func(name string) { + err := k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: "default"}, &batch.Job{}) + Expect(errors.IsNotFound(err)).To(BeTrue(), "a parked reflash must not leave a Job behind") + } + + It("should build the flash Job from the firmware-update template", func() { + r := newTestReconciler() + p := reflashPlan("plan-reflash-build", "evt-reflash-build", containerFirmware()) + p.Spec.XpuSmi = intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:v1", PullPolicy: "Always"} + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(evt.JobName).To(Equal("recovery-evt-reflash-build-0")) + + job := getJob(evt.JobName) + Expect(job.Labels).To(HaveKeyWithValue(recoveryJobLabelPlan, "plan-reflash-build")) + Expect(job.Labels).To(HaveKeyWithValue(recoveryJobLabelEvent, "evt-reflash-build")) + Expect(job.Spec.Template.Spec.NodeName).To(Equal("node07")) + + // The firmware comes out of its own image, which is the initContainer's whole job. + copyC := containerByName(job.Spec.Template.Spec.InitContainers, reflashCopyContainer) + Expect(copyC).NotTo(BeNil()) + Expect(copyC.Image).To(Equal(fwImage)) + + flashC := containerByName(job.Spec.Template.Spec.Containers, reflashJobContainer) + Expect(flashC).NotTo(BeNil()) + Expect(flashC.Image).To(Equal("registry/xpu-smi:v1")) + Expect(flashC.ImagePullPolicy).To(Equal(core.PullAlways)) + + // The template runs /bin/sh -c, so the flash has to stay one command line: replacing the + // command with an argv, as a reset does, would leave the shell nothing to run. + Expect(flashC.Command).To(Equal([]string{"/bin/sh", "-c"})) + Expect(flashC.Args).To(Equal(buildFDOFlashCommand(reflashBDF, reflashFile))) + + // An admin who has to check what was flashed onto which card reads this, not the pod log. + Expect(p.Status.Messages).To(ContainElement(SatisfyAll( + ContainSubstring(reflashBDF), + ContainSubstring(reflashFile), + ContainSubstring(evt.JobName), + ))) + }) + + // The two halves of the reflash are wired together by convention, not by anything either side + // checks: the initContainer copies one directory into the staging volume, and xpu-smi is + // handed a path under it. If the template's directories and the operator's constants drift + // apart the Job still starts and fails minutes later, from inside a pod, on a missing file. + It("should flash from the directory the initContainer stages into", func() { + tmpl := deployments.XpuManagerFWUpdateJob() + + copyC := containerByName(tmpl.Spec.Template.Spec.InitContainers, reflashCopyContainer) + Expect(copyC).NotTo(BeNil()) + Expect(copyC.Args).To(HaveLen(1)) + Expect(copyC.Args[0]).To(SatisfyAll( + ContainSubstring(firmwareImageDir), + ContainSubstring(reflashStagingDir), + ), "the copy must read where firmwareImagePath looks and write where the flash reads") + + // mountedAt returns the name of the volume the container sees at the given path, so the + // two containers can be shown to be talking about the same emptyDir. + mountedAt := func(c *core.Container, path string) string { + for i := range c.VolumeMounts { + if c.VolumeMounts[i].MountPath == path { + return c.VolumeMounts[i].Name + } + } + + return "" + } + + flashC := containerByName(tmpl.Spec.Template.Spec.Containers, reflashJobContainer) + Expect(flashC).NotTo(BeNil()) + Expect(mountedAt(copyC, reflashStagingDir)).NotTo(BeEmpty()) + Expect(mountedAt(flashC, reflashStagingDir)).To(Equal(mountedAt(copyC, reflashStagingDir))) + + Expect(strings.Join(buildFDOFlashCommand(reflashBDF, reflashFile), " ")).To( + ContainSubstring(reflashStagingDir + "/" + reflashFile)) + Expect(firmwareImagePath(reflashFile)).To(Equal(firmwareImageDir + "/" + reflashFile)) + }) + + // -y because there is no terminal to answer the prompt on, and --force because the card is in + // survivability mode: xpu-smi otherwise declines to write an image it judges no newer than + // what is on the device, and what is on the device is exactly what has to go. + It("should force an unattended FDO flash", func() { + Expect(strings.Join(buildFDOFlashCommand("0000:02:00.0", "fw.bin"), " ")).To( + Equal("xpu-smi updatefw -d 0000:02:00.0 -t FDO -f /update/fw.bin -y --force")) + }) + + DescribeTable("should park in missing-firmware rather than build an unusable Job", + func(planName, evtID string, fw *intelv1a1.FirmwareSpec, wantMsg string) { + r := newTestReconciler() + p := reflashPlan(planName, evtID, fw) + + // Not an error: nothing has gone wrong in the cluster, the plan is simply not + // finished. Returning one would put the whole reconcile into backoff and pile the + // same message into status.errors on every retry. + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateMissingFirmware)) + Expect(evt.JobName).To(BeEmpty()) + Expect(evt.StateMessage).To(ContainSubstring(wantMsg)) + Expect(p.Status.Messages).To(ContainElement(ContainSubstring(wantMsg))) + + expectNoJob(recoveryJobName(evtID, 0)) + }, + Entry("no firmware at all", "plan-reflash-nofw", "evt-reflash-nofw", + nil, "spec.firmware"), + Entry("a source but no file", "plan-reflash-nofile", "evt-reflash-nofile", + &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: fwImage}, + }, + }, "spec.firmware"), + // volumeSource is accepted by the CRD but not acted on: the reflash Job copies firmware + // out of a container image. Parking says so rather than building a Job whose + // initContainer would copy from an image that holds no firmware. + Entry("a volume source, which is not implemented", "plan-reflash-vol", "evt-reflash-vol", + &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + VolumeSource: &intelv1a1.VolumeFirmwareSource{Name: "fw-pvc"}, + }, + File: reflashFile, + }, "containerSource"), + ) + }) + + // Every image a recovery Job pulls is checked against its registry first. A Job pinned to an + // image that does not resolve is not a fast failure: it reports in-progress while its pod sits in + // ImagePullBackOff until activeDeadlineSeconds expires, so a mistyped reference reads as minutes + // of recovery followed by a failure that names the Job rather than the typo. + Context("Pre-flight image verification", func() { + const ( + imgBDF = "0000:5e:00.0" + imgFile = "fdo.bin" + imgFW = "registry.example.com/fw:1.0" + imgSmi = "registry.example.com/xpu-smi:1.0" + ) + + // imgPlan creates a plan whose single event is approved by a blanket EventID approval and + // whose drain is off, so processApprovals runs the gate and then goes straight to the Job. + imgPlan := func(planName, evtID string, rt intelv1a1.RecoveryType) *intelv1a1.GPURecoveryPlan { + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: planName}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + Drain: intelv1a1.DrainSpec{Enable: ptr.To(false)}, + XpuSmi: intelv1a1.XpuSmiSpec{Image: imgSmi}, + Approvals: []intelv1a1.RecoveryApproval{{ID: "app-" + evtID, EventID: evtID}}, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{{ + ID: evtID, + NodeName: "node11", + GPUBDF: imgBDF, + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: rt}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + LastUpdated: ptr.To(metav1.Now()), + }}, + }, + } + + if rt == intelv1a1.RecoveryTypeReflash { + p.Spec.Firmware = &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: imgFW}, + }, + File: imgFile, + } + } + + createPlanForOwnerRef(p) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: recoveryJobName(evtID, 0), Namespace: "default", + }}) + }) + + return p + } + + It("should check the xpu-smi image before creating a reset Job", func() { + fake := &fakeContentImageVerifier{} + r := newTestReconcilerVerifying(fake) + r.Opts.SecretName = "operator-pull-secret" + + p := imgPlan("plan-img-reset", "evt-img-reset", intelv1a1.RecoveryTypeSlot) + + r.processApprovals(ctx, p) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + + // No Files: nothing inside the xpu-smi image is the operator's business, and a content + // check would stream hundreds of megabytes to learn nothing. The operator's own pull + // secret has to be there, or a private registry answers "unauthorized" for an image the + // kubelet would have pulled perfectly well. + Expect(fake.requests).To(ConsistOf(ImageVerifyRequest{ + Image: imgSmi, + PullSecret: "operator-pull-secret", + })) + }) + + It("should check the firmware image for its file as well as its existence", func() { + fake := &fakeContentImageVerifier{} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-reflash", "evt-img-reflash", intelv1a1.RecoveryTypeReflash) + + r.processApprovals(ctx, p) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(fake.requests).To(HaveLen(2)) + + // An image that exists but does not carry spec.firmware.file fails the same way a missing + // image does, only later and inside the initContainer, where the diagnostic is a shell + // error in a pod log. No checksum is asked for: the plan does not declare one. + Expect(fake.requests[1]).To(Equal(ImageVerifyRequest{ + Image: imgFW, + Files: []ImageFile{{Name: firmwareImagePath(imgFile)}}, + })) + }) + + // The xpu-smi image and the firmware image can live on different registries, only one of + // which serves a certificate the operator cannot chase. Folding the two opt-outs together + // would silently widen whichever one the admin did not ask for. + It("should carry each image's own TLS opt-out", func() { + fake := &fakeContentImageVerifier{} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-tls", "evt-img-tls", intelv1a1.RecoveryTypeReflash) + p.Spec.Firmware.Source.ContainerSource.InsecureSkipTLSVerify = true + + r.processApprovals(ctx, p) + + Expect(fake.requests).To(HaveLen(2)) + Expect(fake.requests[0].InsecureSkipTLSVerify).To(BeFalse()) + Expect(fake.requests[1].InsecureSkipTLSVerify).To(BeTrue()) + }) + + // Pull policy Never means the kubelet never contacts a registry: the image is on the node + // already, put there by something outside Kubernetes. Verifying it against a registry that + // may not even hold it would park a recovery whose image is sitting right there, leaving the + // card broken. + It("should not check an image the kubelet will not pull", func() { + fake := &fakeContentImageVerifier{err: fmt.Errorf("not in any registry")} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-never", "evt-img-never", intelv1a1.RecoveryTypeSlot) + p.Spec.XpuSmi.PullPolicy = string(core.PullNever) + + r.processApprovals(ctx, p) + + Expect(fake.requests).To(BeEmpty()) + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + }) + + // The pull policy is spec.xpuSmi's; the firmware image is pulled by the initContainer at + // whatever the template says. A preloaded xpu-smi therefore says nothing about the firmware. + It("should still check the firmware image when xpu-smi is preloaded", func() { + fake := &fakeContentImageVerifier{} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-never-fw", "evt-img-never-fw", intelv1a1.RecoveryTypeReflash) + p.Spec.XpuSmi.PullPolicy = string(core.PullNever) + + r.processApprovals(ctx, p) + + Expect(fake.requests).To(HaveLen(1)) + Expect(fake.requests[0].Image).To(Equal(imgFW)) + }) + + It("should skip the check entirely when the plan asks it to", func() { + fake := &fakeContentImageVerifier{err: fmt.Errorf("registry unreachable")} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-skip", "evt-img-skip", intelv1a1.RecoveryTypeSlot) + p.Spec.SkipImageVerification = true + + r.processApprovals(ctx, p) + + Expect(fake.requests).To(BeEmpty()) + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress), + "an air-gapped cluster must be able to opt out and still recover its GPUs") + }) + + It("should hold the recovery and keep the approval when an image cannot be pulled", func() { + fake := &fakeContentImageVerifier{err: fmt.Errorf("MANIFEST_UNKNOWN")} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-hold", "evt-img-hold", intelv1a1.RecoveryTypeSlot) + + r.processApprovals(ctx, p) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) + Expect(evt.JobName).To(BeEmpty()) + + // The state says "waiting for an approval" while an approval is sitting right there, so + // the message is the only thing that tells an admin what is actually wrong, and it has to + // name the field to correct as well as the failure. + Expect(evt.StateMessage).To(SatisfyAll( + ContainSubstring("spec.xpuSmi.image"), + ContainSubstring(imgSmi), + ContainSubstring("MANIFEST_UNKNOWN"), + )) + Expect(evt.ImageVerifyGeneration).To(Equal(p.Generation)) + + Expect(p.Spec.Approvals[0].Consumed).To(BeFalse(), + "correcting the reference must be enough; the admin must not have to approve twice") + Expect(p.Status.Messages).To(ContainElement(ContainSubstring("spec.xpuSmi.image"))) + + err := k8sClient.Get(ctx, types.NamespacedName{ + Name: recoveryJobName("evt-img-hold", 0), Namespace: "default", + }, &batch.Job{}) + Expect(errors.IsNotFound(err)).To(BeTrue(), "no Job may exist for a held recovery") + }) + + // What failed is a value in the plan, and only an admin edit can change that answer — which + // is also what advances metadata.generation. Retrying on the reconcile cadence would re-ask a + // question the spec has already settled, and append a message every time. + It("should not re-check or re-report until the plan changes", func() { + fake := &fakeContentImageVerifier{err: fmt.Errorf("MANIFEST_UNKNOWN")} + r := newTestReconcilerVerifying(fake) + + p := imgPlan("plan-img-once", "evt-img-once", intelv1a1.RecoveryTypeSlot) + + r.processApprovals(ctx, p) + afterFirst := len(p.Status.Messages) + + r.processApprovals(ctx, p) + r.processApprovals(ctx, p) + + Expect(fake.requests).To(HaveLen(1), "one registry round trip per spec version") + Expect(p.Status.Messages).To(HaveLen(afterFirst)) + }) + + It("should resume the recovery once the plan is corrected", func() { + failing := &fakeContentImageVerifier{err: fmt.Errorf("MANIFEST_UNKNOWN")} + p := imgPlan("plan-img-resume", "evt-img-resume", intelv1a1.RecoveryTypeSlot) + + newTestReconcilerVerifying(failing).processApprovals(ctx, p) + Expect(p.Status.Events[0].ImageVerifyGeneration).To(Equal(p.Generation)) + + // What an admin fixing the reference does: a spec write, which the API server answers + // with a new generation. The approval is still there, unconsumed. + p.Spec.XpuSmi.Image = "registry.example.com/xpu-smi:1.1" + p.Generation++ + + passing := &fakeContentImageVerifier{} + newTestReconcilerVerifying(passing).processApprovals(ctx, p) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress), + "messages: %v", p.Status.Messages) + Expect(evt.ImageVerifyGeneration).To(BeZero(), + "a cleared hold must not make the next failure look like an old one") + Expect(evt.StateMessage).To(BeEmpty(), + "a stale explanation on a running recovery points at a problem that is gone") + Expect(passing.requests).To(HaveLen(1)) + Expect(passing.requests[0].Image).To(Equal("registry.example.com/xpu-smi:1.1")) + + // The transition out of a hold is worth a line: the plan's own history is where an admin + // checks whether their edit was the one that worked. + Expect(p.Status.Messages).To(ContainElement(ContainSubstring("resuming"))) + Expect(p.Spec.Approvals[0].Consumed).To(BeTrue()) + }) + + // A block clears by itself within minutes, so holding the node through one is cheap. An + // unpullable image waits on a person and may wait indefinitely, and a NoSchedule taint parked + // on a working node for the lifetime of a config typo takes real capacity out of the cluster. + It("should not cordon the node while a recovery is held on an image", func() { + const heldNode = "img-held-node" + + node := &core.Node{ObjectMeta: metav1.ObjectMeta{Name: heldNode}} + Expect(k8sClient.Create(ctx, node)).To(Succeed()) + DeferCleanup(func() { + fresh := &core.Node{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: heldNode}, fresh); err == nil { + fresh.Spec.Taints = nil + _ = k8sClient.Update(ctx, fresh) + _ = k8sClient.Delete(ctx, fresh) + } + }) + + putTaintedSlice(ctx, "slice-img-held", heldNode, "0x1234", imgBDF, deviceTaintKeyReset) + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: "plan-img-nocordon", Finalizers: []string{recoveryPlanFinalizer}, + }, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0x1234", + MaxRetries: 3, + // The drain is on, which is what makes this worth asserting: a reset would + // normally taint the node on its way to the Job. + Drain: intelv1a1.DrainSpec{Enable: ptr.To(true), TimeoutSeconds: 300}, + XpuSmi: intelv1a1.XpuSmiSpec{Image: imgSmi}, + Approvals: []intelv1a1.RecoveryApproval{{ + ID: "app-img-nocordon", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSlot}, + }}, + }, + } + Expect(k8sClient.Create(ctx, p)).To(Succeed()) + DeferCleanup(func() { + fresh := &intelv1a1.GPURecoveryPlan{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: p.Name}, fresh); err == nil { + fresh.Finalizers = nil + _ = k8sClient.Update(ctx, fresh) + _ = k8sClient.Delete(ctx, fresh) + } + }) + + _, err := reconcilePlanVerifying(ctx, p.Name, + &fakeContentImageVerifier{err: fmt.Errorf("MANIFEST_UNKNOWN")}) + Expect(err).NotTo(HaveOccurred()) + + updated := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: p.Name}, updated)).To(Succeed()) + Expect(updated.Status.Events).To(HaveLen(1)) + Expect(updated.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) + Expect(updated.Status.Events[0].DrainStartedAt).To(BeNil()) + + // The plan must say it needs attention: waiting-approval alone would read as normal. + Expect(updated.Status.State).To(Equal(intelv1a1.PlanStateError)) + + // Only the operator's own taint is asserted on: envtest runs no kubelet, so the Node + // carries node.kubernetes.io/not-ready of its own accord. + fresh := &core.Node{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: heldNode}, fresh)).To(Succeed()) + Expect(fresh.Spec.Taints).NotTo(ContainElement(HaveField("Key", recoveryTaintKey)), + "a recovery that cannot start must not take the node out of service") + }) + }) + Context("Reconcile: long node names still produce a creatable Job", func() { It("should create the recovery Job for a node name well over the limit", func() { r := newTestReconciler() @@ -3097,6 +3805,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xbeef", MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, Approvals: []intelv1a1.RecoveryApproval{{ ID: "app-any-reset", Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSlot}, @@ -3358,6 +4067,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0x1234", MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "local/xpusmi:devel"}, // Spelled out rather than left to CRD defaulting: these specs are about what // the drain does, so what it was asked to do belongs in the fixture. Drain: intelv1a1.DrainSpec{ @@ -3506,6 +4216,17 @@ var _ = Describe("GPURecoveryPlan Controller", func() { makeWorkloadPod("reflash-bystander", drainNode) key := makeDrainPlan("plan-drain-reflash", intelv1a1.RecoveryTypeReflash) + // Firmware configured, so the reflash really runs: what is under test is that the Job is + // reached without a drain, not the missing-firmware parking that would also skip one. + p := fetch(key) + p.Spec.Firmware = &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: "registry/fw:v2"}, + }, + File: "gfx.bin", + } + Expect(k8sClient.Update(ctx, p)).To(Succeed()) + _, err := reconcilePlan(ctx, key.Name) Expect(err).NotTo(HaveOccurred()) @@ -3514,15 +4235,19 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // A reflash writes firmware to a device already in survivability mode, without // resetting the bus. There is nothing on the node for a drain to protect, so evicting - // unrelated workloads would be pure disruption. This version parks the reflash instead - // of carrying it out, which is what the state message says — but the point here is - // that the node was never touched on the way to that decision. + // unrelated workloads would be pure disruption. evt := updated.Status.Events[0] - Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval), + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress), "a reflash must not enter draining; messages: %v", updated.Status.Messages) - Expect(evt.StateMessage).To(ContainSubstring("reflash")) + Expect(evt.JobName).NotTo(BeEmpty()) Expect(evt.DrainStartedAt).To(BeNil()) + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: evt.JobName, Namespace: "default", + }}) + }) + Expect(nodeTaints(drainNode)).NotTo(ContainElement(recoveryTaint(key.Name)), "a reflash must not cordon the node") @@ -5112,14 +5837,3 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) }) - -// findJobContainer returns the named container from a Job's pod template, or nil. -func findJobContainer(job *batch.Job, name string) *core.Container { - for i := range job.Spec.Template.Spec.Containers { - if job.Spec.Template.Spec.Containers[i].Name == name { - return &job.Spec.Template.Spec.Containers[i] - } - } - - return nil -} diff --git a/internal/controller/gpurecoveryplan_helpers.go b/internal/controller/gpurecoveryplan_helpers.go index 37ef67e..e58014e 100644 --- a/internal/controller/gpurecoveryplan_helpers.go +++ b/internal/controller/gpurecoveryplan_helpers.go @@ -235,6 +235,52 @@ func recoveryTypeToArgs(bdf string, rt intelv1a1.RecoveryType) []string { } } +// buildFDOFlashCommand returns the shell command line that reflashes one GPU from a firmware file the +// fw-copy initContainer has staged in the shared volume. +func buildFDOFlashCommand(bdf, file string) []string { + return []string{"xpu-smi", "updatefw", "-d", bdf, "-t", "FDO", "-f", fmt.Sprintf("%s/%s", reflashStagingDir, file), "-y", "--force"} +} + +// firmwareImagePath returns path to the firmware file inside the firmware image. +func firmwareImagePath(file string) string { + return fmt.Sprintf("%s/%s", firmwareImageDir, file) +} + +// containerByName returns the named container from a container list, or nil. +func containerByName(containers []core.Container, name string) *core.Container { + for i := range containers { + if containers[i].Name == name { + return &containers[i] + } + } + + return nil +} + +// recoveryJobTimeout returns the activeDeadlineSeconds for the event's recovery Job. +func recoveryJobTimeout(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) int64 { + if evt.RecoveryType.IsReflash() { + if plan.Spec.Timeouts.ReflashSeconds > 0 { + return int64(plan.Spec.Timeouts.ReflashSeconds) + } + + return defaultReflashJobTimeout + } + + if plan.Spec.Timeouts.ResetSeconds > 0 { + return int64(plan.Spec.Timeouts.ResetSeconds) + } + + return defaultResetJobTimeout +} + +// applyXpuSmiImage points the container that runs xpu-smi at the image and pull policy the plan asks +// for, leaving the template's own values in place where the plan states none. +func applyXpuSmiImage(c *core.Container, plan *intelv1a1.GPURecoveryPlan) { + c.Image = plan.Spec.XpuSmi.Image + c.ImagePullPolicy = core.PullPolicy(plan.Spec.XpuSmi.PullPolicy) +} + // nodeSelectorMatches reports whether the named node carries all labels in sel. An empty // selector matches anything, mirroring how the rest of the approval selector treats unset // fields. @@ -603,7 +649,17 @@ func updatePlanState(plan *intelv1a1.GPURecoveryPlan) { intelv1a1.RecoveryEventStateDraining, intelv1a1.RecoveryEventStateInProgress: // blocked is active, not stuck: it clears on its own once the node frees up. - anyActive = true + // + // waiting-approval needs a second look. An event whose images did not verify against the + // current spec is parked there (see ensureImagesUsable) and stays until the plan is + // edited, so it is indistinguishable from one merely awaiting a decision — but the admin + // has already decided and something in the plan is wrong, which is an error. + if evt.State == intelv1a1.RecoveryEventStateWaitingApproval && + evt.ImageVerifyGeneration != 0 && evt.ImageVerifyGeneration == plan.Generation { + anyStuck = true + } else { + anyActive = true + } case intelv1a1.RecoveryEventStateMissingFirmware: // Blocked on operator configuration, not on hardware or an admin decision: @@ -774,6 +830,12 @@ func escalateEvent(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent evt.ApprovalID = "" evt.ApprovalMatchedAt = nil + // The escalated operation may not pull the same images — a reset needs xpu-smi, a reflash needs + // the firmware image too. Carrying the recorded generation over would treat a firmware image that + // has never been checked as already answered by another image's failure. Cleared before the state + // is set, so the escalation message below survives. + clearImageVerifyHold(evt) + setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, "escalated from %s to %s (%s); the approval for the previous type no longer applies", oldType, need.rt, need.reason) diff --git a/internal/controller/gpurecoveryplan_imageverific.go b/internal/controller/gpurecoveryplan_imageverific.go new file mode 100644 index 0000000..5587ba8 --- /dev/null +++ b/internal/controller/gpurecoveryplan_imageverific.go @@ -0,0 +1,137 @@ +/* +Copyright 2026 Intel Corporation. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "fmt" + + core "k8s.io/api/core/v1" + "k8s.io/klog/v2" + + intelv1a1 "github.com/intel/gpu-base-operator/api/v1alpha1" +) + +type recoveryImage struct { + image string + field string + + // insecureSkipTLSVerify carries the per-image opt-out from registry certificate validation. + insecureSkipTLSVerify bool + + // files, when non-empty, must exist in the image. Empty means the check is reachability only + files []ImageFile +} + +// recoveryImagesForEvent lists the images the Job for this event will pull, in check order. +func recoveryImagesForEvent(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) []recoveryImage { + imgs := make([]recoveryImage, 0, 2) + + // Ignore xpuSmi image verification if pull policy is Never. + if plan.Spec.XpuSmi.PullPolicy != string(core.PullNever) { + if img := plan.Spec.XpuSmi.Image; img != "" { + imgs = append(imgs, recoveryImage{ + image: img, + field: "spec.xpuSmi.image", + insecureSkipTLSVerify: plan.Spec.XpuSmi.InsecureSkipTLSVerify, + }) + } + } + + // If the event is a reflash, check also the firmware image. + if evt.RecoveryType.IsReflash() { + fw := plan.Spec.Firmware + if fw != nil && fw.File != "" && fw.Source.ContainerSource != nil && fw.Source.ContainerSource.Name != "" { + imgs = append(imgs, recoveryImage{ + image: fw.Source.ContainerSource.Name, + field: "spec.firmware.source.containerSource.name", + insecureSkipTLSVerify: fw.Source.ContainerSource.InsecureSkipTLSVerify, + files: []ImageFile{{Name: firmwareImagePath(fw.File)}}, + }) + } + } + + return imgs +} + +// holdForImageFix sends an event whose images did not verify back to waiting-approval, recording the +// spec version the check was made against so it is not repeated until the plan changes. +func holdForImageFix(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent, img recoveryImage, verifyErr error) { + evt.ImageVerifyGeneration = plan.Generation + + // The state this leaves the event in says "waiting for an approval", which is not what is + // happening — the approval is already there — so the message is mandatory here. Without it an + // admin reading status.events would see a recovery that had silently stopped. + setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, + "%s (%s) cannot be pulled: %v; correct the plan to retry (approval retained)", + img.field, img.image, verifyErr) + + klog.Warningf("GPURecoveryPlan %s: event %s held at waiting-approval: %s (%s) is not usable: %v", + plan.Name, evt.ID, img.field, img.image, verifyErr) + + appendMessage(plan, fmt.Sprintf("Event %s: recovery held back — %s", evt.ID, evt.StateMessage)) +} + +// ensureImagesUsable reports whether the recovery for this event may go ahead, verifying against the +// registry that every image its Job needs can be pulled. On failure the event is sent back to +// waiting-approval and false is returned; the approval is left unconsumed by the caller, so the +// recovery resumes on its own once the plan is corrected — one admin action, not two. +func ensureImagesUsable(imgVerify ContentImageVerifier, secretName string, ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) bool { + if plan.Spec.SkipImageVerification { + return true + } + + // Already asked, against this exact spec. The approval is still in place, so this is reached on + // every reconcile until the plan changes; going quiet is the point. + if evt.ImageVerifyGeneration != 0 && evt.ImageVerifyGeneration == plan.Generation { + klog.V(2).Infof("GPURecoveryPlan %s: event %s failed image verification at generation %d; waiting for a spec change", + plan.Name, evt.ID, plan.Generation) + + return false + } + + // Form a list of images and then check them, if not yet checked previously. + for _, img := range recoveryImagesForEvent(plan, evt) { + req := ImageVerifyRequest{ + Image: img.image, + PullSecret: secretName, + InsecureSkipTLSVerify: img.insecureSkipTLSVerify, + Files: img.files, + } + + if err := imgVerify.VerifyImage(ctx, req); err != nil { + holdForImageFix(plan, evt, img, err) + + return false + } + } + + if evt.ImageVerifyGeneration != 0 { + appendMessage(plan, fmt.Sprintf("Event %s: images verified after an earlier failure; resuming recovery", evt.ID)) + klog.Infof("GPURecoveryPlan %s: event %s images now verify; resuming", plan.Name, evt.ID) + } + + clearImageVerifyHold(evt) + + return true +} + +// clearImageVerifyHold drops what a failed image verification left on the event. +func clearImageVerifyHold(evt *intelv1a1.RecoveryEvent) { + evt.ImageVerifyGeneration = 0 + evt.StateMessage = "" +}