Kubelet: persist restart count of a container

Currently, restart count are generated by examine dead docker containers, which
are subject to background garbage collection. Therefore, the restart count is
capped at 5 and can decrement if GC happens.

This change leverages the container statuses recorded in the pod status as a
reference point. If a container finished after the last observation, restart
count is incremented on top of the last observed count. If container is created
after last observation, but GC'd before the current observation time, kubelet
would not be aware of the existence of such a container, and would not increase
the restart count accordingly. However, the chance of this should be low, given
that pod statuses are reported frequently. Also, the restart cound would still
be increasing monotonically (with the exception of container insepct error).

pkg/kubelet/dockertools/manager.go

@@ -231,8 +231,17 @@ func (self *DockerManager) GetPodStatus(pod *api.Pod) (*api.PodStatus, error) {
 	uid := pod.UID
 	manifest := pod.Spec
 
+	oldStatuses := make(map[string]api.ContainerStatus, len(pod.Spec.Containers))
+	lastObservedTime := make(map[string]util.Time, len(pod.Spec.Containers))
+	for _, status := range pod.Status.ContainerStatuses {
+		oldStatuses[status.Name] = status
+		if status.LastTerminationState.Termination != nil {
+			lastObservedTime[status.Name] = status.LastTerminationState.Termination.FinishedAt
+		}
+	}
+
 	var podStatus api.PodStatus
-	statuses := make(map[string]api.ContainerStatus)
+	statuses := make(map[string]api.ContainerStatus, len(pod.Spec.Containers))
 
 	expectedContainers := make(map[string]api.Container)
 	for _, container := range manifest.Containers {
@@ -245,6 +254,7 @@ func (self *DockerManager) GetPodStatus(pod *api.Pod) (*api.PodStatus, error) {
 		return nil, err
 	}
 
+	containerDone := util.NewStringSet()
 	for _, value := range containers {
 		if len(value.Names) == 0 {
 			continue
@@ -261,23 +271,53 @@ func (self *DockerManager) GetPodStatus(pod *api.Pod) (*api.PodStatus, error) {
 		}
 		dockerContainerName := dockerName.ContainerName
 		c, found := expectedContainers[dockerContainerName]
-		terminationMessagePath := ""
 		if !found {
-			// TODO(dchen1107): should figure out why not continue here
-			// continue
-		} else {
-			terminationMessagePath = c.TerminationMessagePath
+			continue
 		}
-		// We assume docker return us a list of containers in time order
+		terminationMessagePath := c.TerminationMessagePath
+		if containerDone.Has(dockerContainerName) {
+			continue
+		}
+		// We assume docker returns a list of containers sorted reversedly by
+		// time.
 		if containerStatus, found := statuses[dockerContainerName]; found {
-			// Populate last termination state
+			inspected := false
+			var terminationState *api.ContainerState = nil
 			if containerStatus.LastTerminationState.Termination == nil {
 				result := self.inspectContainer(value.ID, dockerContainerName, terminationMessagePath)
+				inspected = true
 				if result.err == nil && result.status.State.Termination != nil {
-					containerStatus.LastTerminationState = result.status.State
+					terminationState = &result.status.State
+					// Populate the last termination state.
+					containerStatus.LastTerminationState = *terminationState
 				}
 			}
-			containerStatus.RestartCount += 1
+
+			doNotCount := false
+			// Only count dead containers terminated after last time we observed,
+			if lastObservedTime, ok := lastObservedTime[dockerContainerName]; ok {
+				// We need to inspect the container for the finish timestamp;
+				// skip the insepction if it's already performed.
+				if !inspected {
+					result := self.inspectContainer(value.ID, dockerContainerName, terminationMessagePath)
+					if result.err == nil && result.status.State.Termination != nil {
+						terminationState = &result.status.State
+					}
+				}
+
+				if terminationState != nil && terminationState.Termination.FinishedAt.After(lastObservedTime.Time) {
+					doNotCount = true
+				} else {
+					// The container finished before the last observation. No
+					// need to examine/count the older containers. Mark the
+					// container name as done.
+					containerDone.Insert(dockerContainerName)
+				}
+			}
+
+			if !doNotCount {
+				containerStatus.RestartCount += 1
+			}
 			statuses[dockerContainerName] = containerStatus
 			continue
 		}
@@ -294,6 +334,10 @@ func (self *DockerManager) GetPodStatus(pod *api.Pod) (*api.PodStatus, error) {
 			}
 		} else {
 			// Add user container information.
+			if oldStatus, found := oldStatuses[dockerContainerName]; found {
+				// Use the last observed restart count if it's available.
+				result.status.RestartCount = oldStatus.RestartCount
+			}
 			statuses[dockerContainerName] = result.status
 		}
 	}
@@ -303,8 +347,14 @@ func (self *DockerManager) GetPodStatus(pod *api.Pod) (*api.PodStatus, error) {
 		if status, found := statuses[container.Name]; found {
 			containerStatus = status
 		} else {
-			// The container has not been created yet. Check image is ready on
-			// the node or not.
+			// No active/dead docker containers are found.
+			if oldStatus, found := oldStatuses[container.Name]; found {
+				// Some states may be lost due to GC; apply the last observed
+				// values if possible.
+				containerStatus.RestartCount = oldStatus.RestartCount
+				containerStatus.LastTerminationState = oldStatus.LastTerminationState
+			}
+			//Check image is ready on the node or not.
 			// TODO: If we integrate DockerPuller into DockerManager, we can
 			// record the pull failure and eliminate the image checking below.
 			image := container.Image

pkg/kubelet/kubelet_test.go

@@ -3833,3 +3833,90 @@ func TestGetPodCreationFailureReason(t *testing.T) {
 		}
 	}
 }
+
+func TestGetRestartCount(t *testing.T) {
+	testKubelet := newTestKubelet(t)
+	testKubelet.fakeCadvisor.On("MachineInfo").Return(&cadvisorApi.MachineInfo{}, nil)
+	kubelet := testKubelet.kubelet
+	fakeDocker := testKubelet.fakeDocker
+
+	containers := []api.Container{
+		{Name: "bar"},
+	}
+	pod := api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			UID:       "12345678",
+			Name:      "foo",
+			Namespace: "new",
+		},
+		Spec: api.PodSpec{
+			Containers: containers,
+		},
+	}
+
+	// format is // k8s_<container-id>_<pod-fullname>_<pod-uid>
+	names := []string{"/k8s_bar." + strconv.FormatUint(dockertools.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
+
+	// Helper function for verifying the restart count.
+	verifyRestartCount := func(pod *api.Pod, expectedCount int) api.PodStatus {
+		status, err := kubelet.generatePodStatus(pod)
+		if err != nil {
+			t.Errorf("unexpected error %v", err)
+		}
+		restartCount := status.ContainerStatuses[0].RestartCount
+		if restartCount != expectedCount {
+			t.Errorf("expected %d restart count, got %d", expectedCount, restartCount)
+		}
+		return status
+	}
+
+	// Container "bar" has failed twice; create two dead docker containers.
+	// 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"}}
+	pod.Status = verifyRestartCount(&pod, 1)
+
+	// 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"}}
+	pod.Status = verifyRestartCount(&pod, 2)
+
+	// All dead containers have been GC'd. The restart count should persist
+	// (i.e., remain the same).
+	fakeDocker.ExitedContainerList = []docker.APIContainers{}
+	verifyRestartCount(&pod, 2)
+}