Support build artifact dependencies #4752

gsquared94 · 2020-09-01T16:15:35Z

Related: #4713

Description:

Added ArtifactDependency struct to Artifact. This will allow us to define a requires section in the skaffold config

build:
 artifacts:
   - image: leeroy-app
     requires:
       - alias: BASE
         image: simple-go-app

Added skaffold config validations:
- Artifact dependencies don't result in a circular dependency
- The same alias or image name aren't repeated in a single dependency list
Replaced InSequence and InParallel build controllers with an InOrder function in scheduler.go

// 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) {

Next steps:

Inject the image dependencies into the various builders. Will start with docker and buildpacks builder where it's obvious that this info will be passed in as build args and env variable respectively.

…troller; validations

codecov · 2020-09-01T16:27:20Z

Codecov Report

Merging #4752 into master will increase coverage by 0.16%.
The diff coverage is 95.50%.

@@            Coverage Diff             @@
##           master    #4752      +/-   ##
==========================================
+ Coverage   73.81%   73.97%   +0.16%     
==========================================
  Files         347      346       -1     
  Lines       13761    13848      +87     
==========================================
+ Hits        10157    10244      +87     
  Misses       2971     2971              
  Partials      633      633

Impacted Files	Coverage Δ
pkg/skaffold/build/cluster/cluster.go	`26.47% <0.00%> (ø)`
pkg/skaffold/build/cluster/kaniko.go	`0.00% <0.00%> (ø)`
pkg/skaffold/build/gcb/cloud_build.go	`0.00% <0.00%> (ø)`
pkg/skaffold/schema/latest/config.go	`100.00% <ø> (ø)`
pkg/skaffold/build/scheduler.go	`98.79% <96.42%> (ø)`
pkg/skaffold/build/local/local.go	`63.63% <100.00%> (ø)`
pkg/skaffold/diagnose/diagnose.go	`58.33% <100.00%> (ø)`
pkg/skaffold/docker/context.go	`80.00% <100.00%> (ø)`
pkg/skaffold/docker/image.go	`73.36% <100.00%> (ø)`
pkg/skaffold/schema/defaults/defaults.go	`91.21% <100.00%> (+0.21%)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 677d665...743f483. Read the comment docs.

briandealwis

Looking good!

pkg/skaffold/schema/latest/config.go

pkg/skaffold/schema/validation/validation.go

briandealwis · 2020-09-01T18:56:45Z

pkg/skaffold/schema/validation/validation.go

+			if namesMap[d.ImageName] == seen {
+				errs = append(errs, fmt.Errorf("invalid build dependency for artifact %q: image name %q repeated", a.ImageName, d.ImageName))
+				namesMap[d.ImageName] = recorded
+			} else if namesMap[d.ImageName] == unseen {
+				namesMap[d.ImageName] = seen
+			}


Why do we disallow repeated image names? We do know some people have several artifacts using the same context.

I believe this is just validating that for each artifact, all dependency aliases/image names are unique. I don't think we want to allow two distinct dependencies which share an image name (and are then effectively the same dependency), right?

exactly what @nkubala said

This code is validating that

all aliases are unique, and

all image references are unique

I agree with (1). But people use multi-stage Dockerfiles with a number of build images with names. We can let that decision be set in the skaffold.yaml.

artifacts: - image: our/jarbuild - image: our/app requires: - alias: lib1 image: our/jarbuild - alias: lib2 image: our/jarbuild

Or let me turn this around: what's the compelling reason why we cannot let the same image have multiple aliases?

^^^ (Github didn't unresolve the conversation when I submitted this comment as part of a review)

@briandealwis I think I get your point now.
The dockerfile:

FROM $builder as builder1 RUN foo1 FROM $builder as builder2 RUN foo2 FROM bar COPY --from=builder1 foobar

can be simplified into:

FROM $builder1 RUN foo1 FROM $builder2 RUN foo2 FROM bar COPY --from=$builder1 foobar

where $builder1 and $builder2 both reference the same image that $builder referenced earlier.
Is this what you mean? Although would this really work since $builder1 and $builder2 resolve to the same value wouldn't it be ambiguous?

There's no compelling reason for this validation, I just thought that it didn't seem valuable to permit multiple aliases referencing the same image. But I'll remove it anyway.

Removed this extra validation in latest commit

pkg/skaffold/schema/validation/validation.go

pkg/skaffold/schema/validation/validation_test.go

pkg/skaffold/build/scheduler.go

pkg/skaffold/build/scheduler_test.go

nkubala

a few comments but in general this is looking good

nkubala · 2020-09-10T21:05:27Z

pkg/skaffold/build/scheduler.go

+	for _, dep := range a.requiredArtifactChans {
+		// wait for dependency to complete build
+		select {
+		case <-ctx.Done():


do we need to catch ctx.Cancelled() here as well? or is that handled somewhere further down/up?

I'm not sure I follow. context here has already been setup with cancellation so when cancel gets called in the main go routine it'll close this done channel.
From docs WithCancel arranges for Done to be closed when cancel is called;

I get it know.

case <-ctx.Done(): if ctx.Err() == ctx.Cancelled(): ...

I'll add that. Thanks!

pkg/skaffold/build/scheduler.go

pkg/skaffold/build/scheduler_test.go

pkg/skaffold/schema/latest/config.go

nkubala · 2020-09-10T21:14:41Z

pkg/skaffold/schema/validation/validation.go

+	for _, artifact := range artifacts {
+		if err := dfs(artifact, visited, make(map[string]bool), m); err != nil {
+			errs = append(errs, err)
+			return


since this is part of a larger error collection block that finishes all checks and surfaces all errors, should we not fail fast here and let this run its course?

since this validation is for cycle detection, we'll get stuck on a specific cycle if it exists and it seemed non-trivial to exit that cycle and then try to find the other cycles. So we won't be able to collect all cycle violations at a go which is why I thought to just surface out the first violation and the user can fix it one by one. WDYT?

@briandealwis WDYT?

nkubala · 2020-09-10T21:17:34Z

pkg/skaffold/schema/validation/validation.go

+			if namesMap[d.ImageName] == seen {
+				errs = append(errs, fmt.Errorf("invalid build dependency for artifact %q: image name %q repeated", a.ImageName, d.ImageName))
+				namesMap[d.ImageName] = recorded
+			} else if namesMap[d.ImageName] == unseen {
+				namesMap[d.ImageName] = seen
+			}


I believe this is just validating that for each artifact, all dependency aliases/image names are unique. I don't think we want to allow two distinct dependencies which share an image name (and are then effectively the same dependency), right?

Co-authored-by: Brian de Alwis <[email protected]>

gsquared94 · 2020-09-14T15:12:40Z

PTAL again @nkubala and @briandealwis

briandealwis

What happens / should happen if there when one of the artifacts encounters a build error?

pkg/skaffold/schema/validation/validation_test.go

briandealwis · 2020-09-14T17:43:54Z

pkg/skaffold/schema/validation/validation.go

+			if namesMap[d.ImageName] == seen {
+				errs = append(errs, fmt.Errorf("invalid build dependency for artifact %q: image name %q repeated", a.ImageName, d.ImageName))
+				namesMap[d.ImageName] = recorded
+			} else if namesMap[d.ImageName] == unseen {
+				namesMap[d.ImageName] = seen
+			}


This code is validating that

all aliases are unique, and

all image references are unique

I agree with (1). But people use multi-stage Dockerfiles with a number of build images with names. We can let that decision be set in the skaffold.yaml.

artifacts: - image: our/jarbuild - image: our/app requires: - alias: lib1 image: our/jarbuild - alias: lib2 image: our/jarbuild

Or let me turn this around: what's the compelling reason why we cannot let the same image have multiple aliases?

pkg/skaffold/build/scheduler_test.go

pkg/skaffold/build/scheduler.go

briandealwis · 2020-09-15T04:03:33Z

Handling dependency errors can be done in runBuild().

gsquared94 · 2020-09-15T10:37:23Z

What happens / should happen if there when one of the artifacts encounters a build error?

Currently it just runs all other builds even if a required build has failed since I haven't introduced the dependency aliases into the builders yet. Once the builders actually read the ArtifactDependency and search for their images it'll fail with a required image not found sort of error.

I'm thinking about changing the current behavior where we just report about the first failed build (see here) and instead string.Join(errors, "\n") and report all the failed builds. If I did that it'd probably be even better to report the errors in the order that they were built. I'll add details about that in the follow up PR. WDYT?

gsquared94 · 2020-09-15T10:40:50Z

Handling dependency errors can be done in runBuild().

yes, that's the intention. Let me know if you want me to add code to handle dependency build failures in this PR itself. Otherwise I'll do that in the followup

pkg/skaffold/schema/validation/validation.go

gsquared94 · 2020-09-18T15:54:59Z

closing this for now, will reopen after incorporating changes from #4794 when that gets approved.

Schema change for build artifact dependencies; new in order build con…

fd1fd20

…troller; validations

pull-request-size bot added the size/XL label Sep 1, 2020

googlebot added the cla: yes label Sep 1, 2020

gsquared94 requested a review from briandealwis September 1, 2020 16:15

gsquared94 marked this pull request as ready for review September 1, 2020 17:36

gsquared94 requested a review from a team as a code owner September 1, 2020 17:36

gsquared94 requested a review from nkubala September 1, 2020 17:42

gsquared94 added 3 commits September 2, 2020 13:19

Simplify go func.

602fde5

fix comment

83fee48

simplify InOrder implementation

ea3c97c

briandealwis reviewed Sep 10, 2020

View reviewed changes

nkubala reviewed Sep 10, 2020

View reviewed changes

gsquared94 and others added 11 commits September 11, 2020 11:01

Update pkg/skaffold/schema/validation/validation.go

2830a89

Co-authored-by: Brian de Alwis <[email protected]>

Merge remote-tracking branch 'upstream/master' into fix-4713

16addd6

Fix tests

fa850a2

Undo change to old schema

26e8d03

Fix schema changes

ac1aea7

Rename functions

b030a10

Update pkg/skaffold/schema/latest/config.go

bae05d8

Co-authored-by: Brian de Alwis <[email protected]>

Add function descriptions

6eca5f7

Fix docs

2c21394

Add failure test case

467c37a

Make alias optional and default to image in an artifact dependency

9775e2a

gsquared94 requested review from briandealwis and nkubala September 14, 2020 07:33

briandealwis changed the title ~~Schema change for build artifact dependencies; new "in order" build controller; validations~~ Support build artifact dependencies Sep 14, 2020

briandealwis reviewed Sep 14, 2020

View reviewed changes

rename artifactWithDeps to artifactChanModel

9f42f6d

add more comments about how InOrder builder works

95b9410

gsquared94 added 2 commits September 15, 2020 16:25

Add more comments to InOrder func.

a979897

remove unique imageNames validation

9aa0edd

gsquared94 requested a review from briandealwis September 15, 2020 11:48

gsquared94 commented Sep 15, 2020

View reviewed changes

pkg/skaffold/schema/validation/validation.go Show resolved Hide resolved

gsquared94 marked this pull request as draft September 15, 2020 16:02

Fix circular dependency check validation.

743f483

gsquared94 marked this pull request as ready for review September 15, 2020 17:12

nkubala assigned briandealwis Sep 17, 2020

gsquared94 closed this Sep 18, 2020

gsquared94 mentioned this pull request Sep 28, 2020

Support dependencies between build artifacts - 1 #4828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support build artifact dependencies #4752

Support build artifact dependencies #4752

gsquared94 commented Sep 1, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

briandealwis left a comment

briandealwis Sep 1, 2020

nkubala Sep 10, 2020

gsquared94 Sep 11, 2020

briandealwis Sep 14, 2020

briandealwis Sep 15, 2020

gsquared94 Sep 15, 2020 •

edited

Loading

gsquared94 Sep 15, 2020

gsquared94 Sep 15, 2020

nkubala left a comment

nkubala Sep 10, 2020

gsquared94 Sep 11, 2020

gsquared94 Sep 18, 2020

nkubala Sep 10, 2020

gsquared94 Sep 11, 2020

gsquared94 Sep 14, 2020

nkubala Sep 10, 2020

gsquared94 commented Sep 14, 2020

briandealwis left a comment

briandealwis Sep 14, 2020

briandealwis commented Sep 15, 2020

gsquared94 commented Sep 15, 2020

gsquared94 commented Sep 15, 2020 •

edited

Loading

gsquared94 commented Sep 18, 2020

Support build artifact dependencies #4752

Support build artifact dependencies #4752

Conversation

gsquared94 commented Sep 1, 2020 • edited Loading

Description:

Next steps:

codecov bot commented Sep 1, 2020 • edited Loading

Codecov Report

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsquared94 Sep 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkubala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsquared94 commented Sep 14, 2020

briandealwis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briandealwis commented Sep 15, 2020

gsquared94 commented Sep 15, 2020

gsquared94 commented Sep 15, 2020 • edited Loading

gsquared94 commented Sep 18, 2020

gsquared94 commented Sep 1, 2020 •

edited

Loading

codecov bot commented Sep 1, 2020 •

edited

Loading

gsquared94 Sep 15, 2020 •

edited

Loading

gsquared94 commented Sep 15, 2020 •

edited

Loading