From 0b47a378611e4aaf3cab9ce87a6f52dcf92d132e Mon Sep 17 00:00:00 2001 From: Yuan Wang Date: Mon, 10 Nov 2025 09:39:20 +0000 Subject: [PATCH] Keep pod in running state and prune past container status from runtime --- pkg/kubelet/container/helpers.go | 48 +++---- pkg/kubelet/kubelet_pods.go | 88 ++++++++----- pkg/kubelet/kubelet_pods_test.go | 61 +++++---- .../kuberuntime/kuberuntime_manager.go | 57 ++++---- .../kuberuntime/kuberuntime_manager_test.go | 122 +++++++++++++----- test/e2e/node/pods.go | 86 +++++++++++- test/e2e_node/restart_all_containers_test.go | 2 +- 7 files changed, 308 insertions(+), 156 deletions(-) diff --git a/pkg/kubelet/container/helpers.go b/pkg/kubelet/container/helpers.go index b0e2d6b8482..ddafc82434c 100644 --- a/pkg/kubelet/container/helpers.go +++ b/pkg/kubelet/container/helpers.go @@ -130,51 +130,38 @@ func ShouldAllContainersRestart(pod *v1.Pod, podStatus *PodStatus, apiPodStatus } } - for _, c := range pod.Spec.InitContainers { - if podStatus != nil { - status := podStatus.FindContainerStatusByName(c.Name) - if status == nil || status.State != ContainerStateExited { - continue - } - exitCode := int32(status.ExitCode) - rule, ok := podutil.FindMatchingContainerRestartRule(c, exitCode) - if ok && rule.Action == v1.ContainerRestartRuleActionRestartAllContainers { - return true - } + nameToAPIStatus := make(map[string]*v1.ContainerStatus) + if apiPodStatus != nil { + for i := range apiPodStatus.InitContainerStatuses { + nameToAPIStatus[apiPodStatus.InitContainerStatuses[i].Name] = &apiPodStatus.InitContainerStatuses[i] } - - if apiPodStatus != nil { - apiStatus, ok := podutil.GetContainerStatus(apiPodStatus.InitContainerStatuses, c.Name) - if !ok || apiStatus.State.Terminated == nil { - continue - } - exitCode := apiStatus.State.Terminated.ExitCode - rule, ok := podutil.FindMatchingContainerRestartRule(c, exitCode) - if ok && rule.Action == v1.ContainerRestartRuleActionRestartAllContainers { - return true - } + for i := range apiPodStatus.ContainerStatuses { + nameToAPIStatus[apiPodStatus.ContainerStatuses[i].Name] = &apiPodStatus.ContainerStatuses[i] } } - for _, c := range pod.Spec.Containers { + + for c := range podutil.ContainerIter(&pod.Spec, podutil.InitContainers|podutil.Containers) { + if c == nil { + continue + } if podStatus != nil { status := podStatus.FindContainerStatusByName(c.Name) if status == nil || status.State != ContainerStateExited { continue } exitCode := int32(status.ExitCode) - rule, ok := podutil.FindMatchingContainerRestartRule(c, exitCode) + rule, ok := podutil.FindMatchingContainerRestartRule(*c, exitCode) if ok && rule.Action == v1.ContainerRestartRuleActionRestartAllContainers { return true } } - if apiPodStatus != nil { - apiStatus, ok := podutil.GetContainerStatus(apiPodStatus.ContainerStatuses, c.Name) + apiStatus, ok := nameToAPIStatus[c.Name] if !ok || apiStatus.State.Terminated == nil { continue } exitCode := apiStatus.State.Terminated.ExitCode - rule, ok := podutil.FindMatchingContainerRestartRule(c, exitCode) + rule, ok := podutil.FindMatchingContainerRestartRule(*c, exitCode) if ok && rule.Action == v1.ContainerRestartRuleActionRestartAllContainers { return true } @@ -187,12 +174,7 @@ func ShouldAllContainersRestart(pod *v1.Pod, podStatus *PodStatus, apiPodStatus // AllContainersRestartCleanedUp returns true if all containers are removed // from the runtime and podStatus. func AllContainersRestartCleanedUp(pod *v1.Pod, podStatus *PodStatus) bool { - for _, initC := range pod.Spec.InitContainers { - if podStatus.FindContainerStatusByName(initC.Name) != nil { - return false - } - } - for _, c := range pod.Spec.Containers { + for c := range podutil.ContainerIter(&pod.Spec, podutil.Containers|podutil.InitContainers) { if podStatus.FindContainerStatusByName(c.Name) != nil { return false } diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index ec32771748e..3e4d1de1ea1 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -81,8 +81,9 @@ const ( // Container state reason list const ( - PodInitializing = "PodInitializing" - ContainerCreating = "ContainerCreating" + PodInitializing = "PodInitializing" + ContainerCreating = "ContainerCreating" + RestartingAllContainers = "RestartingAllContainers" kubeletUser = "kubelet" ) @@ -1664,7 +1665,11 @@ func getPhase(pod *v1.Pod, info []v1.ContainerStatus, podIsTerminal, podHasIniti if exitCode != 0 { failedInitialization++ if utilfeature.DefaultFeatureGate.Enabled(features.ContainerRestartRules) { - if !podutil.ContainerShouldRestart(container, pod.Spec, exitCode) { + restartable := podutil.ContainerShouldRestart(container, pod.Spec, exitCode) + if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { + restartable = restartable || containerStatus.State.Terminated.Reason == RestartingAllContainers + } + if !restartable { failedInitializationNotRestartable++ } } @@ -1674,10 +1679,12 @@ func getPhase(pod *v1.Pod, info []v1.ContainerStatus, podIsTerminal, podHasIniti exitCode := containerStatus.LastTerminationState.Terminated.ExitCode if exitCode != 0 { failedInitialization++ - if utilfeature.DefaultFeatureGate.Enabled(features.ContainerRestartRules) { - if !podutil.ContainerShouldRestart(container, pod.Spec, exitCode) { - failedInitializationNotRestartable++ - } + restartable := podutil.ContainerShouldRestart(container, pod.Spec, exitCode) + if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { + restartable = restartable || containerStatus.LastTerminationState.Terminated.Reason == RestartingAllContainers + } + if !restartable { + failedInitializationNotRestartable++ } } } else { @@ -1745,7 +1752,11 @@ func getPhase(pod *v1.Pod, info []v1.ContainerStatus, podIsTerminal, podHasIniti stopped++ exitCode := containerStatus.State.Terminated.ExitCode if utilfeature.DefaultFeatureGate.Enabled(features.ContainerRestartRules) { - if !podutil.ContainerShouldRestart(container, pod.Spec, exitCode) { + restartable := podutil.ContainerShouldRestart(container, pod.Spec, exitCode) + if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { + restartable = restartable || containerStatus.State.Terminated.Reason == RestartingAllContainers + } + if !restartable { stoppedNotRestartable++ } } @@ -1757,7 +1768,11 @@ func getPhase(pod *v1.Pod, info []v1.ContainerStatus, podIsTerminal, podHasIniti stopped++ if utilfeature.DefaultFeatureGate.Enabled(features.ContainerRestartRules) { exitCode := containerStatus.LastTerminationState.Terminated.ExitCode - if !podutil.ContainerShouldRestart(container, pod.Spec, exitCode) { + restartable := podutil.ContainerShouldRestart(container, pod.Spec, exitCode) + if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { + restartable = restartable || containerStatus.LastTerminationState.Terminated.Reason == RestartingAllContainers + } + if !restartable { stoppedNotRestartable++ } } @@ -1970,7 +1985,9 @@ func (kl *Kubelet) generateAPIPodStatus(pod *v1.Pod, podStatus *kubecontainer.Po Status: v1.ConditionTrue, }) if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { - s.Conditions = append(s.Conditions, status.GenerateAllContainersRestartingCondition(pod, podStatus, &oldPodStatus, s.Phase)) + if podutil.AllContainersCouldRestart(&pod.Spec) { + s.Conditions = append(s.Conditions, status.GenerateAllContainersRestartingCondition(pod, podStatus, &oldPodStatus, s.Phase)) + } } // set HostIP/HostIPs and initialize PodIP/PodIPs for host network pods if kl.kubeClient != nil { @@ -2161,29 +2178,26 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon case cs.State == kubecontainer.ContainerStateUnknown && oldStatus != nil && // we have an old status oldStatus.State.Running != nil: // our previous status was running - // If the pod is restarting, consider the container as pending + + reason := kubecontainer.ContainerReasonStatusUnknown + // If the pod is restarting, the reason should be RestartingAllContainers if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) && podRestarting { - status.State.Terminated = nil - status.State.Waiting = &v1.ContainerStateWaiting{ - Reason: kubecontainer.ContainerReasonStatusUnknown, - Message: "container removed during pod restart", - } - status.RestartCount = oldStatus.RestartCount + 1 - } else { - // if this happens, then we know that this container was previously running and isn't anymore (assuming the CRI isn't failing to return running containers). - // you can imagine this happening in cases where a container failed and the kubelet didn't ask about it in time to see the result. - // in this case, the container should not to into waiting state immediately because that can make cases like runonce pods actually run - // twice. "container never ran" is different than "container ran and failed". This is handled differently in the kubelet - // and it is handled differently in higher order logic like crashloop detection and handling - status.State.Terminated = &v1.ContainerStateTerminated{ - Reason: kubecontainer.ContainerReasonStatusUnknown, - Message: "The container could not be located when the pod was terminated", - ExitCode: 137, // this code indicates an error - } - // the restart count normally comes from the CRI (see near the top of this method), but since this is being added explicitly - // for the case where the CRI did not return a status, we need to manually increment the restart count to be accurate. - status.RestartCount = oldStatus.RestartCount + 1 + reason = RestartingAllContainers } + // if this happens, then we know that this container was previously running and isn't anymore (assuming the CRI isn't failing to return running containers). + // you can imagine this happening in cases where a container failed and the kubelet didn't ask about it in time to see the result. + // in this case, the container should not to into waiting state immediately because that can make cases like runonce pods actually run + // twice. "container never ran" is different than "container ran and failed". This is handled differently in the kubelet + // and it is handled differently in higher order logic like crashloop detection and handling + status.State.Terminated = &v1.ContainerStateTerminated{ + Reason: reason, + Message: "The container could not be located when the pod was terminated", + ExitCode: 137, // this code indicates an error + } + // the restart count normally comes from the CRI (see near the top of this method), but since this is being added explicitly + // for the case where the CRI did not return a status, we need to manually increment the restart count to be accurate. + status.RestartCount = oldStatus.RestartCount + 1 + default: // this collapses any unknown state to container waiting. If any container is waiting, then the pod status moves to pending even if it is running. // if I'm reading this correctly, then any failure to read status on any container results in the entire pod going pending even if the containers @@ -2365,16 +2379,20 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *v1.Pod, podStatus *kubecon } // If the container is missing, RestartAllContainers in place, and previous status is not waiting, then the container - // is removed from runtime. It should be considered Waiting, with no LastTerminationState, to avoid confusions - // after pod restart. + // is removed from runtime. It should be considered Waiting. The LastTerminationState should have a "RestartingAllContainers" + // reason to avoid confusion when containers are restarted. if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) && podRestarting && oldStatus.State.Waiting == nil { status := statuses[container.Name] status.State.Waiting = &v1.ContainerStateWaiting{ - Reason: kubecontainer.ContainerReasonStatusUnknown, + Reason: RestartingAllContainers, Message: "The container is removed because RestartAllContainers in place", } status.State.Terminated = nil - status.State.Running = nil + status.LastTerminationState.Terminated = &v1.ContainerStateTerminated{ + Reason: RestartingAllContainers, + Message: "The container is removed because RestartAllContainers in place", + ExitCode: 137, + } status.RestartCount = oldStatus.RestartCount + 1 statuses[container.Name] = status continue diff --git a/pkg/kubelet/kubelet_pods_test.go b/pkg/kubelet/kubelet_pods_test.go index f8890050e79..7678b70c715 100644 --- a/pkg/kubelet/kubelet_pods_test.go +++ b/pkg/kubelet/kubelet_pods_test.go @@ -2092,15 +2092,22 @@ func waitingStateWithNonZeroTermination(cName string) v1.ContainerStatus { }, } } -func waitingStateWithPodRestart(cName string) v1.ContainerStatus { +func waitingStateWithRestartingAllContainers(cName string) v1.ContainerStatus { return v1.ContainerStatus{ Name: cName, State: v1.ContainerState{ Waiting: &v1.ContainerStateWaiting{ - Reason: kubecontainer.ContainerReasonStatusUnknown, + Reason: RestartingAllContainers, Message: "The container is removed because RestartAllContainers in place", }, }, + LastTerminationState: v1.ContainerState{ + Terminated: &v1.ContainerStateTerminated{ + Reason: RestartingAllContainers, + Message: "The container is removed because RestartAllContainers in place", + ExitCode: 137, + }, + }, } } func runningState(cName string) v1.ContainerStatus { @@ -3569,7 +3576,7 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { statuses []v1.ContainerStatus expectedPhase v1.PodPhase }{ - // Triggere RestartAllContainers + // Trigger RestartAllContainers { name: "regular container triggers RestartAllContainers", spec: &v1.PodSpec{ @@ -3589,6 +3596,7 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { }, statuses: []v1.ContainerStatus{ failedStateWithExitCode("container", 42), + waitingState("regular"), }, expectedPhase: v1.PodPending, }, @@ -3627,9 +3635,9 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { RestartPolicy: v1.RestartPolicyNever, }, statuses: []v1.ContainerStatus{ - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), }, - expectedPhase: v1.PodPending, + expectedPhase: v1.PodRunning, }, { name: "init container triggers RestartAllContainers, cleaned up", @@ -3638,7 +3646,8 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { RestartPolicy: v1.RestartPolicyNever, }, statuses: []v1.ContainerStatus{ - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), + waitingState("regular"), }, expectedPhase: v1.PodPending, }, @@ -3650,7 +3659,7 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { RestartPolicy: v1.RestartPolicyNever, }, statuses: []v1.ContainerStatus{ - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), waitingState("regular"), }, expectedPhase: v1.PodPending, @@ -3663,8 +3672,8 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { RestartPolicy: v1.RestartPolicyNever, }, statuses: []v1.ContainerStatus{ - waitingStateWithPodRestart("container"), - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), + waitingStateWithRestartingAllContainers("container"), waitingState("regular"), }, expectedPhase: v1.PodPending, @@ -3679,9 +3688,9 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { }, statuses: []v1.ContainerStatus{ succeededState("init"), - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), }, - expectedPhase: v1.PodPending, + expectedPhase: v1.PodRunning, }, { name: "regular container triggered RestartAllContainers; init container failed", @@ -3692,7 +3701,7 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { }, statuses: []v1.ContainerStatus{ failedState("init"), - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), }, expectedPhase: v1.PodFailed, }, @@ -3730,10 +3739,10 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { RestartPolicy: v1.RestartPolicyNever, }, statuses: []v1.ContainerStatus{ - waitingStateWithPodRestart("container"), - waitingStateWithPodRestart("regular"), + waitingStateWithRestartingAllContainers("container"), + waitingStateWithRestartingAllContainers("regular"), }, - expectedPhase: v1.PodPending, + expectedPhase: v1.PodRunning, }, { name: "sidecar container triggered RestartAllContainers kills regular container; sidecar running", @@ -3744,9 +3753,9 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { }, statuses: []v1.ContainerStatus{ runningState("container"), - waitingStateWithPodRestart("regular"), + waitingStateWithRestartingAllContainers("regular"), }, - expectedPhase: v1.PodPending, + expectedPhase: v1.PodRunning, }, { name: "sidecar container triggered RestartAllContainers; kills init container; sidecar running", @@ -3757,7 +3766,7 @@ func TestPodPhaseWithRestartAllContainers(t *testing.T) { }, statuses: []v1.ContainerStatus{ runningState("container"), - waitingStateWithPodRestart("container"), + waitingStateWithRestartingAllContainers("container"), waitingState("regular"), }, expectedPhase: v1.PodPending, @@ -3943,7 +3952,7 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { podRestarting: true, expected: []v1.ContainerStatus{ failedStateWithExitCode("containerA", 137), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, { @@ -3959,8 +3968,8 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { containers: desiredState.Containers, podRestarting: true, expected: []v1.ContainerStatus{ - withRestartCount(waitingStateWithPodRestart("containerA"), 1), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerA"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, { @@ -3976,8 +3985,8 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { containers: desiredState.Containers, podRestarting: true, expected: []v1.ContainerStatus{ - withRestartCount(waitingStateWithPodRestart("containerA"), 1), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerA"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, { @@ -3999,7 +4008,7 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { podRestarting: true, expected: []v1.ContainerStatus{ withRestartCount(runningState("containerA"), 1), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, { @@ -4022,7 +4031,7 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { podRestarting: true, expected: []v1.ContainerStatus{ withRestartCount(succeededState("containerA"), 1), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, { @@ -4045,7 +4054,7 @@ func TestConvertToAPIContainerStatuses(t *testing.T) { podRestarting: true, expected: []v1.ContainerStatus{ withRestartCount(failedStateWithExitCode("containerA", 1), 1), - withRestartCount(waitingStateWithPodRestart("containerB"), 1), + withRestartCount(waitingStateWithRestartingAllContainers("containerB"), 1), }, }, } diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager.go b/pkg/kubelet/kuberuntime/kuberuntime_manager.go index ec5ddb75e29..e573af2afca 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager.go @@ -552,8 +552,6 @@ type containerToUpdateInfo struct { type containerToRemoveInfo struct { // The ID of the container. containerID kubecontainer.ContainerID - // The name of the container - name string // The spec of the container. container *v1.Container // Whether to kill the container before removal. @@ -592,13 +590,15 @@ type podActions struct { ContainersToUpdate map[v1.ResourceName][]containerToUpdateInfo // UpdatePodResources is true if container(s) need resource update with restart UpdatePodResources bool - // ContainersToRemove is a list of containers to be removed for RestartAllContainers. - ContainersToRemove []containerToRemoveInfo + // ContainersToReset is a list of containers to be killed (if running) and removed from + // runtime for RestartAllContainers. The container that triggered RestartAllContainers + // will be reset the last. + ContainersToReset []containerToRemoveInfo } func (p podActions) String() string { return fmt.Sprintf("KillPod: %t, CreateSandbox: %t, UpdatePodResources: %t, Attempt: %d, InitContainersToStart: %v, ContainersToStart: %v, EphemeralContainersToStart: %v,ContainersToUpdate: %v, ContainersToKill: %v, ContainersToRemove: %v", - p.KillPod, p.CreateSandbox, p.UpdatePodResources, p.Attempt, p.InitContainersToStart, p.ContainersToStart, p.EphemeralContainersToStart, p.ContainersToUpdate, p.ContainersToKill, p.ContainersToRemove) + p.KillPod, p.CreateSandbox, p.UpdatePodResources, p.Attempt, p.InitContainersToStart, p.ContainersToStart, p.EphemeralContainersToStart, p.ContainersToUpdate, p.ContainersToKill, p.ContainersToReset) } // containerChanged will determine whether the container has changed based on the fields that will affect the running of the container. @@ -1037,16 +1037,14 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * // Needs to kill and remove all containers in reverse order when the pod is marked for RestartAllContainers. if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) && restartAllContainers { logger.V(3).Info("Pod marked for RestartAllContainers", "pod", klog.KObj(pod)) - changes.KillPod = false - changes.CreateSandbox = false // Kill and remove containers in reverse order. Source containers (which exited and triggered // RestartAllContainers) are removed last. - sourceInitContainers, targetInitContainers := m.getContainersToRemove(ctx, pod.Spec.InitContainers, podStatus) - sourceContainers, targetContainers := m.getContainersToRemove(ctx, pod.Spec.Containers, podStatus) - changes.ContainersToRemove = append(changes.ContainersToRemove, targetContainers...) - changes.ContainersToRemove = append(changes.ContainersToRemove, targetInitContainers...) - changes.ContainersToRemove = append(changes.ContainersToRemove, sourceContainers...) - changes.ContainersToRemove = append(changes.ContainersToRemove, sourceInitContainers...) + sourceInitContainers, targetInitContainers := m.getContainersToReset(pod.Spec.InitContainers, podStatus) + sourceContainers, targetContainers := m.getContainersToReset(pod.Spec.Containers, podStatus) + changes.ContainersToReset = append(changes.ContainersToReset, targetContainers...) + changes.ContainersToReset = append(changes.ContainersToReset, targetInitContainers...) + changes.ContainersToReset = append(changes.ContainersToReset, sourceContainers...) + changes.ContainersToReset = append(changes.ContainersToReset, sourceInitContainers...) return changes } @@ -1234,14 +1232,19 @@ func (m *kubeGenericRuntimeManager) computePodActions(ctx context.Context, pod * return changes } -func (m *kubeGenericRuntimeManager) getContainersToRemove(ctx context.Context, containers []v1.Container, podStatus *kubecontainer.PodStatus) (sources []containerToRemoveInfo, targets []containerToRemoveInfo) { - for idx := len(containers) - 1; idx >= 0; idx-- { - c := containers[idx] - containerStatus := podStatus.FindContainerStatusByName(c.Name) - if containerStatus != nil { +// getContainersToReset returns container info about the containers to remove from the runtime. +// The first list are the containers that triggered the RestartAllContainers; the second list +// are the containers that are victim of the RestartAllContainers. +func (m *kubeGenericRuntimeManager) getContainersToReset(containers []v1.Container, podStatus *kubecontainer.PodStatus) (sources []containerToRemoveInfo, targets []containerToRemoveInfo) { + for idx, c := range containers { + // podStatus.FindContainerStatusByName cannot be used because there can be multiple container + // statuses per container, and RestartAllContainers require all container to be purged from runtime. + for _, containerStatus := range podStatus.ContainerStatuses { + if containerStatus.Name != c.Name { + continue + } info := containerToRemoveInfo{ containerID: containerStatus.ID, - name: containerStatus.Name, container: &containers[idx], } if containerStatus.State == kubecontainer.ContainerStateExited { @@ -1321,23 +1324,25 @@ func (m *kubeGenericRuntimeManager) SyncPod(ctx context.Context, pod *v1.Pod, po // Removes the containers if they are marked for removal (for in-place restart) if utilfeature.DefaultFeatureGate.Enabled(features.RestartAllContainersOnContainerExits) { - for _, containerInfo := range podContainerChanges.ContainersToRemove { - logger.V(3).Info("Removing container before pod restarts", "containerName", containerInfo.name, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) - removeContainerResult := kubecontainer.NewSyncResult(kubecontainer.RemoveContainer, containerInfo.name) + for _, containerInfo := range podContainerChanges.ContainersToReset { + cName := containerInfo.container.Name + logger.V(3).Info("Removing container before pod restarts", "containerName", cName, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) + removeContainerResult := kubecontainer.NewSyncResult(kubecontainer.RemoveContainer, cName) result.AddSyncResult(removeContainerResult) if containerInfo.kill { - logger.V(3).Info("Killing container before removal", "containerName", containerInfo.name, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) + logger.V(3).Info("Killing container before removal", "containerName", cName, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) // Killing containers without grace period. var gracePeriod int64 = 0 - if err := m.killContainer(ctx, pod, containerInfo.containerID, containerInfo.name, "killing", reasonRestartAllContainers, &gracePeriod, nil); err != nil { + if err := m.killContainer(ctx, pod, containerInfo.containerID, cName, "killing", reasonRestartAllContainers, &gracePeriod, nil); err != nil { removeContainerResult.Fail(kubecontainer.ErrKillContainer, err.Error()) - logger.Error(err, "killContainer for pod failed", "containerName", containerInfo.name, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) + logger.Error(err, "killContainer for pod failed", "containerName", cName, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) return } } + // TODO(yuanwang04): Revisit whether container logs should be persisted. if err := m.removeContainer(ctx, containerInfo.containerID.ID); err != nil { removeContainerResult.Fail(kubecontainer.ErrRemoveContainer, err.Error()) - logger.Error(err, "removeContainer for pod failed", "containerName", containerInfo.name, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) + logger.Error(err, "removeContainer for pod failed", "containerName", cName, "containerID", containerInfo.containerID, "pod", klog.KObj(pod)) return } } diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index d2a75942219..20651c228a3 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -1421,9 +1421,9 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "foo3", kill: true}, - {name: "foo2", kill: true}, - {name: "foo1", kill: true}, + {container: &v1.Container{Name: "foo1"}, containerID: kubecontainer.ContainerID{ID: "id1"}, kill: true}, + {container: &v1.Container{Name: "foo2"}, containerID: kubecontainer.ContainerID{ID: "id2"}, kill: true}, + {container: &v1.Container{Name: "foo3"}, containerID: kubecontainer.ContainerID{ID: "id3"}, kill: true}, }, }, "pod marked for RestartAllContainers with init containers": { @@ -1438,9 +1438,9 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "init3"}, - {name: "init2"}, - {name: "init1"}, + {container: &v1.Container{Name: "init1"}, containerID: kubecontainer.ContainerID{ID: "initid1"}}, + {container: &v1.Container{Name: "init2"}, containerID: kubecontainer.ContainerID{ID: "initid2"}}, + {container: &v1.Container{Name: "init3"}, containerID: kubecontainer.ContainerID{ID: "initid3"}}, }, }, "pod marked for RestartAllContainers with restartable init containers": { @@ -1455,15 +1455,15 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "restartable-init-3", kill: true}, - {name: "restartable-init-2", kill: true}, - {name: "restartable-init-1", kill: true}, + {container: &v1.Container{Name: "restartable-init-1"}, containerID: kubecontainer.ContainerID{ID: "initid1"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-2"}, containerID: kubecontainer.ContainerID{ID: "initid2"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-3"}, containerID: kubecontainer.ContainerID{ID: "initid3"}, kill: true}, }, }, - "init container exit triggers RestartAllContainres": { + "init container exit triggers RestartAllContainers": { // The init3 container exited and triggers RestartAllContainers, - // the init2 container is a running sidecar. First, the init2 - // should be killed and removed; second, the init1 containers should + // the init2 container is a running sidecar. First, the init1 + // should be removed; second, the init2 containers should killed and // be removed; lastly, init3 container should be removed. podFunc: func() *v1.Pod { pod, _ := makeBasePodAndStatusWithInitContainers() @@ -1489,14 +1489,14 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "init2", kill: true}, - {name: "init1"}, - {name: "init3"}, + {container: &v1.Container{Name: "init1"}, containerID: kubecontainer.ContainerID{ID: "initid1"}}, + {container: &v1.Container{Name: "init2"}, containerID: kubecontainer.ContainerID{ID: "initid2"}, kill: true}, + {container: &v1.Container{Name: "init3"}, containerID: kubecontainer.ContainerID{ID: "initid3"}}, }, }, "sidecar container exit triggers RestartAllContainres": { // Restartable-init-3 fails and triggers RestartAllContainers. - // The running foo1 should be killed and removed first; then init-2 and init-1 + // The running foo1 should be killed and removed first; then init-1 and init-2 // should be killed and removed; lastly init-3 should be removed. podFunc: func() *v1.Pod { pod, _ := makeBasePodAndStatusWithRestartableInitContainers() @@ -1523,10 +1523,10 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "foo1", kill: true}, - {name: "restartable-init-2", kill: true}, - {name: "restartable-init-1", kill: true}, - {name: "restartable-init-3"}, + {container: &v1.Container{Name: "foo1"}, containerID: kubecontainer.ContainerID{ID: "id1"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-1"}, containerID: kubecontainer.ContainerID{ID: "initid1"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-2"}, containerID: kubecontainer.ContainerID{ID: "initid2"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-3"}, containerID: kubecontainer.ContainerID{ID: "initid3"}}, }, }, "regular container exit triggers RestartAllContainers": { @@ -1561,12 +1561,72 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { }, restartAllContainers: true, containersToRemove: []containerToRemoveInfo{ - {name: "foo2", kill: true}, - {name: "foo1", kill: true}, - {name: "restartable-init-3", kill: true}, - {name: "restartable-init-2", kill: true}, - {name: "restartable-init-1", kill: true}, - {name: "foo3"}, + {container: &v1.Container{Name: "foo1"}, containerID: kubecontainer.ContainerID{ID: "id1"}, kill: true}, + {container: &v1.Container{Name: "foo2"}, containerID: kubecontainer.ContainerID{ID: "id2"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-1"}, containerID: kubecontainer.ContainerID{ID: "initid1"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-2"}, containerID: kubecontainer.ContainerID{ID: "initid2"}, kill: true}, + {container: &v1.Container{Name: "restartable-init-3"}, containerID: kubecontainer.ContainerID{ID: "initid3"}, kill: true}, + {container: &v1.Container{Name: "foo3"}, containerID: kubecontainer.ContainerID{ID: "id3"}}, + }, + }, + "removes past terminated statuses": { + // foo3 terminated and triggers restart all containers. All containers have started once at t0, and + // restarted once at t1. All container statuses should be removed; foo3 should be removed last + // because it triggered restart all containers. + podFunc: func() *v1.Pod { + pod, _ := makeBasePodAndStatus() + pod.Spec.RestartPolicy = v1.RestartPolicyAlways + source := pod.Spec.Containers[2] + source.RestartPolicy = &restartPolicyAlways + source.RestartPolicyRules = restartAllContainersRules + pod.Spec.Containers[2] = source + pod.Status.Conditions = allContainersRestartingTrue + return pod + }, + podStatusFunc: func() *kubecontainer.PodStatus { + _, status := makeBasePodAndStatus() + t1 := time.Now() + t0 := t1.Add(-time.Minute) + status.ContainerStatuses[0].CreatedAt = t1 + status.ContainerStatuses[1].CreatedAt = t1 + status.ContainerStatuses[2] = &kubecontainer.Status{ + Name: "foo3", + State: kubecontainer.ContainerStateExited, + ExitCode: 1, + ID: kubecontainer.ContainerID{ID: "id3"}, + CreatedAt: t1, + } + status.ContainerStatuses = append(status.ContainerStatuses, &kubecontainer.Status{ + Name: "foo1", + State: kubecontainer.ContainerStateExited, + ExitCode: 99, + ID: kubecontainer.ContainerID{ID: "id1-past"}, + CreatedAt: t0, + }) + status.ContainerStatuses = append(status.ContainerStatuses, &kubecontainer.Status{ + Name: "foo2", + State: kubecontainer.ContainerStateExited, + ExitCode: 99, + ID: kubecontainer.ContainerID{ID: "id2-past"}, + CreatedAt: t0, + }) + status.ContainerStatuses = append(status.ContainerStatuses, &kubecontainer.Status{ + Name: "foo3", + State: kubecontainer.ContainerStateExited, + ExitCode: 99, + ID: kubecontainer.ContainerID{ID: "id3-past"}, + CreatedAt: t0, + }) + return status + }, + restartAllContainers: true, + containersToRemove: []containerToRemoveInfo{ + {container: &v1.Container{Name: "foo1"}, containerID: kubecontainer.ContainerID{ID: "id1"}, kill: true}, + {container: &v1.Container{Name: "foo1"}, containerID: kubecontainer.ContainerID{ID: "id1-past"}}, + {container: &v1.Container{Name: "foo2"}, containerID: kubecontainer.ContainerID{ID: "id2"}, kill: true}, + {container: &v1.Container{Name: "foo2"}, containerID: kubecontainer.ContainerID{ID: "id2-past"}}, + {container: &v1.Container{Name: "foo3"}, containerID: kubecontainer.ContainerID{ID: "id3-past"}}, + {container: &v1.Container{Name: "foo3"}, containerID: kubecontainer.ContainerID{ID: "id3"}}, }, }, "all containers removed, start init container": { @@ -1615,10 +1675,6 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { expected.InitContainersToStart = test.initContainersToStart } - containerStatusByName := make(map[string]*kubecontainer.Status) - for _, c := range status.ContainerStatuses { - containerStatusByName[c.Name] = c - } containerSpecByName := make(map[string]*v1.Container) for idx, c := range pod.Spec.Containers { containerSpecByName[c.Name] = &pod.Spec.Containers[idx] @@ -1627,9 +1683,9 @@ func TestComputePodActionsForRestartAllContainers(t *testing.T) { containerSpecByName[c.Name] = &pod.Spec.InitContainers[idx] } for _, info := range test.containersToRemove { - info.container = containerSpecByName[info.name] - info.containerID = containerStatusByName[info.name].ID - expected.ContainersToRemove = append(expected.ContainersToRemove, info) + cName := info.container.Name + info.container = containerSpecByName[cName] + expected.ContainersToReset = append(expected.ContainersToReset, info) } verifyActions(t, expected, &actions, desc) diff --git a/test/e2e/node/pods.go b/test/e2e/node/pods.go index cafac5aa726..2cbf75c6542 100644 --- a/test/e2e/node/pods.go +++ b/test/e2e/node/pods.go @@ -917,9 +917,15 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature { Name: "source-container", Image: imageutils.GetE2EImage(imageutils.BusyBox), - Command: []string{"/bin/sh", "-c", "sleep 60; exit 42"}, + Command: []string{"/bin/sh", "-c", "if [ -f /mnt/restart-complete ]; then sleep 10000; else touch /mnt/restart-complete; exit 42; fi"}, RestartPolicy: &containerRestartPolicyNever, RestartPolicyRules: restartAllContainersRules, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir", + MountPath: "/mnt", + }, + }, }, { Name: "regular", @@ -927,6 +933,14 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature Command: []string{"/bin/sh", "-c", "sleep 10000"}, }, }, + Volumes: []v1.Volume{ + { + Name: "workdir", + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + }, }, } @@ -938,6 +952,8 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature return podClient.Delete(ctx, pod.Name, metav1.DeleteOptions{}) }) validateAllContainersRestarted(ctx, f, pod, []string{"init", "sidecar", "source-container", "regular"}) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "source-container", 3*time.Minute)) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "regular", 3*time.Minute)) }) ginkgo.It("should restart all containers on sidecar container exit", func(ctx context.Context) { @@ -963,9 +979,15 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature { Name: "source-sidecar", Image: imageutils.GetE2EImage(imageutils.BusyBox), - Command: []string{"/bin/sh", "-c", "sleep 60; exit 42"}, + Command: []string{"/bin/sh", "-c", "if [ -f /mnt/init-complete ]; then sleep 10000; else touch /mnt/init-complete; sleep 30; exit 42; fi"}, RestartPolicy: &containerRestartPolicyAlways, RestartPolicyRules: restartAllContainersRules, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir", + MountPath: "/mnt", + }, + }, }, }, Containers: []v1.Container{ @@ -975,6 +997,14 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature Command: []string{"/bin/sh", "-c", "sleep 10000"}, }, }, + Volumes: []v1.Volume{ + { + Name: "workdir", + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + }, }, } @@ -986,6 +1016,8 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature return podClient.Delete(ctx, pod.Name, metav1.DeleteOptions{}) }) validateAllContainersRestarted(ctx, f, pod, []string{"init", "sidecar", "source-sidecar", "regular"}) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "regular", 3*time.Minute)) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "source-sidecar", 3*time.Minute)) }) ginkgo.It("should restart init and sidecar containers on init container exit", func(ctx context.Context) { @@ -1103,6 +1135,56 @@ var _ = SIGDescribe("Pod Extended (RestartAllContainers)", framework.WithFeature framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "source-container", 3*time.Minute)) framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "regular", 3*time.Minute)) }) + + ginkgo.It("should restart all containers on a previously restarted regular container exit ", func(ctx context.Context) { + podName := "restart-rules-exit-code-" + string(uuid.NewUUID()) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: podName, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "source-container", + Image: imageutils.GetE2EImage(imageutils.BusyBox), + Command: []string{"/bin/sh", "-c", "if [ -f /mnt/restart-complete ]; then sleep 10000; elif [ -f /mnt/restart-1 ]; then touch /mnt/restart-complete; exit 42; else touch /mnt/restart-1; exit 1; fi"}, + RestartPolicy: &containerRestartPolicyAlways, + RestartPolicyRules: restartAllContainersRules, + VolumeMounts: []v1.VolumeMount{ + { + Name: "workdir", + MountPath: "/mnt", + }, + }, + }, + { + Name: "regular", + Image: imageutils.GetE2EImage(imageutils.BusyBox), + Command: []string{"/bin/sh", "-c", "sleep 10000"}, + }, + }, + Volumes: []v1.Volume{ + { + Name: "workdir", + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + }, + }, + } + + // All containers should be restarted once + podClient := e2epod.NewPodClient(f) + podClient.Create(ctx, pod) + ginkgo.DeferCleanup(func(ctx context.Context) error { + ginkgo.By("deleting the pod") + return podClient.Delete(ctx, pod.Name, metav1.DeleteOptions{}) + }) + validateAllContainersRestarted(ctx, f, pod, []string{"source-container", "regular"}) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "source-container", 3*time.Minute)) + framework.ExpectNoError(e2epod.WaitForContainerRunning(ctx, f.ClientSet, f.Namespace.Name, podName, "regular", 3*time.Minute)) + }) }) }) diff --git a/test/e2e_node/restart_all_containers_test.go b/test/e2e_node/restart_all_containers_test.go index f7def010fd9..5de189108b9 100644 --- a/test/e2e_node/restart_all_containers_test.go +++ b/test/e2e_node/restart_all_containers_test.go @@ -2,7 +2,7 @@ // +build linux /* -Copyright 2015 The Kubernetes Authors. +Copyright The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.