Skip to content
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.

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

Podman-remote run should wait for exit code #3934

Merged
merged 2 commits into from
Sep 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions cmd/podman/containers_prune.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ func pruneContainersCmd(c *cliconfig.PruneContainersValues) error {
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
if len(c.InputArgs) > 1 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
} else {
exitCode = 1
}
}
return err
}
if len(failures) > 0 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
}
return printCmdResults(ok, failures)
}
8 changes: 5 additions & 3 deletions cmd/podman/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/containers/libpod/cmd/podman/cliconfig"
"github.com/containers/libpod/libpod"
"github.com/containers/libpod/libpod/define"
_ "github.com/containers/libpod/pkg/hooks/0.1.0"
"github.com/containers/libpod/pkg/rootless"
"github.com/containers/libpod/version"
Expand All @@ -20,7 +21,7 @@ import (
// This is populated by the Makefile from the VERSION file
// in the repository
var (
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
Ctx context.Context
span opentracing.Span
closer io.Closer
Expand Down Expand Up @@ -152,11 +153,12 @@ func main() {
if err := rootCmd.Execute(); err != nil {
outputError(err)
} else {
// The exitCode modified from 125, indicates an application
// The exitCode modified from define.ExecErrorCodeGeneric,
// indicates an application
// running inside of a container failed, as opposed to the
// podman command failed. Must exit with that exit code
// otherwise command exited correctly.
if exitCode == 125 {
if exitCode == define.ExecErrorCodeGeneric {
exitCode = 0
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/podman/pause.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ func pauseCmd(c *cliconfig.PauseValues) error {
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
if len(c.InputArgs) > 1 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
} else {
exitCode = 1
}
}
return err
}
if len(failures) > 0 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
}
return printCmdResults(ok, failures)
}
4 changes: 2 additions & 2 deletions cmd/podman/restart.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,15 @@ func restartCmd(c *cliconfig.RestartValues) error {
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
if len(c.InputArgs) > 1 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
} else {
exitCode = 1
}
}
return err
}
if len(failures) > 0 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
}
return printCmdResults(ok, failures)
}
4 changes: 2 additions & 2 deletions cmd/podman/unpause.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ func unpauseCmd(c *cliconfig.UnpauseValues) error {
if err != nil {
if errors.Cause(err) == define.ErrNoSuchCtr {
if len(c.InputArgs) > 1 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
} else {
exitCode = 1
}
}
return err
}
if len(failures) > 0 {
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
}
return printCmdResults(ok, failures)
}
13 changes: 11 additions & 2 deletions libpod/container_api.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ func (c *Container) Kill(signal uint) error {
}

// Exec starts a new process inside the container
// Returns an exit code and an error. If Exec was not able to exec in the container before a failure, an exit code of 126 is returned.
// If another generic error happens, an exit code of 125 is returned.
// Returns an exit code and an error. If Exec was not able to exec in the container before a failure, an exit code of define.ExecErrorCodeCannotInvoke is returned.
// If another generic error happens, an exit code of define.ExecErrorCodeGeneric is returned.
// Sometimes, the $RUNTIME exec call errors, and if that is the case, the exit code is the exit code of the call.
// Otherwise, the exit code will be the exit code of the executed call inside of the container.
// TODO investigate allowing exec without attaching
Expand Down Expand Up @@ -821,3 +821,12 @@ func (c *Container) Restore(ctx context.Context, options ContainerCheckpointOpti
defer c.newContainerEvent(events.Restore)
return c.restore(ctx, options)
}

// AutoRemove indicates whether the container will be removed after it is executed
func (c *Container) AutoRemove() bool {
spec := c.config.Spec
if spec.Annotations == nil {
return false
}
return c.Spec().Annotations[InspectAnnotationAutoremove] == InspectResponseTrue
}
18 changes: 18 additions & 0 deletions libpod/define/exec_codes.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package define

import (
"strings"

"github.com/pkg/errors"
)

Expand Down Expand Up @@ -28,3 +30,19 @@ func TranslateExecErrorToExitCode(originalEC int, err error) int {
}
return originalEC
}

