Add design proposal for supporting dependencies between build artifacts #4794

gsquared94 · 2020-09-16T16:22:52Z

Added a design proposal for supporting build artifact dependencies.

codecov · 2020-09-16T16:31:10Z

Codecov Report

Merging #4794 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4794   +/-   ##
=======================================
  Coverage   71.73%   71.73%           
=======================================
  Files         353      353           
  Lines       12090    12090           
=======================================
  Hits         8673     8673           
  Misses       2773     2773           
  Partials      644      644

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 65583f1...f7df32b. Read the comment docs.

docs/design_proposals/artifact-dependencies.md

tejal29

Thanks for the explanation. Few other follows up can be discussed in a team meeting setting.

just 1 qq. (pardon me if this was clarified in the design document linked)

build:
  tagPolicy:
    gitCommit: {}
  - image:simple-go-app
    ..
   - image: leeroy-app
     requires:
       - image: simple-go-app
         alias: BASE

In this case with the tagging strategy, for the dependent docker build arg BASE will replaced with fully qualified image name right.
e.g. if default-repo is set to "gcr.io/X`

BASE = "gcr.io/X/simple-go-app@<TAG>

Maybe you could add this in section called "Docker Builder Changes" if not in the parent design document.

docs/design_proposals/artifact-dependencies.md

gsquared94 · 2020-09-18T12:02:43Z

Thanks for the explanation. Few other follows up can be discussed in a team meeting setting.

just 1 qq. (pardon me if this was clarified in the design document linked)
build:
  tagPolicy:
    gitCommit: {}
  - image:simple-go-app
    ..
   - image: leeroy-app
     requires:
       - image: simple-go-app
         alias: BASE
In this case with the tagging strategy, for the dependent docker build arg BASE will replaced with fully qualified image name right.
e.g. if default-repo is set to "gcr.io/X`
BASE = "gcr.io/X/simple-go-app@<TAG>
Maybe you could add this in section called "Docker Builder Changes" if not in the parent design document.

Added section Referencing dependencies inside docker builder and also added future work with initial idea about how the other builder could support it.

briandealwis

Should have submitted this as individual comments rather than as a review

docs/design_proposals/artifact-dependencies.md

nkubala

this looks really good, I think it's close to done. @briandealwis and @tejal29 any final feedback?

docs/design_proposals/artifact-dependencies.md

nkubala · 2020-09-22T17:56:01Z

docs/design_proposals/artifact-dependencies.md

+
+There are two quirks in this: 
+- It reports in the order of artifacts in the `Artifact` slice instead of the actual order in which they get built. 
+- It only reports a single artifact build failure even though there could have been multiple failures.


#2077 and #2000 :)

docs/design_proposals/artifact-dependencies.md

tejal29

LGMT to me except for Buildpacks. I will defer to Brain.

docs/design_proposals/artifact-dependencies.md

briandealwis · 2020-09-24T03:46:27Z

docs/design_proposals/artifact-dependencies.md

+
+Skaffold currently allows specifying the `concurrency` property in `build` which affects how many builds can be running at the same time. However it doesn't address the issue of certain builders (`jib` and `bazel`) not being safe for multiple concurrent runs against the same workspace or context. We can fix this also since we are reworking the build controller anyways. 
+
+We define a concept of lease on workspaces by preprocessing the list of artifacts. Each builder tries to acquire a lease on the context/workspace prior to starting the build. Only workspaces associated with concurrency-safe builders allot multiple leases, otherwise it assigns one lease at a time.


What happens if we have several artifacts of different builder-type but same context?

Only workspaces associated with concurrency-safe builders allot multiple leases

So even if one builder is concurrency unsafe, it'll be single leases for that context.

Sounds like we need a builder interface (rather than just the ArtifactBuilder function signature). Can you sketch out what this interface would look like?

docs/design_proposals/artifact-dependencies.md

googlebot · 2020-09-24T06:11:56Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

googlebot · 2020-09-24T06:35:11Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

briandealwis · 2020-09-24T14:01:42Z

docs/design_proposals/artifact-dependencies.md

+- Unique artifact aliases.
+  - We ensure that within *each* artifact dependency slice the aliases are unique. 
+- Valid aliases
+  - Since aliases are used as environment variable keys or template keys we validate that they match the regex `[a-zA-Z_][a-zA-Z0-9_]*`


I think we should move this requirement into the applicable builders. Just in case some builders may find it useful to be able to alias image-refs directly, like a rewrite rule.

I think adding it as a config validation helps to show the error early and immediately. I've changed it to validate only for docker and custom builders where we are using the alias with naming restrictions.

docs/design_proposals/artifact-dependencies.md

briandealwis · 2020-09-24T14:13:15Z

docs/design_proposals/artifact-dependencies.md

