Delete all dead containers only after pod syncing is done.

pkg/kubelet/kubelet.go

@@ -2409,6 +2409,13 @@ func (kl *Kubelet) HandlePodReconcile(pods []*api.Pod) {
 		// Update the pod in pod manager, status manager will do periodically reconcile according
 		// to the pod manager.
 		kl.podManager.UpdatePod(pod)
+
+		// After an evicted pod is synced, all dead containers in the pod can be removed.
+		if eviction.PodIsEvicted(pod.Status) {
+			if podStatus, err := kl.podCache.Get(pod.UID); err == nil {
+				kl.containerDeletor.deleteContainersInPod("", podStatus, true)
+			}
+		}
 	}
 }
 
@@ -2989,10 +2996,12 @@ func (kl *Kubelet) ListenAndServeReadOnly(address net.IP, port uint) {
 // Delete the eligible dead container instances in a pod. Depending on the configuration, the latest dead containers may be kept around.
 func (kl *Kubelet) cleanUpContainersInPod(podId types.UID, exitedContainerID string) {
 	if podStatus, err := kl.podCache.Get(podId); err == nil {
-		if status, ok := kl.statusManager.GetPodStatus(podId); ok {
-			// If a pod is evicted, we can delete all the dead containers.
-			kl.containerDeletor.deleteContainersInPod(exitedContainerID, podStatus, eviction.PodIsEvicted(status))
+		removeAll := false
+		if syncedPod, ok := kl.podManager.GetPodByUID(podId); ok {
+			// When an evicted pod has already synced, all containers can be removed.
+			removeAll = eviction.PodIsEvicted(syncedPod.Status)
 		}
+		kl.containerDeletor.deleteContainersInPod(exitedContainerID, podStatus, removeAll)
 	}
 }
 

pkg/kubelet/pod_container_deletor.go

@@ -58,29 +58,33 @@ func newPodContainerDeletor(runtime kubecontainer.Runtime, containersToKeep int)
 	}
 }
 