// ExitCode reads the error message when failing to executing container process
// and then returns 0 if no error, ExecErrorCodeNotFound if command does not exist, or ExecErrorCodeCannotInvoke for
// all other errors
func ExitCode(err error) int {
if err == nil {
return 0
}
e := strings.ToLower(err.Error())
if strings.Contains(e, "file not found") ||
strings.Contains(e, "no such file or directory") {
return ExecErrorCodeNotFound
}

return ExecErrorCodeCannotInvoke
}
26 changes: 7 additions & 19 deletions pkg/adapter/containers.go
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,7 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode
// if the container was created as part of a pod, also start its dependencies, if any.
if err := ctr.Start(ctx, c.IsSet("pod")); err != nil {
// This means the command did not exist
exitCode = 127
e := strings.ToLower(err.Error())
if strings.Contains(e, "permission denied") || strings.Contains(e, "operation not permitted") || strings.Contains(e, "file not found") || strings.Contains(e, "no such file or directory") {
exitCode = 126
}
return exitCode, err
return define.ExitCode(err), err
}

fmt.Printf("%s\n", ctr.ID())
Expand Down Expand Up @@ -401,21 +396,14 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode
// Do not perform cleanup, or wait for container exit code
// Just exit immediately
if errors.Cause(err) == define.ErrDetach {
exitCode = 0
return exitCode, nil
}
// This means the command did not exist
exitCode = 127
e := strings.ToLower(err.Error())
if strings.Contains(e, "permission denied") || strings.Contains(e, "operation not permitted") {
exitCode = 126
return 0, nil
}
if c.IsSet("rm") {
if deleteError := r.Runtime.RemoveContainer(ctx, ctr, true, false); deleteError != nil {
logrus.Debugf("unable to remove container %s after failing to start and attach to it", ctr.ID())
}
}
return exitCode, err
return define.ExitCode(err), err
}

if ecode, err := ctr.Wait(); err != nil {
Expand All @@ -424,7 +412,7 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode
event, err := r.Runtime.GetLastContainerEvent(ctr.ID(), events.Exited)
if err != nil {
logrus.Errorf("Cannot get exit code: %v", err)
exitCode = 127
exitCode = define.ExecErrorCodeNotFound
} else {
exitCode = event.ContainerExitCode
}
Expand Down Expand Up @@ -576,7 +564,7 @@ func (r *LocalRuntime) Restore(ctx context.Context, c *cliconfig.RestoreValues)
// Start will start a container
func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigProxy bool) (int, error) {
var (
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
lastError error
)

Expand Down Expand Up @@ -636,7 +624,7 @@ func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigP
event, err := r.Runtime.GetLastContainerEvent(ctr.ID(), events.Exited)
if err != nil {
logrus.Errorf("Cannot get exit code: %v", err)
exitCode = 127
exitCode = define.ExecErrorCodeNotFound
} else {
exitCode = event.ContainerExitCode
}
Expand Down Expand Up @@ -914,7 +902,7 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal
cmd []string
)
// default invalid command exit code
ec := 125
ec := define.ExecErrorCodeGeneric

