Merge pull request #15965 from Random-Liu/use-docker-label

pkg/kubelet/dockertools/fake_docker_client.go

@@ -147,6 +147,9 @@ func (f *FakeDockerClient) ListContainers(options docker.ListContainersOptions)
 	f.called = append(f.called, "list")
 	err := f.popError("list")
 	if options.All {
+		// Althought the container is not sorted, but the container with the same name should be in order,
+		// that is enough for us now.
+		// TODO (random-liu) Is a fully sorted array needed?
 		return append(f.ContainerList, f.ExitedContainerList...), err
 	}
 	return append([]docker.APIContainers{}, f.ContainerList...), err
@@ -204,7 +207,10 @@ func (f *FakeDockerClient) CreateContainer(c docker.CreateContainerOptions) (*do
 	// This is not a very good fake. We'll just add this container's name to the list.
 	// Docker likes to add a '/', so copy that behavior.
 	name := "/" + c.Name
-	f.ContainerList = append(f.ContainerList, docker.APIContainers{ID: name, Names: []string{name}, Image: c.Config.Image})
+	// The newest container should be in front, because we assume so in GetPodStatus()
+	f.ContainerList = append([]docker.APIContainers{
+		{ID: name, Names: []string{name}, Image: c.Config.Image, Labels: c.Config.Labels},
+	}, f.ContainerList...)
 	container := docker.Container{ID: name, Name: name, Config: c.Config}
 	if f.ContainerMap != nil {
 		containerCopy := container
@@ -266,7 +272,8 @@ func (f *FakeDockerClient) StopContainer(id string, timeout uint) error {
 	var newList []docker.APIContainers
 	for _, container := range f.ContainerList {
 		if container.ID == id {
-			f.ExitedContainerList = append(f.ExitedContainerList, container)
+			// The newest exited container should be in front. Because we assume so in GetPodStatus()
+			f.ExitedContainerList = append([]docker.APIContainers{container}, f.ExitedContainerList...)
 			continue
 		}
 		newList = append(newList, container)

pkg/kubelet/dockertools/manager_test.go

@@ -544,6 +544,7 @@ func runSyncPod(t *testing.T, dm *DockerManager, fakeDocker *FakeDockerClient, p
 		t.Fatalf("unexpected error: %v", err)
 	}
 	runningPod := kubecontainer.Pods(runningPods).FindPodByID(pod.UID)
+
 	podStatus, err := dm.GetPodStatus(pod)
 	if err != nil {
 		t.Errorf("unexpected error: %v", err)
@@ -713,6 +714,14 @@ func TestSyncPodDeletesWithNoPodInfraContainer(t *testing.T) {
 		},
 	}
 
+	fakeDocker.ContainerMap = map[string]*docker.Container{
+		"1234": {
+			ID:         "1234",
+			Config:     &docker.Config{},
+			HostConfig: &docker.HostConfig{},
+		},
+	}
+
 	runSyncPod(t, dm, fakeDocker, pod, nil)
 
 	verifyCalls(t, fakeDocker, []string{
@@ -1526,49 +1535,16 @@ func TestGetRestartCount(t *testing.T) {
 			Namespace: "new",
 		},
 		Spec: api.PodSpec{
-			Containers: containers,
+			Containers:    containers,
+			RestartPolicy: "Always",
 		},
 	}
 
-	// format is // k8s_<container-id>_<pod-fullname>_<pod-uid>
-	names := []string{"/k8s_bar." + strconv.FormatUint(kubecontainer.HashContainer(&containers[0]), 16) + "_foo_new_12345678_0"}
-	currTime := time.Now()
-	containerMap := map[string]*docker.Container{
-		"1234": {
-			ID:     "1234",
-			Name:   "bar",
-			Config: &docker.Config{},
-			State: docker.State{
-				ExitCode:   42,
-				StartedAt:  currTime.Add(-60 * time.Second),
-				FinishedAt: currTime.Add(-60 * time.Second),
-			},
-		},
-		"5678": {
-			ID:     "5678",
-			Name:   "bar",
-			Config: &docker.Config{},
-			State: docker.State{
-				ExitCode:   42,
-				StartedAt:  currTime.Add(-30 * time.Second),
-				FinishedAt: currTime.Add(-30 * time.Second),
-			},
-		},
-		"9101": {
-			ID:     "9101",
-			Name:   "bar",
-			Config: &docker.Config{},
-			State: docker.State{
-				ExitCode:   42,
-				StartedAt:  currTime.Add(30 * time.Minute),
-				FinishedAt: currTime.Add(30 * time.Minute),
-			},
-		},
-	}
-	fakeDocker.ContainerMap = containerMap
+	fakeDocker.ContainerMap = map[string]*docker.Container{}
 
 	// Helper function for verifying the restart count.
 	verifyRestartCount := func(pod *api.Pod, expectedCount int) api.PodStatus {
+		runSyncPod(t, dm, fakeDocker, pod, nil)
 		status, err := dm.GetPodStatus(pod)
 		if err != nil {
 			t.Fatalf("unexpected error %v", err)
@@ -1580,21 +1556,48 @@ func TestGetRestartCount(t *testing.T) {
 		return *status
 	}
 
-	// Container "bar" has failed twice; create two dead docker containers.
+	killOneContainer := func(pod *api.Pod) {
+		status, err := dm.GetPodStatus(pod)
+		if err != nil {
+			t.Fatalf("unexpected error %v", err)
+		}
+		containerID := kubecontainer.ParseContainerID(status.ContainerStatuses[0].ContainerID)
+		dm.KillContainerInPod(containerID, &pod.Spec.Containers[0], pod)
+	}
+	// Container "bar" starts the first time.
 	// TODO: container lists are expected to be sorted reversely by time.
 	// We should fix FakeDockerClient to sort the list before returning.
-	fakeDocker.ExitedContainerList = []docker.APIContainers{{Names: names, ID: "5678"}, {Names: names, ID: "1234"}}
+	// (randome-liu) Just partially sorted now.
+	pod.Status = verifyRestartCount(&pod, 0)
+	killOneContainer(&pod)
+
+	// Poor container "bar" has been killed, and should be restarted with restart count 1
 	pod.Status = verifyRestartCount(&pod, 1)
+	killOneContainer(&pod)
 
-	// Found a new dead container. The restart count should be incremented.
-	fakeDocker.ExitedContainerList = []docker.APIContainers{
-		{Names: names, ID: "9101"}, {Names: names, ID: "5678"}, {Names: names, ID: "1234"}}
+	// Poor container "bar" has been killed again, and should be restarted with restart count 2
 	pod.Status = verifyRestartCount(&pod, 2)
+	killOneContainer(&pod)
+
+	// Poor container "bar" has been killed again ang again, and should be restarted with restart count 3
+	pod.Status = verifyRestartCount(&pod, 3)
+
+	// The oldest container has been garbage collected
+	exitedContainers := fakeDocker.ExitedContainerList
+	fakeDocker.ExitedContainerList = exitedContainers[:len(exitedContainers)-1]
+	pod.Status = verifyRestartCount(&pod, 3)
 
-	// All dead containers have been GC'd. The restart count should persist
-	// (i.e., remain the same).
+	// The last two oldest containers have been garbage collected
+	fakeDocker.ExitedContainerList = exitedContainers[:len(exitedContainers)-2]
+	pod.Status = verifyRestartCount(&pod, 3)
+
+	// All exited containers have been garbage collected
 	fakeDocker.ExitedContainerList = []docker.APIContainers{}
-	verifyRestartCount(&pod, 2)
+	pod.Status = verifyRestartCount(&pod, 3)
+	killOneContainer(&pod)
+
+	// Poor container "bar" has been killed again ang again and again, and should be restarted with restart count 4
+	pod.Status = verifyRestartCount(&pod, 4)
 }
 
 func TestSyncPodWithPodInfraCreatesContainerCallsHandler(t *testing.T) {

pkg/kubelet/kubelet.go

@@ -2698,17 +2698,6 @@ func (kl *Kubelet) generatePodStatus(pod *api.Pod) (api.PodStatus, error) {
 
 	podFullName := kubecontainer.GetPodFullName(pod)
 	glog.V(3).Infof("Generating status for %q", podFullName)
-	if existingStatus, hasExistingStatus := kl.statusManager.GetPodStatus(pod.UID); hasExistingStatus {
-		// This is a hacky fix to ensure container restart counts increment
-		// monotonically. Normally, we should not modify given pod. In this
-		// case, we check if there are cached status for this pod, and update
-		// the pod so that we update restart count appropriately.
-		// TODO(yujuhong): We will not need to count dead containers every time
-		// once we add the runtime pod cache.
-		// Note that kubelet restarts may still cause temporarily setback of
-		// restart counts.
-		pod.Status = existingStatus
-	}
 
 	// TODO: Consider include the container information.
 	if kl.pastActiveDeadline(pod) {