diff --git a/docs/content/en/schemas/v2beta6.json b/docs/content/en/schemas/v2beta6.json index bc8533b2100..32438e34588 100755 --- a/docs/content/en/schemas/v2beta6.json +++ b/docs/content/en/schemas/v2beta6.json @@ -1279,8 +1279,8 @@ ], "properties": { "artifactOverrides": { - "description": "key value pairs where the key represents the parameter used in the `--set-string` Helm CLI flag to define a container image and the value corresponds to artifact i.e. `ImageName` defined in `Build.Artifacts` section. The resulting command-line is controlled by `ImageStrategy`.", - "x-intellij-html-description": "key value pairs where the key represents the parameter used in the --set-string Helm CLI flag to define a container image and the value corresponds to artifact i.e. ImageName defined in Build.Artifacts section. The resulting command-line is controlled by ImageStrategy." + "description": "key value pairs. If present, Skaffold will send `--set-string` flag to Helm CLI and append all pairs after the flag.", + "x-intellij-html-description": "key value pairs. If present, Skaffold will send --set-string flag to Helm CLI and append all pairs after the flag." }, "chartPath": { "type": "string", @@ -1289,8 +1289,8 @@ }, "imageStrategy": { "$ref": "#/definitions/HelmImageStrategy", - "description": "controls how an `ArtifactOverrides` entry is turned into `--set-string` Helm CLI flag or flags.", - "x-intellij-html-description": "controls how an ArtifactOverrides entry is turned into --set-string Helm CLI flag or flags." + "description": "adds image configurations to the Helm `values` file.", + "x-intellij-html-description": "adds image configurations to the Helm values file." }, "name": { "type": "string", diff --git a/docs/content/en/schemas/v2beta8.json b/docs/content/en/schemas/v2beta8.json index 30c4d0bc965..8fc5234f1dc 100755 --- a/docs/content/en/schemas/v2beta8.json +++ b/docs/content/en/schemas/v2beta8.json @@ -64,6 +64,14 @@ "gcr.io/k8s-skaffold/example" ] }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -74,7 +82,8 @@ "preferredOrder": [ "image", "context", - "sync" + "sync", + "requires" ], "additionalProperties": false }, @@ -99,6 +108,14 @@ "gcr.io/k8s-skaffold/example" ] }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -110,6 +127,7 @@ "image", "context", "sync", + "requires", "docker" ], "additionalProperties": false @@ -135,6 +153,14 @@ "gcr.io/k8s-skaffold/example" ] }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -146,6 +172,7 @@ "image", "context", "sync", + "requires", "bazel" ], "additionalProperties": false @@ -171,6 +198,14 @@ "description": "builds images using the [Jib plugins for Maven or Gradle](https://github.com/GoogleContainerTools/jib/).", "x-intellij-html-description": "builds images using the Jib plugins for Maven or Gradle." }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -182,6 +217,7 @@ "image", "context", "sync", + "requires", "jib" ], "additionalProperties": false @@ -207,6 +243,14 @@ "description": "builds images using [kaniko](https://github.com/GoogleContainerTools/kaniko).", "x-intellij-html-description": "builds images using kaniko." }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -218,6 +262,7 @@ "image", "context", "sync", + "requires", "kaniko" ], "additionalProperties": false @@ -243,6 +288,14 @@ "gcr.io/k8s-skaffold/example" ] }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -254,6 +307,7 @@ "image", "context", "sync", + "requires", "buildpacks" ], "additionalProperties": false @@ -279,6 +333,14 @@ "gcr.io/k8s-skaffold/example" ] }, + "requires": { + "items": { + "$ref": "#/definitions/ArtifactDependency" + }, + "type": "array", + "description": "describes build artifacts that this artifact depends on.", + "x-intellij-html-description": "describes build artifacts that this artifact depends on." + }, "sync": { "$ref": "#/definitions/Sync", "description": "*beta* local files synced to pods instead of triggering an image build when modified. If no files are listed, sync all the files and infer the destination.", @@ -290,6 +352,7 @@ "image", "context", "sync", + "requires", "custom" ], "additionalProperties": false @@ -298,6 +361,30 @@ "description": "items that need to be built, along with the context in which they should be built.", "x-intellij-html-description": "items that need to be built, along with the context in which they should be built." }, + "ArtifactDependency": { + "required": [ + "image" + ], + "properties": { + "alias": { + "type": "string", + "description": "a token that will be replaced with the image reference in the builder definition files. For example, the `docker` builder will use the alias as a build-time variable. Defaults to the value of `image`.", + "x-intellij-html-description": "a token that will be replaced with the image reference in the builder definition files. For example, the docker builder will use the alias as a build-time variable. Defaults to the value of image." + }, + "image": { + "type": "string", + "description": "artifact image name.", + "x-intellij-html-description": "artifact image name." + } + }, + "preferredOrder": [ + "image", + "alias" + ], + "additionalProperties": false, + "description": "describes a specific build dependency for an artifact.", + "x-intellij-html-description": "describes a specific build dependency for an artifact." + }, "Auto": { "description": "cannot be customized.", "x-intellij-html-description": "cannot be customized." @@ -1285,8 +1372,8 @@ ], "properties": { "artifactOverrides": { - "description": "key value pairs. If present, Skaffold will send `--set-string` flag to Helm CLI and append all pairs after the flag.", - "x-intellij-html-description": "key value pairs. If present, Skaffold will send --set-string flag to Helm CLI and append all pairs after the flag." + "description": "key value pairs where the key represents the parameter used in the `--set-string` Helm CLI flag to define a container image and the value corresponds to artifact i.e. `ImageName` defined in `Build.Artifacts` section. The resulting command-line is controlled by `ImageStrategy`.", + "x-intellij-html-description": "key value pairs where the key represents the parameter used in the --set-string Helm CLI flag to define a container image and the value corresponds to artifact i.e. ImageName defined in Build.Artifacts section. The resulting command-line is controlled by ImageStrategy." }, "chartPath": { "type": "string", @@ -1295,8 +1382,8 @@ }, "imageStrategy": { "$ref": "#/definitions/HelmImageStrategy", - "description": "adds image configurations to the Helm `values` file.", - "x-intellij-html-description": "adds image configurations to the Helm values file." + "description": "controls how an `ArtifactOverrides` entry is turned into `--set-string` Helm CLI flag or flags.", + "x-intellij-html-description": "controls how an ArtifactOverrides entry is turned into --set-string Helm CLI flag or flags." }, "name": { "type": "string", diff --git a/pkg/skaffold/build/cluster/cluster.go b/pkg/skaffold/build/cluster/cluster.go index 7e47da0c207..dd8d269deb8 100644 --- a/pkg/skaffold/build/cluster/cluster.go +++ b/pkg/skaffold/build/cluster/cluster.go @@ -46,7 +46,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, } builder := build.WithLogFile(b.buildArtifact, b.muted) - return build.InParallel(ctx, out, tags, artifacts, builder, b.ClusterDetails.Concurrency) + return build.InOrder(ctx, out, tags, artifacts, builder, b.ClusterDetails.Concurrency) } func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { diff --git a/pkg/skaffold/build/gcb/cloud_build.go b/pkg/skaffold/build/gcb/cloud_build.go index 84f51c084e1..71d4da1397b 100644 --- a/pkg/skaffold/build/gcb/cloud_build.go +++ b/pkg/skaffold/build/gcb/cloud_build.go @@ -47,7 +47,7 @@ import ( // Build builds a list of artifacts with Google Cloud Build. func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact) ([]build.Artifact, error) { builder := build.WithLogFile(b.buildArtifactWithCloudBuild, b.muted) - return build.InParallel(ctx, out, tags, artifacts, builder, b.GoogleCloudBuild.Concurrency) + return build.InOrder(ctx, out, tags, artifacts, builder, b.GoogleCloudBuild.Concurrency) } func (b *Builder) buildArtifactWithCloudBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { diff --git a/pkg/skaffold/build/local/local.go b/pkg/skaffold/build/local/local.go index 0327a2cae15..9a2deac062e 100644 --- a/pkg/skaffold/build/local/local.go +++ b/pkg/skaffold/build/local/local.go @@ -43,7 +43,7 @@ func (b *Builder) Build(ctx context.Context, out io.Writer, tags tag.ImageTags, defer b.localDocker.Close() builder := build.WithLogFile(b.buildArtifact, b.muted) - return build.InParallel(ctx, out, tags, artifacts, builder, *b.cfg.Concurrency) + return build.InOrder(ctx, out, tags, artifacts, builder, *b.cfg.Concurrency) } func (b *Builder) buildArtifact(ctx context.Context, out io.Writer, a *latest.Artifact, tag string) (string, error) { diff --git a/pkg/skaffold/build/parallel.go b/pkg/skaffold/build/scheduler.go similarity index 54% rename from pkg/skaffold/build/parallel.go rename to pkg/skaffold/build/scheduler.go index 2d04b4c2533..a708312727b 100644 --- a/pkg/skaffold/build/parallel.go +++ b/pkg/skaffold/build/scheduler.go @@ -35,20 +35,15 @@ type ArtifactBuilder func(ctx context.Context, out io.Writer, artifact *latest.A // For testing var ( - buffSize = bufferedLinesPerArtifact - runInSequence = InSequence + buffSize = bufferedLinesPerArtifact ) -// InParallel builds a list of artifacts in parallel but prints the logs in sequential order. -func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact ArtifactBuilder, concurrency int) ([]Artifact, error) { - if len(artifacts) == 0 { - return nil, nil - } - - if len(artifacts) == 1 || concurrency == 1 { - return runInSequence(ctx, out, tags, artifacts, buildArtifact) - } - +// InOrder builds a list of artifacts in dependency order. +// `concurrency` specifies the max number of builds that can run at any one time. If concurrency is 0, then it's set to the length of the `artifacts` slice. +// Each artifact build runs in its own goroutine. It limits the max concurrency using a buffered channel like a semaphore. +// At the same time, it uses the `artifactChanModel` to model the artifacts dependency graph and to make each artifact build wait for its required artifacts' builds. +func InOrder(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact ArtifactBuilder, concurrency int) ([]Artifact, error) { + acmSlice := makeArtifactChanModel(artifacts) var wg sync.WaitGroup defer wg.Wait() @@ -56,28 +51,30 @@ func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifact defer cancel() results := new(sync.Map) - outputs := make([]chan string, len(artifacts)) + outputs := make([]chan string, len(acmSlice)) if concurrency == 0 { - concurrency = len(artifacts) + concurrency = len(acmSlice) } sem := make(chan bool, concurrency) - // Run builds in // wg.Add(len(artifacts)) - for i := range artifacts { + for i := range acmSlice { outputs[i] = make(chan string, buffSize) r, w := io.Pipe() - // Run build and write output/logs to piped writer and store build result in - // sync.Map - go func(i int) { + // Create a goroutine for each element in acmSlice. Each goroutine waits on its dependencies to finish building. + // Because our artifacts form a DAG, at least one of the goroutines should be able to start building. + go func(a *artifactChanModel) { + a.waitForDependencies(ctx) sem <- true - runBuild(ctx, w, tags, artifacts[i], results, buildArtifact) + // Run build and write output/logs to piped writer and store build result in sync.Map + runBuild(ctx, w, tags, a.artifact, results, buildArtifact) + a.markComplete() <-sem wg.Done() - }(i) + }(acmSlice[i]) // Read build output/logs and write to buffered channel go readOutputAndWriteToChannel(r, outputs[i]) @@ -87,6 +84,48 @@ func InParallel(ctx context.Context, out io.Writer, tags tag.ImageTags, artifact return collectResults(out, artifacts, results, outputs) } +// artifactChanModel models the artifact dependency graph using a set of channels. +// Each artifact has a channel that it closes once it completes building (either success or failure) by calling `markComplete`. This notifies *all* listeners waiting for this artifact. +// Additionally it has a list of channels for each of its dependencies. +// Calling `waitForDependencies` ensures that all required artifacts' channels have already been closed and as such have finished building. +// This model allows for composing any arbitrary function with dependency ordering. +type artifactChanModel struct { + artifact *latest.Artifact + artifactChan chan interface{} + requiredArtifactChans []chan interface{} +} + +func (a *artifactChanModel) markComplete() { + // closing channel notifies all listeners waiting for this build to complete + close(a.artifactChan) +} +func (a *artifactChanModel) waitForDependencies(ctx context.Context) { + for _, dep := range a.requiredArtifactChans { + // wait for dependency to complete build + select { + case <-ctx.Done(): + case <-dep: + } + } +} + +func makeArtifactChanModel(artifacts []*latest.Artifact) []*artifactChanModel { + chanMap := make(map[string]chan interface{}) + for _, a := range artifacts { + chanMap[a.ImageName] = make(chan interface{}) + } + + var acmSlice []*artifactChanModel + for _, a := range artifacts { + acm := &artifactChanModel{artifact: a, artifactChan: chanMap[a.ImageName]} + for _, d := range a.Dependencies { + acm.requiredArtifactChans = append(acm.requiredArtifactChans, chanMap[d.ImageName]) + } + acmSlice = append(acmSlice, acm) + } + return acmSlice +} + func runBuild(ctx context.Context, cw io.WriteCloser, tags tag.ImageTags, artifact *latest.Artifact, results *sync.Map, build ArtifactBuilder) { event.BuildInProgress(artifact.ImageName) diff --git a/pkg/skaffold/build/parallel_test.go b/pkg/skaffold/build/scheduler_test.go similarity index 72% rename from pkg/skaffold/build/parallel_test.go rename to pkg/skaffold/build/scheduler_test.go index d73d8067e5e..085370e76b1 100644 --- a/pkg/skaffold/build/parallel_test.go +++ b/pkg/skaffold/build/scheduler_test.go @@ -27,7 +27,10 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -194,7 +197,7 @@ func TestCollectResults(t *testing.T) { } } -func TestInParallel(t *testing.T) { +func TestInOrder(t *testing.T) { tests := []struct { description string buildFunc ArtifactBuilder @@ -236,14 +239,14 @@ And new lines } initializeEvents() - InParallel(context.Background(), out, tags, artifacts, test.buildFunc, 0) + InOrder(context.Background(), out, tags, artifacts, test.buildFunc, 0) t.CheckDeepEqual(test.expected, out.String()) }) } } -func TestInParallelConcurrency(t *testing.T) { +func TestInOrderConcurrency(t *testing.T) { tests := []struct { artifacts int limit int @@ -291,7 +294,7 @@ func TestInParallelConcurrency(t *testing.T) { } initializeEvents() - results, err := InParallel(context.Background(), ioutil.Discard, tags, artifacts, builder, test.limit) + results, err := InOrder(context.Background(), ioutil.Discard, tags, artifacts, builder, test.limit) t.CheckNoError(err) t.CheckDeepEqual(test.artifacts, len(results)) @@ -299,31 +302,66 @@ func TestInParallelConcurrency(t *testing.T) { } } -func TestInParallelForArgs(t *testing.T) { +func TestInOrderForArgs(t *testing.T) { tests := []struct { description string - inSeqFunc func(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact, ArtifactBuilder) ([]Artifact, error) buildArtifact ArtifactBuilder artifactLen int + concurrency int + dependency map[int][]int expected []Artifact + err error }{ { - description: "runs in sequence for 1 artifact", - inSeqFunc: func(context.Context, io.Writer, tag.ImageTags, []*latest.Artifact, ArtifactBuilder) ([]Artifact, error) { - return []Artifact{{ImageName: "singleArtifact", Tag: "one"}}, nil + description: "runs in parallel for 2 artifacts with no dependency", + buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, tag string) (string, error) { + return tag, nil + }, + artifactLen: 2, + expected: []Artifact{ + {ImageName: "artifact1", Tag: "artifact1@tag1"}, + {ImageName: "artifact2", Tag: "artifact2@tag2"}, }, - artifactLen: 1, - expected: []Artifact{{ImageName: "singleArtifact", Tag: "one"}}, }, { - description: "runs in parallel for 2 artifacts", + description: "runs in parallel for 5 artifacts with dependencies", buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, tag string) (string, error) { return tag, nil }, - artifactLen: 2, + dependency: map[int][]int{ + 0: {2, 3}, + 1: {3}, + 2: {1}, + 3: {4}, + }, + artifactLen: 5, expected: []Artifact{ {ImageName: "artifact1", Tag: "artifact1@tag1"}, {ImageName: "artifact2", Tag: "artifact2@tag2"}, + {ImageName: "artifact3", Tag: "artifact3@tag3"}, + {ImageName: "artifact4", Tag: "artifact4@tag4"}, + {ImageName: "artifact5", Tag: "artifact5@tag5"}, + }, + }, + { + description: "runs with max concurrency of 2 for 5 artifacts with dependencies", + buildArtifact: func(_ context.Context, _ io.Writer, _ *latest.Artifact, tag string) (string, error) { + return tag, nil + }, + dependency: map[int][]int{ + 0: {2, 3}, + 1: {3}, + 2: {1}, + 3: {4}, + }, + artifactLen: 5, + concurrency: 2, + expected: []Artifact{ + {ImageName: "artifact1", Tag: "artifact1@tag1"}, + {ImageName: "artifact2", Tag: "artifact2@tag2"}, + {ImageName: "artifact3", Tag: "artifact3@tag3"}, + {ImageName: "artifact4", Tag: "artifact4@tag4"}, + {ImageName: "artifact5", Tag: "artifact5@tag5"}, }, }, { @@ -331,6 +369,24 @@ func TestInParallelForArgs(t *testing.T) { artifactLen: 0, expected: nil, }, + { + description: "build fails for artifacts with dependencies", + buildArtifact: func(_ context.Context, _ io.Writer, a *latest.Artifact, tag string) (string, error) { + if a.ImageName == "artifact2" { + return "", fmt.Errorf(`some error occurred while building "artifact2"`) + } + return tag, nil + }, + dependency: map[int][]int{ + 0: {1}, + 1: {2}, + 2: {3}, + 3: {4}, + }, + artifactLen: 5, + expected: nil, + err: fmt.Errorf("couldn't build %q: %w", "artifact2", fmt.Errorf("some error occurred while building %q", "artifact2")), + }, } for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { @@ -341,17 +397,34 @@ func TestInParallelForArgs(t *testing.T) { artifacts[i] = &latest.Artifact{ImageName: a} tags[a] = fmt.Sprintf("%s@tag%d", a, i+1) } - if test.inSeqFunc != nil { - t.Override(&runInSequence, test.inSeqFunc) - } + + setDependencies(artifacts, test.dependency) initializeEvents() - actual, _ := InParallel(context.Background(), ioutil.Discard, tags, artifacts, test.buildArtifact, 0) + actual, err := InOrder(context.Background(), ioutil.Discard, tags, artifacts, test.buildArtifact, test.concurrency) t.CheckDeepEqual(test.expected, actual) + t.CheckDeepEqual(test.err, err, cmp.Comparer(errorsComparer)) }) } } +// setDependencies constructs a graph of artifact dependencies using the map as an adjacency list representation of indices in the artifacts array. +// For example: +// m = { +// 0 : {1, 2}, +// 2 : {3}, +//} +// implies that a[0] artifact depends on a[1] and a[2]; and a[2] depends on a[3]. +func setDependencies(a []*latest.Artifact, d map[int][]int) { + for k, dep := range d { + for i := range dep { + a[k].Dependencies = append(a[k].Dependencies, &latest.ArtifactDependency{ + ImageName: a[dep[i]].ImageName, + }) + } + } +} + func setUpChannels(n int) []chan string { outputs := make([]chan string, n) for i := 0; i < n; i++ { @@ -360,3 +433,25 @@ func setUpChannels(n int) []chan string { } return outputs } + +func initializeEvents() { + pipe := latest.Pipeline{ + Deploy: latest.DeployConfig{}, + Build: latest.BuildConfig{ + BuildType: latest.BuildType{ + LocalBuild: &latest.LocalBuild{}, + }, + }, + } + event.InitializeState(pipe, "temp", true, true, true) +} + +func errorsComparer(a, b error) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return a.Error() == b.Error() +} diff --git a/pkg/skaffold/build/sequence.go b/pkg/skaffold/build/sequence.go deleted file mode 100644 index e63ed7b70af..00000000000 --- a/pkg/skaffold/build/sequence.go +++ /dev/null @@ -1,59 +0,0 @@ -/* -Copyright 2019 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" - "fmt" - "io" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/color" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" -) - -// InSequence builds a list of artifacts in sequence. -func InSequence(ctx context.Context, out io.Writer, tags tag.ImageTags, artifacts []*latest.Artifact, buildArtifact ArtifactBuilder) ([]Artifact, error) { - var builds []Artifact - - for _, artifact := range artifacts { - color.Default.Fprintf(out, "Building [%s]...\n", artifact.ImageName) - - event.BuildInProgress(artifact.ImageName) - - tag, present := tags[artifact.ImageName] - if !present { - return nil, fmt.Errorf("unable to find tag for image %s", artifact.ImageName) - } - - finalTag, err := buildArtifact(ctx, out, artifact, tag) - if err != nil { - event.BuildFailed(artifact.ImageName, err) - return nil, fmt.Errorf("couldn't build %q: %w", artifact.ImageName, err) - } - - event.BuildComplete(artifact.ImageName) - - builds = append(builds, Artifact{ - ImageName: artifact.ImageName, - Tag: finalTag, - }) - } - - return builds, nil -} diff --git a/pkg/skaffold/build/sequence_test.go b/pkg/skaffold/build/sequence_test.go deleted file mode 100644 index d9dc3d19479..00000000000 --- a/pkg/skaffold/build/sequence_test.go +++ /dev/null @@ -1,156 +0,0 @@ -/* -Copyright 2019 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 ( - "bytes" - "context" - "fmt" - "io" - "io/ioutil" - "testing" - - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/tag" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/event" - "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" - "github.com/GoogleContainerTools/skaffold/testutil" -) - -func TestInSequence(t *testing.T) { - tests := []struct { - description string - buildArtifact ArtifactBuilder - tags tag.ImageTags - expectedArtifacts []Artifact - expectedOut string - shouldErr bool - }{ - { - description: "build succeeds", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - return fmt.Sprintf("%s@sha256:abac", tag), nil - }, - tags: tag.ImageTags{ - "skaffold/image1": "skaffold/image1:v0.0.1", - "skaffold/image2": "skaffold/image2:v0.0.2", - }, - expectedArtifacts: []Artifact{ - {ImageName: "skaffold/image1", Tag: "skaffold/image1:v0.0.1@sha256:abac"}, - {ImageName: "skaffold/image2", Tag: "skaffold/image2:v0.0.2@sha256:abac"}, - }, - expectedOut: "Building [skaffold/image1]...\nBuilding [skaffold/image2]...\n", - }, - { - description: "build fails", - buildArtifact: func(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - return "", fmt.Errorf("build fails") - }, - tags: tag.ImageTags{ - "skaffold/image1": "", - }, - expectedOut: "Building [skaffold/image1]...\n", - shouldErr: true, - }, - { - description: "tag not found", - tags: tag.ImageTags{}, - expectedOut: "Building [skaffold/image1]...\n", - shouldErr: true, - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - out := new(bytes.Buffer) - artifacts := []*latest.Artifact{ - {ImageName: "skaffold/image1"}, - {ImageName: "skaffold/image2"}, - } - - got, err := InSequence(context.Background(), out, test.tags, artifacts, test.buildArtifact) - - t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expectedArtifacts, got) - t.CheckDeepEqual(test.expectedOut, out.String()) - }) - } -} - -func TestInSequenceResultsOrder(t *testing.T) { - tests := []struct { - description string - images []string - expected []Artifact - shouldErr bool - }{ - { - description: "shd concatenate the tag", - images: []string{"a", "b", "c", "d"}, - expected: []Artifact{ - {ImageName: "a", Tag: "a:a"}, - {ImageName: "b", Tag: "b:ab"}, - {ImageName: "c", Tag: "c:abc"}, - {ImageName: "d", Tag: "d:abcd"}, - }, - }, - } - for _, test := range tests { - testutil.Run(t, test.description, func(t *testutil.T) { - out := ioutil.Discard - initializeEvents() - artifacts := make([]*latest.Artifact, len(test.images)) - tags := tag.ImageTags{} - for i, image := range test.images { - artifacts[i] = &latest.Artifact{ - ImageName: image, - } - tags[image] = image - } - builder := concatTagger{} - - got, err := InSequence(context.Background(), out, tags, artifacts, builder.doBuild) - - t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, got) - }) - } -} - -// concatTagger builder sums all the numbers -type concatTagger struct { - tag string -} - -// doBuild calculate the tag based by concatinating the tag values for artifact -// builds seen so far. It mimics artifact dependency where the next build result -// depends on the previous build result. -func (t *concatTagger) doBuild(ctx context.Context, out io.Writer, artifact *latest.Artifact, tag string) (string, error) { - t.tag += tag - return fmt.Sprintf("%s:%s", artifact.ImageName, t.tag), nil -} - -func initializeEvents() { - event.InitializeState(latest.Pipeline{ - Deploy: latest.DeployConfig{}, - Build: latest.BuildConfig{ - BuildType: latest.BuildType{ - LocalBuild: &latest.LocalBuild{}, - }, - }, - }, - "temp", - true, - true, - true) -} diff --git a/pkg/skaffold/schema/defaults/defaults.go b/pkg/skaffold/schema/defaults/defaults.go index 2ed376e0e7e..da0abf6cba1 100644 --- a/pkg/skaffold/schema/defaults/defaults.go +++ b/pkg/skaffold/schema/defaults/defaults.go @@ -68,6 +68,10 @@ func Set(c *latest.SkaffoldConfig) error { case a.BuildpackArtifact != nil: setBuildpackArtifactDefaults(a.BuildpackArtifact) } + + for _, d := range a.Dependencies { + setDefaultArtifactDependencyAlias(d) + } } withLocalBuild(c, @@ -371,3 +375,9 @@ func setDefaultAddress(pf *latest.PortForwardResource) { pf.Address = constants.DefaultPortForwardAddress } } + +func setDefaultArtifactDependencyAlias(d *latest.ArtifactDependency) { + if d.Alias == "" { + d.Alias = d.ImageName + } +} diff --git a/pkg/skaffold/schema/defaults/defaults_test.go b/pkg/skaffold/schema/defaults/defaults_test.go index e341f4332df..453d32b6484 100644 --- a/pkg/skaffold/schema/defaults/defaults_test.go +++ b/pkg/skaffold/schema/defaults/defaults_test.go @@ -35,6 +35,10 @@ func TestSetDefaults(t *testing.T) { Artifacts: []*latest.Artifact{ { ImageName: "first", + Dependencies: []*latest.ArtifactDependency{ + {ImageName: "second", Alias: "secondAlias"}, + {ImageName: "third"}, + }, }, { ImageName: "second", @@ -83,6 +87,8 @@ func TestSetDefaults(t *testing.T) { testutil.CheckDeepEqual(t, "first", cfg.Build.Artifacts[0].ImageName) testutil.CheckDeepEqual(t, ".", cfg.Build.Artifacts[0].Workspace) testutil.CheckDeepEqual(t, "Dockerfile", cfg.Build.Artifacts[0].DockerArtifact.DockerfilePath) + testutil.CheckDeepEqual(t, "secondAlias", cfg.Build.Artifacts[0].Dependencies[0].Alias) + testutil.CheckDeepEqual(t, "third", cfg.Build.Artifacts[0].Dependencies[1].Alias) testutil.CheckDeepEqual(t, "second", cfg.Build.Artifacts[1].ImageName) testutil.CheckDeepEqual(t, "folder", cfg.Build.Artifacts[1].Workspace) diff --git a/pkg/skaffold/schema/latest/config.go b/pkg/skaffold/schema/latest/config.go index 979fcfe0954..a70431009c5 100644 --- a/pkg/skaffold/schema/latest/config.go +++ b/pkg/skaffold/schema/latest/config.go @@ -732,6 +732,9 @@ type Artifact struct { // ArtifactType describes how to build an artifact. ArtifactType `yaml:",inline"` + + // Dependencies describes build artifacts that this artifact depends on. + Dependencies []*ArtifactDependency `yaml:"requires,omitempty"` } // Sync *beta* specifies what files to sync into the container. @@ -854,6 +857,16 @@ type ArtifactType struct { CustomArtifact *CustomArtifact `yaml:"custom,omitempty" yamltags:"oneOf=artifact"` } +// ArtifactDependency describes a specific build dependency for an artifact. +type ArtifactDependency struct { + // ImageName is the artifact image name. + ImageName string `yaml:"image" yamltags:"required"` + // Alias is a token that will be replaced with the image reference in the builder definition files. + // For example, the `docker` builder will use the alias as a build-time variable. + // Defaults to the value of `image`. + Alias string `yaml:"alias,omitempty"` +} + // BuildpackArtifact *alpha* describes an artifact built using [Cloud Native Buildpacks](https://buildpacks.io/). // It can be used to build images out of project's sources without any additional configuration. type BuildpackArtifact struct { diff --git a/pkg/skaffold/schema/validation/validation.go b/pkg/skaffold/schema/validation/validation.go index 6c3c1a6448c..343bc2b7957 100644 --- a/pkg/skaffold/schema/validation/validation.go +++ b/pkg/skaffold/schema/validation/validation.go @@ -37,6 +37,7 @@ var ( func Process(config *latest.SkaffoldConfig) error { errs := visitStructs(config, validateYamltags) errs = append(errs, validateImageNames(config.Build.Artifacts)...) + errs = append(errs, validateArtifactDependencies(config.Build.Artifacts)...) errs = append(errs, validateDockerNetworkMode(config.Build.Artifacts)...) errs = append(errs, validateCustomDependencies(config.Build.Artifacts)...) errs = append(errs, validateSyncRules(config.Build.Artifacts)...) @@ -77,6 +78,76 @@ func validateImageNames(artifacts []*latest.Artifact) (errs []error) { return } +func validateArtifactDependencies(artifacts []*latest.Artifact) (errs []error) { + errs = append(errs, validateUniqueDependencyAliases(artifacts)...) + errs = append(errs, validateAcyclicDependencies(artifacts)...) + return +} + +// validateAcyclicDependencies makes sure all artifact dependencies are found and don't have cyclic references +func validateAcyclicDependencies(artifacts []*latest.Artifact) (errs []error) { + m := make(map[string]*latest.Artifact) + for _, artifact := range artifacts { + m[artifact.ImageName] = artifact + } + visited := make(map[string]bool) + for _, artifact := range artifacts { + if err := dfs(artifact, visited, make(map[string]bool), m); err != nil { + errs = append(errs, err) + return + } + } + return +} + +// dfs runs a Depth First Search algorithm for cycle detection in a directed graph +func dfs(artifact *latest.Artifact, visited, marked map[string]bool, artifacts map[string]*latest.Artifact) error { + if marked[artifact.ImageName] { + return fmt.Errorf("cycle detected in build dependencies involving '%s'", artifact.ImageName) + } + marked[artifact.ImageName] = true + defer func() { + marked[artifact.ImageName] = false + }() + if visited[artifact.ImageName] { + return nil + } + visited[artifact.ImageName] = true + + for _, dep := range artifact.Dependencies { + d, found := artifacts[dep.ImageName] + if !found { + return fmt.Errorf("unknown build dependency %q for artifact %q", dep.ImageName, artifact.ImageName) + } + if err := dfs(d, visited, marked, artifacts); err != nil { + return err + } + } + return nil +} + +// validateUniqueDependencyAliases makes sure that artifact dependency aliases are unique for each artifact +func validateUniqueDependencyAliases(artifacts []*latest.Artifact) (errs []error) { + type State int + var ( + unseen State = 0 + seen State = 1 + recorded State = 2 + ) + for _, a := range artifacts { + aliasMap := make(map[string]State) + for _, d := range a.Dependencies { + if aliasMap[d.Alias] == seen { + errs = append(errs, fmt.Errorf("invalid build dependency for artifact %q: alias %q repeated", a.ImageName, d.Alias)) + aliasMap[d.Alias] = recorded + } else if aliasMap[d.Alias] == unseen { + aliasMap[d.Alias] = seen + } + } + } + return +} + // validateDockerNetworkMode makes sure that networkMode is one of `bridge`, `none`, or `host` if set. func validateDockerNetworkMode(artifacts []*latest.Artifact) (errs []error) { for _, a := range artifacts { diff --git a/pkg/skaffold/schema/validation/validation_test.go b/pkg/skaffold/schema/validation/validation_test.go index 12cb2380f0b..61ee03e6008 100644 --- a/pkg/skaffold/schema/validation/validation_test.go +++ b/pkg/skaffold/schema/validation/validation_test.go @@ -20,6 +20,8 @@ import ( "fmt" "testing" + "github.com/google/go-cmp/cmp" + "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest" "github.com/GoogleContainerTools/skaffold/testutil" ) @@ -750,3 +752,133 @@ func TestValidateLogsConfig(t *testing.T) { }) } } + +func TestValidateArtifactCircularDependencies(t *testing.T) { + tests := []struct { + description string + artifactLen int + dependency map[int][]int + shouldErr bool + }{ + { + description: "artifacts with no dependency", + artifactLen: 5, + }, + { + description: "artifacts with no circular dependencies 1", + dependency: map[int][]int{ + 0: {2, 3}, + 1: {3}, + 2: {1}, + 3: {4}, + }, + artifactLen: 5, + }, + { + description: "artifacts with no circular dependencies 2", + dependency: map[int][]int{ + 0: {4, 5}, + 1: {4, 5}, + 2: {4, 5}, + 3: {4, 5}, + }, + artifactLen: 6, + }, + { + description: "artifacts with circular dependencies", + dependency: map[int][]int{ + 0: {2, 3}, + 1: {0}, + 2: {1}, + 3: {4}, + }, + artifactLen: 5, + shouldErr: true, + }, + { + description: "artifacts with circular dependencies (self)", + dependency: map[int][]int{ + 0: {0}, + 1: {}, + }, + artifactLen: 2, + shouldErr: true, + }, + { + description: "0 artifacts", + artifactLen: 0, + }, + } + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + artifacts := make([]*latest.Artifact, test.artifactLen) + for i := 0; i < test.artifactLen; i++ { + a := fmt.Sprintf("artifact%d", i+1) + artifacts[i] = &latest.Artifact{ImageName: a} + } + + setDependencies(artifacts, test.dependency) + errs := validateAcyclicDependencies(artifacts) + expected := []error{ + fmt.Errorf("cycle detected in build dependencies involving 'artifact1'"), + } + if test.shouldErr { + t.CheckDeepEqual(expected, errs, cmp.Comparer(errorsComparer)) + } else { + t.CheckDeepEqual(0, len(errs)) + } + }) + } +} + +// setDependencies constructs a graph of artifact dependencies using the map as an adjacency list representation of indices in the artifacts array. +// For example: +// m = { +// 0 : {1, 2}, +// 2 : {3}, +//} +// implies that a[0] artifact depends on a[1] and a[2]; and a[2] depends on a[3]. +func setDependencies(a []*latest.Artifact, d map[int][]int) { + for k, dep := range d { + for i := range dep { + a[k].Dependencies = append(a[k].Dependencies, &latest.ArtifactDependency{ + ImageName: a[dep[i]].ImageName, + }) + } + } +} + +func TestValidateArtifactDependencyUniqueness(t *testing.T) { + artifacts := []*latest.Artifact{ + { + ImageName: "artifact1", + Dependencies: []*latest.ArtifactDependency{ + {Alias: "alias2", ImageName: "artifact2a"}, + {Alias: "alias2", ImageName: "artifact2b"}, + }, + }, + { + ImageName: "artifact2", + Dependencies: []*latest.ArtifactDependency{ + {Alias: "alias1", ImageName: "artifact1"}, + {Alias: "alias2", ImageName: "artifact1"}, + }, + }, + } + expected := []error{ + fmt.Errorf(`invalid build dependency for artifact "artifact1": alias "alias2" repeated`), + fmt.Errorf(`unknown build dependency "artifact2a" for artifact "artifact1"`), + } + errs := validateArtifactDependencies(artifacts) + testutil.CheckDeepEqual(t, expected, errs, cmp.Comparer(errorsComparer)) +} + +func errorsComparer(a, b error) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return a.Error() == b.Error() +}