Fix 5301: Build dependencies for sync inherited from required artifacts; cache build dependencies between devloops

pkg/skaffold/build/dependencies.go

@@ -30,8 +30,59 @@ import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
 
-// DependenciesForArtifact returns the dependencies for a given artifact.
-func DependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) {
+// for testing
+var getDependenciesFunc = sourceDependenciesForArtifact
+
+// TransitiveSourceDependenciesCache provides an interface to evaluate and cache the source dependencies for artifacts.
+// This additionally includes the source dependencies from all other artifacts that are in the transitive closure of its artifact dependencies.
+type TransitiveSourceDependenciesCache interface {
+	ResolveForArtifact(ctx context.Context, a *latest.Artifact) ([]string, error)
+	Reset()
+}
+
+func NewTransitiveSourceDependenciesCache(cfg docker.Config, r docker.ArtifactResolver, g ArtifactGraph) TransitiveSourceDependenciesCache {
+	return &dependencyResolverImpl{cfg: cfg, artifactResolver: r, artifactGraph: g, cache: util.NewSyncStore()}
+}
+
+type dependencyResolverImpl struct {
+	cfg              docker.Config
+	artifactResolver docker.ArtifactResolver
+	artifactGraph    ArtifactGraph
+	cache            *util.SyncStore
+}
+
+// ResolveForArtifact returns the source dependencies for the target artifact. It includes the source dependencies from all other artifacts that are in the transitive closure of its artifact dependencies.
+// The result (even if an error) is cached so that the function is evaluated only once for every artifact. The cache is reset before the start of the next devloop.
+func (r *dependencyResolverImpl) ResolveForArtifact(ctx context.Context, a *latest.Artifact) ([]string, error) {
+	res := r.cache.Exec(a.ImageName, func() interface{} {
+		d, e := getDependenciesFunc(ctx, a, r.cfg, r.artifactResolver)
+		if e != nil {
+			return e
+		}
+		return d
+	})
+	if err, ok := res.(error); ok {
+		return nil, err
+	}
+
+	deps := res.([]string)
+	for _, ad := range a.Dependencies {
+		d, err := r.ResolveForArtifact(ctx, r.artifactGraph[ad.ImageName])
+		if err != nil {
+			return nil, err
+		}
+		deps = append(deps, d...)
+	}
+	return deps, nil
+}
+
+// Reset removes the cached source dependencies for all artifacts
+func (r *dependencyResolverImpl) Reset() {
+	r.cache = util.NewSyncStore()
+}
+
+// sourceDependenciesForArtifact returns the build dependencies for the current artifact.
+func sourceDependenciesForArtifact(ctx context.Context, a *latest.Artifact, cfg docker.Config, r docker.ArtifactResolver) ([]string, error) {
 	var (
 		paths []string
 		err   error

pkg/skaffold/build/dependencies_test.go

@@ -0,0 +1,57 @@
+/*
+Copyright 2020 The Skaffold Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package build
+
+import (
+	"context"
+	"testing"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/testutil"
+)
+
+func TestTransitiveSourceDependenciesCache(t *testing.T) {
+	testutil.Run(t, "TestTransitiveSourceDependenciesCache", func(t *testutil.T) {
+		g := map[string]*latest.Artifact{
+			"img1": {ImageName: "img1", Dependencies: []*latest.ArtifactDependency{{ImageName: "img2"}}},
+			"img2": {ImageName: "img2", Dependencies: []*latest.ArtifactDependency{{ImageName: "img3"}, {ImageName: "img4"}}},
+			"img3": {ImageName: "img3", Dependencies: []*latest.ArtifactDependency{{ImageName: "img4"}}},
+			"img4": {ImageName: "img4"},
+		}
+		deps := map[string][]string{
+			"img1": {"file11", "file12"},
+			"img2": {"file21", "file22"},
+			"img3": {"file31", "file32"},
+			"img4": {"file41", "file42"},
+		}
+		counts := map[string]int{"img1": 0, "img2": 0, "img3": 0, "img4": 0}
+		t.Override(&getDependenciesFunc, func(_ context.Context, a *latest.Artifact, _ docker.Config, _ docker.ArtifactResolver) ([]string, error) {
+			counts[a.ImageName]++
+			return deps[a.ImageName], nil
+		})
+
+		r := NewTransitiveSourceDependenciesCache(nil, nil, g)
+		d, err := r.ResolveForArtifact(context.Background(), g["img1"])
+		t.CheckNoError(err)
+		expectedDeps := []string{"file11", "file12", "file21", "file22", "file31", "file32", "file41", "file42", "file41", "file42"}
+		t.CheckDeepEqual(expectedDeps, d)
+		for _, v := range counts {
+			t.CheckDeepEqual(v, 1)
+		}
+	})
+}

pkg/skaffold/build/docker/docker_test.go

@@ -106,7 +106,7 @@ func TestDockerCLIBuild(t *testing.T) {
 			}
 			t.Override(&util.OSEnviron, func() []string { return []string{"KEY=VALUE"} })
 
-			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, test.mode, nil, mockArtifactResolver{make(map[string]string)})
+			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv(test.extraEnv), test.localBuild.UseDockerCLI, test.localBuild.UseBuildkit, false, false, test.mode, nil, mockArtifactResolver{make(map[string]string)}, nil)
 
 			artifact := &latest.Artifact{
 				Workspace: ".",

pkg/skaffold/build/docker/errors_test.go

@@ -59,7 +59,7 @@ Refer https://skaffold.dev/docs/references/yaml/#build-artifacts-docker for deta
 				"docker build . --file "+dockerfilePath+" -t tag",
 			))
 			t.Override(&docker.DefaultAuthHelper, stubAuth{})
-			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), false, true, false, false, config.RunModes.Build, nil, mockArtifactResolver{make(map[string]string)})
+			builder := NewArtifactBuilder(fakeLocalDaemonWithExtraEnv([]string{}), false, true, false, false, config.RunModes.Build, nil, mockArtifactResolver{make(map[string]string)}, nil)
 
 			artifact := &latest.Artifact{
 				ImageName: "test-image",

pkg/skaffold/build/docker/types.go

@@ -17,8 +17,11 @@ limitations under the License.
 package docker
 
 import (
+	"context"
+
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
 )
 
 // Builder is an artifact builder that uses docker
@@ -31,15 +34,21 @@ type Builder struct {
 	mode               config.RunMode
 	insecureRegistries map[string]bool
 	artifacts          ArtifactResolver
+	sourceDependencies TransitiveSourceDependenciesResolver
 }
 
 // ArtifactResolver provides an interface to resolve built artifact tags by image name.
 type ArtifactResolver interface {
 	GetImageTag(imageName string) (string, bool)
 }
 
+// TransitiveSourceDependenciesResolver provides an interface to to evaluate the source dependencies for artifacts.
+type TransitiveSourceDependenciesResolver interface {
+	ResolveForArtifact(ctx context.Context, a *latest.Artifact) ([]string, error)
+}
+
 // NewBuilder returns an new instance of a docker builder
-func NewArtifactBuilder(localDocker docker.LocalDaemon, useCLI, useBuildKit, pushImages, prune bool, mode config.RunMode, insecureRegistries map[string]bool, r ArtifactResolver) *Builder {
+func NewArtifactBuilder(localDocker docker.LocalDaemon, useCLI, useBuildKit, pushImages, prune bool, mode config.RunMode, insecureRegistries map[string]bool, ar ArtifactResolver, dr TransitiveSourceDependenciesResolver) *Builder {
 	return &Builder{
 		localDocker:        localDocker,
 		pushImages:         pushImages,
@@ -48,6 +57,7 @@ func NewArtifactBuilder(localDocker docker.LocalDaemon, useCLI, useBuildKit, pus
 		useBuildKit:        useBuildKit,
 		mode:               mode,
 		insecureRegistries: insecureRegistries,
-		artifacts:          r,
+		artifacts:          ar,
+		sourceDependencies: dr,
 	}
 }

pkg/skaffold/build/gcb/cloud_build.go

@@ -94,7 +94,7 @@ func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer
 		return "", fmt.Errorf("checking bucket is in correct project: %w", err)
 	}
 
-	dependencies, err := build.DependenciesForArtifact(ctx, artifact, b.cfg, b.artifactStore)
+	dependencies, err := b.sourceDependencies.ResolveForArtifact(ctx, artifact)
 	if err != nil {
 		return "", fmt.Errorf("getting dependencies for %q: %w", artifact.ImageName, err)
 	}

pkg/skaffold/build/gcb/spec_test.go

@@ -57,6 +57,14 @@ func TestBuildSpecFail(t *testing.T) {
 type mockBuilderContext struct {
 	runcontext.RunContext // Embedded to provide the default values.
 	artifactStore         build.ArtifactStore
+	sourceDepsResolver    func() build.TransitiveSourceDependenciesCache
+}
+
+func (c *mockBuilderContext) SourceDependenciesResolver() build.TransitiveSourceDependenciesCache {
+	if c.sourceDepsResolver != nil {
+		return c.sourceDepsResolver()
+	}
+	return nil
 }
 
 func (c *mockBuilderContext) ArtifactStore() build.ArtifactStore { return c.artifactStore }

pkg/skaffold/build/gcb/types.go

@@ -80,10 +80,11 @@ func NewStatusBackoff() *wait.Backoff {
 type Builder struct {
 	*latest.GoogleCloudBuild
 
-	cfg           Config
-	skipTests     bool
-	muted         build.Muted
-	artifactStore build.ArtifactStore
+	cfg                Config
+	skipTests          bool
+	muted              build.Muted
+	artifactStore      build.ArtifactStore
+	sourceDependencies build.TransitiveSourceDependenciesCache
 }
 
 type Config interface {
@@ -96,16 +97,18 @@ type Config interface {
 type BuilderContext interface {
 	Config
 	ArtifactStore() build.ArtifactStore
+	SourceDependenciesResolver() build.TransitiveSourceDependenciesCache
 }
 
 // NewBuilder creates a new Builder that builds artifacts with Google Cloud Build.
 func NewBuilder(bCtx BuilderContext, buildCfg *latest.GoogleCloudBuild) *Builder {
 	return &Builder{
-		GoogleCloudBuild: buildCfg,
-		cfg:              bCtx,
-		skipTests:        bCtx.SkipTests(),
-		muted:            bCtx.Muted(),
-		artifactStore:    bCtx.ArtifactStore(),
+		GoogleCloudBuild:   buildCfg,
+		cfg:                bCtx,
+		skipTests:          bCtx.SkipTests(),
+		muted:              bCtx.Muted(),
+		artifactStore:      bCtx.ArtifactStore(),
+		sourceDependencies: bCtx.SourceDependenciesResolver(),
 	}
 }
 

pkg/skaffold/build/local/local_test.go

@@ -413,6 +413,7 @@ type mockBuilderContext struct {
 	mode                  config.RunMode
 	cluster               config.Cluster
 	artifactStore         build.ArtifactStore
+	sourceDepsResolver    func() build.TransitiveSourceDependenciesCache
 }
 
 func (c *mockBuilderContext) Mode() config.RunMode {
@@ -426,3 +427,10 @@ func (c *mockBuilderContext) GetCluster() config.Cluster {
 func (c *mockBuilderContext) ArtifactStore() build.ArtifactStore {
 	return c.artifactStore
 }
+
+func (c *mockBuilderContext) SourceDependenciesResolver() build.TransitiveSourceDependenciesCache {
+	if c.sourceDepsResolver != nil {
+		return c.sourceDepsResolver()
+	}
+	return nil
+}

pkg/skaffold/build/local/types.go

@@ -55,6 +55,7 @@ type Builder struct {
 	muted              build.Muted
 	localPruner        *pruner
 	artifactStore      build.ArtifactStore
+	sourceDependencies build.TransitiveSourceDependenciesCache
 }
 
 type Config interface {
@@ -72,6 +73,7 @@ type Config interface {
 type BuilderContext interface {
 	Config
 	ArtifactStore() build.ArtifactStore
+	SourceDependenciesResolver() build.TransitiveSourceDependenciesCache
 }
 
 // NewBuilder returns an new instance of a local Builder.
@@ -109,6 +111,7 @@ func NewBuilder(bCtx BuilderContext, buildCfg *latest.LocalBuild) (*Builder, err
 		insecureRegistries: bCtx.GetInsecureRegistries(),
 		muted:              bCtx.Muted(),
 		artifactStore:      bCtx.ArtifactStore(),
+		sourceDependencies: bCtx.SourceDependenciesResolver(),
 	}, nil
 }
 
@@ -136,7 +139,7 @@ type artifactBuilder interface {
 func newPerArtifactBuilder(b *Builder, a *latest.Artifact) (artifactBuilder, error) {
 	switch {
 	case a.DockerArtifact != nil:
-		return dockerbuilder.NewArtifactBuilder(b.localDocker, b.local.UseDockerCLI, b.local.UseBuildkit, b.pushImages, b.prune, b.cfg.Mode(), b.cfg.GetInsecureRegistries(), b.artifactStore), nil
+		return dockerbuilder.NewArtifactBuilder(b.localDocker, b.local.UseDockerCLI, b.local.UseBuildkit, b.pushImages, b.prune, b.cfg.Mode(), b.cfg.GetInsecureRegistries(), b.artifactStore, b.sourceDependencies), nil
 
 	case a.BazelArtifact != nil:
 		return bazel.NewArtifactBuilder(b.localDocker, b.cfg, b.pushImages), nil

pkg/skaffold/diagnose/diagnose.go

@@ -36,6 +36,7 @@ type Config interface {
 	docker.Config
 
 	GetPipelines() []latest.Pipeline
+	Artifacts() []*latest.Artifact
 }
 
 func CheckArtifacts(ctx context.Context, cfg Config, out io.Writer) error {
@@ -113,9 +114,11 @@ func typeOfArtifact(a *latest.Artifact) string {
 	}
 }
 
-func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg docker.Config) (string, []string, error) {
+func timeToListDependencies(ctx context.Context, a *latest.Artifact, cfg Config) (string, []string, error) {
 	start := time.Now()
-	paths, err := build.DependenciesForArtifact(ctx, a, cfg, nil)
+	graph := build.ToArtifactGraph(cfg.Artifacts())
+	sourceDependencies := build.NewTransitiveSourceDependenciesCache(cfg, nil, graph)
+	paths, err := sourceDependencies.ResolveForArtifact(ctx, a)
 	return util.ShowHumanizeTime(time.Since(start)), paths, err
 }
 

pkg/skaffold/diagnose/diagnose_test.go

@@ -106,3 +106,7 @@ func (c *mockConfig) PipelineForImage() latest.Pipeline {
 	pipeline.Build.Artifacts = c.artifacts
 	return pipeline
 }
+
+func (c *mockConfig) Artifacts() []*latest.Artifact {
+	return c.artifacts
+}

pkg/skaffold/runner/builder.go

@@ -32,16 +32,21 @@ import (
 // builderCtx encapsulates a given skaffold run context along with additional builder constructs.
 type builderCtx struct {
 	*runcontext.RunContext
-	artifactStore build.ArtifactStore
+	artifactStore           build.ArtifactStore
+	sourceDependenciesCache build.TransitiveSourceDependenciesCache
 }
 
 func (b *builderCtx) ArtifactStore() build.ArtifactStore {
 	return b.artifactStore
 }
 
+func (b *builderCtx) SourceDependenciesResolver() build.TransitiveSourceDependenciesCache {
+	return b.sourceDependenciesCache
+}
+
 // getBuilder creates a builder from a given RunContext and build pipeline type.
-func getBuilder(runCtx *runcontext.RunContext, store build.ArtifactStore, p latest.Pipeline) (build.PipelineBuilder, error) {
-	bCtx := &builderCtx{artifactStore: store, RunContext: runCtx}
+func getBuilder(r *runcontext.RunContext, s build.ArtifactStore, d build.TransitiveSourceDependenciesCache, p latest.Pipeline) (build.PipelineBuilder, error) {
+	bCtx := &builderCtx{artifactStore: s, sourceDependenciesCache: d, RunContext: r}
 	switch {
 	case p.Build.LocalBuild != nil:
 		logrus.Debugln("Using builder: local")

pkg/skaffold/runner/dev.go

@@ -185,7 +185,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 		default:
 			if err := r.monitor.Register(
 				func() ([]string, error) {
-					return build.DependenciesForArtifact(ctx, artifact, r.runCtx, r.artifactStore)
+					return r.sourceDependencies.ResolveForArtifact(ctx, artifact)
 				},
 				func(e filemon.Events) {
 					s, err := sync.NewItem(ctx, artifact, e, r.builds, r.runCtx, len(g[artifact.ImageName]))
@@ -195,7 +195,7 @@ func (r *SkaffoldRunner) Dev(ctx context.Context, out io.Writer, artifacts []*la
 					case s != nil:
 						r.changeSet.AddResync(s)
 					default:
-						addRebuild(g, artifact, r.changeSet.AddRebuild, r.runCtx.Opts.IsTargetImage)
+						r.changeSet.AddRebuild(artifact)
 					}
 				},
 			); err != nil {
@@ -304,13 +304,3 @@ func getTransposeGraph(artifacts []*latest.Artifact) graph {
 	}
 	return g
 }
-
-// addRebuild runs the `rebuild` function for all target artifacts in the transitive closure on the source `artifact` in graph `g`.
-func addRebuild(g graph, artifact *latest.Artifact, rebuild func(*latest.Artifact), isTarget func(*latest.Artifact) bool) {
-	if isTarget(artifact) {
-		rebuild(artifact)
-	}
-	for _, a := range g[artifact.ImageName] {
-		addRebuild(g, a, rebuild, isTarget)
-	}
-}