Merge pull request #14590 from kubernetes/revert-14035-kubelet/client/server

Revert "Refactor image manager for client/server implementation of th…

pkg/kubelet/image_manager.go

@@ -22,11 +22,12 @@ import (
 	"sync"
 	"time"
 
+	docker "github.com/fsouza/go-dockerclient"
 	"github.com/golang/glog"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/client/record"
 	"k8s.io/kubernetes/pkg/kubelet/cadvisor"
-	"k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
 	"k8s.io/kubernetes/pkg/util"
 	"k8s.io/kubernetes/pkg/util/sets"
 )
@@ -58,8 +59,8 @@ type ImageGCPolicy struct {
 }
 
 type realImageManager struct {
-	// Container runtime
+	// Connection to the Docker daemon.
-	runtime container.Runtime
+	dockerClient dockertools.DockerInterface
 
 	// Records of images and their use.
 	imageRecords     map[string]*imageRecord
@@ -90,7 +91,7 @@ type imageRecord struct {
 	size int64
 }
 
-func newImageManager(runtime container.Runtime, cadvisorInterface cadvisor.Interface, recorder record.EventRecorder, nodeRef *api.ObjectReference, policy ImageGCPolicy) (imageManager, error) {
+func newImageManager(dockerClient dockertools.DockerInterface, cadvisorInterface cadvisor.Interface, recorder record.EventRecorder, nodeRef *api.ObjectReference, policy ImageGCPolicy) (imageManager, error) {
 	// Validate policy.
 	if policy.HighThresholdPercent < 0 || policy.HighThresholdPercent > 100 {
 		return nil, fmt.Errorf("invalid HighThresholdPercent %d, must be in range [0-100]", policy.HighThresholdPercent)
@@ -99,7 +100,7 @@ func newImageManager(runtime container.Runtime, cadvisorInterface cadvisor.Inter
 		return nil, fmt.Errorf("invalid LowThresholdPercent %d, must be in range [0-100]", policy.LowThresholdPercent)
 	}
 	im := &realImageManager{
-		runtime:      runtime,
+		dockerClient: dockerClient,
 		policy:       policy,
 		imageRecords: make(map[string]*imageRecord),
 		cadvisor:     cadvisorInterface,
@@ -129,21 +130,21 @@ func (im *realImageManager) Start() error {
 }
 
 func (im *realImageManager) detectImages(detected time.Time) error {
-	images, err := im.runtime.ListImages()
+	images, err := im.dockerClient.ListImages(docker.ListImagesOptions{})
 	if err != nil {
 		return err
 	}
-	pods, err := im.runtime.GetPods(true)
+	containers, err := im.dockerClient.ListContainers(docker.ListContainersOptions{
+		All: true,
+	})
 	if err != nil {
 		return err
 	}
 
 	// Make a set of images in use by containers.
 	imagesInUse := sets.NewString()
-	for _, pod := range pods {
+	for _, container := range containers {
-		for _, container := range pod.Containers {
+		imagesInUse.Insert(container.Image)
-			imagesInUse.Insert(container.Image)
-		}
 	}
 
 	// Add new images and record those being used.
@@ -162,11 +163,11 @@ func (im *realImageManager) detectImages(detected time.Time) error {
 		}
 
 		// Set last used time to now if the image is being used.
-		if isImageUsed(image, imagesInUse) {
+		if isImageUsed(&image, imagesInUse) {
 			im.imageRecords[image.ID].lastUsed = now
 		}
 
-		im.imageRecords[image.ID].size = image.Size
+		im.imageRecords[image.ID].size = image.VirtualSize
 	}
 
 	// Remove old images from our records.
@@ -252,7 +253,7 @@ func (im *realImageManager) freeSpace(bytesToFree int64) (int64, error) {
 
 		// Remove image. Continue despite errors.
 		glog.Infof("[ImageManager]: Removing image %q to free %d bytes", image.id, image.size)
-		err := im.runtime.RemoveImage(container.ImageSpec{Image: image.id})
+		err := im.dockerClient.RemoveImage(image.id)
 		if err != nil {
 			lastErr = err
 			continue
@@ -286,12 +287,12 @@ func (ev byLastUsedAndDetected) Less(i, j int) bool {
 	}
 }
 
-func isImageUsed(image container.Image, imagesInUse sets.String) bool {
+func isImageUsed(image *docker.APIImages, imagesInUse sets.String) bool {
 	// Check the image ID and all the RepoTags.
 	if _, ok := imagesInUse[image.ID]; ok {
 		return true
 	}
-	for _, tag := range image.Tags {
+	for _, tag := range image.RepoTags {
 		if _, ok := imagesInUse[tag]; ok {
 			return true
 		}

pkg/kubelet/image_manager_test.go

@@ -21,27 +21,30 @@ import (
 	"testing"
 	"time"
 
+	docker "github.com/fsouza/go-dockerclient"
 	cadvisorApiV2 "github.com/google/cadvisor/info/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/kubernetes/pkg/client/record"
 	"k8s.io/kubernetes/pkg/kubelet/cadvisor"
-	"k8s.io/kubernetes/pkg/kubelet/container"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
-	"k8s.io/kubernetes/pkg/types"
+	"k8s.io/kubernetes/pkg/util/sets"
 )
 
 var zero time.Time
 
-func newRealImageManager(policy ImageGCPolicy) (*realImageManager, *container.FakeRuntime, *cadvisor.Mock) {
+func newRealImageManager(policy ImageGCPolicy) (*realImageManager, *dockertools.FakeDockerClient, *cadvisor.Mock) {
-	fakeRuntime := &container.FakeRuntime{}
+	fakeDocker := &dockertools.FakeDockerClient{
+		RemovedImages: sets.NewString(),
+	}
 	mockCadvisor := new(cadvisor.Mock)
 	return &realImageManager{
-		runtime:      fakeRuntime,
+		dockerClient: fakeDocker,
 		policy:       policy,
 		imageRecords: make(map[string]*imageRecord),
 		cadvisor:     mockCadvisor,
 		recorder:     &record.FakeRecorder{},
-	}, fakeRuntime, mockCadvisor
+	}, fakeDocker, mockCadvisor
 }
 
 // Accessors used for thread-safe testing.
@@ -64,33 +67,29 @@ func imageName(id int) string {
 }
 
 // Make an image with the specified ID.
-func makeImage(id int, size int64) container.Image {
+func makeImage(id int, size int64) docker.APIImages {
-	return container.Image{
+	return docker.APIImages{
-		ID:   imageName(id),
+		ID:          imageName(id),
-		Size: size,
+		VirtualSize: size,
 	}
 }
 
 // Make a container with the specified ID. It will use the image with the same ID.
-func makeContainer(id int) *container.Container {
+func makeContainer(id int) docker.APIContainers {
-	return &container.Container{
+	return docker.APIContainers{
-		ID:    types.UID(fmt.Sprintf("container-%d", id)),
+		ID:    fmt.Sprintf("container-%d", id),
 		Image: imageName(id),
 	}
 }
 
 func TestDetectImagesInitialDetect(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 
 	startTime := time.Now().Add(-time.Millisecond)
@@ -110,17 +109,13 @@ func TestDetectImagesInitialDetect(t *testing.T) {
 
 func TestDetectImagesWithNewImage(t *testing.T) {
 	// Just one image initially.
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 
 	err := manager.detectImages(zero)
@@ -129,7 +124,7 @@ func TestDetectImagesWithNewImage(t *testing.T) {
 	assert.Equal(manager.imageRecordsLen(), 2)
 
 	// Add a new image.
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 1024),
 		makeImage(2, 1024),
@@ -155,17 +150,13 @@ func TestDetectImagesWithNewImage(t *testing.T) {
 }
 
 func TestDetectImagesContainerStopped(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 
 	err := manager.detectImages(zero)
@@ -176,8 +167,7 @@ func TestDetectImagesContainerStopped(t *testing.T) {
 	require.True(t, ok)
 
 	// Simulate container being stopped.
-	fakeRuntime.ContainerList = []*container.Container{}
+	fakeDocker.ContainerList = []docker.APIContainers{}
-	fakeRuntime.PodList = []*container.Pod{}
 	err = manager.detectImages(time.Now())
 	require.NoError(t, err)
 	assert.Equal(manager.imageRecordsLen(), 2)
@@ -192,17 +182,13 @@ func TestDetectImagesContainerStopped(t *testing.T) {
 }
 
 func TestDetectImagesWithRemovedImages(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 
 	err := manager.detectImages(zero)
@@ -211,63 +197,48 @@ func TestDetectImagesWithRemovedImages(t *testing.T) {
 	assert.Equal(manager.imageRecordsLen(), 2)
 
 	// Simulate both images being removed.
-	fakeRuntime.ImageList = []container.Image{}
+	fakeDocker.Images = []docker.APIImages{}
 	err = manager.detectImages(time.Now())
 	require.NoError(t, err)
 	assert.Equal(manager.imageRecordsLen(), 0)
 }
 
 func TestFreeSpaceImagesInUseContainersAreIgnored(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 
 	spaceFreed, err := manager.freeSpace(2048)
 	assert := assert.New(t)
 	require.NoError(t, err)
 	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
+	assert.Len(fakeDocker.RemovedImages, 1)
+	assert.True(fakeDocker.RemovedImages.Has(imageName(0)))
 }
 
 func TestFreeSpaceRemoveByLeastRecentlyUsed(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(0),
-			Containers: []*container.Container{
+		makeContainer(1),
-				makeContainer(0),
-				makeContainer(1),
-			},
-		},
 	}
 
 	// Make 1 be more recently used than 0.
 	require.NoError(t, manager.detectImages(zero))
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(1),
-			Containers: []*container.Container{
-				makeContainer(1),
-			},
-		},
 	}
 	require.NoError(t, manager.detectImages(time.Now()))
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{}
-		{
-			Containers: []*container.Container{},
-		},
-	}
 	require.NoError(t, manager.detectImages(time.Now()))
 	require.Equal(t, manager.imageRecordsLen(), 2)
 
@@ -275,51 +246,53 @@ func TestFreeSpaceRemoveByLeastRecentlyUsed(t *testing.T) {
 	assert := assert.New(t)
 	require.NoError(t, err)
 	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
+	assert.Len(fakeDocker.RemovedImages, 1)
+	assert.True(fakeDocker.RemovedImages.Has(imageName(0)))
 }
 
 func TestFreeSpaceTiesBrokenByDetectedTime(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 	}
-	fakeRuntime.PodList = []*container.Pod{
+	fakeDocker.ContainerList = []docker.APIContainers{
-		{
+		makeContainer(0),
-			Containers: []*container.Container{
-				makeContainer(0),
-			},
-		},
 	}
 
 	// Make 1 more recently detected but used at the same time as 0.
 	require.NoError(t, manager.detectImages(zero))
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		makeImage(1, 2048),
 	}
+	fakeDocker.ContainerList = []docker.APIContainers{
+		makeContainer(0),
+		makeContainer(1),
+	}
 	require.NoError(t, manager.detectImages(time.Now()))
-	fakeRuntime.PodList = []*container.Pod{}
+	fakeDocker.ContainerList = []docker.APIContainers{}
 	require.NoError(t, manager.detectImages(time.Now()))
 	require.Equal(t, manager.imageRecordsLen(), 2)
 
 	spaceFreed, err := manager.freeSpace(1024)
 	assert := assert.New(t)
 	require.NoError(t, err)
-	assert.EqualValues(2048, spaceFreed)
+	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
+	assert.Len(fakeDocker.RemovedImages, 1)
+	assert.True(fakeDocker.RemovedImages.Has(imageName(0)))
 }
 
 func TestFreeSpaceImagesAlsoDoesLookupByRepoTags(t *testing.T) {
-	manager, fakeRuntime, _ := newRealImageManager(ImageGCPolicy{})
+	manager, fakeDocker, _ := newRealImageManager(ImageGCPolicy{})
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 1024),
 		{
-			ID:   "5678",
+			ID:          "5678",
-			Tags: []string{"potato", "salad"},
+			RepoTags:    []string{"potato", "salad"},
-			Size: 2048,
+			VirtualSize: 2048,
 		},
 	}
-	fakeRuntime.ContainerList = []*container.Container{
+	fakeDocker.ContainerList = []docker.APIContainers{
 		{
 			ID:    "c5678",
 			Image: "salad",
@@ -330,7 +303,8 @@ func TestFreeSpaceImagesAlsoDoesLookupByRepoTags(t *testing.T) {
 	assert := assert.New(t)
 	require.NoError(t, err)
 	assert.EqualValues(1024, spaceFreed)
-	assert.Len(fakeRuntime.ImageList, 1)
+	assert.Len(fakeDocker.RemovedImages, 1)
+	assert.True(fakeDocker.RemovedImages.Has(imageName(0)))
 }
 
 func TestGarbageCollectBelowLowThreshold(t *testing.T) {
@@ -365,14 +339,14 @@ func TestGarbageCollectBelowSuccess(t *testing.T) {
 		HighThresholdPercent: 90,
 		LowThresholdPercent:  80,
 	}
-	manager, fakeRuntime, mockCadvisor := newRealImageManager(policy)
+	manager, fakeDocker, mockCadvisor := newRealImageManager(policy)
 
 	// Expect 95% usage and most of it gets freed.
 	mockCadvisor.On("DockerImagesFsInfo").Return(cadvisorApiV2.FsInfo{
 		Usage:    950,
 		Capacity: 1000,
 	}, nil)
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 450),
 	}
 
@@ -384,14 +358,14 @@ func TestGarbageCollectNotEnoughFreed(t *testing.T) {
 		HighThresholdPercent: 90,
 		LowThresholdPercent:  80,
 	}
-	manager, fakeRuntime, mockCadvisor := newRealImageManager(policy)
+	manager, fakeDocker, mockCadvisor := newRealImageManager(policy)
 
 	// Expect 95% usage and little of it gets freed.
 	mockCadvisor.On("DockerImagesFsInfo").Return(cadvisorApiV2.FsInfo{
 		Usage:    950,
 		Capacity: 1000,
 	}, nil)
-	fakeRuntime.ImageList = []container.Image{
+	fakeDocker.Images = []docker.APIImages{
 		makeImage(0, 50),
 	}
 

pkg/kubelet/kubelet.go

@@ -237,7 +237,10 @@ func NewMainKubelet(
 	if err != nil {
 		return nil, err
 	}
-
+	imageManager, err := newImageManager(dockerClient, cadvisorInterface, recorder, nodeRef, imageGCPolicy)
+	if err != nil {
+		return nil, fmt.Errorf("failed to initialize image manager: %v", err)
+	}
 	diskSpaceManager, err := newDiskSpaceManager(cadvisorInterface, diskSpacePolicy)
 	if err != nil {
 		return nil, fmt.Errorf("failed to initialize disk manager: %v", err)
@@ -275,6 +278,7 @@ func NewMainKubelet(
 		recorder:                       recorder,
 		cadvisor:                       cadvisorInterface,
 		containerGC:                    containerGC,
+		imageManager:                   imageManager,
 		diskSpaceManager:               diskSpaceManager,
 		statusManager:                  statusManager,
 		volumeManager:                  volumeManager,
@@ -356,13 +360,6 @@ func NewMainKubelet(
 		return nil, fmt.Errorf("unsupported container runtime %q specified", containerRuntime)
 	}
 
-	// setup imageManager
-	imageManager, err := newImageManager(klet.containerRuntime, cadvisorInterface, recorder, nodeRef, imageGCPolicy)
-	if err != nil {
-		return nil, fmt.Errorf("failed to initialize image manager: %v", err)
-	}
-	klet.imageManager = imageManager
-
 	// Setup container manager, can fail if the devices hierarchy is not mounted
 	// (it is required by Docker however).
 	containerManager, err := newContainerManager(mounter, cadvisorInterface, dockerDaemonContainer, systemContainer, resourceContainer)