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.

Sign up for GitHub

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

Add --preserve-file-descriptors=N to create #1320

Merged
merged 1 commit into from
Feb 24, 2017
Merged

Add --preserve-file-descriptors=N to create #1320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 4 additions & 0 deletions create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ command(s) that get executed on start, edit the args parameter of the spec. See
Name: "no-new-keyring",
Usage: "do not create a new session keyring for the container. This will cause the container to inherit the calling processes session key",
},
cli.IntFlag{
Name: "preserve-fds",
Usage: "Pass N additional file descriptors to the container (stdio + $LISTEN_FDS + N in total)",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we also add this to run.go? They almost have same options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that would make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They were even similar enough that git show create.go | patch run.go did the right thing ;-)

},
Action: func(context *cli.Context) error {
if err := checkArgs(context, 1, exactArgs); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ command(s) that get executed on start, edit the args parameter of the spec. See
Name: "no-new-keyring",
Usage: "do not create a new session keyring for the container. This will cause the container to inherit the calling processes session key",
},
cli.IntFlag{
Name: "preserve-fds",
Usage: "Pass N additional file descriptors to the container (stdio + $LISTEN_FDS + N in total)",
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to have this sort of API, I would prefer that we make it more explicit than this. For example, make it so that you have to specify which file descriptors you want to pass (IntSliceFlag). This would also mean that cases where you don't want to use low file descriptors (see this SQLite option for more reasons why that might matter).

Copy link
Member

Choose a reason for hiding this comment

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

So it would look like

runc run --preserve-fd 512 --preserve-fd 1337 ctr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current interface doesn't allow you to pass through fds 0, 1, or 2 in this way so I think the SQLlite reasoning for avoiding those fds doesn't really apply here.

I can take a look switching things to behave like you suggest but it would have to be contingent on Go's os/exec Cmd.ExtraFiles supporting the necessary semantics (i.e. a bug slice full of nils apart from at offset 512 and 1337 in your example). Personally I don't think it is terribly useful to be able to pass arbitrary fds, it's pretty easy (and arguably more controlled) for the caller to dup2 things into the right place after fork but before exec of runc.

Copy link
Member

Choose a reason for hiding this comment

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

For this i'm not sure defining individual fds is optimal. If you have a large amount of fds to pass you would run out of space for all the --preserve-fd 3 --preserve-fd 4 --preserve-fd 5...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I was a bit surprised that IntSliceFlag didn't support comma separated lists or even N-M spans (or if it does that the syntax wasn't more obvious...).

FWIW I did confirm that Cmd.ExtraFiles looks on first glance to DTRT with nil in the list, although finalizeNamespace and CloseExecFrom would probably need extending to handle a sparse fd space to be absolutely safe.

},
},
Action: func(context *cli.Context) error {
if err := checkArgs(context, 1, exactArgs); err != nil {
Expand Down
8 changes: 8 additions & 0 deletions utils_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ type runner struct {
shouldDestroy bool
detach bool
listenFDs []*os.File
preserveFDs int
pidFile string
consoleSocket string
container libcontainer.Container
Expand All @@ -196,11 +197,17 @@ func (r *runner) run(config *specs.Process) (int, error) {
r.destroy()
return -1, err
}

if len(r.listenFDs) > 0 {
process.Env = append(process.Env, fmt.Sprintf("LISTEN_FDS=%d", len(r.listenFDs)), "LISTEN_PID=1")
process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...)
}

baseFd := 3 + len(process.ExtraFiles)
for i := baseFd; i < baseFd+r.preserveFDs; i++ {
process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i)))
}

rootuid, err := r.container.Config().HostUID()
if err != nil {
r.destroy()
Expand Down Expand Up @@ -353,6 +360,7 @@ func startContainer(context *cli.Context, spec *specs.Spec, create bool) (int, e
consoleSocket: context.String("console-socket"),
detach: context.Bool("detach"),
pidFile: context.String("pid-file"),
preserveFDs: context.Int("preserve-fds"),
create: create,
}
return r.run(&spec.Process)
Expand Down