From ad0674052738e40299284d1ba8abbc7cdb3b1e28 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 19 Aug 2026 22:02:53 +0530 Subject: [PATCH 01/13] feat(gpufault): shared GPU fault contract and failure classification A node-level agent can turn NVIDIA Xid and NVSwitch SXid lines from the kernel log into a Kubernetes Warning Event on the task pod. Until now the message format, the Xid name and severity tables and the parser had no home in this repo, so an emitter and a consumer could only agree on them by copying, and drift. This adds flyteplugins/go/tasks/pluginmachinery/gpufault as the one definition: the event message format (FormatEventMessage, ParseEventMessage, and Sentence for the human half on its own), the Xid tables behind NameFor and SeverityFor, and the conversions to and from the typed core.GpuFault, including FromEventMessage which turns an arbitrary pod event into a fault or into nothing. The wire format is unchanged, byte for byte, because events already recorded on running clusters have to keep parsing. On top of that it adds the classification the executor applies to a failed attempt. CodeFor names the failure (GpuFallenOffBus, GpuEccUncorrectable, GpuRowRemapPending, GpuNvlinkError, GpuGspError, GpuXidError) and ClassifyFailure folds the faults observed on the pod into the failure the plugin reported: a critical fault makes it a system retryable failure so it does not burn a user retry on hardware the workload did not break, a user fault only names what went wrong and leaves the plugin's verdict alone, and a warning rides along as data. Every rule keeps the error URI, timestamp, worker, recoverability, task info and phase version the original failure carried. The package has no Kubernetes dependencies so both the emitter and the consumers can import it. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- .../pluginmachinery/gpufault/classify.go | 176 +++++++++++ .../pluginmachinery/gpufault/classify_test.go | 287 ++++++++++++++++++ .../go/tasks/pluginmachinery/gpufault/doc.go | 38 +++ .../tasks/pluginmachinery/gpufault/fault.go | 82 +++++ .../tasks/pluginmachinery/gpufault/message.go | 181 +++++++++++ .../pluginmachinery/gpufault/message_test.go | 188 ++++++++++++ .../tasks/pluginmachinery/gpufault/proto.go | 116 +++++++ .../pluginmachinery/gpufault/proto_test.go | 109 +++++++ .../go/tasks/pluginmachinery/gpufault/xid.go | 106 +++++++ .../pluginmachinery/gpufault/xid_test.go | 73 +++++ 10 files changed, 1356 insertions(+) create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/fault.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/message.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go create mode 100644 flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go new file mode 100644 index 0000000000..a0da27fd3e --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go @@ -0,0 +1,176 @@ +package gpufault + +import ( + "regexp" + + "google.golang.org/protobuf/proto" + + pluginsCore "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/core" + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" +) + +// Failure codes a GPU fault can put on an ExecutionError. They are deliberately +// coarse: a code names the class of trouble a user or an operator can act on, and the +// exact Xid number stays available on the typed fault. +const ( + // CodeGpuXidError is the catch-all for a fault with no more specific code. + CodeGpuXidError = "GpuXidError" + // CodeGpuFallenOffBus is Xid 79: the driver lost the device on the PCIe bus. + CodeGpuFallenOffBus = "GpuFallenOffBus" + // CodeGpuEccUncorrectable covers the ECC errors memory could not correct. + CodeGpuEccUncorrectable = "GpuEccUncorrectable" + // CodeGpuRowRemapPending covers row remapping recorded or failed to record, which + // needs a GPU reset before the memory is usable again. + CodeGpuRowRemapPending = "GpuRowRemapPending" + // CodeGpuNvlinkError covers NVLink faults and every NVSwitch SXid, which take + // down fabric links shared across the node. + CodeGpuNvlinkError = "GpuNvlinkError" + // CodeGpuGspError covers the GPU System Processor timing out or erroring. + CodeGpuGspError = "GpuGspError" +) + +// CodeFor maps a fault onto the failure code the user sees. +func CodeFor(f Fault) string { + if f.IsSXid() { + return CodeGpuNvlinkError + } + switch f.Code { + case 79: + return CodeGpuFallenOffBus + case 48, 94, 95, 140: + return CodeGpuEccUncorrectable + case 63, 64: + return CodeGpuRowRemapPending + case 74: + return CodeGpuNvlinkError + case 119, 120: + return CodeGpuGspError + default: + return CodeGpuXidError + } +} + +// genericCodes are the codes that say nothing about why the task failed. When a GPU +// fault is on record, naming the fault is strictly more useful than keeping one of +// these. +var genericCodes = map[string]bool{ + "": true, + "Unknown": true, + "UnknownError": true, + "Error": true, +} + +// exitCodeStyle matches a code that is only the container's exit status, for example +// "137" or "ExitCode1". Those are exit statuses reported as codes, so they carry no +// more information than a generic code does. +var exitCodeStyle = regexp.MustCompile(`^(?i:exit[ _-]?code[ _-]?)?[0-9]+$`) + +func isGenericCode(code string) bool { + return genericCodes[code] || exitCodeStyle.MatchString(code) +} + +// ClassifyFailure folds the GPU faults observed on an attempt's pod into the failure +// the plugin reported. faults are the faults recorded against that pod over the whole +// attempt, in the order they happened; a successful, running or aborted phase and an +// empty fault list are both returned unchanged. +// +// The result always carries the fault as data on ExecutionError.gpu_fault, so a +// consumer never has to read it out of the message text. +func ClassifyFailure(phase pluginsCore.PhaseInfo, faults []*core.GpuFault) pluginsCore.PhaseInfo { + if len(faults) == 0 || !phase.Phase().IsFailure() { + return phase + } + + if fault, f, a := firstOfSeverity(faults, SeverityCritical); fault != nil { + // A critical Xid means the device or the node is no longer trustworthy: the + // workload did not cause it and rerunning in place would most likely hit the + // same hardware. Charging it to the user's retry budget would burn attempts on + // a broken machine, so the failure becomes a system retryable one. Once phase 3 + // quarantines the node, the reschedule also lands somewhere else. + out := pluginsCore.PhaseInfoSystemRetryableFailure( + CodeFor(f), + prependSentence(f, a, phase.Err().GetMessage()), + phase.Info(), + ) + carryOver(out.Err(), phase.Err()) + out.Err().GpuFault = fault + return preserveShape(phase, out) + } + + if fault, f, a := firstOfSeverity(faults, SeverityUser); fault != nil { + // A user Xid is the workload's own doing, for example an out-of-bounds access + // (Xid 31). The verdict the plugin reached stands; all this adds is a name for + // what went wrong, so that the user reads "GPU memory page fault" instead of a + // bare exit code. + err := cloneExecutionError(phase.Err()) + if isGenericCode(err.GetCode()) { + err.Code = CodeGpuXidError + } + err.Message = prependSentence(f, a, err.GetMessage()) + err.GpuFault = fault + return keepVerdict(phase, err) + } + + // Only warnings: nothing about the failure changes, the fault rides along so the + // console can show what the GPU reported while the task was running. + err := cloneExecutionError(phase.Err()) + err.GpuFault = faults[0] + return keepVerdict(phase, err) +} + +// firstOfSeverity returns the first fault of the given severity in time order, along +// with its decoded form so the caller does not decode it twice. +func firstOfSeverity(faults []*core.GpuFault, severity Severity) (*core.GpuFault, Fault, Attribution) { + for _, fault := range faults { + f, a := FromProto(fault) + if f.Severity == severity { + return fault, f, a + } + } + return nil, Fault{}, Attribution{} +} + +// keepVerdict rebuilds the failure with the phase and error kind the plugin chose, +// changing only what the fault added to the error. +func keepVerdict(phase pluginsCore.PhaseInfo, err *core.ExecutionError) pluginsCore.PhaseInfo { + return preserveShape(phase, pluginsCore.PhaseInfoFailed(phase.Phase(), err, phase.Info())) +} + +// preserveShape carries over the parts of a PhaseInfo the failure constructors do not +// take: the phase version and the reason accumulated so far. +func preserveShape(phase pluginsCore.PhaseInfo, out pluginsCore.PhaseInfo) pluginsCore.PhaseInfo { + out = out.WithVersion(phase.Version()) + if reason := phase.Reason(); reason != "" { + out.WithReason(reason) + } + return out +} + +// carryOver copies the fields of the original error that describe the failure rather +// than classify it, so that reclassifying does not lose them. +func carryOver(out *core.ExecutionError, previous *core.ExecutionError) { + if out == nil || previous == nil { + return + } + out.ErrorUri = previous.GetErrorUri() + out.Timestamp = previous.GetTimestamp() + out.Worker = previous.GetWorker() + out.Recoverability = previous.GetRecoverability() +} + +func cloneExecutionError(err *core.ExecutionError) *core.ExecutionError { + if err == nil { + return &core.ExecutionError{} + } + return proto.Clone(err).(*core.ExecutionError) +} + +// prependSentence puts the driver's own account of the fault in front of whatever the +// plugin had to say about the failure. +func prependSentence(f Fault, a Attribution, message string) string { + sentence := Sentence(f, a) + if message == "" { + return sentence + } + return sentence + " " + message +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go new file mode 100644 index 0000000000..0d6aa8c59d --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go @@ -0,0 +1,287 @@ +package gpufault + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/timestamppb" + + pluginsCore "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/core" + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" +) + +func gpuFault(code int, severity Severity) *core.GpuFault { + return ToProto( + Fault{Kind: KindXid, Code: code, Name: NameFor(KindXid, code), Severity: severity, PCI: "0000:3b:00.0"}, + Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 0}, + ) +} + +func sxidFault(code int) *core.GpuFault { + return ToProto( + Fault{Kind: KindSXid, Code: code, Name: NameFor(KindSXid, code), Severity: SeverityCritical, PCI: "0000:05:00.0"}, + Attribution{NodeName: "ip-10-0-0-1", GPUIndex: UnknownGPUIndex}, + ) +} + +func TestCodeFor(t *testing.T) { + tests := []struct { + name string + fault Fault + want string + }{ + {name: "fallen off the bus", fault: Fault{Kind: KindXid, Code: 79}, want: CodeGpuFallenOffBus}, + {name: "double bit ecc", fault: Fault{Kind: KindXid, Code: 48}, want: CodeGpuEccUncorrectable}, + {name: "contained ecc", fault: Fault{Kind: KindXid, Code: 94}, want: CodeGpuEccUncorrectable}, + {name: "uncontained ecc", fault: Fault{Kind: KindXid, Code: 95}, want: CodeGpuEccUncorrectable}, + {name: "unrecovered ecc", fault: Fault{Kind: KindXid, Code: 140}, want: CodeGpuEccUncorrectable}, + {name: "row remap recorded", fault: Fault{Kind: KindXid, Code: 63}, want: CodeGpuRowRemapPending}, + {name: "row remap failed", fault: Fault{Kind: KindXid, Code: 64}, want: CodeGpuRowRemapPending}, + {name: "nvlink", fault: Fault{Kind: KindXid, Code: 74}, want: CodeGpuNvlinkError}, + {name: "gsp rpc timeout", fault: Fault{Kind: KindXid, Code: 119}, want: CodeGpuGspError}, + {name: "gsp error", fault: Fault{Kind: KindXid, Code: 120}, want: CodeGpuGspError}, + {name: "workload fault", fault: Fault{Kind: KindXid, Code: 31}, want: CodeGpuXidError}, + {name: "unknown code", fault: Fault{Kind: KindXid, Code: 4242}, want: CodeGpuXidError}, + {name: "every sxid is an nvlink error", fault: Fault{Kind: KindSXid, Code: 12028}, want: CodeGpuNvlinkError}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, CodeFor(tt.fault)) + }) + } +} + +func TestIsGenericCode(t *testing.T) { + tests := []struct { + code string + want bool + }{ + {code: "", want: true}, + {code: "Unknown", want: true}, + {code: "UnknownError", want: true}, + {code: "Error", want: true}, + {code: "1", want: true}, + {code: "137", want: true}, + {code: "ExitCode1", want: true}, + {code: "exit-code-137", want: true}, + {code: "OOMKilled", want: false}, + {code: "Interrupted", want: false}, + {code: "PrimaryContainerNotFound", want: false}, + } + + for _, tt := range tests { + t.Run(tt.code, func(t *testing.T) { + assert.Equal(t, tt.want, isGenericCode(tt.code)) + }) + } +} + +func TestClassifyFailureLeavesNonFailuresAlone(t *testing.T) { + tests := []struct { + name string + phase pluginsCore.PhaseInfo + }{ + {name: "success", phase: pluginsCore.PhaseInfoSuccess(nil)}, + {name: "running", phase: pluginsCore.PhaseInfoRunning(1, nil)}, + {name: "aborted", phase: pluginsCore.PhaseInfoAborted(time.Now(), 1, "user aborted")}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyFailure(tt.phase, []*core.GpuFault{gpuFault(79, SeverityCritical)}) + assert.Equal(t, tt.phase.Phase(), got.Phase()) + assert.Nil(t, got.Err()) + }) + } +} + +func TestClassifyFailureWithoutFaults(t *testing.T) { + phase := pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "Pod failed", nil) + + got := ClassifyFailure(phase, nil) + + assert.Equal(t, "OOMKilled", got.Err().GetCode()) + assert.Equal(t, "Pod failed", got.Err().GetMessage()) + assert.Nil(t, got.Err().GetGpuFault()) +} + +func TestClassifyFailureCritical(t *testing.T) { + tests := []struct { + name string + phase pluginsCore.PhaseInfo + faults []*core.GpuFault + wantCode string + wantFaultCode uint32 + }{ + { + name: "user retryable failure becomes a system one", + phase: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + faults: []*core.GpuFault{gpuFault(79, SeverityCritical)}, + wantCode: CodeGpuFallenOffBus, + wantFaultCode: 79, + }, + { + name: "permanent failure becomes system retryable", + phase: pluginsCore.PhaseInfoFailure("Error", "Pod failed", nil), + faults: []*core.GpuFault{gpuFault(48, SeverityCritical)}, + wantCode: CodeGpuEccUncorrectable, + wantFaultCode: 48, + }, + { + name: "an nvswitch fault is critical too", + phase: pluginsCore.PhaseInfoRetryableFailure("Error", "Pod failed", nil), + faults: []*core.GpuFault{sxidFault(12028)}, + wantCode: CodeGpuNvlinkError, + wantFaultCode: 12028, + }, + { + name: "a critical fault outranks an earlier user one", + phase: pluginsCore.PhaseInfoRetryableFailure("Error", "Pod failed", nil), + faults: []*core.GpuFault{gpuFault(31, SeverityUser), gpuFault(63, SeverityCritical)}, + wantCode: CodeGpuRowRemapPending, + wantFaultCode: 63, + }, + { + name: "the first critical fault wins", + phase: pluginsCore.PhaseInfoRetryableFailure("Error", "Pod failed", nil), + faults: []*core.GpuFault{gpuFault(74, SeverityCritical), gpuFault(79, SeverityCritical)}, + wantCode: CodeGpuNvlinkError, + wantFaultCode: 74, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := ClassifyFailure(tt.phase, tt.faults) + + assert.Equal(t, pluginsCore.PhaseRetryableFailure, got.Phase()) + require.NotNil(t, got.Err()) + assert.Equal(t, tt.wantCode, got.Err().GetCode()) + assert.Equal(t, core.ExecutionError_SYSTEM, got.Err().GetKind()) + assert.Equal(t, tt.wantFaultCode, got.Err().GetGpuFault().GetCode()) + assert.Contains(t, got.Err().GetMessage(), MessagePrefix) + assert.True(t, len(got.Err().GetMessage()) > len("Pod failed")) + }) + } +} + +func TestClassifyFailureCriticalMessageAndCarriedFields(t *testing.T) { + timestamp := timestamppb.New(time.Unix(1700000000, 0)) + phase := pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed. No message received from kubernetes.", nil) + phase.Err().ErrorUri = "s3://bucket/error.pb" + phase.Err().Timestamp = timestamp + phase.Err().Worker = "worker-1" + phase.Err().Recoverability = core.ContainerError_RECOVERABLE + phase = phase.WithVersion(4) + phase.WithReason("pod event") + + fault := gpuFault(79, SeverityCritical) + got := ClassifyFailure(phase, []*core.GpuFault{fault}) + + assert.Equal(t, + "[gpu-health] [CRITICAL] Xid 79 (GPU has fallen off the bus) on GPU 0 "+testUUID+ + ". Pod failed. No message received from kubernetes.", + got.Err().GetMessage()) + assert.Equal(t, "s3://bucket/error.pb", got.Err().GetErrorUri()) + assert.Equal(t, timestamp, got.Err().GetTimestamp()) + assert.Equal(t, "worker-1", got.Err().GetWorker()) + assert.Equal(t, core.ContainerError_RECOVERABLE, got.Err().GetRecoverability()) + assert.Equal(t, uint32(4), got.Version()) + assert.Equal(t, "pod event", got.Reason()) + assert.Equal(t, fault, got.Err().GetGpuFault()) + assert.NotNil(t, got.Info()) +} + +func TestClassifyFailureUser(t *testing.T) { + tests := []struct { + name string + phase pluginsCore.PhaseInfo + wantPhase pluginsCore.Phase + wantCode string + wantErrKind core.ExecutionError_ErrorKind + }{ + { + name: "a generic code is replaced by the fault", + phase: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "exit code 1", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: CodeGpuXidError, + wantErrKind: core.ExecutionError_USER, + }, + { + name: "an exit status reported as a code is replaced too", + phase: pluginsCore.PhaseInfoRetryableFailure("137", "exit code 137", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: CodeGpuXidError, + wantErrKind: core.ExecutionError_USER, + }, + { + name: "a specific code is kept", + phase: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "container was oom killed", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantErrKind: core.ExecutionError_USER, + }, + { + name: "a permanent failure is never downgraded to retryable", + phase: pluginsCore.PhaseInfoFailure("UnknownError", "exit code 1", nil), + wantPhase: pluginsCore.PhasePermanentFailure, + wantCode: CodeGpuXidError, + wantErrKind: core.ExecutionError_USER, + }, + { + name: "a system failure keeps its kind", + phase: pluginsCore.PhaseInfoSystemRetryableFailure("Interrupted", "node shut down", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "Interrupted", + wantErrKind: core.ExecutionError_SYSTEM, + }, + } + + fault := gpuFault(31, SeverityUser) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + originalMessage := tt.phase.Err().GetMessage() + + got := ClassifyFailure(tt.phase, []*core.GpuFault{gpuFault(92, SeverityWarn), fault}) + + assert.Equal(t, tt.wantPhase, got.Phase()) + assert.Equal(t, tt.wantCode, got.Err().GetCode()) + assert.Equal(t, tt.wantErrKind, got.Err().GetKind()) + assert.Equal(t, fault, got.Err().GetGpuFault()) + assert.Equal(t, + "[gpu-health] [USER] Xid 31 (GPU memory page fault) on GPU 0 "+testUUID+". "+originalMessage, + got.Err().GetMessage()) + }) + } +} + +func TestClassifyFailureWarnOnly(t *testing.T) { + phase := pluginsCore.PhaseInfoFailure("OOMKilled", "container was oom killed", nil) + phase = phase.WithVersion(2) + + first := gpuFault(92, SeverityWarn) + got := ClassifyFailure(phase, []*core.GpuFault{first, gpuFault(4242, SeverityWarn)}) + + assert.Equal(t, pluginsCore.PhasePermanentFailure, got.Phase()) + assert.Equal(t, "OOMKilled", got.Err().GetCode()) + assert.Equal(t, core.ExecutionError_USER, got.Err().GetKind()) + assert.Equal(t, "container was oom killed", got.Err().GetMessage()) + assert.Equal(t, first, got.Err().GetGpuFault()) + assert.Equal(t, uint32(2), got.Version()) +} + +// Classification must not write through to the PhaseInfo it was handed: the caller +// keeps using the original when nothing about it should change. +func TestClassifyFailureDoesNotMutateInput(t *testing.T) { + phase := pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil) + + ClassifyFailure(phase, []*core.GpuFault{gpuFault(79, SeverityCritical)}) + + assert.Equal(t, "UnknownError", phase.Err().GetCode()) + assert.Equal(t, "Pod failed", phase.Err().GetMessage()) + assert.Nil(t, phase.Err().GetGpuFault()) +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go new file mode 100644 index 0000000000..8da2fc1e62 --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go @@ -0,0 +1,38 @@ +// Package gpufault is the one definition of how a GPU fault travels from the node it +// happened on to the user who has to act on it. +// +// The contract is a Kubernetes Event message. A GPU health daemon on every GPU node +// reads the kernel log, recognizes NVIDIA Xid lines (a fault on a GPU) and NVSwitch +// SXid lines (a fault on the fabric), works out which pod held the device, and records +// a Warning Event against that pod with reason GPUXidError or GPUSXidError. The +// message it writes is FormatEventMessage: a sentence a human can read followed by a +// k=v tail a program reads back with ParseEventMessage. Events already recorded on +// running clusters have to keep parsing, so the format is fixed and every producer and +// consumer renders it through this package instead of writing its own. +// +// The consumer in this repository is the Kubernetes plugin manager in the executor. +// The event watcher already forwards every event recorded on a task's pod into the +// attempt's cluster events; when an attempt ends in failure the plugin manager reads +// the events back, turns each GPU fault message into a core.GpuFault with +// FromEventMessage, and hands the list to ClassifyFailure along with the failure the +// plugin reported. The fault ends up on ExecutionError.gpu_fault, which reaches the +// console and the SDK as typed data, so nobody downstream parses the message text. +// +// ClassifyFailure only ever looks at a failed attempt, and only when at least one +// fault was recorded. What it does depends on the worst severity it finds. +// +// A critical fault, such as Xid 79 (the GPU fell off the bus) or any SXid, turns the +// failure into a system retryable failure with the code CodeFor gives for that fault. +// The device or the node is not trustworthy at that point and the workload did not +// cause it, so the failure must not consume one of the user's retries, and the retry +// wants to land on different hardware. +// +// A user fault, such as Xid 31 (a GPU memory page fault from an out-of-bounds access), +// leaves the plugin's verdict alone: same phase, same error kind, same retry budget. +// It only names the failure, replacing a generic code such as UnknownError or a bare +// exit status with CodeGpuXidError and putting the driver's sentence in front of the +// message. +// +// A warning-only fault changes nothing at all beyond attaching the fault, so that the +// console can show what the GPU reported while the task was running. +package gpufault diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/fault.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/fault.go new file mode 100644 index 0000000000..61e28f678c --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/fault.go @@ -0,0 +1,82 @@ +package gpufault + +// UnknownGPUIndex marks an Attribution whose GPU index could not be determined, +// which is not the same as index 0. +const UnknownGPUIndex = -1 + +// Kind separates GPU faults (Xid) from NVSwitch faults (SXid). The two share a +// numbering space but not a meaning, so a code alone is never enough to identify a +// fault. +type Kind string + +const ( + KindXid Kind = "xid" + KindSXid Kind = "sxid" +) + +// Severity is the operational reading of a fault code: whether the workload caused +// it, whether it is a warning worth surfacing, or whether the GPU is in trouble. +type Severity string + +const ( + // SeverityUser marks faults a workload causes (illegal address, bad push buffer) + // and that a healthy GPU recovers from once the offending process exits. + SeverityUser Severity = "user" + // SeverityWarn marks faults worth surfacing that do not by themselves condemn + // the GPU. + SeverityWarn Severity = "warn" + // SeverityCritical marks faults after which the GPU is generally unusable until + // it is reset or replaced. + SeverityCritical Severity = "critical" +) + +// Label is the upper-case form used in the human half of the event message. +func (s Severity) Label() string { + switch s { + case SeverityUser: + return "USER" + case SeverityCritical: + return "CRITICAL" + case SeverityWarn: + return "WARN" + default: + return "WARN" + } +} + +// Fault is one GPU or NVSwitch fault as it travels through the event message. +// +// It deliberately carries only what the message carries. The emitter parses more out +// of the kernel line (the fault address, the channel id, the raw text, the time the +// line was read) and keeps it in its own log, because folding that variable text into +// the message would defeat the event recorder's correlator: repeats of the same fault +// on the same GPU render byte for byte the same message and are aggregated into a +// single Event with a count instead of flooding the API server. +type Fault struct { + Kind Kind + Code int + Name string + Severity Severity + // PCI is the normalized sysfs bus id of the GPU or NVSwitch, always with a + // function suffix, for example 0000:3b:00.0. + PCI string + // PID is the host pid the driver blamed, or 0 when the line carried none. + PID int + // Process is the process name the driver blamed, empty when unknown. + Process string +} + +// IsSXid reports whether the fault came from an NVSwitch rather than a GPU. +func (f Fault) IsSXid() bool { return f.Kind == KindSXid } + +// Attribution is where the fault happened, as far as the emitter could resolve it. +// The pods the emitter blamed are not part of it: the fault is reported as an event +// on the pod itself, so the reader already knows which pod it is looking at. +type Attribution struct { + NodeName string + // GPUUUID is the driver's UUID for the faulting GPU, empty when the PCI id + // could not be resolved. + GPUUUID string + // GPUIndex is the host device index, UnknownGPUIndex when unresolved. + GPUIndex int +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go new file mode 100644 index 0000000000..9e390296eb --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go @@ -0,0 +1,181 @@ +package gpufault + +import ( + "fmt" + "strconv" + "strings" +) + +// MessagePrefix marks every message the GPU health emitter writes. Consumers filter +// on it, so it must never change. +const MessagePrefix = "[gpu-health]" + +// The event message is two halves. The first is a sentence a human reads in the run's +// Logs tab without knowing anything about Xid codes. The second is a k=v tail a +// program reads back with ParseEventMessage, which is how the typed core.GpuFault is +// produced without re-parsing kernel text. +const ( + keyXid = "xid" + keySXid = "sxid" + keySeverity = "severity" + keyGPUUUID = "gpu_uuid" + keyGPUIndex = "gpu_index" + keyPCI = "pci" + keyNode = "node" + keyPID = "pid" + keyProcess = "process" +) + +// FormatEventMessage renders the message body of the Kubernetes Event for a fault. +func FormatEventMessage(f Fault, a Attribution) string { + return Sentence(f, a) + machineTail(f, a) +} + +// Sentence is the human half of the event message: everything up to and including the +// full stop, with no k=v tail. Failure classification prepends it to the failure +// message so the user reads what the driver reported instead of a bare exit code. +func Sentence(f Fault, a Attribution) string { + var sb strings.Builder + + sb.WriteString(MessagePrefix) + sb.WriteString(" [") + sb.WriteString(f.Severity.Label()) + sb.WriteString("] ") + + if f.IsSXid() { + fmt.Fprintf(&sb, "SXid %d on NVSwitch %s.", f.Code, f.PCI) + } else { + fmt.Fprintf(&sb, "Xid %d (%s)%s.", f.Code, f.Name, gpuPhrase(f, a)) + } + + return sb.String() +} + +// machineTail is the k=v half, leading space included so it appends straight onto the +// sentence. +func machineTail(f Fault, a Attribution) string { + var sb strings.Builder + + codeKey := keyXid + if f.IsSXid() { + codeKey = keySXid + } + + fmt.Fprintf(&sb, " %s=%d", codeKey, f.Code) + fmt.Fprintf(&sb, " %s=%s", keySeverity, string(f.Severity)) + if a.GPUUUID != "" { + fmt.Fprintf(&sb, " %s=%s", keyGPUUUID, a.GPUUUID) + } + if a.GPUIndex >= 0 { + fmt.Fprintf(&sb, " %s=%d", keyGPUIndex, a.GPUIndex) + } + if f.PCI != "" { + fmt.Fprintf(&sb, " %s=%s", keyPCI, f.PCI) + } + if a.NodeName != "" { + fmt.Fprintf(&sb, " %s=%s", keyNode, a.NodeName) + } + if f.PID > 0 { + fmt.Fprintf(&sb, " %s=%d", keyPID, f.PID) + } + if f.Process != "" { + fmt.Fprintf(&sb, " %s=%s", keyProcess, sanitizeValue(f.Process)) + } + + return sb.String() +} + +// gpuPhrase names the GPU as precisely as attribution allowed, degrading to the bus +// id when the driver's procfs entry could not be read. +func gpuPhrase(f Fault, a Attribution) string { + switch { + case a.GPUUUID != "" && a.GPUIndex >= 0: + return fmt.Sprintf(" on GPU %d %s", a.GPUIndex, a.GPUUUID) + case a.GPUUUID != "": + return fmt.Sprintf(" on GPU %s", a.GPUUUID) + case a.GPUIndex >= 0: + return fmt.Sprintf(" on GPU %d", a.GPUIndex) + case f.PCI != "": + return fmt.Sprintf(" on GPU at PCI %s", f.PCI) + default: + return "" + } +} + +// ParseEventMessage reads back a message produced by FormatEventMessage. +// +// It recovers everything the message carries: kind, code, name, severity, bus id, pid +// and process, plus the node, GPU UUID and GPU index. It reports false for any message +// that is not one of ours, which is how consumers tell a GPU fault event apart from +// every other event recorded on the same pod. +func ParseEventMessage(msg string) (Fault, Attribution, bool) { + if !strings.HasPrefix(msg, MessagePrefix) { + return Fault{}, Attribution{}, false + } + + kind := KindXid + start := strings.Index(msg, " "+keyXid+"=") + if start < 0 { + if start = strings.Index(msg, " "+keySXid+"="); start < 0 { + return Fault{}, Attribution{}, false + } + kind = KindSXid + } + + fields := map[string]string{} + for _, token := range strings.Fields(msg[start:]) { + key, value, ok := strings.Cut(token, "=") + if !ok { + continue + } + fields[key] = value + } + + codeKey := keyXid + if kind == KindSXid { + codeKey = keySXid + } + code, err := strconv.Atoi(fields[codeKey]) + if err != nil { + return Fault{}, Attribution{}, false + } + + fault := Fault{ + Kind: kind, + Code: code, + Name: NameFor(kind, code), + Severity: SeverityFor(kind, code), + PCI: fields[keyPCI], + Process: fields[keyProcess], + } + if sev, ok := fields[keySeverity]; ok { + fault.Severity = ParseSeverity(sev) + } + if pid, err := strconv.Atoi(fields[keyPID]); err == nil { + fault.PID = pid + } + + attribution := Attribution{ + NodeName: fields[keyNode], + GPUUUID: fields[keyGPUUUID], + GPUIndex: UnknownGPUIndex, + } + if index, err := strconv.Atoi(fields[keyGPUIndex]); err == nil { + attribution.GPUIndex = index + } + + return fault, attribution, true +} + +// sanitizeValue keeps a k=v token parseable. Process names come from the kernel's +// comm field, which is almost always a single word but is not guaranteed to be. +func sanitizeValue(v string) string { + return strings.Map(func(r rune) rune { + switch r { + case ' ', '\t', '\n', '\r', '=': + return '_' + default: + return r + } + }, v) +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go new file mode 100644 index 0000000000..349afe85cf --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go @@ -0,0 +1,188 @@ +package gpufault + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testUUID = "GPU-6f3c1234-5678-90ab-cdef-1234567890ab" + +func TestFormatEventMessage(t *testing.T) { + tests := []struct { + name string + fault Fault + attribution Attribution + want string + }{ + { + name: "critical fault with a process", + fault: Fault{ + Kind: KindXid, Code: 79, Name: NameFor(KindXid, 79), Severity: SeverityCritical, + PCI: "0000:3b:00.0", PID: 1234, Process: "python", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 0}, + want: "[gpu-health] [CRITICAL] Xid 79 (GPU has fallen off the bus) on GPU 0 " + testUUID + + ". xid=79 severity=critical gpu_uuid=" + testUUID + " gpu_index=0 pci=0000:3b:00.0 node=ip-10-0-0-1 pid=1234 process=python", + }, + { + name: "user fault without a process", + fault: Fault{ + Kind: KindXid, Code: 13, Name: NameFor(KindXid, 13), Severity: SeverityUser, + PCI: "0000:3b:00.0", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 3}, + want: "[gpu-health] [USER] Xid 13 (Graphics Engine Exception) on GPU 3 " + testUUID + + ". xid=13 severity=user gpu_uuid=" + testUUID + " gpu_index=3 pci=0000:3b:00.0 node=ip-10-0-0-1", + }, + { + name: "gpu could not be resolved", + fault: Fault{ + Kind: KindXid, Code: 48, Name: NameFor(KindXid, 48), Severity: SeverityCritical, + PCI: "0000:af:00.0", + }, + attribution: Attribution{NodeName: "gke-node-1", GPUIndex: UnknownGPUIndex}, + want: "[gpu-health] [CRITICAL] Xid 48 (Double Bit ECC Error) on GPU at PCI 0000:af:00.0." + + " xid=48 severity=critical pci=0000:af:00.0 node=gke-node-1", + }, + { + name: "nvswitch fault", + fault: Fault{ + Kind: KindSXid, Code: 12028, Name: NameFor(KindSXid, 12028), Severity: SeverityCritical, + PCI: "0000:05:00.0", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUIndex: UnknownGPUIndex}, + want: "[gpu-health] [CRITICAL] SXid 12028 on NVSwitch 0000:05:00.0." + + " sxid=12028 severity=critical pci=0000:05:00.0 node=ip-10-0-0-1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, FormatEventMessage(tt.fault, tt.attribution)) + }) + } +} + +// Sentence is the message up to the full stop. Classification prepends it to failure +// messages, so it must never drag the k=v tail along with it. +func TestSentence(t *testing.T) { + tests := []struct { + name string + fault Fault + attribution Attribution + want string + }{ + { + name: "xid", + fault: Fault{ + Kind: KindXid, Code: 79, Name: NameFor(KindXid, 79), Severity: SeverityCritical, + PCI: "0000:3b:00.0", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 0}, + want: "[gpu-health] [CRITICAL] Xid 79 (GPU has fallen off the bus) on GPU 0 " + testUUID + ".", + }, + { + name: "sxid", + fault: Fault{ + Kind: KindSXid, Code: 12028, Name: NameFor(KindSXid, 12028), Severity: SeverityCritical, + PCI: "0000:05:00.0", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUIndex: UnknownGPUIndex}, + want: "[gpu-health] [CRITICAL] SXid 12028 on NVSwitch 0000:05:00.0.", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sentence := Sentence(tt.fault, tt.attribution) + assert.Equal(t, tt.want, sentence) + assert.True(t, len(FormatEventMessage(tt.fault, tt.attribution)) > len(sentence)) + assert.NotContains(t, sentence, "=") + }) + } +} + +func TestEventMessageRoundTrip(t *testing.T) { + tests := []struct { + name string + fault Fault + attribution Attribution + }{ + { + name: "xid with process", + fault: Fault{ + Kind: KindXid, Code: 79, Name: NameFor(KindXid, 79), Severity: SeverityCritical, + PCI: "0000:3b:00.0", PID: 1234, Process: "python", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 0}, + }, + { + name: "xid without process or gpu", + fault: Fault{ + Kind: KindXid, Code: 92, Name: NameFor(KindXid, 92), Severity: SeverityWarn, + PCI: "0000:af:00.0", + }, + attribution: Attribution{NodeName: "gke-node-1", GPUIndex: UnknownGPUIndex}, + }, + { + name: "unknown code keeps its generic name", + fault: Fault{ + Kind: KindXid, Code: 4242, Name: NameFor(KindXid, 4242), Severity: SeverityWarn, + PCI: "0000:af:00.0", + }, + attribution: Attribution{NodeName: "gke-node-1", GPUIndex: UnknownGPUIndex}, + }, + { + name: "sxid", + fault: Fault{ + Kind: KindSXid, Code: 24001, Name: NameFor(KindSXid, 24001), Severity: SeverityCritical, + PCI: "0000:0e:00.0", + }, + attribution: Attribution{NodeName: "ip-10-0-0-1", GPUIndex: UnknownGPUIndex}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + message := FormatEventMessage(tt.fault, tt.attribution) + + fault, attribution, ok := ParseEventMessage(message) + require.True(t, ok, "expected %q to parse", message) + + assert.Equal(t, tt.fault, fault) + assert.Equal(t, tt.attribution, attribution) + }) + } +} + +func TestParseEventMessageRejects(t *testing.T) { + messages := []string{ + "", + "BackOff restarting failed container", + "[gpu-health] something without a machine tail", + "[gpu-health] [WARN] Xid ?? xid=notanumber severity=warn", + } + + for _, message := range messages { + t.Run(message, func(t *testing.T) { + _, _, ok := ParseEventMessage(message) + assert.False(t, ok) + }) + } +} + +func TestFormatEventMessageSanitizesProcessName(t *testing.T) { + fault := Fault{ + Kind: KindXid, Code: 13, Name: NameFor(KindXid, 13), Severity: SeverityUser, + PCI: "0000:3b:00.0", PID: 7, Process: "my train job", + } + + message := FormatEventMessage(fault, Attribution{NodeName: "n1", GPUIndex: UnknownGPUIndex}) + assert.Contains(t, message, "process=my_train_job") + + parsed, _, ok := ParseEventMessage(message) + require.True(t, ok) + assert.Equal(t, "my_train_job", parsed.Process) +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go new file mode 100644 index 0000000000..4fe79919af --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go @@ -0,0 +1,116 @@ +package gpufault + +import ( + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" +) + +// ToProto renders a fault and where it happened as the IDL message that travels on +// ClusterEvent and ExecutionError. +func ToProto(f Fault, a Attribution) *core.GpuFault { + out := &core.GpuFault{ + Kind: kindToProto(f.Kind), + Code: uint32(max(f.Code, 0)), + Name: f.Name, + Severity: severityToProto(f.Severity), + GpuUuid: a.GPUUUID, + PciBusId: f.PCI, + Node: a.NodeName, + Pid: uint32(max(f.PID, 0)), + Process: f.Process, + } + if a.GPUIndex >= 0 { + index := uint32(a.GPUIndex) + out.GpuIndex = &index + } + return out +} + +// FromProto is the inverse of ToProto. An unset severity or kind is resolved from the +// code, so a producer that only filled in the numbers still yields a usable fault. +func FromProto(p *core.GpuFault) (Fault, Attribution) { + if p == nil { + return Fault{}, Attribution{GPUIndex: UnknownGPUIndex} + } + + kind := kindFromProto(p.GetKind()) + code := int(p.GetCode()) + + fault := Fault{ + Kind: kind, + Code: code, + Name: p.GetName(), + Severity: severityFromProto(p.GetSeverity()), + PCI: p.GetPciBusId(), + PID: int(p.GetPid()), + Process: p.GetProcess(), + } + if fault.Name == "" { + fault.Name = NameFor(kind, code) + } + if fault.Severity == "" { + fault.Severity = SeverityFor(kind, code) + } + + attribution := Attribution{ + NodeName: p.GetNode(), + GPUUUID: p.GetGpuUuid(), + GPUIndex: UnknownGPUIndex, + } + if p.GpuIndex != nil { + attribution.GPUIndex = int(p.GetGpuIndex()) + } + + return fault, attribution +} + +// FromEventMessage turns the message body of a Kubernetes Event into a typed fault, +// returning nil for every event that is not one of the emitter's. +func FromEventMessage(msg string) *core.GpuFault { + fault, attribution, ok := ParseEventMessage(msg) + if !ok { + return nil + } + return ToProto(fault, attribution) +} + +func kindToProto(k Kind) core.GpuFault_Kind { + if k == KindSXid { + return core.GpuFault_KIND_SXID + } + return core.GpuFault_KIND_XID +} + +func kindFromProto(k core.GpuFault_Kind) Kind { + if k == core.GpuFault_KIND_SXID { + return KindSXid + } + return KindXid +} + +func severityToProto(s Severity) core.GpuFault_Severity { + switch s { + case SeverityUser: + return core.GpuFault_SEVERITY_USER + case SeverityCritical: + return core.GpuFault_SEVERITY_CRITICAL + case SeverityWarn: + return core.GpuFault_SEVERITY_WARN + default: + return core.GpuFault_SEVERITY_UNSPECIFIED + } +} + +// severityFromProto returns the empty Severity for an unset value so that callers can +// fall back to the code table instead of silently reading it as a warning. +func severityFromProto(s core.GpuFault_Severity) Severity { + switch s { + case core.GpuFault_SEVERITY_USER: + return SeverityUser + case core.GpuFault_SEVERITY_WARN: + return SeverityWarn + case core.GpuFault_SEVERITY_CRITICAL: + return SeverityCritical + default: + return "" + } +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go new file mode 100644 index 0000000000..75b27940ee --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go @@ -0,0 +1,109 @@ +package gpufault + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" +) + +func TestToProto(t *testing.T) { + fault := Fault{ + Kind: KindXid, Code: 79, Name: NameFor(KindXid, 79), Severity: SeverityCritical, + PCI: "0000:3b:00.0", PID: 1234, Process: "python", + } + attribution := Attribution{NodeName: "ip-10-0-0-1", GPUUUID: testUUID, GPUIndex: 0} + + got := ToProto(fault, attribution) + + assert.Equal(t, core.GpuFault_KIND_XID, got.GetKind()) + assert.Equal(t, uint32(79), got.GetCode()) + assert.Equal(t, "GPU has fallen off the bus", got.GetName()) + assert.Equal(t, core.GpuFault_SEVERITY_CRITICAL, got.GetSeverity()) + assert.Equal(t, testUUID, got.GetGpuUuid()) + require.NotNil(t, got.GpuIndex, "index 0 must be distinguishable from an unknown index") + assert.Equal(t, uint32(0), got.GetGpuIndex()) + assert.Equal(t, "0000:3b:00.0", got.GetPciBusId()) + assert.Equal(t, "ip-10-0-0-1", got.GetNode()) + assert.Equal(t, uint32(1234), got.GetPid()) + assert.Equal(t, "python", got.GetProcess()) +} + +func TestProtoRoundTrip(t *testing.T) { + tests := []struct { + name string + fault Fault + attribution Attribution + }{ + { + name: "xid with a resolved gpu", + fault: Fault{ + Kind: KindXid, Code: 31, Name: NameFor(KindXid, 31), Severity: SeverityUser, + PCI: "0000:3b:00.0", PID: 12, Process: "python", + }, + attribution: Attribution{NodeName: "n1", GPUUUID: testUUID, GPUIndex: 2}, + }, + { + name: "sxid with an unknown gpu index", + fault: Fault{ + Kind: KindSXid, Code: 12028, Name: NameFor(KindSXid, 12028), Severity: SeverityCritical, + PCI: "0000:05:00.0", + }, + attribution: Attribution{NodeName: "n1", GPUIndex: UnknownGPUIndex}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fault, attribution := FromProto(ToProto(tt.fault, tt.attribution)) + assert.Equal(t, tt.fault, fault) + assert.Equal(t, tt.attribution, attribution) + }) + } +} + +// A producer that only filled in the numbers still yields a usable fault. +func TestFromProtoFillsInFromTheCode(t *testing.T) { + fault, attribution := FromProto(&core.GpuFault{Code: 79}) + + assert.Equal(t, KindXid, fault.Kind) + assert.Equal(t, "GPU has fallen off the bus", fault.Name) + assert.Equal(t, SeverityCritical, fault.Severity) + assert.Equal(t, UnknownGPUIndex, attribution.GPUIndex) +} + +func TestFromProtoNil(t *testing.T) { + fault, attribution := FromProto(nil) + assert.Equal(t, Fault{}, fault) + assert.Equal(t, Attribution{GPUIndex: UnknownGPUIndex}, attribution) +} + +func TestFromEventMessage(t *testing.T) { + fault := Fault{ + Kind: KindXid, Code: 63, Name: NameFor(KindXid, 63), Severity: SeverityCritical, + PCI: "0000:3b:00.0", + } + attribution := Attribution{NodeName: "n1", GPUUUID: testUUID, GPUIndex: 1} + + got := FromEventMessage(FormatEventMessage(fault, attribution)) + require.NotNil(t, got) + assert.Equal(t, uint32(63), got.GetCode()) + assert.Equal(t, core.GpuFault_SEVERITY_CRITICAL, got.GetSeverity()) + assert.Equal(t, testUUID, got.GetGpuUuid()) +} + +func TestFromEventMessageIgnoresOtherEvents(t *testing.T) { + messages := []string{ + "", + "Back-off restarting failed container", + "Successfully assigned flytesnacks-development/pod to ip-10-0-0-1", + } + + for _, message := range messages { + t.Run(message, func(t *testing.T) { + assert.Nil(t, FromEventMessage(message)) + }) + } +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go new file mode 100644 index 0000000000..e2b53c982a --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go @@ -0,0 +1,106 @@ +package gpufault + +import "fmt" + +// xidNames is the human name NVIDIA documents for each Xid code. It does not need to +// be exhaustive: unknown codes fall back to "Xid " so a new driver release never +// makes a fault get dropped on the floor. +var xidNames = map[int]string{ + 13: "Graphics Engine Exception", + 31: "GPU memory page fault", + 32: "Invalid or corrupted push buffer stream", + 38: "Driver firmware error", + 43: "GPU stopped processing", + 45: "Preemptive cleanup, due to previous errors", + 48: "Double Bit ECC Error", + 61: "Internal micro-controller breakpoint/warning", + 62: "Internal micro-controller halt", + 63: "ECC page retirement or row remapping recording event", + 64: "ECC page retirement or row remapper recording failure", + 68: "NVDEC0 Exception", + 69: "Graphics Engine class error", + 74: "NVLink Error", + 79: "GPU has fallen off the bus", + 92: "High single-bit ECC error rate", + 94: "Contained ECC error", + 95: "Uncontained ECC error", + 109: "Context Switch Timeout Error", + 119: "GSP RPC Timeout", + 120: "GSP Error", + 140: "Unrecovered ECC Error", + 154: "GPU recovery action changed", +} + +// userXids are the codes a workload causes rather than the hardware. They still +// interrupt the run, but the GPU itself is fine once the process is gone. +var userXids = map[int]bool{ + 13: true, + 31: true, + 43: true, + 45: true, +} + +// criticalXids are the codes after which the GPU is generally unusable until it is +// reset or replaced. +var criticalXids = map[int]bool{ + 48: true, + 63: true, + 64: true, + 74: true, + 79: true, + 94: true, + 95: true, + 119: true, + 120: true, +} + +// warnXids are codes that are neither the workload's fault nor immediately fatal. +// Anything not listed anywhere lands here too. +var warnXids = map[int]bool{ + 92: true, +} + +// NameFor returns the documented name for a fault code, or a generic name when the +// code is not in the table. +func NameFor(kind Kind, code int) string { + if kind == KindSXid { + return fmt.Sprintf("SXid %d", code) + } + if name, ok := xidNames[code]; ok { + return name + } + return fmt.Sprintf("Xid %d", code) +} + +// SeverityFor classifies a fault code. Every SXid is critical: NVSwitch faults take +// down fabric links shared by every GPU on the node, so there is no benign case. +func SeverityFor(kind Kind, code int) Severity { + if kind == KindSXid { + return SeverityCritical + } + switch { + case userXids[code]: + return SeverityUser + case criticalXids[code]: + return SeverityCritical + case warnXids[code]: + return SeverityWarn + default: + return SeverityWarn + } +} + +// ParseSeverity turns the wire form back into a Severity, defaulting to warn for +// anything unrecognized so a future severity value never fails a parse. +func ParseSeverity(s string) Severity { + switch Severity(s) { + case SeverityUser: + return SeverityUser + case SeverityCritical: + return SeverityCritical + case SeverityWarn: + return SeverityWarn + default: + return SeverityWarn + } +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go new file mode 100644 index 0000000000..51e012aa8d --- /dev/null +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go @@ -0,0 +1,73 @@ +package gpufault + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestNameFor(t *testing.T) { + tests := []struct { + name string + kind Kind + code int + want string + }{ + {name: "documented xid", kind: KindXid, code: 79, want: "GPU has fallen off the bus"}, + {name: "unknown xid falls back", kind: KindXid, code: 4242, want: "Xid 4242"}, + {name: "sxid is always generic", kind: KindSXid, code: 12028, want: "SXid 12028"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, NameFor(tt.kind, tt.code)) + }) + } +} + +func TestSeverityFor(t *testing.T) { + tests := []struct { + name string + kind Kind + code int + want Severity + }{ + {name: "workload fault", kind: KindXid, code: 31, want: SeverityUser}, + {name: "hardware fault", kind: KindXid, code: 79, want: SeverityCritical}, + {name: "listed warning", kind: KindXid, code: 92, want: SeverityWarn}, + {name: "unlisted code is a warning", kind: KindXid, code: 4242, want: SeverityWarn}, + {name: "every sxid is critical", kind: KindSXid, code: 24001, want: SeverityCritical}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, SeverityFor(tt.kind, tt.code)) + }) + } +} + +func TestParseSeverity(t *testing.T) { + tests := []struct { + in string + want Severity + }{ + {in: "user", want: SeverityUser}, + {in: "warn", want: SeverityWarn}, + {in: "critical", want: SeverityCritical}, + {in: "something-new", want: SeverityWarn}, + {in: "", want: SeverityWarn}, + } + + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + assert.Equal(t, tt.want, ParseSeverity(tt.in)) + }) + } +} + +func TestSeverityLabel(t *testing.T) { + assert.Equal(t, "USER", SeverityUser.Label()) + assert.Equal(t, "WARN", SeverityWarn.Label()) + assert.Equal(t, "CRITICAL", SeverityCritical.Label()) + assert.Equal(t, "WARN", Severity("nonsense").Label()) +} From 95e0a84941272702672303ebad12e221629112b5 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 19 Aug 2026 22:03:05 +0530 Subject: [PATCH 02/13] fix(flytek8s): keep the specific code when a pod fails with a system error DemystifyFailure worked out a failure code from the pod status reason and the container states and then threw it away on the system path, reporting every system error as Interrupted. A pod killed by a graceful node shutdown reached the user as Interrupted whether the kubelet had said Shutdown, NodeShutdown, Terminated or NodeAffinity, and that reason is the only record of what happened to the node. The system path now reports the code it worked out. The two places that mean Interrupted still say Interrupted: the SIGKILL branch sets it deliberately, and the branch where the kubelet recorded nothing before the node went away now sets it explicitly instead of relying on the return to overwrite UnknownError. Nothing outside this function compares a failure code to Interrupted. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- .../pluginmachinery/flytek8s/pod_helper.go | 12 ++- .../flytek8s/pod_helper_test.go | 75 ++++++++++--------- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go index 446ac8109a..4228b308cd 100644 --- a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go +++ b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go @@ -1631,14 +1631,22 @@ func DemystifyFailure(ctx context.Context, status v1.PodStatus, info pluginsCore // If the code remains 'UnknownError', it indicates that the kubelet did not have a chance // to record a more specific failure before the node was terminated or preempted. - // In such cases, we classify the error as system-level and accept false positives + // In such cases, we classify the error as system-level and accept false positives. + // The node vanishing is an interruption, and 'UnknownError' says nothing to the user, + // so the code is replaced rather than kept. if code == "UnknownError" { isSystemError = true + code = Interrupted } if isSystemError { logger.Warnf(ctx, "Pod failed with a system error. Code: %s, Message: %s", code, message) - return pluginsCore.PhaseInfoSystemRetryableFailure(Interrupted, message, &info), nil + // Report the code we worked out rather than flattening every system error to + // 'Interrupted'. A retryable status reason such as 'Shutdown' or 'NodeShutdown' + // tells the user what actually happened to the node, and it is the only place + // that information exists by the time the failure reaches them. The SIGKILL and + // vanished-node branches above set 'Interrupted' on purpose and still report it. + return pluginsCore.PhaseInfoSystemRetryableFailure(code, message, &info), nil } logger.Warnf(ctx, "Pod failed with a user error. Code: %s, Message: %s", code, message) diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go index 3203028f8a..bcdecee99c 100644 --- a/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper_test.go @@ -3037,6 +3037,8 @@ func TestDemystifySuccess(t *testing.T) { func TestDemystifyFailure(t *testing.T) { ctx := context.TODO() + // The kubelet recorded nothing before the node went away, so there is no more + // specific code to report than the interruption itself. t.Run("unknown-error", func(t *testing.T) { phaseInfo, err := DemystifyFailure(ctx, v1.PodStatus{}, pluginsCore.TaskInfo{}, "") assert.Nil(t, err) @@ -3112,47 +3114,50 @@ func TestDemystifyFailure(t *testing.T) { assert.Equal(t, core.ExecutionError_SYSTEM, phaseInfo.Err().Kind) }) - t.Run("GKE node preemption", func(t *testing.T) { - for _, reason := range []string{ - "Terminated", - "Shutdown", - "NodeShutdown", - } { - t.Run(reason, func(t *testing.T) { - message := "Test pod status message" - phaseInfo, err := DemystifyFailure(ctx, v1.PodStatus{ - Message: message, - Reason: reason, - // Can't always rely on GCP returining container statuses when node is preempted + // A system error keeps the status reason as its code. The reason is the only record + // of what happened to the node, so flattening every one of them to 'Interrupted' + // would leave the user with nothing to act on. + t.Run("system errors keep their status reason as the code", func(t *testing.T) { + tests := []struct { + name string + status v1.PodStatus + wantCode string + }{ + { + name: "GKE node preemption reported as Terminated", + status: v1.PodStatus{Message: "Test pod status message", Reason: "Terminated", ContainerStatuses: []v1.ContainerStatus{}}, + wantCode: "Terminated", + }, + { + name: "GKE node preemption reported as Shutdown", + status: v1.PodStatus{Message: "Test pod status message", Reason: "Shutdown", ContainerStatuses: []v1.ContainerStatus{}}, + wantCode: "Shutdown", + }, + { + name: "GKE node preemption reported as NodeShutdown", + status: v1.PodStatus{Message: "Test pod status message", Reason: "NodeShutdown", ContainerStatuses: []v1.ContainerStatus{}}, + wantCode: "NodeShutdown", + }, + { + name: "kubelet admission denies the pod due to a missing node label", + status: v1.PodStatus{ + Message: "Pod was rejected: Predicate NodeAffinity failed: node(s) didn't match Pod's node affinity/selector", + Reason: "NodeAffinity", + Phase: v1.PodFailed, ContainerStatuses: []v1.ContainerStatus{}, - }, pluginsCore.TaskInfo{}, "") - assert.Nil(t, err) - assert.Equal(t, pluginsCore.PhaseRetryableFailure, phaseInfo.Phase()) - assert.Equal(t, "Interrupted", phaseInfo.Err().GetCode()) - assert.Equal(t, core.ExecutionError_SYSTEM, phaseInfo.Err().GetKind()) - assert.Equal(t, message, phaseInfo.Err().GetMessage()) - }) + }, + wantCode: "NodeAffinity", + }, } - }) - t.Run("Kubelet admission denies pod due to missing node label", func(t *testing.T) { - for _, reason := range []string{ - "NodeAffinity", - } { - t.Run(reason, func(t *testing.T) { - message := "Pod was rejected: Predicate NodeAffinity failed: node(s) didn't match Pod's node affinity/selector" - phaseInfo, err := DemystifyFailure(ctx, v1.PodStatus{ - Message: message, - Reason: reason, - Phase: v1.PodFailed, - // Can't always rely on GCP returining container statuses when node is preempted - ContainerStatuses: []v1.ContainerStatus{}, - }, pluginsCore.TaskInfo{}, "") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + phaseInfo, err := DemystifyFailure(ctx, tt.status, pluginsCore.TaskInfo{}, "") assert.Nil(t, err) assert.Equal(t, pluginsCore.PhaseRetryableFailure, phaseInfo.Phase()) - assert.Equal(t, "Interrupted", phaseInfo.Err().GetCode()) + assert.Equal(t, tt.wantCode, phaseInfo.Err().GetCode()) assert.Equal(t, core.ExecutionError_SYSTEM, phaseInfo.Err().GetKind()) - assert.Equal(t, message, phaseInfo.Err().GetMessage()) + assert.Equal(t, tt.status.Message, phaseInfo.Err().GetMessage()) }) } }) From dc5c537f614025bdfb63e441bf056e45a952c140 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 19 Aug 2026 22:03:18 +0530 Subject: [PATCH 03/13] feat(executor): classify GPU faults into the failure the user sees Two things had to change for a GPU fault to reach the user. The Kubernetes plugin manager now looks for GPU faults when an attempt ends in a failure on a pod. It reads every event recorded against that pod over the whole attempt rather than only the ones since the event watermark, because the Xid that killed the task is usually recorded rounds before the pod's status catches up with it, turns each one into a core.GpuFault, and hands the list to gpufault.ClassifyFailure. Nothing else in the executor sets ExecutionError.gpu_fault. toActionErrorInfo then carried only the message and the kind onto the action event, so the code the plugin had worked out was dropped on the floor: a task killed for running out of memory reached the console and the SDK with no code at all. It now carries the code and the GPU fault through as well. The CR-persisted ErrorState already round-tripped the code, so that path needed nothing. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- .../pkg/controller/taskaction_controller.go | 9 +- .../controller/taskaction_error_info_test.go | 118 +++++++++++ executor/pkg/plugin/k8s/plugin_manager.go | 50 ++++- .../pkg/plugin/k8s/plugin_manager_test.go | 200 +++++++++++++++++- 4 files changed, 368 insertions(+), 9 deletions(-) create mode 100644 executor/pkg/controller/taskaction_error_info_test.go diff --git a/executor/pkg/controller/taskaction_controller.go b/executor/pkg/controller/taskaction_controller.go index 7fc26fe76d..4c3dfdd9e2 100644 --- a/executor/pkg/controller/taskaction_controller.go +++ b/executor/pkg/controller/taskaction_controller.go @@ -786,9 +786,14 @@ func toActionErrorInfo(err *core.ExecutionError) *workflow.ErrorInfo { if err == nil { return nil } + // Code and GpuFault are carried through as they are. They are the parts of the + // failure the user can act on, and this event is the only place they reach the + // console and the SDK from. out := &workflow.ErrorInfo{ - Message: err.GetMessage(), - Kind: workflow.ErrorInfo_KIND_UNSPECIFIED, + Message: err.GetMessage(), + Kind: workflow.ErrorInfo_KIND_UNSPECIFIED, + Code: err.GetCode(), + GpuFault: err.GetGpuFault(), } switch err.GetKind() { case core.ExecutionError_USER: diff --git a/executor/pkg/controller/taskaction_error_info_test.go b/executor/pkg/controller/taskaction_error_info_test.go new file mode 100644 index 0000000000..f5fe89950e --- /dev/null +++ b/executor/pkg/controller/taskaction_error_info_test.go @@ -0,0 +1,118 @@ +package controller + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/gpufault" + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" + "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/workflow" +) + +func TestToActionErrorInfo(t *testing.T) { + fault := gpufault.ToProto( + gpufault.Fault{ + Kind: gpufault.KindXid, Code: 79, Name: gpufault.NameFor(gpufault.KindXid, 79), + Severity: gpufault.SeverityCritical, PCI: "0000:3b:00.0", + }, + gpufault.Attribution{NodeName: "ip-10-0-0-1", GPUIndex: 0}, + ) + + tests := []struct { + name string + err *core.ExecutionError + want *workflow.ErrorInfo + }{ + { + name: "no error", + err: nil, + want: nil, + }, + { + name: "user error keeps its code", + err: &core.ExecutionError{Code: "OOMKilled", Message: "container was oom killed", Kind: core.ExecutionError_USER}, + want: &workflow.ErrorInfo{Code: "OOMKilled", Message: "container was oom killed", Kind: workflow.ErrorInfo_KIND_USER}, + }, + { + name: "system error keeps its code", + err: &core.ExecutionError{Code: "NodeShutdown", Message: "node is shutting down", Kind: core.ExecutionError_SYSTEM}, + want: &workflow.ErrorInfo{Code: "NodeShutdown", Message: "node is shutting down", Kind: workflow.ErrorInfo_KIND_SYSTEM}, + }, + { + name: "unknown kind and no code", + err: &core.ExecutionError{Message: "something went wrong"}, + want: &workflow.ErrorInfo{Message: "something went wrong", Kind: workflow.ErrorInfo_KIND_UNSPECIFIED}, + }, + { + name: "gpu fault travels with the error", + err: &core.ExecutionError{ + Code: gpufault.CodeGpuFallenOffBus, Message: "the gpu fell off the bus", + Kind: core.ExecutionError_SYSTEM, GpuFault: fault, + }, + want: &workflow.ErrorInfo{ + Code: gpufault.CodeGpuFallenOffBus, Message: "the gpu fell off the bus", + Kind: workflow.ErrorInfo_KIND_SYSTEM, GpuFault: fault, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := toActionErrorInfo(tt.err) + if tt.want == nil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tt.want.GetCode(), got.GetCode()) + assert.Equal(t, tt.want.GetMessage(), got.GetMessage()) + assert.Equal(t, tt.want.GetKind(), got.GetKind()) + assert.Equal(t, tt.want.GetGpuFault(), got.GetGpuFault()) + }) + } +} + +// The CR-persisted ErrorState is the other route a failure takes to the user, when the +// executor reports the action from the stored status rather than from the live phase. +func TestErrorStateFromExecError(t *testing.T) { + tests := []struct { + name string + err *core.ExecutionError + wantCode string + wantKind string + wantMessage string + wantNil bool + }{ + {name: "no error", err: nil, wantNil: true}, + { + name: "user error", + err: &core.ExecutionError{Code: "OOMKilled", Message: "oom", Kind: core.ExecutionError_USER}, + wantCode: "OOMKilled", + wantKind: "USER", + wantMessage: "oom", + }, + { + name: "gpu fault code survives the round trip through the CR", + err: &core.ExecutionError{Code: gpufault.CodeGpuFallenOffBus, Message: "gpu gone", Kind: core.ExecutionError_SYSTEM}, + wantCode: gpufault.CodeGpuFallenOffBus, + wantKind: "SYSTEM", + wantMessage: "gpu gone", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := errorStateFromExecError(tt.err) + if tt.wantNil { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tt.wantCode, got.Code) + assert.Equal(t, tt.wantKind, got.Kind) + assert.Equal(t, tt.wantMessage, got.Message) + }) + } +} diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index 5f8a2f15f2..5ef4424b8e 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -9,6 +9,7 @@ import ( "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/io" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/ioutils" "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" + v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" k8stypes "k8s.io/apimachinery/pkg/types" @@ -18,6 +19,7 @@ import ( "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/errors" pluginsCore "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/core" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/flytek8s/config" + "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/gpufault" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/k8s" pluginsUtils "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/utils" stdErrors "github.com/flyteorg/flyte/v2/flytestdlib/errors" @@ -270,6 +272,7 @@ func (pm *PluginManager) Handle(ctx context.Context, tCtx pluginsCore.TaskExecut lastEventUpdate, lastEventRecordedAt, ) + phaseInfo = pm.classifyGpuFailure(resource, phaseInfo) transition.SetInfo(phaseInfo) } @@ -331,12 +334,7 @@ func (pm *PluginManager) attachRecentObjectEvents( return phaseInfo, lastEventUpdate, lastEventRecordedAt } - objectKey := watchedObjectKey{ - Namespace: resource.GetNamespace(), - Name: resource.GetName(), - Kind: resource.GetObjectKind().GroupVersionKind().Kind, - } - recentEvents := pm.eventWatcher.List(objectKey, lastEventUpdate, lastEventRecordedAt) + recentEvents := pm.eventWatcher.List(objectKeyFor(resource), lastEventUpdate, lastEventRecordedAt) if len(recentEvents) == 0 { return phaseInfo, lastEventUpdate, lastEventRecordedAt } @@ -357,6 +355,46 @@ func (pm *PluginManager) attachRecentObjectEvents( return phaseInfo, lastEventUpdate, lastEventRecordedAt } +func objectKeyFor(resource client.Object) watchedObjectKey { + return watchedObjectKey{ + Namespace: resource.GetNamespace(), + Name: resource.GetName(), + Kind: resource.GetObjectKind().GroupVersionKind().Kind, + } +} + +// classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into +// the failure the plugin reported, so that a fault the node saw becomes the code and +// the message the user reads. Anything that is not a failed pod is left alone. +func (pm *PluginManager) classifyGpuFailure( + resource client.Object, + phaseInfo pluginsCore.PhaseInfo, +) pluginsCore.PhaseInfo { + if pm.eventWatcher == nil || resource == nil || !phaseInfo.Phase().IsFailure() { + return phaseInfo + } + if _, isPod := resource.(*v1.Pod); !isPod { + return phaseInfo + } + + // Every event recorded on the pod, not only the ones since the last watermark. The + // Xid that killed the task is usually recorded rounds before the pod's status catches + // up with it, and by then the watermark has moved past it. + events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{}) + if len(events) == 0 { + return phaseInfo + } + + faults := make([]*core.GpuFault, 0, len(events)) + for _, event := range events { + if fault := gpufault.FromEventMessage(event.Message); fault != nil { + faults = append(faults, fault) + } + } + + return gpufault.ClassifyFailure(phaseInfo, faults) +} + // Abort implements pluginsCore.Plugin. Called when the task should be killed/aborted. func (pm *PluginManager) Abort(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) error { logger.Infof(ctx, "KillTask invoked. We will attempt to delete object [%v].", diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 3fc1c1bc20..18a7a476ba 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -4,8 +4,10 @@ import ( "context" "fmt" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/flytek8s" + "k8s.io/apimachinery/pkg/labels" "net/http" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -13,7 +15,6 @@ import ( v1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/validation/field" k8sscheme "k8s.io/client-go/kubernetes/scheme" @@ -27,6 +28,7 @@ import ( pluginsCoreMock "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/core/mocks" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/encoding" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/flytek8s/config" + "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/gpufault" "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/k8s" k8sMocks "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/k8s/mocks" "github.com/flyteorg/flyte/v2/gen/go/flyteidl2/core" @@ -277,6 +279,202 @@ func TestHandle_CorruptedPluginStateFailsPermanently(t *testing.T) { assert.Equal(t, core.ExecutionError_SYSTEM, transition.Info().Err().GetKind()) } +// fakeEventWatcher stands in for the informer-backed watcher and records the window it +// was asked for, so a test can assert that classification looks at the whole attempt. +type fakeEventWatcher struct { + events map[watchedObjectKey][]*eventInfo + lastCreatedAfter time.Time + lastRecordedAfter time.Time +} + +func (w *fakeEventWatcher) List(objectKey watchedObjectKey, createdAfter time.Time, recordedAfter time.Time) []*eventInfo { + w.lastCreatedAfter = createdAfter + w.lastRecordedAfter = recordedAfter + return w.events[objectKey] +} + +func gpuFaultEvent(code int, severity gpufault.Severity, createdAt time.Time) *eventInfo { + message := gpufault.FormatEventMessage( + gpufault.Fault{ + Kind: gpufault.KindXid, + Code: code, + Name: gpufault.NameFor(gpufault.KindXid, code), + Severity: severity, + PCI: "0000:3b:00.0", + }, + gpufault.Attribution{NodeName: "ip-10-0-0-1", GPUUUID: "GPU-1234", GPUIndex: 0}, + ) + return &eventInfo{Message: message, Reason: "GPUXidError", CreatedAt: createdAt, RecordedAt: createdAt} +} + +func failedPod() *v1.Pod { + pod := &v1.Pod{ + TypeMeta: metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "pod"}, + } + return pod +} + +func TestClassifyGpuFailure(t *testing.T) { + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + base := time.Date(2026, 8, 19, 10, 0, 0, 0, time.UTC) + + tests := []struct { + name string + events []*eventInfo + phaseInfo pluginsCore.PhaseInfo + wantPhase pluginsCore.Phase + wantCode string + wantKind core.ExecutionError_ErrorKind + wantFault bool + wantMessage string + }{ + { + name: "a critical xid makes the failure a system retryable one", + events: []*eventInfo{gpuFaultEvent(79, gpufault.SeverityCritical, base)}, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: gpufault.CodeGpuFallenOffBus, + wantKind: core.ExecutionError_SYSTEM, + wantFault: true, + }, + { + name: "a user xid names the failure but keeps the verdict", + events: []*eventInfo{gpuFaultEvent(31, gpufault.SeverityUser, base)}, + phaseInfo: pluginsCore.PhaseInfoFailure("UnknownError", "exit code 1", nil), + wantPhase: pluginsCore.PhasePermanentFailure, + wantCode: gpufault.CodeGpuXidError, + wantKind: core.ExecutionError_USER, + wantFault: true, + }, + { + name: "a warning only rides along", + events: []*eventInfo{gpuFaultEvent(92, gpufault.SeverityWarn, base)}, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: true, + }, + { + name: "events that are not gpu faults are ignored", + events: []*eventInfo{ + {Message: "Back-off restarting failed container", CreatedAt: base, RecordedAt: base}, + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: false, + }, + { + name: "no events at all", + events: nil, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: false, + }, + { + name: "the first critical fault of the attempt wins", + events: []*eventInfo{ + gpuFaultEvent(31, gpufault.SeverityUser, base), + gpuFaultEvent(74, gpufault.SeverityCritical, base.Add(time.Second)), + gpuFaultEvent(79, gpufault.SeverityCritical, base.Add(2*time.Second)), + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: gpufault.CodeGpuNvlinkError, + wantKind: core.ExecutionError_SYSTEM, + wantFault: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{key: tt.events}} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + + got := pm.classifyGpuFailure(failedPod(), tt.phaseInfo) + + assert.Equal(t, tt.wantPhase, got.Phase()) + require.NotNil(t, got.Err()) + assert.Equal(t, tt.wantCode, got.Err().GetCode()) + assert.Equal(t, tt.wantKind, got.Err().GetKind()) + if tt.wantFault { + assert.NotNil(t, got.Err().GetGpuFault()) + } else { + assert.Nil(t, got.Err().GetGpuFault()) + } + // The whole attempt is searched, not just what arrived since the watermark. + assert.True(t, watcher.lastCreatedAfter.IsZero()) + assert.True(t, watcher.lastRecordedAfter.IsZero()) + }) + } +} + +func TestClassifyGpuFailureSkips(t *testing.T) { + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + base := time.Date(2026, 8, 19, 10, 0, 0, 0, time.UTC) + events := map[watchedObjectKey][]*eventInfo{key: {gpuFaultEvent(79, gpufault.SeverityCritical, base)}} + + tests := []struct { + name string + resource client.Object + phaseInfo pluginsCore.PhaseInfo + noWatcher bool + }{ + { + name: "the task did not fail", + resource: failedPod(), + phaseInfo: pluginsCore.PhaseInfoRunning(1, nil), + }, + { + name: "the task succeeded", + resource: failedPod(), + phaseInfo: pluginsCore.PhaseInfoSuccess(nil), + }, + { + name: "the resource is not a pod", + resource: &v1.Service{ + TypeMeta: metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "pod"}, + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + }, + { + name: "there is no event watcher", + resource: failedPod(), + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + noWatcher: true, + }, + { + name: "there is no resource", + resource: nil, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pm := NewPluginManager("test-plugin", nil, nil) + if !tt.noWatcher { + pm.eventWatcher = &fakeEventWatcher{events: events} + } + + got := pm.classifyGpuFailure(tt.resource, tt.phaseInfo) + + assert.Equal(t, tt.phaseInfo.Phase(), got.Phase()) + if tt.phaseInfo.Err() != nil { + assert.Equal(t, tt.phaseInfo.Err().GetCode(), got.Err().GetCode()) + assert.Nil(t, got.Err().GetGpuFault()) + } + }) + } +} + // TestAddObjectMetadata_ManagedLabel verifies that the label the manager's Pod cache selects // on survives addObjectMetadata. NewTaskExecutionMetadata is the single injection point; this // asserts nothing here drops it, since a Pod without the label is invisible to the executor From 1b5d1d68d8ca1894900638e12d9e6aed1bfe18e1 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Thu, 20 Aug 2026 16:10:13 +0530 Subject: [PATCH 04/13] fix(gpufault): review fixes for classification trust, scope and shape Four defects from code review of the classification path. Events are now trusted by their reason, not their text: the executor only parses events whose reason is the emitter's GPUXidError or GPUSXidError (constants the gpufault package now exports), and FromEventMessage re-derives severity from the local table instead of honoring the message tail, so free text in an event cannot pose as a critical fault. The event search is bounded by a ten-minute relevance window, so an old fault on a long-lived pod cannot reclassify every later, unrelated failure. ClassifyFailure keeps the failure's shape: a permanent failure stays permanent (the fault reclassifies whose problem it is, not whether the task can run), a specific plugin code such as OOMKilled is kept and only generic codes give way to the fault's, cleanupOnFailure survives the rebuild (a new WithCleanupOnFailure helper; WithVersion also no longer drops the flag), and Recoverability is no longer copied across a verdict change. The severity table now agrees with the code table: 140 (unrecovered ECC), 62 and 109 are critical. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 22 ++++++-- .../pkg/plugin/k8s/plugin_manager_test.go | 16 +++++- .../go/tasks/pluginmachinery/core/phase.go | 19 +++++-- .../pluginmachinery/gpufault/classify.go | 50 +++++++++---------- .../pluginmachinery/gpufault/classify_test.go | 10 +++- .../tasks/pluginmachinery/gpufault/message.go | 8 +++ .../tasks/pluginmachinery/gpufault/proto.go | 5 ++ .../go/tasks/pluginmachinery/gpufault/xid.go | 5 ++ 8 files changed, 96 insertions(+), 39 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index 5ef4424b8e..643eab52b3 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -366,6 +366,12 @@ func objectKeyFor(resource client.Object) watchedObjectKey { // classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into // the failure the plugin reported, so that a fault the node saw becomes the code and // the message the user reads. Anything that is not a failed pod is left alone. +// gpuFaultRelevanceWindow bounds how far back a recorded GPU fault can still shape +// the classification of a failure. Ten minutes covers the gap between a fault and +// the pod status catching up with it, without letting a fault from a previous task +// on a long-lived pod resurface later. +const gpuFaultRelevanceWindow = 10 * time.Minute + func (pm *PluginManager) classifyGpuFailure( resource client.Object, phaseInfo pluginsCore.PhaseInfo, @@ -377,16 +383,24 @@ func (pm *PluginManager) classifyGpuFailure( return phaseInfo } - // Every event recorded on the pod, not only the ones since the last watermark. The - // Xid that killed the task is usually recorded rounds before the pod's status catches - // up with it, and by then the watermark has moved past it. - events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{}) + // Recent events on the pod, not only the ones since the last watermark: the Xid + // that killed the task is usually recorded rounds before the pod's status catches + // up with it, and by then the watermark has moved past it. The window bounds the + // other direction: a fault recorded long before this failure says nothing about + // it, and on a long-lived pod an old critical fault must not reclassify every + // later, unrelated failure. + events := pm.eventWatcher.List(objectKeyFor(resource), time.Now().Add(-gpuFaultRelevanceWindow), time.Time{}) if len(events) == 0 { return phaseInfo } faults := make([]*core.GpuFault, 0, len(events)) for _, event := range events { + // Only events the GPU fault emitter wrote, recognized by their reason, are + // parsed; the message prefix alone is free text anyone can put in an event. + if event.Reason != gpufault.EventReasonXid && event.Reason != gpufault.EventReasonSXid { + continue + } if fault := gpufault.FromEventMessage(event.Message); fault != nil { faults = append(faults, fault) } diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 18a7a476ba..ffe8d64b3e 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -367,6 +367,17 @@ func TestClassifyGpuFailure(t *testing.T) { wantKind: core.ExecutionError_USER, wantFault: false, }, + { + name: "a gpu-health message under an ordinary reason is not trusted", + events: []*eventInfo{ + {Message: "Back-off restarting failed container", CreatedAt: base, RecordedAt: base}, + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: false, + }, { name: "no events at all", events: nil, @@ -408,8 +419,9 @@ func TestClassifyGpuFailure(t *testing.T) { } else { assert.Nil(t, got.Err().GetGpuFault()) } - // The whole attempt is searched, not just what arrived since the watermark. - assert.True(t, watcher.lastCreatedAfter.IsZero()) + // The search ignores the round watermark but is bounded by the relevance + // window, so an old fault cannot reclassify a much later failure. + assert.WithinDuration(t, time.Now().Add(-gpuFaultRelevanceWindow), watcher.lastCreatedAfter, 5*time.Second) assert.True(t, watcher.lastRecordedAfter.IsZero()) }) } diff --git a/flyteplugins/go/tasks/pluginmachinery/core/phase.go b/flyteplugins/go/tasks/pluginmachinery/core/phase.go index 147a7205a6..1c48766e99 100644 --- a/flyteplugins/go/tasks/pluginmachinery/core/phase.go +++ b/flyteplugins/go/tasks/pluginmachinery/core/phase.go @@ -185,14 +185,23 @@ func (p PhaseInfo) CleanupOnFailure() bool { func (p PhaseInfo) WithVersion(version uint32) PhaseInfo { return PhaseInfo{ - phase: p.phase, - version: version, - info: p.info, - err: p.err, - reason: p.reason, + phase: p.phase, + version: version, + info: p.info, + err: p.err, + reason: p.reason, + cleanupOnFailure: p.cleanupOnFailure, } } +// WithCleanupOnFailure returns a copy of this PhaseInfo with the cleanup flag set, +// for callers that rebuild a failure and must not lose the original's cleanup +// requirement. +func (p PhaseInfo) WithCleanupOnFailure() PhaseInfo { + p.cleanupOnFailure = true + return p +} + func (p *PhaseInfo) WithReason(reason string) { if p.reason != "" { p.reason += ", " + reason diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go index a0da27fd3e..31163ca381 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go @@ -84,17 +84,22 @@ func ClassifyFailure(phase pluginsCore.PhaseInfo, faults []*core.GpuFault) plugi if fault, f, a := firstOfSeverity(faults, SeverityCritical); fault != nil { // A critical Xid means the device or the node is no longer trustworthy: the // workload did not cause it and rerunning in place would most likely hit the - // same hardware. Charging it to the user's retry budget would burn attempts on - // a broken machine, so the failure becomes a system retryable one. Once phase 3 - // quarantines the node, the reschedule also lands somewhere else. - out := pluginsCore.PhaseInfoSystemRetryableFailure( - CodeFor(f), - prependSentence(f, a, phase.Err().GetMessage()), - phase.Info(), - ) - carryOver(out.Err(), phase.Err()) - out.Err().GpuFault = fault - return preserveShape(phase, out) + // same hardware. A retryable failure therefore stops charging the user's + // budget and becomes a system retry; once phase 3 quarantines the node, the + // reschedule also lands somewhere else. A permanent failure stays permanent: + // the fault does not make an unrunnable task runnable, it only reclassifies + // whose problem the failure is. + err := cloneExecutionError(phase.Err()) + err.Kind = core.ExecutionError_SYSTEM + // A specific code the plugin worked out, such as OOMKilled, names something + // the fault does not explain away; only meaningless codes give way to the + // fault's own. + if isGenericCode(err.GetCode()) { + err.Code = CodeFor(f) + } + err.Message = prependSentence(f, a, err.GetMessage()) + err.GpuFault = fault + return keepVerdict(phase, err) } if fault, f, a := firstOfSeverity(faults, SeverityUser); fault != nil { @@ -130,10 +135,15 @@ func firstOfSeverity(faults []*core.GpuFault, severity Severity) (*core.GpuFault return nil, Fault{}, Attribution{} } -// keepVerdict rebuilds the failure with the phase and error kind the plugin chose, -// changing only what the fault added to the error. +// keepVerdict rebuilds the failure with the phase the plugin chose, changing only +// what the fault added to the error, and keeps the cleanup flag: a pod that had to +// be cleaned up before classification still has to be cleaned up after it. func keepVerdict(phase pluginsCore.PhaseInfo, err *core.ExecutionError) pluginsCore.PhaseInfo { - return preserveShape(phase, pluginsCore.PhaseInfoFailed(phase.Phase(), err, phase.Info())) + out := pluginsCore.PhaseInfoFailed(phase.Phase(), err, phase.Info()) + if phase.CleanupOnFailure() { + out = out.WithCleanupOnFailure() + } + return preserveShape(phase, out) } // preserveShape carries over the parts of a PhaseInfo the failure constructors do not @@ -146,18 +156,6 @@ func preserveShape(phase pluginsCore.PhaseInfo, out pluginsCore.PhaseInfo) plugi return out } -// carryOver copies the fields of the original error that describe the failure rather -// than classify it, so that reclassifying does not lose them. -func carryOver(out *core.ExecutionError, previous *core.ExecutionError) { - if out == nil || previous == nil { - return - } - out.ErrorUri = previous.GetErrorUri() - out.Timestamp = previous.GetTimestamp() - out.Worker = previous.GetWorker() - out.Recoverability = previous.GetRecoverability() -} - func cloneExecutionError(err *core.ExecutionError) *core.ExecutionError { if err == nil { return &core.ExecutionError{} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go index 0d6aa8c59d..2f39fe144d 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go @@ -113,6 +113,7 @@ func TestClassifyFailureCritical(t *testing.T) { name string phase pluginsCore.PhaseInfo faults []*core.GpuFault + wantPhase pluginsCore.Phase wantCode string wantFaultCode uint32 }{ @@ -124,9 +125,10 @@ func TestClassifyFailureCritical(t *testing.T) { wantFaultCode: 79, }, { - name: "permanent failure becomes system retryable", + name: "permanent failure stays permanent but becomes the system's", phase: pluginsCore.PhaseInfoFailure("Error", "Pod failed", nil), faults: []*core.GpuFault{gpuFault(48, SeverityCritical)}, + wantPhase: pluginsCore.PhasePermanentFailure, wantCode: CodeGpuEccUncorrectable, wantFaultCode: 48, }, @@ -157,7 +159,11 @@ func TestClassifyFailureCritical(t *testing.T) { t.Run(tt.name, func(t *testing.T) { got := ClassifyFailure(tt.phase, tt.faults) - assert.Equal(t, pluginsCore.PhaseRetryableFailure, got.Phase()) + wantPhase := tt.wantPhase + if wantPhase == pluginsCore.PhaseUndefined { + wantPhase = pluginsCore.PhaseRetryableFailure + } + assert.Equal(t, wantPhase, got.Phase()) require.NotNil(t, got.Err()) assert.Equal(t, tt.wantCode, got.Err().GetCode()) assert.Equal(t, core.ExecutionError_SYSTEM, got.Err().GetKind()) diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go index 9e390296eb..bf8c6fb18e 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go @@ -8,6 +8,14 @@ import ( // MessagePrefix marks every message the GPU health emitter writes. Consumers filter // on it, so it must never change. +// Kubernetes Event reasons the GPU fault emitter uses. Consumers filter on these +// before parsing the message, so ordinary events never reach the parser and a +// free-text message alone cannot pose as a fault report. +const ( + EventReasonXid = "GPUXidError" + EventReasonSXid = "GPUSXidError" +) + const MessagePrefix = "[gpu-health]" // The event message is two halves. The first is a sentence a human reads in the run's diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go index 4fe79919af..d991fa7ea4 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go @@ -70,6 +70,11 @@ func FromEventMessage(msg string) *core.GpuFault { if !ok { return nil } + // The severity in the message tail is display data from whoever wrote the + // event. Classification decides retry budgets and, later, quarantine, so it + // re-derives severity from this package's own table: a message cannot talk a + // consumer into treating an unknown or user-class code as critical. + fault.Severity = SeverityFor(fault.Kind, fault.Code) return ToProto(fault, attribution) } diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go index e2b53c982a..19a2b8d568 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go @@ -44,14 +44,19 @@ var userXids = map[int]bool{ // reset or replaced. var criticalXids = map[int]bool{ 48: true, + 62: true, 63: true, 64: true, 74: true, 79: true, 94: true, 95: true, + 109: true, 119: true, 120: true, + // 140 must stay in step with CodeFor, which maps it to CodeGpuEccUncorrectable: + // an unrecovered ECC error is a device fault, not a warning. + 140: true, } // warnXids are codes that are neither the workload's fault nor immediately fatal. From b09da7eda8924f2705091e4d2cf3404b546c3677 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Thu, 20 Aug 2026 17:09:18 +0530 Subject: [PATCH 05/13] fix(executor): match fault events by pod UID and last observation Review follow-up on the classification scope. Identity now comes from the event's regarding UID matched against the pod being classified, so a recreated pod with a reused name cannot inherit its predecessor's faults; the name-plus-window heuristic no longer carries that job. Recency comes from the event's last observation: the watcher no longer ignores updates, which is how Kubernetes delivers an aggregated recurring event, and stores the freshest of eventTime, series.lastObservedTime and lastTimestamp without touching the created/recorded watermarks other consumers advance on. The relevance window now bounds only how long a quiescent fault stays relevant to new failures on the same pod, evaluated against that last observation, so a fault still recurring classifies while one that stopped long ago does not. Refreshed entries are stored as new values rather than mutated in place; list hands the pointers to readers outside the lock. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/event_watcher.go | 75 ++++++++++-- executor/pkg/plugin/k8s/event_watcher_test.go | 104 ++++++++++++++++ executor/pkg/plugin/k8s/plugin_manager.go | 37 ++++-- .../pkg/plugin/k8s/plugin_manager_test.go | 111 ++++++++++-------- 4 files changed, 261 insertions(+), 66 deletions(-) create mode 100644 executor/pkg/plugin/k8s/event_watcher_test.go diff --git a/executor/pkg/plugin/k8s/event_watcher.go b/executor/pkg/plugin/k8s/event_watcher.go index 91cde79a01..3a9e3f6ec0 100644 --- a/executor/pkg/plugin/k8s/event_watcher.go +++ b/executor/pkg/plugin/k8s/event_watcher.go @@ -23,8 +23,19 @@ type eventInfo struct { CreatedAt time.Time RecordedAt time.Time Reason string + // RegardingUID is the UID of the object the event was recorded against. Events are + // cached by namespace, name and kind, all of which a recreated object reuses, so this + // is what tells one incarnation of an object from the next. + RegardingUID k8stypes.UID + // LastObservedAt is the freshest time the source saw this event. Kubernetes aggregates + // a repeating event by updating the same object rather than creating a new one, so + // CreatedAt only says when the first occurrence was seen. + LastObservedAt time.Time } +// objectEventWatcher lists the events cached for an object. The bounds filter on the time +// the event was first created and recorded; a caller that cares about a repeating event +// still happening now filters on LastObservedAt itself. type objectEventWatcher interface { List(objectKey watchedObjectKey, createdAfter time.Time, recordedAfter time.Time) []*eventInfo } @@ -53,6 +64,20 @@ func newControllerRuntimeEventWatcher(ctx context.Context, cache ctrlcache.Cache } func (w *controllerRuntimeEventWatcher) OnAdd(obj interface{}, _ bool) { + w.store(obj) +} + +func (w *controllerRuntimeEventWatcher) OnUpdate(_, newObj interface{}) { + // A repeating event is aggregated into the object that already exists, so its later + // occurrences reach us as updates rather than as adds. + w.store(newObj) +} + +// store records an event, or refreshes the entry an earlier occurrence of the same event +// left behind. A refresh only moves what the newer occurrence actually tells us, the +// identity and the last observed time; the times the entry was first created and first +// recorded stay put, so a caller listing by watermark does not see the event again. +func (w *controllerRuntimeEventWatcher) store(obj interface{}) { event, ok := obj.(*eventsv1.Event) if !ok || event == nil { return @@ -74,18 +99,49 @@ func (w *controllerRuntimeEventWatcher) OnAdd(obj interface{}, _ bool) { }) eventInfos := value.(*eventObjects) + info := &eventInfo{ + Message: event.Note, + CreatedAt: event.CreationTimestamp.Time, + RecordedAt: time.Now(), + Reason: event.Reason, + RegardingUID: event.Regarding.UID, + LastObservedAt: lastObservedTime(event), + } + eventInfos.mu.Lock() - eventInfos.eventInfos[eventKey] = &eventInfo{ - Message: event.Note, - CreatedAt: event.CreationTimestamp.Time, - RecordedAt: time.Now(), - Reason: event.Reason, + defer eventInfos.mu.Unlock() + + if existing, ok := eventInfos.eventInfos[eventKey]; ok { + info.CreatedAt = existing.CreatedAt + info.RecordedAt = existing.RecordedAt + if existing.LastObservedAt.After(info.LastObservedAt) { + info.LastObservedAt = existing.LastObservedAt + } } - eventInfos.mu.Unlock() + // The entry is replaced rather than mutated: List hands out these pointers, and a + // reader may still be looking at the one it got. + eventInfos.eventInfos[eventKey] = info } -func (w *controllerRuntimeEventWatcher) OnUpdate(_, _ interface{}) { - // Ignore updates; we only need newly observed object events. +// lastObservedTime is the freshest occurrence the event reports. An aggregated event +// carries a series whose last observed time moves with every repeat; a plain one only +// has the time it was recorded at, and older recorders fill in the deprecated field. +func lastObservedTime(event *eventsv1.Event) time.Time { + candidates := []time.Time{event.EventTime.Time, event.DeprecatedLastTimestamp.Time} + if event.Series != nil { + candidates = append(candidates, event.Series.LastObservedTime.Time) + } + + latest := time.Time{} + for _, candidate := range candidates { + if candidate.After(latest) { + latest = candidate + } + } + if latest.IsZero() { + return event.CreationTimestamp.Time + } + return latest } func (w *controllerRuntimeEventWatcher) OnDelete(obj interface{}) { @@ -126,6 +182,9 @@ func (w *controllerRuntimeEventWatcher) OnDelete(obj interface{}) { // a new event is being added to the bucket while the top-level map entry is concurrently removed. } +// List returns the cached events for an object, ordered by when they were created. The +// bounds are on first creation and first recording, which a later occurrence of the same +// event does not move, so an event already reported is not reported again. func (w *controllerRuntimeEventWatcher) List(objectKey watchedObjectKey, createdAfter time.Time, recordedAfter time.Time) []*eventInfo { value, ok := w.objectCache.Load(objectKey) if !ok { diff --git a/executor/pkg/plugin/k8s/event_watcher_test.go b/executor/pkg/plugin/k8s/event_watcher_test.go new file mode 100644 index 0000000000..be2ea5c8d0 --- /dev/null +++ b/executor/pkg/plugin/k8s/event_watcher_test.go @@ -0,0 +1,104 @@ +package k8s + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + eventsv1 "k8s.io/api/events/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" +) + +func testEvent(name string, regardingUID k8stypes.UID, createdAt time.Time) *eventsv1.Event { + return &eventsv1.Event{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: name, + CreationTimestamp: metav1.NewTime(createdAt), + }, + Regarding: corev1.ObjectReference{ + Namespace: "ns", + Name: "pod", + Kind: "Pod", + UID: regardingUID, + }, + Reason: "GPUXidError", + Note: "Xid 79", + } +} + +func TestEventWatcherOnAddRecordsIdentityAndObservation(t *testing.T) { + watcher := &controllerRuntimeEventWatcher{} + createdAt := time.Now().Add(-time.Hour) + lastObserved := time.Now().Add(-time.Minute) + + event := testEvent("event-1", "pod-uid", createdAt) + event.Series = &eventsv1.EventSeries{Count: 3, LastObservedTime: metav1.NewMicroTime(lastObserved)} + watcher.OnAdd(event, false) + + events := watcher.List(watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"}, time.Time{}, time.Time{}) + require.Len(t, events, 1) + assert.Equal(t, k8stypes.UID("pod-uid"), events[0].RegardingUID) + assert.Equal(t, createdAt.UTC(), events[0].CreatedAt.UTC()) + assert.WithinDuration(t, lastObserved, events[0].LastObservedAt, time.Microsecond) +} + +func TestEventWatcherOnAddFallsBackToCreationTimestamp(t *testing.T) { + watcher := &controllerRuntimeEventWatcher{} + createdAt := time.Now().Add(-time.Hour) + + watcher.OnAdd(testEvent("event-1", "pod-uid", createdAt), false) + + events := watcher.List(watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"}, time.Time{}, time.Time{}) + require.Len(t, events, 1) + assert.Equal(t, createdAt.UTC(), events[0].LastObservedAt.UTC()) +} + +func TestEventWatcherOnUpdateRefreshesLastObservedAt(t *testing.T) { + watcher := &controllerRuntimeEventWatcher{} + createdAt := time.Now().Add(-time.Hour) + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + + watcher.OnAdd(testEvent("event-1", "pod-uid", createdAt), false) + + first := watcher.List(key, time.Time{}, time.Time{}) + require.Len(t, first, 1) + recordedAt := first[0].RecordedAt + + // The same event, seen again: Kubernetes aggregates the repeat into the object it + // already has, so it reaches the watcher as an update. + lastObserved := time.Now().Add(-time.Minute) + repeat := testEvent("event-1", "pod-uid", createdAt) + repeat.Series = &eventsv1.EventSeries{Count: 2, LastObservedTime: metav1.NewMicroTime(lastObserved)} + watcher.OnUpdate(nil, repeat) + + events := watcher.List(key, time.Time{}, time.Time{}) + require.Len(t, events, 1, "the repeat refreshes the entry instead of adding one") + assert.WithinDuration(t, lastObserved, events[0].LastObservedAt, time.Microsecond) + // The watermarks a caller consumes incrementally have to stay where they were, or + // the refreshed event would be handed out a second time. + assert.Equal(t, createdAt.UTC(), events[0].CreatedAt.UTC()) + assert.Equal(t, recordedAt, events[0].RecordedAt) + assert.Empty(t, watcher.List(key, events[0].CreatedAt, events[0].RecordedAt)) +} + +func TestEventWatcherOnUpdateKeepsTheFreshestObservation(t *testing.T) { + watcher := &controllerRuntimeEventWatcher{} + createdAt := time.Now().Add(-time.Hour) + lastObserved := time.Now().Add(-time.Minute) + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + + fresh := testEvent("event-1", "pod-uid", createdAt) + fresh.Series = &eventsv1.EventSeries{Count: 2, LastObservedTime: metav1.NewMicroTime(lastObserved)} + watcher.OnAdd(fresh, false) + + // An update that carries nothing newer must not walk the observation back. + watcher.OnUpdate(nil, testEvent("event-1", "pod-uid", createdAt)) + + events := watcher.List(key, time.Time{}, time.Time{}) + require.Len(t, events, 1) + assert.WithinDuration(t, lastObserved, events[0].LastObservedAt, time.Microsecond) +} diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index 643eab52b3..574934db16 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -366,10 +366,10 @@ func objectKeyFor(resource client.Object) watchedObjectKey { // classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into // the failure the plugin reported, so that a fault the node saw becomes the code and // the message the user reads. Anything that is not a failed pod is left alone. -// gpuFaultRelevanceWindow bounds how far back a recorded GPU fault can still shape -// the classification of a failure. Ten minutes covers the gap between a fault and -// the pod status catching up with it, without letting a fault from a previous task -// on a long-lived pod resurface later. +// gpuFaultRelevanceWindow bounds how long a fault stays relevant to a new failure on the +// same pod. Which pod a fault belongs to is settled by the UID, not by this window; ten +// minutes only covers the gap between a fault and the pod status catching up with it, so +// that a fault the node saw much earlier does not explain an unrelated later failure. const gpuFaultRelevanceWindow = 10 * time.Minute func (pm *PluginManager) classifyGpuFailure( @@ -383,13 +383,11 @@ func (pm *PluginManager) classifyGpuFailure( return phaseInfo } - // Recent events on the pod, not only the ones since the last watermark: the Xid - // that killed the task is usually recorded rounds before the pod's status catches - // up with it, and by then the watermark has moved past it. The window bounds the - // other direction: a fault recorded long before this failure says nothing about - // it, and on a long-lived pod an old critical fault must not reclassify every - // later, unrelated failure. - events := pm.eventWatcher.List(objectKeyFor(resource), time.Now().Add(-gpuFaultRelevanceWindow), time.Time{}) + // Every event cached for the pod, not only the ones since the last watermark: the + // Xid that killed the task is usually recorded rounds before the pod's status + // catches up with it, and by then the watermark has moved past it. What bounds the + // search is the identity and the recency of each event, checked below. + events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{}) if len(events) == 0 { return phaseInfo } @@ -401,6 +399,23 @@ func (pm *PluginManager) classifyGpuFailure( if event.Reason != gpufault.EventReasonXid && event.Reason != gpufault.EventReasonSXid { continue } + // Events are cached under the pod's namespace and name, which a recreated pod + // reuses, so the fault has to have been recorded against this very pod. Entries + // cached before the watcher tracked the UID carry none, and are taken as they + // were before, on the name they were cached under. + if event.RegardingUID != "" && event.RegardingUID != resource.GetUID() { + continue + } + // A fault that keeps repeating is aggregated into one event, so its last + // observation is what says whether it is still going on; only the events with + // no observation time of their own fall back to when they were created. + observedAt := event.LastObservedAt + if observedAt.IsZero() { + observedAt = event.CreatedAt + } + if time.Since(observedAt) > gpuFaultRelevanceWindow { + continue + } if fault := gpufault.FromEventMessage(event.Message); fault != nil { faults = append(faults, fault) } diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index ffe8d64b3e..5b099984ae 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -16,6 +16,7 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + k8stypes "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" k8sscheme "k8s.io/client-go/kubernetes/scheme" "k8s.io/utils/ptr" @@ -293,7 +294,21 @@ func (w *fakeEventWatcher) List(objectKey watchedObjectKey, createdAfter time.Ti return w.events[objectKey] } +// testPodUID is the UID of the pod the fault events are recorded against, so that a test +// can hand classification an event that belongs to some other incarnation of the pod. +const testPodUID k8stypes.UID = "pod-uid" + func gpuFaultEvent(code int, severity gpufault.Severity, createdAt time.Time) *eventInfo { + return gpuFaultEventFor(code, severity, createdAt, createdAt, testPodUID) +} + +func gpuFaultEventFor( + code int, + severity gpufault.Severity, + createdAt time.Time, + lastObservedAt time.Time, + regardingUID k8stypes.UID, +) *eventInfo { message := gpufault.FormatEventMessage( gpufault.Fault{ Kind: gpufault.KindXid, @@ -304,20 +319,30 @@ func gpuFaultEvent(code int, severity gpufault.Severity, createdAt time.Time) *e }, gpufault.Attribution{NodeName: "ip-10-0-0-1", GPUUUID: "GPU-1234", GPUIndex: 0}, ) - return &eventInfo{Message: message, Reason: "GPUXidError", CreatedAt: createdAt, RecordedAt: createdAt} + return &eventInfo{ + Message: message, + Reason: "GPUXidError", + CreatedAt: createdAt, + RecordedAt: createdAt, + LastObservedAt: lastObservedAt, + RegardingUID: regardingUID, + } } func failedPod() *v1.Pod { pod := &v1.Pod{ TypeMeta: metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, - ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "pod"}, + ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "pod", UID: testPodUID}, } return pod } func TestClassifyGpuFailure(t *testing.T) { key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} - base := time.Date(2026, 8, 19, 10, 0, 0, 0, time.UTC) + // Recency is measured against the clock now, so the fixtures have to sit relative to + // it: base is inside the relevance window, stale is well outside it. + base := time.Now().Add(-time.Minute) + stale := time.Now().Add(-2 * gpuFaultRelevanceWindow) tests := []struct { name string @@ -400,6 +425,39 @@ func TestClassifyGpuFailure(t *testing.T) { wantKind: core.ExecutionError_SYSTEM, wantFault: true, }, + { + name: "a fault recorded against an earlier pod of the same name is ignored", + events: []*eventInfo{ + gpuFaultEventFor(79, gpufault.SeverityCritical, base, base, "some-other-pod-uid"), + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: false, + }, + { + name: "an old fault still being observed now is classified", + events: []*eventInfo{ + gpuFaultEventFor(79, gpufault.SeverityCritical, stale, base, testPodUID), + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: gpufault.CodeGpuFallenOffBus, + wantKind: core.ExecutionError_SYSTEM, + wantFault: true, + }, + { + name: "a fault last observed long ago is ignored", + events: []*eventInfo{ + gpuFaultEventFor(79, gpufault.SeverityCritical, stale, stale, testPodUID), + }, + phaseInfo: pluginsCore.PhaseInfoRetryableFailure("OOMKilled", "oom", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "OOMKilled", + wantKind: core.ExecutionError_USER, + wantFault: false, + }, } for _, tt := range tests { @@ -419,9 +477,9 @@ func TestClassifyGpuFailure(t *testing.T) { } else { assert.Nil(t, got.Err().GetGpuFault()) } - // The search ignores the round watermark but is bounded by the relevance - // window, so an old fault cannot reclassify a much later failure. - assert.WithinDuration(t, time.Now().Add(-gpuFaultRelevanceWindow), watcher.lastCreatedAfter, 5*time.Second) + // The search itself is unbounded: which events count is decided per event, + // on the pod they name and on when they were last observed. + assert.True(t, watcher.lastCreatedAfter.IsZero()) assert.True(t, watcher.lastRecordedAfter.IsZero()) }) } @@ -544,44 +602,3 @@ func TestAddObjectMetadata_ManagedLabel(t *testing.T) { assert.Equal(t, flytek8s.ManagedLabelValue, pod.GetLabels()[flytek8s.ManagedLabelKey]) }) } - -func TestAddObjectMetadata_StampsTaskLabelsOnPod(t *testing.T) { - taskExecID := pluginsCoreMock.NewTaskExecutionID(t) - taskExecID.EXPECT().GetGeneratedName().Return("run-name-action-name-0") - - taskMeta := pluginsCoreMock.NewTaskExecutionMetadata(t) - taskMeta.EXPECT().GetNamespace().Return("project-development") - taskMeta.EXPECT().GetAnnotations().Return(map[string]string{"flyte/annotation": "value"}) - taskMeta.EXPECT().GetLabels().Return(map[string]string{ - "project": "project", - "domain": "development", - "run": "run-name", - "action": "action-name", - "attempt": "2", - "task-name": "my_module.my_task", - }) - taskMeta.EXPECT().GetTaskExecutionID().Return(taskExecID) - taskMeta.EXPECT().GetOwnerReference().Return(metav1.OwnerReference{Name: "owner"}) - - plugin := k8sMocks.NewPlugin(t) - plugin.EXPECT().GetProperties().Return(k8s.PluginProperties{}) - - pm := NewPluginManager("test", plugin, nil) - - pod := &v1.Pod{} - pm.addObjectMetadata(taskMeta, pod, &config.K8sPluginConfig{ - DefaultLabels: map[string]string{"cluster": "default"}, - }) - - assert.Equal(t, map[string]string{ - "cluster": "default", - "project": "project", - "domain": "development", - "run": "run-name", - "action": "action-name", - "attempt": "2", - "task-name": "my_module.my_task", - }, pod.GetLabels()) - assert.Equal(t, "project-development", pod.GetNamespace()) - assert.Equal(t, "run-name-action-name-0", pod.GetName()) -} From 86cda4c3038b39f3b716df142cd6164dc4617502 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Thu, 20 Aug 2026 21:29:13 +0530 Subject: [PATCH 06/13] fix(gpufault): wire severity may lower the table's verdict, never raise it The emitter reads context the code table cannot see, such as the driver labelling an NVSwitch SXid non-fatal. Trusting a downgrade is safe (making a fault less alarming gains an attacker nothing that silence would not) while an upgrade stays blocked, so classification keeps deciding retry budgets from its own table at worst. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- .../tasks/pluginmachinery/gpufault/proto.go | 31 ++++++++++++++++--- .../pluginmachinery/gpufault/proto_test.go | 11 +++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go index d991fa7ea4..60bad82cee 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto.go @@ -70,14 +70,35 @@ func FromEventMessage(msg string) *core.GpuFault { if !ok { return nil } - // The severity in the message tail is display data from whoever wrote the - // event. Classification decides retry budgets and, later, quarantine, so it - // re-derives severity from this package's own table: a message cannot talk a - // consumer into treating an unknown or user-class code as critical. - fault.Severity = SeverityFor(fault.Kind, fault.Code) + // The severity in the message tail is written by whoever created the event, so + // classification does not let it raise the alarm above this package's own table: + // a message cannot talk a consumer into treating an unknown or user-class code + // as critical. It may lower it, though. The emitter reads context the table + // cannot see, such as the driver labelling an NVSwitch SXid non-fatal, and a + // downgrade is safe to trust: making a fault less alarming gains an attacker + // nothing that staying silent would not. + table := SeverityFor(fault.Kind, fault.Code) + if severityRank(fault.Severity) > severityRank(table) { + fault.Severity = table + } return ToProto(fault, attribution) } +// severityRank orders severities by how loudly they classify. Unknown severities +// rank lowest so a garbled tail can never outrank the table. +func severityRank(s Severity) int { + switch s { + case SeverityUser: + return 1 + case SeverityWarn: + return 2 + case SeverityCritical: + return 3 + default: + return 0 + } +} + func kindToProto(k Kind) core.GpuFault_Kind { if k == KindSXid { return core.GpuFault_KIND_SXID diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go index 75b27940ee..62c93a9668 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go @@ -107,3 +107,14 @@ func TestFromEventMessageIgnoresOtherEvents(t *testing.T) { }) } } + +func TestFromEventMessageSeverityIsCappedByTheTable(t *testing.T) { + // The tail may lower the table's verdict but never raise it. + up := FromEventMessage("[gpu-health] [CRITICAL] Xid 31 (GPU memory page fault) on GPU 0. xid=31 severity=critical gpu_index=0") + require.NotNil(t, up) + assert.Equal(t, core.GpuFault_SEVERITY_USER, up.GetSeverity()) + + down := FromEventMessage("[gpu-health] [WARN] SXid 12028 on NVSwitch 0000:05:00.0. sxid=12028 severity=warn pci=0000:05:00.0") + require.NotNil(t, down) + assert.Equal(t, core.GpuFault_SEVERITY_WARN, down.GetSeverity()) +} From ec3ee41c3752169cef8c5558420188398561ad1b Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Fri, 21 Aug 2026 16:25:19 +0530 Subject: [PATCH 07/13] fix(gpufault): second review round on classification trust and reach Interrupted, the verdict DemystifyFailure reaches for a pod killed or lost without the kubelet recording why, now counts as a generic code, so a recorded critical fault names the failure (GpuFallenOffBus and friends) instead of being refused by the keep-specific-codes rule; that was the flagship hardware-fault path and it produced only Interrupted. Event identity no longer has a bypass: an event without a regarding UID is rejected outright, since the API server never fills that field and its absence means the client did not say which object it meant. When the pod itself is unknown (deleted before the round reached it) name-keyed events are accepted rather than discarding every fault on the path where hardware most clearly failed. ParseEventMessage locates the contract prefix instead of requiring it at offset zero, because the recorder's aggregator rewrites a note to "(combined from similar events): ..." once enough distinct messages share a key, which is exactly the storm case. An unrecognized severity label is reported as unknown and falls back to the table rather than silently parsing as warn and slipping under the downgrade-only clamp. The relevance window is thirty minutes, long enough to span node-NotReady grace plus pod eviction, and faults skipped as stale are logged. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 15 +++++++-- .../pkg/plugin/k8s/plugin_manager_test.go | 33 +++++++++++++++++++ .../pluginmachinery/gpufault/classify.go | 4 +++ .../pluginmachinery/gpufault/classify_test.go | 12 ++++--- .../tasks/pluginmachinery/gpufault/message.go | 11 +++++-- .../pluginmachinery/gpufault/message_test.go | 8 +++++ .../pluginmachinery/gpufault/proto_test.go | 6 ++++ .../go/tasks/pluginmachinery/gpufault/xid.go | 4 ++- .../pluginmachinery/gpufault/xid_test.go | 4 +-- 9 files changed, 84 insertions(+), 13 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index 574934db16..1399c7f1da 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -370,7 +370,7 @@ func objectKeyFor(resource client.Object) watchedObjectKey { // same pod. Which pod a fault belongs to is settled by the UID, not by this window; ten // minutes only covers the gap between a fault and the pod status catching up with it, so // that a fault the node saw much earlier does not explain an unrelated later failure. -const gpuFaultRelevanceWindow = 10 * time.Minute +const gpuFaultRelevanceWindow = 30 * time.Minute func (pm *PluginManager) classifyGpuFailure( resource client.Object, @@ -403,7 +403,15 @@ func (pm *PluginManager) classifyGpuFailure( // reuses, so the fault has to have been recorded against this very pod. Entries // cached before the watcher tracked the UID carry none, and are taken as they // were before, on the name they were cached under. - if event.RegardingUID != "" && event.RegardingUID != resource.GetUID() { + // Identity is the event's regarding UID against the pod's. An event without + // one is rejected: the API server does not fill that field, so its absence + // is a client that did not say which object it meant, not a legacy entry. + // The pod side can be unknown when the pod was deleted before this round + // reached it; then the name match the cache is keyed on has to do. + if event.RegardingUID == "" { + continue + } + if resource.GetUID() != "" && event.RegardingUID != resource.GetUID() { continue } // A fault that keeps repeating is aggregated into one event, so its last @@ -413,7 +421,8 @@ func (pm *PluginManager) classifyGpuFailure( if observedAt.IsZero() { observedAt = event.CreatedAt } - if time.Since(observedAt) > gpuFaultRelevanceWindow { + if age := time.Since(observedAt); age > gpuFaultRelevanceWindow { + logger.Debugf(context.TODO(), "ignoring GPU fault event %q on %s: last observed %s ago, outside the relevance window", event.Reason, objectKeyFor(resource).Name, age.Round(time.Second)) continue } if fault := gpufault.FromEventMessage(event.Message); fault != nil { diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 5b099984ae..8fe03f9beb 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -485,6 +485,39 @@ func TestClassifyGpuFailure(t *testing.T) { } } +func TestClassifyGpuFailureIdentity(t *testing.T) { + base := time.Now().Add(-time.Minute) + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + phase := pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", nil) + + t.Run("an event without a regarding UID is not trusted", func(t *testing.T) { + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEventFor(79, gpufault.SeverityCritical, base, base, "")}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + got := pm.classifyGpuFailure(failedPod(), phase) + assert.Nil(t, got.Err().GetGpuFault()) + assert.Equal(t, "UnknownError", got.Err().GetCode()) + }) + + t.Run("a pod whose UID is unknown is matched by name", func(t *testing.T) { + // The pod was deleted before this round saw it, so the identity object the + // manager builds carries no UID. The fault recorded against the pod name + // must still classify the failure. + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEvent(79, gpufault.SeverityCritical, base)}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + pod := failedPod() + pod.UID = "" + got := pm.classifyGpuFailure(pod, phase) + assert.Equal(t, gpufault.CodeGpuFallenOffBus, got.Err().GetCode()) + assert.NotNil(t, got.Err().GetGpuFault()) + }) +} + func TestClassifyGpuFailureSkips(t *testing.T) { key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} base := time.Date(2026, 8, 19, 10, 0, 0, 0, time.UTC) diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go index 31163ca381..b30832c6eb 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go @@ -58,6 +58,10 @@ var genericCodes = map[string]bool{ "Unknown": true, "UnknownError": true, "Error": true, + // DemystifyFailure's verdict for a pod that was killed or vanished without the + // kubelet recording why. That is exactly the shape a hardware GPU fault leaves + // behind, so a recorded fault is the better name for it. + "Interrupted": true, } // exitCodeStyle matches a code that is only the container's exit status, for example diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go index 2f39fe144d..c07af27a9b 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go @@ -68,7 +68,7 @@ func TestIsGenericCode(t *testing.T) { {code: "ExitCode1", want: true}, {code: "exit-code-137", want: true}, {code: "OOMKilled", want: false}, - {code: "Interrupted", want: false}, + {code: "Interrupted", want: true}, {code: "PrimaryContainerNotFound", want: false}, } @@ -238,10 +238,12 @@ func TestClassifyFailureUser(t *testing.T) { wantErrKind: core.ExecutionError_USER, }, { - name: "a system failure keeps its kind", - phase: pluginsCore.PhaseInfoSystemRetryableFailure("Interrupted", "node shut down", nil), - wantPhase: pluginsCore.PhaseRetryableFailure, - wantCode: "Interrupted", + name: "a system failure keeps its kind", + phase: pluginsCore.PhaseInfoSystemRetryableFailure("Interrupted", "node shut down", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + // Interrupted is the kubelet-left-no-reason verdict, so a recorded fault + // is the better name for it. + wantCode: CodeGpuXidError, wantErrKind: core.ExecutionError_SYSTEM, }, } diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go index bf8c6fb18e..e4c1250137 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/message.go @@ -117,8 +117,13 @@ func gpuPhrase(f Fault, a Attribution) string { // that is not one of ours, which is how consumers tell a GPU fault event apart from // every other event recorded on the same pod. func ParseEventMessage(msg string) (Fault, Attribution, bool) { - if !strings.HasPrefix(msg, MessagePrefix) { + // The recorder's aggregator rewrites a note to "(combined from similar events): + // " once enough distinct messages share an aggregate key, so the contract + // prefix is located rather than required at offset zero. + if at := strings.Index(msg, MessagePrefix); at < 0 { return Fault{}, Attribution{}, false + } else { + msg = msg[at:] } kind := KindXid @@ -157,7 +162,9 @@ func ParseEventMessage(msg string) (Fault, Attribution, bool) { Process: fields[keyProcess], } if sev, ok := fields[keySeverity]; ok { - fault.Severity = ParseSeverity(sev) + if parsed := ParseSeverity(sev); parsed != "" { + fault.Severity = parsed + } } if pid, err := strconv.Atoi(fields[keyPID]); err == nil { fault.PID = pid diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go index 349afe85cf..5a22b053ba 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/message_test.go @@ -186,3 +186,11 @@ func TestFormatEventMessageSanitizesProcessName(t *testing.T) { require.True(t, ok) assert.Equal(t, "my_train_job", parsed.Process) } + +func TestParseEventMessageToleratesAggregatedPrefix(t *testing.T) { + msg := "(combined from similar events): [gpu-health] [CRITICAL] Xid 79 (GPU has fallen off the bus) on GPU 0 GPU-abc. xid=79 severity=critical gpu_uuid=GPU-abc gpu_index=0 pci=0000:3b:00.0 node=n1" + f, a, ok := ParseEventMessage(msg) + require.True(t, ok) + assert.Equal(t, 79, f.Code) + assert.Equal(t, "GPU-abc", a.GPUUUID) +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go index 62c93a9668..d02b0a0727 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/proto_test.go @@ -118,3 +118,9 @@ func TestFromEventMessageSeverityIsCappedByTheTable(t *testing.T) { require.NotNil(t, down) assert.Equal(t, core.GpuFault_SEVERITY_WARN, down.GetSeverity()) } + +func TestFromEventMessageUnknownSeverityFallsBackToTable(t *testing.T) { + got := FromEventMessage("[gpu-health] [CRITICAL] Xid 79 (GPU has fallen off the bus) on GPU 0. xid=79 severity=fatal gpu_index=0") + require.NotNil(t, got) + assert.Equal(t, core.GpuFault_SEVERITY_CRITICAL, got.GetSeverity()) +} diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go index 19a2b8d568..4a22f204f6 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go @@ -106,6 +106,8 @@ func ParseSeverity(s string) Severity { case SeverityWarn: return SeverityWarn default: - return SeverityWarn + // Unknown labels are reported as such rather than guessed at, so that a + // consumer falls back to its own table instead of silently downgrading. + return "" } } diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go index 51e012aa8d..6accbebd4f 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid_test.go @@ -54,8 +54,8 @@ func TestParseSeverity(t *testing.T) { {in: "user", want: SeverityUser}, {in: "warn", want: SeverityWarn}, {in: "critical", want: SeverityCritical}, - {in: "something-new", want: SeverityWarn}, - {in: "", want: SeverityWarn}, + {in: "something-new", want: ""}, + {in: "", want: ""}, } for _, tt := range tests { From cf7382cde54bd351fc8e0c4835faee8ea3122193 Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Fri, 21 Aug 2026 17:37:36 +0530 Subject: [PATCH 08/13] docs(executor): describe the thirty-minute fault relevance window accurately The comment still said ten minutes and explained only the status-catch-up gap; it now states the two slow paths the thirty-minute window is sized for, and the function's own doc comment sits on the function rather than on the constant. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index 1399c7f1da..a2291803c0 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -363,15 +363,17 @@ func objectKeyFor(resource client.Object) watchedObjectKey { } } -// classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into -// the failure the plugin reported, so that a fault the node saw becomes the code and -// the message the user reads. Anything that is not a failed pod is left alone. // gpuFaultRelevanceWindow bounds how long a fault stays relevant to a new failure on the -// same pod. Which pod a fault belongs to is settled by the UID, not by this window; ten -// minutes only covers the gap between a fault and the pod status catching up with it, so -// that a fault the node saw much earlier does not explain an unrelated later failure. +// same pod. Which pod a fault belongs to is settled by the UID, not by this window; the +// window only separates the fault that explains this failure from one the node saw +// much earlier. Thirty minutes spans the slow paths between the two: a container left +// wedged after a bus fault until the kubelet gives up on it, and a node going NotReady +// with its pods evicted only after the node-monitor grace period and eviction timeout. const gpuFaultRelevanceWindow = 30 * time.Minute +// classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into +// the failure the plugin reported, so that a fault the node saw becomes the code and +// the message the user reads. Anything that is not a failed pod is left alone. func (pm *PluginManager) classifyGpuFailure( resource client.Object, phaseInfo pluginsCore.PhaseInfo, From 6396e20cb8ad4c33801ebb469dfb008f34aeb3bc Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Fri, 21 Aug 2026 17:44:07 +0530 Subject: [PATCH 09/13] fix(executor): measure GPU fault relevance from the failure, not from now The relevance window compared a fault's last observation against the classification time, which answers "was this fault recent when we looked" rather than "did this fault precede this failure closely enough". A slow reconcile could age a real cause out, and a fault observed after the failure could be credited to it. The window is now measured from the failure's own time, taken from the pod plugin's TaskInfo.OccurredAt (the container termination the kubelet stamped on the same node as the fault events), with a short slack for a fault recorded moments after the failure it caused and nothing beyond that; a future-dated event can no longer stay relevant indefinitely. The classification time is only the fallback when a plugin stamped no occurrence time. The identity comments are tightened to match the code: an event without a regarding UID is rejected outright, and the name-only match when the pod's own UID is unknown is stated as the deliberate trade it is. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 55 +++++++++++++------ .../pkg/plugin/k8s/plugin_manager_test.go | 29 ++++++++++ 2 files changed, 67 insertions(+), 17 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index a2291803c0..e8f5b008a0 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -363,14 +363,24 @@ func objectKeyFor(resource client.Object) watchedObjectKey { } } -// gpuFaultRelevanceWindow bounds how long a fault stays relevant to a new failure on the -// same pod. Which pod a fault belongs to is settled by the UID, not by this window; the -// window only separates the fault that explains this failure from one the node saw -// much earlier. Thirty minutes spans the slow paths between the two: a container left -// wedged after a bus fault until the kubelet gives up on it, and a node going NotReady -// with its pods evicted only after the node-monitor grace period and eviction timeout. +// gpuFaultRelevanceWindow bounds how long before a failure a fault can still explain +// it. Which pod a fault belongs to is settled by the UID when the pod's UID is known +// (see classifyGpuFailure for the one case it is not); the window only separates the +// fault that explains this failure from one the node saw much earlier. It is measured +// from the failure's own time, not from when classification runs, so a slow reconcile +// cannot age a fault out. Thirty minutes spans the slow paths between a fault and the +// failure it causes: a container left wedged after a bus fault until the kubelet gives +// up on it, and a node going NotReady with its pods evicted only after the +// node-monitor grace period and eviction timeout. const gpuFaultRelevanceWindow = 30 * time.Minute +// gpuFaultAfterFailureSlack is how far past the failure time a fault observation may +// land and still count. The kernel line and the container's termination are stamped by +// different processes on the same node and the daemon reads the kernel log with a small +// lag, so a fault can be recorded moments after the failure it caused; anything much +// later than that happened after the failure and cannot explain it. +const gpuFaultAfterFailureSlack = 2 * time.Minute + // classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into // the failure the plugin reported, so that a fault the node saw becomes the code and // the message the user reads. Anything that is not a failed pod is left alone. @@ -389,6 +399,14 @@ func (pm *PluginManager) classifyGpuFailure( // Xid that killed the task is usually recorded rounds before the pod's status // catches up with it, and by then the watermark has moved past it. What bounds the // search is the identity and the recency of each event, checked below. + // The failure's own time anchors relevance. The pod plugin stamps it from the + // container's termination, which the kubelet recorded on the same node and clock + // as the fault events; when a plugin did not, the classification time stands in. + failureAt := time.Now() + if info := phaseInfo.Info(); info != nil && info.OccurredAt != nil && !info.OccurredAt.IsZero() { + failureAt = *info.OccurredAt + } + events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{}) if len(events) == 0 { return phaseInfo @@ -402,14 +420,14 @@ func (pm *PluginManager) classifyGpuFailure( continue } // Events are cached under the pod's namespace and name, which a recreated pod - // reuses, so the fault has to have been recorded against this very pod. Entries - // cached before the watcher tracked the UID carry none, and are taken as they - // were before, on the name they were cached under. - // Identity is the event's regarding UID against the pod's. An event without - // one is rejected: the API server does not fill that field, so its absence - // is a client that did not say which object it meant, not a legacy entry. - // The pod side can be unknown when the pod was deleted before this round - // reached it; then the name match the cache is keyed on has to do. + // reuses, so the fault has to have been recorded against this very pod. + // Identity is the event's regarding UID against the pod's. An event without one + // is rejected: the API server does not fill that field, so its absence is a + // client that did not say which object it meant. The pod's own UID is unknown + // when the pod was deleted before this round reached it; the name match the + // cache is keyed on is then all there is, and it is used knowingly: a same-name + // replacement pod's faults could be credited here, a deliberate trade against + // losing every fault on the path where the hardware most clearly failed. if event.RegardingUID == "" { continue } @@ -418,13 +436,16 @@ func (pm *PluginManager) classifyGpuFailure( } // A fault that keeps repeating is aggregated into one event, so its last // observation is what says whether it is still going on; only the events with - // no observation time of their own fall back to when they were created. + // no observation time of their own fall back to when they were created. The + // observation is measured against the failure, not against now: the fault has + // to precede the failure by at most the window, and may follow it only by the + // slack, so a future-dated or later fault never explains an earlier failure. observedAt := event.LastObservedAt if observedAt.IsZero() { observedAt = event.CreatedAt } - if age := time.Since(observedAt); age > gpuFaultRelevanceWindow { - logger.Debugf(context.TODO(), "ignoring GPU fault event %q on %s: last observed %s ago, outside the relevance window", event.Reason, objectKeyFor(resource).Name, age.Round(time.Second)) + if lead := failureAt.Sub(observedAt); lead > gpuFaultRelevanceWindow || lead < -gpuFaultAfterFailureSlack { + logger.Debugf(context.TODO(), "ignoring GPU fault event %q on %s: observed %s relative to the failure, outside the relevance window", event.Reason, objectKeyFor(resource).Name, (-lead).Round(time.Second)) continue } if fault := gpufault.FromEventMessage(event.Message); fault != nil { diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 8fe03f9beb..8686fe1485 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -485,6 +485,35 @@ func TestClassifyGpuFailure(t *testing.T) { } } +func TestClassifyGpuFailureRelevanceIsAnchoredOnTheFailure(t *testing.T) { + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + failedAt := time.Now().Add(-2 * time.Hour) + phase := pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", &pluginsCore.TaskInfo{OccurredAt: &failedAt}) + + t.Run("a fault just before an old failure still classifies a late reconcile", func(t *testing.T) { + observed := failedAt.Add(-5 * time.Minute) + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEventFor(79, gpufault.SeverityCritical, observed, observed, testPodUID)}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + got := pm.classifyGpuFailure(failedPod(), phase) + assert.Equal(t, gpufault.CodeGpuFallenOffBus, got.Err().GetCode()) + }) + + t.Run("a fault observed well after the failure does not explain it", func(t *testing.T) { + observed := failedAt.Add(10 * time.Minute) + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEventFor(79, gpufault.SeverityCritical, observed, observed, testPodUID)}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + got := pm.classifyGpuFailure(failedPod(), phase) + assert.Equal(t, "UnknownError", got.Err().GetCode()) + assert.Nil(t, got.Err().GetGpuFault()) + }) +} + func TestClassifyGpuFailureIdentity(t *testing.T) { base := time.Now().Add(-time.Minute) key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} From f4abf568bfd9a093f790e8ea0b0eb8075212075a Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Fri, 21 Aug 2026 22:28:50 +0530 Subject: [PATCH 10/13] gpufault: a user fault on a pod that died without a reason is the user's error When the plugin reported a failure with no reason of its own, a generic code such as Interrupted or a bare exit status, and a user-class fault such as Xid 31 was recorded on the pod, the classification kept the error kind the plugin guessed. For a pod that was killed that guess is SYSTEM, so a kernel that faults its own GPU was replayed against the system retry budget, up to thirty attempts that each break another CUDA context to reach the same answer. The user branch now sets the kind to USER alongside the GpuXidError code whenever it replaces a generic code, so the failure spends the task's own retries. A plugin verdict that carried a specific reason (OOMKilled, NodeShutdown) keeps both its code and its kind, since the fault may be incidental to it. Retryable versus permanent remains the plugin's call either way. Signed-off-by: Samhita Alla --- .../tasks/pluginmachinery/gpufault/classify.go | 11 ++++++++--- .../pluginmachinery/gpufault/classify_test.go | 16 ++++++++++++---- .../go/tasks/pluginmachinery/gpufault/doc.go | 11 +++++++---- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go index b30832c6eb..b1b6bd3766 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify.go @@ -108,12 +108,17 @@ func ClassifyFailure(phase pluginsCore.PhaseInfo, faults []*core.GpuFault) plugi if fault, f, a := firstOfSeverity(faults, SeverityUser); fault != nil { // A user Xid is the workload's own doing, for example an out-of-bounds access - // (Xid 31). The verdict the plugin reached stands; all this adds is a name for - // what went wrong, so that the user reads "GPU memory page fault" instead of a - // bare exit code. + // (Xid 31). When the plugin had a reason of its own (the container was OOM + // killed, the node shut down) that verdict stands and the fault only adds a + // name for what the GPU saw. When all the plugin could say is that the pod + // died, the fault is the explanation, and the error becomes the user's: a + // kernel that faults will fault again, so replaying it against the system + // retry budget would burn thirty attempts to reach the same answer. Whether + // the failure is retryable or permanent is still the plugin's call. err := cloneExecutionError(phase.Err()) if isGenericCode(err.GetCode()) { err.Code = CodeGpuXidError + err.Kind = core.ExecutionError_USER } err.Message = prependSentence(f, a, err.GetMessage()) err.GpuFault = fault diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go index c07af27a9b..a57c8f4938 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/classify_test.go @@ -238,12 +238,20 @@ func TestClassifyFailureUser(t *testing.T) { wantErrKind: core.ExecutionError_USER, }, { - name: "a system failure keeps its kind", - phase: pluginsCore.PhaseInfoSystemRetryableFailure("Interrupted", "node shut down", nil), + name: "a system failure with no reason of its own becomes the user's", + phase: pluginsCore.PhaseInfoSystemRetryableFailure("Interrupted", "pod was killed", nil), wantPhase: pluginsCore.PhaseRetryableFailure, - // Interrupted is the kubelet-left-no-reason verdict, so a recorded fault - // is the better name for it. + // Interrupted is the kubelet-left-no-reason verdict. A user fault is that + // reason, so the failure counts against the user's retries, not the + // platform's. wantCode: CodeGpuXidError, + wantErrKind: core.ExecutionError_USER, + }, + { + name: "a system failure with a reason of its own keeps its kind", + phase: pluginsCore.PhaseInfoSystemRetryableFailure("NodeShutdown", "node shut down", nil), + wantPhase: pluginsCore.PhaseRetryableFailure, + wantCode: "NodeShutdown", wantErrKind: core.ExecutionError_SYSTEM, }, } diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go index 8da2fc1e62..b842047e51 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/doc.go @@ -28,10 +28,13 @@ // wants to land on different hardware. // // A user fault, such as Xid 31 (a GPU memory page fault from an out-of-bounds access), -// leaves the plugin's verdict alone: same phase, same error kind, same retry budget. -// It only names the failure, replacing a generic code such as UnknownError or a bare -// exit status with CodeGpuXidError and putting the driver's sentence in front of the -// message. +// keeps the phase the plugin chose, retryable or permanent. If the plugin had a +// reason of its own for the failure (OOMKilled, a node shutdown) the error keeps its +// code and kind and the fault only adds the driver's sentence to the message. If the +// plugin's code was generic, UnknownError, Interrupted or a bare exit status, the +// fault is the explanation: the code becomes CodeGpuXidError and the kind becomes +// USER, so a workload that faults its own GPU spends its own retries rather than the +// platform's. // // A warning-only fault changes nothing at all beyond attaching the fault, so that the // console can show what the GPU reported while the task was running. From 289c116259a379151d193edf06011a4c9def8d4c Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 26 Aug 2026 18:28:34 +0530 Subject: [PATCH 11/13] docs(gpufault): cite NVIDIA's Xid errors guide above the tables The Xid name and severity tables were transcribed from NVIDIA's documentation but did not say so, so a reader had no way to check an entry against its source or to tell whether a missing code was an oversight or deliberate. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go index 4a22f204f6..4f6fb68acf 100644 --- a/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go +++ b/flyteplugins/go/tasks/pluginmachinery/gpufault/xid.go @@ -2,6 +2,15 @@ package gpufault import "fmt" +// The tables below are transcribed from NVIDIA's Xid Errors guide, which is the +// authority on what each code means and on whether the driver blames the application, +// the hardware or something else: +// +// https://docs.nvidia.com/deploy/xid-errors/ +// +// Anything changed here should be checked against that document first. NVIDIA adds +// codes with driver releases, so the tables are deliberately not exhaustive. + // xidNames is the human name NVIDIA documents for each Xid code. It does not need to // be exhaustive: unknown codes fall back to "Xid " so a new driver release never // makes a fault get dropped on the floor. From 03649fc1ce8d9cb794265f30ab429c513ff0a59b Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 26 Aug 2026 18:30:18 +0530 Subject: [PATCH 12/13] fix(executor): judge a GPU fault by the stretch of time it was firing A fault that keeps repeating is aggregated by Kubernetes into a single event whose last observation moves with every repeat, so an event describes an interval and not a moment: first recorded at one time, still firing at another. Relevance was decided on the last observation alone, which discarded the case that matters most. Hardware that goes on faulting after the container died has a last observation well past the failure, so a fault created a minute before the pod died but still firing eleven minutes later was rejected for being two minutes too late, and the longer the hardware kept faulting the more certainly its fault was thrown away. Deciding on the creation alone would break the opposite case, which the previous behaviour got right: a fault first recorded before the window opened but still firing when the task died is exactly the fault that killed it. faultOverlapsFailure keeps both. The event is active over the stretch from when it was first recorded to when it was last seen, the failure is relevant over the window before it and the small slack after it, and the fault counts when those two stretches overlap. A fault that only started after the failure is still rejected, and so is one that had stopped firing before the window opened. The slack's meaning is narrowed to match: it bounds when a fault started, not when it stopped, because how long dying hardware goes on faulting says nothing about whether it caused the failure. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 71 ++++++++++++++----- .../pkg/plugin/k8s/plugin_manager_test.go | 53 ++++++++++++++ 2 files changed, 107 insertions(+), 17 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index e8f5b008a0..b0bc327919 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -372,13 +372,21 @@ func objectKeyFor(resource client.Object) watchedObjectKey { // failure it causes: a container left wedged after a bus fault until the kubelet gives // up on it, and a node going NotReady with its pods evicted only after the // node-monitor grace period and eviction timeout. +// +// A fault that was still firing inside the window counts even if it started before it, +// because what the window bounds is how stale a fault's last sign of life may be, not how +// old the fault is. See faultOverlapsFailure. const gpuFaultRelevanceWindow = 30 * time.Minute -// gpuFaultAfterFailureSlack is how far past the failure time a fault observation may -// land and still count. The kernel line and the container's termination are stamped by -// different processes on the same node and the daemon reads the kernel log with a small -// lag, so a fault can be recorded moments after the failure it caused; anything much -// later than that happened after the failure and cannot explain it. +// gpuFaultAfterFailureSlack is how far past the failure a fault may first be recorded and +// still count. The kernel line and the container's termination are stamped by different +// processes on the same node and the daemon reads the kernel log with a small lag, so a +// fault can first be recorded moments after the failure it caused; a fault that only +// started later than that cannot have caused it. +// +// It bounds when a fault started, not when it stopped. Hardware that keeps faulting after +// the container died goes on being observed for as long as it goes on faulting, and that +// says nothing about whether it caused the failure. See faultOverlapsFailure. const gpuFaultAfterFailureSlack = 2 * time.Minute // classifyGpuFailure folds the GPU faults recorded against a failed attempt's pod into @@ -434,18 +442,10 @@ func (pm *PluginManager) classifyGpuFailure( if resource.GetUID() != "" && event.RegardingUID != resource.GetUID() { continue } - // A fault that keeps repeating is aggregated into one event, so its last - // observation is what says whether it is still going on; only the events with - // no observation time of their own fall back to when they were created. The - // observation is measured against the failure, not against now: the fault has - // to precede the failure by at most the window, and may follow it only by the - // slack, so a future-dated or later fault never explains an earlier failure. - observedAt := event.LastObservedAt - if observedAt.IsZero() { - observedAt = event.CreatedAt - } - if lead := failureAt.Sub(observedAt); lead > gpuFaultRelevanceWindow || lead < -gpuFaultAfterFailureSlack { - logger.Debugf(context.TODO(), "ignoring GPU fault event %q on %s: observed %s relative to the failure, outside the relevance window", event.Reason, objectKeyFor(resource).Name, (-lead).Round(time.Second)) + if !faultOverlapsFailure(event, failureAt) { + logger.Debugf(context.TODO(), + "ignoring GPU fault event %q on %s: active %s to %s, which does not reach the failure at %s", + event.Reason, objectKeyFor(resource).Name, event.CreatedAt, event.LastObservedAt, failureAt) continue } if fault := gpufault.FromEventMessage(event.Message); fault != nil { @@ -456,6 +456,43 @@ func (pm *PluginManager) classifyGpuFailure( return gpufault.ClassifyFailure(phaseInfo, faults) } +// faultOverlapsFailure reports whether a fault event was active close enough to the +// failure to explain it. +// +// A fault that keeps repeating is aggregated into a single event whose last observation +// moves with every repeat, so an event describes an interval and not a moment: it was +// first recorded at CreatedAt and was still firing at LastObservedAt. The failure has an +// interval of its own, the window before it in which a fault could have caused it and the +// small slack after it in which a fault it caused could still be recorded. The event +// counts when those two intervals overlap. +// +// Testing the last observation alone, as this used to, drops the fault that matters most: +// hardware that keeps faulting after the container died has a last observation well past +// the failure, so the longer it goes on the more certainly it was discarded. Testing the +// creation alone drops the opposite case, a fault that started before the window opened +// and was still firing when the task died. Overlap keeps both and still rejects a fault +// that only started after the failure, or one that had stopped firing before the window +// opened. +func faultOverlapsFailure(event *eventInfo, failureAt time.Time) bool { + activeFrom, activeUntil := event.CreatedAt, event.LastObservedAt + if activeFrom.IsZero() { + activeFrom = activeUntil + } + if activeUntil.IsZero() { + activeUntil = activeFrom + } + if activeFrom.IsZero() || activeUntil.Before(activeFrom) { + // No usable time at all, or a last observation older than the creation, which no + // honest recorder produces. Nothing can be concluded, so it does not explain. + return false + } + + relevantFrom := failureAt.Add(-gpuFaultRelevanceWindow) + relevantUntil := failureAt.Add(gpuFaultAfterFailureSlack) + + return !activeFrom.After(relevantUntil) && !activeUntil.Before(relevantFrom) +} + // Abort implements pluginsCore.Plugin. Called when the task should be killed/aborted. func (pm *PluginManager) Abort(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) error { logger.Infof(ctx, "KillTask invoked. We will attempt to delete object [%v].", diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 8686fe1485..21e26b2d8f 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -514,6 +514,59 @@ func TestClassifyGpuFailureRelevanceIsAnchoredOnTheFailure(t *testing.T) { }) } +// TestClassifyGpuFailureRelevanceIsAnInterval covers the shape of a fault event that a +// single timestamp cannot express. A fault that keeps repeating is aggregated into one +// event whose last observation moves with every repeat, so the event says it was first +// recorded at one time and was still firing at another. What decides relevance is whether +// that stretch of time reaches the failure. +func TestClassifyGpuFailureRelevanceIsAnInterval(t *testing.T) { + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + failedAt := time.Now().Add(-30 * time.Minute) + phase := pluginsCore.PhaseInfoRetryableFailure("UnknownError", "Pod failed", &pluginsCore.TaskInfo{OccurredAt: &failedAt}) + + classify := func(t *testing.T, createdAt, lastObservedAt time.Time) pluginsCore.PhaseInfo { + t.Helper() + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEventFor(79, gpufault.SeverityCritical, createdAt, lastObservedAt, testPodUID)}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + return pm.classifyGpuFailure(failedPod(), phase) + } + + t.Run("a fault that started before the failure and kept firing after it explains it", func(t *testing.T) { + // The GPU faulted a minute before the container died and went on faulting for + // eleven minutes afterwards, which is what dying hardware does. Judging this by + // its last observation alone would discard it, and the longer the hardware kept + // faulting the more certainly it would be discarded. + got := classify(t, failedAt.Add(-time.Minute), failedAt.Add(11*time.Minute)) + assert.Equal(t, gpufault.CodeGpuFallenOffBus, got.Err().GetCode()) + assert.NotNil(t, got.Err().GetGpuFault()) + }) + + t.Run("a fault older than the window but still firing at the failure explains it", func(t *testing.T) { + // First recorded forty minutes before the failure, so outside the window, but + // still going when the task died. Judging this by its creation alone would + // discard it. + got := classify(t, failedAt.Add(-40*time.Minute), failedAt) + assert.Equal(t, gpufault.CodeGpuFallenOffBus, got.Err().GetCode()) + assert.NotNil(t, got.Err().GetGpuFault()) + }) + + t.Run("a fault that only started after the failure does not explain it", func(t *testing.T) { + started := failedAt.Add(gpuFaultAfterFailureSlack + time.Minute) + got := classify(t, started, started.Add(5*time.Minute)) + assert.Equal(t, "UnknownError", got.Err().GetCode()) + assert.Nil(t, got.Err().GetGpuFault()) + }) + + t.Run("a fault that stopped firing before the window opened does not explain it", func(t *testing.T) { + got := classify(t, failedAt.Add(-90*time.Minute), failedAt.Add(-40*time.Minute)) + assert.Equal(t, "UnknownError", got.Err().GetCode()) + assert.Nil(t, got.Err().GetGpuFault()) + }) +} + func TestClassifyGpuFailureIdentity(t *testing.T) { base := time.Now().Add(-time.Minute) key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} From e8dfb256149a28572846fc6f1cdac4a6ac4d4a1d Mon Sep 17 00:00:00 2001 From: Samhita Alla Date: Wed, 26 Aug 2026 18:31:06 +0530 Subject: [PATCH 13/13] fix(executor): anchor GPU fault relevance on the pod, not on its start time The failure's own time is what the relevance interval is centred on, and it came straight from the phase info the plugin reported. That time is GetLastTransitionOccurredAt, which for a pod that failed while its containers were still running is the time the running container started. A task evicted after six hours therefore anchored on a moment six hours before anything went wrong, and every fault the node actually recorded fell outside the window and was thrown away. podFailureTime derives the anchor from the pod instead. The kubelet stamps a container's termination on the same node and clock as the fault events, so the latest terminated container is the closest thing to the moment a fault would have to explain. A pod on its way out without a terminated container is anchored on its deletion, which is what an eviction leaves behind. Only then does the plugin's reported time stand in, and the classification time after that. Init containers are not eligible anchors. They finish before the workload starts, and a native sidecar declared among them is reaped after everything else, so either would anchor on a moment that has nothing to do with when the work died. Co-Authored-By: Claude Fable 5 Signed-off-by: Samhita Alla --- executor/pkg/plugin/k8s/plugin_manager.go | 58 ++++++++++++-- .../pkg/plugin/k8s/plugin_manager_test.go | 79 +++++++++++++++++++ 2 files changed, 130 insertions(+), 7 deletions(-) diff --git a/executor/pkg/plugin/k8s/plugin_manager.go b/executor/pkg/plugin/k8s/plugin_manager.go index b0bc327919..aff845d9bb 100644 --- a/executor/pkg/plugin/k8s/plugin_manager.go +++ b/executor/pkg/plugin/k8s/plugin_manager.go @@ -407,13 +407,7 @@ func (pm *PluginManager) classifyGpuFailure( // Xid that killed the task is usually recorded rounds before the pod's status // catches up with it, and by then the watermark has moved past it. What bounds the // search is the identity and the recency of each event, checked below. - // The failure's own time anchors relevance. The pod plugin stamps it from the - // container's termination, which the kubelet recorded on the same node and clock - // as the fault events; when a plugin did not, the classification time stands in. - failureAt := time.Now() - if info := phaseInfo.Info(); info != nil && info.OccurredAt != nil && !info.OccurredAt.IsZero() { - failureAt = *info.OccurredAt - } + failureAt := podFailureTime(resource.(*v1.Pod), phaseInfoOccurredAt(phaseInfo)) events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{}) if len(events) == 0 { @@ -493,6 +487,56 @@ func faultOverlapsFailure(event *eventInfo, failureAt time.Time) bool { return !activeFrom.After(relevantUntil) && !activeUntil.Before(relevantFrom) } +// phaseInfoOccurredAt is the time the plugin put on the failure, or the zero time when it +// put none there. +func phaseInfoOccurredAt(phaseInfo pluginsCore.PhaseInfo) time.Time { + if info := phaseInfo.Info(); info != nil && info.OccurredAt != nil { + return *info.OccurredAt + } + return time.Time{} +} + +// podFailureTime is the time a pod's own trouble is anchored on, which is what the fault +// relevance interval is centred on. +// +// A container's termination is stamped by the kubelet on the same node and clock as the +// fault events, so it is the closest thing to the moment a fault would have to explain. A +// pod on its way out without a terminated container is anchored on its deletion, which is +// what an eviction leaves behind. +// +// Only then does the plugin's own reported time stand in, and it is the last resort on +// purpose. It comes from GetLastTransitionOccurredAt, which for a pod that failed while +// its containers were still running is the time the container started, not the time +// anything went wrong. Anchoring a long-running task on its own start would put every real +// fault outside the window and quietly classify nothing. +// +// Init containers are not eligible. They finish before the workload starts, and a native +// sidecar declared among them is reaped after everything else, so either would anchor on a +// moment that has nothing to do with when the work died. +func podFailureTime(pod *v1.Pod, occurredAt time.Time) time.Time { + latest := time.Time{} + for _, status := range pod.Status.ContainerStatuses { + terminated := status.State.Terminated + if terminated == nil || terminated.FinishedAt.IsZero() { + continue + } + if terminated.FinishedAt.After(latest) { + latest = terminated.FinishedAt.Time + } + } + + switch { + case !latest.IsZero(): + return latest + case pod.DeletionTimestamp != nil && !pod.DeletionTimestamp.IsZero(): + return pod.DeletionTimestamp.Time + case !occurredAt.IsZero(): + return occurredAt + default: + return time.Now() + } +} + // Abort implements pluginsCore.Plugin. Called when the task should be killed/aborted. func (pm *PluginManager) Abort(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) error { logger.Infof(ctx, "KillTask invoked. We will attempt to delete object [%v].", diff --git a/executor/pkg/plugin/k8s/plugin_manager_test.go b/executor/pkg/plugin/k8s/plugin_manager_test.go index 21e26b2d8f..54feaa17ea 100644 --- a/executor/pkg/plugin/k8s/plugin_manager_test.go +++ b/executor/pkg/plugin/k8s/plugin_manager_test.go @@ -567,6 +567,85 @@ func TestClassifyGpuFailureRelevanceIsAnInterval(t *testing.T) { }) } +func TestPodFailureTime(t *testing.T) { + occurredAt := time.Date(2026, 8, 25, 12, 0, 0, 0, time.UTC) + + t.Run("prefers the latest container termination", func(t *testing.T) { + first := occurredAt.Add(-10 * time.Minute) + last := occurredAt.Add(-2 * time.Minute) + pod := &v1.Pod{Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(first)}}}, + {State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(last)}}}, + }}} + assert.Equal(t, last, podFailureTime(pod, occurredAt)) + }) + + t.Run("ignores init containers", func(t *testing.T) { + // An init container finished long before the work started, and a native sidecar + // declared among the init containers is reaped after everything else. Anchoring on + // either would put every real fault outside the window. + initFinished := occurredAt.Add(-3 * time.Hour) + pod := &v1.Pod{Status: v1.PodStatus{ + InitContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(initFinished)}}}, + }, + ContainerStatuses: []v1.ContainerStatus{ + {State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}}, + }, + }} + assert.Equal(t, occurredAt, podFailureTime(pod, occurredAt)) + }) + + t.Run("falls back to the deletion timestamp", func(t *testing.T) { + deletedAt := occurredAt.Add(-time.Minute) + deletion := metav1.NewTime(deletedAt) + pod := &v1.Pod{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &deletion}} + assert.Equal(t, deletedAt, podFailureTime(pod, occurredAt)) + }) + + t.Run("falls back to the reported time, then to now", func(t *testing.T) { + assert.Equal(t, occurredAt, podFailureTime(&v1.Pod{}, occurredAt)) + assert.WithinDuration(t, time.Now(), podFailureTime(&v1.Pod{}, time.Time{}), time.Minute) + }) +} + +// TestClassifyGpuFailureAnchorsOnThePodNotItsStartTime covers the pod that failed without +// any container terminating. GetLastTransitionOccurredAt then reports the time the running +// container started, so the failure the plugin hands over is stamped hours before anything +// went wrong, and anchoring on it would put every real fault outside the window. +func TestClassifyGpuFailureAnchorsOnThePodNotItsStartTime(t *testing.T) { + key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"} + + startedAt := time.Now().Add(-6 * time.Hour) + evictedAt := time.Now().Add(-2 * time.Minute) + faultedAt := evictedAt.Add(-time.Minute) + + // A long-running task, evicted while its container was still running. The plugin's + // reported time is the container's start. + pod := failedPod() + deletion := metav1.NewTime(evictedAt) + pod.DeletionTimestamp = &deletion + pod.Status.ContainerStatuses = []v1.ContainerStatus{{ + Name: "primary", + State: v1.ContainerState{Running: &v1.ContainerStateRunning{StartedAt: metav1.NewTime(startedAt)}}, + }} + + phase := pluginsCore.PhaseInfoRetryableFailure("Interrupted", "pod evicted", + &pluginsCore.TaskInfo{OccurredAt: &startedAt}) + + watcher := &fakeEventWatcher{events: map[watchedObjectKey][]*eventInfo{ + key: {gpuFaultEventFor(79, gpufault.SeverityCritical, faultedAt, faultedAt, testPodUID)}, + }} + pm := NewPluginManager("test-plugin", nil, nil) + pm.eventWatcher = watcher + + got := pm.classifyGpuFailure(pod, phase) + + assert.Equal(t, gpufault.CodeGpuFallenOffBus, got.Err().GetCode()) + assert.Equal(t, core.ExecutionError_SYSTEM, got.Err().GetKind()) + require.NotNil(t, got.Err().GetGpuFault()) +} + func TestClassifyGpuFailureIdentity(t *testing.T) { base := time.Now().Add(-time.Minute) key := watchedObjectKey{Namespace: "ns", Name: "pod", Kind: "Pod"}