From e3bc2dea22a79e53c2016184af233644158b083d Mon Sep 17 00:00:00 2001 From: Alexander Lyon Date: Thu, 25 Jun 2020 11:45:09 +0200 Subject: [PATCH 1/3] Try to import docker images before falling back to building --- pkg/skaffold/build/cache/lookup.go | 39 ++++++++++++++++++++++++- pkg/skaffold/build/cache/lookup_test.go | 4 +++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index abc96c5c715..e526ebeee13 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -19,8 +19,11 @@ package cache import ( "context" "fmt" + "io/ioutil" "sync" + "github.com/sirupsen/logrus" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" @@ -52,7 +55,10 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cach entry, cacheHit := c.artifactCache[hash] if !cacheHit { - return needsBuilding{hash: hash} + if entry, err = c.tryImport(ctx, a, tag, hash); err != nil { + logrus.Debugf("Could not import artifact from Docker, building instead (%s)", err) + return needsBuilding{hash: hash} + } } if c.imagesAreLocal { @@ -108,3 +114,34 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, entry ImageD return needsBuilding{hash: hash} } + +func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, hash string) (ImageDetails, error) { + entry := ImageDetails{} + + if !c.client.ImageExists(ctx, tag) { + logrus.Debugf("Importing artifact %s from docker registry", tag) + err := c.client.Pull(ctx, ioutil.Discard, tag) + if err != nil { + return entry, err + } + } else { + logrus.Debugf("Importing artifact %s from local docker", tag) + } + + imageID, err := c.client.ImageID(ctx, a.ImageName) + if err != nil { + return entry, err + } + + if imageID != "" { + entry.ID = imageID + } + + if digest, err := docker.RemoteDigest(tag, c.insecureRegistries); err == nil { + logrus.Debugf("Added digest for %s to cache entry", tag) + entry.Digest = digest + } + + c.artifactCache[hash] = entry + return entry, nil +} diff --git a/pkg/skaffold/build/cache/lookup_test.go b/pkg/skaffold/build/cache/lookup_test.go index 53a0c24ad5e..62ac9882206 100644 --- a/pkg/skaffold/build/cache/lookup_test.go +++ b/pkg/skaffold/build/cache/lookup_test.go @@ -40,6 +40,8 @@ func TestLookupLocal(t *testing.T) { { description: "miss", hasher: mockHasher("thehash"), + api: &testutil.FakeAPIClient{}, + cache: map[string]ImageDetails{}, expected: needsBuilding{hash: "thehash"}, }, { @@ -134,6 +136,8 @@ func TestLookupRemote(t *testing.T) { { description: "miss", hasher: mockHasher("hash"), + api: &testutil.FakeAPIClient{ErrImagePull: true}, + cache: map[string]ImageDetails{}, expected: needsBuilding{hash: "hash"}, }, { From fc94b235c1276fa506f0135697c010edd320a29a Mon Sep 17 00:00:00 2001 From: Alexander Lyon Date: Mon, 10 Aug 2020 10:41:51 +0200 Subject: [PATCH 2/3] Add flag to skaffold.yaml f --- docs/content/en/schemas/v2beta8.json | 7 +++++++ pkg/skaffold/build/cache/cache.go | 4 +++- pkg/skaffold/build/cache/lookup.go | 4 ++++ pkg/skaffold/build/cache/retrieve_test.go | 4 ++-- pkg/skaffold/build/local/types.go | 8 ++++++++ pkg/skaffold/runner/new.go | 7 ++++++- pkg/skaffold/schema/latest/config.go | 4 ++++ 7 files changed, 34 insertions(+), 4 deletions(-) diff --git a/docs/content/en/schemas/v2beta8.json b/docs/content/en/schemas/v2beta8.json index 30c4d0bc965..a6b1381d34b 100755 --- a/docs/content/en/schemas/v2beta8.json +++ b/docs/content/en/schemas/v2beta8.json @@ -1872,6 +1872,12 @@ "description": "should images be pushed to a registry. If not specified, images are pushed only if the current Kubernetes context connects to a remote cluster.", "x-intellij-html-description": "should images be pushed to a registry. If not specified, images are pushed only if the current Kubernetes context connects to a remote cluster." }, + "tryImportMissing": { + "type": "boolean", + "description": "whether to attempt to import artifacts from Docker (either a local or remote registry) if not in the cache.", + "x-intellij-html-description": "whether to attempt to import artifacts from Docker (either a local or remote registry) if not in the cache.", + "default": "false" + }, "useBuildkit": { "type": "boolean", "description": "use BuildKit to build Docker images.", @@ -1887,6 +1893,7 @@ }, "preferredOrder": [ "push", + "tryImportMissing", "useDockerCLI", "useBuildkit", "concurrency" diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index dad459700c8..749ebf4f3b2 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -49,6 +49,7 @@ type cache struct { insecureRegistries map[string]bool cacheFile string imagesAreLocal bool + tryImportMissing bool hashForArtifact func(ctx context.Context, a *latest.Artifact) (string, error) } @@ -64,7 +65,7 @@ type Config interface { } // NewCache returns the current state of the cache -func NewCache(cfg Config, imagesAreLocal bool, dependencies DependencyLister) (Cache, error) { +func NewCache(cfg Config, imagesAreLocal bool, tryImportMissing bool, dependencies DependencyLister) (Cache, error) { if !cfg.CacheArtifacts() { return &noCache{}, nil } @@ -92,6 +93,7 @@ func NewCache(cfg Config, imagesAreLocal bool, dependencies DependencyLister) (C insecureRegistries: cfg.GetInsecureRegistries(), cacheFile: cacheFile, imagesAreLocal: imagesAreLocal, + tryImportMissing: tryImportMissing, hashForArtifact: func(ctx context.Context, a *latest.Artifact) (string, error) { return getHashForArtifact(ctx, dependencies, a, cfg.Mode()) }, diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index e526ebeee13..caa9d6b6679 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -116,6 +116,10 @@ func (c *cache) lookupRemote(ctx context.Context, hash, tag string, entry ImageD } func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, hash string) (ImageDetails, error) { + if !c.tryImportMissing { + return ImageDetails{}, fmt.Errorf("import of missing images disabled") + } + entry := ImageDetails{} if !c.client.ImageExists(ctx, tag) { diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index 3c6c293ac4a..c5f7fe6372f 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -129,7 +129,7 @@ func TestCacheBuildLocal(t *testing.T) { cfg := &mockConfig{ cacheFile: tmpDir.Path("cache"), } - artifactCache, err := NewCache(cfg, true, deps) + artifactCache, err := NewCache(cfg, true, false, deps) t.CheckNoError(err) // First build: Need to build both artifacts @@ -224,7 +224,7 @@ func TestCacheBuildRemote(t *testing.T) { cfg := &mockConfig{ cacheFile: tmpDir.Path("cache"), } - artifactCache, err := NewCache(cfg, false, deps) + artifactCache, err := NewCache(cfg, false, false, deps) t.CheckNoError(err) // First build: Need to build both artifacts diff --git a/pkg/skaffold/build/local/types.go b/pkg/skaffold/build/local/types.go index 153c8f87907..f20277c4f1f 100644 --- a/pkg/skaffold/build/local/types.go +++ b/pkg/skaffold/build/local/types.go @@ -36,6 +36,7 @@ type Builder struct { localDocker docker.LocalDaemon localCluster bool pushImages bool + tryImportMissing bool prune bool pruneChildren bool skipTests bool @@ -88,12 +89,15 @@ func NewBuilder(cfg Config) (*Builder, error) { pushImages = *cfg.Pipeline().Build.LocalBuild.Push } + tryImportMissing := cfg.Pipeline().Build.LocalBuild.TryImportMissing + return &Builder{ cfg: *cfg.Pipeline().Build.LocalBuild, kubeContext: cfg.GetKubeContext(), localDocker: localDocker, localCluster: localCluster, pushImages: pushImages, + tryImportMissing: tryImportMissing, skipTests: cfg.SkipTests(), mode: cfg.Mode(), prune: cfg.Prune(), @@ -107,6 +111,10 @@ func (b *Builder) PushImages() bool { return b.pushImages } +func (b *Builder) TryImportMissing() bool { + return b.tryImportMissing +} + // Prune uses the docker API client to remove all images built with Skaffold func (b *Builder) Prune(ctx context.Context, out io.Writer) error { return b.localDocker.Prune(ctx, out, b.builtImages, b.pruneChildren) diff --git a/pkg/skaffold/runner/new.go b/pkg/skaffold/runner/new.go index 4bf23c13cca..0240dd70bca 100644 --- a/pkg/skaffold/runner/new.go +++ b/pkg/skaffold/runner/new.go @@ -55,6 +55,11 @@ func NewForConfig(runCtx *runcontext.RunContext) (*SkaffoldRunner, error) { return nil, fmt.Errorf("creating builder: %w", err) } + tryImportMissing := false + if localBuilder, ok := builder.(*local.Builder); ok { + tryImportMissing = localBuilder.TryImportMissing() + } + labeller := deploy.NewLabeller(runCtx.AddSkaffoldLabels(), runCtx.CustomLabels()) tester := getTester(runCtx, imagesAreLocal) syncer := getSyncer(runCtx) @@ -74,7 +79,7 @@ func NewForConfig(runCtx *runcontext.RunContext) (*SkaffoldRunner, error) { return append(buildDependencies, testDependencies...), nil } - artifactCache, err := cache.NewCache(runCtx, imagesAreLocal, depLister) + artifactCache, err := cache.NewCache(runCtx, imagesAreLocal, tryImportMissing, depLister) if err != nil { return nil, fmt.Errorf("initializing cache: %w", err) } diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 55a798d64f6..984191f283d 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -217,6 +217,10 @@ type LocalBuild struct { // connects to a remote cluster. Push *bool `yaml:"push,omitempty"` + // TryImportMissing whether to attempt to import artifacts from + // Docker (either a local or remote registry) if not in the cache. + TryImportMissing bool `yaml:"tryImportMissing,omitempty"` + // UseDockerCLI use `docker` command-line interface instead of Docker Engine APIs. UseDockerCLI bool `yaml:"useDockerCLI,omitempty"` From 21de9ea44c2cf4696dd1b9b2e1ab7fddd70f0810 Mon Sep 17 00:00:00 2001 From: Alexander Lyon Date: Thu, 10 Sep 2020 11:37:54 +0100 Subject: [PATCH 3/3] Introduce RWMutex to prevent concurrent read/write during to cache during lookup --- pkg/skaffold/build/cache/cache.go | 2 + pkg/skaffold/build/cache/details.go | 2 + pkg/skaffold/build/cache/lookup.go | 4 ++ pkg/skaffold/build/cache/retrieve.go | 4 ++ pkg/skaffold/build/cache/retrieve_test.go | 63 +++++++++++++++++++++++ 5 files changed, 75 insertions(+) diff --git a/pkg/skaffold/build/cache/cache.go b/pkg/skaffold/build/cache/cache.go index 749ebf4f3b2..0f15d941268 100644 --- a/pkg/skaffold/build/cache/cache.go +++ b/pkg/skaffold/build/cache/cache.go @@ -21,6 +21,7 @@ import ( "fmt" "io/ioutil" "path/filepath" + "sync" homedir "github.com/mitchellh/go-homedir" "github.com/sirupsen/logrus" @@ -45,6 +46,7 @@ type ArtifactCache map[string]ImageDetails // cache holds any data necessary for accessing the cache type cache struct { artifactCache ArtifactCache + cacheMutex sync.RWMutex client docker.LocalDaemon insecureRegistries map[string]bool cacheFile string diff --git a/pkg/skaffold/build/cache/details.go b/pkg/skaffold/build/cache/details.go index 6bb880ca7b7..512dc930cef 100644 --- a/pkg/skaffold/build/cache/details.go +++ b/pkg/skaffold/build/cache/details.go @@ -111,8 +111,10 @@ func (d needsPushing) Push(ctx context.Context, out io.Writer, c *cache) error { } // Update cache + c.cacheMutex.Lock() e := c.artifactCache[d.hash] e.Digest = digest c.artifactCache[d.hash] = e + c.cacheMutex.Unlock() return nil } diff --git a/pkg/skaffold/build/cache/lookup.go b/pkg/skaffold/build/cache/lookup.go index caa9d6b6679..98b3274a256 100644 --- a/pkg/skaffold/build/cache/lookup.go +++ b/pkg/skaffold/build/cache/lookup.go @@ -53,7 +53,9 @@ func (c *cache) lookup(ctx context.Context, a *latest.Artifact, tag string) cach return failed{err: fmt.Errorf("getting hash for artifact %q: %s", a.ImageName, err)} } + c.cacheMutex.RLock() entry, cacheHit := c.artifactCache[hash] + c.cacheMutex.RUnlock() if !cacheHit { if entry, err = c.tryImport(ctx, a, tag, hash); err != nil { logrus.Debugf("Could not import artifact from Docker, building instead (%s)", err) @@ -146,6 +148,8 @@ func (c *cache) tryImport(ctx context.Context, a *latest.Artifact, tag string, h entry.Digest = digest } + c.cacheMutex.Lock() c.artifactCache[hash] = entry + c.cacheMutex.Unlock() return entry, nil } diff --git a/pkg/skaffold/build/cache/retrieve.go b/pkg/skaffold/build/cache/retrieve.go index 25546c73090..380a7cf8a3e 100644 --- a/pkg/skaffold/build/cache/retrieve.go +++ b/pkg/skaffold/build/cache/retrieve.go @@ -89,7 +89,9 @@ func (c *cache) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, ar } // Image is already built + c.cacheMutex.RLock() entry := c.artifactCache[result.Hash()] + c.cacheMutex.RUnlock() tag := tags[artifact.ImageName] var uniqueTag string @@ -166,7 +168,9 @@ func (c *cache) addArtifacts(ctx context.Context, bRes []build.Artifact, hashByN entry.ID = imageID } + c.cacheMutex.Lock() c.artifactCache[hashByName[a.ImageName]] = entry + c.cacheMutex.Unlock() } return nil diff --git a/pkg/skaffold/build/cache/retrieve_test.go b/pkg/skaffold/build/cache/retrieve_test.go index c5f7fe6372f..fafb7bd3076 100644 --- a/pkg/skaffold/build/cache/retrieve_test.go +++ b/pkg/skaffold/build/cache/retrieve_test.go @@ -261,6 +261,69 @@ func TestCacheBuildRemote(t *testing.T) { }) } +func TestCacheFindMissing(t *testing.T) { + testutil.Run(t, "", func(t *testutil.T) { + tmpDir := t.NewTempDir(). + Write("dep1", "content1"). + Write("dep2", "content2"). + Write("dep3", "content3"). + Chdir() + + tags := map[string]string{ + "artifact1": "artifact1:tag1", + "artifact2": "artifact2:tag2", + } + artifacts := []*latest.Artifact{ + {ImageName: "artifact1", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, + {ImageName: "artifact2", ArtifactType: latest.ArtifactType{DockerArtifact: &latest.DockerArtifact{}}}, + } + deps := depLister(map[string][]string{ + "artifact1": {"dep1", "dep2"}, + "artifact2": {"dep3"}, + }) + + // Mock Docker + dockerDaemon := fakeLocalDaemon(&testutil.FakeAPIClient{}) + t.Override(&docker.NewAPIClient, func(docker.Config) (docker.LocalDaemon, error) { + return dockerDaemon, nil + }) + t.Override(&docker.DefaultAuthHelper, stubAuth{}) + t.Override(&docker.RemoteDigest, func(ref string, _ map[string]bool) (string, error) { + switch ref { + case "artifact1:tag1": + return "sha256:51ae7fa00c92525c319404a3a6d400e52ff9372c5a39cb415e0486fe425f3165", nil + case "artifact2:tag2": + return "sha256:35bdf2619f59e6f2372a92cb5486f4a0bf9b86e0e89ee0672864db6ed9c51539", nil + default: + return "", errors.New("unknown remote tag") + } + }) + + // Mock args builder + t.Override(&docker.EvalBuildArgs, func(mode config.RunMode, workspace string, a *latest.DockerArtifact) (map[string]*string, error) { + return a.BuildArgs, nil + }) + + // Create cache + cfg := &mockConfig{ + cacheFile: tmpDir.Path("cache"), + } + artifactCache, err := NewCache(cfg, false, true, deps) + t.CheckNoError(err) + + // Because the artifacts are in the docker registry, we expect them to be imported correctly. + builder := &mockBuilder{dockerDaemon: dockerDaemon, push: false} + bRes, err := artifactCache.Build(context.Background(), ioutil.Discard, tags, artifacts, builder.BuildAndTest) + + t.CheckNoError(err) + t.CheckDeepEqual(0, len(builder.built)) + t.CheckDeepEqual(2, len(bRes)) + // Artifacts should always be returned in their original order + t.CheckDeepEqual("artifact1", bRes[0].ImageName) + t.CheckDeepEqual("artifact2", bRes[1].ImageName) + }) +} + type mockConfig struct { runcontext.RunContext // Embedded to provide the default values. cacheFile string