Rewrite BatchSandbox pod-failure recovery on current main (supersedes #1523)

[Pangjiping]

Summary

#1497 is still reproducible on main. #1523 attempted a fix but has been closed: it drifted ~535 commits behind main, was in a conflicting state, and its CI never ran beyond auto-label, so the 218 lines of new tests were never executed. This issue tracks a clean rewrite against current main, preserving the parts of #1523 that were reviewed and validated.

The defect on main today

kubernetes/internal/controller/batchsandbox_status.go:362, inside applySteadyRuntimePhase:

if status.Phase == sandboxv1alpha1.BatchSandboxPhaseFailed {
    return
}

Once a BatchSandbox reaches Failed, the steady-state path returns unconditionally. If the same Pod (same UID) later recovers to Running and Ready, the phase is never recomputed and the BatchSandbox stays Failed forever. This is exactly the scenario reported in #1497.

Prior art from #1523 worth keeping

These were reviewed and confirmed correct; they should carry over rather than be redesigned:

• Recovery keyed on Pod UID, not name. A replacement Pod can reuse the same name, so name-based recovery is unsafe. fix(kubernetes): recover BatchSandbox after Pod restart #1523 recorded the failing UIDs in a new status.failedPodUIDs field and only allowed recovery when every recorded UID was observed Ready with its main container Running.
• Clearing provenance on resume failure stays terminal. applyResumingRuntimePhase deliberately clears the recorded UIDs so a resume failure cannot later be reinterpreted as a recovered transient failure.
• Explicit null in the merge patch. updateStatus (batchsandbox_status.go:480) marshals the desired status and sends it as types.MergePatchType. Because the field is omitempty, assigning nil in Go omits the key instead of clearing it server-side. fix(kubernetes): recover BatchSandbox after Pod restart #1523 handled this by marshalling to map[string]any and setting statusPatch["failedPodUIDs"] = nil when the stored status has UIDs and the desired one does not. This part of updateStatus is unchanged on main, so the approach still applies.

Constraints the rewrite must satisfy

1. Collect provenance in summarizePodFailuresWith, not in summarizePodFailures. main refactored the summarizer into a shared summarizePodFailuresWith(pods, detectFailure, includeSampleDetail) (batchsandbox_status.go:238) with two detectors: summarizePodFailures and summarizeTerminalPodFailures (:234). applySteadyRuntimePhase routes the ""/Pending phase through the terminal detector. If UID collection is added only to the non-terminal path, a failure detected by the terminal path records no UIDs, recovery can never be established, and the BatchSandbox becomes permanently pinned in Failed — strictly worse than the current behaviour.

2. Account for the reordered condition writes. In the failure branch, main now sets PodFailed before ResumeFailed, and adds the terminal-summary branch at the top of the function. The FailedPodUIDs assignment has to be re-derived against this shape, not merged textually.

3. CRD parity is generated, not hand-edited. kubernetes/charts/ no longer exists; charts moved to manifests/charts/. After adding the field to the Go type and regenerating kubernetes/config/crd/bases/, run make -C manifests helm-gen-crds to sync manifests/charts/base/files/crds.yaml. That bundle currently contains pauseObservedGeneration but no equivalent for this field, so a hand-edit is both unnecessary and insufficient.

4. CI must actually run. Go unit tests and lint need to execute on the PR before review. Regressions here are silent: a wrong detector path produces a permanently Failed BatchSandbox with no error surfaced.

Acceptance criteria

• A BatchSandbox that entered Failed due to a transient Pod failure returns to Succeed once the same Pod (same UID) is Ready with its main container Running, covering both the normal and terminal failure detectors.
• A replacement Pod that reuses the name of a failed Pod does not trigger recovery.
• A resume failure remains terminal and is not later reinterpreted as recovered.
• status.failedPodUIDs is cleared server-side (observable as null in the merge patch, not merely absent) whenever the desired status no longer carries it.
• make -C manifests helm-gen-crds output is committed and matches the generated CRD bases.

References

• BatchSandbox remains Failed after the same Pod recovers to Running and Ready #1497 — the original bug report (still open)
• fix(kubernetes): recover BatchSandbox after Pod restart #1523 — closed prior attempt, source of the validated design points above

[hpliStartAgain]

Hi, I'd like to work on this rewrite.

I'll re-implement the recovery path against current main rather than porting #1523 textually, while preserving the reviewed invariants from the previous attempt: UID-based recovery provenance, terminal resume-failure semantics, and explicit null clearing in the status merge patch.

I'll also cover both failure-detector paths in regression tests, regenerate the CRDs through the current generation flow, sync the Helm CRD bundle, and make sure the Kubernetes unit/lint CI actually runs before opening the PR.