From 8271ede6698b95f57388ba914611424b17dad1f8 Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Thu, 8 May 2025 17:43:58 -0600 Subject: [PATCH 1/6] Fix build output Multiplexing stderr to a buffer accidentally broke TTY detection in docker. This PR restores it by using a pipe instead of a wrapper. Also fixed `--progress` not being passed to the second build step. --- go.mod | 1 + go.sum | 2 ++ pkg/docker/docker_command.go | 18 ++++++++++++++++-- pkg/image/build.go | 5 +++-- 4 files changed, 22 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 5901a89987..a11c2f3cec 100644 --- a/go.mod +++ b/go.mod @@ -91,6 +91,7 @@ require ( github.com/containerd/platforms v0.2.1 // indirect github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect github.com/cpuguy83/dockercfg v0.3.2 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/curioswitch/go-reassign v0.3.0 // indirect github.com/daixiang0/gci v0.13.5 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect diff --git a/go.sum b/go.sum index 1270b5536a..fca692a117 100644 --- a/go.sum +++ b/go.sum @@ -123,6 +123,8 @@ github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHf github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/curioswitch/go-reassign v0.3.0 h1:dh3kpQHuADL3cobV/sSGETA8DOv457dwl+fbBAhrQPs= github.com/curioswitch/go-reassign v0.3.0/go.mod h1:nApPCCTtqLJN/s8HfItCcKV0jIPwluBOvZP+dsJGA88= github.com/daixiang0/gci v0.13.5 h1:kThgmH1yBmZSBCh1EJVxQ7JsHpm5Oms0AMed/0LaH4c= diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index 151dc7fc80..9ce0606aff 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -1,6 +1,7 @@ package docker import ( + "bytes" "context" "encoding/json" "errors" @@ -12,6 +13,7 @@ import ( "runtime" "strings" + "github.com/creack/pty" "github.com/docker/cli/cli/config" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/cli/config/types" @@ -337,14 +339,26 @@ func (c *DockerCommand) exec(ctx context.Context, in io.Reader, out io.Writer, d dockerCmd := DockerCommandFromEnvironment() cmd := exec.CommandContext(ctx, dockerCmd, args...) + pty, tty, _ := pty.Open() + defer pty.Close() + defer tty.Close() + cmd.Stderr = tty + if out == nil { out = os.Stderr } + var teeBuf bytes.Buffer // the ring buffer captures the last N bytes written to `w` so we have some context to return in an error - errbuf := util.NewRingBufferWriter(out, 1024) + errbuf := util.NewRingBufferWriter(&teeBuf, 1024) + go func() { + io.Copy(io.MultiWriter(out, errbuf), pty) + }() cmd.Stdout = errbuf - cmd.Stderr = errbuf + + // errbuf := util.NewRingBufferWriter(out, 1024) + // cmd.Stdout = errbuf + // cmd.Stderr = errbuf if in != nil { cmd.Stdin = in diff --git a/pkg/image/build.go b/pkg/image/build.go index 45dee4dcad..a7f934681f 100644 --- a/pkg/image/build.go +++ b/pkg/image/build.go @@ -271,7 +271,7 @@ func Build(ctx context.Context, cfg *config.Config, dir, imageName string, secre labels[key] = val } - if err := BuildAddLabelsAndSchemaToImage(ctx, dockerCommand, imageName, labels, bundledSchemaFile); err != nil { + if err := BuildAddLabelsAndSchemaToImage(ctx, dockerCommand, imageName, labels, bundledSchemaFile, progressOutput); err != nil { return fmt.Errorf("Failed to add labels to image: %w", err) } return nil @@ -280,7 +280,7 @@ func Build(ctx context.Context, cfg *config.Config, dir, imageName string, secre // BuildAddLabelsAndSchemaToImage builds a cog model with labels and schema. // // The new image is based on the provided image with the labels and schema file appended to it. -func BuildAddLabelsAndSchemaToImage(ctx context.Context, dockerClient command.Command, image string, labels map[string]string, bundledSchemaFile string) error { +func BuildAddLabelsAndSchemaToImage(ctx context.Context, dockerClient command.Command, image string, labels map[string]string, bundledSchemaFile string, progressOutput string) error { dockerfile := "FROM " + image + "\n" dockerfile += "COPY " + bundledSchemaFile + " .cog\n" @@ -288,6 +288,7 @@ func BuildAddLabelsAndSchemaToImage(ctx context.Context, dockerClient command.Co DockerfileContents: dockerfile, ImageName: image, Labels: labels, + ProgressOutput: progressOutput, } if err := dockerClient.ImageBuild(ctx, buildOpts); err != nil { From b79a8ad00449cd582432794f1f03b1f8e0aecb69 Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Mon, 12 May 2025 10:19:43 -0600 Subject: [PATCH 2/6] fix potential stderr/stdout deadlock --- pkg/docker/docker_command.go | 78 ++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index 9ce0606aff..b1bc4416b7 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -55,7 +55,7 @@ func (c *DockerCommand) Pull(ctx context.Context, image string, force bool) (*im "--platform", "linux/amd64", } - err := c.exec(ctx, nil, nil, "", args) + err := c.exec(ctx, nil, nil, nil, "", args) if err != nil { // A "not found" error message will be different depending on what flavor of engine and // registry version we're hitting. This checks for both docker and OCI lingo. @@ -76,7 +76,7 @@ func (c *DockerCommand) Pull(ctx context.Context, image string, force bool) (*im func (c *DockerCommand) Push(ctx context.Context, image string) error { console.Debugf("=== DockerCommand.Push %s", image) - return c.exec(ctx, nil, nil, "", []string{"push", image}) + return c.exec(ctx, nil, nil, nil, "", []string{"push", image}) } func (c *DockerCommand) LoadUserInformation(ctx context.Context, registryHost string) (*command.UserInfo, error) { @@ -120,7 +120,7 @@ func (c *DockerCommand) CreateTarFile(ctx context.Context, image string, tmpDir "/", folder, } - if err := c.exec(ctx, nil, nil, "", args); err != nil { + if err := c.exec(ctx, nil, nil, nil, "", args); err != nil { return "", err } return filepath.Join(tmpDir, tarFile), nil @@ -145,7 +145,7 @@ func (c *DockerCommand) CreateAptTarFile(ctx context.Context, tmpDir string, apt "/buildtmp/" + aptTarFile, } args = append(args, packages...) - if err := c.exec(ctx, nil, nil, "", args); err != nil { + if err := c.exec(ctx, nil, nil, nil, "", args); err != nil { return "", err } @@ -206,7 +206,7 @@ func (c *DockerCommand) ContainerLogs(ctx context.Context, containerID string, w "--follow", } - return c.exec(ctx, nil, w, "", args) + return c.exec(ctx, nil, w, nil, "", args) } func (c *DockerCommand) ContainerInspect(ctx context.Context, id string) (*container.InspectResponse, error) { @@ -247,7 +247,7 @@ func (c *DockerCommand) ContainerStop(ctx context.Context, containerID string) e containerID, } - if err := c.exec(ctx, nil, nil, "", args); err != nil { + if err := c.exec(ctx, nil, nil, nil, "", args); err != nil { if strings.Contains(err.Error(), "No such container") { err = &command.NotFoundError{Object: "container", Ref: containerID} } @@ -332,56 +332,72 @@ func (c *DockerCommand) ImageBuild(ctx context.Context, options command.ImageBui in := strings.NewReader(options.DockerfileContents) - return c.exec(ctx, in, nil, options.WorkingDir, args) + return c.exec(ctx, in, nil, nil, options.WorkingDir, args) } -func (c *DockerCommand) exec(ctx context.Context, in io.Reader, out io.Writer, dir string, args []string) error { +func (c *DockerCommand) exec(ctx context.Context, in io.Reader, outw, errw io.Writer, dir string, args []string) error { + if outw == nil { + outw = os.Stdout + } + if errw == nil { + errw = os.Stderr + } + dockerCmd := DockerCommandFromEnvironment() cmd := exec.CommandContext(ctx, dockerCmd, args...) + if dir != "" { + cmd.Dir = dir + } - pty, tty, _ := pty.Open() - defer pty.Close() - defer tty.Close() - cmd.Stderr = tty - - if out == nil { - out = os.Stderr + // setup pty for stderr output + stderrpty, stderrtty, err := pty.Open() + if err != nil { + return fmt.Errorf("failed to open stderr pty: %w", err) } - var teeBuf bytes.Buffer - // the ring buffer captures the last N bytes written to `w` so we have some context to return in an error - errbuf := util.NewRingBufferWriter(&teeBuf, 1024) + cmd.Stderr = stderrtty + + // setup stderr buffer & writer to errw and buffer + var stderrBuf bytes.Buffer go func() { - io.Copy(io.MultiWriter(out, errbuf), pty) + defer stderrpty.Close() + defer stderrtty.Close() + + io.Copy(io.MultiWriter( + errw, + util.NewRingBufferWriter(&stderrBuf, 1024), + ), stderrpty) }() - cmd.Stdout = errbuf - // errbuf := util.NewRingBufferWriter(out, 1024) - // cmd.Stdout = errbuf - // cmd.Stderr = errbuf + // setup stdout pipe + outpipe, err := cmd.StdoutPipe() + if err != nil { + return fmt.Errorf("failed to create stdout pipe: %w", err) + } + // copy stdout to outw + go func() { + defer outpipe.Close() + + io.Copy(outw, outpipe) + }() if in != nil { cmd.Stdin = in } - if dir != "" { - cmd.Dir = dir - } - console.Debug("$ " + strings.Join(cmd.Args, " ")) - err := cmd.Run() - if err != nil { + if err := cmd.Run(); err != nil { if errors.Is(err, context.Canceled) { return err } - return fmt.Errorf("command failed: %s: %w", errbuf.String(), err) + return fmt.Errorf("command failed: %s: %w", stderrBuf.String(), err) } return nil } func (c *DockerCommand) execCaptured(ctx context.Context, in io.Reader, dir string, args []string) (string, error) { var out strings.Builder - err := c.exec(ctx, in, &out, dir, args) + err := c.exec(ctx, in, &out, nil, dir, args) if err != nil { return "", err } From 94490ad6ea43824107470e8873b22f16af9161ac Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Mon, 12 May 2025 10:42:01 -0600 Subject: [PATCH 3/6] only use pty if error writer is a tty --- go.mod | 4 ++-- go.sum | 2 -- pkg/docker/docker_command.go | 44 ++++++++++++++++++++++-------------- 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/go.mod b/go.mod index a11c2f3cec..03206b30dc 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aws/aws-sdk-go-v2 v1.36.3 github.com/aws/aws-sdk-go-v2/credentials v1.17.67 github.com/aws/aws-sdk-go-v2/service/s3 v1.79.3 + github.com/creack/pty v1.1.24 github.com/docker/cli v28.1.1+incompatible github.com/docker/docker v28.1.1+incompatible github.com/docker/go-connections v0.5.0 @@ -33,6 +34,7 @@ require ( golang.org/x/sys v0.32.0 golang.org/x/term v0.31.0 gopkg.in/yaml.v2 v2.4.0 + gopkg.in/yaml.v3 v3.0.1 sigs.k8s.io/yaml v1.4.0 ) @@ -91,7 +93,6 @@ require ( github.com/containerd/platforms v0.2.1 // indirect github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect github.com/cpuguy83/dockercfg v0.3.2 // indirect - github.com/creack/pty v1.1.24 // indirect github.com/curioswitch/go-reassign v0.3.0 // indirect github.com/daixiang0/gci v0.13.5 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect @@ -280,7 +281,6 @@ require ( google.golang.org/genproto/googleapis/api v0.0.0-20250218202821-56aae31c358a // indirect google.golang.org/grpc v1.71.0 // indirect google.golang.org/protobuf v1.36.6 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect gotest.tools/gotestsum v1.12.2 // indirect honnef.co/go/tools v0.6.1 // indirect mvdan.cc/gofumpt v0.7.0 // indirect diff --git a/go.sum b/go.sum index fca692a117..08e4aa3d3d 100644 --- a/go.sum +++ b/go.sum @@ -121,8 +121,6 @@ github.com/containerd/stargz-snapshotter/estargz v0.16.3/go.mod h1:uyr4BfYfOj3G9 github.com/cpuguy83/dockercfg v0.3.2 h1:DlJTyZGBDlXqUZ2Dk2Q3xHs/FtnooJJVaad2S9GKorA= github.com/cpuguy83/dockercfg v0.3.2/go.mod h1:sugsbF4//dDlL/i+S+rtpIWp+5h0BHJHfjj5/jFyUJc= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= -github.com/creack/pty v1.1.18/go.mod h1:MOBLtS5ELjhRRrroQr9kyvTxUAFNvYEK993ew/Vr4O4= github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/curioswitch/go-reassign v0.3.0 h1:dh3kpQHuADL3cobV/sSGETA8DOv457dwl+fbBAhrQPs= diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index b1bc4416b7..b3caa2146c 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -349,25 +349,32 @@ func (c *DockerCommand) exec(ctx context.Context, in io.Reader, outw, errw io.Wr cmd.Dir = dir } - // setup pty for stderr output - stderrpty, stderrtty, err := pty.Open() - if err != nil { - return fmt.Errorf("failed to open stderr pty: %w", err) - } - - cmd.Stderr = stderrtty - // setup stderr buffer & writer to errw and buffer var stderrBuf bytes.Buffer - go func() { - defer stderrpty.Close() - defer stderrtty.Close() - io.Copy(io.MultiWriter( - errw, - util.NewRingBufferWriter(&stderrBuf, 1024), - ), stderrpty) - }() + // if errw is a TTY, use a pty for stderr output so that the child process will properly detect an interactive console + if f, ok := errw.(*os.File); ok && console.IsTTY(f) { + stderrpty, stderrtty, err := pty.Open() + if err != nil { + return fmt.Errorf("failed to open stderr pty: %w", err) + } + cmd.Stderr = stderrtty + + go func() { + defer stderrpty.Close() + defer stderrtty.Close() + + _, err = io.Copy(io.MultiWriter( + errw, + util.NewRingBufferWriter(&stderrBuf, 1024), + ), stderrpty) + if err != nil { + console.Errorf("failed to copy stderr pty to errw: %s", err) + } + }() + } else { + cmd.Stderr = io.MultiWriter(errw, util.NewRingBufferWriter(&stderrBuf, 1024)) + } // setup stdout pipe outpipe, err := cmd.StdoutPipe() @@ -378,7 +385,10 @@ func (c *DockerCommand) exec(ctx context.Context, in io.Reader, outw, errw io.Wr go func() { defer outpipe.Close() - io.Copy(outw, outpipe) + _, err = io.Copy(outw, outpipe) + if err != nil { + console.Errorf("failed to copy stdout to outw: %s", err) + } }() if in != nil { From a96066a16ff5b544edebb1e62f3d3e0281ff5878 Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Mon, 12 May 2025 11:04:11 -0600 Subject: [PATCH 4/6] fix deprecation warning --- pkg/docker/docker_command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index b3caa2146c..06092489be 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -243,7 +243,7 @@ func (c *DockerCommand) ContainerStop(ctx context.Context, containerID string) e args := []string{ "container", "stop", - "--time", "3", + "--timeout", "3", containerID, } From 537c428652223caefb25b21fcb3f3a87453cb646 Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Mon, 12 May 2025 11:04:25 -0600 Subject: [PATCH 5/6] container stop prints the id, discard it --- go.mod | 2 +- pkg/docker/docker_command.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 03206b30dc..a65adcfddb 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,6 @@ require ( github.com/aws/aws-sdk-go-v2 v1.36.3 github.com/aws/aws-sdk-go-v2/credentials v1.17.67 github.com/aws/aws-sdk-go-v2/service/s3 v1.79.3 - github.com/creack/pty v1.1.24 github.com/docker/cli v28.1.1+incompatible github.com/docker/docker v28.1.1+incompatible github.com/docker/go-connections v0.5.0 @@ -93,6 +92,7 @@ require ( github.com/containerd/platforms v0.2.1 // indirect github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect github.com/cpuguy83/dockercfg v0.3.2 // indirect + github.com/creack/pty v1.1.24 // indirect github.com/curioswitch/go-reassign v0.3.0 // indirect github.com/daixiang0/gci v0.13.5 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index 06092489be..bb3af15feb 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -247,7 +247,7 @@ func (c *DockerCommand) ContainerStop(ctx context.Context, containerID string) e containerID, } - if err := c.exec(ctx, nil, nil, nil, "", args); err != nil { + if err := c.exec(ctx, nil, io.Discard, nil, "", args); err != nil { if strings.Contains(err.Error(), "No such container") { err = &command.NotFoundError{Object: "container", Ref: containerID} } From 4048f69650a4fb3aa241d6afaf69656690d11e71 Mon Sep 17 00:00:00 2001 From: Michael Dwan Date: Mon, 12 May 2025 11:49:12 -0600 Subject: [PATCH 6/6] out goes to stderr unless a writer is provided --- go.mod | 2 +- pkg/docker/docker_command.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index a65adcfddb..03206b30dc 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( github.com/aws/aws-sdk-go-v2 v1.36.3 github.com/aws/aws-sdk-go-v2/credentials v1.17.67 github.com/aws/aws-sdk-go-v2/service/s3 v1.79.3 + github.com/creack/pty v1.1.24 github.com/docker/cli v28.1.1+incompatible github.com/docker/docker v28.1.1+incompatible github.com/docker/go-connections v0.5.0 @@ -92,7 +93,6 @@ require ( github.com/containerd/platforms v0.2.1 // indirect github.com/containerd/stargz-snapshotter/estargz v0.16.3 // indirect github.com/cpuguy83/dockercfg v0.3.2 // indirect - github.com/creack/pty v1.1.24 // indirect github.com/curioswitch/go-reassign v0.3.0 // indirect github.com/daixiang0/gci v0.13.5 // indirect github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect diff --git a/pkg/docker/docker_command.go b/pkg/docker/docker_command.go index bb3af15feb..3e16249e6f 100644 --- a/pkg/docker/docker_command.go +++ b/pkg/docker/docker_command.go @@ -247,7 +247,7 @@ func (c *DockerCommand) ContainerStop(ctx context.Context, containerID string) e containerID, } - if err := c.exec(ctx, nil, io.Discard, nil, "", args); err != nil { + if err := c.exec(ctx, nil, nil, nil, "", args); err != nil { if strings.Contains(err.Error(), "No such container") { err = &command.NotFoundError{Object: "container", Ref: containerID} } @@ -337,7 +337,7 @@ func (c *DockerCommand) ImageBuild(ctx context.Context, options command.ImageBui func (c *DockerCommand) exec(ctx context.Context, in io.Reader, outw, errw io.Writer, dir string, args []string) error { if outw == nil { - outw = os.Stdout + outw = os.Stderr } if errw == nil { errw = os.Stderr