Skip to content

Commit 67990be

Browse files
samhita-allaclaude
andcommitted
refactor(flytek8s): export the failure anchor a fault is judged against
Working out when a pod's trouble actually happened is the other half of deciding whether a GPU fault explains a failure, and it was still private to the executor. The leaseworker in unionai/cloud is now adopting the relevance check, so without this it would carry a second copy that has to move in lockstep with the first. PodFailureTime moves to flytek8s, which is where pod inspection already lives, and lands next to GetLastTransitionOccurredAt on purpose: that function is the reason this one exists. It reports the latest transition across every container including the init containers, so for a pod that failed while its containers were still running it hands back the time the container started. Anchoring a six-hour task on its own start puts every real fault outside the window. PodFailureTime takes the latest terminated main container instead, then the deletion timestamp an eviction leaves behind, and only then whatever time the caller already had. It stays out of gpufault, which has no Kubernetes imports and should keep none. phaseInfoOccurredAt stays in the executor: it reads a PhaseInfo, not a pod, and nothing outside the plugin machinery has one. The table tests move with the function and gained the cases that were awkward to reach through the executor, including one asserting the divergence from GetLastTransitionOccurredAt that motivates the whole helper. The executor keeps its end-to-end test for the evicted pod, which is what catches the anchor being wired up wrong rather than computed wrong. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Samhita Alla <aallasamhita@gmail.com>
1 parent 1df51ea commit 67990be

4 files changed

Lines changed: 155 additions & 84 deletions

File tree

executor/pkg/plugin/k8s/plugin_manager.go

Lines changed: 2 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818

