From 6a0f5610e331ce846d865dbf4a9cab52e27e9cbe Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Fri, 30 May 2025 14:15:56 -0500 Subject: [PATCH] controller: remove the controller interface The controller interface is removed and the local controller is used for only the initial build, invoke, and rebuilds. Process control has been moved to the monitor. Signed-off-by: Jonathan A. Sternberg --- commands/build.go | 44 ++++++++---------------- commands/debug/root.go | 11 ++---- controller/control/controller.go | 26 --------------- controller/controller.go | 13 -------- controller/local/controller.go | 41 +++++++---------------- monitor/monitor.go | 57 +++++++++++++++++++++++++------- monitor/types/types.go | 23 +++++++++++-- 7 files changed, 94 insertions(+), 121 deletions(-) delete mode 100644 controller/control/controller.go delete mode 100644 controller/controller.go diff --git a/commands/build.go b/commands/build.go index 7c9027584..60fbb4272 100644 --- a/commands/build.go +++ b/commands/build.go @@ -21,10 +21,9 @@ import ( "github.com/docker/buildx/build" "github.com/docker/buildx/builder" "github.com/docker/buildx/commands/debug" - "github.com/docker/buildx/controller" cbuild "github.com/docker/buildx/controller/build" - "github.com/docker/buildx/controller/control" controllererrors "github.com/docker/buildx/controller/errdefs" + "github.com/docker/buildx/controller/local" controllerapi "github.com/docker/buildx/controller/pb" "github.com/docker/buildx/monitor" "github.com/docker/buildx/store" @@ -431,28 +430,19 @@ func runControllerBuild(ctx context.Context, dockerCli command.Cli, opts *cbuild // stdin must be usable for monitor return nil, nil, errors.Errorf("Dockerfile or context from stdin is not supported with invoke") } - c := controller.NewController(ctx, dockerCli) - defer func() { - if err := c.Close(); err != nil { - logrus.Warnf("failed to close server connection %v", err) - } - }() - // NOTE: buildx server has the current working directory different from the client - // so we need to resolve paths to abosolute ones in the client. - opts, err := cbuild.ResolveOptionPaths(opts) - if err != nil { - return nil, nil, err - } + c := local.NewController(ctx, dockerCli) + defer c.Close() - var ref string - var retErr error - var resp *client.SolveResponse - var inputs *build.Inputs + var ( + ref string + retErr error + + f *ioset.SingleForwarder + pr io.ReadCloser + pw io.WriteCloser + ) - var f *ioset.SingleForwarder - var pr io.ReadCloser - var pw io.WriteCloser if options.invokeConfig == nil { pr = dockerCli.In() } else { @@ -466,7 +456,7 @@ func runControllerBuild(ctx context.Context, dockerCli command.Cli, opts *cbuild }) } - resp, inputs, err = c.Build(ctx, opts, pr, printer) + resp, inputs, err := c.Build(ctx, opts, pr, printer) if err != nil { var be *controllererrors.BuildError if errors.As(err, &be) { @@ -508,10 +498,6 @@ func runControllerBuild(ctx context.Context, dockerCli command.Cli, opts *cbuild // Update return values with the last build result from monitor resp, retErr = monitorBuildResult.Resp, monitorBuildResult.Err } - } else { - if err := c.Close(); err != nil { - logrus.Warnf("close error: %v", err) - } } return resp, inputs, retErr @@ -1003,13 +989,9 @@ func (cfg *invokeConfig) needsDebug(retErr error) bool { } } -func (cfg *invokeConfig) runDebug(ctx context.Context, ref string, options *cbuild.Options, c control.BuildxController, stdin io.ReadCloser, stdout io.WriteCloser, stderr console.File, progress *progress.Printer) (*monitor.MonitorBuildResult, error) { +func (cfg *invokeConfig) runDebug(ctx context.Context, ref string, options *cbuild.Options, c *local.Controller, stdin io.ReadCloser, stdout io.WriteCloser, stderr console.File, progress *progress.Printer) (*monitor.MonitorBuildResult, error) { con := console.Current() if err := con.SetRaw(); err != nil { - // TODO: run disconnect in build command (on error case) - if err := c.Close(); err != nil { - logrus.Warnf("close error: %v", err) - } return nil, errors.Errorf("failed to configure terminal: %v", err) } defer con.Reset() diff --git a/commands/debug/root.go b/commands/debug/root.go index f63bde3ac..9d0d0bb4f 100644 --- a/commands/debug/root.go +++ b/commands/debug/root.go @@ -5,7 +5,7 @@ import ( "os" "github.com/containerd/console" - "github.com/docker/buildx/controller" + "github.com/docker/buildx/controller/local" controllerapi "github.com/docker/buildx/controller/pb" "github.com/docker/buildx/monitor" "github.com/docker/buildx/util/cobrautil" @@ -13,7 +13,6 @@ import ( "github.com/docker/cli/cli/command" "github.com/moby/buildkit/util/progress/progressui" "github.com/pkg/errors" - "github.com/sirupsen/logrus" "github.com/spf13/cobra" ) @@ -47,12 +46,8 @@ func RootCmd(dockerCli command.Cli, children ...DebuggableCmd) *cobra.Command { } ctx := context.TODO() - c := controller.NewController(ctx, dockerCli) - defer func() { - if err := c.Close(); err != nil { - logrus.Warnf("failed to close server connection %v", err) - } - }() + c := local.NewController(ctx, dockerCli) + con := console.Current() if err := con.SetRaw(); err != nil { return errors.Errorf("failed to configure terminal: %v", err) diff --git a/controller/control/controller.go b/controller/control/controller.go deleted file mode 100644 index cc501e9bb..000000000 --- a/controller/control/controller.go +++ /dev/null @@ -1,26 +0,0 @@ -package control - -import ( - "context" - "io" - - "github.com/docker/buildx/build" - cbuild "github.com/docker/buildx/controller/build" - controllerapi "github.com/docker/buildx/controller/pb" - "github.com/docker/buildx/controller/processes" - "github.com/docker/buildx/util/progress" - "github.com/moby/buildkit/client" -) - -type BuildxController interface { - Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, inputs *build.Inputs, err error) - // Invoke starts an IO session into the specified process. - // If pid doesn't match to any running processes, it starts a new process with the specified config. - // If there is no container running or InvokeConfig.Rollback is specified, the process will start in a newly created container. - // NOTE: If needed, in the future, we can split this API into three APIs (NewContainer, NewProcess and Attach). - Invoke(ctx context.Context, pid string, options *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error - Close() error - ListProcesses(ctx context.Context) (infos []*processes.ProcessInfo, retErr error) - DisconnectProcess(ctx context.Context, pid string) error - Inspect(ctx context.Context) *cbuild.Options -} diff --git a/controller/controller.go b/controller/controller.go deleted file mode 100644 index dfa0f47d5..000000000 --- a/controller/controller.go +++ /dev/null @@ -1,13 +0,0 @@ -package controller - -import ( - "context" - - "github.com/docker/buildx/controller/control" - "github.com/docker/buildx/controller/local" - "github.com/docker/cli/cli/command" -) - -func NewController(ctx context.Context, dockerCli command.Cli) control.BuildxController { - return local.NewLocalBuildxController(ctx, dockerCli) -} diff --git a/controller/local/controller.go b/controller/local/controller.go index 174a002c7..671c4ba0e 100644 --- a/controller/local/controller.go +++ b/controller/local/controller.go @@ -7,7 +7,6 @@ import ( "github.com/docker/buildx/build" cbuild "github.com/docker/buildx/controller/build" - "github.com/docker/buildx/controller/control" controllererrors "github.com/docker/buildx/controller/errdefs" controllerapi "github.com/docker/buildx/controller/pb" "github.com/docker/buildx/controller/processes" @@ -18,10 +17,9 @@ import ( "github.com/pkg/errors" ) -func NewLocalBuildxController(ctx context.Context, dockerCli command.Cli) control.BuildxController { - return &localController{ +func NewController(ctx context.Context, dockerCli command.Cli) *Controller { + return &Controller{ dockerCli: dockerCli, - processes: processes.NewManager(), } } @@ -32,15 +30,14 @@ type buildConfig struct { buildOptions *cbuild.Options } -type localController struct { +type Controller struct { dockerCli command.Cli buildConfig buildConfig - processes *processes.Manager buildOnGoing atomic.Bool } -func (b *localController) Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (*client.SolveResponse, *build.Inputs, error) { +func (b *Controller) Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (*client.SolveResponse, *build.Inputs, error) { if !b.buildOnGoing.CompareAndSwap(false, true) { return nil, nil, errors.New("build ongoing") } @@ -63,27 +60,15 @@ func (b *localController) Build(ctx context.Context, options *cbuild.Options, in return resp, dockerfileMappings, nil } -func (b *localController) ListProcesses(ctx context.Context) (infos []*processes.ProcessInfo, retErr error) { - return b.processes.ListProcesses(), nil -} - -func (b *localController) DisconnectProcess(ctx context.Context, pid string) error { - return b.processes.DeleteProcess(pid) -} - -func (b *localController) cancelRunningProcesses() { - b.processes.CancelRunningProcesses() -} - -func (b *localController) Invoke(ctx context.Context, pid string, cfg *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error { - proc, ok := b.processes.Get(pid) +func (b *Controller) Invoke(ctx context.Context, processes *processes.Manager, pid string, cfg *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error { + proc, ok := processes.Get(pid) if !ok { // Start a new process. if b.buildConfig.resultCtx == nil { return errors.New("no build result is registered") } var err error - proc, err = b.processes.StartProcess(pid, b.buildConfig.resultCtx, cfg) + proc, err = processes.StartProcess(pid, b.buildConfig.resultCtx, cfg) if err != nil { return err } @@ -103,15 +88,13 @@ func (b *localController) Invoke(ctx context.Context, pid string, cfg *controlle } } -func (b *localController) Close() error { - b.cancelRunningProcesses() +func (b *Controller) Inspect(ctx context.Context) *cbuild.Options { + return b.buildConfig.buildOptions +} + +func (b *Controller) Close() error { if b.buildConfig.resultCtx != nil { b.buildConfig.resultCtx.Done() } - // TODO: cancel ongoing builds? return nil } - -func (b *localController) Inspect(ctx context.Context) *cbuild.Options { - return b.buildConfig.buildOptions -} diff --git a/monitor/monitor.go b/monitor/monitor.go index 750f44393..6156dc834 100644 --- a/monitor/monitor.go +++ b/monitor/monitor.go @@ -12,8 +12,9 @@ import ( "github.com/containerd/console" "github.com/docker/buildx/build" cbuild "github.com/docker/buildx/controller/build" - "github.com/docker/buildx/controller/control" + "github.com/docker/buildx/controller/local" controllerapi "github.com/docker/buildx/controller/pb" + "github.com/docker/buildx/controller/processes" "github.com/docker/buildx/monitor/commands" "github.com/docker/buildx/monitor/types" "github.com/docker/buildx/util/ioset" @@ -32,13 +33,7 @@ type MonitorBuildResult struct { } // RunMonitor provides an interactive session for running and managing containers via specified IO. -func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, invokeConfig *controllerapi.InvokeConfig, c control.BuildxController, stdin io.ReadCloser, stdout io.WriteCloser, stderr console.File, progress *progress.Printer) (*MonitorBuildResult, error) { - defer func() { - if err := c.Close(); err != nil { - logrus.Warnf("close error: %v", err) - } - }() - +func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, invokeConfig *controllerapi.InvokeConfig, c *local.Controller, stdin io.ReadCloser, stdout io.WriteCloser, stderr console.File, progress *progress.Printer) (*MonitorBuildResult, error) { if err := progress.Pause(); err != nil { return nil, err } @@ -70,8 +65,9 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv invokeForwarder := ioset.NewForwarder() invokeForwarder.SetIn(&containerIn) m := &monitor{ - BuildxController: c, - invokeIO: invokeForwarder, + c: c, + processes: processes.NewManager(), + invokeIO: invokeForwarder, muxIO: ioset.NewMuxIO(ioset.In{ Stdin: io.NopCloser(stdin), Stdout: nopCloser{stdout}, @@ -84,6 +80,12 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv return "Switched IO\n" }), } + + defer func() { + if err := m.Close(); err != nil { + logrus.Warnf("close error: %v", err) + } + }() m.ref.Store(curRef) // Start container automatically @@ -231,7 +233,7 @@ type readWriter struct { } type monitor struct { - control.BuildxController + c *local.Controller ref atomic.Value muxIO *ioset.MuxIO @@ -240,14 +242,24 @@ type monitor struct { attachedPid atomic.Value lastBuildResult *MonitorBuildResult + + processes *processes.Manager } func (m *monitor) Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, input *build.Inputs, err error) { - resp, _, err = m.BuildxController.Build(ctx, options, in, progress) + resp, _, err = m.c.Build(ctx, options, in, progress) m.lastBuildResult = &MonitorBuildResult{Resp: resp, Err: err} // Record build result return } +func (m *monitor) Invoke(ctx context.Context, pid string, cfg *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error { + return m.c.Invoke(ctx, m.processes, pid, cfg, ioIn, ioOut, ioErr) +} + +func (m *monitor) Inspect(ctx context.Context) *cbuild.Options { + return m.c.Inspect(ctx) +} + func (m *monitor) Rollback(ctx context.Context, cfg *controllerapi.InvokeConfig) string { pid := identity.NewID() cfg1 := cfg @@ -332,6 +344,27 @@ func (m *monitor) invoke(ctx context.Context, pid string, cfg *controllerapi.Inv return err } +func (m *monitor) Close() error { + m.cancelRunningProcesses() + // if m.buildConfig.resultCtx != nil { + // b.buildConfig.resultCtx.Done() + // } + // TODO: cancel ongoing builds? + return nil +} + +func (m *monitor) ListProcesses(ctx context.Context) (infos []*processes.ProcessInfo, retErr error) { + return m.processes.ListProcesses(), nil +} + +func (m *monitor) DisconnectProcess(ctx context.Context, pid string) error { + return m.processes.DeleteProcess(pid) +} + +func (m *monitor) cancelRunningProcesses() { + m.processes.CancelRunningProcesses() +} + type nopCloser struct { io.Writer } diff --git a/monitor/types/types.go b/monitor/types/types.go index 201f273ac..da837f32c 100644 --- a/monitor/types/types.go +++ b/monitor/types/types.go @@ -2,14 +2,31 @@ package types import ( "context" + "io" - "github.com/docker/buildx/controller/control" + "github.com/docker/buildx/build" + cbuild "github.com/docker/buildx/controller/build" controllerapi "github.com/docker/buildx/controller/pb" + "github.com/docker/buildx/controller/processes" + "github.com/docker/buildx/util/progress" + "github.com/moby/buildkit/client" ) // Monitor provides APIs for attaching and controlling the buildx server. type Monitor interface { - control.BuildxController + Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, inputs *build.Inputs, err error) + + Inspect(ctx context.Context) *cbuild.Options + + // Invoke starts an IO session into the specified process. + // If pid doesn't match to any running processes, it starts a new process with the specified config. + // If there is no container running or InvokeConfig.Rollback is specified, the process will start in a newly created container. + // NOTE: If needed, in the future, we can split this API into three APIs (NewContainer, NewProcess and Attach). + Invoke(ctx context.Context, pid string, options *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error + + ListProcesses(ctx context.Context) (infos []*processes.ProcessInfo, retErr error) + + DisconnectProcess(ctx context.Context, pid string) error // Rollback re-runs the interactive container with initial rootfs contents. Rollback(ctx context.Context, cfg *controllerapi.InvokeConfig) string @@ -25,6 +42,8 @@ type Monitor interface { // Detach detaches IO from the container. Detach() + + io.Closer } // CommandInfo is information about a command.