+}
+```
+
+This necessitates all multi-artifact builder implementations to also accept a slice of already built artifacts' information. All single artifact builders require an `ArtifactResolver` that can provide the required artifacts.


It would be helpful to explain why multi-artifact builders need a list of already-built artifacts (vs an ArtifactResolver)

docs/design_proposals/artifact-dependencies.md

briandealwis · 2020-09-24T14:18:40Z

docs/design_proposals/artifact-dependencies.md

+
+Skaffold currently allows specifying the `concurrency` property in `build` which affects how many builds can be running at the same time. However it doesn't address the issue of certain builders (`jib` and `bazel`) not being safe for multiple concurrent runs against the same workspace or context. We can fix this also since we are reworking the build controller anyways. 
+
+We define a concept of lease on workspaces by preprocessing the list of artifacts. Each builder tries to acquire a lease on the context/workspace prior to starting the build. Only workspaces associated with concurrency-safe builders allot multiple leases, otherwise it assigns one lease at a time.


Sounds like we need a builder interface (rather than just the ArtifactBuilder function signature). Can you sketch out what this interface would look like?

docs/design_proposals/artifact-dependencies.md

briandealwis

LGTM.

docs/design_proposals/artifact-dependencies.md

gsquared94 requested a review from a team as a code owner September 16, 2020 16:22

gsquared94 requested a review from nkubala September 16, 2020 16:22

googlebot added the cla: yes label Sep 16, 2020

pull-request-size bot added the size/L label Sep 16, 2020

tejal29 suggested changes Sep 16, 2020

View reviewed changes

gsquared94 requested a review from tejal29 September 17, 2020 15:57

nkubala assigned tejal29 Sep 17, 2020

tejal29 reviewed Sep 17, 2020

View reviewed changes

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

gsquared94 requested a review from tejal29 September 18, 2020 15:15

gsquared94 mentioned this pull request Sep 18, 2020

Support build artifact dependencies #4752

Closed

briandealwis reviewed Sep 18, 2020

View reviewed changes

docs/design_proposals/artifact-dependencies.md Outdated Show resolved Hide resolved

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

gsquared94 changed the title ~~Add design proposal for build artifact dependencies.~~ Add design proposal for supporting dependencies between build artifacts Sep 21, 2020

briandealwis mentioned this pull request Sep 22, 2020

Building a base image artifact #3451

Closed

nkubala reviewed Sep 22, 2020

View reviewed changes

gsquared94 requested review from nkubala and briandealwis September 23, 2020 11:50

tejal29 approved these changes Sep 23, 2020

View reviewed changes

briandealwis reviewed Sep 24, 2020

View reviewed changes

gsquared94 force-pushed the build-artifact-dependencies branch from 0b6b790 to 28c3b01 Compare September 24, 2020 06:11

googlebot added cla: no and removed cla: yes labels Sep 24, 2020

Design proposal for supporting dependencies between build artifacts

92d9af6

gsquared94 force-pushed the build-artifact-dependencies branch from 28c3b01 to 92d9af6 Compare September 24, 2020 06:35

googlebot added cla: yes and removed cla: no labels Sep 24, 2020

fix typo

497ea95

gsquared94 added 3 commits September 24, 2020 12:24

simplify

04b6656

fix sample code

f453c6d

validate alias

2a48ff9

briandealwis reviewed Sep 24, 2020

View reviewed changes

gsquared94 added 2 commits September 24, 2020 21:21

Remove Go templating.

bc1e0ef

Relax alias validation

5fc3172

loosebazooka reviewed Sep 24, 2020

View reviewed changes

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

docs/design_proposals/artifact-dependencies.md Show resolved Hide resolved

briandealwis approved these changes Sep 24, 2020

View reviewed changes

gsquared94 added 2 commits September 25, 2020 19:01

PR feedback

cb45ad7

install postcss explicitly

f7df32b

gsquared94 merged commit 558dfa0 into GoogleContainerTools:master Sep 25, 2020

fmotrifork mentioned this pull request Oct 1, 2020

skaffold 1.15.0 fishworks/fish-food#989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add design proposal for supporting dependencies between build artifacts #4794

Add design proposal for supporting dependencies between build artifacts #4794

gsquared94 commented Sep 16, 2020

codecov bot commented Sep 16, 2020 •

edited

Loading

tejal29 left a comment

gsquared94 commented Sep 18, 2020

briandealwis left a comment

nkubala left a comment

nkubala Sep 22, 2020

tejal29 left a comment

briandealwis Sep 24, 2020

gsquared94 Sep 24, 2020

briandealwis Sep 24, 2020

gsquared94 Sep 24, 2020

googlebot commented Sep 24, 2020

googlebot commented Sep 24, 2020

briandealwis Sep 24, 2020

gsquared94 Sep 24, 2020

briandealwis Sep 24, 2020

briandealwis Sep 24, 2020

briandealwis left a comment


		Skaffold currently allows specifying the `concurrency` property in `build` which affects how many builds can be running at the same time. However it doesn't address the issue of certain builders (`jib` and `bazel`) not being safe for multiple concurrent runs against the same workspace or context. We can fix this also since we are reworking the build controller anyways.

		We define a concept of lease on workspaces by preprocessing the list of artifacts. Each builder tries to acquire a lease on the context/workspace prior to starting the build. Only workspaces associated with concurrency-safe builders allot multiple leases, otherwise it assigns one lease at a time.

Add design proposal for supporting dependencies between build artifacts #4794

Add design proposal for supporting dependencies between build artifacts #4794

Conversation

gsquared94 commented Sep 16, 2020

codecov bot commented Sep 16, 2020 • edited Loading

Codecov Report

tejal29 left a comment

Choose a reason for hiding this comment

gsquared94 commented Sep 18, 2020

briandealwis left a 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

tejal29 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

googlebot commented Sep 24, 2020

googlebot commented Sep 24, 2020

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

briandealwis left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2020 •

edited

Loading