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

exec: fix pipes #4818

Merged

Conversation

haircommander
Copy link
Collaborator

In a largely anticlimatic solution to the saga of piped input from conmon, we come to this solution.

When we pass the Stdin stream to the exec.Command structure, it's immediately consumed and lost, instead of being consumed through CopyDetachable().

When we don't pass -i in, conmon is not told to create a masterfd_stdin, and won't pass anything to the container.

With both, we can do

echo hi | podman exec -til cat

and get the expected hi

fixes: #4785
fixes: #4326
fixes: #3302

Signed-off-by: Peter Hunt pehunt@redhat.com

In a largely anticlimatic solution to the saga of piped input from conmon, we come to this solution.

When we pass the Stdin stream to the exec.Command structure, it's immediately consumed and lost, instead of being consumed through CopyDetachable().

When we don't pass -i in, conmon is not told to create a masterfd_stdin, and won't pass anything to the container.

With both, we can do

echo hi | podman exec -til cat

and get the expected hi

Signed-off-by: Peter Hunt <pehunt@redhat.com>
@haircommander
Copy link
Collaborator Author

haircommander commented Jan 8, 2020

is it a day of triumph or mourning when a bug this long lasting has this simple of a fix? I feel both ways.

@mheon
Copy link
Member

mheon commented Jan 8, 2020

Hm. Do we need to do anything for podman run? I think it has this issue as well... We might need to store -i in the DB and use it to determine if Conmon needs -i?

@mheon
Copy link
Member

mheon commented Jan 8, 2020

Damn, though, glad to see we have a fix here

@mheon
Copy link
Member

mheon commented Jan 8, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, 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 Jan 8, 2020
@haircommander
Copy link
Collaborator Author

haircommander commented Jan 8, 2020

Hm. Do we need to do anything for podman run? I think it has this issue as well... We might need to store -i in the DB and use it to determine if Conmon needs -i?

hm it does seem run has the same problem, and it's not fixed here. Poking around..
just kidding, my reproducer dropped -i

 $ echo hi | podman run -i alpine cat
hi

works

@giuseppe
Copy link
Member

giuseppe commented Jan 9, 2020

echo hi | podman exec -til cat

is it expected to work with --tty as well? If I add -t the exec hangs.

In this case, Docker fails with the input device is not a TTY

@giuseppe
Copy link
Member

giuseppe commented Jan 9, 2020

not worth another iteration but if you are going to re-push for any reason, please shorten the commit log lines

@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2020

/lgtm
Is this worth putting out a point release? Maybe wait a week to see if anything else comes up?

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit 154b5ca into containers:master Jan 9, 2020
@mheon
Copy link
Member

mheon commented Jan 9, 2020

Yeah, I'd want to give this a bit to see if anything else pops up with 1.7.0.

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
6 participants