-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
exec: get the exit code from sync pipe instead of file #5373
exec: get the exit code from sync pipe instead of file #5373
Conversation
f1b0e33
to
edab75f
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, haircommander 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 |
code lgtm |
This may cause issues with my detached exec code for APIv2 - how do we
source exit codes there?
…On Mon, Mar 2, 2020, 16:39 Brent Baude ***@***.***> wrote:
code lgtm
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5373>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCE5HNCF6RFLATOC23TRFQRP3ANCNFSM4K73IRNQ>
.
|
it would probably take an entirely different code path. We only wait on the exit code when we are attached to it. In updating the exec exit code (for an podman exec list or something, whatever the api will be for listing a container's exec session), we could check for the existence of the exit code via file. If need be, we could even save the pipe fd in the db or the location of the file, and open it as needed. This PR should fit right into your exec rework PR as is, and we can logic through how detach works with this when you get there. But for now this solves an immediate CI flake, so I'm inclined to go forward and think about reworks later. |
Before, we were getting the exit code from the file, in which we waited an arbitrary amount of time (5 seconds) for the file, and segfaulted if we didn't find it. instead, we should be a bit more certain conmon has sent the exit code. Luckily, it sends the exit code along the sync pipe fd, so we can read it from there Adapt the ExecContainer interface to pass along a channel to get the pid and exit code from conmon, to be able to read both from the pipe Signed-off-by: Peter Hunt <pehunt@redhat.com>
edab75f
to
e4c89aa
Compare
Before, we were using -1 as a bogus value in podman to signify something went wrong when reading from a conmon pipe. However, conmon uses negative values to indicate the runtime failed, and return the runtime's exit code. instead, we should use a bogus value that is actually bogus. Define that value in the define package as MinInt32 (-1<< 31 - 1), which is outside of the range of possible pids (-1 << 31) Signed-off-by: Peter Hunt <pehunt@redhat.com>
e4c89aa
to
d3d97a2
Compare
This is ready |
LGTM |
/hold |
/lgtm |
/hold cancel |
This reverts commit 4632b81. We are reverting containers#5373 as well, which lays the foundation for this commit, so it has to go as well. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
This reverts commit 4632b81. We are reverting containers#5373 as well, which lays the foundation for this commit, so it has to go as well. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
Before, we were getting the exit code from the file, in which we waited an arbitrary amount of time (5 seconds) for the file, and segfaulted if we didn't find it. instead, we should be a bit more certain conmon has sent the exit code. Luckily, it sends the exit code along the sync pipe fd, so we can read it from there
Adapt the ExecContainer interface to pass along a channel to get the pid and exit code from conmon, to be able to read both from the pipe
Also slightly change the way we report a bogus value when failing to read from the conmon pipe. we now set it to a value that cannot be a valid pid * -1, to make sure we correctly report the place an error comes from
Signed-off-by: Peter Hunt pehunt@redhat.com