Allow SAVE ARTIFACT after a `--push` #735

dchw · 2021-01-27T17:38:14Z

No description provided.

dchw · 2021-01-27T17:39:42Z

builder/builder.go

+	s         *solver
+	opt       Opt
+	resolver  *buildcontext.Resolver
+	builtMain bool


Same concept as phases; just need to know one thing instead of a list index... we may need phases in future

dchw · 2021-01-27T17:40:15Z

builder/builder.go

@@ -124,6 +125,9 @@ func (b *Builder) convertAndBuild(ctx context.Context, target domain.Target, opt
 	destPathWhitelist := make(map[string]bool)
 	manifestLists := make(map[string][]manifest) // parent image -> child images
 	var mts *states.MultiTarget
+	depIndex := 0
+	imageIndex := 0
+	dirIndex := 0


Indexes need to increase monotonically across all artifacts to avoid overwriting issues, so keep them out side and close on them

dchw · 2021-01-27T17:50:17Z

builder/builder.go

+	}
+	return sts.SaveLocals
+}
+


Similar things to phases to enable use of the gwClient. However, its not quite the same as before where the state tracks it. Kept it simpler using the existing RunPush struct, and phases only happen here in builder where they are cared about now.

Open to pushing this down and cleaning up some more; perhaps as a separate PR though? Its just touching a lot to do that

Not sure what you have in mind by "cleaning up some more"?

This looks pretty good BTW.

Cleaning up meaning pushing this down into the states package. Think of it as phases-lite; so the builder doesn't have to be so grabby with SingleTargets fields

I think the way it is in this PR is simple enough. We could do that if things get more complex.

dchw · 2021-01-27T17:51:15Z

earthfile2llb/converter.go

+				llb.WithCustomNamef(
+					"%sSAVE ARTIFACT %s%s %s AS LOCAL %s",
+					c.vertexPrefix(false), strIf(ifExists, "--if-exists "), saveFrom, artifact.String(), saveAsLocalTo))
+		}


State to copy from depends on if we are pushing

dchw · 2021-01-27T17:52:26Z

earthfile2llb/listener.go

@@ -462,7 +458,7 @@ func (l *listener) ExitSaveArtifact(c *parser.SaveArtifactContext) {
 	saveFrom := l.expandArgs(fs.Args()[0], false)
 	saveTo = l.expandArgs(saveTo, false)
 	saveAsLocalTo = l.expandArgs(saveAsLocalTo, false)
-	err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, *keepTs, *keepOwn, *ifExists)
+	err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, *keepTs, *keepOwn, *ifExists, l.pushOnlyAllowed)


pushOnlyAllowed is here to signify we have encountered a push. Could rename this var; but didn't because im not globally allowing everything like before.

If you wanted you could do

Suggested change

err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, *keepTs, *keepOwn, *ifExists, l.pushOnlyAllowed)

isPushPhase := l.pushOnlyAllowed

err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, *keepTs, *keepOwn, *ifExists, isPushPhase)

Ehhhhhhhh that feels like extra ceremony. Not worth it IMO; I would rather rename it to pushEncountered across all of converter if I went renaming; since that preserves the business logic meaning well enough

🤷

I like ceremonies :-)

dchw · 2021-01-27T17:52:58Z

examples/tests/Earthfile

+    RUN --privileged \
+        --entrypoint \
+        --mount=type=tmpfs,target=/tmp/earthly \
+        -- +copy-test 2>&1 | perl -pe 'BEGIN {$status=1} END {exit $status} $status=0 if /not found/;'


Can't copy artifacts that were saved in a different targets --push

vladaionescu

This looks pretty good - left an important comment, though.

I assume the output has two ===== SUCCESS ====== lines, right? We should print out a more obvious distinction for the two phases. Maybe ===== Build phase SUCCESS ====== and ===== Push phase SUCCESS ======. Or something like that - whatever looks pretty enough.

vladaionescu · 2021-01-27T18:00:06Z

