Try to import docker images before falling back to building #3891

arlyon · 2020-03-31T00:33:50Z

Fixes: #3886

Description

When perfoming a lookup of artifacts, this change will attempt to import artifacts from docker running locally as well as whatever remote registry is configured. This enables the worklow outlined in the issue above.

This PR is not done because there needs to be some discussion as it changes the behaviour of builds. Should the functionality be placed behind a flag? Since the contents of images aren't verified, this could for example cause problems if a malicious (or unaware) user uploads images tagged with an incorrect commit however for those who are aware of the (imo limited) drawbacks the productivity boost far-outweighs the risk. That said, I would argue to make it opt-in.

User facing changes

This would cause anyone running skaffold build to have the images be imported from docker, if possible, 'short circuiting' some builds. This is particularly useful when sharing images across teams (say someone needs to check out your branch to help diagnose an issue).

Follow-up work

Once the questions above are answered, then the PR needs to be adapted slightly and the docs updated.

googlebot · 2020-03-31T00:33:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

arlyon · 2020-03-31T00:36:28Z

@googlebot I signed it!

googlebot · 2020-03-31T00:36:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

pkg/skaffold/build/cache/lookup.go

arlyon · 2020-04-27T14:24:59Z

@tstromberg Just fixed the failing test and added the debug log!

dgageot · 2020-04-27T14:37:28Z

@arlyon I think it's best to make it opt-in or to make it possible to get the current behaviour back. I've always found it super important in a fully local mode to be able to never connect to a remote registry. For example, you are in a train, with no network, assuming all the layers where already downloaded, you are able to iterate quickly without reaching out to the network.

arlyon · 2020-04-27T17:12:45Z

@arlyon I think it's best to make it opt-in or to make it possible to get the current behaviour back. I've always found it super important in a fully local mode to be able to never connect to a remote registry. For example, you are in a train, with no network, assuming all the layers where already downloaded, you are able to iterate quickly without reaching out to the network.

