8000 cmd/docker: split handling exit-code to a separate utility by thaJeztah · Pull Request #5229 · docker/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

cmd/docker: split handling exit-code to a separate utility #5229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Jul 5, 2024
Merged

cmd/docker: split handling exit-code to a separate utility #5229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5e7948e
e2e/cli-plugins: rename var that shadowed import
thaJeztah Jul 4, 2024
e9f32ed
e2e/cli-plugins: explicitly ignore fmt.Printxx errors
thaJeztah Jul 5, 2024
c6b4064
e2e/cli-plugins: use identifiable output for test
thaJeztah Jul 4, 2024
baf35da
e2e/cli-plugins: use cmd.CombinedOutput() instead of custom buffer
thaJeztah Jul 4, 2024
2f83064
e2e/cli-plugins: check for exit-errors in tests
thaJeztah Jul 4, 2024
3dd6fc3
cmd/docker: don't discard cli.StatusError errors without custom message
thaJeztah Jul 4, 2024
350a0b6
cli-plugins: Run(): don't discard cli.StatusError errors without message
thaJeztah Jul 4, 2024
b7695d6
cli-plugins: RunPlugin(): rename error-variable that's possibly shadowed
thaJeztah Jul 4, 2024
eae7509
cmd/docker: split handling exit-code to a separate utility
thaJeztah Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions cli-plugins/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package plugin
import (
"context"
"encoding/json"
"errors"
"fmt"
"os"
"sync"
Expand Down Expand Up @@ -34,7 +35,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager

var persistentPreRunOnce sync.Once
PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
var err error
var retErr error
persistentPreRunOnce.Do(func() {
ctx, cancel := context.WithCancel(cmd.Context())
cmd.SetContext(ctx)
Expand All @@ -46,7 +47,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
opts = append(opts, withPluginClientConn(plugin.Name()))
}
opts = append(opts, command.WithEnableGlobalMeterProvider(), command.WithEnableGlobalTracerProvider())
err = tcmd.Initialize(opts...)
retErr = tcmd.Initialize(opts...)
ogRunE := cmd.RunE
if ogRunE == nil {
ogRun := cmd.Run
Expand All @@ -66,7 +67,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
return err
}
})
return err
return retErr
}

