diff --git a/docs/kubernetes/index.md b/docs/kubernetes/index.md index f812a35eb..3af872385 100644 --- a/docs/kubernetes/index.md +++ b/docs/kubernetes/index.md @@ -494,6 +494,8 @@ For a BatchSandbox with multiple replicas, `Succeed` also does not mean that eve | `Resuming` | The controller is restoring runtime resources after a pause. | | `Failed` | The controller detected a sandbox runtime failure. Inspect conditions and Pod events for details. | +When a transient Pod failure clears, the controller returns `Failed` to `Succeed` only if the Pods recorded at failure time are still the same Kubernetes objects (matching UIDs) and are Running and Ready. A replacement Pod does not count as recovery, even if it reuses the same name. Lifecycle failures such as a failed resume remain terminal. + The controller records active conditions with `status: "True"`: | Condition | Meaning when `True` | diff --git a/kubernetes/apis/sandbox/v1alpha1/batchsandbox_types.go b/kubernetes/apis/sandbox/v1alpha1/batchsandbox_types.go index b4ee0560e..b85f7435c 100644 --- a/kubernetes/apis/sandbox/v1alpha1/batchsandbox_types.go +++ b/kubernetes/apis/sandbox/v1alpha1/batchsandbox_types.go @@ -18,6 +18,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" runtime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" ) // +kubebuilder:validation:Enum=Pending;Succeed;Pausing;Paused;Resuming;Failed @@ -177,6 +178,12 @@ type BatchSandboxStatus struct { // +optional Phase BatchSandboxPhase `json:"phase,omitempty"` + // FailedPodUIDs records the Pods whose transient runtime failures caused the + // current Failed phase. The controller uses these UIDs to distinguish an + // in-place recovery from a replacement Pod that reuses the same name. + // +optional + FailedPodUIDs []types.UID `json:"failedPodUIDs,omitempty"` + // PauseObservedGeneration is the generation most recently ACKed by the Controller // when entering pause/resume dispatch logic. Written immediately to prevent reentry (idempotent gating). // +optional diff --git a/kubernetes/apis/sandbox/v1alpha1/zz_generated.deepcopy.go b/kubernetes/apis/sandbox/v1alpha1/zz_generated.deepcopy.go index e3f6718d1..d812397f9 100644 --- a/kubernetes/apis/sandbox/v1alpha1/zz_generated.deepcopy.go +++ b/kubernetes/apis/sandbox/v1alpha1/zz_generated.deepcopy.go @@ -21,6 +21,7 @@ package v1alpha1 import ( "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" ) @@ -163,6 +164,11 @@ func (in *BatchSandboxSpec) DeepCopy() *BatchSandboxSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *BatchSandboxStatus) DeepCopyInto(out *BatchSandboxStatus) { *out = *in + if in.FailedPodUIDs != nil { + in, out := &in.FailedPodUIDs, &out.FailedPodUIDs + *out = make([]types.UID, len(*in)) + copy(*out, *in) + } if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]BatchSandboxCondition, len(*in)) diff --git a/kubernetes/charts/opensandbox-controller/templates/crds/batchsandboxes.yaml b/kubernetes/charts/opensandbox-controller/templates/crds/batchsandboxes.yaml index df6d978b7..e3d280db7 100644 --- a/kubernetes/charts/opensandbox-controller/templates/crds/batchsandboxes.yaml +++ b/kubernetes/charts/opensandbox-controller/templates/crds/batchsandboxes.yaml @@ -195,6 +195,18 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + failedPodUIDs: + description: |- + FailedPodUIDs records the Pods whose transient runtime failures caused the + current Failed phase. The controller uses these UIDs to distinguish an + in-place recovery from a replacement Pod that reuses the same name. + items: + description: |- + UID is a type that holds unique ID values, including UUIDs. Because we + don't ONLY use UUIDs, this is an alias to string. Being a type captures + intent and helps make sure that UIDs and names do not get conflated. + type: string + type: array observedGeneration: description: |- ObservedGeneration is the most recent generation observed for this BatchSandbox. It corresponds to the diff --git a/kubernetes/config/crd/bases/sandbox.opensandbox.io_batchsandboxes.yaml b/kubernetes/config/crd/bases/sandbox.opensandbox.io_batchsandboxes.yaml index 9311c132c..792277b31 100644 --- a/kubernetes/config/crd/bases/sandbox.opensandbox.io_batchsandboxes.yaml +++ b/kubernetes/config/crd/bases/sandbox.opensandbox.io_batchsandboxes.yaml @@ -186,6 +186,18 @@ spec: x-kubernetes-list-map-keys: - type x-kubernetes-list-type: map + failedPodUIDs: + description: |- + FailedPodUIDs records the Pods whose transient runtime failures caused the + current Failed phase. The controller uses these UIDs to distinguish an + in-place recovery from a replacement Pod that reuses the same name. + items: + description: |- + UID is a type that holds unique ID values, including UUIDs. Because we + don't ONLY use UUIDs, this is an alias to string. Being a type captures + intent and helps make sure that UIDs and names do not get conflated. + type: string + type: array observedGeneration: description: |- ObservedGeneration is the most recent generation observed for this BatchSandbox. It corresponds to the diff --git a/kubernetes/internal/controller/batchsandbox_pause_resume_test.go b/kubernetes/internal/controller/batchsandbox_pause_resume_test.go index 6e5f728dc..f5e8a95c3 100644 --- a/kubernetes/internal/controller/batchsandbox_pause_resume_test.go +++ b/kubernetes/internal/controller/batchsandbox_pause_resume_test.go @@ -1600,6 +1600,27 @@ func TestPersistRuntimeView_PreservesPauseFailedConditionFromLatestStatus(t *tes assert.True(t, foundPauseFailed, "persistRuntimeView should preserve PauseFailed condition once informer cache catches up") } +func TestUpdateStatus_ClearsPersistedFailedPodUIDs(t *testing.T) { + bs := &sandboxv1alpha1.BatchSandbox{ + ObjectMeta: metav1.ObjectMeta{Name: "test-bs", Namespace: "default"}, + Status: sandboxv1alpha1.BatchSandboxStatus{ + Phase: sandboxv1alpha1.BatchSandboxPhaseFailed, + FailedPodUIDs: []types.UID{"failed-pod-uid"}, + }, + } + r := newTestReconciler(bs) + + desiredStatus := bs.Status.DeepCopy() + desiredStatus.Phase = sandboxv1alpha1.BatchSandboxPhaseSucceed + desiredStatus.FailedPodUIDs = nil + require.NoError(t, r.updateStatus(context.Background(), bs, desiredStatus)) + + updated := &sandboxv1alpha1.BatchSandbox{} + require.NoError(t, r.Get(context.Background(), types.NamespacedName{Namespace: bs.Namespace, Name: bs.Name}, updated)) + assert.Equal(t, sandboxv1alpha1.BatchSandboxPhaseSucceed, updated.Status.Phase) + assert.Nil(t, updated.Status.FailedPodUIDs, "the merge patch must remove persisted failure provenance") +} + func TestPersistRuntimeView_SkipsStatusUpdateWhenRuntimeStatusUnchanged(t *testing.T) { transitionTime := metav1.NewTime(time.Now().Add(-5 * time.Minute)) bs := &sandboxv1alpha1.BatchSandbox{ @@ -1807,6 +1828,176 @@ func TestBuildRuntimeView_AggregatesPodFailuresInSteadyState(t *testing.T) { assert.Equal(t, "3/4 observed pods failed; primary reason=ErrImagePull; sample pod=err-image-0", podFailed.Message) } +func TestBuildRuntimeView_RecoversOnlySameFailedPod(t *testing.T) { + bs := &sandboxv1alpha1.BatchSandbox{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bs", + Namespace: "default", + }, + Status: sandboxv1alpha1.BatchSandboxStatus{ + Phase: sandboxv1alpha1.BatchSandboxPhasePending, + }, + } + failedPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bs-0", + Namespace: "default", + UID: types.UID("original-pod-uid"), + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "main"}}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "main", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{ + Reason: "CreateContainerConfigError", + Message: "temporary container creation failure", + }, + }, + }}, + }, + } + + failedView := buildRuntimeView(bs, []*corev1.Pod{failedPod}) + require.Equal(t, sandboxv1alpha1.BatchSandboxPhaseFailed, failedView.status.Phase) + require.Equal(t, []types.UID{failedPod.UID}, failedView.status.FailedPodUIDs) + + tests := []struct { + name string + podUID types.UID + mainContainerRunning bool + wantPhase sandboxv1alpha1.BatchSandboxPhase + }{ + { + name: "same Pod recovers", + podUID: failedPod.UID, + mainContainerRunning: true, + wantPhase: sandboxv1alpha1.BatchSandboxPhaseSucceed, + }, + { + name: "same Pod is Ready but main container is not running", + podUID: failedPod.UID, + mainContainerRunning: false, + wantPhase: sandboxv1alpha1.BatchSandboxPhaseFailed, + }, + { + name: "replacement Pod reuses name", + podUID: types.UID("replacement-pod-uid"), + mainContainerRunning: true, + wantPhase: sandboxv1alpha1.BatchSandboxPhaseFailed, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + afterFailure := bs.DeepCopy() + afterFailure.Status = *failedView.status.DeepCopy() + recoveredPod := failedPod.DeepCopy() + recoveredPod.UID = tt.podUID + mainContainerState := corev1.ContainerState{} + if tt.mainContainerRunning { + mainContainerState.Running = &corev1.ContainerStateRunning{} + } + recoveredPod.Status = corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "main", + State: mainContainerState, + }}, + } + + view := buildRuntimeView(afterFailure, []*corev1.Pod{recoveredPod}) + assert.Equal(t, tt.wantPhase, view.status.Phase) + if tt.wantPhase == sandboxv1alpha1.BatchSandboxPhaseSucceed { + assert.Empty(t, view.status.FailedPodUIDs) + } else { + assert.Equal(t, []types.UID{failedPod.UID}, view.status.FailedPodUIDs) + } + }) + } + + t.Run("failure without recorded Pod identity remains terminal", func(t *testing.T) { + afterFailure := bs.DeepCopy() + afterFailure.Status = *failedView.status.DeepCopy() + afterFailure.Status.FailedPodUIDs = nil + recoveredPod := failedPod.DeepCopy() + recoveredPod.Status = corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "main", + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }}, + } + + view := buildRuntimeView(afterFailure, []*corev1.Pod{recoveredPod}) + assert.Equal(t, sandboxv1alpha1.BatchSandboxPhaseFailed, view.status.Phase) + }) +} + +func TestBuildRuntimeView_DoesNotRecoverResumeFailure(t *testing.T) { + bs := &sandboxv1alpha1.BatchSandbox{ + ObjectMeta: metav1.ObjectMeta{Name: "test-bs", Namespace: "default"}, + Status: sandboxv1alpha1.BatchSandboxStatus{ + Phase: sandboxv1alpha1.BatchSandboxPhaseResuming, + }, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bs-0", + Namespace: "default", + UID: types.UID("original-pod-uid"), + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{Name: "main"}}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "main", + State: corev1.ContainerState{ + Waiting: &corev1.ContainerStateWaiting{Reason: "ImagePullBackOff"}, + }, + }}, + }, + } + + failedView := buildRuntimeView(bs, []*corev1.Pod{pod}) + require.Equal(t, sandboxv1alpha1.BatchSandboxPhaseFailed, failedView.status.Phase) + require.Empty(t, failedView.status.FailedPodUIDs) + + afterFailure := bs.DeepCopy() + afterFailure.Status = *failedView.status.DeepCopy() + pod.Status = corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{{ + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }}, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "main", + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }}, + } + + view := buildRuntimeView(afterFailure, []*corev1.Pod{pod}) + assert.Equal(t, sandboxv1alpha1.BatchSandboxPhaseFailed, view.status.Phase) +} + func TestBuildRuntimeView_AggregatesResumeFailures(t *testing.T) { bs := &sandboxv1alpha1.BatchSandbox{ ObjectMeta: metav1.ObjectMeta{ diff --git a/kubernetes/internal/controller/batchsandbox_status.go b/kubernetes/internal/controller/batchsandbox_status.go index 0427df935..5f7166633 100644 --- a/kubernetes/internal/controller/batchsandbox_status.go +++ b/kubernetes/internal/controller/batchsandbox_status.go @@ -125,6 +125,7 @@ type podFailureSummary struct { failed int primaryReason string samplePod string + podUIDs []types.UID } func summarizePodFailures(pods []*corev1.Pod) (podFailureSummary, bool) { @@ -140,6 +141,9 @@ func summarizePodFailures(pods []*corev1.Pod) (podFailureSummary, bool) { } summary.failed++ + if pod.UID != "" { + summary.podUIDs = append(summary.podUIDs, pod.UID) + } if _, exists := firstPodByReason[reason]; !exists { firstPodByReason[reason] = pod.Name } @@ -203,28 +207,64 @@ func applyResumingRuntimePhase(status *sandboxv1alpha1.BatchSandboxStatus, pods if summary, hasFailures := summarizePodFailures(pods); hasFailures { setConditionInStatus(status, sandboxv1alpha1.BatchSandboxConditionResumeFailed, sandboxv1alpha1.ConditionTrue, summary.primaryReason, summary.message(true)) setConditionInStatus(status, sandboxv1alpha1.BatchSandboxConditionPodFailed, sandboxv1alpha1.ConditionTrue, summary.primaryReason, summary.message(false)) + status.FailedPodUIDs = nil status.Phase = sandboxv1alpha1.BatchSandboxPhaseFailed return } if status.Ready > 0 { status.Phase = sandboxv1alpha1.BatchSandboxPhaseSucceed + status.FailedPodUIDs = nil setConditionInStatus(status, sandboxv1alpha1.BatchSandboxConditionPodFailed, sandboxv1alpha1.ConditionFalse, "", "") } } +func failedPodsRecovered(failedPodUIDs []types.UID, pods []*corev1.Pod) bool { + // Without provenance, recovery is unsafe because a replacement Pod may reuse the same name. + if len(failedPodUIDs) == 0 { + return false + } + + recoveredUIDs := make(map[types.UID]struct{}, len(failedPodUIDs)) + for _, pod := range pods { + if pod.DeletionTimestamp != nil || !utils.IsPodReady(pod) || len(pod.Spec.Containers) == 0 { + continue + } + + // The first container is the sandbox's main runtime container by convention. + mainContainerName := pod.Spec.Containers[0].Name + for _, containerStatus := range pod.Status.ContainerStatuses { + if containerStatus.Name == mainContainerName && containerStatus.State.Running != nil { + recoveredUIDs[pod.UID] = struct{}{} + break + } + } + } + + for _, uid := range failedPodUIDs { + if _, recovered := recoveredUIDs[uid]; !recovered { + return false + } + } + return true +} + func applySteadyRuntimePhase(batchSbx *sandboxv1alpha1.BatchSandbox, status *sandboxv1alpha1.BatchSandboxStatus, pods []*corev1.Pod) { if summary, hasFailures := summarizePodFailures(pods); hasFailures { if batchSbx.Status.Phase != sandboxv1alpha1.BatchSandboxPhaseFailed { setConditionInStatus(status, sandboxv1alpha1.BatchSandboxConditionPodFailed, sandboxv1alpha1.ConditionTrue, summary.primaryReason, summary.message(false)) + status.FailedPodUIDs = append([]types.UID(nil), summary.podUIDs...) status.Phase = sandboxv1alpha1.BatchSandboxPhaseFailed } return } if status.Phase == sandboxv1alpha1.BatchSandboxPhaseFailed { - return + if !failedPodsRecovered(status.FailedPodUIDs, pods) { + return + } } + status.FailedPodUIDs = nil setConditionInStatus(status, sandboxv1alpha1.BatchSandboxConditionPodFailed, sandboxv1alpha1.ConditionFalse, "", "") if status.Ready > 0 { status.Phase = sandboxv1alpha1.BatchSandboxPhaseSucceed @@ -314,7 +354,20 @@ func (r *BatchSandboxReconciler) updateStatus(ctx context.Context, batchSandbox log := logf.FromContext(ctx) mergedStatus := newStatus.DeepCopy() mergedStatus.Conditions = mergeLifecycleConditions(mergedStatus.Conditions, batchSandbox.Status.Conditions) - patchData, err := json.Marshal(map[string]any{"status": mergedStatus}) + statusPatch := make(map[string]any) + statusJSON, err := json.Marshal(mergedStatus) + if err != nil { + return fmt.Errorf("failed to marshal status patch: %w", err) + } + if err := json.Unmarshal(statusJSON, &statusPatch); err != nil { + return fmt.Errorf("failed to unmarshal status patch: %w", err) + } + // JSON merge patches preserve fields omitted by omitempty. Explicitly clear + // recorded failure provenance when the desired status no longer has it. + if len(batchSandbox.Status.FailedPodUIDs) > 0 && len(mergedStatus.FailedPodUIDs) == 0 { + statusPatch["failedPodUIDs"] = nil + } + patchData, err := json.Marshal(map[string]any{"status": statusPatch}) if err != nil { return fmt.Errorf("failed to marshal status patch: %w", err) }