Skip to content

Commit bb5b5e3

Browse files
tonistiigitonistiigi
authored
Merge pull request #3219 from jsternberg/monitor-reload-refactor
monitor: refactor how reload works
2 parents d61853b + 21ebf82 commit bb5b5e3

File tree

7 files changed

+107
-143
lines changed

7 files changed

+107
-143
lines changed

‎commands/build.go‎

Lines changed: 58 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -431,57 +431,75 @@ func runControllerBuild(ctx context.Context, dockerCli command.Cli, opts *cbuild
431431
return nil, nil, errors.Errorf("Dockerfile or context from stdin is not supported with invoke")
432432
}
433433

434-
c := local.NewController(ctx, dockerCli)
435-
defer c.Close()
436-
437-
var in io.ReadCloser
434+
var (
435+
in io.ReadCloser
436+
f *ioset.SingleForwarder
437+
)
438438
if options.invokeConfig == nil {
439439
in = dockerCli.In()
440+
} else {
441+
f = ioset.NewSingleForwarder()
442+
f.SetReader(dockerCli.In())
440443
}
441444

442-
resp, inputs, err := c.Build(ctx, opts, in, printer)
443-
if err != nil {
444-
var be *controllererrors.BuildError
445-
if errors.As(err, &be) {
446-
retErr = err
447-
// We can proceed to monitor
448-
} else {
449-
return nil, nil, errors.Wrapf(err, "failed to build")
445+
for {
446+
c := local.NewController(ctx, dockerCli)
447+
448+
resp, inputs, err := c.Build(ctx, opts, in, printer)
449+
if err != nil {
450+
var be *controllererrors.BuildError
451+
if errors.As(err, &be) {
452+
retErr = err
453+
// We can proceed to monitor
454+
} else {
455+
c.Close()
456+
return nil, nil, errors.Wrapf(err, "failed to build")
457+
}
450458
}
451-
}
452459

453-
if options.invokeConfig != nil && options.invokeConfig.needsDebug(retErr) {
454-
// Print errors before launching monitor
455-
if err := printError(retErr, printer); err != nil {
456-
logrus.Warnf("failed to print error information: %v", err)
460+
if options.invokeConfig != nil {
461+
if err := runMonitorIfNeeded(ctx, options.invokeConfig, retErr, c, f, os.Stdout, os.Stderr, printer); err != nil {
462+
c.Close()
463+
if errors.Is(err, monitor.ErrReload) {
464+
retErr = nil
465+
continue
466+
}
467+
logrus.Warnf("failed to run monitor: %v", err)
468+
}
457469
}
458470

459-
pr, pw := io.Pipe()
471+
c.Close()
472+
return resp, inputs, err
473+
}
474+
}
460475

461-
f := ioset.NewSingleForwarder()
462-
f.SetReader(dockerCli.In())
463-
f.SetWriter(pw, func() io.WriteCloser {
464-
pw.Close() // propagate EOF
465-
return nil
466-
})
467-
468-
// TODO: ref was never set to a value in the original code. Removed the variable to
469-
// reduce confusion but it also probably means this call is wrong in some way.
470-
// This area should be removed during the refactor anyway so it doesn't matter that much.
471-
monitorBuildResult, err := options.invokeConfig.runDebug(ctx, "", opts, c, pr, os.Stdout, os.Stderr, printer)
472-
if err := pw.Close(); err != nil {
473-
logrus.Debug("failed to close monitor stdin pipe reader")
474-
}
475-
if err != nil {
476-
logrus.Warnf("failed to run monitor: %v", err)
477-
}
478-
if monitorBuildResult != nil {
479-
// Update return values with the last build result from monitor
480-
resp, retErr = monitorBuildResult.Resp, monitorBuildResult.Err
481-
}
476+
func runMonitorIfNeeded(ctx context.Context, cfg *invokeConfig, retErr error, c *local.Controller, stdin *ioset.SingleForwarder, stdout io.WriteCloser, stderr console.File, printer *progress.Printer) error {
477+
if !cfg.needsDebug(retErr) {
478+
return nil
479+
}
480+
481+
// Print errors before launching monitor
482+
if err := printError(retErr, printer); err != nil {
483+
logrus.Warnf("failed to print error information: %v", err)
484+
}
485+
486+
pr, pw := io.Pipe()
487+
stdin.SetWriter(pw, func() io.WriteCloser {
488+
pw.Close() // propagate EOF
489+
return nil
490+
})
491+
492+
con := console.Current()
493+
if err := con.SetRaw(); err != nil {
494+
return errors.Errorf("failed to configure terminal: %v", err)
482495
}
496+
defer con.Reset()
483497

484-
return resp, inputs, retErr
498+
monitorErr := monitor.RunMonitor(ctx, &cfg.InvokeConfig, c, pr, stdout, stderr, printer)
499+
if err := pw.Close(); err != nil {
500+
logrus.Debug("failed to close monitor stdin pipe reader")
501+
}
502+
return monitorErr
485503
}
486504

487505
func printError(err error, printer *progress.Printer) error {
@@ -970,15 +988,6 @@ func (cfg *invokeConfig) needsDebug(retErr error) bool {
970988
}
971989
}
972990

973-
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) {
974-
con := console.Current()
975-
if err := con.SetRaw(); err != nil {
976-
return nil, errors.Errorf("failed to configure terminal: %v", err)
977-
}
978-
defer con.Reset()
979-
return monitor.RunMonitor(ctx, ref, options, &cfg.InvokeConfig, c, stdin, stdout, stderr, progress)
980-
}
981-
982991
func (cfg *invokeConfig) parseInvokeConfig(invoke, on string) error {
983992
cfg.onFlag = on
984993
cfg.invokeFlag = invoke

‎commands/debug/root.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ func RootCmd(dockerCli command.Cli, children ...DebuggableCmd) *cobra.Command {
5353
return errors.Errorf("failed to configure terminal: %v", err)
5454
}
5555

56-
_, err = monitor.RunMonitor(ctx, "", nil, &controllerapi.InvokeConfig{
56+
err = monitor.RunMonitor(ctx, &controllerapi.InvokeConfig{
5757
Tty: true,
5858
}, c, dockerCli.In(), os.Stdout, os.Stderr, printer)
5959
con.Reset()

‎controller/local/controller.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,10 +88,6 @@ func (b *Controller) Invoke(ctx context.Context, processes *processes.Manager, p
8888
}
8989
}
9090

91-
func (b *Controller) Inspect(ctx context.Context) *cbuild.Options {
92-
return b.buildConfig.buildOptions
93-
}
94-
9591
func (b *Controller) Close() error {
9692
if b.buildConfig.resultCtx != nil {
9793
b.buildConfig.resultCtx.Done()

‎monitor/commands/reload.go‎

Lines changed: 3 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,16 @@ package commands
22

33
import (
44
"context"
5-
"fmt"
6-
"io"
75

8-
cbuild "github.com/docker/buildx/controller/build"
9-
controllererrors "github.com/docker/buildx/controller/errdefs"
10-
controllerapi "github.com/docker/buildx/controller/pb"
116
"github.com/docker/buildx/monitor/types"
12-
"github.com/docker/buildx/util/progress"
13-
"github.com/moby/buildkit/solver/errdefs"
14-
"github.com/pkg/errors"
157
)
168

179
type ReloadCmd struct {
1810
m types.Monitor
19-
20-
stdout io.WriteCloser
21-
progress *progress.Printer
22-
23-
options *cbuild.Options
24-
invokeConfig *controllerapi.InvokeConfig
2511
}
2612

27-
func NewReloadCmd(m types.Monitor, stdout io.WriteCloser, progress *progress.Printer, options *cbuild.Options, invokeConfig *controllerapi.InvokeConfig) types.Command {
28-
return &ReloadCmd{m, stdout, progress, options, invokeConfig}
13+
func NewReloadCmd(m types.Monitor) types.Command {
14+
return &ReloadCmd{m: m}
2915
}
3016

3117
func (cm *ReloadCmd) Info() types.CommandInfo {
@@ -40,31 +26,6 @@ Usage:
4026
}
4127

4228
func (cm *ReloadCmd) Exec(ctx context.Context, args []string) error {
43-
bo := cm.m.Inspect(ctx)
44-
45-
var resultUpdated bool
46-
cm.progress.Unpause()
47-
_, _, err := cm.m.Build(ctx, bo, nil, cm.progress) // TODO: support stdin, hold build ref
48-
cm.progress.Pause()
49-
if err != nil {
50-
var be *controllererrors.BuildError
51-
if errors.As(err, &be) {
52-
resultUpdated = true
53-
} else {
54-
fmt.Printf("failed to reload: %v\n", err)
55-
}
56-
// report error
57-
for _, s := range errdefs.Sources(err) {
58-
s.Print(cm.stdout)
59-
}
60-
fmt.Fprintf(cm.stdout, "ERROR: %v\n", err)
61-
} else {
62-
resultUpdated = true
63-
}
64-
if resultUpdated {
65-
// rollback the running container with the new result
66-
id := cm.m.Rollback(ctx, cm.invokeConfig)
67-
fmt.Fprintf(cm.stdout, "Interactive container was restarted with process %q. Press Ctrl-a-c to switch to the new container\n", id)
68-
}
29+
cm.m.Reload()
6930
return nil
7031
}

‎monitor/monitor.go‎

Lines changed: 22 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,6 @@ import (
1010
"text/tabwriter"
1111

1212
"github.com/containerd/console"
13-
"github.com/docker/buildx/build"
14-
cbuild "github.com/docker/buildx/controller/build"
1513
"github.com/docker/buildx/controller/local"
1614
controllerapi "github.com/docker/buildx/controller/pb"
1715
"github.com/docker/buildx/controller/processes"
@@ -20,25 +18,23 @@ import (
2018
"github.com/docker/buildx/util/ioset"
2119
"github.com/docker/buildx/util/progress"
2220
"github.com/google/shlex"
23-
"github.com/moby/buildkit/client"
2421
"github.com/moby/buildkit/identity"
2522
"github.com/pkg/errors"
2623
"github.com/sirupsen/logrus"
2724
"golang.org/x/term"
2825
)
2926

30-
type MonitorBuildResult struct {
31-
Resp *client.SolveResponse
32-
Err error
33-
}
27+
var ErrReload = errors.New("monitor: reload")
3428

3529
// RunMonitor provides an interactive session for running and managing containers via specified IO.
36-
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) {
30+
func RunMonitor(ctx context.Context, invokeConfig *controllerapi.InvokeConfig, c *local.Controller, stdin io.ReadCloser, stdout io.WriteCloser, stderr console.File, progress *progress.Printer) error {
3731
if err := progress.Pause(); err != nil {
38-
return nil, err
32+
return err
3933
}
4034
defer progress.Unpause()
4135

36+
defer stdin.Close()
37+
4238
monitorIn, monitorOut := ioset.Pipe()
4339
defer func() {
4440
monitorIn.Close()
@@ -80,13 +76,13 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv
8076
return "Switched IO\n"
8177
}),
8278
}
79+
m.ctx, m.cancel = context.WithCancelCause(context.Background())
8380

8481
defer func() {
8582
if err := m.Close(); err != nil {
8683
logrus.Warnf("close error: %v", err)
8784
}
8885
}()
89-
m.ref.Store(curRef)
9086

9187
// Start container automatically
9288
fmt.Fprintf(stdout, "Launching interactive container. Press Ctrl-a-c to switch to monitor console\n")
@@ -96,7 +92,7 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv
9692
fmt.Fprintf(stdout, "Interactive container was restarted with process %q. Press Ctrl-a-c to switch to the new container\n", id)
9793

9894
availableCommands := []types.Command{
99-
commands.NewReloadCmd(m, stdout, progress, options, invokeConfig),
95+
commands.NewReloadCmd(m),
10096
commands.NewRollbackCmd(m, invokeConfig, stdout),
10197
commands.NewAttachCmd(m, stdout),
10298
commands.NewExecCmd(m, invokeConfig, stdout),
@@ -128,6 +124,11 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv
128124
}()
129125
t := term.NewTerminal(readWriter{in.Stdin, in.Stdout}, "(buildx) ")
130126
for {
127+
if err := m.ctx.Err(); err != nil {
128+
errCh <- context.Cause(m.ctx)
129+
return
130+
}
131+
131132
l, err := t.ReadLine()
132133
if err != nil {
133134
if err != io.EOF {
@@ -176,10 +177,10 @@ func RunMonitor(ctx context.Context, curRef string, options *cbuild.Options, inv
176177
select {
177178
case <-doneCh:
178179
m.close()
179-
return m.lastBuildResult, nil
180+
return nil
180181
case err := <-errCh:
181182
m.close()
182-
return m.lastBuildResult, err
183+
return err
183184
case <-monitorDisableCh:
184185
}
185186
monitorForwarder.SetOut(nil)
@@ -233,33 +234,23 @@ type readWriter struct {
233234
}
234235

235236
type monitor struct {
236-
c *local.Controller
237-
ref atomic.Value
237+
ctx context.Context
238+
cancel context.CancelCauseFunc
239+
240+
c *local.Controller
238241

239242
muxIO *ioset.MuxIO
240243
invokeIO *ioset.Forwarder
241244
invokeCancel func()
242245
attachedPid atomic.Value
243246

244-
lastBuildResult *MonitorBuildResult
245-
246247
processes *processes.Manager
247248
}
248249

249-
func (m *monitor) Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, input *build.Inputs, err error) {
250-
resp, _, err = m.c.Build(ctx, options, in, progress)
251-
m.lastBuildResult = &MonitorBuildResult{Resp: resp, Err: err} // Record build result
252-
return
253-
}
254-
255250
func (m *monitor) Invoke(ctx context.Context, pid string, cfg *controllerapi.InvokeConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error {
256251
return m.c.Invoke(ctx, m.processes, pid, cfg, ioIn, ioOut, ioErr)
257252
}
258253

259-
func (m *monitor) Inspect(ctx context.Context) *cbuild.Options {
260-
return m.c.Inspect(ctx)
261-
}
262-
263254
func (m *monitor) Rollback(ctx context.Context, cfg *controllerapi.InvokeConfig) string {
264255
pid := identity.NewID()
265256
cfg1 := cfg
@@ -281,6 +272,10 @@ func (m *monitor) Detach() {
281272
}
282273
}
283274

275+
func (m *monitor) Reload() {
276+
m.cancel(ErrReload)
277+
}
278+
284279
func (m *monitor) AttachedPID() string {
285280
return m.attachedPid.Load().(string)
286281
}

‎monitor/types/types.go‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,12 @@ import (
44
"context"
55
"io"
66

7-
"github.com/docker/buildx/build"
8-
cbuild "github.com/docker/buildx/controller/build"
97
controllerapi "github.com/docker/buildx/controller/pb"
108
"github.com/docker/buildx/controller/processes"
11-
"github.com/docker/buildx/util/progress"
12-
"github.com/moby/buildkit/client"
139
)
1410

1511
// Monitor provides APIs for attaching and controlling the buildx server.
1612
type Monitor interface {
17-
Build(ctx context.Context, options *cbuild.Options, in io.ReadCloser, progress progress.Writer) (resp *client.SolveResponse, inputs *build.Inputs, err error)
18-
19-
Inspect(ctx context.Context) *cbuild.Options
20-
2113
// Invoke starts an IO session into the specified process.
2214
// If pid doesn't match to any running processes, it starts a new process with the specified config.
2315
// If there is no container running or InvokeConfig.Rollback is specified, the process will start in a newly created container.
@@ -43,6 +35,9 @@ type Monitor interface {
4335
// Detach detaches IO from the container.
4436
Detach()
4537

38+
// Reload will signal the monitor to be reloaded.
39+
Reload()
40+
4641
io.Closer
4742
}
4843

0 commit comments

Comments
 (0)