Add docs for terminals #1730

deitch · 2018-02-22T09:35:40Z

As discussed with @cyphar , a first attempt to document how to use terminals with runc. See discussion here particularly this comment

I am sure there are some errors here, so happy to take updates.

cyphar · 2018-02-23T07:28:01Z

I will take a thorough look over this when I get back from vacation.

deitch · 2018-03-14T07:04:55Z

Hey @cyphar . How was vacation/holiday? Did I capture it well? Happy to make the next person to walk down my path have an easier walk. :-)

cyphar

My main concern with the current state of this is that there are some assumptions you've made (see inline comments for details) that aren't quite right.

You've also exclusively focused on the shell usecase -- which is important -- but feels like the wrong way of explaining this to me. Really, shells are the exception (especially the part where you talk about interspersed output and indeterminate input race conditions) and make it hard to understand what is meant to be happening.

The rest of it looks good though.

Here is what I'd recommend doing:

Move the parts that talk about shell-specific details to a warning section at the bottom of the document (or in a block quote with a NOTE).
I would introduce "new terminal" first and then discuss pass-through as being "not new terminal". In particular, with non-detached mode runc's pass-through is more like a pipe than a pass-through (so the description you give first confuses the later clarification you have).
I'm not sure if you saw the security worries about using pass-through (detached) in a shell, but please include those (if you need clarification on them feel free to ping me). This is actually the most significant issue with using detached pass-through in a shell.

cyphar · 2018-03-19T01:16:35Z

docs/terminals.md

+* new terminal
+
+### Pass-Through
+In pass-through mode, the `stdio` passed to the `runc` call will be passed as is to the primary container. This means that if you do the following:


This isn't totally accurate. In the case of non-detached, the stdio passed to the container are actually pipes (which are then read from by the still-running runc process). The reason this might be important to someone is so they know that fd=2 in your example isn't actually the file /tmp/log.out.

I am not sure I knew that, although it makes sense. Updating.

cyphar · 2018-03-19T01:17:16Z

docs/terminals.md

+
+Then the stdout of `some_container` will appear in the file `/tmp/log.out`, the stderr will appear in `/tmp/log.err`, and the process will receive input of `input`. `runc`'s stdout was redirected to `/tmp/log.out`, and `runc`, in turn, passed that handle to the container itself.
+
+To invoke pass-through mode, configure your container's `config.json` with `terminal: false` (which is the default).


It's not the default if you use runc spec.

Well, it still is the default if no terminal: setting is specific. runc spec just happens to actually specify it in the config.json. But I don't mind updating it.

cyphar · 2018-03-19T01:19:03Z

docs/terminals.md

+You can use either terminal mode with either runc mode. However, there are considerations that may indicate preference for one mode over another.
+
+### Foreground
+When running in foreground, `runc` and the container you invoke are most like running a regular foreground process. This is the scenario most likely to be useful for running in pass-through terminal mode. As the example above showed:


This is the scenario most likely to be useful for running in pass-through terminal mode.

