[flyteplugins]: require a fresh LastReconcileTime for operator liveness in the kfoperator stale-CR check

[pfernandes21]

Tracking issue

Stacked on #24 (targets its branch). Addresses the review finding that a one-time lastReconcileTime becomes permanent liveness.

Why are the changes needed?

#24 makes OperatorNeverReconciled return false whenever status.lastReconcileTime != nil. That only proves the operator saw the job once. If the operator writes lastReconcileTime and then dies or wedges, the stale-CR check never fires again for that job, so a queued gang job sits in Queued indefinitely instead of failing after kf-operator.timeout.

The training operator (v1.8.0, pkg/controller.v1/common/job.go) sets lastReconcileTime only in the gang-wait branch (DelayPodCreationDueToPodGroup), and the status write re-enqueues the job, so a live operator keeps refreshing it every reconcile while the PodGroup waits. Freshness is therefore a valid liveness signal: recent timestamp = operator alive, frozen timestamp = operator gone.

What changes were proposed in this pull request?

// before (#24)
func OperatorNeverReconciled(status JobStatus) bool {
	return status.StartTime == nil && status.LastReconcileTime == nil
}
// callers: OperatorNeverReconciled(app.Status) && CreationTimestamp+timeout < now

// after
func OperatorStale(created meta_v1.Time, status JobStatus, timeout time.Duration, now time.Time) bool {
	if status.StartTime != nil { return false }
	lastSeen := max(created, status.LastReconcileTime)
	return lastSeen + timeout < now
}
// callers: OperatorStale(app.CreationTimestamp, app.Status, timeout, time.Now())

Semantics: with no StartTime, the job is stale when neither creation nor the latest reconcile happened within timeout. A lastReconcileTime older than creation (clock skew) never shortens the grace period. Behaviour for StartTime != nil, suspended jobs, and never-reconciled jobs is unchanged from #24.

How was this patch tested?

• TestOperatorStale covers: never reconciled (inside/outside timeout), StartTime set, fresh lastReconcileTime past creation timeout (Queued), lastReconcileTime exactly inside the window, stale lastReconcileTime (fails), skewed lastReconcileTime older than creation.
• TestGetTaskPhase in mpi/pytorch/tensorflow: existing gang-wait case now uses a fresh (-1s) timestamp, plus a new "operator reconciled once, then went away" case asserting the kubeflow operator hasn't updated error.
• go test ./go/tasks/plugins/k8s/kfoperators/... passes; gofmt -l clean.

Labels

fixed

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

#24

Link to Devin session: https://app.devin.ai/sessions/72abcac0dc9d453ea9989e647eebfa32
Open in Devin Desktop: https://app.devin.ai/desktop/session/72abcac0dc9d453ea9989e647eebfa32?variant=devin
Requested by: @pfernandes21

[devin-ai-integration]

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

• Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
• Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

• Disable automatic comment, CI, and merge conflict monitoring

[devin-ai-integration]

Heron review (relayed)

Heron reviewed head 152469f but could not submit a GitHub review on this repo (403 Resource not accessible by integration), so the text is relayed verbatim below. Session: https://heron.internal.exa.ai/sessions/3cfd7768-6154-48e7-b274-5c2d77763baf

---

Review decision: Request changes

Inline — common/common_operator.go:34 (blocking)

training-operator v1.8.0 does not guarantee that this timestamp keeps refreshing. The gang-wait path writes metav1.Now() and returns an empty ctrl.Result; its only self-trigger is the status-update watch. However, metav1.Time serializes at whole-second precision. If the status-triggered reconcile runs in the same second, it writes identical serialized status, Kubernetes short-circuits the update as a no-op, and no further watch event is emitted. There is no RequeueAfter here. PodGroup updates may cause additional reconciles, but they provide no guaranteed cadence either.

A healthy operator can therefore leave lastReconcileTime frozen after one or two reconciles, and this predicate will fail a still-waiting Volcano job once timeout elapses—the original failure mode. Please use a liveness signal with a guaranteed cadence, or add such a cadence upstream, rather than aging this field. The tests inject fresh timestamps and therefore cannot expose the missing refresh loop.

Inline — common/common_operator.go:37 (edge case)

startTime does not always mean replica pods were created. When a suspended job is resumed, v1.8.0 ReconcileJobs sets StartTime before reaching the PodGroup admission gate (job.go:167–172 versus 322–334). A pre-start suspended gang job can therefore be resumed, remain held with no pods, and become permanently exempt here if the operator then dies. Please account for that lifecycle or narrow the documented guarantee; add a test modeling an old resumed job rather than only appending a Running condition to a fresh fixture.

Summary

The max(creationTimestamp, lastReconcileTime) calculation is internally consistent, and the direct unit cases cover its intended boundary behavior, but the central heartbeat assumption is invalid, so this is not safe to merge yet.

Other requested edges checked:

• Suspended CRs are skipped while spec.runPolicy.suspend=true. On resume, before the operator observes the spec change, the stale check can also race against the old creation timestamp and fail immediately; this behavior predates this PR.
• With gang scheduling disabled, v1.8.0 never writes lastReconcileTime; successful PyTorch/TF/MPI reconciliation sets startTime, so prior behavior is preserved.
• Taking the later timestamp protects only the tested early-skew case where reconcile time predates creation. Ongoing negative skew larger than the timeout can falsely report staleness; positive skew extends detection by the skew.
• controller-runtime v0.17.2 error retries use exponential backoff capped at 1000s, well above the default 1m timeout, so even explicit error requeues cannot guarantee the assumed refresh cadence.
• The flyteplugins unit-test CI passed at 152469f. The two red checks appear unrelated: pre-existing generated flytepropeller flags and dependency graph being disabled.

[devin-ai-integration]

Closing per @pfernandes21: Heron's blocking finding above holds — v1.8.0 does not keep refreshing lastReconcileTime while a gang waits (second-precision timestamp → identical status → no-op update → no further reconcile), so aging it would re-introduce the false-positive that #24 fixes. Keeping #24's non-nil check as-is.