Should Terminal be in the process spec?

[crosbymichael]

We have had this for a while and I'm starting to question if this is the best place for this. Because of the console work and much of this has to be coordinated outside of the runtime and container I don't know if the spec is the right place to have this.

As an example, the shim with containerd, we have to create the socket and wait to get the pty master back from the runtime, the only way the shim would know if it is supposed to create this socket and wait is if:

a. the higher level pass down the information that you should get a console from the container
b. we read the spec

Reading the spec id kinda weird when the higher layers make the spec and give it to you, you open and unmarshal it to figureout this one field, then hand it off to the runtime.

It is more ideal if the higher levels just say, "hey i want a terminal with this container" and the shim can just launch runc with --tty or --console-socket and if this is specified then runc creates one, if not, it does not. This information is not split between the spec and higher levels.

What do you all think? Have you ran into this same issue implementing the spec or high level systems with runc or other runtimes?

@mlaventure wdyt?

[mlaventure]

I personally think that the Terminal is not a property of a process, it's more the "environment" in which it runs.

All binaries that cares about having a terminal check at startup what's their current IOs points to.

So 👍 from me to move outside the spec and let the users/tools provide a console if so desired.

[wking]

I'm fine dropping it from the config, but think the command-line API should cover how to set up a terminal (since we still want the runtime doing the terminal-creation inside the container namespace). And this issue may be a dup of #494, where @crosbymichael floated dropping the terminal property in a comment.

[tianon]

@RobDolinMS can you comment regarding Windows implications? (especially since "console size" is something that was added to the spec explicitly for Windows initially, IIRC)

IMO, it is kind of nice to be able to have the bundle author note that the process they've configured as the "container process" requires a TTY, but can definitely see the value from the runc perspective on punting that to the higher layers (since it needs to be able to coordinate, as you described); definitely think it'd be good to get comments from other runtime authors actually implementing the spec (mostly just wanted to ping Rob for explicit Windows comments but figured I might as well throw in my own thoughts while I was at it).

[hqhq]

So 👍 from me to move outside the spec and let the users/tools provide a console if so desired.

Now we create console in container's namespace, not provided by users/tools (which turned out to be wrong and could cause some weird issues), so it makes more sense to me than before that terminal be a property of process.

[mlaventure]

@hqhq The console can still be created within the container. I'm just in favor of having the option to do so to be triggered by a flag of the runtime instead of it being a field within the spec.

[crosbymichael]

@hqhq ya, its more about who triggers this creation. Since you have to coordinate with the runtime to accept the fd via socket it has to know about it.

[hqhq]

@mlaventure We can do both, like what runc exec does today, I think we can leave that to implementations.

@crosbymichael It can always be the upper level tools or config authors or administrators who trigger this creation, I think it's about how we trigger this creation, assigned by a flag from upper level tools and written to the spec, then runc read the spec and decide to create console or not seems reasonable to me, like all other container properties.

After a second thought, I agree this property might not belong to the process, process only sees stdio and don't have to know how the backend works, but I still think it should be part of container's properties, especially when it's always created by the container. And how it'll be coordinated with upper level tools can be an implementation detail.

[cyphar]

On the one hand, I would like it if we could remove all descriptions of consoles from the spec. It's "just" stdio after all. And this would make a lot of things much simpler to handle...

Unfortunately consoles aren't "just" stdio. Even the trivial case of "what is the size of the terminal I'm writing to" is only supported if you're writing to a console. Resizing doesn't exist for pipes as stdio. There's also the security concern about pipes, not to mention the blocking semantics and all of the other things that happen as a result of interacting with consoles vs pipes (or files).

The biggest thing in my mind is that if we punt on how consoles are handled within the spec ("it's the job of higher layers to provide these interfaces") it now means that a user of the spec cannot be sure what stdio interface they will be able to see! For curses applications, this means that you won't be able to actually provide a GUI because you don't know how to find out the terminal size.

However, I do agree that the current situation is a problem, I'm just unsure how to solve it.

[crosbymichael]

I'm not saying remove the concept at all, i'm saying remove it from the spec and move it to a runtime flag. Basically if we still have to pass flags at runtime for --console-socket then why do we need this terminal information in the spec as well? Its just duplicate information.

