From 56bde2df9ff14fd1a193664bd578ae2459d4bf12 Mon Sep 17 00:00:00 2001 From: Random-Liu Date: Fri, 27 May 2016 13:42:09 -0700 Subject: [PATCH 1/2] Cache image history --- pkg/kubelet/dockertools/images.go | 79 +++++++++++++++++++++--------- pkg/kubelet/dockertools/manager.go | 2 +- 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/pkg/kubelet/dockertools/images.go b/pkg/kubelet/dockertools/images.go index 647327fb13c..6f7be9e0eb2 100644 --- a/pkg/kubelet/dockertools/images.go +++ b/pkg/kubelet/dockertools/images.go @@ -18,54 +18,85 @@ package dockertools import ( "fmt" + "sync" "github.com/golang/glog" dockertypes "github.com/docker/engine-api/types" runtime "k8s.io/kubernetes/pkg/kubelet/container" - "k8s.io/kubernetes/pkg/util/sets" ) // imageStatsProvider exposes stats about all images currently available. type imageStatsProvider struct { + sync.Mutex + // layers caches the current layers, key is the layer ID. + layers map[string]*dockertypes.ImageHistory + // imageToLayerIDs maps image to its layer IDs. + imageToLayerIDs map[string][]string // Docker remote API client c DockerInterface } +func newImageStatsProvider(c DockerInterface) *imageStatsProvider { + return &imageStatsProvider{ + layers: make(map[string]*dockertypes.ImageHistory), + imageToLayerIDs: make(map[string][]string), + c: c, + } +} + func (isp *imageStatsProvider) ImageStats() (*runtime.ImageStats, error) { images, err := isp.c.ListImages(dockertypes.ImageListOptions{}) if err != nil { return nil, fmt.Errorf("failed to list docker images - %v", err) } - // A map of all the image layers to its corresponding size. - imageMap := sets.NewString() - ret := &runtime.ImageStats{} + // Take the lock to protect the cache + isp.Lock() + defer isp.Unlock() + // Create new cache each time, this is a little more memory consuming, but: + // * ImageStats is only called every 10 seconds + // * We use pointers and reference to copy cache elements. + // The memory usage should be acceptable. + // TODO(random-liu): Add more logic to implement in place cache update. + newLayers := make(map[string]*dockertypes.ImageHistory) + newImageToLayerIDs := make(map[string][]string) for _, image := range images { - // Get information about the various layers of each docker image. - history, err := isp.c.ImageHistory(image.ID) - if err != nil { - glog.V(2).Infof("failed to get history of docker image %v - %v", image, err) - continue - } - // Store size information of each layer. - for _, layer := range history { - // Skip empty layers. - if layer.Size == 0 { - glog.V(10).Infof("skipping image layer %v with size 0", layer) + layerIDs, ok := isp.imageToLayerIDs[image.ID] + if !ok { + // Get information about the various layers of the given docker image. + history, err := isp.c.ImageHistory(image.ID) + if err != nil { + // Skip the image and inspect again in next ImageStats if the image is still there + glog.V(2).Infof("failed to get history of docker image %+v - %v", image, err) continue } - key := layer.ID - // Some of the layers are empty. - // We are hoping that these layers are unique to each image. - // Still keying with the CreatedBy field to be safe. - if key == "" || key == "" { - key = key + layer.CreatedBy + // Cache each layer + for i := range history { + layer := &history[i] + key := layer.ID + // Some of the layers are empty. + // We are hoping that these layers are unique to each image. + // Still keying with the CreatedBy field to be safe. + if key == "" || key == "" { + key = key + layer.CreatedBy + } + layerIDs = append(layerIDs, key) + newLayers[key] = layer } - if !imageMap.Has(key) { - ret.TotalStorageBytes += uint64(layer.Size) + } else { + for _, layerID := range layerIDs { + newLayers[layerID] = isp.layers[layerID] } - imageMap.Insert(key) } + newImageToLayerIDs[image.ID] = layerIDs } + ret := &runtime.ImageStats{} + // Calculate the total storage bytes + for _, layer := range newLayers { + ret.TotalStorageBytes += uint64(layer.Size) + } + // Update current cache + isp.layers = newLayers + isp.imageToLayerIDs = newImageToLayerIDs return ret, nil } diff --git a/pkg/kubelet/dockertools/manager.go b/pkg/kubelet/dockertools/manager.go index fa552d41abf..f55f47396f2 100644 --- a/pkg/kubelet/dockertools/manager.go +++ b/pkg/kubelet/dockertools/manager.go @@ -254,7 +254,7 @@ func NewDockerManager( cpuCFSQuota: cpuCFSQuota, enableCustomMetrics: enableCustomMetrics, configureHairpinMode: hairpinMode, - imageStatsProvider: &imageStatsProvider{client}, + imageStatsProvider: newImageStatsProvider(client), seccompProfileRoot: seccompProfileRoot, } dm.runner = lifecycle.NewHandlerRunner(httpClient, dm, dm) From 52a3d8a19d8250948842ec0ff901bbd10e6189e7 Mon Sep 17 00:00:00 2001 From: Random-Liu Date: Fri, 27 May 2016 13:42:31 -0700 Subject: [PATCH 2/2] Add unit test for image history cache --- pkg/kubelet/dockertools/fake_docker_client.go | 18 +- pkg/kubelet/dockertools/images_test.go | 235 +++++++++++++++++- 2 files changed, 244 insertions(+), 9 deletions(-) diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index 02f302fa03b..686a2ccd76e 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -30,7 +30,6 @@ import ( dockercontainer "github.com/docker/engine-api/types/container" "k8s.io/kubernetes/pkg/api" - "k8s.io/kubernetes/pkg/util/sets" ) // FakeDockerClient is a simple fake docker client, so that kubelet can be run for testing without requiring a real docker setup. @@ -49,7 +48,6 @@ type FakeDockerClient struct { Created []string Stopped []string Removed []string - RemovedImages sets.String VersionInfo dockertypes.Version Information dockertypes.Info ExecInspect *dockertypes.ContainerExecInspect @@ -68,10 +66,9 @@ func NewFakeDockerClient() *FakeDockerClient { func NewFakeDockerClientWithVersion(version, apiVersion string) *FakeDockerClient { return &FakeDockerClient{ - VersionInfo: dockertypes.Version{Version: version, APIVersion: apiVersion}, - Errors: make(map[string]error), - RemovedImages: sets.String{}, - ContainerMap: make(map[string]*dockertypes.ContainerJSON), + VersionInfo: dockertypes.Version{Version: version, APIVersion: apiVersion}, + Errors: make(map[string]error), + ContainerMap: make(map[string]*dockertypes.ContainerJSON), } } @@ -471,6 +468,7 @@ func (f *FakeDockerClient) InspectExec(id string) (*dockertypes.ContainerExecIns } func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error) { + f.called = append(f.called, "list_images") err := f.popError("list_images") return f.Images, err } @@ -478,7 +476,12 @@ func (f *FakeDockerClient) ListImages(opts dockertypes.ImageListOptions) ([]dock func (f *FakeDockerClient) RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error) { err := f.popError("remove_image") if err == nil { - f.RemovedImages.Insert(image) + for i := range f.Images { + if f.Images[i].ID == image { + f.Images = append(f.Images[:i], f.Images[i+1:]...) + break + } + } } return []dockertypes.ImageDelete{{Deleted: image}}, err } @@ -538,6 +541,7 @@ func (f *FakeDockerPuller) IsImagePresent(name string) (bool, error) { func (f *FakeDockerClient) ImageHistory(id string) ([]dockertypes.ImageHistory, error) { f.Lock() defer f.Unlock() + f.called = append(f.called, "image_history") history := f.ImageHistoryMap[id] return history, nil } diff --git a/pkg/kubelet/dockertools/images_test.go b/pkg/kubelet/dockertools/images_test.go index 2607e0b982d..6ef35562958 100644 --- a/pkg/kubelet/dockertools/images_test.go +++ b/pkg/kubelet/dockertools/images_test.go @@ -25,10 +25,11 @@ import ( func TestImageStatsNoImages(t *testing.T) { fakeDockerClient := NewFakeDockerClientWithVersion("1.2.3", "1.2") - isp := &imageStatsProvider{fakeDockerClient} + isp := newImageStatsProvider(fakeDockerClient) st, err := isp.ImageStats() as := assert.New(t) as.NoError(err) + as.NoError(fakeDockerClient.AssertCalls([]string{"list_images"})) as.Equal(st.TotalStorageBytes, uint64(0)) } @@ -94,10 +95,240 @@ func TestImageStatsWithImages(t *testing.T) { ID: "busybox-new", }, }) - isp := &imageStatsProvider{fakeDockerClient} + isp := newImageStatsProvider(fakeDockerClient) st, err := isp.ImageStats() as := assert.New(t) as.NoError(err) + as.NoError(fakeDockerClient.AssertCalls([]string{"list_images", "image_history", "image_history", "image_history"})) const expectedOutput uint64 = 1300 as.Equal(expectedOutput, st.TotalStorageBytes, "expected %d, got %d", expectedOutput, st.TotalStorageBytes) } + +func TestImageStatsWithCachedImages(t *testing.T) { + for _, test := range []struct { + oldLayers map[string]*dockertypes.ImageHistory + oldImageToLayerIDs map[string][]string + images []dockertypes.Image + history map[string][]dockertypes.ImageHistory + expectedCalls []string + expectedLayers map[string]*dockertypes.ImageHistory + expectedImageToLayerIDs map[string][]string + expectedTotalStorageSize uint64 + }{ + { + // No cache + oldLayers: make(map[string]*dockertypes.ImageHistory), + oldImageToLayerIDs: make(map[string][]string), + images: []dockertypes.Image{ + { + ID: "busybox", + }, + { + ID: "kubelet", + }, + }, + history: map[string][]dockertypes.ImageHistory{ + "busybox": { + { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + }, + "kubelet": { + { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + }, + expectedCalls: []string{"list_images", "image_history", "image_history"}, + expectedLayers: map[string]*dockertypes.ImageHistory{ + "0123456": { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + "1123456": { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + "baz": { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + "1baz": { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + expectedImageToLayerIDs: map[string][]string{ + "busybox": {"0123456", "baz"}, + "kubelet": {"1123456", "1baz"}, + }, + expectedTotalStorageSize: 1000, + }, + { + // Use cache value + oldLayers: map[string]*dockertypes.ImageHistory{ + "0123456": { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + "baz": { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + }, + oldImageToLayerIDs: map[string][]string{ + "busybox": {"0123456", "baz"}, + }, + images: []dockertypes.Image{ + { + ID: "busybox", + }, + { + ID: "kubelet", + }, + }, + history: map[string][]dockertypes.ImageHistory{ + "busybox": { + { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + }, + "kubelet": { + { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + }, + expectedCalls: []string{"list_images", "image_history"}, + expectedLayers: map[string]*dockertypes.ImageHistory{ + "0123456": { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + "1123456": { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + "baz": { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + "1baz": { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + expectedImageToLayerIDs: map[string][]string{ + "busybox": {"0123456", "baz"}, + "kubelet": {"1123456", "1baz"}, + }, + expectedTotalStorageSize: 1000, + }, + { + // Unused cache value + oldLayers: map[string]*dockertypes.ImageHistory{ + "0123456": { + ID: "0123456", + CreatedBy: "foo", + Size: 100, + }, + "baz": { + ID: "", + CreatedBy: "baz", + Size: 300, + }, + }, + oldImageToLayerIDs: map[string][]string{ + "busybox": {"0123456", "baz"}, + }, + images: []dockertypes.Image{ + { + ID: "kubelet", + }, + }, + history: map[string][]dockertypes.ImageHistory{ + "kubelet": { + { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + }, + expectedCalls: []string{"list_images", "image_history"}, + expectedLayers: map[string]*dockertypes.ImageHistory{ + "1123456": { + ID: "1123456", + CreatedBy: "foo", + Size: 200, + }, + "1baz": { + ID: "", + CreatedBy: "1baz", + Size: 400, + }, + }, + expectedImageToLayerIDs: map[string][]string{ + "kubelet": {"1123456", "1baz"}, + }, + expectedTotalStorageSize: 600, + }, + } { + fakeDockerClient := NewFakeDockerClientWithVersion("1.2.3", "1.2") + fakeDockerClient.InjectImages(test.images) + fakeDockerClient.InjectImageHistory(test.history) + isp := newImageStatsProvider(fakeDockerClient) + isp.layers = test.oldLayers + isp.imageToLayerIDs = test.oldImageToLayerIDs + st, err := isp.ImageStats() + as := assert.New(t) + as.NoError(err) + as.NoError(fakeDockerClient.AssertCalls(test.expectedCalls)) + as.Equal(test.expectedLayers, isp.layers, "expected %+v, got %+v", test.expectedLayers, isp.layers) + as.Equal(test.expectedImageToLayerIDs, isp.imageToLayerIDs, "expected %+v, got %+v", test.expectedImageToLayerIDs, isp.imageToLayerIDs) + as.Equal(test.expectedTotalStorageSize, st.TotalStorageBytes, "expected %d, got %d", test.expectedTotalStorageSize, st.TotalStorageBytes) + } +}