Unfortunately it's not as simple as this. It really depends on how you're using pass-through. If you want to set up your own stdio then actually detached makes more sense in a lot of cases (unless you don't want to bother with writing the exit-code handling).

My inclination would be to leave this part. It doesn't say "only", but rather "most likely". I am coming at this as someone who hadn't messed around with runc before, banged my head on the wall to understand, and am trying to save others the headache. Simplifications, along with options to get deeper, are the best way to explain it to people IMHO.

The problem is that in this case (foreground and "pass-through", runc creates a pipe that is used for stdio). So the description isn't quite right. However, this is quite ... nuanced and if you like I can reword it after it's merged so you don't have to keep doing drafts.

Please do. I think I need to see how you rewrite it once it is in.

cyphar · 2018-03-19T01:21:05Z

docs/terminals.md

+When running runc detached with terminal Pass-Through, the stdin/stdout/stderr of the calling process will be passed to `runc` and from there to the container's process. However, since `runc` detaches, there are several side effects to take into account:
+
+* The stdout/stderr of the process can become intermixed with the stdout/stderr of the calling process.
+* Any input into the calling process can cause indeterminate issues with which process actually receives the input.


These notes appear to only be meaningful for shell users (and quite a few other points also seem quite shell-focused). Really this should just be a caution to not use pass-through detached in a shell -- rather than a warning about pass-through detached in general.

Is that true? I could have a process with its own stdout/stderr/stdin that is not a shell, and if it calls runc detached with pass-through, then the intermixing will happen, and anything fed into stdin will be indeterminate. Granted, stdin is used most often in shells, but it is not exclusive. I think the issue you described to me (slack? wherever it was...) actually is a general one.

That's fair enough.

cyphar · 2018-03-19T01:22:02Z

docs/terminals.md

+
+Further, a third, possibly temporary problem exists. Pass-Through Detached only works correctly when stdout/stderr are either an actual terminal or a file. If you use _anything_ else, e.g. a pipe, it will hang, for reasons unknown. There is an [open issue](https://github.com/opencontainers/runc/issues/1721) on this.
+
+For this reason, we **strongly recommend** using New Terminal mode when running `runc` detached.


... unless you set up your own stdio outside the container.

Good point. Fixing.

cyphar · 2018-03-19T01:22:51Z

docs/terminals.md

+3. Listen on the socket to get the master file descriptor
+4. Use that file descriptor to control stdio
+
+Note the one shortcoming: the single descriptor means that you have mixed stdout/stderr. There is no way to separate them. Essentially, this is a _console_, which has no separate stdout and stderr.


(Though this shortcoming is identical to regular terminal emulators -- take a look at /proc/self/fd/[12] in your shell right now.)

Of course. It really is only something you can separate when you start a new process with separate descriptors. Happy to add a comment here.

cyphar · 2018-03-19T01:24:05Z

docs/terminals.md

+
+* `0`: standard-in (`stdin`), the input stream into the process
+* `1`: standard-out (`stdout`), the output stream from the process
+* `2`: standard-error (`stderr`), the error stream from the process


I would recommend at least mentioning --preserve-fds here, with the note that it is separate to all of this handling for stdio and that those file descriptors are passed verbatim in all cases.

OK. I just added something at the end.

deitch · 2018-03-19T07:46:19Z

@cyphar I updated with most of the changes requested; have another look.

I'm not sure if you saw the security worries about using pass-through (detached) in a shell, but please include those (if you need clarification on them feel free to ping me). This is actually the most significant issue with using detached pass-through in a shell.

Please do clarify?

deitch · 2018-04-10T11:20:09Z

@cyphar any update?

deitch · 2018-05-06T08:09:44Z

Hi @cyphar ; this is almost 2 months old. Can we review the changes and get it in?

cyphar · 2018-05-06T12:20:40Z

docs/terminals.md

+```
+
+## Other File Descriptors
+Completely separately from stdio - file descriptors `0`,`1`,`2` - you also can pass other file descriptors to the container process "as is" using `--preserve-fds <descriptor>`. Thus, for example, if your calling process has an open handle to the file `/var/foo` as file descriptor `5`, and you want to pass it to your container process, you can call `runc`:


This description is not quite correct -- --preserve-fds 5 means that all file descriptors from 3 to 5 (inclusive) are preserved. The way this is currently written it looks like you're saying that only 5 is preserved.

Fixing now.

cyphar · 2018-05-06T12:24:39Z

docs/terminals.md

+$ echo input | runc run some_container > /tmp/log.out 2>& /tmp/log.err
+```
+
+Nonetheless, you still have the option, when running in foreground runc mode, of using New Terminal mode. However, if the process is short-lived, you will not have much time to retrieve the console's fds from the socket and doing anything with it.


... for instance this last sentence is inaccurate because you cannot use --console-socket with foreground mode (and so from the host management perspective there's no difference between the two terminal modes in foreground -- though there is a difference in the container). But as I said above, I can fix this up separately.

OK, please do.

I would prefer to remove the inaccurate part if we like to merge this now.

cyphar · 2018-05-06T12:25:44Z

Sorry for the long wait. I've added some comments. However, I have a feeling at this point we might as well merge it (with the --preserve-fds section fixed) and I can reword the relevant parts in my own time (since I'm sure it's annoying to re-send drafts).

cyphar · 2018-05-06T12:25:54Z

Eugh, sorry. Wrong button.

deitch · 2018-05-06T15:28:42Z

However, I have a feeling at this point we might as well merge it (with the --preserve-fds section fixed) and I can reword the relevant parts in my own time (since I'm sure it's annoying to re-send drafts).

Sure. I will do an immediate update to the best of my knowledge and send it back to you.

deitch · 2018-05-06T15:34:24Z

My one request, please add your patches soon after merging? I finally understand most of it, want to grasp the entire thing! :-)

deitch · 2018-05-06T15:35:10Z

Updated the --preserve-fds comment. Back at you @cyphar .

cyphar · 2018-06-15T03:08:07Z

LGTM. I will do a rewrite of the relevant parts after it's merged.

/cc @opencontainers/runc-maintainers

cyphar · 2018-06-15T03:08:43Z

PullApprove appears to be broken? /cc @caniszczyk

caniszczyk · 2018-06-15T13:54:28Z

LGTM

caniszczyk · 2018-06-15T13:55:00Z

@cyphar weird try to LGTM again

dqminh · 2018-06-15T14:56:39Z

@cyphar does it make more sense to just commit directly into the branch instead of us having to merge with some incompleted / incorrect parts ?

dqminh

Thanks for doing this ! I just have a few nits.

dqminh · 2018-06-15T14:57:57Z

docs/terminals.md

+lrwx------    1 root     root            64 Mar 19 07:09 /proc/self/fd/2 -> /0
+```
+
+The following sample code, partially taken from the reference implementation [recvtty](https://github.com/opencontainers/runc/blob/master/contrib/cmd/recvtty/recvtty.go), shows how to accept a file descriptor for the socket. It does not contain the usual `err` handling and `defer`red closing.


i would prefer to just point directly to recvtty.go rather than attaching sample code here so there are one less place to keep change.

dqminh · 2018-06-15T14:59:40Z

docs/terminals.md

+$ echo input | runc run some_container > /tmp/log.out 2>& /tmp/log.err
+```
+
+Nonetheless, you still have the option, when running in foreground runc mode, of using New Terminal mode. However, if the process is short-lived, you will not have much time to retrieve the console's fds from the socket and doing anything with it.


I would prefer to remove the inaccurate part if we like to merge this now.

cyphar · 2018-06-15T15:47:45Z

@dqminh Ah, I didn't even check if @deitch has given us push access. I'll send some corrections to his branch then re-LGTM it.

deitch · 2018-06-15T16:23:19Z

I did give push access IIRC. I always open PRs that way.

cyphar · 2018-06-24T13:32:30Z

@deitch I have done a fairly extensive rewrite of the document, and now I'm pretty happy with how it stands. Since it still has your name on it, I'd like your blessing before we merge it -- does the current document help explain how things work?

/cc @opencontainers/runc-maintainers

cyphar · 2018-06-24T13:32:56Z

LGTM (obviously) -- but wait for @deitch's response before merging.

deitch · 2018-06-24T13:33:51Z

I'd like your blessing before we merge it

Aleksa is the expert here! Not sure asking my opinion makes sense... :-)

I will do a quick review.

deitch · 2018-06-24T13:38:55Z

docs/terminals.md

+In addition to `--preserve-fds`, `LISTEN_FDS` file descriptors are passed
+automatically to allow for `systemd`-style socket activation. They always take
+up the first file descriptors after `stdio` (and `--preserve-fds` always follow).
+To disable this, simply unset `LISTEN_FDS`.


what happens if LISTEN_FDS are set and --preserve-fds? An example like the above might help?

By default, isn't LISTEN_FDS unset anyways?

What happens if LISTEN_FDS are set and --preserve-fds? An example like the above might help?

The first LISTEN_FDS file descriptors come first, then the --preserve-fds ones (the --help for runc run explains it a bit). I could try to add an example, but LISTEN_FDS is a bit difficult to given an example of without having a systemd service and all the rest of that fun stuff.

By default, isn't LISTEN_FDS unset anyways?

Yup, but it is important to mention (because it's quite related to --preserve-fds).

That helps, thanks.

deitch · 2018-06-24T13:42:02Z

docs/terminals.md

+passing of file descriptors -- [details below](#runc-modes)). As an example:
+
+```
+% echo input | runc run some_container > /tmp/log.out 2>& /tmp/log.err


Hmm... isn't runc's default mode terminal: true?

Yup, though in this section I thought it was implied. I can add a one-liner about it.

deitch · 2018-06-24T13:44:36Z

docs/terminals.md

+to the container's `stdio`. The purpose of this option is to allow a user to
+set up `stdio` for a container themselves and then force `runc` to just use
+their pre-prepared `stdio` (without any pseudo-terminal funny business). *If
+you don't see why this would be useful, don't use this option.*


I love this line!

deitch · 2018-06-24T13:46:18Z

docs/terminals.md

+> path name.
+
+In order to help users make use of detached new terminal mode, we have provided
+a [Go implementation in the `go-runc` bindings][containerd/go-runc.Socket].


This reference is very helpful.

deitch · 2018-06-24T13:48:15Z

docs/terminals.md

+to a Unix domain socket which `runc` will connect to and send the
+pseudo-terminal master file descriptor down (after which `runc` will exit and
+the only holder of the pseudo-terminal master file descriptor will be whatever
+process read the file descriptor from the socket).


Maybe add a paragraph that says something like the following?

So to use runc in "Detached New Terminal" mode:

Set up a unix domain socket

Run runc in detached new terminal mode, passing it the path to the socket as the argument to --console-socket

Retrieve the file descriptor for the new pseudo-terminal from the socket

Use the file descriptor to communicate with stdio of the container, now running detached

Sure, I'll explain what the binding does (roughly).

deitch · 2018-06-24T13:49:57Z

Thanks @cyphar , really better. I made one small suggestion, and 1-2 places I was a bit confused.

In the end, this is complex, so it will be confusing. But this is a really good starting point.

cyphar · 2018-06-24T17:23:57Z

@deitch Fixed, PTAL.

deitch

Just that one typo. Other than that, really great.

deitch · 2018-06-24T18:17:23Z

docs/terminals.md

+1. Create a unix domain socket at some path, `$socket_path`.
+2. Call `runc run` or `runc create` with the argument `--console-socket
+   $socket_path`.
+3. Using `recvmsg(2)` retreive the file descriptor sent using `SCM_RIGHTS` by


One nit: retrieve instead of retreive on the above line.

Users can get very confused by how terminals work with runc, and the quite confusing "terminal: ..." option. Add a document which goes through all of the important parts of terminal handling in runc, in the hopes that we can just point people to this as an explanation. Signed-off-by: Avi Deitcher <[email protected]> [cyphar: quite a large rewrite to fix factual errors and structure] Co-authored-by: Avi Deitcher <[email protected]> Signed-off-by: Aleksa Sarai <[email protected]>

cyphar · 2018-06-24T19:35:29Z

LGTM.

/cc @opencontainers/runc-maintainers

crosbymichael · 2018-06-25T15:22:34Z

LGTM

deitch · 2018-06-25T15:27:08Z

👍

deitch force-pushed the document-terminal branch from 39f6df7 to 4149ab5 Compare February 22, 2018 11:40

This was referenced Feb 23, 2018

User Interface Question #1732

Closed

runc create hangs when passed a pipe for stdout or stderr #1721

Open

cyphar self-assigned this Feb 23, 2018

cyphar self-requested a review February 23, 2018 07:29

cyphar requested changes Mar 19, 2018

View reviewed changes

cyphar reviewed May 6, 2018

View reviewed changes

cyphar closed this May 6, 2018

cyphar reopened this May 6, 2018

deitch force-pushed the document-terminal branch from 76e5a24 to a52b51f Compare May 6, 2018 15:34

cyphar approved these changes Jun 15, 2018

View reviewed changes

dqminh reviewed Jun 15, 2018

View reviewed changes

cyphar force-pushed the document-terminal branch from a52b51f to b258423 Compare June 24, 2018 13:29

deitch commented Jun 24, 2018

View reviewed changes

cyphar force-pushed the document-terminal branch from b258423 to 3276782 Compare June 24, 2018 17:23

deitch commented Jun 24, 2018

View reviewed changes

cyphar force-pushed the document-terminal branch from 3276782 to 472fcb3 Compare June 24, 2018 19:35

crosbymichael merged commit 0d05939 into opencontainers:master Jun 25, 2018

deitch deleted the document-terminal branch June 25, 2018 15:27

thaJeztah mentioned this pull request Aug 23, 2018

Update containerd client and dependencies to v1.2.0-rc.1 moby/moby#37710

Merged


		Then the stdout of `some_container` will appear in the file `/tmp/log.out`, the stderr will appear in `/tmp/log.err`, and the process will receive input of `input`. `runc`'s stdout was redirected to `/tmp/log.out`, and `runc`, in turn, passed that handle to the container itself.

		To invoke pass-through mode, configure your container's `config.json` with `terminal: false` (which is the default).


		Further, a third, possibly temporary problem exists. Pass-Through Detached only works correctly when stdout/stderr are either an actual terminal or a file. If you use _anything_ else, e.g. a pipe, it will hang, for reasons unknown. There is an [open issue](https://github.com/opencontainers/runc/issues/1721) on this.

		For this reason, we strongly recommend using New Terminal mode when running `runc` detached.

Add docs for terminals #1730

Add docs for terminals #1730

Conversation

deitch commented Feb 22, 2018

cyphar commented Feb 23, 2018

deitch commented Mar 14, 2018

cyphar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deitch commented Mar 19, 2018

deitch commented Apr 10, 2018

deitch commented May 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented May 6, 2018

cyphar commented May 6, 2018

deitch commented May 6, 2018

deitch commented May 6, 2018

deitch commented May 6, 2018

cyphar commented Jun 15, 2018

cyphar commented Jun 15, 2018

caniszczyk commented Jun 15, 2018 • edited Loading

caniszczyk commented Jun 15, 2018

dqminh commented Jun 15, 2018

dqminh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jun 15, 2018

deitch commented Jun 15, 2018

cyphar commented Jun 24, 2018

cyphar commented Jun 24, 2018 • edited by caniszczyk Loading

deitch commented Jun 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deitch commented Jun 24, 2018

cyphar commented Jun 24, 2018

deitch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar commented Jun 24, 2018 • edited by caniszczyk Loading

crosbymichael commented Jun 25, 2018

deitch commented Jun 25, 2018

cyphar left a comment •

edited

Loading

caniszczyk commented Jun 15, 2018 •

edited

Loading

cyphar commented Jun 24, 2018 •

edited by caniszczyk

Loading

cyphar commented Jun 24, 2018 •

edited by caniszczyk

Loading