exec: support --preserve-fds #2426
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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 |
openshift-ci-robot
added
approved
| @@ -47,6 +49,7 @@ func init() { | |||
| flags.BoolVarP(&execCommand.Tty, "tty", "t", false, "Allocate a pseudo-TTY. The default is false") | |||
| flags.StringVarP(&execCommand.User, "user", "u", "", "Sets the username or UID used and optionally the groupname or GID for the specified command") | |||
|
|
|||
| flags.IntVar(&execCommand.PreserveFDs, "preserve-fds", 0, "Pass N additional file descriptors to the container") | |||
There was a problem hiding this comment.
I think flatpak passes in the specific numbers of the FDs in question, and allows the flag to be specified multiple times. Not sure if compatibility with them is important here.
There was a problem hiding this comment.
I think we should rather look at what runc already does, it supports --preserve-fds, but only for create/run at the moment.
There was a problem hiding this comment.
If this is runc's behavior, matching it SGTM
| @@ -82,11 +85,34 @@ func execCmd(c *cliconfig.ExecValues) error { | |||
| return errors.Wrapf(err, "unable to exec into %s", args[0]) | |||
| } | |||
|
|
|||
| if c.PreserveFDs > 0 { | |||
| entries, err := ioutil.ReadDir("/proc/self/fd") | |||
There was a problem hiding this comment.
Can you use os.NewFile here, and directly pass the FD number? The docs say it will ensure a file wrapping a valid FD is returned, so we can use it to check if the FD in question is present
There was a problem hiding this comment.
I think it means it checks only that once casted it is not <0:
fmt.Printf("got: %v\n", os.NewFile(1, "f1"))
fmt.Printf("got: %v\n", os.NewFile(100000000000, "f2"))
fmt.Printf("got: %v\n", os.NewFile(9223372036854775808, "f3"))
I get:
got: &{0xc00006c180}
got: &{0xc00006c1e0}
got: <nil>
There was a problem hiding this comment.
Ah, so it's fake checking and the stdlib docs are lying, then.
It would have been neater than checking /proc, but if we can't do it this works fine.
|
/retest |
|
Do we no longer need this? |
|
we do, not sure how it got closed |
|
@giuseppe Does this require a new version of runc? |
yes, the PR implementing it was merged yesterday |
|
Ok so lets wait for v1.1 is shipped and then we can put this in master, and start moving runc through the process of getting it released. |
|
Needs to update RELEASE_NOTES.md with changes |
|
Nevermind the noise, don't bother with release notes |
|
☔ The latest upstream changes (presumably #2487) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm ready to merge this once the conflicts are fixed |
Allow to pass additional FDs to the process being executed. Closes: containers#2372 Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
rebased |
giuseppe
force-pushed
the
exec-preserve-fds
branch
from
4c35f76
to
0b34327
Compare
|
LGTM |
|
@mheon is this good to go? |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Allow to pass additional FDs to the process being executed.
Closes: #2372
Depends on: opencontainers/runc#1995
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com