Skip to content

Commit 61c6ff2

Browse files
thaJeztahthaJeztah
authored
Merge pull request docker#5229 from thaJeztah/exit_error
cmd/docker: split handling exit-code to a separate utility
2 parents cad08ff + eae7509 commit 61c6ff2

File tree

4 files changed

+82
-70
lines changed

4 files changed

+82
-70
lines changed

‎cli-plugins/plugin/plugin.go

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package plugin
33
import (
44
"context"
55
"encoding/json"
6+
"errors"
67
"fmt"
78
"os"
89
"sync"
@@ -34,7 +35,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
3435

3536
var persistentPreRunOnce sync.Once
3637
PersistentPreRunE = func(cmd *cobra.Command, _ []string) error {
37-
var err error
38+
var retErr error
3839
persistentPreRunOnce.Do(func() {
3940
ctx, cancel := context.WithCancel(cmd.Context())
4041
cmd.SetContext(ctx)
@@ -46,7 +47,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
4647
opts = append(opts, withPluginClientConn(plugin.Name()))
4748
}
4849
opts = append(opts, command.WithEnableGlobalMeterProvider(), command.WithEnableGlobalTracerProvider())
49-
err = tcmd.Initialize(opts...)
50+
retErr = tcmd.Initialize(opts...)
5051
ogRunE := cmd.RunE
5152
if ogRunE == nil {
5253
ogRun := cmd.Run
@@ -66,7 +67,7 @@ func RunPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager
6667
return err
6768
}
6869
})
69-
return err
70+
return retErr
7071
}
7172

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

9495
if err := RunPlugin(dockerCli, plugin, meta); err != nil {
95-
if sterr, ok := err.(cli.StatusError); ok {
96-
if sterr.Status != "" {
97-
fmt.Fprintln(dockerCli.Err(), sterr.Status)
98-
}
96+
var stErr cli.StatusError
97+
if errors.As(err, &stErr) {
9998
// StatusError should only be used for errors, and all errors should
10099
// have a non-zero exit status, so never exit with 0
101-
if sterr.StatusCode == 0 {
102-
os.Exit(1)
100+
if stErr.StatusCode == 0 { // FIXME(thaJeztah): this should never be used with a zero status-code. Check if we do this anywhere.
101+
stErr.StatusCode = 1
103102
}
104-
os.Exit(sterr.StatusCode)
103+
_, _ = fmt.Fprintln(dockerCli.Err(), stErr)
104+
os.Exit(stErr.StatusCode)
105105
}
106-
fmt.Fprintln(dockerCli.Err(), err)
106+
_, _ = fmt.Fprintln(dockerCli.Err(), err)
107107
os.Exit(1)
108108
}
109109
}

‎cmd/docker/docker.go

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -29,43 +29,41 @@ import (
2929
)
3030

3131
func main() {
32-
statusCode := dockerMain()
33-
if statusCode != 0 {
34-
os.Exit(statusCode)
32+
err := dockerMain(context.Background())
33+
if err != nil && !errdefs.IsCancelled(err) {
34+
_, _ = fmt.Fprintln(os.Stderr, err)
35+
os.Exit(getExitCode(err))
3536
}
3637
}
3738

38-
func dockerMain() int {
39-
ctx, cancelNotify := signal.NotifyContext(context.Background(), platformsignals.TerminationSignals...)
39+
func dockerMain(ctx context.Context) error {
40+
ctx, cancelNotify := signal.NotifyContext(ctx, platformsignals.TerminationSignals...)
4041
defer cancelNotify()
4142

4243
dockerCli, err := command.NewDockerCli(command.WithBaseContext(ctx))
4344
if err != nil {
44-
fmt.Fprintln(os.Stderr, err)
45-
return 1
45+
return err
4646
}
4747
logrus.SetOutput(dockerCli.Err())
4848
otel.SetErrorHandler(debug.OTELErrorHandler)
4949

50-
if err := runDocker(ctx, dockerCli); err != nil {
51-
if sterr, ok := err.(cli.StatusError); ok {
52-
if sterr.Status != "" {
53-
fmt.Fprintln(dockerCli.Err(), sterr.Status)
54-
}
55-
// StatusError should only be used for errors, and all errors should
56-
// have a non-zero exit status, so never exit with 0
57-
if sterr.StatusCode == 0 {
58-
return 1
59-
}
60-
return sterr.StatusCode
61-
}
62-
if errdefs.IsCancelled(err) {
63-
return 0
64-
}
65-
fmt.Fprintln(dockerCli.Err(), err)
66-
return 1
50+
return runDocker(ctx, dockerCli)
51+
}
52+
53+
// getExitCode returns the exit-code to use for the given error.
54+
// If err is a [cli.StatusError] and has a StatusCode set, it uses the
55+
// status-code from it, otherwise it returns "1" for any error.
56+
func getExitCode(err error) int {
57+
if err == nil {
58+
return 0
6759
}
68-
return 0
60+
var stErr cli.StatusError
61+
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.
62+
return stErr.StatusCode
63+
}
64+
65+
// No status-code provided; all errors should have a non-zero exit code.
66+
return 1
6967
}
7068

7169
func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand {

‎e2e/cli-plugins/plugins/presocket/main.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -39,18 +39,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
3939
RunE: func(cmd *cobra.Command, args []string) error {
4040
go func() {
4141
<-cmd.Context().Done()
42-
fmt.Fprintln(dockerCli.Out(), "context cancelled")
42+
_, _ = fmt.Fprintln(dockerCli.Out(), "test-no-socket: exiting after context was done")
4343
os.Exit(2)
4444
}()
4545
signalCh := make(chan os.Signal, 10)
4646
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
4747
go func() {
4848
for range signalCh {
49-
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
49+
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
5050
}
5151
}()
5252
<-time.After(3 * time.Second)
53-
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
53+
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
5454
return nil
5555
},
5656
})
@@ -64,18 +64,18 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
6464
RunE: func(cmd *cobra.Command, args []string) error {
6565
go func() {
6666
<-cmd.Context().Done()
67-
fmt.Fprintln(dockerCli.Out(), "context cancelled")
67+
_, _ = fmt.Fprintln(dockerCli.Out(), "test-socket: exiting after context was done")
6868
os.Exit(2)
6969
}()
7070
signalCh := make(chan os.Signal, 10)
7171
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
7272
go func() {
7373
for range signalCh {
74-
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
74+
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
7575
}
7676
}()
7777
<-time.After(3 * time.Second)
78-
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
78+
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
7979
return nil
8080
},
8181
})
@@ -91,11 +91,11 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
9191
signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
9292
go func() {
9393
for range signalCh {
94-
fmt.Fprintln(dockerCli.Out(), "received SIGINT")
94+
_, _ = fmt.Fprintln(dockerCli.Out(), "received SIGINT")
9595
}
9696
}()
9797
<-time.After(3 * time.Second)
98-
fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
98+
_, _ = fmt.Fprintln(dockerCli.Err(), "exit after 3 seconds")
9999
return nil
100100
},
101101
})
@@ -113,7 +113,7 @@ func RootCmd(dockerCli command.Cli) *cobra.Command {
113113
select {
114114
case <-done:
115115
case <-time.After(2 * time.Second):
116-
fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
116+
_, _ = fmt.Fprint(dockerCli.Err(), "timeout after 2 seconds")
117117
}
118118
return nil
119119
},

‎e2e/cli-plugins/socket_test.go

Lines changed: 39 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
package cliplugins
22

33
import (
4-
"bytes"
4+
"errors"
55
"io"
66
"os/exec"
77
"strings"
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/creack/pty"
1313
"gotest.tools/v3/assert"
14+
is "gotest.tools/v3/assert/cmp"
1415
)
1516

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

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

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

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