-// getContainersToDeleteInPod returns the exited containers in a pod whose name matches the name inferred from exitedContainerID, ordered by the creation time from the latest to the earliest.
-func getContainersToDeleteInPod(exitedContainerID string, podStatus *kubecontainer.PodStatus, containersToKeep int) containerStatusbyCreatedList {
-	var matchedContainer *kubecontainer.ContainerStatus
-	var exitedContainers []*kubecontainer.ContainerStatus
-	// Find all exited containers in the pod
-	for _, containerStatus := range podStatus.ContainerStatuses {
-		if containerStatus.State != kubecontainer.ContainerStateExited {
-			continue
+// getContainersToDeleteInPod returns the exited containers in a pod whose name matches the name inferred from filterContainerId (if not empty), ordered by the creation time from the latest to the earliest.
+// If filterContainerId is empty, all dead containers in the pod are returned.
+func getContainersToDeleteInPod(filterContainerId string, podStatus *kubecontainer.PodStatus, containersToKeep int) containerStatusbyCreatedList {
+	matchedContainer := func(filterContainerId string, podStatus *kubecontainer.PodStatus) *kubecontainer.ContainerStatus {
+		if filterContainerId == "" {
+			return nil
 		}
-		if containerStatus.ID.ID == exitedContainerID {
-			matchedContainer = containerStatus
+		for _, containerStatus := range podStatus.ContainerStatuses {
+			if containerStatus.ID.ID == filterContainerId {
+				return containerStatus
+			}
 		}
-		exitedContainers = append(exitedContainers, containerStatus)
-	}
-	if matchedContainer == nil {
-		glog.Warningf("Container %q not found in pod's exited containers", exitedContainerID)
+		return nil
+	}(filterContainerId, podStatus)
+
+	if filterContainerId != "" && matchedContainer == nil {
+		glog.Warningf("Container %q not found in pod's containers", filterContainerId)
 		return containerStatusbyCreatedList{}
 	}
 
-	// Find the exited containers whose name matches the name of the container with id being exitedContainerID
+	// Find the exited containers whose name matches the name of the container with id being filterContainerId
 	var candidates containerStatusbyCreatedList
-	for _, containerStatus := range exitedContainers {
-		if matchedContainer.Name == containerStatus.Name {
+	for _, containerStatus := range podStatus.ContainerStatuses {
+		if containerStatus.State != kubecontainer.ContainerStateExited {
+			continue
+		}
+		if matchedContainer == nil || matchedContainer.Name == containerStatus.Name {
 			candidates = append(candidates, containerStatus)
 		}
 	}
@@ -93,12 +97,13 @@ func getContainersToDeleteInPod(exitedContainerID string, podStatus *kubecontain
 }
 
 // deleteContainersInPod issues container deletion requests for containers selected by getContainersToDeleteInPod.
-func (p *podContainerDeletor) deleteContainersInPod(exitedContainerID string, podStatus *kubecontainer.PodStatus, removeAll bool) {
+func (p *podContainerDeletor) deleteContainersInPod(filterContainerId string, podStatus *kubecontainer.PodStatus, removeAll bool) {
 	containersToKeep := p.containersToKeep
 	if removeAll {
 		containersToKeep = 0
 	}
-	for _, candidate := range getContainersToDeleteInPod(exitedContainerID, podStatus, containersToKeep) {
+
+	for _, candidate := range getContainersToDeleteInPod(filterContainerId, podStatus, containersToKeep) {
 		select {
 		case p.worker <- candidate.ID:
 		default:

pkg/kubelet/pod_container_deletor_test.go

@@ -24,7 +24,7 @@ import (
 	kubecontainer "k8s.io/kubernetes/pkg/kubelet/container"
 )
 
-func testGetContainersToDeleteInPod(t *testing.T) {
+func TestGetContainersToDeleteInPodWithFilter(t *testing.T) {
 	pod := kubecontainer.PodStatus{
 		ContainerStatuses: []*kubecontainer.ContainerStatus{
 			{
@@ -62,7 +62,7 @@ func testGetContainersToDeleteInPod(t *testing.T) {
 
 	testCases := []struct {
 		containersToKeep           int
-		expectedContainersToDelete []*kubecontainer.ContainerStatus
+		expectedContainersToDelete containerStatusbyCreatedList
 	}{
 		{
 			0,
@@ -80,7 +80,123 @@ func testGetContainersToDeleteInPod(t *testing.T) {
 
 	for _, test := range testCases {
 		candidates := getContainersToDeleteInPod("4", &pod, test.containersToKeep)
-		if !reflect.DeepEqual(getContainersToDeleteInPod("4", &pod, test.containersToKeep), test.expectedContainersToDelete) {
+		if !reflect.DeepEqual(candidates, test.expectedContainersToDelete) {
+			t.Errorf("expected %v got %v", test.expectedContainersToDelete, candidates)
+		}
+	}
+}
+
+func TestGetContainersToDeleteInPod(t *testing.T) {
+	pod := kubecontainer.PodStatus{
+		ContainerStatuses: []*kubecontainer.ContainerStatus{
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "1"},
+				Name:      "foo",
+				CreatedAt: time.Now(),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "2"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "3"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(2 * time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "4"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(3 * time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "5"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(4 * time.Second),
+				State:     kubecontainer.ContainerStateRunning,
+			},
+		},
+	}
+
+	testCases := []struct {
+		containersToKeep           int
+		expectedContainersToDelete containerStatusbyCreatedList
+	}{
+		{
+			0,
+			[]*kubecontainer.ContainerStatus{pod.ContainerStatuses[3], pod.ContainerStatuses[2], pod.ContainerStatuses[1], pod.ContainerStatuses[0]},
+		},
+		{
+			1,
+			[]*kubecontainer.ContainerStatus{pod.ContainerStatuses[2], pod.ContainerStatuses[1], pod.ContainerStatuses[0]},
+		},
+		{
+			2,
+			[]*kubecontainer.ContainerStatus{pod.ContainerStatuses[1], pod.ContainerStatuses[0]},
+		},
+	}
+
+	for _, test := range testCases {
+		candidates := getContainersToDeleteInPod("", &pod, test.containersToKeep)
+		if !reflect.DeepEqual(candidates, test.expectedContainersToDelete) {
+			t.Errorf("expected %v got %v", test.expectedContainersToDelete, candidates)
+		}
+	}
+}
+
+func TestGetContainersToDeleteInPodWithNoMatch(t *testing.T) {
+	pod := kubecontainer.PodStatus{
+		ContainerStatuses: []*kubecontainer.ContainerStatus{
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "1"},
+				Name:      "foo",
+				CreatedAt: time.Now(),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "2"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "3"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(2 * time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "4"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(3 * time.Second),
+				State:     kubecontainer.ContainerStateExited,
+			},
+			{
+				ID:        kubecontainer.ContainerID{Type: "test", ID: "5"},
+				Name:      "bar",
+				CreatedAt: time.Now().Add(4 * time.Second),
+				State:     kubecontainer.ContainerStateRunning,
+			},
+		},
+	}
+
+	testCases := []struct {
+		filterId                   string
+		expectedContainersToDelete containerStatusbyCreatedList
+	}{
+		{
+			"abc",
+			[]*kubecontainer.ContainerStatus{},
+		},
+	}
+
+	for _, test := range testCases {
+		candidates := getContainersToDeleteInPod(test.filterId, &pod, len(pod.ContainerStatuses))
+		if !reflect.DeepEqual(candidates, test.expectedContainersToDelete) {
 			t.Errorf("expected %v got %v", test.expectedContainersToDelete, candidates)
 		}
 	}