Add fd section for linux container process by mrunalp · Pull Request #113 · opencontainers/runtime-spec

mrunalp · 2015-08-21T18:51:35Z

Closes #100
Signed-off-by: Mrunal Patel [email protected]

wking · 2015-08-21T19:13:27Z

I think we need to specify the mechanism for configuring which file
descriptors should be leaked (although this is a runtime-caller issue
more than a bundle-author issue, so maybe it deserves a different spec
1).

The bundle author might want a way to map file descriptor names to a
particular semantic interpretation (e.g. so you can tell the
application that fd 3 is the listening socket), but you could handle
that with environment variables (2 and opencontainers/runc#200), so
I don't think we need any bundle-author docs beyond this note that it
may be possible to get additional file descriptors (although it would
be nice to link to the runtime-caller docs explaining how to configure
which file descriptors are leaked).

philips · 2015-08-26T14:28:44Z

perhaps pass instead of leak?

also, I think we should settle on something other than "container process" as the word container means too many things today. "application" seems reasonable to me.

container process should be container's init process i think. Do we want to pass extra file descriptors for non-init process ?

@dqminh what is an "init" process? In my mind "init" is /sbin/init and I don't think the runtime should care whether it is a non-init or init process.

hmm i was thinking of init process as the first process that is running inside the container, as we can execute multiple processes over the lifetime of the container.

http://man7.org/linux/man-pages/man7/pid_namespaces.7.html calls it as the "init" process for the pid namespace. However, we could be in a situation where we don't use a new pid namespace.

i'm fine with calling it an application. I think we should standardize one term for it through out the specs. application sounds fine as user may want to run it inside a VM instead of container for example. Then process/init process is not so approriate.

Well, application implies that there is only one application being run in the container whereas in reality there could be many.

@mrunalp As defined today an OCI bundle only has a single application.

@philips Updated.

conqerAtapple · 2015-08-27T18:37:45Z

Since the use case here is running services inside containers with a mapped handle between the host and the container, why not use terminology like service, handles and host mapping?

wking · 2015-08-28T05:33:33Z

The wording changes from 3174a03→22ede9f look good to me.

Do we want to suggest environment variables 1 and/or external specs
(like 2) for associating semantic meaning with file descriptors? Or
are we happy to leave that up to spec readers to figure out on their
own?

wking · 2015-08-28T14:26:57Z

stdin, stdout, and stderr only seem to be attached between the runtime and the application when process.terminal is false. When it's true, the applications standard streams are attached to a TTY. See (1).

Yes, what those fds point to depends on the TTY setting, but the main point being that these are open for the application process.

On Fri, Aug 28, 2015 at 08:21:14AM -0700, Mrunal Patel wrote:

Yes, what those fds point to depends on the TTY setting, but the
main point being that these are open for the application process.

Ah, good point.

the standard streams should maybe not be in backticks. Alternatively,
these existing occurrences should be in backticks [1,2] ;).

mrunalp · 2015-08-28T18:24:42Z

@philips @crosbymichael @vbatts ping

crosbymichael · 2015-08-28T18:27:49Z

LGTM

LK4D4 · 2015-08-28T18:30:42Z

@mrunalp I'm sorta failing to understand how this closes #100. Shouldn't this include how you can pass additional file descriptors?

mrunalp · 2015-08-28T18:42:24Z

@LK4D4 That is left as an implementation detail to the runtime. For e.g. using environment variables like in opencontainers/runc#231. What we concluded was that having a serializable field in the spec json for ExtraFields doesn't make much sense as the fds and what values they have is dynamic and determined at runtime. What could make sense instead over there is passing fds for specific files.

LK4D4 · 2015-08-28T18:49:27Z

@mrunalp Hmm, I don't like implementation detail, why we need anything in spec then? Why this field can't be in runtime?

philips · 2015-08-28T18:57:16Z

LGTM

philips · 2015-08-28T18:58:18Z

I would like for the section to expand to explain that some of the fds may be opened but connected to /dev/null (like stdin).

mrunalp · 2015-08-28T19:04:02Z

@LK4D4 Yeah, so we aren't adding anything to the spec. Just saying that the runtimes may pass additional fds. If we want to standardize that then we may have to prescribe environment variables like OCI_LISTEN_FDS and OCI_LISTEN_FDS_START but they will still have to set from the application specific ones to make sense.

Signed-off-by: Mrunal Patel <[email protected]>

LK4D4 · 2015-08-28T20:16:34Z

LGTM

wking · 2015-08-29T08:03:07Z

/dev/null should probably be in backticks.

philips · 2015-08-29T15:33:04Z

Most everyone has LGTM'd this so I am going to fix the one nit about backticks around the /dev/null and merge.

philips · 2015-08-29T15:35:23Z

Merged via e3ee431

mrunalp · 2015-08-29T20:16:43Z

@philips Thanks!

This landed in runC with [1], but the bundle-author <-> runtime specs explicitly avoid talking about how this is set (since the bundle-author doesn't care about the runtime-caller <-> runtime interface) [2]. However, *this* spec is about the runtime-caller <-> runtime interface, so we need to document it here. I've left LISTEN_PID [3,4] out, since I don't see how the runtime-caller would choose anything other than 1 for its value. It seems like something that a process would have to set for itself (because guessing the PID of a child before spawning it seems racy ;). In any event, the runC implementation seems to set this to 1 regardless of what systemd passes to it [4]. I've borrowed Shishir's wording for the example [4]. [1]: opencontainers/runc#231 [2]: opencontainers/runtime-spec#113 (comment) [3]: http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html [4]: opencontainers/runc#231 (comment) Signed-off-by: W. Trevor King <[email protected]>

mrunalp mentioned this pull request Aug 21, 2015

Add extra file descriptors to Process #100

Closed

philips reviewed Aug 26, 2015
View reviewed changes

mrunalp force-pushed the linux_fds branch from 3174a03 to 22ede9f Compare August 27, 2015 21:27

wking mentioned this pull request Aug 28, 2015

Add runtime state configuration and structs #87

Merged

wking reviewed Aug 28, 2015
View reviewed changes

Add fd section for linux container process

0b5324b

Signed-off-by: Mrunal Patel <[email protected]>

mrunalp force-pushed the linux_fds branch from 22ede9f to 0b5324b Compare August 28, 2015 19:51

wking reviewed Aug 29, 2015
View reviewed changes

Comment thread runtime-linux.md

Copy link
Copy Markdown

Contributor

wking Aug 29, 2015

Choose a reason for hiding this comment

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

/dev/null should probably be in backticks.

philips closed this Aug 29, 2015

wking mentioned this pull request Aug 30, 2015

Adjust filesnames to match associated JSON #126

Closed

wking pushed a commit to wking/opencontainer-runtime-spec that referenced this pull request Jul 25, 2016

Conversation

mrunalp commented Aug 21, 2015

Uh oh!

wking commented Aug 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

conqerAtapple commented Aug 27, 2015

Uh oh!

wking commented Aug 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Aug 28, 2015

Uh oh!

crosbymichael commented Aug 28, 2015

Uh oh!

LK4D4 commented Aug 28, 2015

Uh oh!

mrunalp commented Aug 28, 2015

Uh oh!

LK4D4 commented Aug 28, 2015

Uh oh!

philips commented Aug 28, 2015

Uh oh!

philips commented Aug 28, 2015

Uh oh!

mrunalp commented Aug 28, 2015

Uh oh!

LK4D4 commented Aug 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philips commented Aug 29, 2015

Uh oh!

philips commented Aug 29, 2015

Uh oh!

mrunalp commented Aug 29, 2015

Uh oh!