107108
t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
@@ -130,13 +131,18 @@ func TestPluginSocketBackwardsCompatible(t *testing.T) {
130131
err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
131132
assert.NilError(t, err, "failed to signal process group")
132133
}()
133-
bytes, err := command.CombinedOutput()
134-
assert.ErrorContains(t, err, "exit status 1")
134+
out, err := command.CombinedOutput()
135+
136+
var exitError *exec.ExitError
137+
assert.Assert(t, errors.As(err, &exitError))
138+
assert.Check(t, exitError.Exited())
139+
assert.Check(t, is.Equal(exitError.ExitCode(), 1))
140+
assert.Check(t, is.ErrorContains(err, "exit status 1"))
135141

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

169175
// the plugin is attached to the TTY, so the parent process
170176
// ignores the received signal, and the plugin receives a SIGINT
171177
// as well
172-
assert.Equal(t, string(bytes), "received SIGINT\r\nexit after 3 seconds\r\n")
178+
assert.Equal(t, string(out), "received SIGINT\r\nexit after 3 seconds\r\n")
173179
})
174180
})
175181

176182
t.Run("detached", func(t *testing.T) {
177183
t.Run("the plugin does not get signalled", func(t *testing.T) {
178184
cmd := run("presocket", "test-socket")
179185
command := exec.Command(cmd.Command[0], cmd.Command[1:]...)
180-
outB := bytes.Buffer{}
181-
command.Stdout = &outB
182-
command.Stderr = &outB
183186
command.SysProcAttr = &syscall.SysProcAttr{
184187
Setpgid: true,
185188
}
@@ -190,13 +193,19 @@ func TestPluginSocketCommunication(t *testing.T) {
190193
err := syscall.Kill(command.Process.Pid, syscall.SIGINT)
191194
assert.NilError(t, err, "failed to signal CLI process")
192195
}()
193-
err := command.Run()
194-
t.Log(outB.String())
195-
assert.ErrorContains(t, err, "exit status 2")
196-
197-
// the plugin does not get signalled, but it does get it's
198-
// context cancelled by the CLI through the socket
199-
assert.Equal(t, outB.String(), "context cancelled\n")
196+
out, err := command.CombinedOutput()
197+
198+
var exitError *exec.ExitError
199+
assert.Assert(t, errors.As(err, &exitError))
200+
assert.Check(t, exitError.Exited())
201+
assert.Check(t, is.Equal(exitError.ExitCode(), 2))
202+
assert.Check(t, is.ErrorContains(err, "exit status 2"))
203+
204+
// the plugin does not get signalled, but it does get its
205+
// context canceled by the CLI through the socket
206+
const expected = "test-socket: exiting after context was done\nexit status 2"
207+
actual := strings.TrimSpace(string(out))
208+
assert.Check(t, is.Equal(actual, expected))
200209
})
201210

202211
t.Run("the main CLI exits after 3 signals", func(t *testing.T) {
@@ -223,13 +232,18 @@ func TestPluginSocketCommunication(t *testing.T) {
223232
err = syscall.Kill(command.Process.Pid, syscall.SIGINT)
224233
assert.NilError(t, err, "failed to signal CLI process§")
225234
}()
226-
bytes, err := command.CombinedOutput()
227-
assert.ErrorContains(t, err, "exit status 1")
235+
out, err := command.CombinedOutput()
236+
237+
var exitError *exec.ExitError
238+
assert.Assert(t, errors.As(err, &exitError))
239+
assert.Check(t, exitError.Exited())
240+
assert.Check(t, is.Equal(exitError.ExitCode(), 1))
241+
assert.Check(t, is.ErrorContains(err, "exit status 1"))
228242

229243
// the plugin process does not receive a SIGINT and does
230-
// not exit after having it's context cancelled, so the CLI
244+
// not exit after having it's context canceled, so the CLI
231245
// kills the plugin process and forcefully exits
232-
assert.Equal(t, string(bytes), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
246+
assert.Equal(t, string(out), "got 3 SIGTERM/SIGINTs, forcefully exiting\n")
233247
})
234248
})
235249
}

0 commit comments

Comments
 (0)