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

[thaJeztah]

This allows dockerMain() to return an error "as usual", and puts the responsibility for turning that into an appropriate exit-code in main() (which also sets the exit-code when terminating).

We could consider putting this utility in the cli package and exporting it if would be useful for doing a similar handling in plugins.

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Working on some other changes related to errors, but this one was pretty isolated, so let me open a small PR for this.

@Benehiko @vvoland ptal

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 61.49%. Comparing base (cad08ff) to head (eae7509).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5229      +/-   ##
==========================================
+ Coverage   61.47%   61.49%   +0.01%     
==========================================
  Files         298      298              
  Lines       20815    20811       -4     
==========================================
  Hits        12797    12797              
+ Misses       7106     7102       -4     
  Partials      912      912              

[thaJeztah]

AH! I broke something probably; I was indeed thinking if the context.Cancelled() should always be ignored here, or only from within dockerMain() 🤔

=== FAIL: e2e/cli-plugins TestPluginSocketCommunication/detached/the_plugin_does_not_get_signalled (1.00s)
    socket_test.go:194: context cancelled
        Status: , Code: 2
        
    socket_test.go:199: assertion failed: 
        --- ←
        +++ →
        @@ -1,3 +1,2 @@
         context cancelled
        -Status: , Code: 2

Slightly wondering now if the test should be checking for the context cancelled error to appear in the output, or if the actual error should be the Status: , Code: 2 that should be printed

cli/e2e/cli-plugins/socket_test.go

Lines 193 to 199 in 5aae44b

	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")

[thaJeztah]

Wait; what's this? Do we have 2 separate spellings of cancelled in our codebase? (Notice the cancelled vs canceled (2 -> 1 l))

=== FAIL: e2e/cli-plugins TestPluginSocketCommunication/detached/the_plugin_does_not_get_signalled (1.00s)
    socket_test.go:198: context cancelled
        
    socket_test.go:207: assertion failed: context cancelled (string) != context canceled (string)

This happened when I changed the test to use context.Canceled.Error();

assert.Check(t, is.Equal(strings.TrimSpace(string(out)), context.Canceled.Error()))

[thaJeztah]

Ha! We do!

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

Lines 64 to 69 in 5aae44b

	RunE: func(cmd *cobra.Command, args []string) error {
	go func() {
	<-cmd.Context().Done()
	fmt.Fprintln(dockerCli.Out(), "context cancelled")
	os.Exit(2)
	}()

[thaJeztah]

And... after my initial confusion around context cancel(l)ed, I think my patch was not "bad", but just now prints the actual (but fugly - to be looked at) cli.StatusError;

=== FAIL: e2e/cli-plugins TestPluginSocketCommunication/detached/the_plugin_does_not_get_signalled (1.00s)
    socket_test.go:208: assertion failed: 
        --- actual
        +++ expected
        @@ -1,2 +1 @@
        -test-socket: exiting after context was done
        -Status: , Code: 2
        +test-socket: exiting after context was done

Before my patch, such errors would be silently discarded;

cli/cmd/docker/docker.go

Lines 52 to 54 in 5aae44b

	if sterr.Status != "" {
	fmt.Fprintln(dockerCli.Err(), sterr.Status)
	}

It's odd that we're constructing our own output, instead of StatusError.Error() handling proper output; it had ONE job, and it did the absolute worst by printing the exit-code as some ugly string.

cli/cli/error.go

Lines 31 to 33 in 5aae44b

	func (e StatusError) Error() string {
	return fmt.Sprintf("Status: %s, Code: %d", e.Status, e.StatusCode)
	}

We can probably borrow similar logic / output as used by Go's exec.ExecError -> ProcessState.String(); https://github.com/golang/go/blob/82c14346d89ec0eeca114f9ca0e88516b2cda454/src/os/exec/exec.go#L872-L874
https://github.com/golang/go/blob/82c14346d89ec0eeca114f9ca0e88516b2cda454/src/os/exec_posix.go#L107-L135

func (p *ProcessState) String() string {
	if p == nil {
		return "<nil>"
	}
	status := p.Sys().(syscall.WaitStatus)
	res := ""
	switch {
	case status.Exited():
		code := status.ExitStatus()
		if runtime.GOOS == "windows" && uint(code) >= 1<<16 { // windows uses large hex numbers
			res = "exit status " + itoa.Uitox(uint(code))
		} else { // unix systems use small decimal integers
			res = "exit status " + itoa.Itoa(code) // unix
		}
	case status.Signaled():
		res = "signal: " + status.Signal().String()
	case status.Stopped():
		res = "stop signal: " + status.StopSignal().String()
		if status.StopSignal() == syscall.SIGTRAP && status.TrapCause() != 0 {
			res += " (trap " + itoa.Itoa(status.TrapCause()) + ")"
		}
	case status.Continued():
		res = "continued"
	}
	if status.CoreDump() {
		res += " (core dumped)"
	}
	return res
}

[laurazard]

Thankyou! Yeah, this had been a bit of a mess for a while 😅

[thaJeztah]

It still is messy, but "baby steps"; at least #5231 helped making these Status: <usualy empty>, Code: <you know you can get the exit code in a shell, right?> errors a bit less fugly, but there's still a lot to be done. I DO think that an error-type that allows setting the exit-code to use is useful (heck, exec.ExitError works fine?), but perhaps it should be a wrapper around the error that's created (which in itself could have information to keep), e.g. thinking along the lines of;

if err := doSomething(); err != nil {
    if errdefs.IsInvalidParameter(err) {
        return cli.StatusErr(err, 123)
    }
    return err
}

But we need to have a good look at that; basically, the status should probably ONLY be set if the error MUST be considered final / terminal; once you set the exit-code, that's it? Every caller handling that error must not ignore the error, and make it trickle up so that the CLI terminates with the given status. We need to look at that consideration if that works (but we could still end up having different classes, basically equivalent to the errdefs types we have, which help putting errors in different "categories").

[thaJeztah]

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)

[vvoland]

Some "regression" after this PR:

[I] pawel ❱ docker run --rm -it alpine
/ # ^C

/ # ^C

/ #
exit status 130
[I] pawel ❱ 

Before:

[I] pawel ~ ❱ docker run --rm -it alpine
/ # ^C

/ # ^C

/ # ^C

/ #

[I] pawel ~ ❱

Notice that after this PR exit status 130 is printed.

cc @thaJeztah

[thaJeztah]

Good one; I think it's worth having a ticket for that @vvoland so that we can look into what the intent was there; do we want it to have a non-zero exit status (if so; why don't we have an error message to go with it?) or did some (context) error trickle through and therefore we set a 130 exit status?

And maybe there's cases we DO want to have just the exit status with no message? 🤔

[thaJeztah]

In the "before" case; was the exit status 0? (echo $?)