cmd, args, err := tcmd.HandleGlobalFlags()
Expand All @@ -92,18 +93,17 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) {
plugin := makeCmd(dockerCli)

if err := RunPlugin(dockerCli, plugin, meta); err != nil {
if sterr, ok := err.(cli.StatusError); ok {
if sterr.Status != "" {
fmt.Fprintln(dockerCli.Err(), sterr.Status)
}
var stErr cli.StatusError
if errors.As(err, &stErr) {
Comment on lines 95 to +97
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably extract the code in the plugin-code as well (so maybe export the handling), but will look at that in follow-up(s)

// StatusError should only be used for errors, and all errors should
// have a non-zero exit status, so never exit with 0
if sterr.StatusCode == 0 {
os.Exit(1)
if stErr.StatusCode == 0 { // FIXME(thaJeztah): this should never be used with a zero status-code. Check if we do this anywhere.
stErr.StatusCode = 1
}
os.Exit(sterr.StatusCode)
_, _ = fmt.Fprintln(dockerCli.Err(), stErr)
os.Exit(stErr.StatusCode)
}
fmt.Fprintln(dockerCli.Err(), err)
_, _ = fmt.Fprintln(dockerCli.Err(), err)
os.Exit(1)
}
}
Expand Down
48 changes: 23 additions & 25 deletions cmd/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,43 +29,41 @@ import (
)

func main() {
statusCode := dockerMain()
if statusCode != 0 {
os.Exit(statusCode)
err := dockerMain(context.Background())
if err != nil && !errdefs.IsCancelled(err) {
_, _ = fmt.Fprintln(os.Stderr, err)
os.Exit(getExitCode(err))
}
}

func dockerMain() int {
ctx, cancelNotify := signal.NotifyContext(context.Background(), platformsignals.TerminationSignals...)
func dockerMain(ctx context.Context) error {
ctx, cancelNotify := signal.NotifyContext(ctx, platformsignals.TerminationSignals...)
defer cancelNotify()

dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))
if err != nil {
fmt.Fprintln(os.Stderr, err)
return 1
return err
}
logrus.SetOutput(dockerCli.Err())
otel.SetErrorHandler(debug.OTELErrorHandler)

if err := runDocker(ctx, dockerCli); err != nil {
if sterr, ok := err.(cli.StatusError); ok {
if sterr.Status != "" {
fmt.Fprintln(dockerCli.Err(), sterr.Status)
}
// StatusError should only be used for errors, and all errors should
// have a non-zero exit status, so never exit with 0
if sterr.StatusCode == 0 {
return 1
}
return sterr.StatusCode
}
if errdefs.IsCancelled(err) {
return 0
}
fmt.Fprintln(dockerCli.Err(), err)
return 1
return runDocker(ctx, dockerCli)
}

// getExitCode returns the exit-code to use for the given error.
// If err is a [cli.StatusError] and has a StatusCode set, it uses the
// status-code from it, otherwise it returns "1" for any error.
func getExitCode(err error) int {
if err == nil {
return 0
}
return 0
var stErr cli.StatusError
if errors.As(err, &stErr) && stErr.StatusCode != 0 { // FIXME(thaJeztah): StatusCode should never be used with a zero status-code. Check if we do this anywhere.
return stErr.StatusCode
}

// No status-code provided; all errors should have a non-zero exit code.
return 1
}

func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand {
Expand Down
18 changes: 9 additions & 9 deletions e2e/cli-plugins/plugins/presocket/main.go
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {
go func() {
<-cmd.Context().Done()
fmt.Fprintln(dockerCli.Out(), "context cancelled")
_, _ = fmt.Fprintln(dockerCli.Out(), "test-no-socket: exiting after context was done")
os.Exit(2)
}()
signalCh := make(chan os.Signal, 10)
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
go func() {
for range signalCh {
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
}
}()
<-time.After(3 * time.Second)
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
return nil
},
})
Expand All @@ -64,18 +64,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
RunE: func(cmd *cobra.Command, args []string) error {
go func() {
<-cmd.Context().Done()
fmt.Fprintln(dockerCli.Out(), "context cancelled")
_, _ = fmt.Fprintln(dockerCli.Out(), "test-socket: exiting after context was done")
os.Exit(2)
}()
signalCh := make(chan os.Signal, 10)
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
go func() {
for range signalCh {
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
}
}()
<-time.After(3 * time.Second)
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
return nil
},
})
Expand All @@ -91,11 +91,11 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
go func() {
for range signalCh {
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
}
}()
<-time.After(3 * time.Second)
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
return nil
},
})
Expand All @@ -113,7 +113,7 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
select {
case <-done:
case <-time.After(2 * time.Second):
fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
_, _ = fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
}
return nil
},
Expand Down
64 changes: 39 additions & 25 deletions e2e/cli-plugins/socket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package cliplugins

import (
"bytes"
"errors"
"io"
"os/exec"
"strings"
Expand All @@ -11,6 +11,7 @@ import (

"github.com/creack/pty"
"gotest.tools/v3/assert"
is "gotest.tools/v3/assert/cmp"
)

// TestPluginSocketBackwardsCompatible executes a plugin binary
Expand All @@ -37,15 +38,15 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
err := syscall.Kill(-command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal process group")
}()
bytes, err := io.ReadAll(ptmx)
out, err := io.ReadAll(ptmx)
if err != nil && !strings.Contains(err.Error(), "input/output error") {
t.Fatal("failed to get command output")
}

// the plugin is attached to the TTY, so the parent process
// ignores the received signal, and the plugin receives a SIGINT
// as well
assert.Equal(t, string(bytes), "received SIGINT\r\nexit after 3 seconds\r\n")
assert.Equal(t, string(out), "received SIGINT\r\nexit after 3 seconds\r\n")
})

// ensure that we don't break plugins that attempt to read from the TTY
Expand Down Expand Up @@ -95,13 +96,13 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
err := syscall.Kill(command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal process group")
}()
bytes, err := command.CombinedOutput()
t.Log("command output: " + string(bytes))
out, err := command.CombinedOutput()
t.Log("command output: " + string(out))
assert.NilError(t, err, "failed to run command")