You're right. For now I implemented it as a config option under local builds. Notice that this either imports or doesn't (it doesn't distinguish between only importing from local docker daemon and a remote registry)

build:
  artifacts:
  - image: arlyon/workout-server
    context: ./server/
  - image: arlyon/workout-learn
    context: ./learn/
  local:
    tryImportMissing: true
    push: true

Input welcome!

tejal29 · 2020-05-18T20:59:44Z

pkg/skaffold/schema/latest/config.go

@@ -191,6 +191,10 @@ type LocalBuild struct {
 	// connects to a remote cluster.
 	Push *bool `yaml:"push,omitempty"`

+	// TryImportMissing should attempt to import this artifact from
+	// Docker (either locally or a remote registry) if not in cache.
+	TryImportMissing bool `yaml:"tryImportMissing,omitempty"`


Can you please make this a pointer like other?
And then you would need to set to it false in defaultToLocalBuild in pkg/skaffold/schemadefaults/

tejal29 · 2020-06-08T20:29:16Z

@arlyon are you still working on this?

nkubala · 2020-06-22T20:42:38Z

@arlyon this PR is getting a bit stale but I'd love to help you get it in. let me know if you need any help with it! i'm still seeing a test failing on a data race in the cache somewhere, can you check that out?

arlyon · 2020-06-25T08:55:49Z

Hey, haven't been using skaffold at my day job recently but I have some time today so I'll rebase and see if I can get the requested changes in today.

codecov · 2020-06-25T10:06:18Z

Codecov Report

Merging #3891 into master will increase coverage by 0.00%.
The diff coverage is 77.27%.

@@           Coverage Diff           @@
##           master    #3891   +/-   ##
=======================================
  Coverage   73.81%   73.82%           
=======================================
  Files         347      347           
  Lines       13758    13799   +41     
=======================================
+ Hits        10156    10187   +31     
- Misses       2970     2977    +7     
- Partials      632      635    +3

Impacted Files	Coverage Δ
pkg/skaffold/build/cache/details.go	`13.79% <0.00%> (-1.03%)`	⬇️
pkg/skaffold/schema/latest/config.go	`100.00% <ø> (ø)`
pkg/skaffold/build/cache/lookup.go	`88.73% <71.42%> (-11.27%)`	⬇️
pkg/skaffold/build/cache/cache.go	`57.77% <100.00%> (+0.95%)`	⬆️
pkg/skaffold/build/cache/retrieve.go	`70.65% <100.00%> (+1.33%)`	⬆️
pkg/skaffold/build/local/types.go	`88.23% <100.00%> (+1.56%)`	⬆️
pkg/skaffold/runner/new.go	`68.18% <100.00%> (+0.63%)`	⬆️

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 9cd560b...21de9ea. Read the comment docs.

arlyon · 2020-06-25T10:55:05Z

I have updated with the requested changes and fixed the potential data race issues by introducing a RWMutex. make test fails complaining that schema v2beta5 is already released but I'm not sure what to do about that. There seems to be a PR to address it already. Do I base on top of that?

gsquared94 · 2020-07-08T18:15:07Z

You can rebase on master now. A new schema version has already been created. That should help with the tests

arlyon · 2020-07-24T15:00:02Z

Hate to ping because I know you're all busy but would appreciate a merge @gsquared94 @tejal29 :)

dgageot · 2020-07-24T18:21:06Z

I can TAL tomorrow. Added to m'y TODO Le ven. 24 juil. 2020 à 17:00, Alexander Lyon <notifications@github.com> a écrit :

…

Hate to ping because I know you're all busy but would appreciate a merge @gsquared94 <https://github.com/gsquared94> @tejal29 <https://github.com/tejal29> :) — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3891 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABFPF7Z4JYJ3KOXVKYIMCLR5GOYHANCNFSM4LXDX4IA> .

dgageot

Here's a first round of review

pkg/skaffold/build/local/types.go

pkg/skaffold/build/cache/lookup.go

pkg/skaffold/build/cache/cache.go

pkg/skaffold/build/cache/details.go

pkg/skaffold/build/local/types.go

nkubala · 2020-09-09T23:13:25Z

@arlyon this PR is getting a bit stale. do you still have the bandwidth to try and push it through?

f

…ring lookup

arlyon · 2020-09-10T10:39:56Z

Hi @nkubala sorry for the delay I though I'd pushed the requested changes but clearly not. I have rebased on master now!

MarlonGamez

@arlyon This looks good to me! Thanks for making all of the requested changes :)

arlyon requested review from balopat, dgageot, nkubala and tejal29 as code owners March 31, 2020 00:33

pull-request-size bot added the size/M label Mar 31, 2020

googlebot added the cla: no label Mar 31, 2020

googlebot added cla: yes and removed cla: no labels Mar 31, 2020

tstromberg added !! wip !! triage/discuss Items for discussion labels Apr 20, 2020

dgageot marked this pull request as draft April 21, 2020 08:24

dgageot changed the title ~~WIP: Try to import docker images before falling back to building~~ Try to import docker images before falling back to building Apr 21, 2020

dgageot removed the !! wip !! label Apr 21, 2020

tstromberg reviewed Apr 24, 2020

View reviewed changes

pkg/skaffold/build/cache/lookup.go Show resolved Hide resolved

arlyon force-pushed the docker-import branch from 1a24129 to dc8c6cd Compare April 27, 2020 14:24

arlyon force-pushed the docker-import branch 2 times, most recently from 6871453 to 8cc15a8 Compare April 27, 2020 14:30

arlyon force-pushed the docker-import branch 2 times, most recently from 9fc4706 to 77bf521 Compare April 27, 2020 17:10

briandealwis removed the triage/discuss Items for discussion label Apr 27, 2020

tejal29 reviewed May 18, 2020

View reviewed changes

tejal29 self-assigned this May 18, 2020

arlyon force-pushed the docker-import branch from 2a078d0 to 0cdd299 Compare June 25, 2020 09:56

pull-request-size bot added size/L and removed size/M labels Jun 25, 2020

arlyon force-pushed the docker-import branch 2 times, most recently from f8a0707 to 8757b78 Compare July 12, 2020 16:25

arlyon marked this pull request as ready for review July 13, 2020 08:42

arlyon requested a review from a team as a code owner July 13, 2020 08:42

gsquared94 added the kokoro:force-run forces a kokoro re-run on a PR label Jul 21, 2020

kokoro-team removed the kokoro:force-run forces a kokoro re-run on a PR label Jul 21, 2020

dgageot suggested changes Jul 27, 2020

View reviewed changes

pkg/skaffold/build/local/types.go Outdated Show resolved Hide resolved

pkg/skaffold/build/cache/lookup.go Outdated Show resolved Hide resolved

pkg/skaffold/build/cache/cache.go Outdated Show resolved Hide resolved

pkg/skaffold/build/cache/details.go Outdated Show resolved Hide resolved

dgageot suggested changes Jul 28, 2020

View reviewed changes

pkg/skaffold/build/local/types.go Outdated Show resolved Hide resolved

arlyon added 3 commits September 10, 2020 10:09

Try to import docker images before falling back to building

e3bc2de

Add flag to skaffold.yaml

fc94b23

f

Introduce RWMutex to prevent concurrent read/write during to cache du…

21de9ea

…ring lookup

arlyon force-pushed the docker-import branch from 8757b78 to 21de9ea Compare September 10, 2020 10:39

MarlonGamez approved these changes Sep 14, 2020

View reviewed changes

MarlonGamez merged commit 10275c6 into GoogleContainerTools:master Sep 14, 2020

arlyon deleted the docker-import branch September 15, 2020 08:44

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

Try to import docker images before falling back to building #3891

Try to import docker images before falling back to building #3891

arlyon commented Mar 31, 2020 •

edited

googlebot commented Mar 31, 2020

arlyon commented Mar 31, 2020

googlebot commented Mar 31, 2020

arlyon commented Apr 27, 2020

dgageot commented Apr 27, 2020

arlyon commented Apr 27, 2020 •

edited

tejal29 May 18, 2020

tejal29 commented Jun 8, 2020

nkubala commented Jun 22, 2020

arlyon commented Jun 25, 2020

codecov bot commented Jun 25, 2020 •

edited

arlyon commented Jun 25, 2020 •

edited

gsquared94 commented Jul 8, 2020

arlyon commented Jul 24, 2020

dgageot commented Jul 24, 2020 via email

dgageot left a comment

nkubala commented Sep 9, 2020

arlyon commented Sep 10, 2020

MarlonGamez left a comment

Try to import docker images before falling back to building #3891

Try to import docker images before falling back to building #3891

Conversation

arlyon commented Mar 31, 2020 • edited

googlebot commented Mar 31, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

arlyon commented Mar 31, 2020

googlebot commented Mar 31, 2020

arlyon commented Apr 27, 2020

dgageot commented Apr 27, 2020

arlyon commented Apr 27, 2020 • edited

tejal29 May 18, 2020

Choose a reason for hiding this comment

tejal29 commented Jun 8, 2020

nkubala commented Jun 22, 2020

arlyon commented Jun 25, 2020

codecov bot commented Jun 25, 2020 • edited

Codecov Report

arlyon commented Jun 25, 2020 • edited

gsquared94 commented Jul 8, 2020

arlyon commented Jul 24, 2020

dgageot commented Jul 24, 2020 via email

dgageot left a comment

Choose a reason for hiding this comment

nkubala commented Sep 9, 2020

arlyon commented Sep 10, 2020

MarlonGamez left a comment

Choose a reason for hiding this comment

arlyon commented Mar 31, 2020 •

edited

arlyon commented Apr 27, 2020 •

edited

codecov bot commented Jun 25, 2020 •

edited

arlyon commented Jun 25, 2020 •

edited