* Refactor kuberuntime unit test

pkg/kubelet/api/testing/fake_runtime_service.go

@@ -123,10 +123,13 @@ func (r *FakeRuntimeService) RunPodSandbox(config *runtimeApi.PodSandboxConfig)
 	readyState := runtimeApi.PodSandBoxState_READY
 	r.Sandboxes[podSandboxID] = &FakePodSandbox{
 		PodSandboxStatus: runtimeApi.PodSandboxStatus{
-			Id:          &podSandboxID,
-			Metadata:    config.Metadata,
-			State:       &readyState,
-			CreatedAt:   &createdAt,
+			Id:        &podSandboxID,
+			Metadata:  config.Metadata,
+			State:     &readyState,
+			CreatedAt: &createdAt,
+			Network: &runtimeApi.PodSandboxNetworkStatus{
+				Ip: &FakePodSandboxIP,
+			},
 			Labels:      config.Labels,
 			Annotations: config.Annotations,
 		},
@@ -174,17 +177,8 @@ func (r *FakeRuntimeService) PodSandboxStatus(podSandboxID string) (*runtimeApi.
 		return nil, fmt.Errorf("pod sandbox %q not found", podSandboxID)
 	}
 
-	return &runtimeApi.PodSandboxStatus{
-		Id:        &podSandboxID,
-		Metadata:  s.Metadata,
-		CreatedAt: s.CreatedAt,
-		State:     s.State,
-		Network: &runtimeApi.PodSandboxNetworkStatus{
-			Ip: &FakePodSandboxIP,
-		},
-		Labels:      s.Labels,
-		Annotations: s.Annotations,
-	}, nil
+	status := s.PodSandboxStatus
+	return &status, nil
 }
 
 func (r *FakeRuntimeService) ListPodSandbox(filter *runtimeApi.PodSandboxFilter) ([]*runtimeApi.PodSandbox, error) {
@@ -228,7 +222,7 @@ func (r *FakeRuntimeService) CreateContainer(podSandboxID string, config *runtim
 
 	// ContainerID should be randomized for real container runtime, but here just use
 	// fixed BuildContainerName() for easily making fake containers.
-	containerID := BuildContainerName(config.Metadata)
+	containerID := BuildContainerName(config.Metadata, podSandboxID)
 	createdAt := time.Now().Unix()
 	createdState := runtimeApi.ContainerState_CREATED
 	imageRef := config.Image.GetImage()
@@ -351,21 +345,8 @@ func (r *FakeRuntimeService) ContainerStatus(containerID string) (*runtimeApi.Co
 		return nil, fmt.Errorf("container %q not found", containerID)
 	}
 
-	return &runtimeApi.ContainerStatus{
-		Id:          c.Id,
-		Metadata:    c.Metadata,
-		State:       c.State,
-		CreatedAt:   c.CreatedAt,
-		Image:       c.Image,
-		ImageRef:    c.ImageRef,
-		Labels:      c.Labels,
-		Annotations: c.Annotations,
-		ExitCode:    c.ExitCode,
-		StartedAt:   c.StartedAt,
-		FinishedAt:  c.FinishedAt,
-		Reason:      c.Reason,
-		Mounts:      c.Mounts,
-	}, nil
+	status := c.ContainerStatus
+	return &status, nil
 }
 
 func (r *FakeRuntimeService) Exec(containerID string, cmd []string, tty bool, stdin io.Reader, stdout, stderr io.WriteCloser) error {

pkg/kubelet/api/testing/utils.go

@@ -22,8 +22,9 @@ import (
 	runtimeApi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime"
 )
 
-func BuildContainerName(metadata *runtimeApi.ContainerMetadata) string {
-	return fmt.Sprintf("%s_%d", metadata.GetName(), metadata.GetAttempt())
+func BuildContainerName(metadata *runtimeApi.ContainerMetadata, sandboxID string) string {
+	// include the sandbox ID to make the container ID unique.
+	return fmt.Sprintf("%s_%s_%d", sandboxID, metadata.GetName(), metadata.GetAttempt())
 }
 
 func BuildSandboxName(metadata *runtimeApi.PodSandboxMetadata) string {

pkg/kubelet/dockertools/container_gc_test.go

@@ -148,6 +148,7 @@ func TestGarbageCollectNoMaxLimit(t *testing.T) {
 	})
 	addPods(gc.podGetter, "foo", "foo1", "foo2", "foo3", "foo4")
 
+	assert.Nil(t, gc.GarbageCollect(kubecontainer.ContainerGCPolicy{MinAge: time.Minute, MaxPerPodContainer: -1, MaxContainers: -1}, true))
 	assert.Len(t, fakeDocker.Removed, 0)
 }
 

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

@@ -47,8 +47,7 @@ func TestRemoveContainer(t *testing.T) {
 	}
 
 	// Create fake sandbox and container
-	_, fakeContainers, err := makeAndSetFakePod(m, fakeRuntime, pod)
-	assert.NoError(t, err)
+	_, fakeContainers := makeAndSetFakePod(t, m, fakeRuntime, pod)
 	assert.Equal(t, len(fakeContainers), 1)
 
 	containerId := fakeContainers[0].GetId()

pkg/kubelet/kuberuntime/kuberuntime_gc.go

@@ -136,6 +136,12 @@ func (cgc *containerGC) removeSandbox(sandboxID string) {
 	}
 }
 
+// isPodDeleted returns true if the pod is already deleted.
+func (cgc *containerGC) isPodDeleted(podUID types.UID) bool {
+	_, found := cgc.podGetter.GetPodByUID(podUID)
+	return !found
+}
+
 // evictableContainers gets all containers that are evictable. Evictable containers are: not running
 // and created more than MinAge ago.
 func (cgc *containerGC) evictableContainers(minAge time.Duration) (containersByEvictUnit, error) {
@@ -179,17 +185,64 @@ func (cgc *containerGC) evictableContainers(minAge time.Duration) (containersByE
 	return evictUnits, nil
 }
 
-// evictableSandboxes gets all sandboxes that are evictable. Evictable sandboxes are: not running
+// evict all containers that are evictable
+func (cgc *containerGC) evictContainers(gcPolicy kubecontainer.ContainerGCPolicy, allSourcesReady bool) error {
+	// Separate containers by evict units.
+	evictUnits, err := cgc.evictableContainers(gcPolicy.MinAge)
+	if err != nil {
+		return err
+	}
+
+	// Remove deleted pod containers if all sources are ready.
+	if allSourcesReady {
+		for key, unit := range evictUnits {
+			if cgc.isPodDeleted(key.uid) {
+				cgc.removeOldestN(unit, len(unit)) // Remove all.
+				delete(evictUnits, key)
+			}
+		}
+	}
+
+	// Enforce max containers per evict unit.
+	if gcPolicy.MaxPerPodContainer >= 0 {
+		cgc.enforceMaxContainersPerEvictUnit(evictUnits, gcPolicy.MaxPerPodContainer)
+	}
+
+	// Enforce max total number of containers.
+	if gcPolicy.MaxContainers >= 0 && evictUnits.NumContainers() > gcPolicy.MaxContainers {
+		// Leave an equal number of containers per evict unit (min: 1).
+		numContainersPerEvictUnit := gcPolicy.MaxContainers / evictUnits.NumEvictUnits()
+		if numContainersPerEvictUnit < 1 {
+			numContainersPerEvictUnit = 1
+		}
+		cgc.enforceMaxContainersPerEvictUnit(evictUnits, numContainersPerEvictUnit)
+
+		// If we still need to evict, evict oldest first.
+		numContainers := evictUnits.NumContainers()
+		if numContainers > gcPolicy.MaxContainers {
+			flattened := make([]containerGCInfo, 0, numContainers)
+			for key := range evictUnits {
+				flattened = append(flattened, evictUnits[key]...)
+			}
+			sort.Sort(byCreated(flattened))
+
+			cgc.removeOldestN(flattened, numContainers-gcPolicy.MaxContainers)
+		}
+	}
+	return nil
+}
+
+// evictSandboxes evicts all sandboxes that are evictable. Evictable sandboxes are: not running
 // and contains no containers at all.
-func (cgc *containerGC) evictableSandboxes(minAge time.Duration) ([]string, error) {
+func (cgc *containerGC) evictSandboxes(minAge time.Duration) error {
 	containers, err := cgc.manager.getKubeletContainers(true)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	sandboxes, err := cgc.manager.getKubeletSandboxes(true)
 	if err != nil {
-		return nil, err
+		return err
 	}
 
 	evictSandboxes := make([]string, 0)
@@ -222,13 +275,10 @@ func (cgc *containerGC) evictableSandboxes(minAge time.Duration) ([]string, erro
 		evictSandboxes = append(evictSandboxes, sandboxID)
 	}
 
-	return evictSandboxes, nil
-}
-
-// isPodDeleted returns true if the pod is already deleted.
-func (cgc *containerGC) isPodDeleted(podUID types.UID) bool {
-	_, found := cgc.podGetter.GetPodByUID(podUID)
-	return !found
+	for _, sandbox := range evictSandboxes {
+		cgc.removeSandbox(sandbox)
+	}
+	return nil
 }
 
 // evictPodLogsDirectories evicts all evictable pod logs directories. Pod logs directories
@@ -278,59 +328,16 @@ func (cgc *containerGC) evictPodLogsDirectories(allSourcesReady bool) error {
 // * gets evictable sandboxes which are not ready and contains no containers.
 // * removes evictable sandboxes.
 func (cgc *containerGC) GarbageCollect(gcPolicy kubecontainer.ContainerGCPolicy, allSourcesReady bool) error {
-	// Separate containers by evict units.
-	evictUnits, err := cgc.evictableContainers(gcPolicy.MinAge)
-	if err != nil {
+	// Remove evictable containers
+	if err := cgc.evictContainers(gcPolicy, allSourcesReady); err != nil {
 		return err
 	}
 
-	// Remove deleted pod containers if all sources are ready.
-	if allSourcesReady {
-		for key, unit := range evictUnits {
-			if cgc.isPodDeleted(key.uid) {
-				cgc.removeOldestN(unit, len(unit)) // Remove all.
-				delete(evictUnits, key)
-			}
-		}
-	}
-
-	// Enforce max containers per evict unit.
-	if gcPolicy.MaxPerPodContainer >= 0 {
-		cgc.enforceMaxContainersPerEvictUnit(evictUnits, gcPolicy.MaxPerPodContainer)
-	}
-
-	// Enforce max total number of containers.
-	if gcPolicy.MaxContainers >= 0 && evictUnits.NumContainers() > gcPolicy.MaxContainers {
-		// Leave an equal number of containers per evict unit (min: 1).
-		numContainersPerEvictUnit := gcPolicy.MaxContainers / evictUnits.NumEvictUnits()
-		if numContainersPerEvictUnit < 1 {
-			numContainersPerEvictUnit = 1
-		}
-		cgc.enforceMaxContainersPerEvictUnit(evictUnits, numContainersPerEvictUnit)
-
-		// If we still need to evict, evict oldest first.
-		numContainers := evictUnits.NumContainers()
-		if numContainers > gcPolicy.MaxContainers {
-			flattened := make([]containerGCInfo, 0, numContainers)
-			for key := range evictUnits {
-				flattened = append(flattened, evictUnits[key]...)
-			}
-			sort.Sort(byCreated(flattened))
-
-			cgc.removeOldestN(flattened, numContainers-gcPolicy.MaxContainers)
-		}
-	}
-
 	// Remove sandboxes with zero containers
-	evictSandboxes, err := cgc.evictableSandboxes(sandboxMinGCAge)
-	if err != nil {
+	if err := cgc.evictSandboxes(sandboxMinGCAge); err != nil {
 		return err
 	}
-	for _, sandbox := range evictSandboxes {
-		cgc.removeSandbox(sandbox)
-	}
 
 	// Remove pod sandbox log directory
-	// TODO(random-liu): Add legacy container log localtion cleanup.
 	return cgc.evictPodLogsDirectories(allSourcesReady)
 }