monitor: Enable to run build and invoke in background #1296
Conversation
| @@ -502,6 +743,9 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { | |||
|
|
|||
| if isExperimental() { | |||
| flags.StringVar(&options.invoke, "invoke", "", "Invoke a command after the build [experimental]") | |||
| flags.StringVar(&options.root, "root", "", "Specify buildx server root [experimental]") | |||
| flags.BoolVar(&options.debug, "debug", false, "Print debug logs [experimental]") | |||
| flags.BoolVar(&options.detach, "detach", runtime.GOOS == "linux", "Detach buildx server (supported only on linux) [experimental]") | |||
There was a problem hiding this comment.
What is the linux specific here? I assume macos shouldn't have much issues?
@crazy-max Maybe you can check windows viability?
There was a problem hiding this comment.
The location of buildx root dir and the logic to detach server (commands/launch_linux.go) currently assumes linux.
commands/build.go
Outdated
| return err | ||
| } | ||
|
|
||
| // connect to buidlx and get client |
commands/build.go
Outdated
| if err := os.MkdirAll(rootDir, 0700); err != nil { | ||
| return err | ||
| } | ||
| instanceRoot, err := os.MkdirTemp(rootDir, "buildx") |
There was a problem hiding this comment.
why is this called instanceRoot, doesn't look like it is instance specific?
There was a problem hiding this comment.
nvm, I think this is invocation instance, not builder instance. Bit confusing to use the same terms.
Yes, I agree with this. Both sound good to me though I'm currently taking the approch of server per build because it was easier to implement. |
FROM ghcr.io/stargz-containers/busybox:1.32.0-org
RUN echo hello > /hello$ BUILDX_EXPERIMENTAL=1 docker buildx build --invoke sh .
#1 [internal] load .dockerignore
#1 transferring context: 2B done
#1 DONE 0.0s
#2 [internal] load build definition from Dockerfile
#2 transferring dockerfile: 111B done
#2 DONE 0.0s
#3 [internal] load metadata for ghcr.io/stargz-containers/busybox:1.32.0-org
#3 DONE 0.2s
#4 [1/2] FROM ghcr.io/stargz-containers/busybox:1.32.0-org@sha256:bde48e1751173b709090c2539fdf12d6ba64e88ec7a4301591227ce925f3c678
#4 resolve ghcr.io/stargz-containers/busybox:1.32.0-org@sha256:bde48e1751173b709090c2539fdf12d6ba64e88ec7a4301591227ce925f3c678 0.0s done
#4 DONE 0.0s
#5 [2/2] RUN echo hello > /hello
#5 CACHED
Launching interactive container. Press Ctrl-a-c to switch to monitor console
/ #
But if I @ktock Let me know if you need more info |
|
@crazy-max Thank you for trying this.
|
|
Couple notes while experimenting 🎉 (awesome work btw)
Also having this.
We don't seem to do this - the server process seems to remain active indefinitely until kill-ed. I think we should probably aim to have the client send a
The error messages aren't super clear for this atm. If we disconnect a client, ideally the server should send some
I'd prefer something like this as well, though the split approach would keep crashes isolated to one build - a broken server taking out all the debugging sessions in process would definitely be a frustrating experience. If we do go shared, I think we should go minimal to start, and simply manage the server by starting it if it's not already running, otherwise, connecting to the existing instance. If we detect a version mismatch, we should restart the server automatically after asking the user to disconnect all their debugging sessions. We could look into managing it with systemd/etc as an advanced use case later, but I think out-of-the-box experience should be low-friction (e.g. buildx can run inside a container, where systemd might not be available).
I think this depends - if we want to allow multiple debugging clients for a single server, and those should both be able to open shells, then they need to be separate shells (or stdin/stdout/stderr will get messy), so we'd always restart the container. If we're ok with only a single shell at the same time, I think it would be nice to switch over without restarting. |
|
Thank you for the review. Updated the patch based on the comments.
Fixed this.
Fixed to share one server among builds.
Added.
Fixed to print a warning on version mismatch. FROM ghcr.io/stargz-containers/busybox:1.32.0-org
RUN echo hello > /hellobuildx build
$ BUILDX_EXPERIMENTAL=1 /tmp/out/buildx build --invoke sh /tmp/ctx
INFO: no buildx server found; launching...
[+] Building 0.4s (5/5) FINISHED
=> [internal] load .dockerignore 0.0s
=> => transferring context: 2B 0.0s
=> [internal] load build definition from Dockerfile 0.0s
=> => transferring dockerfile: 111B 0.0s
=> [internal] load metadata for ghcr.io/stargz-containers/busybox:1.32.0-org 0.4s
=> [1/2] FROM ghcr.io/stargz-containers/busybox:1.32.0-org@sha256:bde48e1751173b709090c2539fdf12d6ba64e88ec7a4 0.0s
=> => resolve ghcr.io/stargz-containers/busybox:1.32.0-org@sha256:bde48e1751173b709090c2539fdf12d6ba64e88ec7a4 0.0s
=> CACHED [2/2] RUN echo hello > /hello 0.0s
Launching interactive container. Press Ctrl-a-c to switch to monitor console
/ # listattachdisconnectCtrl-D disconnects the session. From another terminal, we can see that
version mismatch
|
hack/update-generated-files
Outdated
| #!/usr/bin/env bash | ||
| # Forked from: https://github.com/moby/buildkit/blob/v0.10.4/hack/update-generated-files | ||
| # Copyright The BuildKit Authors. | ||
| # Copyright The Buildx Authors. | ||
| # Licensed under the Apache License, Version 2.0 |
There was a problem hiding this comment.
cc @crazy-max do you have any ideas of how we could do this so that we don't need to copy this over?
There was a problem hiding this comment.
I think it's fine for now but in a follow-up, we could just build using the hack/dockerfiles/generated-files.Dockerfile from BuildKit repo that would be fetched using the BuildKit reference from go.mod so we would not need an extra dockerfile.
I will also take a look at cherry picking moby/buildkit@3fd0785 from moby/buildkit#3204 so we use the go tools build constraint and avoid hardcoded versions in the Dockerfile and fully relies on ones from go.mod for protobuf/gogo tooling.
There was a problem hiding this comment.
moby/buildkit#3236 is merged now. Should be easier with https://github.com/moby/buildkit/blob/master/hack/update-generated-files
As we are using bake in buildx repo it would look like this:
docs/reference/buildx_build.md
Outdated
| | [`-f`](https://docs.docker.com/engine/reference/commandline/build/#specify-a-dockerfile--f), [`--file`](https://docs.docker.com/engine/reference/commandline/build/#specify-a-dockerfile--f) | `string` | | Name of the Dockerfile (default: `PATH/Dockerfile`) | | ||
| | `--iidfile` | `string` | | Write the image ID to the file | | ||
| | `--invoke` | `string` | | Invoke a command after the build [experimental] | | ||
| | `--label` | `stringArray` | | Set metadata for an image | | ||
| | [`--load`](#load) | | | Shorthand for `--output=type=docker` | | ||
| | `--log-file` | `string` | | Specify file to output buildx server log [experimental] | |
There was a problem hiding this comment.
Not sure about the command line args, I think we should maybe group "client/server" control together somehow, or maybe we should have a config file?
--debug also feels like something we might want to keep for the debugging functionality, instead of for increasing logging verbosity.
There was a problem hiding this comment.
Introduced a config file for the server.
commands/build.go
Outdated
| } | ||
| go wait() | ||
| return nil | ||
| case controlModeServe: |
There was a problem hiding this comment.
If we can, I think it makes sense to actually have a separate internal sub-command for this, instead of trying to distinguish by env var - only the client-side parts should need to be in this function.
There was a problem hiding this comment.
Fixed to use hidden subcommands.
commands/build.go
Outdated
| @@ -434,7 +456,263 @@ func buildCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { | |||
| options.contextPath = args[0] | |||
| options.builder = rootOpts.builder | |||
| cmd.Flags().VisitAll(checkWarnedFlags) | |||
| return runBuild(dockerCli, options) | |||
| if !isExperimental() || !options.detach { | |||
There was a problem hiding this comment.
I'm not sure I understand this condition 😄
If --detach is set, then BUILDX_EXPERIMENTAL must be enabled, since --detach is an experimental option. I assume it's explicitly to run the build on the server, but I think we should always default to that in experimental mode - so I'm not sure if we need to keep the detach option?
There was a problem hiding this comment.
Currently --detach is supported only on linux. So could we preserve --detach=false option for non-linux platform?
|
I like the refactor to use a single server 🎉 Left a couple high-level comments, got a few notes I found while playing:
I'm happy if we circle back and address these later, just wanted to list them out so I don't forget! |
ktock
force-pushed
the
monitor-list
branch
2 times, most recently
from
a88b3f5 to
9e534bc
Compare
|
@jedevc Thank you for the review.
Thank you for the suggestion. Could we address them in following-up patches as this PR is already very big? |
commands/build.go
Outdated
| if in.pull != nil { | ||
| pull = *in.pull | ||
| } | ||
| _, err = runBuildContext(ctx, dockerCli, in.BuildOptions, os.Stdin, in.progress, in.invoke != "", nil) |
There was a problem hiding this comment.
If my reading of the code here is right, we only support --invoke when going through the runBuildOnBuilder path? That seems fine to me, especially as it looks we'll make this the default eventually?
If so, I think we might be able to remove the allowNoOutput flag at this point.
There was a problem hiding this comment.
Thank you for the review. Fixed.
commands/builderremote_linux.go
Outdated
| ) | ||
| } | ||
|
|
||
| func launchCmd(dockerCli command.Cli, rootOpts *rootOptions) *cobra.Command { |
There was a problem hiding this comment.
Why do we need both launchCmd and serveCmd? It seems like launch just calls serve, but I might be missing something as to why it's necessary.
There was a problem hiding this comment.
This was for immeditely detaching buildx server processs from the buildx (client) process and reparenting it to the init process. But it's not strictly necessary because reparenting happens after buildx (client) exits.
Fixed to directly exec buildx server.
|
Hey @ktock sorry it's taking so long to review 🎉 I'm (personally) happy with the design, and would be happy to approve soon. I think the design is good atm, the interface feels quite natural, but there is some complexity there we should address in a follow-up (IMO). My comments are mostly about making the code clear so I'd feel comfortable working on it in the future myself 😄 A couple notes:
Some things for follow-ups:
|
|
@jedevc Thank you for your comments.
Added godoc to
Tried to make it clearer by renaming some of them as the following.
Please feel free to apply more changes (maybe in following-up PRs) if you come up with the better names.
Renamed
Let's work on it in following up PRs. |
commands/controllerremote.go
Outdated
| func rootDataDir() (string, error) { | ||
| if xdh := os.Getenv("XDG_DATA_HOME"); xdh != "" { | ||
| return filepath.Join(xdh, "buildx"), nil | ||
| } | ||
| home := os.Getenv("HOME") | ||
| if home == "" { | ||
| return "", fmt.Errorf("environment variable HOME is not set") | ||
| } | ||
| return filepath.Join(home, ".local/share/buildx"), nil | ||
| } |
There was a problem hiding this comment.
I wonder if we should not just use the current buildx config dir:
buildx/util/confutil/config.go
Lines 17 to 26 in b242e32
And create a controller folder in it?
There was a problem hiding this comment.
Thanks for the review. Fixed.
| @@ -0,0 +1,53 @@ | |||
| # Forked from https://github.com/moby/buildkit/blob/v0.10.4/hack/dockerfiles/generated-files.Dockerfile | |||
There was a problem hiding this comment.
This has changed a bit, see moby/buildkit#3236
I think we can do the same here.
There was a problem hiding this comment.
Thanks for the suggestion. Updated this file.
hack/update-generated-files
Outdated
| #!/usr/bin/env bash | ||
| # Forked from: https://github.com/moby/buildkit/blob/v0.10.4/hack/update-generated-files | ||
| # Copyright The BuildKit Authors. | ||
| # Copyright The Buildx Authors. | ||
| # Licensed under the Apache License, Version 2.0 |
There was a problem hiding this comment.
moby/buildkit#3236 is merged now. Should be easier with https://github.com/moby/buildkit/blob/master/hack/update-generated-files
As we are using bake in buildx repo it would look like this:
hack/validate-generated-files
Outdated
| @@ -0,0 +1,35 @@ | |||
| #!/usr/bin/env bash | |||
| # Forked from: https://github.com/moby/buildkit/blob/v0.10.4/hack/validate-generated-files | |||
There was a problem hiding this comment.
ktock
force-pushed
the
monitor-list
branch
2 times, most recently
from
a65c4b2 to
59e2ca0
Compare
Signed-off-by: Kohei Tokunaga <[email protected]>
|
Heya @ktock sorry this took so long 😢 Hopefully we can open up some bandwidth now to get some follow-ups done ❤️ |
|
@jedevc I'm sorry for my slow update, and thank you for your time for reviewing this patch! |
|
Aha, so, I just found something right off the bat 😢 Any ideas as to why we're no longer able to find the |
|
Ah the detached server has a different working directory to the client - so I think we need to do something special to get the paths to resolve correctly? |
|
@jedevc Sorry for that bug. Maybe we need something like the following(not tested). Can I open the following-up PR? func resolveAbsolutePaths(options *controllerapi.BuildOptions) (_ *controllerapi.BuildOptions, err error) {
options.ContextPath, err = absIfLocal(options.ContextPath)
if err != nil {
return nil, err
}
// ... Do the same for DockerfileName, etc.
return options, nil
}
func absIfLocal(p string) (string, error) {
if _, err := os.Stat(p); err == nil {
return filepath.Abs(p)
}
return p, nil
}
|
|
Yeah sure! Go for it 🎉 |
#1104
This commit enables to launch builds in a background process and adds some related commands.
Please give me feedbacks on the design of the detaching and the CLI.
This covers PR4 and PR5 of #1104.
Example:
BUILDX_EXPERIMENTAL=1is required to enable detached buildx.buildx build
The above command launches two buildx processes: client and server.
The server is detached in a separated session.
The detached buildx server actually runs the build and the foreground client buildx forwards I/O and provides monitor prompt.
ps command on the host:
The detached server lives even after the client exits.
list
listlists available server instances.USINGwill betureif the current monitor connects to that instance.attach
attachattaches to the specified instance.Monitor commands like
reloadandrollbackcan be issued to the newly connected instance.kill
killkills the specified instance.If no argument is provided, it kills the connecting instance.
How it works
buildx buildforks and detaches itself as a server daemon. After launching the detached server, the foreground one continues running as a client.Each buildx server instance provides gRPC API via socket (e.g.
~/.local/share/buildx/<instance-name>/buildx.sock) for serving build and invoke functionalities. buildx client provides monitor prompt and forwards I/O. It issues builds and invokes to the server everytime the user requests on the monitor.I/O for the build (e.g. Dockerfile via stdin) and container (e.g. stdio) are forwarded via gRPC API similarly done by BuildKit's
NewContainergateway API.Please note that the server doesn't support multiple clients as of now. Every time the user runs
buildx build, it launches a new server process with a newly created socket. If a client requests a build to the server while another client's build is ongoing, the old one is cancelled and the new one starts. In the future, the server can support parallel multiple builds and invokes. Maybe we need some additional refactorings for thebuildx buildimplementation for supporting parallel calls.TODOs
Considerations
attachcommand internally runsrollbackcommand for detaching the old client and attaching to the new client. Should we allow this without restarting the container?TODOs