Skip to content

Commit

Permalink
cli/command/container: remove reportError, and put StatusError to use
Browse files Browse the repository at this point in the history 
The `reportError` utility was present because cli.StatusError would print
the error decorated with `Status: <error-message>, Code: <exit-code>`.
That was not desirable in many cases as it would mess-up the output. To
prevent this, the CLI had code to check for an empty `Status` (error message)
in which case the error would be "ignored" (and only used for the exit-status),
and the `reportError` utility would be used to manually print a custom error
message before returning the error.

Now that bca2090 fixed the output format
of `cli.StatusError`, and 3dd6fc3 and
350a0b6 no longer discard these error,
we can get rid of this utility, and just set the error-message for
the status-error.

This patch:

- Introduces a `withHelp` which takes care of decorating errors with
  a "Run --help" hint for the user.
- Introduces a `toStatusError` utility that detects certain errors in
  the container to assign a corresponding exit-code (these error-codes
  can be used to distinguish "client" errors from "container" errors).
- Removes the `reportError` utility, and removes code that manually
  printed errors before returning.

Behavior is mostly unmodified, with the exception of some slight reformatting
of the errors:

- `withHelp` adds a `docker:` prefix to the error, to indicate the error
  is produced by the `docker` command. This prefix was already present
  in most cases.
- The "--help" hint is slightly updated ("Run 'docker run --help' for
  more information" instead of "See 'docker run --help'"), to make it
  more clear that it's a "call to action".
- An empty is added before the "--help" hint to separate it better from
  the error-message.

Before this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never".
    See 'docker run --help'.
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.
    $ echo $?
    127

With this patch:

    $ docker run --pull=invalid-option alpine
    docker: invalid pull option: 'invalid-option': must be one of "always", "missing" or "never"

    Run 'docker run --help' for more information
    $ echo $?
    125

    $ docker run --rm alpine nosuchcommand
    docker: Error response from daemon: failed to create task for container: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "nosuchcommand": executable file not found in $PATH: unknown.

    Run 'docker run --help' for more information
    $ echo $?
    127

Signed-off-by: Sebastiaan van Stijn <[email protected]>
  • Loading branch information
@thaJeztah
thaJeztah committed Jul 5, 2024
1 parent 61c6ff2 commit f8e5f7b
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 38 deletions.
18 changes: 12 additions & 6 deletions cli/command/container/create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,10 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command {

func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error {
if err := validatePullOpt(options.pull); err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
newEnv := []string{}
Expand All @@ -97,12 +99,16 @@ func runCreate(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet,
copts.env = *opts.NewListOptsRef(&newEnv, nil)
containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
if err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.Client().ClientVersion()); err != nil {
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "create").Error(),
StatusCode: 125,
}
}
id, err := createContainer(ctx, dockerCli, containerCfg, options)
if err != nil {
Expand Down
78 changes: 46 additions & 32 deletions cli/command/container/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,10 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command {

func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error {
if err := validatePullOpt(ropts.pull); err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll()))
newEnv := []string{}
Expand All @@ -107,12 +109,16 @@ func runRun(ctx context.Context, dockerCli command.Cli, flags *pflag.FlagSet, ro
containerCfg, err := parse(flags, copts, dockerCli.ServerInfo().OSType)
// just in case the parse does not exit
if err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
if err = validateAPIVersion(containerCfg, dockerCli.CurrentVersion()); err != nil {
reportError(dockerCli.Err(), "run", err.Error(), true)
return cli.StatusError{StatusCode: 125}
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}
return runContainer(ctx, dockerCli, ropts, copts, containerCfg)
}
Expand Down Expand Up @@ -145,8 +151,7 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption

containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions)
if err != nil {
reportError(stderr, "run", err.Error(), true)
return runStartContainerErr(err)
return toStatusError(err)
}
if runOpts.sigProxy {
sigc := notifyAllSignals()
Expand Down Expand Up @@ -207,12 +212,11 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
<-errCh
}

reportError(stderr, "run", err.Error(), false)
if copts.autoRemove {
// wait container to be removed
<-statusChan
}
return runStartContainerErr(err)
return toStatusError(err)
}

if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
Expand Down Expand Up @@ -295,30 +299,40 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, containerID str
return resp.Close, nil
}

// reportError is a utility method that prints a user-friendly message
// containing the error that occurred during parsing and a suggestion to get help
func reportError(stderr io.Writer, name string, str string, withHelp bool) {
str = strings.TrimSuffix(str, ".") + "."
if withHelp {
str += "\nSee 'docker " + name + " --help'."
}
_, _ = fmt.Fprintln(stderr, "docker:", str)
// withHelp decorates the error with a suggestion to use "--help".
func withHelp(err error, commandName string) error {
return fmt.Errorf("docker: %w\n\nRun 'docker %s --help' for more information", err, commandName)
}

// if container start fails with 'not found'/'no such' error, return 127
// if container start fails with 'permission denied' error, return 126
// return 125 for generic docker daemon failures
func runStartContainerErr(err error) error {
trimmedErr := strings.TrimPrefix(err.Error(), "Error response from daemon: ")
statusError := cli.StatusError{StatusCode: 125}
if strings.Contains(trimmedErr, "executable file not found") ||
strings.Contains(trimmedErr, "no such file or directory") ||
strings.Contains(trimmedErr, "system cannot find the file specified") {
statusError = cli.StatusError{StatusCode: 127}
} else if strings.Contains(trimmedErr, syscall.EACCES.Error()) ||
strings.Contains(trimmedErr, syscall.EISDIR.Error()) {
statusError = cli.StatusError{StatusCode: 126}
// toStatusError attempts to detect specific error-conditions to assign
// an appropriate exit-code for situations where the problem originates
// from the container. It returns [cli.StatusError] with the original
// error message and the Status field set as follows:
//
// - 125: for generic failures sent back from the daemon
// - 126: if container start fails with 'permission denied' error
// - 127: if container start fails with 'not found'/'no such' error
func toStatusError(err error) error {
// TODO(thaJeztah): some of these errors originate from the container: should we actually suggest "--help" for those?

errMsg := err.Error()

if strings.Contains(errMsg, "executable file not found") || strings.Contains(errMsg, "no such file or directory") || strings.Contains(errMsg, "system cannot find the file specified") {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 127,
}
}

return statusError
if strings.Contains(errMsg, syscall.EACCES.Error()) || strings.Contains(errMsg, syscall.EISDIR.Error()) {
return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 126,
}
}

return cli.StatusError{
Status: withHelp(err, "run").Error(),
StatusCode: 125,
}
}

0 comments on commit f8e5f7b

Please sign in to comment.