1919
"github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/errors"
2020
pluginsCore "github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/core"
21+
"github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/flytek8s"
2122
"github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/flytek8s/config"
2223
"github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/gpufault"
2324
"github.com/flyteorg/flyte/v2/flyteplugins/go/tasks/pluginmachinery/k8s"
@@ -381,7 +382,7 @@ func (pm *PluginManager) classifyGpuFailure(
381382
// Xid that killed the task is usually recorded rounds before the pod's status
382383
// catches up with it, and by then the watermark has moved past it. What bounds the
383384
// search is the identity and the recency of each event, checked below.
384-
failureAt := podFailureTime(resource.(*v1.Pod), phaseInfoOccurredAt(phaseInfo))
385+
failureAt := flytek8s.PodFailureTime(resource.(*v1.Pod), phaseInfoOccurredAt(phaseInfo))
385386

386387
events := pm.eventWatcher.List(objectKeyFor(resource), time.Time{}, time.Time{})
387388
if len(events) == 0 {
@@ -433,47 +434,6 @@ func phaseInfoOccurredAt(phaseInfo pluginsCore.PhaseInfo) time.Time {
433434
return time.Time{}
434435
}
435436

436-
// podFailureTime is the time a pod's own trouble is anchored on, which is what the fault
437-
// relevance interval is centred on.
438-
//
439-
// A container's termination is stamped by the kubelet on the same node and clock as the
440-
// fault events, so it is the closest thing to the moment a fault would have to explain. A
441-
// pod on its way out without a terminated container is anchored on its deletion, which is
442-
// what an eviction leaves behind.
443-
//
444-
// Only then does the plugin's own reported time stand in, and it is the last resort on
445-
// purpose. It comes from GetLastTransitionOccurredAt, which for a pod that failed while
446-
// its containers were still running is the time the container started, not the time
447-
// anything went wrong. Anchoring a long-running task on its own start would put every real
448-
// fault outside the window and quietly classify nothing.
449-
//
450-
// Init containers are not eligible. They finish before the workload starts, and a native
451-
// sidecar declared among them is reaped after everything else, so either would anchor on a
452-
// moment that has nothing to do with when the work died.
453-
func podFailureTime(pod *v1.Pod, occurredAt time.Time) time.Time {
454-
latest := time.Time{}
455-
for _, status := range pod.Status.ContainerStatuses {
456-
terminated := status.State.Terminated
457-
if terminated == nil || terminated.FinishedAt.IsZero() {
458-
continue
459-
}
460-
if terminated.FinishedAt.After(latest) {
461-
latest = terminated.FinishedAt.Time
462-
}
463-
}
464-
465-
switch {
466-
case !latest.IsZero():
467-
return latest
468-
case pod.DeletionTimestamp != nil && !pod.DeletionTimestamp.IsZero():
469-
return pod.DeletionTimestamp.Time
470-
case !occurredAt.IsZero():
471-
return occurredAt
472-
default:
473-
return time.Now()
474-
}
475-
}
476-
477437
// Abort implements pluginsCore.Plugin. Called when the task should be killed/aborted.
478438
func (pm *PluginManager) Abort(ctx context.Context, tCtx pluginsCore.TaskExecutionContext) error {
479439
logger.Infof(ctx, "KillTask invoked. We will attempt to delete object [%v].",

executor/pkg/plugin/k8s/plugin_manager_test.go

Lines changed: 0 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -567,48 +567,6 @@ func TestClassifyGpuFailureRelevanceIsAnInterval(t *testing.T) {
567567
})
568568
}
569569

570-
func TestPodFailureTime(t *testing.T) {
571-
occurredAt := time.Date(2026, 8, 25, 12, 0, 0, 0, time.UTC)
572-
573-
t.Run("prefers the latest container termination", func(t *testing.T) {
574-
first := occurredAt.Add(-10 * time.Minute)
575-
last := occurredAt.Add(-2 * time.Minute)
576-
pod := &v1.Pod{Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{
577-
{State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(first)}}},
578-
{State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(last)}}},
579-
}}}
580-
assert.Equal(t, last, podFailureTime(pod, occurredAt))
581-
})
582-
583-
t.Run("ignores init containers", func(t *testing.T) {
584-
// An init container finished long before the work started, and a native sidecar
585-
// declared among the init containers is reaped after everything else. Anchoring on
586-
// either would put every real fault outside the window.
587-
initFinished := occurredAt.Add(-3 * time.Hour)
588-
pod := &v1.Pod{Status: v1.PodStatus{
589-
InitContainerStatuses: []v1.ContainerStatus{
590-
{State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(initFinished)}}},
591-
},
592-
ContainerStatuses: []v1.ContainerStatus{
593-
{State: v1.ContainerState{Running: &v1.ContainerStateRunning{}}},
594-
},
595-
}}
596-
assert.Equal(t, occurredAt, podFailureTime(pod, occurredAt))
597-
})
598-
599-
t.Run("falls back to the deletion timestamp", func(t *testing.T) {
600-
deletedAt := occurredAt.Add(-time.Minute)
601-
deletion := metav1.NewTime(deletedAt)
602-
pod := &v1.Pod{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &deletion}}
603-
assert.Equal(t, deletedAt, podFailureTime(pod, occurredAt))
604-
})
605-
606-
t.Run("falls back to the reported time, then to now", func(t *testing.T) {
607-
assert.Equal(t, occurredAt, podFailureTime(&v1.Pod{}, occurredAt))
608-
assert.WithinDuration(t, time.Now(), podFailureTime(&v1.Pod{}, time.Time{}), time.Minute)
609-
})
610-
}
611-
612570
// TestClassifyGpuFailureAnchorsOnThePodNotItsStartTime covers the pod that failed without
613571
// any container terminating. GetLastTransitionOccurredAt then reports the time the running
614572
// container started, so the failure the plugin hands over is stamped hours before anything
Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
package flytek8s
2+
3+
import (
4+
"testing"
5+
"time"
6+
7+
"github.com/stretchr/testify/assert"
8+
v1 "k8s.io/api/core/v1"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
)
11+
12+
func terminatedAt(at time.Time) v1.ContainerStatus {
13+
return v1.ContainerStatus{
14+
State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(at)}},
15+
}
16+
}
17+
18+
func runningSince(at time.Time) v1.ContainerStatus {
19+
return v1.ContainerStatus{
20+
State: v1.ContainerState{Running: &v1.ContainerStateRunning{StartedAt: metav1.NewTime(at)}},
21+
}
22+
}
23+
24+
func TestPodFailureTime(t *testing.T) {
25+
occurredAt := time.Date(2026, 8, 25, 12, 0, 0, 0, time.UTC)
26+
27+
t.Run("prefers the latest container termination", func(t *testing.T) {
28+
first := occurredAt.Add(-10 * time.Minute)
29+
last := occurredAt.Add(-2 * time.Minute)
30+
pod := &v1.Pod{Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{
31+
terminatedAt(first),
32+
terminatedAt(last),
33+
}}}
34+
assert.Equal(t, last, PodFailureTime(pod, occurredAt))
35+
})
36+
37+
t.Run("ignores a container that has not terminated", func(t *testing.T) {
38+
died := occurredAt.Add(-2 * time.Minute)
39+
pod := &v1.Pod{Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{
40+
runningSince(occurredAt.Add(-6 * time.Hour)),
41+
terminatedAt(died),
42+
}}}
43+
assert.Equal(t, died, PodFailureTime(pod, occurredAt))
44+
})
45+
46+
t.Run("ignores a termination with no finish time", func(t *testing.T) {
47+
pod := &v1.Pod{Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{
48+
{State: v1.ContainerState{Terminated: &v1.ContainerStateTerminated{}}},
49+
}}}
50+
assert.Equal(t, occurredAt, PodFailureTime(pod, occurredAt))
51+
})
52+
53+
t.Run("ignores init containers", func(t *testing.T) {
54+
// This is where it parts company with GetLastTransitionOccurredAt. An init
55+
// container finished before the work started, and a native sidecar declared among
56+
// the init containers is reaped after everything else. Anchoring on either would
57+
// name a moment that has nothing to do with when the work died.
58+
initFinished := occurredAt.Add(-3 * time.Hour)
59+
pod := &v1.Pod{Status: v1.PodStatus{
60+
InitContainerStatuses: []v1.ContainerStatus{terminatedAt(initFinished)},
61+
ContainerStatuses: []v1.ContainerStatus{runningSince(occurredAt.Add(-3 * time.Hour))},
62+
}}
63+
assert.Equal(t, occurredAt, PodFailureTime(pod, occurredAt))
64+
assert.NotEqual(t, initFinished, PodFailureTime(pod, occurredAt))
65+
})
66+
67+
t.Run("falls back to the deletion timestamp", func(t *testing.T) {
68+
// What an eviction leaves behind: nothing terminated, but the pod is on its way
69+
// out and the API server stamped when.
70+
deletedAt := occurredAt.Add(-time.Minute)
71+
deletion := metav1.NewTime(deletedAt)
72+
pod := &v1.Pod{
73+
ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &deletion},
74+
Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{runningSince(occurredAt.Add(-6 * time.Hour))}},
75+
}
76+
assert.Equal(t, deletedAt, PodFailureTime(pod, occurredAt))
77+
})
78+
79+
t.Run("prefers a termination over the deletion timestamp", func(t *testing.T) {
80+
died := occurredAt.Add(-5 * time.Minute)
81+
deletion := metav1.NewTime(occurredAt.Add(-time.Minute))
82+
pod := &v1.Pod{
83+
ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &deletion},
84+
Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{terminatedAt(died)}},
85+
}
86+
assert.Equal(t, died, PodFailureTime(pod, occurredAt))
87+
})
88+
89+
t.Run("falls back to the time the caller offered, then to now", func(t *testing.T) {
90+
assert.Equal(t, occurredAt, PodFailureTime(&v1.Pod{}, occurredAt))
91+
assert.WithinDuration(t, time.Now(), PodFailureTime(&v1.Pod{}, time.Time{}), time.Minute)
92+
})
93+
}
94+
95+
// The anchor exists because GetLastTransitionOccurredAt cannot serve as one. For a pod
96+
// that failed while its containers were still running it reports the time the container
97+
// started, so a long-running task would be anchored hours before anything went wrong.
98+
func TestPodFailureTimeDoesNotAnchorOnAStartTime(t *testing.T) {
99+
startedAt := time.Now().Add(-6 * time.Hour)
100+
evictedAt := time.Now().Add(-2 * time.Minute)
101+
102+
deletion := metav1.NewTime(evictedAt)
103+
pod := &v1.Pod{
104+
ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &deletion},
105+
Status: v1.PodStatus{ContainerStatuses: []v1.ContainerStatus{runningSince(startedAt)}},
106+
}
107+
108+
assert.Equal(t, startedAt.Unix(), GetLastTransitionOccurredAt(pod).Unix())
109+
assert.Equal(t, evictedAt, PodFailureTime(pod, GetLastTransitionOccurredAt(pod).Time))
110+
}

