Proposal: Remove build controller

[jsternberg]

The build controller was created as a way to abstract away the command line interface with the idea it would be useful for creating a remote version of buildx that could be persisted and used for debugging. The CLI would create a background buildx process and would communicate through it.

The reality of the implementation is that it was never completed and currently just lives hidden behind the BUILDX_EXPERIMENTAL tag. It introduces additional complexity in the CLI code paths that are difficult to follow and have subtle differences that result in bugs when BUILDX_EXPERIMENTAL is used. There are currently no plans to finish the implementation and we're not really sure if this is the best way to complete the work on debuggable build environments.

We'd like to move the debugging builds conversation to creating a DAP interface for builds so we are only building the debugger and are not also building a frontend for the debugger. This alternative method would give us compatibility with a wider array of common editors: particularly vscode and neovim which have either DAP support builtin or prominent dap plugins.

At this point, the build controller was a useful experiment but we think it would not be worth the time and effort to finish the implementation.

[thompson-shaun]

cc @ktock @jedevc (might be of interest)

[tonistiigi]

My best attempt to map current logic after #3164 + dap PR #1656

flowchart TD
    %% Root Level
    A1[commands/build.runBuild]
    A2[commands/bake.runBake]
    A3[commands/dap.runE]

    %% Build.runBuild Branch
    A1 --> B1[commands/build.runControllerBuild]
    B1 --> C1[controller/control.BuildxController.Build]
    C1 --> D1[controller/build.RunBuild]
    D1 --> E1[controller/build.buildTargets]
    E1 --> F1[build.BuildWithResultHandler]
    E1 --> F2[build.Build]

    B1 --> B2[runDebug]
    B2 --> G1[monitor.RunMonitor_ref_controller]
    G1 --> C1
    G1 --> G2[controller/control.BuildxController.Invoke]
    G2 --> H1[startProcess]
    H1 --> I1[build.NewContainer]

    A1 --> B3[command/build.runBasicBuild]
    B3 --> D1

    %% Bake.runBake Branch
    A2 --> J1[bake.TargetsToBuildOpt]
    J1 --> F2

    %% Dap.runE Branch
    A3 --> K1[dap.NewServer]
    K1 --> L1[launchDebugger]
    L1 --> N1[debug=on]
    L1 --> N2[debug=off]
    N1 --> C1
    N2 --> C1

Loading

The next steps can start from removing more of these unnecessary controller layers and rebasing DAP PR to the current master. Bake is atm the only clean path that doesn't go through these controller layers.

For the end state, we should have a version that uses the same code path for all 4 use cases (build, bake, build+invoke loop, dap debugger loop) without big exceptions. Ideally, it would also be easy to use as an SDK library in 3rd party tools (although other languages should just use the bake definition as input protocol).

To summarize my understanding of the current logic:

• everything ends up with build.Build eventually
• dap is based on passing debug bool to build, if set it only evaluates target to LLB definition and not to build result
• invoke and dap are based on BuildWithResultHandler where a struct is returned with quite arbitrary options that allow further work on the result of the build and calling NewContainer etc.

I generally think the definition for build.Build() is correct. We could separately refactor its implementation to reduce cyclomatic complexity, but I think the inputs are correct.

• ctx
• nodes (driver definition behind interface)
• map[string]Definition the build definition for all the targets. targets can be chained into pipelines
• dockerCli - to export to Docker
• dockerAuth - registry authentication
• printer - where progress is written

I don't think it would make sense to split up nodes or pass targets one-by-one because there is a lot of complexity in choosing correct builder node and invoking internal build targets in the correct order that would be painful to do from the caller side.

Now the question is what is the best way to handle the "invoke" and "dap" cases where builds are issued repeatedly, also printer mode is switched and other buildkit functionality like NewContainer or build from LLB can be used. Should every iteration of that loop call build.Build again or should it all be handled as part of a single build.Build lifecycle. Note also that bake does not currently support invoke+dap but I think it should. It's ok to think of commands.runBuild just putting together a single bake target in the future. Because of that, I think it might be better to use single build.Build call for one buildx command invocation. In order to create these debug loops this would need some optional "hooks" passed to build.Build and something similar to ResultHandler but hopefully bit better defined.

Use cases:

• Normal builds (no hooks needed) build targets to final result, export it and return metadata.
• DAP builds build targets to LLB definition. Caller can make additional requests with this definition that are scoped to same node that the specific target was built on. After it is done, it can return an LLB that is evaluated into a result.
• Invoke builds build targets to result but don't exit with metadata. Caller can make additional requests with this result that are scoped to same node that the specific target was built on. After it is done, it can return result that exits with metadata.
• Any time target errors, caller can parse the typed error and make additional requests. In the end deciding what the next command should be.

This means following hooks:

• OnLLBDefinition(target, def, builder) next - invoked when LLB has been evaluated. builder is interface for additional requests.
• OnResult(target, resp, builder) next - invoked when there is result
• OnError(target, err, builder) next

When targets are chained, then the internal build.Build behavior waits before the dependent target reaches the required state, and completes the hooks that need to run before the child target can start building.

Next command can be:

• Error - give up and return error from build.Build()
• Build - build LLB into a result
• Result - export ref and metadata
• Promise - instead of returning next command directly, pause processing on that target (eg. to run NewContainer, or walk LLB graph for DAP) and return next command later after this task is finished. (this may be just done with blocking a goroutine rather than real command type).

builder passed to the hooks is interface with the capability to evaluate LLB and launch BuildKit NewContainer APIs for interactive containers. After you return next command from the hook, the lifecycle of builder object is complete.

The Printer object would be automatically connected to build.Build. When a hook wants to run the interactive container, and then do more builds later it would use something like printer.Acquire() (release func()) that would wait before the current progress writers have completed. Then hook can write to IO, and after it releases printer the progress from build.Build can continue. Alternatively this switching between progress, interactive containers and rest of TUI could be builtin to the printer object. I think the printer changes can be done separately.

[jsternberg]

I've been diving into this and I think we can potentially make things more simple for the different use cases. Right now, we have 3 primary use cases.

1. Normal execution.
2. Execute until a result is produced or an error. Then allow an interactive container to be created.
3. Debug adapter protocol.

I think all of these can be implemented by just customizing how Solve is computed and I'd say that we can get this functionality by providing a way to customize how Solve is invoked. Specifically, this definition:

func Solve(ctx context.Context, c gateway.Client, req gateway.SolveRequest) (*gateway.Result, error)

In the code, this signature is invoked here.

For the number 1 case, we just invoke it as-is and continue.

For the number 2 case, I think this is just a more specific iteration of the DAP protocol. I think this is the equivalent of invoking with the debug adapter protocol and placing an exception breakpoint for when an error happens or a breakpoint when we're about to return the final result. This can be modeled with DAP events and then we can utilize the evaluate request for controlling the resulting image.

For the number 3 case, I think we do this through overwriting the Solve method. Instead of calling Solve directly and returning immediately, we invoke Solve to return the result and retrieve the definition from it. We then take the definition returned from that, split it into the different walking steps, and do a solve on each step with evaluate to make sure that errors are returned. When we are finished, we return the completed result.

For DAP, Solve would interact with the DAP server that's created before the build is started and its behavior would be customized by using setBreakpoints and setExceptionBreakpoints. For each solve, we do the following.

1. Run Solve to get the gateway.Result.
2. Retrieve the definition from the gateway.Result using ToState.
3. Split the state into separate invocations to create a set of "steps".
4. Run Solve with evaluate set to true for each "step".
5. If an error happens and setExceptionBreakpoints was configured to break on error, then pause, send the "stopped", and keep the current state.
6. On exit, if a breakpoint was set for getting the result, send the "stopped" event to give a chance to use the final result.
7. Return the result.

A key part of this is the control of Solve is done through communication with DAP which is configured before the solve. Rather than return from Solve and the build function immediately, we would send an event to DAP, pause our own execution of the build, and not return from the build function. On an error, DAP could send the terminate request to end the debugging session (which would send the error back), it could send the restart request to restart the build, it could send requests to ascertain the current state of the build, or it could send an evaluate request to launch and interact with the input container.

I think with this method we could also just remove ResultHandle. I can try to prototype something to show my intentions.

[tonistiigi]

For the number 2 case,
I think this is the equivalent of invoking with the debug adapter protocol and placing an exception breakpoint for when an error happens or a breakpoint when we're about to return the final result.

DAP protocol is something started from a client that understands DAP (eg. vscode, vim etc). That's not what is happening with build --on=error (that would probably become default) or other invoke cases. Also build package should know nothing about DAP (I don't think it does in your description but just for clarity).

If I imagine your case to be similar with mine except for there being only one hook, I think example case that can't be done is if I have chained targets in bake and also want to call invoke either on the result or in error. The logic for chaining targets is implemented in build package and happens after initial LLB resolve. So you need 1) solve to LLB 2) initialize context for child targets 3) evaluate 4) invoke+release child. If your single hook gets called after solve to LLB then you can't use the same hook to do invoke as well later after the result has been built. Also you can't detect errors from multiple stages.

Run Solve to get the gateway.Result.

Idk which Solve you mean in here. I assume this is the hook?

I think with this method we could also just remove ResultHandle.

There is no ResultHandle in my variant as well. Only Build() with parameters I listed + optional hooks. Each hook has access to (subset of) gateway.Client to access Solve() (needed for DAP LLB walking as you described) and NewContainer (needed for invoke).