From fe7c2f3883f5c6bd3516562c2cd94c437360dc6b Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Thu, 7 Mar 2019 15:23:06 -0800 Subject: [PATCH 1/3] Only get image list from docker daemon once per build loop --- pkg/skaffold/build/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/skaffold/build/cache.go b/pkg/skaffold/build/cache.go index 74c868c865a..7e46419f669 100644 --- a/pkg/skaffold/build/cache.go +++ b/pkg/skaffold/build/cache.go @@ -180,7 +180,7 @@ func (c *Cache) resolveCachedArtifact(ctx context.Context, out io.Writer, a *lat color.Green.Fprint(out, ". Retagging") } if details.needsPush { - color.Green.Fprint(out, ". Pushing") + color.Green.Fprint(out, ". Pushing.") } color.Default.Fprintln(out) From a038779027afe463496093d83b03779c0fe92636 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Thu, 7 Mar 2019 16:41:23 -0800 Subject: [PATCH 2/3] Update tests so that image list is retrieved from local daemon only once --- pkg/skaffold/build/cache.go | 49 ++++++-- pkg/skaffold/build/cache_test.go | 191 +++++++++++++++++++++++++----- pkg/skaffold/docker/image.go | 37 +----- pkg/skaffold/docker/image_test.go | 131 -------------------- pkg/skaffold/runner/runner.go | 2 +- 5 files changed, 205 insertions(+), 205 deletions(-) diff --git a/pkg/skaffold/build/cache.go b/pkg/skaffold/build/cache.go index 7e46419f669..fe7e743560c 100644 --- a/pkg/skaffold/build/cache.go +++ b/pkg/skaffold/build/cache.go @@ -31,6 +31,7 @@ import ( "time" skafconfig "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config" + "github.com/docker/docker/api/types" "github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/cmd/config" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" @@ -64,6 +65,7 @@ type Cache struct { artifactCache ArtifactCache client docker.LocalDaemon builder Builder + imageList []types.ImageSummary cacheFile string useCache bool needsPush bool @@ -74,11 +76,12 @@ var ( hashForArtifact = getHashForArtifact localCluster = config.GetLocalCluster remoteDigest = docker.RemoteDigest + newDockerCilent = docker.NewAPIClient noCache = &Cache{} ) // NewCache returns the current state of the cache -func NewCache(builder Builder, opts *skafconfig.SkaffoldOptions, needsPush bool) *Cache { +func NewCache(ctx context.Context, builder Builder, opts *skafconfig.SkaffoldOptions, needsPush bool) *Cache { if !opts.CacheArtifacts { return noCache } @@ -92,11 +95,15 @@ func NewCache(builder Builder, opts *skafconfig.SkaffoldOptions, needsPush bool) logrus.Warnf("Error retrieving artifact cache, not using skaffold cache: %v", err) return noCache } - client, err := docker.NewAPIClient() + client, err := newDockerCilent() if err != nil { logrus.Warnf("Error retrieving local daemon client, not using skaffold cache: %v", err) return noCache } + imageList, err := client.ImageList(ctx, types.ImageListOptions{}) + if err != nil { + logrus.Warn("Unable to get list of images from local docker daemon, won't be checked for cache.") + } return &Cache{ artifactCache: cache, cacheFile: cf, @@ -104,6 +111,7 @@ func NewCache(builder Builder, opts *skafconfig.SkaffoldOptions, needsPush bool) client: client, builder: builder, needsPush: needsPush, + imageList: imageList, } } @@ -171,7 +179,7 @@ func (c *Cache) resolveCachedArtifact(ctx context.Context, out io.Writer, a *lat color.Default.Fprintf(out, " - %s: ", a.ImageName) if details.needsRebuild { - color.Red.Fprintln(out, "Not found. Rebuilding") + color.Red.Fprintln(out, "Not found. Rebuilding.") return nil, nil } @@ -239,7 +247,7 @@ func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, a *latest.Art }, nil } // Check for a local image with the same digest as the image we want to build - prebuiltImage, err := c.retrievePrebuiltImage(ctx, imageDetails) + prebuiltImage, err := c.retrievePrebuiltImage(imageDetails) if err != nil { return nil, errors.Wrapf(err, "getting prebuilt image") } @@ -255,15 +263,32 @@ func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, a *latest.Art }, nil } -func (c *Cache) retrievePrebuiltImage(ctx context.Context, details ImageDetails) (string, error) { - img, err := c.client.FindTaggedImage(ctx, details.ID, details.Digest) - if err != nil { - return "", errors.Wrap(err, "unable to find tagged image") - } - if img == "" { - return img, errors.New("no prebuilt image") +func (c *Cache) retrievePrebuiltImage(details ImageDetails) (string, error) { + for _, r := range c.imageList { + if r.ID == details.ID && details.ID != "" { + if len(r.RepoTags) == 0 { + return "", nil + } + return r.RepoTags[0], nil + } + if details.Digest == "" { + continue + } + for _, d := range r.RepoDigests { + if getDigest(d) == details.Digest { + // Return a tagged version of this image, since we can't retag an image in the image@sha256: format + if len(r.RepoTags) > 0 { + return r.RepoTags[0], nil + } + } + } } - return img, nil + return "", errors.New("no prebuilt image") +} + +func getDigest(img string) string { + ref, _ := name.NewDigest(img, name.WeakValidation) + return ref.DigestStr() } func imageExistsRemotely(image, digest string) bool { diff --git a/pkg/skaffold/build/cache_test.go b/pkg/skaffold/build/cache_test.go index 81784ac58ec..d9850a0aefa 100644 --- a/pkg/skaffold/build/cache_test.go +++ b/pkg/skaffold/build/cache_test.go @@ -35,7 +35,10 @@ import ( ) var ( - digest = "sha256:0000000000000000000000000000000000000000000000000000000000000000" + digest = "sha256:0000000000000000000000000000000000000000000000000000000000000000" + digestOne = "sha256:1111111111111111111111111111111111111111111111111111111111111111" + image = fmt.Sprintf("image@%s", digest) + imageOne = fmt.Sprintf("image1@%s", digestOne) ) var defaultArtifactCache = ArtifactCache{"hash": ImageDetails{ @@ -50,16 +53,14 @@ func mockHashForArtifact(hashes map[string]string) func(context.Context, Builder } func Test_NewCache(t *testing.T) { - client, err := docker.NewAPIClient() - if err != nil { - t.Fatalf("error gettting docker api client: %v", err) - } tests := []struct { updateCacheFile bool needsPush bool + updateClient bool name string opts *config.SkaffoldOptions expectedCache *Cache + api *testutil.FakeAPIClient cacheFileContents interface{} }{ { @@ -69,10 +70,22 @@ func Test_NewCache(t *testing.T) { opts: &config.SkaffoldOptions{ CacheArtifacts: true, }, + updateClient: true, + api: &testutil.FakeAPIClient{ + ImageSummaries: []types.ImageSummary{ + { + ID: "image", + }, + }, + }, expectedCache: &Cache{ artifactCache: defaultArtifactCache, useCache: true, - client: client, + imageList: []types.ImageSummary{ + { + ID: "image", + }, + }, }, }, { @@ -80,19 +93,21 @@ func Test_NewCache(t *testing.T) { cacheFileContents: defaultArtifactCache, needsPush: true, updateCacheFile: true, + updateClient: true, opts: &config.SkaffoldOptions{ CacheArtifacts: true, }, + api: &testutil.FakeAPIClient{}, expectedCache: &Cache{ artifactCache: defaultArtifactCache, useCache: true, - client: client, needsPush: true, }, }, { name: "valid cache file exists, but useCache is false", cacheFileContents: defaultArtifactCache, + api: &testutil.FakeAPIClient{}, opts: &config.SkaffoldOptions{}, expectedCache: &Cache{}, }, @@ -111,12 +126,24 @@ func Test_NewCache(t *testing.T) { t.Run(test.name, func(t *testing.T) { cacheFile := createTempCacheFile(t, test.cacheFileContents) - if test.updateCacheFile { test.expectedCache.cacheFile = cacheFile } test.opts.CacheFile = cacheFile - actualCache := NewCache(nil, test.opts, test.needsPush) + + originalDockerClient := newDockerCilent + newDockerCilent = func() (docker.LocalDaemon, error) { + return docker.NewLocalDaemon(test.api, nil), nil + } + defer func() { + newDockerCilent = originalDockerClient + }() + + if test.updateClient { + test.expectedCache.client = docker.NewLocalDaemon(test.api, nil) + } + + actualCache := NewCache(context.Background(), nil, test.opts, test.needsPush) // cmp.Diff cannot access unexported fields, so use reflect.DeepEqual here directly if !reflect.DeepEqual(test.expectedCache, actualCache) { @@ -317,18 +344,16 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) { name: "image in cache, prebuilt image exists, remote cluster", artifact: &latest.Artifact{ImageName: "image"}, hashes: map[string]string{"image": "hash"}, - api: testutil.FakeAPIClient{ - ImageSummaries: []types.ImageSummary{ + cache: &Cache{ + useCache: true, + artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, + imageList: []types.ImageSummary{ { RepoDigests: []string{fmt.Sprintf("image@%s", digest)}, RepoTags: []string{"anotherimage:hash"}, }, }, }, - cache: &Cache{ - useCache: true, - artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, - }, digest: digest, expected: &cachedArtifactDetails{ needsRetag: true, @@ -342,18 +367,16 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) { artifact: &latest.Artifact{ImageName: "image"}, hashes: map[string]string{"image": "hash"}, localCluster: true, - api: testutil.FakeAPIClient{ - ImageSummaries: []types.ImageSummary{ + cache: &Cache{ + useCache: true, + artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, + imageList: []types.ImageSummary{ { RepoDigests: []string{fmt.Sprintf("image@%s", digest)}, RepoTags: []string{"anotherimage:hash"}, }, }, }, - cache: &Cache{ - useCache: true, - artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, - }, digest: digest, expected: &cachedArtifactDetails{ needsRetag: true, @@ -366,19 +389,17 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) { artifact: &latest.Artifact{ImageName: "image"}, hashes: map[string]string{"image": "hash"}, localCluster: true, - api: testutil.FakeAPIClient{ - ImageSummaries: []types.ImageSummary{ + cache: &Cache{ + useCache: true, + needsPush: true, + artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, + imageList: []types.ImageSummary{ { RepoDigests: []string{fmt.Sprintf("image@%s", digest)}, RepoTags: []string{"anotherimage:hash"}, }, }, }, - cache: &Cache{ - useCache: true, - needsPush: true, - artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}}, - }, digest: digest, expected: &cachedArtifactDetails{ needsRetag: true, @@ -552,3 +573,117 @@ func TestCacheHasher(t *testing.T) { }) } } + +func TestRetrievePrebuiltImage(t *testing.T) { + tests := []struct { + name string + cache *Cache + imageDetails ImageDetails + shouldErr bool + expected string + }{ + { + name: "one image id exists", + cache: &Cache{ + imageList: []types.ImageSummary{ + { + RepoTags: []string{"image:mytag"}, + RepoDigests: []string{image}, + }, + { + RepoTags: []string{"image1:latest"}, + RepoDigests: []string{imageOne}, + }, + }, + }, + imageDetails: ImageDetails{ + Digest: digest, + }, + expected: "image:mytag", + }, + { + name: "no image id exists", + cache: &Cache{ + imageList: []types.ImageSummary{ + { + RepoTags: []string{"image:mytag"}, + RepoDigests: []string{image}, + }, + { + RepoTags: []string{"image:mytag"}, + RepoDigests: []string{image}, + }, + }, + }, + shouldErr: true, + imageDetails: ImageDetails{ + Digest: "dne", + }, + expected: "", + }, + { + name: "one image id exists", + cache: &Cache{ + imageList: []types.ImageSummary{ + { + RepoTags: []string{"image1", "image2"}, + ID: "something", + }, + { + RepoTags: []string{"image3"}, + ID: "imageid", + }, + }, + }, + imageDetails: ImageDetails{ + ID: "imageid", + }, + expected: "image3", + }, + { + name: "multiple image ids exist", + cache: &Cache{ + imageList: []types.ImageSummary{ + { + RepoTags: []string{"image1", "image2"}, + ID: "something", + }, + { + RepoTags: []string{"image3", "image4"}, + ID: "imageid", + }, + }, + }, + imageDetails: ImageDetails{ + ID: "imageid", + }, + expected: "image3", + }, + { + name: "no image id exists", + cache: &Cache{ + imageList: []types.ImageSummary{ + { + RepoTags: []string{"image1", "image2"}, + ID: "something", + }, + { + RepoTags: []string{"image3"}, + ID: "somethingelse", + }, + }, + }, + imageDetails: ImageDetails{ + ID: "imageid", + }, + shouldErr: true, + expected: "", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + actual, err := test.cache.retrievePrebuiltImage(test.imageDetails) + testutil.CheckErrorAndDeepEqual(t, test.shouldErr, err, test.expected, actual) + }) + } +} diff --git a/pkg/skaffold/docker/image.go b/pkg/skaffold/docker/image.go index b0f69983bb2..978d3bef4ad 100644 --- a/pkg/skaffold/docker/image.go +++ b/pkg/skaffold/docker/image.go @@ -31,7 +31,6 @@ import ( "github.com/docker/docker/pkg/progress" "github.com/docker/docker/pkg/streamformatter" "github.com/docker/docker/pkg/term" - "github.com/google/go-containerregistry/pkg/name" v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -50,7 +49,7 @@ type LocalDaemon interface { Tag(ctx context.Context, image, ref string) error ImageID(ctx context.Context, ref string) (string, error) RepoDigest(ctx context.Context, ref string) (string, error) - FindTaggedImage(ctx context.Context, id, digest string) (string, error) + ImageList(ctx context.Context, options types.ImageListOptions) ([]types.ImageSummary, error) ImageExists(ctx context.Context, ref string) bool } @@ -288,32 +287,9 @@ func (l *localDaemon) ImageID(ctx context.Context, ref string) (string, error) { return image.ID, nil } -// FindTaggedImage returns the name of an image with a matching id or digest -func (l *localDaemon) FindTaggedImage(ctx context.Context, id, digest string) (string, error) { - resp, err := l.apiClient.ImageList(ctx, types.ImageListOptions{}) - if err != nil { - return "", errors.Wrap(err, "listing images") - } - for _, r := range resp { - if r.ID == id && id != "" { - if len(r.RepoTags) == 0 { - return "", nil - } - return r.RepoTags[0], nil - } - if digest == "" { - continue - } - for _, d := range r.RepoDigests { - if getDigest(d) == digest { - // Return a tagged version of this image, since we can't retag an image in the image@sha256: format - if len(r.RepoTags) > 0 { - return r.RepoTags[0], nil - } - } - } - } - return "", nil +// ImageList returns a list of all images in the local daemon +func (l *localDaemon) ImageList(ctx context.Context, options types.ImageListOptions) ([]types.ImageSummary, error) { + return l.apiClient.ImageList(ctx, options) } // RepoDigest returns a repo digest for the given ref @@ -333,11 +309,6 @@ func (l *localDaemon) ImageExists(ctx context.Context, ref string) bool { return err == nil } -func getDigest(img string) string { - ref, _ := name.NewDigest(img, name.WeakValidation) - return ref.DigestStr() -} - // GetBuildArgs gives the build args flags for docker build. func GetBuildArgs(a *latest.DockerArtifact) []string { var args []string diff --git a/pkg/skaffold/docker/image_test.go b/pkg/skaffold/docker/image_test.go index 1ab5f0825a8..a234f25358e 100644 --- a/pkg/skaffold/docker/image_test.go +++ b/pkg/skaffold/docker/image_test.go @@ -18,7 +18,6 @@ package docker import ( "context" - "fmt" "io/ioutil" "os" "testing" @@ -26,7 +25,6 @@ import ( "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/util" "github.com/GoogleContainerTools/skaffold/testutil" - "github.com/docker/docker/api/types" "github.com/google/go-cmp/cmp" ) @@ -222,67 +220,6 @@ func TestGetBuildArgs(t *testing.T) { } } -var ( - digest = "sha256:0000000000000000000000000000000000000000000000000000000000000000" - digestOne = "sha256:1111111111111111111111111111111111111111111111111111111111111111" - image = fmt.Sprintf("image@%s", digest) - imageOne = fmt.Sprintf("image1@%s", digestOne) -) - -func TestFindTaggedImageByDigest(t *testing.T) { - tests := []struct { - name string - digest string - imageSummaries []types.ImageSummary - expected string - }{ - { - name: "one image id exists", - digest: digest, - imageSummaries: []types.ImageSummary{ - { - RepoTags: []string{"image:mytag"}, - RepoDigests: []string{image}, - }, - { - RepoTags: []string{"image1:latest"}, - RepoDigests: []string{imageOne}, - }, - }, - expected: "image:mytag", - }, - { - name: "no image id exists", - digest: "dne", - imageSummaries: []types.ImageSummary{ - { - RepoTags: []string{"image:mytag"}, - RepoDigests: []string{image}, - }, - { - RepoTags: []string{"image:mytag"}, - RepoDigests: []string{image}, - }, - }, - expected: "", - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - api := &testutil.FakeAPIClient{ - ImageSummaries: test.imageSummaries, - } - - localDocker := &localDaemon{ - apiClient: api, - } - - actual, err := localDocker.FindTaggedImage(context.Background(), "", test.digest) - testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, actual) - }) - } -} - func TestImageExists(t *testing.T) { client, _ := NewAPIClient() t.Log(client.ImageExists(context.Background(), "somethingranodm")) @@ -381,71 +318,3 @@ func TestRepoDigest(t *testing.T) { }) } } -func TestFindTaggedImageByID(t *testing.T) { - tests := []struct { - name string - id string - imageSummaries []types.ImageSummary - expected string - }{ - { - name: "one image id exists", - id: "imageid", - imageSummaries: []types.ImageSummary{ - { - RepoTags: []string{"image1", "image2"}, - ID: "something", - }, - { - RepoTags: []string{"image3"}, - ID: "imageid", - }, - }, - expected: "image3", - }, - { - name: "multiple image ids exist", - id: "imageid", - imageSummaries: []types.ImageSummary{ - { - RepoTags: []string{"image1", "image2"}, - ID: "something", - }, - { - RepoTags: []string{"image3", "image4"}, - ID: "imageid", - }, - }, - expected: "image3", - }, - { - name: "no image id exists", - id: "imageid", - imageSummaries: []types.ImageSummary{ - { - RepoTags: []string{"image1", "image2"}, - ID: "something", - }, - { - RepoTags: []string{"image3"}, - ID: "somethingelse", - }, - }, - expected: "", - }, - } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - api := &testutil.FakeAPIClient{ - ImageSummaries: test.imageSummaries, - } - - localDocker := &localDaemon{ - apiClient: api, - } - - actual, err := localDocker.FindTaggedImage(context.Background(), test.id, "") - testutil.CheckErrorAndDeepEqual(t, false, err, test.expected, actual) - }) - } -} diff --git a/pkg/skaffold/runner/runner.go b/pkg/skaffold/runner/runner.go index 98856735e11..4d018bcf847 100644 --- a/pkg/skaffold/runner/runner.go +++ b/pkg/skaffold/runner/runner.go @@ -324,7 +324,7 @@ func (r *SkaffoldRunner) BuildAndTest(ctx context.Context, out io.Writer, artifa return nil, errors.Wrap(err, "generating tag") } - artifactCache := build.NewCache(r.Builder, r.opts, r.needsPush) + artifactCache := build.NewCache(ctx, r.Builder, r.opts, r.needsPush) artifactsToBuild, res := artifactCache.RetrieveCachedArtifacts(ctx, out, artifacts) bRes, err := r.Build(ctx, out, tags, artifactsToBuild) if err != nil { From f4575d95cb1e0bdaf9b3d09f1e880fc4891aa430 Mon Sep 17 00:00:00 2001 From: Priya Wadhwa Date: Thu, 7 Mar 2019 16:41:41 -0800 Subject: [PATCH 3/3] update docs --- docs/content/en/schemas/v1beta6.json | 11 ----------- docs/content/en/schemas/v1beta7.json | 11 +++++++++++ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/content/en/schemas/v1beta6.json b/docs/content/en/schemas/v1beta6.json index ad789defb44..cdc96da81c4 100755 --- a/docs/content/en/schemas/v1beta6.json +++ b/docs/content/en/schemas/v1beta6.json @@ -1424,17 +1424,6 @@ "x-intellij-html-description": "beta describes how to do a build on the local docker daemon and optionally push to a repository." }, "LocalDir": { - "properties": { - "initImage": { - "type": "string", - "description": "image used to run init container which mounts kaniko context.", - "x-intellij-html-description": "image used to run init container which mounts kaniko context." - } - }, - "preferredOrder": [ - "initImage" - ], - "additionalProperties": false, "description": "configures how Kaniko mounts sources directly via an `emptyDir` volume.", "x-intellij-html-description": "configures how Kaniko mounts sources directly via an emptyDir volume." }, diff --git a/docs/content/en/schemas/v1beta7.json b/docs/content/en/schemas/v1beta7.json index cdc96da81c4..ad789defb44 100755 --- a/docs/content/en/schemas/v1beta7.json +++ b/docs/content/en/schemas/v1beta7.json @@ -1424,6 +1424,17 @@ "x-intellij-html-description": "beta describes how to do a build on the local docker daemon and optionally push to a repository." }, "LocalDir": { + "properties": { + "initImage": { + "type": "string", + "description": "image used to run init container which mounts kaniko context.", + "x-intellij-html-description": "image used to run init container which mounts kaniko context." + } + }, + "preferredOrder": [ + "initImage" + ], + "additionalProperties": false, "description": "configures how Kaniko mounts sources directly via an `emptyDir` volume.", "x-intellij-html-description": "configures how Kaniko mounts sources directly via an emptyDir volume." },