diff --git a/.trivyignore.yaml b/.trivyignore.yaml index 8050848..c1819ef 100644 --- a/.trivyignore.yaml +++ b/.trivyignore.yaml @@ -6,6 +6,7 @@ misconfigurations: NOTE: FW update could also use monitoring/admin access from GPU DP or DRA, and then drop privileged access. paths: - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0005 statement: "XPUM requires SYS_ADMIN for some GPU metrics" @@ -19,11 +20,13 @@ misconfigurations: - dp/dp.yaml - xpum/xpum.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0017 statement: "DP levelzero & fwupdate requires privileged access / allowPrivilegeEscalation" paths: - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0022 statement: "XPUM requires SYS_ADMIN for some GPU metrics" @@ -37,6 +40,7 @@ misconfigurations: - dra/daemonset.yaml - xpum/xpum.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0025 statement: "container_device_plugin_t is a valid SELinux profile" @@ -68,6 +72,7 @@ misconfigurations: - dp/dp.yaml - xpum/xpum.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0118 statement: "DP is not deployed to default NS but to where-ever the operator is deployed" @@ -76,6 +81,7 @@ misconfigurations: - dra/daemonset.yaml - xpum/xpum.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0125 statement: "Dockerhub.io is ok" @@ -84,12 +90,14 @@ misconfigurations: - xpum/xpum.yaml - dra/daemonset.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-KSV-0121 statement: "/sys is required for DRA" paths: - dra/daemonset.yaml - xpum/xpum-fwupdate-job.yaml + - xpum/xpum-reset-job.yaml - id: AVD-DS-0002 statement: "" diff --git a/charts/gpu-base-operator/templates/role.yaml b/charts/gpu-base-operator/templates/role.yaml index 55640d1..c9a4999 100644 --- a/charts/gpu-base-operator/templates/role.yaml +++ b/charts/gpu-base-operator/templates/role.yaml @@ -73,6 +73,7 @@ rules: resources: - clusterpolicies/finalizers - gpufirmwareupdates/finalizers + - gpurecoveryplans/finalizers verbs: - update - apiGroups: @@ -92,6 +93,8 @@ rules: verbs: - get - list + - patch + - update - watch - apiGroups: - kmm.sigs.x-k8s.io diff --git a/config/deployments/deployments.go b/config/deployments/deployments.go index 9f61eb5..23ff2a5 100644 --- a/config/deployments/deployments.go +++ b/config/deployments/deployments.go @@ -169,6 +169,13 @@ func XpuManagerFWUpdateJob() *batch.Job { return getJob(xpumFWUpdateJob).DeepCopy() } +//go:embed xpum/xpum-reset-job.yaml +var xpumResetJob []byte + +func XpuManagerResetJob() *batch.Job { + return getJob(xpumResetJob).DeepCopy() +} + // generic functions func getDaemonset(content []byte) *apps.DaemonSet { diff --git a/config/deployments/deployments_test.go b/config/deployments/deployments_test.go index f2af8e8..41f790d 100644 --- a/config/deployments/deployments_test.go +++ b/config/deployments/deployments_test.go @@ -149,6 +149,85 @@ func TestXpuFwUpdateJob(t *testing.T) { } } +func TestXpuResetJob(t *testing.T) { + job := XpuManagerResetJob() + if job == nil { + t.Error("XpuManagerResetJob returned nil") + } +} + +// The operator only ever overwrites the resetter container's image and args, so everything else +// the reset needs has to be right in the template: the /sys mount the PCIe reset writes through, +// and a restart policy that does not re-run a reset the Job controller already gave up on. +func TestXpuResetJob_ResetterContainer(t *testing.T) { + job := XpuManagerResetJob() + + c := findContainer(job.Spec.Template.Spec.Containers, "resetter") + if c == nil { + t.Fatal("resetter container not found") + } + + found := false + for _, m := range c.VolumeMounts { + if m.MountPath == "/sys" { + found = true + if m.ReadOnly { + t.Error("/sys must be mounted writable: the reset is issued by writing to sysfs") + } + } + } + if !found { + t.Error("resetter has no /sys mount") + } + + if job.Spec.Template.Spec.RestartPolicy != core.RestartPolicyNever { + t.Errorf("restartPolicy: got %v, want Never", job.Spec.Template.Spec.RestartPolicy) + } + + if job.Spec.Template.Spec.AutomountServiceAccountToken == nil { + t.Fatal("automountServiceAccountToken must be set (non-nil)") + } + if *job.Spec.Template.Spec.AutomountServiceAccountToken != false { + t.Error("automountServiceAccountToken must be false") + } +} + +func TestXpuResetJob_ResetterSecurityContext(t *testing.T) { + job := XpuManagerResetJob() + + c := findContainer(job.Spec.Template.Spec.Containers, "resetter") + if c == nil { + t.Fatal("resetter container not found") + } + if c.SecurityContext == nil { + t.Fatal("SecurityContext must be set on resetter") + } + // Privileged is deliberate here, unlike everywhere else in this file: resetting a GPU means + // writing to the device's PCIe config space through sysfs. + if c.SecurityContext.Privileged == nil || !*c.SecurityContext.Privileged { + t.Error("resetter must be privileged to issue a PCIe reset") + } + if c.SecurityContext.SeccompProfile == nil { + t.Fatal("SeccompProfile must be set on resetter") + } + if c.SecurityContext.SeccompProfile.Type != core.SeccompProfileTypeRuntimeDefault { + t.Errorf("SeccompProfile.Type: got %v, want RuntimeDefault on resetter", c.SecurityContext.SeccompProfile.Type) + } + if c.SecurityContext.Capabilities == nil { + t.Fatal("Capabilities must be set on resetter") + } + found := false + for _, cap := range c.SecurityContext.Capabilities.Drop { + if cap == allCaps { + found = true + break + } + } + if !found { + t.Error("capabilities.drop must contain ALL on resetter") + } +} + func TestOTelConfig(t *testing.T) { cfg := XpuManagerOTelConfig() if cfg == nil { diff --git a/config/deployments/xpum/xpum-reset-job.yaml b/config/deployments/xpum/xpum-reset-job.yaml new file mode 100644 index 0000000..05500a9 --- /dev/null +++ b/config/deployments/xpum/xpum-reset-job.yaml @@ -0,0 +1,69 @@ +# Copyright (C) 2026 Intel Corporation +# SPDX-License-Identifier: Apache-2.0 +# @file xpum-reset-job.yaml +# +# Template for GPU hardware reset recovery Jobs. +# The operator fills in: spec.template.spec.nodeName, containers[resetter].image, +# and containers[resetter].args with the appropriate xpu-smi reset command. +# +# Supported commands (set by the operator from the event's recoveryType): +# sbr: xpu-smi config -d --reset +# slot: xpu-smi config -d --coldreset +# amc: xpu-smi amc --gpureset -d -y +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: intel-reset-job +spec: + podFailurePolicy: + rules: + - action: FailJob + onExitCodes: + containerName: resetter + operator: NotIn + values: [0] + - action: FailJob + onPodConditions: + - type: ConfigIssue + activeDeadlineSeconds: 300 + template: + spec: + volumes: + - name: host-sys + hostPath: + path: /sys + type: Directory + automountServiceAccountToken: false + containers: + - name: resetter + image: intel/gpu-fwupdater-mock:devel # replaced by the operator (spec.xpuSmi.image) + imagePullPolicy: IfNotPresent + command: [ "/usr/local/bin/xpu-smi" ] + args: + - "config" + - "-d" + - "0000:00:00.0" # replaced by the operator + - "--reset" # replaced by the operator + resources: + requests: + cpu: 50m + memory: 32Mi + limits: + cpu: 100m + memory: 64Mi + securityContext: + readOnlyRootFilesystem: true + allowPrivilegeEscalation: true + runAsUser: 0 + privileged: true + seccompProfile: + type: RuntimeDefault + capabilities: + drop: [ "ALL" ] + volumeMounts: + - name: host-sys + mountPath: /sys + readOnly: false + imagePullSecrets: + restartPolicy: Never diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index ec46197..eb7206a 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -73,6 +73,7 @@ rules: resources: - clusterpolicies/finalizers - gpufirmwareupdates/finalizers + - gpurecoveryplans/finalizers verbs: - update - apiGroups: @@ -92,6 +93,8 @@ rules: verbs: - get - list + - patch + - update - watch - apiGroups: - kmm.sigs.x-k8s.io diff --git a/internal/controller/gpurecoveryplan_const.go b/internal/controller/gpurecoveryplan_const.go index 2e3843d..d7757ab 100644 --- a/internal/controller/gpurecoveryplan_const.go +++ b/internal/controller/gpurecoveryplan_const.go @@ -17,6 +17,19 @@ limitations under the License. package controller const ( + // recoveryPlanFinalizer is set on every live GPURecoveryPlan. It holds the object in place + // while a recovery Job is still running, so the Jobs are cleaned up rather than orphaned. + recoveryPlanFinalizer = "gpurecoveryplan.intel.com/finalizer" + + // recoveryJobLabelPlan is the label key placed on every recovery Job; its value is the name + // of the owning GPURecoveryPlan. It is what finds a plan's Jobs when status.events no longer + // names them. + recoveryJobLabelPlan = "gpurecoveryplan.intel.com/plan" + + // recoveryJobLabelEvent is the label key placed on every recovery Job; its value is the ID of + // the RecoveryEvent the Job was created for. + recoveryJobLabelEvent = "gpurecoveryplan.intel.com/event" + // DRA's device attributes // deviceAttrDeviceID is the ResourceSlice device attribute name for the PCI device ID. deviceAttrDeviceID = "pciId" diff --git a/internal/controller/gpurecoveryplan_controller.go b/internal/controller/gpurecoveryplan_controller.go index e4d3256..55e370f 100644 --- a/internal/controller/gpurecoveryplan_controller.go +++ b/internal/controller/gpurecoveryplan_controller.go @@ -18,12 +18,16 @@ package controller import ( "context" + "errors" "fmt" "reflect" + "strings" "time" - "unicode/utf8" + batch "k8s.io/api/batch/v1" + core "k8s.io/api/core/v1" resv1 "k8s.io/api/resource/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -31,10 +35,12 @@ import ( "k8s.io/klog/v2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" intelv1a1 "github.com/intel/gpu-base-operator/api/v1alpha1" + "github.com/intel/gpu-base-operator/config/deployments" ) type GPURecoveryPlanReconciler struct { @@ -43,18 +49,24 @@ type GPURecoveryPlanReconciler struct { Opts ControllerOpts } -// +kubebuilder:rbac:groups=intel.com,resources=gpurecoveryplans,verbs=get;list;watch +// +kubebuilder:rbac:groups=intel.com,resources=gpurecoveryplans,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups=intel.com,resources=gpurecoveryplans/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=intel.com,resources=gpurecoveryplans/finalizers,verbs=update // +kubebuilder:rbac:groups=resource.k8s.io,resources=resourceslices,verbs=get;list;watch +// Node labels decide whether a selector approval covers the node an event is on. +// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch + // Reconcile is the main reconciliation loop for GPURecoveryPlan. // -// The loop is triggered either by a change to a GPURecoveryPlan or by a ResourceSlice event -// routed through resourceSliceToPlans. +// The loop is triggered by a change to a GPURecoveryPlan (an admin adding an approval, or the +// operator's own status write), by a recovery Job the plan owns reaching a new state, or by a +// ResourceSlice event routed through resourceSliceToPlans. // -// Detection only, for now: it reflects the GPUs the DRA driver has tainted into status.events -// and derives status.state from them. Nothing is acted upon — every event it creates waits for -// an admin approval that no later phase yet consumes. +// The phases run in a fixed order, each reading what the one before it wrote: detection mirrors +// the tainted GPUs into status.events, approvals turn the approved ones into recovery Jobs, and +// the Job sync reports what those Jobs did. status.state is derived once, at the end, from the +// event states all of them have settled on. func (r *GPURecoveryPlanReconciler) Reconcile(ctx context.Context, req ctrl.Request) (retRes ctrl.Result, retErr error) { klog.V(2).Infof("Reconciling GPURecoveryPlan %s", req.Name) @@ -78,53 +90,162 @@ func (r *GPURecoveryPlanReconciler) Reconcile(ctx context.Context, req ctrl.Requ } }() + // Finalizer management. + if done, err := r.handleFinalizer(ctx, plan); err != nil || done { + // Deletion is blocked on an in-flight recovery Job: poll rather than fail. The status + // update in the deferred block still runs, so the "waiting for N active Job(s)" message + // reaches the CR before it disappears. + if errors.Is(err, requeueReconcileErr{}) { + return ctrl.Result{RequeueAfter: r.Opts.RequeueDelay}, nil + } + + return ctrl.Result{}, err + } + // Reflect the current cluster GPU state into status.events. if err := r.syncRecoveryEventsFromSlices(ctx, plan); err != nil { return ctrl.Result{}, fmt.Errorf("syncRecoveryEventsFromSlices: %w", err) } + // Start the recovery of every event an admin has approved. + r.processApprovals(ctx, plan) + + // Update event states from the outcomes of the Jobs they are running. + if err := r.syncJobStatuses(ctx, plan); err != nil { + return ctrl.Result{}, fmt.Errorf("syncJobStatuses: %w", err) + } + + // Drop consumed approvals no event refers to any more. + pruneConsumedApprovals(plan) + // Derive status.state from the resulting event states. - r.updatePlanState(plan) + updatePlanState(plan) + + // Requeue while a Job is in flight. A reconcile is also triggered by Job changes. + if hasActiveJobs(plan) { + return ctrl.Result{RequeueAfter: r.Opts.RequeueDelay}, nil + } return ctrl.Result{}, nil } -// persistPlan writes back whatever the reconcile phases changed on plan's status. It is called -// from Reconcile's defer and returns the error encountered, so the caller can fail the reconcile -// and get a retry with backoff. +// persistPlan writes back whatever the reconcile phases changed on plan: status first, then spec. +// It is called from Reconcile's defer and returns the first error encountered, so the caller can +// fail the reconcile and get a retry with backoff. +// +// Write ordering: status must land BEFORE spec, because the spec write (consuming a one-shot +// approval) triggers an immediate new reconcile. If that reconcile saw the old status it would +// still read the event as waiting-approval and could act on it twice. func (r *GPURecoveryPlanReconciler) persistPlan(ctx context.Context, key types.NamespacedName, orig, plan *intelv1a1.GPURecoveryPlan) error { - if reflect.DeepEqual(orig.Status, plan.Status) { + statusChanged := !reflect.DeepEqual(orig.Status, plan.Status) + specChanged := !reflect.DeepEqual(orig.Spec, plan.Spec) + + if !statusChanged && !specChanged { return nil } wantStatus := plan.Status.DeepCopy() + wantSpec := plan.Spec.DeepCopy() - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - if err := r.Get(ctx, key, plan); err != nil { - return err + var firstErr error + + if statusChanged { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, key, plan); err != nil { + return err + } + + plan.Status = *wantStatus.DeepCopy() + + return r.Status().Update(ctx, plan) + }) + if err != nil { + klog.Errorf("GPURecoveryPlan %s: failed to update status: %v", plan.Name, err) + + firstErr = fmt.Errorf("updating status: %w", err) } + } - plan.Status = *wantStatus.DeepCopy() + // Attempted even when the status write failed: an approval that has already produced a Job + // must be marked consumed, or the next pass creates a second Job for the same GPU. The + // reconcile still fails, so the lost status is rewritten on the retry. + if specChanged { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, key, plan); err != nil { + return err + } + + plan.Spec = *wantSpec.DeepCopy() - return r.Status().Update(ctx, plan) - }) - if err != nil { - klog.Errorf("GPURecoveryPlan %s: failed to update status: %v", plan.Name, err) + return r.Update(ctx, plan) + }) + if err != nil { + klog.Errorf("GPURecoveryPlan %s: failed to update spec: %v", plan.Name, err) - return fmt.Errorf("updating status: %w", err) + if firstErr == nil { + firstErr = fmt.Errorf("updating spec: %w", err) + } + } } - return nil + return firstErr +} + +// handleFinalizer keeps the finalizer on a live plan and carries out the plan's own teardown when +// it is deleted. Returns done=true when the caller must stop reconciling: either the plan is on +// its way out, or the finalizer was just added and the resulting Update has already queued +// another pass. +func (r *GPURecoveryPlanReconciler) handleFinalizer(ctx context.Context, plan *intelv1a1.GPURecoveryPlan) (done bool, err error) { + if !plan.DeletionTimestamp.IsZero() { + // A recovery Job may be mid-flight through a PCIe reset. Letting the CR go now would + // delete the Job's owner and, with it, a reset nobody is watching any more. + running, err := runningRecoveryJobs(r.Client, ctx, r.Opts.Namespace, plan) + if err != nil { + return true, fmt.Errorf("listing recovery jobs during deletion: %w", err) + } + + if len(running) > 0 { + klog.Infof("GPURecoveryPlan %s: deletion blocked, waiting for %d active recovery Job(s): %s", + plan.Name, len(running), strings.Join(running, ", ")) + appendMessage(plan, fmt.Sprintf("Deletion waiting for %d active recovery Job(s): %s", + len(running), strings.Join(running, ", "))) + + // Requeue-not-an-error: Reconcile returns this with a nil error. + return true, requeueReconcileErr{fmt.Errorf("waiting for %d active recovery job(s)", len(running))} + } + + // Delete the Jobs explicitly rather than leaving them to the garbage collector, so their + // pods are gone by the time the CR is. + r.deleteAllJobs(ctx, plan) + + controllerutil.RemoveFinalizer(plan, recoveryPlanFinalizer) + + if err := r.Update(ctx, plan); err != nil { + return true, fmt.Errorf("removing finalizer: %w", err) + } + + return true, nil + } + + if !controllerutil.ContainsFinalizer(plan, recoveryPlanFinalizer) { + controllerutil.AddFinalizer(plan, recoveryPlanFinalizer) + + if err := r.Update(ctx, plan); err != nil { + return true, fmt.Errorf("adding finalizer: %w", err) + } + + // The Update triggers a new reconcile; nothing further to do in this cycle. + return true, nil + } + + return false, nil } // syncRecoveryEventsFromSlices scans all ResourceSlices for GPU devices that match this // plan's spec.deviceId and carry recovery-related device taints, then reconciles -// status.events against what it found: events whose taint has cleared are removed, and newly -// tainted devices get an event (or have their existing one escalated). -// -// The taint keys it recognises are deviceTaintKeyReset, deviceTaintKeyReflash and -// deviceTaintKeyXpumdReflash; see taintToDeviceNeed, which resolves the first against -// plan.Spec.DefaultResetType. +// status.events against what it found: failed events whose taint persists are re-queued for +// another attempt, events whose taint has cleared are removed, and newly tainted devices get an +// event (or have their existing one escalated). func (r *GPURecoveryPlanReconciler) syncRecoveryEventsFromSlices(ctx context.Context, plan *intelv1a1.GPURecoveryPlan) error { sliceList := &resv1.ResourceSliceList{} @@ -180,12 +301,15 @@ func (r *GPURecoveryPlanReconciler) syncRecoveryEventsFromSlices(ctx context.Con } } + // Send failed events round again while their taint persists and their retry budget lasts. + requeueFailedEvents(plan, activeKeys) + // Remove events whose taint has cleared. This runs before the add loop so that resolved // events free up room under maxStatusEvents in the same pass. - r.removeResolvedEvents(plan, activeKeys) + r.removeResolvedEvents(ctx, plan, activeKeys) // Add new events for newly tainted devices. - r.addNewEvents(plan, activeKeys) + addNewEvents(plan, activeKeys) // status.state is deliberately NOT set here: Reconcile derives it once, after every phase // that can move an event has run. @@ -193,254 +317,453 @@ func (r *GPURecoveryPlanReconciler) syncRecoveryEventsFromSlices(ctx context.Con return nil } -// findEventForDevice returns the index into status.events of the existing event for the -// given node+BDF, or -1 if there is none. -// -// This enforces a single-event-per-device invariant: at most one recovery may run against a GPU at a -// time, so a new event is only created once the previous one has been removed (the taint cleared) and -// the first match is the only match. A device whose taints escalate is handled by escalateEvent -// rather than by adding a second event. -func (r *GPURecoveryPlanReconciler) findEventForDevice(plan *intelv1a1.GPURecoveryPlan, nodeName, bdf string) int { +// processApprovals starts the recovery of every event an admin has authorised: one in +// waiting-approval that spec.approvals covers, or a permanently failed one an admin has named +// explicitly. +func (r *GPURecoveryPlanReconciler) processApprovals(ctx context.Context, plan *intelv1a1.GPURecoveryPlan) { + // consumedIDs collects the one-shot approvals used this cycle. Marking them is deferred until + // after the loop so that a single selector approval matches every event currently waiting — + // three GPUs all needing an sbr, say — rather than being spent on the first one reached. + consumedIDs := make(map[string]bool) + for i := range plan.Status.Events { - if plan.Status.Events[i].NodeName == nodeName && plan.Status.Events[i].GPUBDF == bdf { - return i + evt := &plan.Status.Events[i] + + // Re-approval path: an event that has spent its retry budget can be restarted by an admin + // adding an approval that names it. + if evt.State == intelv1a1.RecoveryEventStateFailed { + approval, ok := r.findExplicitApprovalForEvent(plan, evt) + if !ok { + continue + } + + now := setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, + "manually re-approved via approval %s after exhausting its retries; retry budget reset", + approval.ID) + evt.RetryCount = 0 + evt.ApprovalID = approval.ID + evt.ApprovalMatchedAt = &now + + appendMessage(plan, fmt.Sprintf("Event %s manually re-approved via approval %s; retry budget reset", + evt.ID, approval.ID)) + klog.Infof("GPURecoveryPlan %s: event %s re-approved via %s, retry budget reset", + plan.Name, evt.ID, approval.ID) + + // State is waiting-approval now; fall through so the Job is created in this same cycle. + } + + if evt.State != intelv1a1.RecoveryEventStateWaitingApproval { + continue + } + + approval, ok := r.findMatchingApproval(ctx, plan, evt) + if !ok { + klog.V(2).Infof("GPURecoveryPlan %s: no matching approval for event %s", plan.Name, evt.ID) + + continue + } + + // Record which approval authorised this event, and when. + if evt.ApprovalID != approval.ID { + now := metav1.NewTime(time.Now()) + evt.ApprovalID = approval.ID + evt.ApprovalMatchedAt = &now + evt.LastUpdated = &now + + appendMessage(plan, fmt.Sprintf("Event %s matched approval %s", evt.ID, approval.ID)) + } + + // Apply any override before creating the Job. + r.applyOverride(plan, evt, approval) + + if err := r.createRecoveryJob(ctx, plan, evt); err != nil { + klog.Errorf("GPURecoveryPlan %s: failed to create job for event %s: %v", plan.Name, evt.ID, err) + appendMessage(plan, fmt.Sprintf("Event %s: failed to create recovery job: %v", evt.ID, err)) + + // State unchanged — the event keeps its approval and is retried on the next pass; only + // the reason it has not started yet is recorded. + setEventState(evt, evt.State, "the recovery Job could not be created: %v", err) + + continue + } + + // Only consume a one-shot approval once the event has actually left waiting-approval. + if !approval.Persistent && evt.State == intelv1a1.RecoveryEventStateInProgress { + consumedIDs[approval.ID] = true } } - return -1 + for id := range consumedIDs { + setApprovalConsumed(plan, id) + } } -// addNewEvents creates a RecoveryEvent for every tainted device that does not have one -// yet, up to maxStatusEvents entries in status.events. -func (r *GPURecoveryPlanReconciler) addNewEvents(plan *intelv1a1.GPURecoveryPlan, active map[deviceKey]deviceNeed) { - skipped := 0 +// findMatchingApproval returns the first spec.approvals entry that authorises the given event. +// An approval matches when: +// - it names the event through eventId (a single approval), or +// - its selector matches the event's recovery type, node name and node labels (a group +// approval). Every field set on the selector must match; unset fields mean "any". +func (r *GPURecoveryPlanReconciler) findMatchingApproval(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, + evt *intelv1a1.RecoveryEvent) (intelv1a1.RecoveryApproval, bool) { + evtType := evt.RecoveryType.Type + if evtType == "" { + klog.Warningf("GPURecoveryPlan %s: event %s has no recovery type; skipping approval matching", + plan.Name, evt.ID) + + return intelv1a1.RecoveryApproval{}, false + } - for dk, need := range active { - // A device that already has an event does not get a second one, but its taints may - // since have escalated to a more severe recovery type. - if i := r.findEventForDevice(plan, dk.node, dk.bdf); i >= 0 { - r.escalateEvent(plan, &plan.Status.Events[i], need) + nodeCache := newNodeLabelCache(ctx, r) + for _, a := range plan.Spec.Approvals { + if a.Consumed { continue } - if len(plan.Status.Events) >= maxStatusEvents { - skipped++ + // A single approval, naming one event. + if a.EventID == evt.ID { + return a, true + } - continue + // A group approval, describing a set of events. + if a.Selector != nil { + if a.Selector.RecoveryType != "" && a.Selector.RecoveryType != evtType { + continue + } + + if a.Selector.NodeName != "" && a.Selector.NodeName != evt.NodeName { + continue + } + + if !nodeSelectorMatches(a.Selector.NodeSelector, evt.NodeName, nodeCache) { + continue + } + + return a, true } + } + + return intelv1a1.RecoveryApproval{}, false +} - r.addRecoveryEvent(plan, dk.node, dk.bdf, need) +// findExplicitApprovalForEvent returns an unconsumed approval that names the event through eventId. +func (r *GPURecoveryPlanReconciler) findExplicitApprovalForEvent(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) (intelv1a1.RecoveryApproval, bool) { + for _, a := range plan.Spec.Approvals { + if a.EventID == evt.ID && !a.Consumed { + return a, true + } } - if skipped > 0 { - msg := fmt.Sprintf("status.events is at its %d-entry limit: %d newly detected device(s) not recorded", - maxStatusEvents, skipped) + return intelv1a1.RecoveryApproval{}, false +} - r.appendMessage(plan, msg) - klog.Warningf("GPURecoveryPlan %s: %s", plan.Name, msg) +// applyOverride re-aims evt's reset type at approval.Override.RecoveryType, if set. The DRA driver +// cannot tell which reset mechanism a platform needs (see taintToDeviceNeed), so this is where an +// admin's choice of a different reset is honoured. +func (r *GPURecoveryPlanReconciler) applyOverride(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent, approval intelv1a1.RecoveryApproval) { + if approval.Override == nil { + return + } + + if evt.RecoveryType.IsReflash() { + klog.Warningf("GPURecoveryPlan %s: approval %s specifies an override but event %s is not a reset-type event; ignoring", + plan.Name, approval.ID, evt.ID) + + return + } + + newType := approval.Override.RecoveryType + if newType == intelv1a1.RecoveryTypeReflash { + klog.Warningf("GPURecoveryPlan %s: approval %s cannot override reset event %s to reflash; ignoring", + plan.Name, approval.ID, evt.ID) + + return + } + + if evt.RecoveryType.Type == newType { + return } + + if evt.RecoveryType.SuggestedType == "" { + evt.RecoveryType.SuggestedType = evt.RecoveryType.Type + } + + klog.Infof("GPURecoveryPlan %s: event %s reset type overridden %s -> %s via approval %s", + plan.Name, evt.ID, evt.RecoveryType.Type, newType, approval.ID) + appendMessage(plan, fmt.Sprintf("Event %s: reset type overridden from %s to %s via approval %s", + evt.ID, evt.RecoveryType.SuggestedType, newType, approval.ID)) + + evt.RecoveryType.Type = newType } -// addRecoveryEvent appends a new RecoveryEvent in waiting-approval state to the plan status. -func (r *GPURecoveryPlanReconciler) addRecoveryEvent(plan *intelv1a1.GPURecoveryPlan, nodeName, bdf string, need deviceNeed) { - id := generateEventID(nodeName, bdf, need.rt) +// prepareRecoveryJob applies the naming, labelling, ownership, node-pinning and pull settings +// every recovery Job needs, and returns the Job name. +func (r *GPURecoveryPlanReconciler) prepareRecoveryJob(job *batch.Job, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) string { + // The attempt index (how many Jobs this event has already run) goes in the name, so each retry + // gets a name of its own and every attempt stays readable until the event is removed. + jobName := recoveryJobName(evt.ID, len(evt.PastJobs)) + job.Name = jobName + job.Namespace = r.Opts.Namespace + + if job.Labels == nil { + job.Labels = make(map[string]string) + } + + job.Labels[recoveryJobLabelPlan] = plan.Name + job.Labels[recoveryJobLabelEvent] = evt.ID - evt := intelv1a1.RecoveryEvent{ - ID: id, - NodeName: nodeName, - GPUBDF: bdf, - Reason: need.reason, - RecoveryType: intelv1a1.RecoveryTypeSpec{Type: need.rt}, - RetryCount: 0, + // Own the Job so that (a) its status changes wake this controller through the + // Owns(&batch.Job{}) watch instead of waiting out a full RequeueDelay, and (b) any Job that + // deleteAllJobs misses is garbage-collected with the plan rather than leaking. + if err := ctrl.SetControllerReference(plan, job, r.Scheme); err != nil { + warning := fmt.Sprintf("Event %s: failed to set controller reference on Job %s: %v", evt.ID, jobName, err) + appendMessage(plan, warning) + klog.Warning(warning) } - // No message: reason, nodeName, gpuBDF and recoveryType already say everything about a new - // event, and restating them would only train an admin to ignore the field. - setEventState(&evt, intelv1a1.RecoveryEventStateWaitingApproval, "") + // Pin the pod to the node hosting the affected GPU. + job.Spec.Template.Spec.NodeName = evt.NodeName - plan.Status.Events = append(plan.Status.Events, evt) + // Tolerate every taint. + job.Spec.Template.Spec.Tolerations = append( + []core.Toleration{{Operator: core.TolerationOpExists}}, + plan.Spec.Tolerations..., + ) - r.appendMessage(plan, fmt.Sprintf("New recovery event %s for %s on %s (reason: %s, type: %s)", - id, bdf, nodeName, need.reason, need.rt)) + if plan.Spec.XpuSmi.PullPolicy != "" { + job.Spec.Template.Spec.Containers[0].ImagePullPolicy = core.PullPolicy(plan.Spec.XpuSmi.PullPolicy) + } - klog.Infof("GPURecoveryPlan %s: added recovery event %s for device %s on node %s (reason: %s)", - plan.Name, id, bdf, nodeName, need.reason) + if r.Opts.SecretName != "" { + job.Spec.Template.Spec.ImagePullSecrets = []core.LocalObjectReference{{Name: r.Opts.SecretName}} + } + + return jobName } -// escalateEvent up-levels an existing event in place when the device's taints now call for a more -// severe recovery than the event was created for. In practice that is the wedged -> survivability -// transition (reset -> reflash): the DRA driver applies the survivability taint alongside the wedged -// one, so a GPU that was merely stuck can turn out to need a firmware reflash while its reset event -// is still pending. -// -// Escalation is in-place rather than a second event because only one recovery may run against a GPU -// at a time; two approvable events for one device would let a reset and a reflash Job race on the -// same hardware. -// -// Escalation is one-way: the reverse (survivability clears, wedged remains) does not downgrade, -// mirroring higherPriorityNeed's "act on the worst condition" rule. Being monotonic also bounds it — -// reflash is the top of the ordering, so a device escalates at most once per event lifetime and -// flapping taints cannot drive a loop. -func (r *GPURecoveryPlanReconciler) escalateEvent(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent, need deviceNeed) { - if recoveryTypePriority(need.rt) <= recoveryTypePriority(evt.RecoveryType.Type) { - return +// createRecoveryJob creates the Job that carries out the event's recovery and moves the event to +// 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 } - oldID := evt.ID - oldType := evt.RecoveryType.Type - - // The ID embeds the recovery type, so it has to be regenerated. That also invalidates - // any spec.approvals entry naming the old ID, which is the point: an admin who approved - // a slot reset has not approved a firmware reflash, and re-using the ID would silently - // promote the narrower approval to the more destructive operation. A selector approval - // for the new type still matches, since that is an explicit standing decision. - evt.ID = generateEventID(evt.NodeName, evt.GPUBDF, need.rt) - evt.RecoveryType = intelv1a1.RecoveryTypeSpec{Type: need.rt} - - // The cause changed too — the device is in survivability mode now, not merely wedged. - evt.Reason = need.reason - - // Back to square one: unapproved, with a fresh retry budget, because the escalated operation is - // not the one the previous attempts were spending that budget on. - evt.RetryCount = 0 - evt.ApprovalID = "" - evt.ApprovalMatchedAt = nil - - // Back in waiting-approval with a different ID than the admin last saw, which needs saying: an - // approval that was granted has stopped applying, and nothing else on the event explains why. - setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, - "escalated from %s to %s (%s); the approval for the previous type no longer applies", - oldType, need.rt, need.reason) - - klog.Infof("GPURecoveryPlan %s: escalated event %s -> %s for %s/%s (%s -> %s); awaiting approval", - plan.Name, oldID, evt.ID, evt.NodeName, evt.GPUBDF, oldType, need.rt) - r.appendMessage(plan, fmt.Sprintf("Event %s escalated to %s on %s/%s (was %s, now %s); previous approval no longer applies", - oldID, evt.ID, evt.NodeName, evt.GPUBDF, oldType, need.rt)) + return r.createResetJob(ctx, plan, evt) } -// removeResolvedEvents removes events whose device taint has cleared: nothing has been done to -// the GPU yet, so a cleared taint means whatever healed it (a node reboot, an admin) has made -// the recovery unnecessary, and keeping the event would leave the plan asking for approval to -// reset a healthy card. -func (r *GPURecoveryPlanReconciler) removeResolvedEvents( - plan *intelv1a1.GPURecoveryPlan, - active map[deviceKey]deviceNeed, -) { - kept := plan.Status.Events[:0] +// createResetJob creates the PCIe-reset Job for a reset event and moves the event to in-progress. +func (r *GPURecoveryPlanReconciler) createResetJob(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent) error { + rt := evt.RecoveryType.Type - for _, evt := range plan.Status.Events { - key := deviceKey{node: evt.NodeName, bdf: evt.GPUBDF} - if _, stillActive := active[key]; stillActive { - kept = append(kept, evt) + args := recoveryTypeToArgs(evt.GPUBDF, rt) + if args == nil { + klog.Warningf("GPURecoveryPlan %s: unsupported recovery type %s for event %s; skipping", plan.Name, rt, evt.ID) - continue + return nil + } + + job := deployments.XpuManagerResetJob() + 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 } + } - klog.Infof("GPURecoveryPlan %s: removing resolved event %s (taint cleared on %s/%s, state: %s)", - plan.Name, evt.ID, evt.NodeName, evt.GPUBDF, evt.State) + if err := r.Create(ctx, job); err != nil { + if !k8serrors.IsAlreadyExists(err) { + return fmt.Errorf("creating recovery Job %s: %w", jobName, err) + } - r.appendMessage(plan, fmt.Sprintf("Event %s cleared: taint resolved on %s/%s", - evt.ID, evt.NodeName, evt.GPUBDF)) + // The Job is already there: an earlier pass created it and lost its status write. Adopting + // it is right — 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: Job %s already exists", plan.Name, jobName) } - plan.Status.Events = kept + evt.JobName = jobName + + setEventState(evt, intelv1a1.RecoveryEventStateInProgress, "") + + appendMessage(plan, fmt.Sprintf("Event %s: recovery Job %s created (type: %s, node: %s, bdf: %s)", + evt.ID, jobName, rt, evt.NodeName, evt.GPUBDF)) + + klog.Infof("GPURecoveryPlan %s: created recovery Job %s for event %s (type: %s, node: %s, bdf: %s)", + plan.Name, jobName, evt.ID, rt, evt.NodeName, evt.GPUBDF) + + return nil } -// setEventState records the state an event is moving to together with the sentence that explains it, -// and stamps LastUpdated. Every state write goes through it, so status.events[].stateMessage always -// describes the state next to it. -// -// Returns the timestamp the event now carries, so a caller that has another clock to set uses the -// same instant rather than reading the wall clock twice. -// -// nolint:unparam // detection only ever parks an event in waiting-approval; the states the -// recovery phases move it through are the reason this takes the state as a parameter. -func setEventState(evt *intelv1a1.RecoveryEvent, state intelv1a1.RecoveryEventState, format string, args ...any) metav1.Time { - msg := "" - if format != "" { - msg = capString(fmt.Sprintf(format, args...), maxStateMessageLen) - } +// 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 + for i := range plan.Status.Events { + evt := &plan.Status.Events[i] + if evt.State != intelv1a1.RecoveryEventStateInProgress || evt.JobName == "" { + continue + } + + job := &batch.Job{} + + if err := r.Get(ctx, types.NamespacedName{Name: evt.JobName, Namespace: r.Opts.Namespace}, job); err != nil { + klog.Warningf("GPURecoveryPlan %s: failed to get Job %s for event %s: %v", + plan.Name, evt.JobName, evt.ID, err) + + continue + } - if evt.State == state && evt.StateMessage == msg && evt.LastUpdated != nil { - return *evt.LastUpdated + for _, cond := range job.Status.Conditions { + if cond.Status != core.ConditionTrue { + continue + } + + switch cond.Type { + case batch.JobComplete: + klog.Infof("GPURecoveryPlan %s: Job %s succeeded for event %s", plan.Name, evt.JobName, evt.ID) + appendMessage(plan, fmt.Sprintf("Event %s: recovery Job %s succeeded — pods retained until taint clears", + evt.ID, evt.JobName)) + + evt.PastJobs = append(evt.PastJobs, evt.JobName) + evt.JobName = "" + + // No message: the state is the whole story, and the Job that produced it is the + // last entry in pastJobs. + setEventState(evt, intelv1a1.RecoveryEventStateSucceeded, "") + + case batch.JobFailed: + klog.Warningf("GPURecoveryPlan %s: Job %s failed for event %s", plan.Name, evt.JobName, evt.ID) + appendMessage(plan, fmt.Sprintf("Event %s: recovery Job %s failed (retries: %d) — pods retained until taint clears", + evt.ID, evt.JobName, evt.RetryCount)) + + failedJob := evt.JobName + + evt.PastJobs = append(evt.PastJobs, evt.JobName) + evt.JobName = "" + evt.RetryCount++ + + // Record which attempt this was, since that says whether the operator will try + // again, plus the Job's own verdict: BackoffLimitExceeded and DeadlineExceeded are + // different problems, and the pod is gone once the event is removed. + setEventState(evt, intelv1a1.RecoveryEventStateFailed, + "recovery Job %s failed on attempt %d of %d: %s", + failedJob, evt.RetryCount, plan.Spec.MaxRetries, jobFailureDetail(cond)) + } + } } - now := metav1.NewTime(time.Now()) - evt.State = state - evt.StateMessage = msg - evt.LastUpdated = &now + return nil +} - return now +// deleteEventJobs deletes the event's current Job, if any, and every Job it has already run. +func (r *GPURecoveryPlanReconciler) deleteEventJobs(ctx context.Context, planName string, evt intelv1a1.RecoveryEvent) { + if evt.JobName != "" { + r.deleteJobByName(ctx, planName, evt.JobName) + } + + for _, name := range evt.PastJobs { + r.deleteJobByName(ctx, planName, name) + } } -// capString truncates a single string for storage in status, marking it when anything was cut so a -// reader can tell a complete message from a clipped one. Counted in bytes rather than runes, since -// the limit exists to bound the size of the stored object. -func capString(s string, limit int) string { - const marker = "..." +// deleteAllJobs deletes every Job belonging to the plan. +func (r *GPURecoveryPlanReconciler) deleteAllJobs(ctx context.Context, plan *intelv1a1.GPURecoveryPlan) { + jobList := &batch.JobList{} - if len(s) <= limit { - return s + if err := r.List(ctx, jobList, + client.InNamespace(r.Opts.Namespace), + client.MatchingLabels{recoveryJobLabelPlan: plan.Name}, + ); err != nil { + klog.Warningf("GPURecoveryPlan %s: failed to list recovery Jobs for deletion: %v", plan.Name, err) } - // Back off to a rune boundary so the result stays valid UTF-8; the API server would otherwise - // reject the status write outright. - cut := limit - len(marker) - for cut > 0 && !utf8.RuneStart(s[cut]) { - cut-- + for i := range jobList.Items { + r.deleteJobByName(ctx, plan.Name, jobList.Items[i].Name) } - return s[:cut] + marker + for _, evt := range plan.Status.Events { + r.deleteEventJobs(ctx, plan.Name, evt) + } } -// updatePlanState derives the overall plan state from the current events and sets status.state, -// the value shown in the "State" print column of `kubectl get gpurecoveryplan`. -func (r *GPURecoveryPlanReconciler) updatePlanState(plan *intelv1a1.GPURecoveryPlan) { - anyActive := false - anyStuck := false +// deleteJobByName deletes a single Job by name, cascading to its pods through Background +// propagation. +func (r *GPURecoveryPlanReconciler) deleteJobByName(ctx context.Context, planName, jobName string) { + job := &batch.Job{} - for _, evt := range plan.Status.Events { - switch evt.State { - case intelv1a1.RecoveryEventStateWaitingApproval, - intelv1a1.RecoveryEventStateBlocked, - intelv1a1.RecoveryEventStateDraining, - intelv1a1.RecoveryEventStateInProgress: - // blocked is active, not stuck: it clears on its own once the node frees up. - anyActive = true - - case intelv1a1.RecoveryEventStateMissingFirmware: - // Blocked on operator configuration, not on hardware or an admin decision: - // the reflash cannot even be attempted until spec.firmware is filled in. - anyStuck = true - - case intelv1a1.RecoveryEventStateFailed: - // A failure within the retry budget is re-queued for another approval, so only an - // event that has spent its budget needs an admin. - if evt.RetryCount >= plan.Spec.MaxRetries { - anyStuck = true - } + if err := r.Get(ctx, types.NamespacedName{Name: jobName, Namespace: r.Opts.Namespace}, job); err != nil { + if !k8serrors.IsNotFound(err) { + klog.Warningf("GPURecoveryPlan %s: failed to get Job %s for deletion: %v", planName, jobName, err) } + + return } - switch { - case anyStuck: - plan.Status.State = intelv1a1.PlanStateError - case anyActive: - plan.Status.State = intelv1a1.PlanStateActive - default: - plan.Status.State = intelv1a1.PlanStateIdle + bg := metav1.DeletePropagationBackground + + if err := r.Delete(ctx, job, &client.DeleteOptions{PropagationPolicy: &bg}); err != nil { + if !k8serrors.IsNotFound(err) { + klog.Warningf("GPURecoveryPlan %s: failed to delete Job %s: %v", planName, jobName, err) + } + + return } + + klog.Infof("GPURecoveryPlan %s: deleted Job %s", planName, jobName) } -// appendMessage appends a message to status.messages, evicting the oldest entry if the -// cap (maxStatusMessages) has been reached. -func (r *GPURecoveryPlanReconciler) appendMessage(plan *intelv1a1.GPURecoveryPlan, msg string) { - plan.Status.Messages = append(plan.Status.Messages, msg) +// removeResolvedEvents removes events whose device taint has cleared, and deletes the Jobs they +// ran. A cleared taint means the GPU no longer needs recovering: either the recovery worked, or +// something else (a node reboot, an admin) healed it, and keeping the event would leave the plan +// asking for approval to reset a healthy card. +func (r *GPURecoveryPlanReconciler) removeResolvedEvents(ctx context.Context, plan *intelv1a1.GPURecoveryPlan, active map[deviceKey]deviceNeed) { + kept := plan.Status.Events[:0] + + for _, evt := range plan.Status.Events { + key := deviceKey{node: evt.NodeName, bdf: evt.GPUBDF} + if _, stillActive := active[key]; stillActive { + kept = append(kept, evt) + + continue + } + + if evt.State == intelv1a1.RecoveryEventStateInProgress { + klog.V(2).Infof("GPURecoveryPlan %s: taint cleared on %s/%s but event %s still has Job %s in flight; keeping it", + plan.Name, evt.NodeName, evt.GPUBDF, evt.ID, evt.JobName) - for len(plan.Status.Messages) > maxStatusMessages { - plan.Status.Messages = plan.Status.Messages[1:] + kept = append(kept, evt) + + continue + } + + klog.Infof("GPURecoveryPlan %s: removing resolved event %s (taint cleared on %s/%s, state: %s)", + plan.Name, evt.ID, evt.NodeName, evt.GPUBDF, evt.State) + + appendMessage(plan, fmt.Sprintf("Event %s cleared: taint resolved on %s/%s", + evt.ID, evt.NodeName, evt.GPUBDF)) + + // The Jobs were kept alive for as long as the event was, so their pods could be read for + // diagnostics. This is where that ends. + r.deleteEventJobs(ctx, plan.Name, evt) } + + plan.Status.Events = kept } // resourceSliceToPlans maps a ResourceSlice event to reconcile requests for all @@ -497,6 +820,11 @@ func (r *GPURecoveryPlanReconciler) SetupWithManager(mgr ctrl.Manager, opts Cont &resv1.ResourceSlice{}, handler.EnqueueRequestsFromMapFunc(r.resourceSliceToPlans), ). + // Recovery Jobs are owned by the plan, so a Job reaching Complete or Failed wakes this + // controller immediately instead of waiting out the RequeueAfter poll. The owner reference + // is cross-scope (cluster-scoped plan, namespaced Job), which is why the request the + // handler produces carries only the plan's name. + Owns(&batch.Job{}). Named("gpurecoveryplan"). Complete(r) } diff --git a/internal/controller/gpurecoveryplan_controller_test.go b/internal/controller/gpurecoveryplan_controller_test.go index 70c7f36..cfc4066 100644 --- a/internal/controller/gpurecoveryplan_controller_test.go +++ b/internal/controller/gpurecoveryplan_controller_test.go @@ -25,28 +25,83 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + batch "k8s.io/api/batch/v1" + core "k8s.io/api/core/v1" resv1 "k8s.io/api/resource/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/client-go/util/workqueue" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/reconcile" intelv1a1 "github.com/intel/gpu-base-operator/api/v1alpha1" ) -// failingStatusClient makes the status write fail on demand so the error path in persistPlan can -// be driven. +// makeTestJob builds a minimal batch.Job with the given condition pre-set, suitable for creating +// in the envtest API server to drive syncJobStatuses and the deletion paths. +func makeTestJob(name, ns string, labels map[string]string, condType batch.JobConditionType) *batch.Job { + return &batch.Job{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: labels, + }, + Spec: batch.JobSpec{ + Template: core.PodTemplateSpec{ + Spec: core.PodSpec{ + RestartPolicy: core.RestartPolicyNever, + Containers: []core.Container{ + {Name: "c", Image: "busybox"}, + }, + }, + }, + }, + Status: batch.JobStatus{ + Conditions: []batch.JobCondition{ + {Type: condType, Status: core.ConditionTrue}, + }, + }, + } +} + +// trackingQueue is a minimal workqueue that only records what was Added, so a real +// controller-runtime EventHandler can be driven in a test and its output inspected. +type trackingQueue struct { + workqueue.TypedRateLimitingInterface[reconcile.Request] + + added []reconcile.Request +} + +func (q *trackingQueue) Add(item reconcile.Request) { + q.added = append(q.added, item) +} + +// failingStatusClient makes writes fail on demand so the error paths in persistPlan can be +// driven. Status().Update() always fails; Update() fails only when failUpdate is set, so a spec +// write can still be observed to land after a failed status write. type failingStatusClient struct { client.Client + + failUpdate bool } func (c *failingStatusClient) Status() client.SubResourceWriter { return &failingSubResourceWriter{SubResourceWriter: c.Client.Status()} } +func (c *failingStatusClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + if c.failUpdate { + return fmt.Errorf("synthetic spec update failure") + } + + return c.Client.Update(ctx, obj, opts...) +} + type failingSubResourceWriter struct { client.SubResourceWriter } @@ -57,6 +112,60 @@ func (w *failingSubResourceWriter) Update( return fmt.Errorf("synthetic status update failure") } +// recordingClient notes the order in which the status and spec sub-writes reach the API server. +// persistPlan must write status before spec: the spec write (consuming a one-shot approval) +// triggers an immediate new reconcile, and if that reconcile read a stale status it would still +// see the event as waiting-approval and could act twice. +type recordingClient struct { + client.Client + + writes *[]string +} + +func (c *recordingClient) Status() client.SubResourceWriter { + return &recordingSubResourceWriter{SubResourceWriter: c.Client.Status(), writes: c.writes} +} + +func (c *recordingClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error { + *c.writes = append(*c.writes, "spec") + + return c.Client.Update(ctx, obj, opts...) +} + +type recordingSubResourceWriter struct { + client.SubResourceWriter + + writes *[]string +} + +func (w *recordingSubResourceWriter) Update( + ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption, +) error { + *w.writes = append(*w.writes, "status") + + return w.SubResourceWriter.Update(ctx, obj, opts...) +} + +// createPlanForOwnerRef persists an in-memory plan fixture so it gains a UID, then copies the +// server-assigned metadata back onto the fixture. Recovery Jobs carry a controller reference to +// the plan and the API server rejects an ownerReference with an empty UID, so any spec that drives +// Job creation needs a plan that really exists — which mirrors production, where Reconcile only +// ever works on an object it just Got. Status is preserved: the fixtures set status.events +// directly and rely on it. +func createPlanForOwnerRef(p *intelv1a1.GPURecoveryPlan) { + status := p.Status + + toCreate := p.DeepCopy() + Expect(k8sClient.Create(context.Background(), toCreate)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(context.Background(), toCreate) + }) + + p.ObjectMeta = toCreate.ObjectMeta + p.Status = status +} + // newTestReconciler builds a GPURecoveryPlanReconciler wired to the shared test client. func newTestReconciler() *GPURecoveryPlanReconciler { return &GPURecoveryPlanReconciler{ @@ -135,6 +244,11 @@ var _ = Describe("GPURecoveryPlan Controller", func() { resource := &intelv1a1.GPURecoveryPlan{ ObjectMeta: metav1.ObjectMeta{ Name: planName, + // Created with the finalizer already on it, as a plan is from its first + // reconcile onwards. Without it the first reconcile does nothing but add the + // finalizer, and every spec below would need a throwaway pass before the one + // it is actually testing. + Finalizers: []string{recoveryPlanFinalizer}, }, Spec: intelv1a1.GPURecoveryPlanSpec{ DeviceID: "0x1234", @@ -149,6 +263,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { By("deleting the GPURecoveryPlan") resource := &intelv1a1.GPURecoveryPlan{} if err := k8sClient.Get(ctx, planKey, resource); err == nil { + // Drop the finalizer first: nothing runs the controller during cleanup, so the object + // would otherwise sit in Terminating forever and the next spec's Create would fail. + resource.Finalizers = nil + Expect(k8sClient.Update(ctx, resource)).To(Succeed()) Expect(k8sClient.Delete(ctx, resource)).To(Succeed()) } }) @@ -244,6 +362,310 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) + Context("Finalizer management", func() { + It("should add the finalizer on first reconcile", func() { + // The shared plan is created with the finalizer already on it, so this uses its own + // object to exercise the path that puts it there. + key := types.NamespacedName{Name: "plan-finalizer-add"} + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", + }, + } + Expect(k8sClient.Create(ctx, p)).To(Succeed()) + + DeferCleanup(func() { + stale := &intelv1a1.GPURecoveryPlan{} + if err := k8sClient.Get(ctx, key, stale); err == nil { + stale.Finalizers = nil + _ = k8sClient.Update(ctx, stale) + _ = k8sClient.Delete(ctx, stale) + } + }) + + _, err := reconcilePlan(ctx, key.Name) + Expect(err).NotTo(HaveOccurred()) + + updated := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, key, updated)).To(Succeed()) + Expect(updated.Finalizers).To(ContainElement(recoveryPlanFinalizer)) + }) + }) + + // The finalizer exists to stop a plan deletion from killing a pod that is mid-reset. These + // specs pin that behaviour: deletion must block while any Job is non-terminal, and must + // complete once they all are. + // + // envtest runs no Job controller, so a Job created without conditions stays non-terminal + // indefinitely — which is exactly the "in-flight" state being tested. + Context("Finalizer: deletion blocks on in-flight recovery Jobs", func() { + // Each spec uses its own plan name: a plan whose finalizer is cleared during cleanup is + // garbage-collected asynchronously, so reusing one name races the next Create. + var ( + delPlanName string + delPlanKey types.NamespacedName + ) + + // newTerminatingPlan creates a plan carrying the finalizer, deletes it so that a real + // deletionTimestamp is set by the API server, and returns it still present in etcd. + newTerminatingPlan := func(name string, events []intelv1a1.RecoveryEvent) *intelv1a1.GPURecoveryPlan { + delPlanName = name + delPlanKey = types.NamespacedName{Name: name} + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Finalizers: []string{recoveryPlanFinalizer}, + }, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", + }, + } + Expect(k8sClient.Create(ctx, p)).To(Succeed()) + + DeferCleanup(func() { + stale := &intelv1a1.GPURecoveryPlan{} + if err := k8sClient.Get(ctx, delPlanKey, stale); err == nil { + stale.Finalizers = nil + _ = k8sClient.Update(ctx, stale) + } + }) + + if len(events) > 0 { + p.Status.Events = events + Expect(k8sClient.Status().Update(ctx, p)).To(Succeed()) + } + + Expect(k8sClient.Delete(ctx, p)).To(Succeed()) + + // The finalizer holds it alive; re-read to pick up the deletionTimestamp. + Expect(k8sClient.Get(ctx, delPlanKey, p)).To(Succeed()) + Expect(p.DeletionTimestamp.IsZero()).To(BeFalse()) + + return p + } + + // createJob puts a Job for delPlanName in the API server. When terminal is true the status + // subresource is driven to Failed; otherwise it is left condition-less and so counts as + // still running. + createJob := func(name string, terminal bool) *batch.Job { + job := makeTestJob(name, "default", map[string]string{ + recoveryJobLabelPlan: delPlanName, + recoveryJobLabelEvent: "evt-del-001", + }, batch.JobFailed) + job.Status = batch.JobStatus{} // status is not settable on create + + Expect(k8sClient.Create(ctx, job)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + if terminal { + // K8s 1.36 requires startTime + FailureTarget=True before Failed=True. + startTime := metav1.Now() + job.Status = batch.JobStatus{ + StartTime: &startTime, + Conditions: []batch.JobCondition{ + {Type: batch.JobFailureTarget, Status: core.ConditionTrue}, + {Type: batch.JobFailed, Status: core.ConditionTrue}, + }, + } + Expect(k8sClient.Status().Update(ctx, job)).To(Succeed()) + } + + return job + } + + It("should keep the finalizer and requeue while a Job is still running", func() { + p := newTerminatingPlan("plan-del-running", []intelv1a1.RecoveryEvent{ + { + ID: "evt-del-001", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateInProgress, + JobName: "recovery-evt-del-001-0", + LastUpdated: ptr.To(metav1.Now()), + }, + }) + Expect(p).NotTo(BeNil()) + + createJob("recovery-evt-del-001-0", false) + + result, err := reconcilePlan(ctx, delPlanName) + + // requeue-not-an-error: the caller sees a nil error plus a RequeueAfter. + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(2 * time.Second)) + + By("keeping the finalizer so the object is not garbage-collected") + updated := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, delPlanKey, updated)).To(Succeed()) + Expect(updated.Finalizers).To(ContainElement(recoveryPlanFinalizer)) + + By("not deleting the in-flight Job") + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-evt-del-001-0", + Namespace: "default", + }, &batch.Job{})).To(Succeed(), "an in-flight reset Job must not be killed by plan deletion") + + By("recording why deletion is waiting") + Expect(updated.Status.Messages).To(ContainElement( + ContainSubstring("Deletion waiting for 1 active recovery Job(s)"))) + }) + + It("should remove the finalizer and delete Jobs once all are terminal", func() { + p := newTerminatingPlan("plan-del-terminal", []intelv1a1.RecoveryEvent{ + { + ID: "evt-del-001", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateFailed, + PastJobs: []string{"recovery-evt-del-002-0"}, + LastUpdated: ptr.To(metav1.Now()), + }, + }) + Expect(p).NotTo(BeNil()) + + createJob("recovery-evt-del-002-0", true) + + _, err := reconcilePlan(ctx, delPlanName) + Expect(err).NotTo(HaveOccurred()) + + By("letting the object go away") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, delPlanKey, &intelv1a1.GPURecoveryPlan{})) + }, 5*time.Second, 100*time.Millisecond).Should(BeTrue()) + + By("cleaning up the terminal Job") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-evt-del-002-0", + Namespace: "default", + }, &batch.Job{})) + }, 5*time.Second, 100*time.Millisecond).Should(BeTrue()) + }) + + It("should delete a terminal labelled Job that no event references", func() { + // Complements the blocking case: once status.events has been pruned, the label is the + // only handle left on the Job, so cleanup must not rely on the events walk or the Job + // would leak after the plan is gone. + p := newTerminatingPlan("plan-del-orphan-cleanup", nil) + Expect(p.Status.Events).To(BeEmpty()) + + createJob("recovery-orphan-terminal-0", true) + + _, err := reconcilePlan(ctx, delPlanName) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-orphan-terminal-0", + Namespace: "default", + }, &batch.Job{})) + }, 5*time.Second, 100*time.Millisecond).Should(BeTrue(), + "a Job with no event entry must still be cleaned up via its plan label") + }) + + It("should block on a labelled Job even when no event references it", func() { + // A Job whose event entry was already pruned from status must still be waited for — + // otherwise pruning status would silently unblock a live reset. + p := newTerminatingPlan("plan-del-orphan", nil) + Expect(p.Status.Events).To(BeEmpty()) + + createJob("recovery-orphan-0", false) + + result, err := reconcilePlan(ctx, delPlanName) + Expect(err).NotTo(HaveOccurred()) + Expect(result.RequeueAfter).To(Equal(2 * time.Second)) + + updated := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, delPlanKey, updated)).To(Succeed()) + Expect(updated.Finalizers).To(ContainElement(recoveryPlanFinalizer)) + }) + + It("should not wait for a Job that is already terminating", func() { + r := newTestReconciler() + delPlanName = "plan-del-terminating" + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: delPlanName}, + } + + // A Job finalizer keeps the object readable after Delete, so it is observable in the + // Terminating state that runningRecoveryJobs must skip. + job := makeTestJob("recovery-terminating-0", "default", map[string]string{ + recoveryJobLabelPlan: delPlanName, + }, batch.JobFailed) + job.Status = batch.JobStatus{} + job.Finalizers = []string{"test.intel.com/hold"} + + Expect(k8sClient.Create(ctx, job)).To(Succeed()) + + DeferCleanup(func() { + stale := &batch.Job{} + if err := k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-terminating-0", Namespace: "default", + }, stale); err == nil { + stale.Finalizers = nil + _ = k8sClient.Update(ctx, stale) + } + }) + + By("confirming it counts as running before deletion") + running, err := runningRecoveryJobs(r.Client, ctx, r.Opts.Namespace, p) + Expect(err).NotTo(HaveOccurred()) + Expect(running).To(ConsistOf("recovery-terminating-0")) + + Expect(k8sClient.Delete(ctx, job)).To(Succeed()) + + By("no longer counting it once a deletionTimestamp is set") + Eventually(func() []string { + running, err := runningRecoveryJobs(r.Client, ctx, r.Opts.Namespace, p) + Expect(err).NotTo(HaveOccurred()) + + return running + }, 5*time.Second, 100*time.Millisecond).Should(BeEmpty()) + }) + }) + + Context("Helper: jobIsTerminal", func() { + cond := func(t batch.JobConditionType, s core.ConditionStatus) batch.JobCondition { + return batch.JobCondition{Type: t, Status: s} + } + jobWith := func(conds ...batch.JobCondition) *batch.Job { + return &batch.Job{Status: batch.JobStatus{Conditions: conds}} + } + + It("should treat a Job with no conditions as running", func() { + Expect(jobIsTerminal(jobWith())).To(BeFalse()) + }) + + It("should treat Complete=True as terminal", func() { + Expect(jobIsTerminal(jobWith(cond(batch.JobComplete, core.ConditionTrue)))).To(BeTrue()) + }) + + It("should treat Failed=True as terminal", func() { + Expect(jobIsTerminal(jobWith(cond(batch.JobFailed, core.ConditionTrue)))).To(BeTrue()) + }) + + It("should not treat Complete=False as terminal", func() { + Expect(jobIsTerminal(jobWith(cond(batch.JobComplete, core.ConditionFalse)))).To(BeFalse()) + }) + + It("should not treat intermediate conditions as terminal", func() { + // SuccessCriteriaMet/FailureTarget precede the real terminal condition; acting on them + // would cut a Job's pod off before it has actually finished. + Expect(jobIsTerminal(jobWith( + cond(batch.JobSuspended, core.ConditionTrue), + cond(batch.JobSuccessCriteriaMet, core.ConditionTrue), + cond(batch.JobFailureTarget, core.ConditionTrue), + ))).To(BeFalse()) + }) + }) + Context("Reconcile with non-existent plan", func() { It("should return no error for a missing plan", func() { _, err := reconcilePlan(ctx, "does-not-exist") @@ -558,13 +980,18 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) }) - Context("persistPlan: status write-back", func() { + Context("persistPlan: status and spec write-back", func() { + // The plan carries one consumable approval, so the spec half of persistPlan has something + // real to write: consuming an approval is the only spec change the operator ever makes. newPersistPlan := func(name string) (*GPURecoveryPlanReconciler, *intelv1a1.GPURecoveryPlan) { p := &intelv1a1.GPURecoveryPlan{ ObjectMeta: metav1.ObjectMeta{Name: name}, Spec: intelv1a1.GPURecoveryPlanSpec{ DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-persist", EventID: "evt-persist-1"}, + }, }, } Expect(k8sClient.Create(ctx, p)).To(Succeed()) @@ -604,6 +1031,63 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(err.Error()).To(ContainSubstring("updating status")) }) + It("should write both status and spec changes back to the API server", func() { + r, p := newPersistPlan("plan-persist-both") + key := types.NamespacedName{Name: p.Name} + orig := p.DeepCopy() + + p.Status.Messages = []string{"hello"} + p.Spec.Approvals[0].Consumed = true + + Expect(r.persistPlan(ctx, key, orig, p)).To(Succeed()) + + got := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, key, got)).To(Succeed()) + Expect(got.Status.Messages).To(Equal([]string{"hello"})) + Expect(got.Spec.Approvals[0].Consumed).To(BeTrue()) + }) + + It("should still write the spec when the status write fails", func() { + // Returning early on a status-update error would silently drop consumed=true. That + // lets the approval match again and reset the same GPU a second time — status is + // recomputable on the next pass, consumed is not. + r, p := newPersistPlan("plan-persist-status-fails") + key := types.NamespacedName{Name: p.Name} + orig := p.DeepCopy() + + r.Client = &failingStatusClient{Client: r.Client} + + p.Status.Messages = []string{"dropped"} + p.Spec.Approvals[0].Consumed = true + + err := r.persistPlan(ctx, key, orig, p) + Expect(err).To(HaveOccurred(), "the status failure must be reported, not swallowed") + Expect(err.Error()).To(ContainSubstring("updating status")) + + got := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, key, got)).To(Succeed()) + Expect(got.Spec.Approvals[0].Consumed).To(BeTrue(), + "consumed=true must survive a failed status write") + }) + + It("should write status before spec", func() { + // The spec write consumes the approval and immediately triggers another reconcile; that + // reconcile must not observe a status still describing the event as waiting-approval, + // or it can act on it a second time. + r, p := newPersistPlan("plan-persist-order") + key := types.NamespacedName{Name: p.Name} + orig := p.DeepCopy() + + writes := []string{} + r.Client = &recordingClient{Client: r.Client, writes: &writes} + + p.Status.Messages = []string{"hello"} + p.Spec.Approvals[0].Consumed = true + + Expect(r.persistPlan(ctx, key, orig, p)).To(Succeed()) + Expect(writes).To(Equal([]string{"status", "spec"})) + }) + It("should recover from a conflict by retrying against the current object", func() { // Simulates the routine case: something else updated the plan between our // Get and our write, so our cached resourceVersion is stale. @@ -629,13 +1113,13 @@ var _ = Describe("GPURecoveryPlan Controller", func() { "the concurrent change must be preserved, not clobbered") }) - It("should do nothing when the status did not change", func() { + It("should do nothing when neither status nor spec changed", func() { r, p := newPersistPlan("plan-persist-noop") key := types.NamespacedName{Name: p.Name} - r.Client = &failingStatusClient{Client: r.Client} + r.Client = &failingStatusClient{Client: r.Client, failUpdate: true} - // orig == p, so there is nothing to write and the failing path is not hit. + // orig == p, so there is nothing to write and neither failing path is hit. Expect(r.persistPlan(ctx, key, p.DeepCopy(), p)).To(Succeed()) }) }) @@ -647,7 +1131,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // reconcile. Only Status().Update is made to fail, and the phases dirty status // (state "" -> idle), so the error can only originate in persistPlan. p := &intelv1a1.GPURecoveryPlan{ - ObjectMeta: metav1.ObjectMeta{Name: "plan-reconcile-writefail"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "plan-reconcile-writefail", + Finalizers: []string{recoveryPlanFinalizer}, + }, Spec: intelv1a1.GPURecoveryPlanSpec{ DefaultResetType: intelv1a1.RecoveryTypeSlot, DeviceID: "0xabcd", }, @@ -655,7 +1142,12 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Expect(k8sClient.Create(ctx, p)).To(Succeed()) DeferCleanup(func() { - _ = k8sClient.Delete(ctx, p) + stale := &intelv1a1.GPURecoveryPlan{} + if err := k8sClient.Get(ctx, types.NamespacedName{Name: p.Name}, stale); err == nil { + stale.Finalizers = nil + _ = k8sClient.Update(ctx, stale) + _ = k8sClient.Delete(ctx, stale) + } }) r := newTestReconciler() @@ -672,11 +1164,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Context("Helper: appendMessage", func() { It("should cap messages at maxStatusMessages", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} for i := 0; i < maxStatusMessages+10; i++ { - r.appendMessage(p, "msg") + appendMessage(p, "msg") } Expect(p.Status.Messages).To(HaveLen(maxStatusMessages)) @@ -748,12 +1239,6 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) Context("Helper: escalateEvent", func() { - var r *GPURecoveryPlanReconciler - - BeforeEach(func() { - r = newTestReconciler() - }) - // waitingEvent builds a single-device event of the given type, waiting for approval, with // approval bookkeeping and a spent retry budget already present so escalation can be seen // to clear them. @@ -778,7 +1263,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-esc"}} evt := waitingEvent(intelv1a1.RecoveryTypeSlot) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeReflash)) Expect(evt.ID).To(Equal("evt-node01-reflash-0000-02-00-0")) @@ -804,7 +1289,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // The approval names this event before escalation... Expect(evt.ID).To(Equal(p.Spec.Approvals[0].EventID)) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) // ...and must not after, or approving a reset would run a reflash. Expect(evt.ID).NotTo(Equal(p.Spec.Approvals[0].EventID), @@ -815,7 +1300,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-esc-reset"}} evt := waitingEvent(intelv1a1.RecoveryTypeSlot) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) Expect(evt.RetryCount).To(BeZero(), "the escalated operation gets its own retry budget") @@ -829,7 +1314,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-esc-msg"}} evt := waitingEvent(intelv1a1.RecoveryTypeSlot) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) Expect(evt.StateMessage).To(SatisfyAll( ContainSubstring(string(intelv1a1.RecoveryTypeSlot)), @@ -842,7 +1327,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { evt := waitingEvent(intelv1a1.RecoveryTypeReflash) before := evt.DeepCopy() - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}) Expect(evt).To(Equal(before), "escalation is one-way; act on the worst condition") }) @@ -852,7 +1337,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { evt := waitingEvent(intelv1a1.RecoveryTypeSlot) before := evt.DeepCopy() - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}) Expect(evt).To(Equal(before)) Expect(p.Status.Messages).To(BeEmpty(), "a steady-state reconcile must not log an escalation") @@ -864,11 +1349,11 @@ var _ = Describe("GPURecoveryPlan Controller", func() { p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-esc-idem"}} evt := waitingEvent(intelv1a1.RecoveryTypeSlot) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) afterFirst := evt.DeepCopy() msgCount := len(p.Status.Messages) - r.escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) Expect(evt).To(Equal(afterFirst)) Expect(p.Status.Messages).To(HaveLen(msgCount)) @@ -877,7 +1362,6 @@ var _ = Describe("GPURecoveryPlan Controller", func() { Context("Helper: findEventForDevice", func() { It("should find an event regardless of its recovery type", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{ Status: intelv1a1.GPURecoveryPlanStatus{ Events: []intelv1a1.RecoveryEvent{ @@ -889,10 +1373,10 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }, } - Expect(r.findEventForDevice(p, "node01", "0000:02:00.0")).To(Equal(0)) - Expect(r.findEventForDevice(p, "node02", "0000:03:00.0")).To(Equal(1)) - Expect(r.findEventForDevice(p, "node03", "0000:02:00.0")).To(Equal(-1)) - Expect(r.findEventForDevice(p, "node01", "0000:09:00.0")).To(Equal(-1)) + Expect(findEventForDevice(p, "node01", "0000:02:00.0")).To(Equal(0)) + Expect(findEventForDevice(p, "node02", "0000:03:00.0")).To(Equal(1)) + Expect(findEventForDevice(p, "node03", "0000:02:00.0")).To(Equal(-1)) + Expect(findEventForDevice(p, "node01", "0000:09:00.0")).To(Equal(-1)) }) }) @@ -909,10 +1393,9 @@ var _ = Describe("GPURecoveryPlan Controller", func() { } It("should add an event per newly tainted device", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} - r.addNewEvents(p, activeSet(3)) + addNewEvents(p, activeSet(3)) Expect(p.Status.Events).To(HaveLen(3)) }) @@ -920,10 +1403,9 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // status.events[].reason is the only place the triggering condition is recorded: the // taint lives on the ResourceSlice and is gone by the time anyone reads the event. It("should record the cause the recovery was derived from", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} - r.addNewEvents(p, map[deviceKey]deviceNeed{ + addNewEvents(p, map[deviceKey]deviceNeed{ {node: "node-w", bdf: "0000:02:00.0"}: { rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged, }, @@ -942,19 +1424,17 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) It("should cap events at maxStatusEvents", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} - r.addNewEvents(p, activeSet(maxStatusEvents+10)) + addNewEvents(p, activeSet(maxStatusEvents+10)) Expect(p.Status.Events).To(HaveLen(maxStatusEvents)) }) It("should report the refusal in status.messages rather than dropping silently", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} - r.addNewEvents(p, activeSet(maxStatusEvents+10)) + addNewEvents(p, activeSet(maxStatusEvents+10)) Expect(p.Status.Messages).NotTo(BeEmpty()) Expect(p.Status.Messages[len(p.Status.Messages)-1]).To(ContainSubstring("not recorded")) @@ -963,13 +1443,12 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // status.events is live state, not a log: an event carries the approval an admin gave it. // Evicting entries FIFO-style to make room would discard that. It("should keep the existing events when the cap is reached", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} - r.addNewEvents(p, activeSet(maxStatusEvents)) + addNewEvents(p, activeSet(maxStatusEvents)) oldest := p.Status.Events[0] - r.addNewEvents(p, map[deviceKey]deviceNeed{ + addNewEvents(p, map[deviceKey]deviceNeed{ {node: "brand-new-node", bdf: "0000:03:00.0"}: { rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged, }, @@ -980,12 +1459,11 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) It("should not add a second event for a device that already has one", func() { - r := newTestReconciler() p := &intelv1a1.GPURecoveryPlan{} active := activeSet(2) - r.addNewEvents(p, active) - r.addNewEvents(p, active) + addNewEvents(p, active) + addNewEvents(p, active) Expect(p.Status.Events).To(HaveLen(2)) }) @@ -1011,7 +1489,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { State: intelv1a1.RecoveryEventStateWaitingApproval, }) - r.removeResolvedEvents(p, map[deviceKey]deviceNeed{}) + r.removeResolvedEvents(ctx, p, map[deviceKey]deviceNeed{}) Expect(p.Status.Events).To(BeEmpty()) Expect(p.Status.Messages).To(ContainElement(ContainSubstring("evt-gone"))) @@ -1023,7 +1501,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { State: intelv1a1.RecoveryEventStateWaitingApproval, }) - r.removeResolvedEvents(p, map[deviceKey]deviceNeed{ + r.removeResolvedEvents(ctx, p, map[deviceKey]deviceNeed{ {node: "node01", bdf: "0000:02:00.0"}: {rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}, }) @@ -1039,7 +1517,7 @@ var _ = Describe("GPURecoveryPlan Controller", func() { State: intelv1a1.RecoveryEventStateWaitingApproval, }) - r.removeResolvedEvents(p, map[deviceKey]deviceNeed{ + r.removeResolvedEvents(ctx, p, map[deviceKey]deviceNeed{ {node: "node01", bdf: "0000:02:00.0"}: {rt: intelv1a1.RecoveryTypeSlot, reason: reasonWedged}, }) @@ -1284,12 +1762,6 @@ var _ = Describe("GPURecoveryPlan Controller", func() { }) Context("Helper: updatePlanState", func() { - var r *GPURecoveryPlanReconciler - - BeforeEach(func() { - r = newTestReconciler() - }) - // planWith builds a plan with maxRetries=3 and the given events, so the retry-budget // comparison in updatePlanState has a real threshold to test against. planWith := func(events ...intelv1a1.RecoveryEvent) *intelv1a1.GPURecoveryPlan { @@ -1306,14 +1778,14 @@ var _ = Describe("GPURecoveryPlan Controller", func() { It("should report idle with no events", func() { p := planWith() - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateIdle)) }) DescribeTable("should report active while an event is on its way somewhere", func(state intelv1a1.RecoveryEventState) { p := planWith(evt(state, 0)) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateActive)) }, Entry("waiting for an approval", intelv1a1.RecoveryEventStateWaitingApproval), @@ -1325,13 +1797,13 @@ var _ = Describe("GPURecoveryPlan Controller", func() { It("should report idle once every event has settled", func() { p := planWith(evt(intelv1a1.RecoveryEventStateSucceeded, 1)) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateIdle)) }) It("should report error for an event that exhausted its retry budget", func() { p := planWith(evt(intelv1a1.RecoveryEventStateFailed, 3)) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateError), "an exhausted event never retries on its own; it needs an explicit re-approval") }) @@ -1340,13 +1812,13 @@ var _ = Describe("GPURecoveryPlan Controller", func() { // a later pass, so surfacing "error" would be a false alarm. It("should not report error for a failure still inside its retry budget", func() { p := planWith(evt(intelv1a1.RecoveryEventStateFailed, 1)) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).NotTo(Equal(intelv1a1.PlanStateError)) }) It("should report error when a reflash is blocked on missing firmware", func() { p := planWith(evt(intelv1a1.RecoveryEventStateMissingFirmware, 0)) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateError), "nothing clears this but an operator filling in spec.firmware") }) @@ -1358,8 +1830,1384 @@ var _ = Describe("GPURecoveryPlan Controller", func() { evt(intelv1a1.RecoveryEventStateInProgress, 0), evt(intelv1a1.RecoveryEventStateFailed, 3), ) - r.updatePlanState(p) + updatePlanState(p) Expect(p.Status.State).To(Equal(intelv1a1.PlanStateError)) }) }) -}) + + Context("Helper: findMatchingApproval", func() { + var r *GPURecoveryPlanReconciler + + BeforeEach(func() { + r = newTestReconciler() + }) + + evt := &intelv1a1.RecoveryEvent{ + ID: "evt-aabb", + NodeName: "node05", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + } + + It("should match a singular eventId approval", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-1234", EventID: "evt-aabb"}, + }, + }, + } + approval, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeTrue()) + Expect(approval.ID).To(Equal("app-1234")) + }) + + It("should match a selector approval with matching recoveryType", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "app-5678", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + }, + }, + }, + } + approval, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeTrue()) + Expect(approval.ID).To(Equal("app-5678")) + }) + + It("should not match a selector with a different recoveryType", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSlot}, + }, + }, + }, + } + _, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeFalse()) + }) + + It("should not match a selector with a different nodeName", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {Selector: &intelv1a1.ApprovalSelector{NodeName: "node99"}}, + }, + }, + } + _, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeFalse()) + }) + + It("should not match a consumed eventId approval", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {EventID: "evt-aabb", Consumed: true}, + }, + }, + } + _, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeFalse()) + }) + + It("should not match a consumed selector approval", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Consumed: true, + }, + }, + }, + } + _, ok := r.findMatchingApproval(ctx, p, evt) + Expect(ok).To(BeFalse()) + }) + + // An event with no recovery type cannot be authorised: a selector with an empty + // recoveryType means "any reset", and matching it against an event whose own type is + // unknown would approve a reset nobody chose. + It("should refuse to match an event with no recovery type", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-any", Selector: &intelv1a1.ApprovalSelector{}}, + }, + }, + } + + _, ok := r.findMatchingApproval(ctx, p, &intelv1a1.RecoveryEvent{ + ID: "evt-typeless", NodeName: "node05", GPUBDF: "0000:02:00.0", + }) + Expect(ok).To(BeFalse()) + }) + + Context("nodeSelector matching", func() { + // These specs create real Nodes in envtest so selector.nodeSelector is matched against + // actual Node labels. + const ( + labelledNode = "node-rack04" + plainNode = "node-plain" + ) + + nodeSelectorApproval := func(sel map[string]string) *intelv1a1.GPURecoveryPlan { + return &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-sel", Selector: &intelv1a1.ApprovalSelector{NodeSelector: sel}}, + }, + }, + } + } + + eventForNode := func(nodeName string) *intelv1a1.RecoveryEvent { + return &intelv1a1.RecoveryEvent{ + ID: "evt-" + nodeName, + NodeName: nodeName, + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + } + } + + BeforeEach(func() { + for name, nodeLabels := range map[string]map[string]string{ + labelledNode: {"rack": "rack-04-32", "gpu": "true"}, + plainNode: nil, + } { + node := &core.Node{ObjectMeta: metav1.ObjectMeta{Name: name, Labels: nodeLabels}} + if err := k8sClient.Create(ctx, node); err != nil { + Expect(errors.IsAlreadyExists(err)).To(BeTrue()) + } + } + }) + + AfterEach(func() { + for _, name := range []string{labelledNode, plainNode} { + node := &core.Node{ObjectMeta: metav1.ObjectMeta{Name: name}} + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, node))).To(Succeed()) + } + }) + + It("should match when the node carries all selector labels", func() { + p := nodeSelectorApproval(map[string]string{"rack": "rack-04-32"}) + + approval, ok := r.findMatchingApproval(ctx, p, eventForNode(labelledNode)) + Expect(ok).To(BeTrue()) + Expect(approval.ID).To(Equal("app-sel")) + }) + + It("should match when every label in a multi-label selector is present", func() { + p := nodeSelectorApproval(map[string]string{"rack": "rack-04-32", "gpu": "true"}) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode(labelledNode)) + Expect(ok).To(BeTrue()) + }) + + It("should NOT match an event on a node without the selector labels", func() { + // Without node-label matching this returns true, silently widening a rack-scoped + // approval cluster-wide. + p := nodeSelectorApproval(map[string]string{"rack": "rack-04-32"}) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode(plainNode)) + Expect(ok).To(BeFalse()) + }) + + It("should NOT match when the label value differs", func() { + p := nodeSelectorApproval(map[string]string{"rack": "rack-99-01"}) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode(labelledNode)) + Expect(ok).To(BeFalse()) + }) + + It("should NOT match when only some of the selector labels are present", func() { + p := nodeSelectorApproval(map[string]string{"rack": "rack-04-32", "zone": "west"}) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode(labelledNode)) + Expect(ok).To(BeFalse()) + }) + + It("should fail closed when the node does not exist", func() { + // A node that cannot be read must never be treated as matching, otherwise a + // transient API error would widen the approval's scope. + p := nodeSelectorApproval(map[string]string{"rack": "rack-04-32"}) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode("node-does-not-exist")) + Expect(ok).To(BeFalse()) + }) + + It("should still honour an empty nodeSelector as match-anything", func() { + p := nodeSelectorApproval(nil) + + _, ok := r.findMatchingApproval(ctx, p, eventForNode(plainNode)) + Expect(ok).To(BeTrue()) + }) + + It("should require both nodeSelector and recoveryType to match", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "app-both", + Selector: &intelv1a1.ApprovalSelector{ + RecoveryType: intelv1a1.RecoveryTypeSlot, + NodeSelector: map[string]string{"rack": "rack-04-32"}, + }, + }, + }, + }, + } + + // Labels match but the recovery type (SBR) does not. + _, ok := r.findMatchingApproval(ctx, p, eventForNode(labelledNode)) + Expect(ok).To(BeFalse()) + }) + }) + }) + + Context("Helper: applyOverride", func() { + var r *GPURecoveryPlanReconciler + + BeforeEach(func() { + r = newTestReconciler() + }) + + It("should up-level a defaulted SBR event to the admin-requested type and record SuggestedType", func() { + p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-override"}} + evt := &intelv1a1.RecoveryEvent{ + ID: "evt-override-1", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + } + approval := intelv1a1.RecoveryApproval{ + ID: "app-override", + Override: &intelv1a1.RecoveryOverride{RecoveryType: intelv1a1.RecoveryTypeSlot}, + } + + r.applyOverride(p, evt, approval) + + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSlot)) + Expect(evt.RecoveryType.SuggestedType).To(Equal(intelv1a1.RecoveryTypeSBR)) + Expect(recoveryTypeToArgs("0000:02:00.0", evt.RecoveryType.Type)).To( + ContainElement("--coldreset"), "an override must change the command actually run") + }) + + It("should be a no-op when the approval has no override", func() { + p := &intelv1a1.GPURecoveryPlan{} + evt := &intelv1a1.RecoveryEvent{ + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + } + + r.applyOverride(p, evt, intelv1a1.RecoveryApproval{ID: "app-no-override"}) + + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSBR)) + Expect(evt.RecoveryType.SuggestedType).To(BeEmpty()) + }) + + It("should ignore an override on a reflash event", func() { + p := &intelv1a1.GPURecoveryPlan{} + evt := &intelv1a1.RecoveryEvent{ + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeReflash}, + } + approval := intelv1a1.RecoveryApproval{ + Override: &intelv1a1.RecoveryOverride{RecoveryType: intelv1a1.RecoveryTypeSBR}, + } + + r.applyOverride(p, evt, approval) + + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeReflash)) + Expect(evt.RecoveryType.SuggestedType).To(BeEmpty()) + }) + + It("should ignore an override that requests reflash for a reset event", func() { + p := &intelv1a1.GPURecoveryPlan{} + evt := &intelv1a1.RecoveryEvent{ + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + } + approval := intelv1a1.RecoveryApproval{ + Override: &intelv1a1.RecoveryOverride{RecoveryType: intelv1a1.RecoveryTypeReflash}, + } + + r.applyOverride(p, evt, approval) + + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSBR)) + Expect(evt.RecoveryType.SuggestedType).To(BeEmpty()) + }) + + // SuggestedType is the audit trail of what the operator itself picked, so a second + // override must not overwrite it with the first override's choice. + It("should record SuggestedType only once across repeated overrides", func() { + p := &intelv1a1.GPURecoveryPlan{} + evt := &intelv1a1.RecoveryEvent{ + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + } + + r.applyOverride(p, evt, intelv1a1.RecoveryApproval{ + Override: &intelv1a1.RecoveryOverride{RecoveryType: intelv1a1.RecoveryTypeSlot}, + }) + r.applyOverride(p, evt, intelv1a1.RecoveryApproval{ + Override: &intelv1a1.RecoveryOverride{RecoveryType: intelv1a1.RecoveryTypeAMC}, + }) + + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeAMC)) + Expect(evt.RecoveryType.SuggestedType).To(Equal(intelv1a1.RecoveryTypeSBR), + "suggestedType must keep the type the operator detected, not the previous override") + }) + }) + + Context("processApprovals: approval consumption", func() { + It("should set Consumed=true on the approval after a non-persistent selector approval fires", func() { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-consume-test"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-nonpersist", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Persistent: false, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-consume-1", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }, + }, + }, + } + + // Recovery Jobs are owned by the plan, and an owner reference needs a UID — so the plan + // must exist in the API server, as it always does in production (Reconcile only ever + // operates on an object it just Got). + createPlanForOwnerRef(p) + + r.processApprovals(ctx, p) + + Expect(p.Spec.Approvals[0].Consumed).To(BeTrue(), + "non-persistent selector approval must be marked consumed after firing") + + // A second call with a new event must not match the now-consumed approval. + p.Status.Events = append(p.Status.Events, intelv1a1.RecoveryEvent{ + ID: "evt-consume-2", + NodeName: "node01", + GPUBDF: "0000:03:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }) + + r.processApprovals(ctx, p) + + var secondEvt *intelv1a1.RecoveryEvent + + for i := range p.Status.Events { + if p.Status.Events[i].ID == "evt-consume-2" { + secondEvt = &p.Status.Events[i] + + break + } + } + + Expect(secondEvt).NotTo(BeNil()) + Expect(secondEvt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval), + "second event must remain waiting-approval because the approval was already consumed") + }) + + // Consumption is deferred to the end of the loop so one approval covers everything + // currently waiting. An admin approving "any sbr" for a node with three wedged GPUs means + // all three, not whichever event happens to be first in status.events. + It("should let one non-persistent approval fire for every event waiting in the same pass", func() { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-consume-batch"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-batch", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-batch-1", NodeName: "node01", GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }, + { + ID: "evt-batch-2", NodeName: "node01", GPUBDF: "0000:03:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }, + }, + }, + } + + createPlanForOwnerRef(p) + + DeferCleanup(func() { + for _, evt := range p.Status.Events { + if evt.JobName != "" { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: evt.JobName, Namespace: "default", + }}) + } + } + }) + + r.processApprovals(ctx, p) + + for i := range p.Status.Events { + Expect(p.Status.Events[i].State).To(Equal(intelv1a1.RecoveryEventStateInProgress), + "event %s should have started under the same approval", p.Status.Events[i].ID) + Expect(p.Status.Events[i].ApprovalID).To(Equal("sel-batch")) + } + + Expect(p.Spec.Approvals[0].Consumed).To(BeTrue()) + }) + + It("should not consume a persistent approval", func() { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-consume-persistent"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-persistent", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Persistent: true, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-persistent-1", NodeName: "node01", GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }, + }, + }, + } + + createPlanForOwnerRef(p) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: "recovery-evt-persistent-1-0", Namespace: "default", + }}) + }) + + r.processApprovals(ctx, p) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(p.Spec.Approvals[0].Consumed).To(BeFalse(), + "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() { + r := newTestReconciler() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-reflash-park"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "app-reflash", EventID: "evt-reflash-park"}, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-reflash-park", NodeName: "node01", GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeReflash}, + Reason: reasonSurvivability, + State: intelv1a1.RecoveryEventStateWaitingApproval, + }, + }, + }, + } + + createPlanForOwnerRef(p) + + r.processApprovals(ctx, p) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) + Expect(evt.JobName).To(BeEmpty()) + Expect(evt.StateMessage).To(ContainSubstring("reflash")) + Expect(p.Spec.Approvals[0].Consumed).To(BeFalse(), + "an approval that produced no Job must stay available") + + job := &batch.Job{} + err := k8sClient.Get(ctx, types.NamespacedName{ + 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") + }) + }) + + Context("Re-approval of permanently failed events", func() { + It("should restart a failed event when an explicit EventID approval is added", func() { + r := newTestReconciler() + now := metav1.Now() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-reapprove"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 2, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "reapp-001", EventID: "evt-exhausted"}, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-exhausted", + NodeName: "node02", + GPUBDF: "0000:03:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateFailed, + RetryCount: 2, // exhausted + LastUpdated: &now, + ApprovalID: "old-approval", + }, + }, + }, + } + + createPlanForOwnerRef(p) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, &batch.Job{ObjectMeta: metav1.ObjectMeta{ + Name: "recovery-evt-exhausted-0", Namespace: "default", + }}) + }) + + r.processApprovals(ctx, p) + + evt := &p.Status.Events[0] + Expect(evt.RetryCount).To(BeZero(), "retry count must be reset on re-approval") + Expect(evt.ApprovalID).To(Equal("reapp-001")) + // Re-approval falls through into the same pass, so the Job starts immediately rather + // than waiting for another reconcile. + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(p.Spec.Approvals[0].Consumed).To(BeTrue()) + }) + + It("should not restart a failed event via a selector approval", func() { + r := newTestReconciler() + now := metav1.Now() + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-no-selector-restart"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 2, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-approval", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Persistent: true, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-perm-failed", + NodeName: "node03", + GPUBDF: "0000:04:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateFailed, + RetryCount: 2, // exhausted + LastUpdated: &now, + }, + }, + }, + } + + r.processApprovals(ctx, p) + + // State must remain failed — a standing approval must not keep retrying a GPU that has + // already failed its way out of the budget that same approval granted. + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateFailed)) + }) + + It("findExplicitApprovalForEvent should only match EventID approvals", func() { + r := newTestReconciler() + + evt := &intelv1a1.RecoveryEvent{ID: "evt-xyz", State: intelv1a1.RecoveryEventStateFailed} + + // Should match an explicit EventID approval. + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "explicit-1", EventID: "evt-xyz"}, + }, + }, + } + approval, ok := r.findExplicitApprovalForEvent(p, evt) + Expect(ok).To(BeTrue()) + Expect(approval.ID).To(Equal("explicit-1")) + + // Should NOT match a consumed EventID approval. + pConsumed := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "explicit-2", EventID: "evt-xyz", Consumed: true}, + }, + }, + } + _, ok = r.findExplicitApprovalForEvent(pConsumed, evt) + Expect(ok).To(BeFalse()) + + // Should NOT match a selector-only approval. + pSelector := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-1", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + }, + }, + }, + } + _, ok = r.findExplicitApprovalForEvent(pSelector, evt) + Expect(ok).To(BeFalse()) + }) + }) + + Context("pruneConsumedApprovals", func() { + It("should remove a consumed non-persistent approval when no event references it", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "consumed-gone", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Consumed: true, + }, + {ID: "active", EventID: "evt-1"}, + }, + }, + // No events reference "consumed-gone". + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{{ID: "evt-1", ApprovalID: "active"}}, + }, + } + + pruneConsumedApprovals(p) + + Expect(p.Spec.Approvals).To(HaveLen(1)) + Expect(p.Spec.Approvals[0].ID).To(Equal("active")) + }) + + It("should keep a consumed non-persistent approval while an event still references it", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "sel-used", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Consumed: true, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + {ID: "evt-active", ApprovalID: "sel-used", State: intelv1a1.RecoveryEventStateInProgress}, + }, + }, + } + + pruneConsumedApprovals(p) + + Expect(p.Spec.Approvals).To(HaveLen(1), + "consumed approval must be kept while its event is still active") + }) + + It("should never prune a persistent approval", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + { + ID: "persistent-sel", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSBR}, + Persistent: true, + // Consumed is deliberately true: persistent approvals are never pruned + // whatever else is set on them. + Consumed: true, + }, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{}, // no events + } + + pruneConsumedApprovals(p) + + Expect(p.Spec.Approvals).To(HaveLen(1), "persistent approvals must never be pruned") + }) + + It("should keep unconsumed approvals regardless of event references", func() { + p := &intelv1a1.GPURecoveryPlan{ + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + Approvals: []intelv1a1.RecoveryApproval{ + {ID: "pending", EventID: "evt-future"}, + }, + }, + Status: intelv1a1.GPURecoveryPlanStatus{}, // no events yet + } + + pruneConsumedApprovals(p) + + Expect(p.Spec.Approvals).To(HaveLen(1), "unconsumed approvals must not be pruned") + }) + }) + + Context("Helper: recoveryTypeToArgs", func() { + const bdf = "0000:02:00.0" + + // Every reset type in the enum must map to a distinct xpu-smi invocation, or an admin's + // override between two of them would look like a change while running the same command. + It("should give every reset type its own distinct command", func() { + resetTypes := []intelv1a1.RecoveryType{ + intelv1a1.RecoveryTypeSBR, + intelv1a1.RecoveryTypeSlot, + intelv1a1.RecoveryTypeAMC, + } + + seen := map[string]intelv1a1.RecoveryType{} + + for _, rt := range resetTypes { + args := recoveryTypeToArgs(bdf, rt) + Expect(args).NotTo(BeEmpty(), "reset type %q must map to a command", rt) + + key := strings.Join(args, " ") + Expect(seen).NotTo(HaveKey(key), + "reset types %q and %q share the command %q, so an override between them is a no-op", + seen[key], rt, key) + + seen[key] = rt + } + }) + + It("should address the BDF the event names", func() { + Expect(recoveryTypeToArgs("0000:af:00.0", intelv1a1.RecoveryTypeSBR)). + To(ContainElement("0000:af:00.0")) + }) + + It("should return nil for reflash, which is not an xpu-smi reset", func() { + Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryTypeReflash)).To(BeNil()) + }) + + It("should return nil for a type outside the enum", func() { + Expect(recoveryTypeToArgs(bdf, intelv1a1.RecoveryType("flr"))).To(BeNil()) + }) + }) + + // 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. + Context("Job outcomes: Jobs retained for diagnostics", func() { + // putJob creates a Job and drives its status subresource to the given terminal condition. + // K8s 1.36 requires startTime plus the interim condition before the terminal one. + putJob := func(name, planName, evtID string, complete bool, reason string) { + job := makeTestJob(name, "default", map[string]string{ + recoveryJobLabelPlan: planName, + recoveryJobLabelEvent: evtID, + }, batch.JobComplete) + job.Status = batch.JobStatus{} // status is not settable on create + + Expect(k8sClient.Create(ctx, job)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + startTime := metav1.Now() + + if complete { + job.Status = batch.JobStatus{ + StartTime: &startTime, + CompletionTime: &startTime, + Conditions: []batch.JobCondition{ + {Type: batch.JobSuccessCriteriaMet, Status: core.ConditionTrue}, + {Type: batch.JobComplete, Status: core.ConditionTrue}, + }, + } + } else { + job.Status = batch.JobStatus{ + StartTime: &startTime, + Conditions: []batch.JobCondition{ + {Type: batch.JobFailureTarget, Status: core.ConditionTrue}, + {Type: batch.JobFailed, Status: core.ConditionTrue, Reason: reason}, + }, + } + } + + Expect(k8sClient.Status().Update(ctx, job)).To(Succeed()) + } + + planInProgress := func(planName, evtID, jobName string) *intelv1a1.GPURecoveryPlan { + return &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: planName}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: 3, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: evtID, + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateInProgress, + JobName: jobName, + }, + }, + }, + } + } + + It("should move JobName to PastJobs and not delete the Job when a Job fails", func() { + r := newTestReconciler() + p := planInProgress("plan-fail-diag", "evt-fail-001", "recovery-evt-fail-001-0") + + putJob("recovery-evt-fail-001-0", p.Name, "evt-fail-001", false, "BackoffLimitExceeded") + + Expect(r.syncJobStatuses(ctx, p)).To(Succeed()) + + evt := &p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateFailed)) + Expect(evt.PastJobs).To(ContainElement("recovery-evt-fail-001-0")) + Expect(evt.JobName).To(BeEmpty()) + Expect(evt.RetryCount).To(BeNumerically("==", 1)) + + // The Job outlives the event, but its pods do not outlive the plan's cleanup, so the + // verdict is copied onto the event. The attempt count is what says whether the operator + // will try again — retryCount alone does not, without also knowing spec.maxRetries. + Expect(evt.StateMessage).To(SatisfyAll( + ContainSubstring("recovery-evt-fail-001-0"), + ContainSubstring("attempt 1 of 3"), + ContainSubstring("BackoffLimitExceeded"), + )) + + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-evt-fail-001-0", + Namespace: "default", + }, &batch.Job{})).To(Succeed(), "a failed Job must remain for diagnostics until the event is removed") + }) + + It("should move JobName to PastJobs and not delete the Job when a Job succeeds", func() { + r := newTestReconciler() + p := planInProgress("plan-success-retain", "evt-ok-001", "recovery-evt-ok-001-0") + + putJob("recovery-evt-ok-001-0", p.Name, "evt-ok-001", true, "") + + Expect(r.syncJobStatuses(ctx, p)).To(Succeed()) + + evt := &p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateSucceeded)) + Expect(evt.PastJobs).To(ContainElement("recovery-evt-ok-001-0")) + Expect(evt.JobName).To(BeEmpty()) + Expect(evt.RetryCount).To(BeZero()) + + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: "recovery-evt-ok-001-0", + Namespace: "default", + }, &batch.Job{})).To(Succeed(), "a succeeded Job must remain until the event is removed") + }) + + // A Job that has gone missing is not a failure of the recovery: reporting one would burn a + // retry and could park the event in failed while the reset it started is still running. + It("should leave an event in-progress when its Job cannot be read", func() { + r := newTestReconciler() + p := planInProgress("plan-job-missing", "evt-missing-001", "recovery-evt-missing-001-0") + + Expect(r.syncJobStatuses(ctx, p)).To(Succeed()) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(p.Status.Events[0].RetryCount).To(BeZero()) + }) + + It("should report an in-flight Job as an active Job", func() { + Expect(hasActiveJobs(planInProgress("plan-active", "evt-a", "job-a"))).To(BeTrue()) + + done := planInProgress("plan-done", "evt-b", "") + done.Status.Events[0].State = intelv1a1.RecoveryEventStateSucceeded + Expect(hasActiveJobs(done)).To(BeFalse()) + }) + }) + + // Recovery Jobs must be owned by the plan. Without an owner reference the Owns(&batch.Job{}) + // watch in SetupWithManager never fires, so every state transition waits out a full + // RequeueDelay, and any Job that deleteAllJobs misses leaks. + Context("Owner references on recovery Jobs", func() { + // planWithEvent returns a plan (created in the API server, so it has a UID for the owner + // reference) plus a pending event of the given recovery type. + planWithEvent := 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, + }, + 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) + + return p + } + + // expectOwned asserts the Job carries exactly one controller reference pointing at the + // plan, with the fields the garbage collector and the owner handler both require. + expectOwned := func(jobName string, p *intelv1a1.GPURecoveryPlan) *batch.Job { + job := &batch.Job{} + Expect(k8sClient.Get(ctx, types.NamespacedName{ + Name: jobName, Namespace: "default", + }, job)).To(Succeed()) + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + Expect(job.OwnerReferences).To(HaveLen(1)) + + ref := job.OwnerReferences[0] + Expect(ref.Kind).To(Equal("GPURecoveryPlan")) + Expect(ref.APIVersion).To(Equal(intelv1a1.GroupVersion.String())) + Expect(ref.Name).To(Equal(p.Name)) + // A UID mismatch makes the GC treat the reference as dangling and delete the Job. + Expect(ref.UID).To(Equal(p.UID)) + Expect(ref.Controller).To(HaveValue(BeTrue())) + + return job + } + + It("should own a reset Job", func() { + r := newTestReconciler() + p := planWithEvent("plan-own-reset", "evt-own-reset", intelv1a1.RecoveryTypeSBR) + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + job := expectOwned("recovery-evt-own-reset-0", p) + Expect(job.Labels).To(HaveKeyWithValue(recoveryJobLabelPlan, "plan-own-reset")) + Expect(job.Labels).To(HaveKeyWithValue(recoveryJobLabelEvent, "evt-own-reset")) + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + }) + + // The reset writes to the GPU's PCIe config space through sysfs on one specific node, and + // nothing else places the pod there: NodeName is set directly, which bypasses the + // scheduler, so the blanket toleration is what keeps the taint manager from evicting it + // mid-reset off a node that is already fenced off as broken. + It("should pin the Job to the event's node and tolerate its taints", func() { + r := newTestReconciler() + p := planWithEvent("plan-own-pinned", "evt-own-pinned", intelv1a1.RecoveryTypeSlot) + p.Spec.XpuSmi = intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:v1", PullPolicy: "Always"} + p.Spec.Tolerations = []core.Toleration{{Key: "extra", Operator: core.TolerationOpExists}} + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + job := expectOwned("recovery-evt-own-pinned-0", p) + + Expect(job.Spec.Template.Spec.NodeName).To(Equal("node01")) + Expect(job.Spec.Template.Spec.Tolerations).To(ContainElement( + core.Toleration{Operator: core.TolerationOpExists})) + Expect(job.Spec.Template.Spec.Tolerations).To(ContainElement( + core.Toleration{Key: "extra", Operator: core.TolerationOpExists})) + + resetter := findJobContainer(job, "resetter") + Expect(resetter).NotTo(BeNil()) + Expect(resetter.Image).To(Equal("registry/xpu-smi:v1")) + Expect(resetter.ImagePullPolicy).To(Equal(core.PullAlways)) + // The BDF has to reach the command line, not just the event. + 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" + + p := planWithEvent("plan-own-secret", "evt-own-secret", intelv1a1.RecoveryTypeSBR) + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + job := expectOwned("recovery-evt-own-secret-0", p) + Expect(job.Spec.Template.Spec.ImagePullSecrets).To(ConsistOf( + core.LocalObjectReference{Name: "operator-pull-secret"})) + }) + + // The reference existing is not the same as it being usable. This drives the real + // controller-runtime owner handler to prove the emitted request is what Reconcile expects: + // a cluster-scoped owner must yield Request{Name: plan} with NO namespace, otherwise the + // lookup would target "default/plan-..." and silently never match. + It("should enqueue a namespace-less request for the plan on a Job event", func() { + r := newTestReconciler() + p := planWithEvent("plan-own-enqueue", "evt-own-enqueue", intelv1a1.RecoveryTypeSBR) + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + job := expectOwned("recovery-evt-own-enqueue-0", p) + + h := handler.EnqueueRequestForOwner(k8sClient.Scheme(), k8sClient.RESTMapper(), + &intelv1a1.GPURecoveryPlan{}, handler.OnlyControllerOwner()) + + q := &trackingQueue{} + h.Update(ctx, event.UpdateEvent{ObjectOld: job, ObjectNew: job}, q) + + Expect(q.added).To(ConsistOf(reconcile.Request{ + NamespacedName: types.NamespacedName{Name: "plan-own-enqueue"}, + })) + }) + + // An earlier pass may have created the Job and lost its status write. The name embeds the + // event ID and attempt index, so the existing Job is the very one this attempt wanted: + // adopting it is right, and failing would strand the event in waiting-approval forever. + It("should adopt an existing Job of the same name", func() { + r := newTestReconciler() + p := planWithEvent("plan-own-adopt", "evt-own-adopt", intelv1a1.RecoveryTypeSBR) + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + expectOwned("recovery-evt-own-adopt-0", p) + + // Second attempt at the same event, still on attempt 0. + p.Status.Events[0].State = intelv1a1.RecoveryEventStateWaitingApproval + p.Status.Events[0].JobName = "" + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(p.Status.Events[0].JobName).To(Equal("recovery-evt-own-adopt-0")) + }) + + // The attempt index in the name is what keeps a retry from colliding with the Job that + // already failed — and that Job is still there, kept for diagnostics. + It("should name a retry after its attempt index", func() { + r := newTestReconciler() + p := planWithEvent("plan-own-retry", "evt-own-retry", intelv1a1.RecoveryTypeSBR) + p.Status.Events[0].PastJobs = []string{"recovery-evt-own-retry-0"} + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + expectOwned("recovery-evt-own-retry-1", p) + Expect(p.Status.Events[0].JobName).To(Equal("recovery-evt-own-retry-1")) + }) + }) + + 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() + key := types.NamespacedName{Name: "plan-long-node"} + + // 62 characters on its own — longer than the whole Job-name budget, and the kind of + // name a real cloud provider hands out. + const longNode = "ip-10-0-134-22.us-west-2.compute.internal.example-cluster.prod" + + slice := &resv1.ResourceSlice{ + ObjectMeta: metav1.ObjectMeta{Name: "slice-long-node"}, + Spec: resv1.ResourceSliceSpec{ + Driver: "gpu.intel.com", + NodeName: ptr.To(longNode), + Pool: resv1.ResourcePool{Name: "pool-long-node", ResourceSliceCount: 1}, + Devices: []resv1.Device{{ + Name: "dev-0000-af-00-0", + Attributes: map[resv1.QualifiedName]resv1.DeviceAttribute{ + deviceAttrDeviceID: {StringValue: ptr.To("0xbeef")}, + // Uppercase hex, as lspci prints it: this must be sanitized rather + // than rejected. + deviceAttrBDF: {StringValue: ptr.To("0000:AF:00.0")}, + }, + Taints: []resv1.DeviceTaint{ + {Key: deviceTaintKeyReset, Effect: resv1.DeviceTaintEffectNoSchedule}, + }, + }}, + }, + } + Expect(k8sClient.Create(ctx, slice)).To(Succeed()) + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, slice) + }) + + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: key.Name, Finalizers: []string{recoveryPlanFinalizer}}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xbeef", + MaxRetries: 3, + Approvals: []intelv1a1.RecoveryApproval{{ + ID: "app-any-reset", + Selector: &intelv1a1.ApprovalSelector{RecoveryType: intelv1a1.RecoveryTypeSlot}, + }}, + }, + } + Expect(k8sClient.Create(ctx, p)).To(Succeed()) + DeferCleanup(func() { + fresh := &intelv1a1.GPURecoveryPlan{} + if err := k8sClient.Get(ctx, key, fresh); err == nil { + fresh.Finalizers = nil + _ = k8sClient.Update(ctx, fresh) + _ = k8sClient.Delete(ctx, fresh) + } + }) + + _, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: key}) + Expect(err).NotTo(HaveOccurred()) + + updated := &intelv1a1.GPURecoveryPlan{} + Expect(k8sClient.Get(ctx, key, updated)).To(Succeed()) + Expect(updated.Status.Events).To(HaveLen(1)) + + evt := updated.Status.Events[0] + + // in-progress with a jobName is the proof: had the create been rejected, the event + // would still be waiting-approval with no Job, and the only trace would be a status + // message. + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress), + "event should be in-progress; messages: %v", updated.Status.Messages) + Expect(evt.JobName).NotTo(BeEmpty()) + + job := &batch.Job{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: evt.JobName, Namespace: "default"}, job)). + To(Succeed(), "the Job the event claims to own must actually exist") + + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, job) + }) + + // The label selector deleteEventJobs uses is subject to the same 63-byte cap as the + // name, so read it back off the created object rather than trusting it. + Expect(job.Labels[recoveryJobLabelEvent]).To(Equal(evt.ID)) + }) + }) + + // A GPU whose taint is still there after a failed attempt gets another go, up to + // spec.maxRetries. The event ID is reused, so a standing group approval re-approves the same + // event rather than fanning one broken GPU out into a flood of events. + Context("Helper: requeueFailedEvents", func() { + failedPlan := func(maxRetries int32, retryCount int32) *intelv1a1.GPURecoveryPlan { + return &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-requeue"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + MaxRetries: maxRetries, + }, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{ + { + ID: "evt-requeue", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSBR}, + State: intelv1a1.RecoveryEventStateFailed, + RetryCount: retryCount, + PastJobs: []string{"recovery-evt-requeue-0"}, + LastUpdated: ptr.To(metav1.Now()), + }, + }, + }, + } + } + + stillTainted := map[deviceKey]deviceNeed{ + {node: "node01", bdf: "0000:02:00.0"}: {rt: intelv1a1.RecoveryTypeSBR, reason: reasonWedged}, + } + + It("should send a failed event back to waiting-approval while the taint persists", func() { + p := failedPlan(3, 1) + + requeueFailedEvents(p, stillTainted) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateWaitingApproval)) + Expect(evt.ID).To(Equal("evt-requeue"), "the ID must be reused so a standing approval re-matches") + Expect(evt.RetryCount).To(BeNumerically("==", 1), "the budget is spent by the failure, not by the re-queue") + Expect(evt.PastJobs).To(ContainElement("recovery-evt-requeue-0")) + Expect(evt.StateMessage).To(ContainSubstring("re-queued")) + Expect(p.Status.Messages).To(ContainElement(ContainSubstring("re-queued for retry 1/3"))) + }) + + It("should leave an event alone once its retry budget is spent", func() { + p := failedPlan(2, 2) + + requeueFailedEvents(p, stillTainted) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateFailed)) + Expect(p.Status.Messages).To(BeEmpty()) + }) + + // maxRetries: 0 turns automatic retrying off entirely, which has to hold on the very first + // failure rather than allowing one free attempt. + It("should not retry at all when maxRetries is zero", func() { + p := failedPlan(0, 0) + + requeueFailedEvents(p, stillTainted) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateFailed)) + }) + + It("should leave a failed event whose taint has cleared for removeResolvedEvents", func() { + p := failedPlan(3, 1) + + requeueFailedEvents(p, map[deviceKey]deviceNeed{}) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateFailed)) + }) + + It("should ignore events in any other state", func() { + p := failedPlan(3, 0) + p.Status.Events[0].State = intelv1a1.RecoveryEventStateInProgress + + requeueFailedEvents(p, stillTainted) + + Expect(p.Status.Events[0].State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + }) + }) + + // An event with a Job in flight is the only record of that Job. Both phases that would + // otherwise rewrite or drop it have to leave it alone until syncJobStatuses has resolved it. + Context("In-progress events are not disturbed", func() { + var r *GPURecoveryPlanReconciler + + BeforeEach(func() { + r = newTestReconciler() + }) + + It("should defer an escalation while a Job is in flight", func() { + p := &intelv1a1.GPURecoveryPlan{ObjectMeta: metav1.ObjectMeta{Name: "plan-esc-inflight"}} + evt := &intelv1a1.RecoveryEvent{ + ID: "evt-esc-inflight", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: intelv1a1.RecoveryTypeSlot}, + State: intelv1a1.RecoveryEventStateInProgress, + JobName: "recovery-evt-esc-inflight-0", + } + + escalateEvent(p, evt, deviceNeed{rt: intelv1a1.RecoveryTypeReflash, reason: reasonSurvivability}) + + // A new ID would orphan the Job named after the old one. + Expect(evt.ID).To(Equal("evt-esc-inflight")) + Expect(evt.RecoveryType.Type).To(Equal(intelv1a1.RecoveryTypeSlot)) + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateInProgress)) + Expect(evt.JobName).To(Equal("recovery-evt-esc-inflight-0")) + }) + + It("should keep an in-progress event whose taint has cleared", func() { + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-resolved-inflight"}, + Status: intelv1a1.GPURecoveryPlanStatus{ + Events: []intelv1a1.RecoveryEvent{{ + ID: "evt-inflight", NodeName: "node01", GPUBDF: "0000:02:00.0", + State: intelv1a1.RecoveryEventStateInProgress, + JobName: "recovery-evt-inflight-0", + }}, + }, + } + + // The taint clearing mid-reset is the normal case: the reset worked. + r.removeResolvedEvents(ctx, p, map[deviceKey]deviceNeed{}) + + Expect(p.Status.Events).To(HaveLen(1), + "dropping the event now would leave its Job collected by nothing") + Expect(p.Status.Messages).To(BeEmpty()) + }) + }) +}) + +// 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 2bbb0d9..39fb2b4 100644 --- a/internal/controller/gpurecoveryplan_helpers.go +++ b/internal/controller/gpurecoveryplan_helpers.go @@ -17,14 +17,23 @@ limitations under the License. package controller import ( + "context" "crypto/sha256" "encoding/hex" "fmt" "strings" + "time" + "unicode/utf8" + batch "k8s.io/api/batch/v1" + core "k8s.io/api/core/v1" resv1 "k8s.io/api/resource/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/klog/v2" + "sigs.k8s.io/controller-runtime/pkg/client" intelv1a1 "github.com/intel/gpu-base-operator/api/v1alpha1" ) @@ -209,3 +218,433 @@ func hashSegment(s string) string { return hex.EncodeToString(sum[:])[:idHashLen] } + +// recoveryTypeToArgs returns the xpu-smi command-line arguments that carry out the given reset +// against the given BDF. Returns nil for a type xpu-smi has no reset for, reflash above all: it +// writes firmware rather than resetting the device, so it is a different Job entirely. +func recoveryTypeToArgs(bdf string, rt intelv1a1.RecoveryType) []string { + switch rt { + case intelv1a1.RecoveryTypeSBR: + return []string{"config", "-d", bdf, "--reset"} + case intelv1a1.RecoveryTypeSlot: + return []string{"config", "-d", bdf, "--coldreset"} + case intelv1a1.RecoveryTypeAMC: + return []string{"amc", "--gpureset", "-d", bdf, "-y"} + default: + return nil + } +} + +// 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. +func nodeSelectorMatches(sel map[string]string, nodeName string, cache *nodeLabelCache) bool { + if len(sel) == 0 { + return true + } + + nodeLabels, ok := cache.get(nodeName) + if !ok { + return false + } + + return labels.SelectorFromSet(sel).Matches(labels.Set(nodeLabels)) +} + +// nodeLabelCache memoises Node label lookups for the duration of a single findMatchingApproval +// call, where many events across a handful of nodes may each be tested against several label +// selectors. A failed lookup is recorded too, so it is not retried within the same call. +type nodeLabelCache struct { + r *GPURecoveryPlanReconciler + ctx context.Context + labels map[string]map[string]string + failed map[string]struct{} +} + +func newNodeLabelCache(ctx context.Context, r *GPURecoveryPlanReconciler) *nodeLabelCache { + return &nodeLabelCache{ + r: r, + ctx: ctx, + labels: make(map[string]map[string]string), + failed: make(map[string]struct{}), + } +} + +// get returns the labels of the named Node. The second return value is false when the Node could +// not be read, in which case the caller must not treat the selector as matched: a standing +// approval scoped to a set of nodes must not authorise a reset on a node whose membership of that +// set could not be confirmed. +func (c *nodeLabelCache) get(nodeName string) (map[string]string, bool) { + if l, ok := c.labels[nodeName]; ok { + return l, true + } + + if _, failed := c.failed[nodeName]; failed { + return nil, false + } + + node := &core.Node{} + if err := c.r.Get(c.ctx, types.NamespacedName{Name: nodeName}, node); err != nil { + klog.Errorf("failed to get node %s for approval label matching: %v", nodeName, err) + c.failed[nodeName] = struct{}{} + + return nil, false + } + + c.labels[nodeName] = node.Labels + + return node.Labels, true +} + +// jobIsTerminal reports whether a Job has finished, successfully or not. Only Complete and Failed +// are terminal; a Job with no conditions yet is still running. +func jobIsTerminal(job *batch.Job) bool { + for _, cond := range job.Status.Conditions { + if cond.Status != core.ConditionTrue { + continue + } + + if cond.Type == batch.JobComplete || cond.Type == batch.JobFailed { + return true + } + } + + return false +} + +// jobFailureDetail renders a failed Job's condition as the reason it failed. The Reason is the +// machine-readable verdict (BackoffLimitExceeded, DeadlineExceeded), the Message is the Job +// controller's sentence about it; either can be empty depending on how the Job failed, so this +// reports whichever are there rather than composing an empty parenthetical. +func jobFailureDetail(cond batch.JobCondition) string { + switch { + case cond.Reason != "" && cond.Message != "": + return fmt.Sprintf("%s (%s)", cond.Reason, cond.Message) + case cond.Reason != "": + return cond.Reason + case cond.Message != "": + return cond.Message + default: + return "no reason reported by the Job controller" + } +} + +// setEventState records the state an event is moving to together with the sentence that explains it, +// and stamps LastUpdated. Every state write goes through it, so status.events[].stateMessage always +// describes the state next to it. +// nolint:unparam +func setEventState(evt *intelv1a1.RecoveryEvent, state intelv1a1.RecoveryEventState, format string, args ...any) metav1.Time { + msg := "" + if format != "" { + msg = capString(fmt.Sprintf(format, args...), maxStateMessageLen) + } + + if evt.State == state && evt.StateMessage == msg && evt.LastUpdated != nil { + return *evt.LastUpdated + } + + now := metav1.NewTime(time.Now()) + evt.State = state + evt.StateMessage = msg + evt.LastUpdated = &now + + return now +} + +// capString truncates a single string for storage in status, marking it when anything was cut so a +// reader can tell a complete message from a clipped one. Counted in bytes rather than runes, since +// the limit exists to bound the size of the stored object. +func capString(s string, limit int) string { + const marker = "..." + + if len(s) <= limit { + return s + } + + // Back off to a rune boundary so the result stays valid UTF-8; the API server would otherwise + // reject the status write outright. + cut := limit - len(marker) + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + + return s[:cut] + marker +} + +// findEventForDevice returns the index into status.events of the existing event for the +// given node+BDF, or -1 if there is none. +func findEventForDevice(plan *intelv1a1.GPURecoveryPlan, nodeName, bdf string) int { + for i := range plan.Status.Events { + if plan.Status.Events[i].NodeName == nodeName && plan.Status.Events[i].GPUBDF == bdf { + return i + } + } + + return -1 +} + +// addRecoveryEvent appends a new RecoveryEvent in waiting-approval state to the plan status. +func addRecoveryEvent(plan *intelv1a1.GPURecoveryPlan, nodeName, bdf string, need deviceNeed) string { + id := generateEventID(nodeName, bdf, need.rt) + + evt := intelv1a1.RecoveryEvent{ + ID: id, + NodeName: nodeName, + GPUBDF: bdf, + Reason: need.reason, + RecoveryType: intelv1a1.RecoveryTypeSpec{Type: need.rt}, + RetryCount: 0, + } + + // No message: reason, nodeName, gpuBDF and recoveryType already say everything about a new + // event, and restating them would only train an admin to ignore the field. + setEventState(&evt, intelv1a1.RecoveryEventStateWaitingApproval, "") + + plan.Status.Events = append(plan.Status.Events, evt) + + return id +} + +// hasActiveJobs reports whether any event still has a Job in flight. +func hasActiveJobs(plan *intelv1a1.GPURecoveryPlan) bool { + for _, evt := range plan.Status.Events { + if evt.State == intelv1a1.RecoveryEventStateInProgress { + return true + } + } + + return false +} + +// updatePlanState derives the overall plan state from the current events and sets status.state, +// the value shown in the "State" print column of `kubectl get gpurecoveryplan`. +func updatePlanState(plan *intelv1a1.GPURecoveryPlan) { + anyActive := false + anyStuck := false + + for _, evt := range plan.Status.Events { + switch evt.State { + case intelv1a1.RecoveryEventStateWaitingApproval, + intelv1a1.RecoveryEventStateBlocked, + intelv1a1.RecoveryEventStateDraining, + intelv1a1.RecoveryEventStateInProgress: + // blocked is active, not stuck: it clears on its own once the node frees up. + anyActive = true + + case intelv1a1.RecoveryEventStateMissingFirmware: + // Blocked on operator configuration, not on hardware or an admin decision: + // the reflash cannot even be attempted until spec.firmware is filled in. + anyStuck = true + + case intelv1a1.RecoveryEventStateFailed: + // A failure within the retry budget is re-queued for another approval, so only an + // event that has spent its budget needs an admin. + if evt.RetryCount >= plan.Spec.MaxRetries { + anyStuck = true + } + } + } + + switch { + case anyStuck: + plan.Status.State = intelv1a1.PlanStateError + case anyActive: + plan.Status.State = intelv1a1.PlanStateActive + default: + plan.Status.State = intelv1a1.PlanStateIdle + } +} + +// pruneConsumedApprovals removes consumed one-shot approvals that are not referred by status.events +func pruneConsumedApprovals(plan *intelv1a1.GPURecoveryPlan) { + referenced := make(map[string]struct{}, len(plan.Status.Events)) + + for _, evt := range plan.Status.Events { + if evt.ApprovalID != "" { + referenced[evt.ApprovalID] = struct{}{} + } + } + + kept := plan.Spec.Approvals[:0] + + for _, a := range plan.Spec.Approvals { + if a.Consumed && !a.Persistent { + if _, active := referenced[a.ID]; !active { + klog.Infof("GPURecoveryPlan %s: pruning consumed approval %s (no active events reference it)", + plan.Name, a.ID) + + continue + } + } + + kept = append(kept, a) + } + + plan.Spec.Approvals = kept +} + +// appendMessage appends a message to status.messages, evicting the oldest entry if the +// cap (maxStatusMessages) has been reached. +func appendMessage(plan *intelv1a1.GPURecoveryPlan, msg string) { + plan.Status.Messages = append(plan.Status.Messages, msg) + + for len(plan.Status.Messages) > maxStatusMessages { + plan.Status.Messages = plan.Status.Messages[1:] + } +} + +// requeueFailedEvents sends a failed event back to waiting-approval while its device taint is +// still there and its retry budget (spec.maxRetries) is not spent. +func requeueFailedEvents(plan *intelv1a1.GPURecoveryPlan, active map[deviceKey]deviceNeed) { + maxRetries := plan.Spec.MaxRetries + + for i := range plan.Status.Events { + evt := &plan.Status.Events[i] + if evt.State != intelv1a1.RecoveryEventStateFailed { + continue + } + + key := deviceKey{node: evt.NodeName, bdf: evt.GPUBDF} + if _, stillActive := active[key]; !stillActive { + // The recovery worked, or something else fixed the GPU: removeResolvedEvents has it. + continue + } + + if evt.RetryCount >= maxRetries { + klog.V(2).Infof("GPURecoveryPlan %s: event %s for %s/%s has reached max retries (%d); leaving as failed", + plan.Name, evt.ID, evt.NodeName, evt.GPUBDF, maxRetries) + + continue + } + + // The message is what distinguishes a re-queued event from one asking for its first + // approval. It replaces the explanation of the failure, which described the state the + // event is now leaving. + setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, + "re-queued for retry %d of %d after the previous attempt failed; the device taint persists", + evt.RetryCount, maxRetries) + + klog.Infof("GPURecoveryPlan %s: re-queuing failed event %s for %s/%s (retry %d/%d)", + plan.Name, evt.ID, evt.NodeName, evt.GPUBDF, evt.RetryCount, maxRetries) + appendMessage(plan, fmt.Sprintf("Event %s re-queued for retry %d/%d (taint persists on %s/%s)", + evt.ID, evt.RetryCount, maxRetries, evt.NodeName, evt.GPUBDF)) + } +} + +// addNewEvents creates a RecoveryEvent for every tainted device that does not have one +// yet, up to maxStatusEvents entries in status.events. +func addNewEvents(plan *intelv1a1.GPURecoveryPlan, active map[deviceKey]deviceNeed) { + skipped := 0 + + for dk, need := range active { + // A device that already has an event does not get a second one, but its taints may + // since have escalated to a more severe recovery type. + if i := findEventForDevice(plan, dk.node, dk.bdf); i >= 0 { + escalateEvent(plan, &plan.Status.Events[i], need) + + continue + } + + if len(plan.Status.Events) >= maxStatusEvents { + skipped++ + + continue + } + + eventId := addRecoveryEvent(plan, dk.node, dk.bdf, need) + + klog.Infof("GPURecoveryPlan %s: added recovery event %s for device %s on node %s (reason: %s)", + plan.Name, eventId, dk.bdf, dk.node, need.reason) + } + + if skipped > 0 { + msg := fmt.Sprintf("status.events is at its %d-entry limit: %d newly detected device(s) not recorded", + maxStatusEvents, skipped) + + appendMessage(plan, msg) + klog.Warningf("GPURecoveryPlan %s: %s", plan.Name, msg) + } +} + +// escalateEvent up-levels an existing event in place when the device's taints now call for a more +// severe recovery than the event was created for. +func escalateEvent(plan *intelv1a1.GPURecoveryPlan, evt *intelv1a1.RecoveryEvent, need deviceNeed) { + if recoveryTypePriority(need.rt) <= recoveryTypePriority(evt.RecoveryType.Type) { + return + } + + if evt.State == intelv1a1.RecoveryEventStateInProgress { + klog.V(2).Infof("GPURecoveryPlan %s: device %s/%s now needs %s but event %s has a Job in flight; deferring escalation", + plan.Name, evt.NodeName, evt.GPUBDF, need.rt, evt.ID) + + return + } + + oldID := evt.ID + oldType := evt.RecoveryType.Type + + // The ID embeds the recovery type, so it has to be regenerated. + evt.ID = generateEventID(evt.NodeName, evt.GPUBDF, need.rt) + evt.RecoveryType = intelv1a1.RecoveryTypeSpec{Type: need.rt} + + // The cause changed too — the device is in survivability mode now, not merely wedged. + evt.Reason = need.reason + + // Reset event back to initial state. + evt.RetryCount = 0 + evt.ApprovalID = "" + evt.ApprovalMatchedAt = nil + + setEventState(evt, intelv1a1.RecoveryEventStateWaitingApproval, + "escalated from %s to %s (%s); the approval for the previous type no longer applies", + oldType, need.rt, need.reason) + + klog.Infof("GPURecoveryPlan %s: escalated event %s -> %s for %s/%s (%s -> %s); awaiting approval", + plan.Name, oldID, evt.ID, evt.NodeName, evt.GPUBDF, oldType, need.rt) + appendMessage(plan, fmt.Sprintf("Event %s escalated to %s on %s/%s (was %s, now %s); previous approval no longer applies", + oldID, evt.ID, evt.NodeName, evt.GPUBDF, oldType, need.rt)) +} + +// runningRecoveryJobs returns the names of this plan's Jobs that have not reached a terminal +// condition. Jobs are found by the plan label rather than by walking status.events, so a Job whose +// event entry was already pruned still holds up deletion. +func runningRecoveryJobs(cli client.Reader, ctx context.Context, ns string, plan *intelv1a1.GPURecoveryPlan) ([]string, error) { + jobList := &batch.JobList{} + + if err := cli.List(ctx, jobList, + client.InNamespace(ns), + client.MatchingLabels{recoveryJobLabelPlan: plan.Name}, + ); err != nil { + return nil, err + } + + running := make([]string, 0, len(jobList.Items)) + + for i := range jobList.Items { + job := &jobList.Items[i] + + // A Job already being deleted is not something to wait for. + if !job.DeletionTimestamp.IsZero() { + continue + } + + if !jobIsTerminal(job) { + running = append(running, job.Name) + } + } + + return running, nil +} + +// setApprovalConsumed marks the approval with the given ID as consumed +func setApprovalConsumed(plan *intelv1a1.GPURecoveryPlan, id string) { + for i := range plan.Spec.Approvals { + if plan.Spec.Approvals[i].ID == id { + plan.Spec.Approvals[i].Consumed = true + + klog.Infof("GPURecoveryPlan %s: approval %s marked as consumed", plan.Name, id) + + return + } + } +}