Merge pull request #1755 from priyawadhwa/improve-caching

Improve caching

Author: dgageot

pkg/skaffold/build/cache.go

@@ -97,8 +97,7 @@ func NewCache(ctx context.Context, builder Builder, opts *skafconfig.SkaffoldOpt
 	}
 	client, err := newDockerCilent()
 	if err != nil {
-		logrus.Warnf("Error retrieving local daemon client, not using skaffold cache: %v", err)
-		return noCache
+		logrus.Warnf("Error retrieving local daemon client; local daemon will not be used as a cache: %v", err)
 	}
 	imageList, err := client.ImageList(ctx, types.ImageListOptions{})
 	if err != nil {
@@ -230,7 +229,7 @@ func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, a *latest.Art
 			needsRebuild: true,
 		}, nil
 	}
-	hashTag := fmt.Sprintf("%s:%s", a.ImageName, hash)
+	hashTag := HashTag(a)
 
 	// Check if we are using a local cluster
 	var existsRemotely bool
@@ -239,8 +238,14 @@ func (c *Cache) retrieveCachedArtifactDetails(ctx context.Context, a *latest.Art
 		existsRemotely = imageExistsRemotely(hashTag, imageDetails.Digest)
 	}
 
+	if existsRemotely {
+		return &cachedArtifactDetails{
+			hashTag: hashTag,
+		}, nil
+	}
+
 	// See if this image exists in the local daemon