flyteplugins/go/tasks/pluginmachinery/flytek8s/pod_helper.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1685,6 +1685,49 @@ func GetLastTransitionOccurredAt(pod *v1.Pod) metav1.Time {
16851685
return lastTransitionTime
16861686
}
16871687

1688+
// PodFailureTime is the time a pod's own trouble is anchored on, which is what a GPU
1689+
// fault's relevance is measured against (see gpufault.RelevantToFailure).
1690+
//
1691+
// A container's termination is stamped by the kubelet on the same node and clock as the
1692+
// fault events, so it is the closest thing to the moment a fault would have to explain. A
1693+
// pod on its way out without a terminated container is anchored on its deletion, which is
1694+
// what an eviction leaves behind.
1695+
//
1696+
// Only then does the time the caller already had stand in, and it is the last resort on
1697+
// purpose. Callers typically get it from GetLastTransitionOccurredAt above, which for a
1698+
// pod that failed while its containers were still running reports the time the container
1699+
// started, not the time anything went wrong. Anchoring a long-running task on its own
1700+
// start would put every real fault outside the window and quietly find nothing. Pass the
1701+
// zero time when there is no such time to offer, and the current time is used instead.
1702+
//
1703+
// Init containers are not eligible, which is where this parts company with
1704+
// GetLastTransitionOccurredAt. They finish before the workload starts, and a native
1705+
// sidecar declared among them is reaped after everything else, so either would anchor on
1706+
// a moment that has nothing to do with when the work died.
1707+
func PodFailureTime(pod *v1.Pod, occurredAt time.Time) time.Time {
1708+
latest := time.Time{}
1709+
for _, status := range pod.Status.ContainerStatuses {
1710+
terminated := status.State.Terminated
1711+
if terminated == nil || terminated.FinishedAt.IsZero() {
1712+
continue
1713+
}
1714+
if terminated.FinishedAt.After(latest) {
1715+
latest = terminated.FinishedAt.Time
1716+
}
1717+
}
1718+
1719+
switch {
1720+
case !latest.IsZero():
1721+
return latest
1722+
case pod.DeletionTimestamp != nil && !pod.DeletionTimestamp.IsZero():
1723+
return pod.DeletionTimestamp.Time
1724+
case !occurredAt.IsZero():
1725+
return occurredAt
1726+
default:
1727+
return time.Now()
1728+
}
1729+
}
1730+
16881731
func GetReportedAt(pod *v1.Pod) metav1.Time {
16891732
var reportedAt metav1.Time
16901733
for _, condition := range pod.Status.Conditions {

0 commit comments

Comments
 (0)