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

Ensure that exec errors write exit codes to the DB #7236

Merged

Conversation

mheon
Copy link
Member

@mheon mheon commented Aug 5, 2020

In local Podman, the frontend interprets the error and exit code given by the Exec API to determine the appropriate exit code to set for Podman itself; special cases like a missing executable receive special exit codes.

Exec for the remote API, however, has to do this inside Libpod itself, as Libpod will be directly queried (via the Inspect API for exec sessions) to get the exit code. This was done correctly when the exec session started properly, but we did not properly handle cases where the OCI runtime fails before the exec session can properly start. Making two error returns that would otherwise not set exit code actually do so should resolve the issue.

Fixes #6893

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2020
@edsantiago
Copy link
Member

edsantiago commented Aug 5, 2020

Um, I just pulled this and I don't see the 126/127 status? Is there still something left to be done?

And if there is, would you mind adding this to the system tests, since there are no tests in e2e?

diff --git a/test/system/075-exec.bats b/test/system/075-exec.bats
index b2c49510a..dc73ad8ce 100644
--- a/test/system/075-exec.bats
+++ b/test/system/075-exec.bats
@@ -19,8 +19,15 @@ load helpers
     run_podman exec $cid sh -c "cat /$rand_filename"
     is "$output" "$rand_content" "Can exec and see file in running container"

+    # Specially defined situations: exec a dir, or no such command.
+    # We don't check the full error message because runc & crun differ.
+    run_podman 126 exec $cid /etc
+    is "$output" ".*permission denied"  "podman exec /etc"
+    run_podman 127 exec $cid /no/such/command
+    is "$output" ".*such file or dir"   "podman exec /no/such/command"
+
+    # Done
     run_podman exec $cid rm -f /$rand_filename
-
     run_podman wait $cid
     is "$output" "0"   "output from podman wait (container exit code)"

@mheon
Copy link
Member Author

mheon commented Aug 5, 2020

sudo ./bin/podman --remote exec -t -i testctr /etc is returning 126 for me.

Are you running the server with --log-level=debug? That screws up the logs (Conmon decides to insert things into them) and can break this.

@mheon
Copy link
Member Author

mheon commented Aug 5, 2020

Are you sure exec /no/such/command should be 127? I think that too is a 126

@mheon mheon force-pushed the write_error_to_inspect branch from 95fabbc to c8c906a Compare August 5, 2020 18:15
@edsantiago
Copy link
Member

Are you sure exec /no/such/command should be 127? I think that too is a 126

126 and 127 are specified in podman-run(1):

       126 The contained command cannot be invoked
       ...
       127 The contained command cannot be found

@mheon
Copy link
Member Author

mheon commented Aug 5, 2020

Ah, you're right. Fixing.

@edsantiago
Copy link
Member

Re: my earlier comment ("duh, not working for me"): PEBKAC. I forgot to restart the server after make. Sorry about that, it's working much better now.

In local Podman, the frontend interprets the error and exit code
given by the Exec API to determine the appropriate exit code to
set for Podman itself; special cases like a missing executable
receive special exit codes.

Exec for the remote API, however, has to do this inside Libpod
itself, as Libpod will be directly queried (via the Inspect API
for exec sessions) to get the exit code. This was done correctly
when the exec session started properly, but we did not properly
handle cases where the OCI runtime fails before the exec session
can properly start. Making two error returns that would otherwise
not set exit code actually do so should resolve the issue.

Fixes containers#6893

Signed-off-by: Matthew Heon <[email protected]>
@mheon mheon force-pushed the write_error_to_inspect branch from c8c906a to 7a64ce3 Compare August 5, 2020 18:30
@TomSweeneyRedHat
Copy link
Member

LGTM

@edsantiago
Copy link
Member

/lgtm

Thanks @mheon

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2020
@openshift-merge-robot openshift-merge-robot merged commit bae6d5d into containers:master Aug 5, 2020
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

podman-remote exec return codes incorrect
5 participants