if cli.Latest {
if ctr, err = r.GetLatestContainer(); err != nil {
Expand Down
39 changes: 22 additions & 17 deletions pkg/adapter/containers_remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -464,19 +464,22 @@ func (r *LocalRuntime) Run(ctx context.Context, c *cliconfig.RunValues, exitCode
results := shared.NewIntermediateLayer(&c.PodmanCommand, true)
cid, err := iopodman.CreateContainer().Call(r.Conn, results.MakeVarlink())
if err != nil {
return 0, err
return exitCode, err
}
if c.Bool("detach") {
_, err := iopodman.StartContainer().Call(r.Conn, cid)
if _, err := iopodman.StartContainer().Call(r.Conn, cid); err != nil {
return exitCode, err
}
fmt.Println(cid)
return 0, err
return 0, nil
}
errChan, err := r.attach(ctx, os.Stdin, os.Stdout, cid, true, c.String("detach-keys"))
exitChan, errChan, err := r.attach(ctx, os.Stdin, os.Stdout, cid, true, c.String("detach-keys"))
if err != nil {
return 0, err
return exitCode, err
}
exitCode = <-exitChan
finalError := <-errChan
return 0, finalError
return exitCode, finalError
}

func ReadExitFile(runtimeTmp, ctrID string) (int, error) {
Expand Down Expand Up @@ -572,7 +575,7 @@ func (r *LocalRuntime) Attach(ctx context.Context, c *cliconfig.AttachValues) er
return err
}
}
errChan, err := r.attach(ctx, inputStream, os.Stdout, c.InputArgs[0], false, c.DetachKeys)
_, errChan, err := r.attach(ctx, inputStream, os.Stdout, c.InputArgs[0], false, c.DetachKeys)
if err != nil {
return err
}
Expand Down Expand Up @@ -669,7 +672,7 @@ func (r *LocalRuntime) Restore(ctx context.Context, c *cliconfig.RestoreValues)
func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigProxy bool) (int, error) {
var (
finalErr error
exitCode = 125
exitCode = define.ExecErrorCodeGeneric
)
// TODO Figure out how to deal with exit codes
inputStream := os.Stdin
Expand All @@ -686,12 +689,13 @@ func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigP
}
// start.go makes sure that if attach, there can be only one ctr
if c.Attach {
errChan, err := r.attach(ctx, inputStream, os.Stdout, containerIDs[0], true, c.DetachKeys)
exitChan, errChan, err := r.attach(ctx, inputStream, os.Stdout, containerIDs[0], true, c.DetachKeys)
if err != nil {
return exitCode, nil
}
exitCode := <-exitChan
err = <-errChan
return 0, err
return exitCode, err
}

// TODO the notion of starting a pod container and its deps still needs to be worked through
Expand All @@ -710,13 +714,13 @@ func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigP
return exitCode, finalErr
}

func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid string, start bool, detachKeys string) (chan error, error) {
func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid string, start bool, detachKeys string) (chan int, chan error, error) {
var (
oldTermState *term.State
)
spec, err := r.Spec(cid)
if err != nil {
return nil, err
return nil, nil, err
}
resize := make(chan remotecommand.TerminalSize, 5)
haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
Expand All @@ -726,7 +730,7 @@ func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid s
if haveTerminal && spec.Process.Terminal {
cancel, oldTermState, err := handleTerminalAttach(ctx, resize)
if err != nil {
return nil, err
return nil, nil, err
}
defer cancel()
defer restoreTerminal(oldTermState)
Expand All @@ -738,19 +742,20 @@ func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid s
reply, err := iopodman.Attach().Send(r.Conn, varlink.Upgrade, cid, detachKeys, start)
if err != nil {
restoreTerminal(oldTermState)
return nil, err
return nil, nil, err
}

// See if the server accepts the upgraded connection or returns an error
_, err = reply()

if err != nil {
restoreTerminal(oldTermState)
return nil, err
return nil, nil, err
}

errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, stdin, stdout, oldTermState, resize, nil)
return errChan, nil
ecChan := make(chan int, 1)
errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, stdin, stdout, oldTermState, resize, ecChan)
return ecChan, errChan, nil
}

// PauseContainers pauses container(s) based on CLI inputs.
Expand Down
16 changes: 16 additions & 0 deletions pkg/util/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,3 +377,19 @@ func ValidatePullType(pullType string) (PullType, error) {
return PullImageMissing, errors.Errorf("invalid pull type %q", pullType)
}
}

// ExitCode reads the error message when failing to executing container process
// and then returns 0 if no error, 126 if command does not exist, or 127 for
// all other errors
func ExitCode(err error) int {
if err == nil {
return 0
}
e := strings.ToLower(err.Error())
if strings.Contains(e, "file not found") ||
strings.Contains(e, "no such file or directory") {
Copy link
Member

Choose a reason for hiding this comment

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

lol that's right, you tell that silly golang the proper way! 😁

Copy link
Member

Choose a reason for hiding this comment

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

Can we not use os.IsNotExist?

Copy link
Member

Choose a reason for hiding this comment

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

I see this is what we were doing before and it horrifies me but I suppose there must be a reason...

Copy link
Member Author

Choose a reason for hiding this comment

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

We are getting a string back from the OCI runtime that looks like.

ERRO[0024] container_linux.go:346: starting container process caused "exec: "/etc1": stat /etc1: no such file or directory": OCI runtime command not found error

Which is why they have this horrendous code.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't get ENOENT.

return 127
}

return 126
}
Loading