-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
llb: asyncronous llb graph generation support #1426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'm assuming we're landing this first before the follow up?
@@ -88,7 +89,14 @@ func goFromGit(repo, tag string) llb.StateOption { | |||
Dirf("/go/src/%s", repo). | |||
Run(llb.Shlexf("git checkout -q %s", tag)).Root() | |||
return func(s llb.State) llb.State { | |||
return s.With(copyFrom(src, "/go", "/")).Reset(s).Dir(src.GetDir()) | |||
return s.With(copyFrom(src, "/go", "/")).Reset(s).Async(func(ctx context.Context, s llb.State) (llb.State, error) { | |||
// TODO: add s.With(s2.DirValue) or s.With(llb.Dir(s2)) or s.Reset(s2, llb.DirMask)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of scope for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. You can comment on which one you prefer though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer s.With(llb.Dir(s2))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #1429, need a new solution for this.
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Improved the async test a bit. |
Adds new
llb.Async
method that can be used as a building block for adding callbacks to llb generation that can be resolved later. The DAG values are not evaluated untilMarshal()
orGet*()
methods are called on a graph. These methods also now take context as an argument as these are the actually blocking functions.Follow-up TODOs:
Refactor image meta resolver to async callbacks. This may also require a new API feature that would make sure that resolving image config and pulling image snapshots always resolve to the same image on the same build if they now run completely in parallel. Eg. Dockerfile frontend atm appends a digest to the image ref after resolving a config for this. Could still use this method as well but then pull could only start after the config has been downloaded. @hinsun ideas?
Refactor marshal method to allow returning partial input DAGs when the current node is blocked on a long-running job. Add a
Solve
method that can takellb.State
directly and input and efficiently marshal and subbuild in that case.Provide helpers for doing things like frontend builds and turning image config returned in frontend metadata back into a state.
Refactor dockerfile frontend to lazily evaluate and use
llb.State
for building up the final image configuration. Currentcontext.TODO()
hacks that I added to Dockerfile frontend should not actually be needed as the evaluation is not needed beforestate.Marshal()
is called. Added thecontext.TODO()
just to get the tests working before continuing.This should make the graph generation slower but not sure if it is meaningful. It could be easily optimized by adding memoized wrappers for cache but not sure where they would be most efficient without some benchmarking code. The old code is not super-optimized either in here, eg. could use something more efficient than a linked list for the values.
Signed-off-by: Tonis Tiigi tonistiigi@gmail.com