-	if c.client.ImageExists(ctx, hashTag) {
+	if c.client != nil && c.client.ImageExists(ctx, hashTag) {
 		return &cachedArtifactDetails{
 			needsPush: (!existsRemotely && !localCluster) || (localCluster && c.needsPush),
 			hashTag:   hashTag,
@@ -325,9 +330,12 @@ func (c *Cache) CacheArtifacts(ctx context.Context, artifacts []*latest.Artifact
 		if digest == "" {
 			logrus.Debugf("couldn't get image digest for %s, will try to cache just image id (expected with a local cluster)", tags[a.ImageName])
 		}
-		id, err := c.client.ImageID(ctx, tags[a.ImageName])
-		if err != nil {
-			logrus.Debugf("couldn't get image id for %s", tags[a.ImageName])
+		var id string
+		if c.client != nil {
+			id, err = c.client.ImageID(ctx, tags[a.ImageName])
+			if err != nil {
+				logrus.Debugf("couldn't get image id for %s", tags[a.ImageName])
+			}
 		}
 		if id == "" && digest == "" {
 			logrus.Debugf("both image id and digest are empty for %s, skipping caching", tags[a.ImageName])
@@ -343,7 +351,7 @@ func (c *Cache) CacheArtifacts(ctx context.Context, artifacts []*latest.Artifact
 
 // Retag retags newly built images in the format [imageName:workspaceHash] and pushes them if using a remote cluster
 func (c *Cache) Retag(ctx context.Context, out io.Writer, artifactsToBuild []*latest.Artifact, buildArtifacts []Artifact) {
-	if !c.useCache || len(artifactsToBuild) == 0 {
+	if !c.useCache || len(artifactsToBuild) == 0 || c.client == nil {
 		return
 	}
 	tags := map[string]string{}
@@ -369,8 +377,12 @@ func (c *Cache) Retag(ctx context.Context, out io.Writer, artifactsToBuild []*la
 	}
 }
 
-// Check local daemon for img digest
+// Check local daemon for img digest, check remote digest if that doesn't work
 func (c *Cache) retrieveImageDigest(ctx context.Context, img string) (string, error) {
+	if c.client == nil {
+		// Check for remote digest
+		return docker.RemoteDigest(img)
+	}
 	repoDigest, err := c.client.RepoDigest(ctx, img)
 	if err != nil {
 		return docker.RemoteDigest(img)
@@ -430,3 +442,7 @@ func cacheHasher(p string) (string, error) {
 	}
 	return hex.EncodeToString(h.Sum(nil)), nil
 }
+
+func HashTag(a *latest.Artifact) string {
+	return fmt.Sprintf("%s:%s", a.ImageName, a.WorkspaceHash)
+}

pkg/skaffold/build/cache_test.go

@@ -272,7 +272,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 		artifact     *latest.Artifact
 		hashes       map[string]string
 		digest       string
-		api          testutil.FakeAPIClient
+		api          *testutil.FakeAPIClient
 		cache        *Cache
 		expected     *cachedArtifactDetails
 	}{
@@ -299,7 +299,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 			name:     "image in cache and exists remotely, remote cluster",
 			artifact: &latest.Artifact{ImageName: "image"},
 			hashes:   map[string]string{"image": "hash"},
-			api: testutil.FakeAPIClient{
+			api: &testutil.FakeAPIClient{
 				TagToImageID: map[string]string{"image:hash": "image:tag"},
 				ImageSummaries: []types.ImageSummary{
 					{
@@ -322,7 +322,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 			artifact:     &latest.Artifact{ImageName: "image"},
 			hashes:       map[string]string{"image": "hash"},
 			localCluster: true,
-			api: testutil.FakeAPIClient{
+			api: &testutil.FakeAPIClient{
 				TagToImageID: map[string]string{"image:hash": "image:tag"},
 				ImageSummaries: []types.ImageSummary{
 					{
@@ -356,10 +356,7 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 			},
 			digest: digest,
 			expected: &cachedArtifactDetails{
-				needsRetag:    true,
-				needsPush:     true,
-				prebuiltImage: "anotherimage:hash",
-				hashTag:       "image:hash",
+				hashTag: "image:hash",
 			},
 		},
 		{
@@ -408,6 +405,20 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 				hashTag:       "image:hash",
 			},
 		},
+		{
+			name:     "no local daemon, image exists remotely",
+			artifact: &latest.Artifact{ImageName: "image"},
+			hashes:   map[string]string{"image": "hash"},
+			cache: &Cache{
+				useCache:      true,
+				needsPush:     true,
+				artifactCache: ArtifactCache{"hash": ImageDetails{Digest: digest}},
+			},
+			digest: digest,
+			expected: &cachedArtifactDetails{
+				hashTag: "image:hash",
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -434,7 +445,9 @@ func TestRetrieveCachedArtifactDetails(t *testing.T) {
 				remoteDigest = originalRemoteDigest
 			}()
 
-			test.cache.client = docker.NewLocalDaemon(&test.api, nil)
+			if test.api != nil {
+				test.cache.client = docker.NewLocalDaemon(test.api, nil)
+			}
 			actual, err := test.cache.retrieveCachedArtifactDetails(context.Background(), test.artifact)
 			if err != nil {
 				t.Fatalf("error retrieving artifact details: %v", err)

pkg/skaffold/build/kaniko/run.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/kaniko/sources"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
@@ -57,6 +58,11 @@ func (b *Builder) run(ctx context.Context, out io.Writer, artifact *latest.Artif
 	args = append(args, b.AdditionalFlags...)
 	args = append(args, docker.GetBuildArgs(artifact.DockerArtifact)...)
 
+	if artifact.WorkspaceHash != "" {
+		hashTag := build.HashTag(artifact)
+		args = append(args, []string{"--destination", hashTag}...)
+	}
+
 	if b.Cache != nil {
 		args = append(args, "--cache=true")
 		if b.Cache.Repo != "" {

pkg/skaffold/plugin/environments/gcb/desc.go

@@ -19,6 +19,7 @@ package gcb
 import (
 	"fmt"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/defaults"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
@@ -28,7 +29,12 @@ import (
 )
 
 func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, object string) (*cloudbuild.Build, error) {
-	steps, err := b.buildSteps(artifact, tag)
+	tags := []string{tag}
+	if artifact.WorkspaceHash != "" {
+		tags = append(tags, build.HashTag(artifact))
+	}
+
+	steps, err := b.buildSteps(artifact, tags)
 	if err != nil {
 		return nil, err
 	}
@@ -42,7 +48,7 @@ func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, objec
 			},
 		},
 		Steps:  steps,
-		Images: []string{tag},
+		Images: tags,
 		Options: &cloudbuild.BuildOptions{
 			DiskSizeGb:  b.DiskSizeGb,
 			MachineType: b.MachineType,
@@ -51,28 +57,29 @@ func (b *Builder) buildDescription(artifact *latest.Artifact, tag, bucket, objec
 	}, nil
 }
 
-func (b *Builder) buildSteps(artifact *latest.Artifact, tag string) ([]*cloudbuild.BuildStep, error) {
+func (b *Builder) buildSteps(artifact *latest.Artifact, tags []string) ([]*cloudbuild.BuildStep, error) {
 	switch {
 	case artifact.BuilderPlugin != nil:
-		return b.pluginBuildSteps(artifact, tag)
+		return b.pluginBuildSteps(artifact, tags)
 	case artifact.DockerArtifact != nil:
-		return b.dockerBuildSteps(artifact.DockerArtifact, tag), nil
+		return b.dockerBuildSteps(artifact.DockerArtifact, tags), nil
 
 	case artifact.BazelArtifact != nil:
 		return nil, errors.New("skaffold can't build a bazel artifact with Google Cloud Build")
 
+		// TODO: build multiple tagged images with jib in GCB (priyawadhwa@)
 	case artifact.JibMavenArtifact != nil:
-		return b.jibMavenBuildSteps(artifact.JibMavenArtifact, tag), nil
+		return b.jibMavenBuildSteps(artifact.JibMavenArtifact, tags[0]), nil
 
 	case artifact.JibGradleArtifact != nil:
-		return b.jibGradleBuildSteps(artifact.JibGradleArtifact, tag), nil
+		return b.jibGradleBuildSteps(artifact.JibGradleArtifact, tags[0]), nil
 
 	default:
 		return nil, fmt.Errorf("undefined artifact type: %+v", artifact.ArtifactType)
 	}
 }
 
-func (b *Builder) pluginBuildSteps(artifact *latest.Artifact, tag string) ([]*cloudbuild.BuildStep, error) {
+func (b *Builder) pluginBuildSteps(artifact *latest.Artifact, tags []string) ([]*cloudbuild.BuildStep, error) {
 	switch artifact.BuilderPlugin.Name {
 	case constants.DockerBuilderPluginName:
 		var da *latest.DockerArtifact
@@ -83,7 +90,7 @@ func (b *Builder) pluginBuildSteps(artifact *latest.Artifact, tag string) ([]*cl
 			da = &latest.DockerArtifact{}
 		}
 		defaults.SetDefaultDockerArtifact(da)
-		return b.dockerBuildSteps(da, tag), nil
+		return b.dockerBuildSteps(da, tags), nil
 	default:
 		return nil, errors.Errorf("the '%s' builder is not supported", artifact.BuilderPlugin.Name)
 	}

pkg/skaffold/plugin/environments/gcb/docker.go

@@ -24,7 +24,7 @@ import (
 	cloudbuild "google.golang.org/api/cloudbuild/v1"
 )
 
-func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string) []*cloudbuild.BuildStep {
+func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tags []string) []*cloudbuild.BuildStep {
 	var steps []*cloudbuild.BuildStep
 
 	for _, cacheFrom := range artifact.CacheFrom {
@@ -35,7 +35,11 @@ func (b *Builder) dockerBuildSteps(artifact *latest.DockerArtifact, tag string)
 		})
 	}
 
-	args := append([]string{"build", "--tag", tag, "-f", artifact.DockerfilePath})
+	args := []string{"build"}
+	for _, t := range tags {
+		args = append(args, []string{"--tag", t}...)
+	}
+	args = append(args, []string{"-f", artifact.DockerfilePath}...)
 	args = append(args, docker.GetBuildArgs(artifact)...)
 	args = append(args, ".")
 

pkg/skaffold/plugin/environments/gcb/docker_test.go

@@ -82,7 +82,7 @@ func TestPullCacheFrom(t *testing.T) {
 			DockerImage: "docker/docker",
 		},
 	}
-	steps := builder.dockerBuildSteps(artifact, "nginx2")
+	steps := builder.dockerBuildSteps(artifact, []string{"nginx2"})
 
 	expected := []*cloudbuild.BuildStep{{
 		Name:       "docker/docker",