Runc still has to know about it either way. It should be runc create --tty or runc run -t because the caller, especially on create, has to provide --console-socket anyways or else it won't work at all.

The caller needs to know that the spec was create with terminal=true by either having the spec creator tell it or it has to read the spec to figure it out. Its much better if we only read the spec once, via runc(or a runtime) and the caller of runc can decide if the container should have a console or not because it has to facilitate receiving on a socket anyways if its going to be useful.

[dqminh]

Do you think there's value in letting container specify that it must require a console for it to work ? Something like:

• if terminal=true, then runtime must allocate and setup a console correctly.
• otherwise either terminal is unset or false ( abit weird here ), runtime can optionally setup a console (i.e. higher layer decides )

[jlbutler]

I agree with the intent of this issue and also agree that whether a TTY is allocated for the process or not seems to be an aspect of the environment in which that process runs. However, this seems more complicated because subsequent Process structs may define additional processes to add to an existing container, and those may or may not require a TTY be allocated (e.g. a container running a daemon process has an interactive shell process added to it). Given that, I believe we have to maintain some per-process metadata regarding tty/console, rather than generalizing for a container environment.

Above references to runtime options seem like a simple way to resolve this, from an implementation perspective, as every invocation can indicate whether or not a TTY is required and it needn't worry about spec metadata. However, I don't see where these options are specified, other than in the reference implementation. If we want to close the loop on this and depend upon runtime options, then they should be part of the spec (maybe I am missing them)? I do recall discussions where we decided to not limit or restrict implementations too much, but without such options specified I don't see how it's possible to indicate in the spec how to add a TTY for a process if we remove Process.Terminal.

Additionally, if we remove any evidence of a TTY allocation from the spec, that seems to imply that the runtime or its supervisor needs to be queried in order to determine if a given process has a TTY allocated. I can't think of a specific use case for needing this information, but it seems awkward in a general data handling context. So it seems if we do specify runtime options, we would need to require that the implementation must set a generic 'interactive' flag or something on the Process structure, at the least.

I would be happy to take a stab at any amount of work related to this thread, but I'd like to get some quorum on the approach before doing so.

[EDIT: I forgot about the CLI spec. I assume that's the place to specify runtime options.]

[crosbymichael]

@jlbutler ya, it would still be an option per process if we remove it from the spec.

as far as metadata, if you are the caller that is up to you to remember unless there are other usecases for getting this information. I don't think so because you really cannot have mutiple readers/writers on a process terminal anyway so it could safely be left to the caller

[crosbymichael]

@mrunalp what do you think on the subject?

[mrunalp]

I think that there is value in retaining the Terminal field for Process. It indicates whether the process' stdio should be dup'ed to a pty slave or not. We could have just that be the minimal statement and not impose anything on the runtime. The runtime can chose to either pass down own pty or create a new one.

[wking]

It indicates whether the process' stdio should be dup'ed to a pty slave or not.

That would also be indicated by the presence of --console-socket in the runtime invocation (#513), wouldn't it? I think @crosbymichael's point is that once you're setting --console-socket if and only if process.terminal is set, process.terminal becomes redundant information.

[hqhq]

I think @crosbymichael's point is that once you're setting --console-socket if and only if process.terminal is set, process.terminal becomes redundant information.

--console-socket has to be used with process.terminal is set, but not vise-versa, so these two should not be duplicated.

[mrunalp]

Yeah, agree that process.terminal is redundant but the advantage of retaining it in config.json is that one can take a look at that file and tell how the container runtime looks like. Maybe in the future we would have a different/better way than --console-socket but process.terminal would still mean the same.

[wking]

[re-posting via GitHub's web UI, since they seem to have eaten my email ;)]

On Wed, Feb 15, 2017 at 01:44:02PM -0800, Mrunal Patel wrote:

Yeah, agree that process.terminal is redundant but the advantage of retaining it in config.json is that one can take a look at that file and tell how the container runtime looks like…

Is that a request to punt this from its current 1.0.0 milestone to 2.0.0? I.e. “we agree that this will not happen for 1.0, but will revisit it before we cut 2.0”? Or are we rejecting this idea? Or still seeking a consensus about if it will be part of 1.0?