// the plugin process does not receive a SIGINT
// so it exits after 3 seconds and prints this message
assert.Equal(t, string(bytes), "exit after 3 seconds\n")
assert.Equal(t, string(out), "exit after 3 seconds\n")
})

t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
Expand Down Expand Up @@ -130,13 +131,18 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal process group")
}()
bytes, err := command.CombinedOutput()
assert.ErrorContains(t, err, "exit status 1")
out, err := command.CombinedOutput()

var exitError *exec.ExitError
assert.Assert(t, errors.As(err, &exitError))
assert.Check(t, exitError.Exited())
assert.Check(t, is.Equal(exitError.ExitCode(), 1))
assert.Check(t, is.ErrorContains(err, "exit status 1"))

// the plugin process does not receive a SIGINT and does
// the CLI cannot cancel it over the socket, so it kills
// the plugin process and forcefully exits
assert.Equal(t, string(bytes), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
assert.Equal(t, string(out), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
})
})
}
Expand All @@ -161,25 +167,22 @@ func TestPluginSocketCommunication(t *testing.T) {
err := syscall.Kill(-command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal process group")
}()
bytes, err := io.ReadAll(ptmx)
out, err := io.ReadAll(ptmx)
if err != nil && !strings.Contains(err.Error(), "input/output error") {
t.Fatal("failed to get command output")
}

// the plugin is attached to the TTY, so the parent process
// ignores the received signal, and the plugin receives a SIGINT
// as well
assert.Equal(t, string(bytes), "received SIGINT\r\nexit after 3 seconds\r\n")
assert.Equal(t, string(out), "received SIGINT\r\nexit after 3 seconds\r\n")
})
})

t.Run("detached", func(t *testing.T) {
t.Run("the plugin does not get signalled", func(t *testing.T) {
cmd := run("presocket", "test-socket")
command := exec.Command(cmd.Command[0], cmd.Command[1:]...)
outB := bytes.Buffer{}
command.Stdout = &outB
command.Stderr = &outB
command.SysProcAttr = &syscall.SysProcAttr{
Setpgid: true,
}
Expand All @@ -190,13 +193,19 @@ func TestPluginSocketCommunication(t *testing.T) {
err := syscall.Kill(command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal CLI process")
}()
err := command.Run()
t.Log(outB.String())
assert.ErrorContains(t, err, "exit status 2")

// the plugin does not get signalled, but it does get it's
// context cancelled by the CLI through the socket
assert.Equal(t, outB.String(), "context cancelled\n")
out, err := command.CombinedOutput()

var exitError *exec.ExitError
assert.Assert(t, errors.As(err, &exitError))
assert.Check(t, exitError.Exited())
assert.Check(t, is.Equal(exitError.ExitCode(), 2))
assert.Check(t, is.ErrorContains(err, "exit status 2"))

// the plugin does not get signalled, but it does get its
// context canceled by the CLI through the socket
const expected = "test-socket: exiting after context was done\nexit status 2"
actual := strings.TrimSpace(string(out))
assert.Check(t, is.Equal(actual, expected))
})

t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
Expand All @@ -223,13 +232,18 @@ func TestPluginSocketCommunication(t *testing.T) {
err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
assert.NilError(t, err, "failed to signal CLI process§")
}()
bytes, err := command.CombinedOutput()
assert.ErrorContains(t, err, "exit status 1")
out, err := command.CombinedOutput()

var exitError *exec.ExitError
assert.Assert(t, errors.As(err, &exitError))
assert.Check(t, exitError.Exited())
assert.Check(t, is.Equal(exitError.ExitCode(), 1))
assert.Check(t, is.ErrorContains(err, "exit status 1"))

// the plugin process does not receive a SIGINT and does
// not exit after having it's context cancelled, so the CLI
// not exit after having it's context canceled, so the CLI
// kills the plugin process and forcefully exits
assert.Equal(t, string(bytes), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
assert.Equal(t, string(out), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
})
})
}
Loading
0