builder/builder.go

 func (b *Builder) stateToRef(ctx context.Context, gwClient gwclient.Client, state llb.State, platform *specs.Platform) (gwclient.Reference, error) {
-	if b.opt.NoCache {
+	if b.opt.NoCache || b.builtMain {


This doesn't look right. I think it should be

Suggested change

if b.opt.NoCache || b.builtMain {

if b.opt.NoCache && !b.builtMain {

Meaning that for the push phase, we do not add the ignore cache flag here (instead we rely on the converter having added this flag to individual commands).

If we add the ignore cache flag here for the push phase, then everything the push phase depends on (including the main phase) will be executed with no cache - so the main phase ends up being executed twice.

What you initially suggested is what I had before; but I noticed that solving the states for the separate artifacts was showing up as cached. I used to have some ls sentinels in here to show when things were being re-run, but I must have pulled them out before swapping to this.

I was trying to get SAVE ARTIFACT inside --push to also not cache when running with --no-cache. Ill rework this some to get both scenarios working

vladaionescu · 2021-01-27T18:01:53Z

builder/builder.go

+	}
+	return sts.SaveLocals
+}
+


Not sure what you have in mind by "cleaning up some more"?

This looks pretty good BTW.

dchw · 2021-01-27T18:32:30Z

@vladaionescu I agree on more distinctive success lines. Though, that is also the current behavior.

Sticking the name in the line feels fine; All the alternatives I can think of would require a little more work.

vladaionescu · 2021-01-27T19:31:28Z

Yeah, let's stick the name in. I think the current behavior is inconsistent: only prints success once, but can print both success and then fail.

dchw · 2021-01-27T22:38:11Z

@vladaionescu Updated to your logic.

The one downside to this is that unchanged artifacts after --push still use cache even when specifying --no-cache on the CLI.

dchw · 2021-01-27T22:39:05Z

How do you feel about solving artifacts state with something like this:

func (b *Builder) artifactStateToRef(ctx context.Context, gwClient gwclient.Client, state llb.State, platform *specs.Platform) (gwclient.Reference, error) {
	if b.opt.NoCache || b.builtMain{
		state = state.SetMarshalDefaults(llb.IgnoreCache)
	}
	return llbutil.StateToRef(ctx, gwClient, state, platform, b.opt.CacheImports)
}

The idea being that we never use cache for artifacts after a push, regardless of the CLI flag.

vladaionescu · 2021-01-27T22:40:12Z

Sounds good - I think we had that bug before too.

…le push phase in general

dchw · 2021-01-27T22:54:36Z

This is part of the actual solution we plan to ship in place of #728. Relates to #586.

dchw added 5 commits January 26, 2021 22:56

Allow save artifacts in RunPush

314046f

Artifact CopyOp should operate on RunPush when after --push

4f7dcd9

Remove commented code

b713ac2

Builder support for SaveArtifact after push

b42d335

Integration test

1f8103a

dchw requested review from alexcb and vladaionescu January 27, 2021 17:38

dchw commented Jan 27, 2021

View reviewed changes

vladaionescu reviewed Jan 27, 2021

View reviewed changes

Update to use the original logic

17f0e81

vladaionescu approved these changes Jan 27, 2021

View reviewed changes

Never use cache to save artifacts, when CLI fLg was set OR in the who…

ac38366

…le push phase in general

dchw added 5 commits January 27, 2021 16:13

Fix no --push messaging

645f9e4

Clarify when to show push messaging

031ac5d

Merge branch 'main' into corey/after-push-5

fde85f6

Merge branch 'main' into corey/after-push-5

1be1888

bump

18ebfcb

dchw merged commit 6744f25 into main Jan 28, 2021

dchw deleted the corey/after-push-5 branch January 28, 2021 18:14

renovate bot mentioned this pull request Jun 6, 2021

chore(deps): update dependency earthly/earthly to v0.6.10 ctison/actions#27

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow SAVE ARTIFACT after a `--push` #735

Allow SAVE ARTIFACT after a `--push` #735

dchw commented Jan 27, 2021

dchw Jan 27, 2021

dchw Jan 27, 2021

dchw Jan 27, 2021

dchw Jan 27, 2021

vladaionescu Jan 27, 2021

dchw Jan 27, 2021

vladaionescu Jan 27, 2021

dchw Jan 27, 2021

dchw Jan 27, 2021

vladaionescu Jan 27, 2021

dchw Jan 27, 2021

vladaionescu Jan 27, 2021

dchw Jan 27, 2021

vladaionescu left a comment

vladaionescu Jan 27, 2021

dchw Jan 27, 2021 •

edited

vladaionescu Jan 27, 2021

dchw commented Jan 27, 2021

vladaionescu commented Jan 27, 2021

dchw commented Jan 27, 2021

dchw commented Jan 27, 2021 •

edited

vladaionescu commented Jan 27, 2021

dchw commented Jan 27, 2021

	err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, keepTs, keepOwn, *ifExists, l.pushOnlyAllowed)
	isPushPhase := l.pushOnlyAllowed
	err = l.converter.SaveArtifact(l.ctx, saveFrom, saveTo, saveAsLocalTo, keepTs, keepOwn, *ifExists, isPushPhase)

	if b.opt.NoCache \|\| b.builtMain {
	if b.opt.NoCache && !b.builtMain {

Allow SAVE ARTIFACT after a --push #735

Allow SAVE ARTIFACT after a --push #735

Conversation

dchw commented Jan 27, 2021

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

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

vladaionescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchw Jan 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchw commented Jan 27, 2021

vladaionescu commented Jan 27, 2021

dchw commented Jan 27, 2021

dchw commented Jan 27, 2021 • edited

vladaionescu commented Jan 27, 2021

dchw commented Jan 27, 2021

Allow SAVE ARTIFACT after a `--push` #735

Allow SAVE ARTIFACT after a `--push` #735

dchw Jan 27, 2021 •

edited

dchw commented Jan 27, 2021 •

edited