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

Add extra file descriptors to Process #100

Closed
wants to merge 1 commit into from

Conversation

mrunalp
Copy link
Contributor

@mrunalp mrunalp commented Aug 6, 2015

It should be possible to pass file descriptors to the container process to support use cases
like systemd socket activation.

Signed-off-by: Mrunal Patel mrunalp@gmail.com

@@ -53,6 +57,8 @@ type Process struct {
// Cwd is the current working directory for the process and must be
// relative to the container's root.
Cwd string `json:"cwd"`
// ExtraFiles specifies open file descriptors that should be passed to the process.
ExtraFiles []*os.File `json:"extraFiles"`
Copy link
Contributor

Choose a reason for hiding this comment

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

is *os.File really serializing to int?

@philips
Copy link
Contributor

philips commented Aug 6, 2015

@mrunalp Why does this need to be in the config though?! Wouldn't they need to be passed through the runtime?

@wking
Copy link
Contributor

wking commented Aug 6, 2015

On Thu, Aug 06, 2015 at 02:18:53PM -0700, Mrunal Patel wrote:

It should be possible to pass file descriptors to the container
process to support use cases like systemd socket activation.

I think Windows uses opaque pointers instead of integer file
descriptors, so it's probably worth pulling in a Windows dev before
pushing this into the cross-platform portion of the spec. Or just
check and see if Go's *os.File is actually an integer on all platforms
(or on any platform? *os.File sounds like a stream pointer on POSIX).

@wking
Copy link
Contributor

wking commented Aug 6, 2015

On Thu, Aug 06, 2015 at 02:23:18PM -0700, Brandon Philips wrote:

@mrunalp Why does this need to be in the config though?! Wouldn't
they need to be passed through the runtime?

Presumably you'd write your bundle config with the understanding that
consumers would launch the runtime with the file you cared about bound
to a particular file descriptor.

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 6, 2015

@philips We can't figure out which fds to pass down in the runtime without them being specified in the config.

@LK4D4
Copy link
Contributor

LK4D4 commented Aug 6, 2015

Yup, it's again this host-dependency problem :/ Like we can't expect this FDs to exist on each host.

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 6, 2015

@LK4D4 Yes, it is, but I think we can hash that out in the other PR.

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 6, 2015

@taylorb-microsoft Could you comment on how this would work on Windows? Thanks!

Signed-off-by: Mrunal Patel <mrunalp@gmail.com>
@wking
Copy link
Contributor

wking commented Aug 6, 2015

On Thu, Aug 06, 2015 at 02:32:15PM -0700, Alexander Morozov wrote:

Yup, it's again this host-dependency problem :/ Like we can't expect
this FDs to exist on each host.

There's no clear line where a container becomes infinitely portable
between all hosts. On that sliding scale, requiring the runtime to
have bundle-expected files bound to bundle-specified descriptors is a
reasonably serious consideration, but in my opinion less serious than
requiring absolute-path executables on the host 1. I imagine that a
bundle using this feature would be distributed with (for example) a
systemd rule for launching with the appropriate file descriptors, and that
would be portable enough.

@@ -53,6 +57,8 @@ type Process struct {
// Cwd is the current working directory for the process and must be
// relative to the container's root.
Cwd string `json:"cwd"`
// ExtraFiles specifies open file descriptors that should be passed to the process.
ExtraFiles []os.File `json:"extraFiles"`

Choose a reason for hiding this comment

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

@mrunalp
This might be ExtraFiles []*os.Filejson:"extraFiles"``

Copy link
Member

Choose a reason for hiding this comment

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

since this is supposed to be a config that is serialized I highly doubt this will work at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crosbymichael @philips This feature makes more sense as a dynamic property. We could have an environment variable passed to the runtime that could be a list of open fds that it should leak into the container process. For e.g.

sh# OCI_PASS_FDS="3,4" runc 4</path/to/some/file # 3 is passed by systemd

WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

what is your use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@crosbymichael Systemd socket activated containers http://0pointer.de/blog/projects/socket-activated-containers.html
We could either make this generic or leave it out and have the runtimes support systemd environment variables for activation.

Copy link
Member

Choose a reason for hiding this comment

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

i think having runtime support for LISTEN_FDS and doing passthrough would be better than adding this runtime information in the spec that is to be serialized.

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Aug 21, 2015 at 11:16:46AM -0700, Michael Crosby wrote:

  • // ExtraFiles specifies open file descriptors that should be passed to the process.
  • ExtraFiles []os.File json:"extraFiles"

i think having runtime support for LISTEN_FDS and doing
passthrough would be better than adding this runtime information in
the spec that is to be serialized.

You're still serializing those file descriptors into the environment
variable, no? With #88 and the LISTEN_FDS suggestion, that would mean
the runtime would have to check:

  • The static config.json
  • The more dynamic runtime.json
  • The LISTEN_FDS environment variable

to figure out how to create the container and launch the application.
Personally, I see no problem with mutating a single config to launch
the container (so the runtime only has to look in one place) [1,2].
But if we do end up getting a second place to put any configuration
considered “too host-specific for config.json”, then I don't see a
need to go further than a runtime.json.

Copy link
Member

Choose a reason for hiding this comment

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

@wking no

Copy link
Contributor

Choose a reason for hiding this comment

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

On Fri, Aug 21, 2015 at 11:25:04AM -0700, Michael Crosby wrote:

  • // ExtraFiles specifies open file descriptors that should be passed to the process.
  • ExtraFiles []os.File json:"extraFiles"

@wking no

Then why not use environment variables for all runtime-specific
settings? If there is a need to use all of:

how do we make decisions about sorting between them? E.g. why use
LISTEN_FDS for passing file descriptors through and runtime.json to
pass namespace paths through?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thecloudtaylor
Copy link

Sorry for the delay, my team is super heads down for the next week or so... The closest proxy in windows to a file descriptor would be a file handle. Which can represent a reference to a file, named pipe, socket, device etc... You generally wouldn't specify a file handle as a configuration parameter, a handle is created by the process using a device descriptor (something like a file name, device object path, IP address etc...)

The general expectation I would have as a user though for accessing these types of resources would be to specify them by type so for example if I wanted to connect a named pipe to a container I would expect to provide a config option/flag/ect.... in the form of NamedPipe=.\pipe\foobar not as a generic handle.

Hopefully that helps (again apologies that we have not been as active as we'd like to be - looking to address that soon).

-Taylor

@philips
Copy link
Contributor

philips commented Aug 12, 2015

@mrunalp Could you make the spec more explicit about how file descriptor passing works in practice between parent and child? Language such as:

List of open file descriptors that are passed to the application from the runtime. If empty then all file descriptors are closed. (What happens to stdout/stderr/stdin?!). If there are open file descriptors passed to the runtime outside of this list they are closed before the application is exec'd.

The two obvious use cases are attaching stderr/stdout/stdin to a terminal and systemd socket activation so it would be good to call them out explicitly and even provide an example, I think.

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 21, 2015

@philips @crosbymichael Opened #113

@wking
Copy link
Contributor

wking commented Aug 30, 2015

On Fri, Aug 21, 2015 at 11:52:07AM -0700, Mrunal Patel wrote:

@philips @crosbymichael Opened #113

#113 took the “don't specify how the runtime knows which FDs to pass”
approach [1,2]. Since #113 has landed, maybe we can close this PR?

@mrunalp
Copy link
Contributor Author

mrunalp commented Aug 30, 2015

Closing this for now, however, we may want to pursue a more limited version that allows passing fds to files specified by paths on the host if there is any demand for such a feature.

@mrunalp mrunalp closed this Aug 30, 2015
@wking wking mentioned this pull request Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants