config: Add consoleSize to process #563

lowenna · 2016-09-14T20:15:36Z

Signed-off-by: John Howard jhoward@microsoft.com

Extracting pieces from the proof of concept PR for Windows OCI support at #504. This adds the consoleSize platform specific field to the Process structure.

philips · 2016-09-14T20:34:45Z

config.md

@@ -118,6 +118,10 @@ For more information about SELinux, see  [Selinux documentation](http://selinuxp
 * **`noNewPrivileges`** (bool, optional) setting `noNewPrivileges` to true prevents the processes in the container from gaining additional privileges.
 [The kernel doc](https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt) has more information on how this is achieved using a prctl system call.

+For Windows-based systems, the process structure supports the following process specific fields:
+
+* **`initialconsolesize`** (array of ints, optional) specifies the initial console size (height, width) of the terminal if attached.


capitalization is significant

Yes, I deliberately did this as the capitalisation is consistent with other items such as rlimits (vs rLimits). Are those incorrect as well? If so, I'll open another PR to address that, but fix the capitalisation to initialConsoleSize here.

wking · 2016-09-14T21:00:12Z

On Wed, Sep 14, 2016 at 01:38:46PM -0700, John Howard wrote:

Yes, I deliberately did this as the capitalisation is consistent
with other items such as rlimits (vs rLimits). Are those
incorrect as well? If so, I'll open another PR to address that, but
fix the capitalisation to initialConsoleSize here.

Touche ;). I'm fine with rlimits, rLimits, and resourceLimits. And
consistency is nice, but there's also a benefit to using the
platform's conventional casing [1]. I'm fine letting per-patform
maintainers go their own way on this sort of style decision.

lowenna · 2016-09-14T21:37:54Z

Updated it to camelCase. Given there's almost no Windows specific fields in the spec currently, I may as well start with a sensible pattern.

tianon · 2016-09-15T15:27:21Z

LGTM

wking · 2016-09-14T22:09:34Z

specs-go/config.go

@@ -51,6 +51,8 @@ type Process struct {
 	ApparmorProfile string `json:"apparmorProfile,omitempty" platform:"linux"`
 	// SelinuxLabel specifies the selinux context that the container process is run as. (this field is platform dependent)
 	SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"`
+	// InitialConsoleSize contains the initial h,w of the console size. (this field is platform dependent)
+	InitialConsoleSize [2]int `json:"initialConsoleSize" platform:"windows"`


Yup. Good catch. Updating - pushing shortly.

Done. And the matching doc change ;)

wking · 2016-09-15T15:33:07Z

specs-go/config.go

@@ -51,6 +51,8 @@ type Process struct {
 	ApparmorProfile string `json:"apparmorProfile,omitempty" platform:"linux"`
 	// SelinuxLabel specifies the selinux context that the container process is run as. (this field is platform dependent)
 	SelinuxLabel string `json:"selinuxLabel,omitempty" platform:"linux"`
+	// InitialConsoleSize contains the initial h,w of the console size. (this field is platform dependent)


Drop “this field is platform dependent” (#568, #569).

Yes, of course :)

tianon · 2016-09-15T16:42:21Z

LGTM redux

wking · 2016-09-15T16:46:43Z

c86f32c looks good to me, although “of the console size” could probably be trimmed to “of the console” ;).

anuthan · 2016-09-15T17:16:55Z

LGTM

lowenna · 2016-09-15T17:29:23Z

Grammer police 🚔 😄 @wking - Fixed, thanks for catching.

wking · 2016-09-15T17:37:09Z

On Thu, Sep 15, 2016 at 10:29:25AM -0700, John Howard wrote:

Grammer police 🚔 😄 @wking - Fixed, thanks for catching.

2a7b7c6 looks even better to me ;).

tianon · 2016-09-15T18:34:50Z

config.md

@@ -118,6 +118,10 @@ For more information about SELinux, see  [Selinux documentation](http://selinuxp
 * **`noNewPrivileges`** (bool, optional) setting `noNewPrivileges` to true prevents the processes in the container from gaining additional privileges.
 [The kernel doc](https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt) has more information on how this is achieved using a prctl system call.

+For Windows-based systems, the process structure supports the following process specific fields:
+
+* **`initialConsoleSize`** (array of uints, optional) specifies the initial console size (height, width) of the terminal if attached.


Just realized this is (height, width) -- isn't it more common for these particular types of values to be ordered (width, height)? Is this ordered this way arbitrarily, or is this how the underlying platform support orders them?

For example:

The ordering here is consistent with the way the platform API is implemented.

👍 thanks for confirming ❤️

tianon · 2016-09-15T19:09:05Z

LGTM redux

laijs · 2016-09-17T00:39:35Z

config.md

@@ -118,6 +118,10 @@ For more information about SELinux, see  [Selinux documentation](http://selinuxp
 * **`noNewPrivileges`** (bool, optional) setting `noNewPrivileges` to true prevents the processes in the container from gaining additional privileges.
 [The kernel doc](https://www.kernel.org/doc/Documentation/prctl/no_new_privs.txt) has more information on how this is achieved using a prctl system call.

+For Windows-based systems, the process structure supports the following process specific fields:


I think it is also needed in the linux.

@laijs Why do you think that? It's needed on Windows to init the conhost terminal, but it's only ever been in Windows to date.

In linux, we also use a default winsize for the tty console when starting the container, and then we resize it later, these is a gap between them.

lowenna · 2016-09-17T22:19:44Z

Updated to not make this a Windows-specific field after comment https://github.com/opencontainers/runtime-spec/pull/563/files#r79284892 from @laijs

laijs · 2016-09-18T02:25:34Z

specs-go/config.go

@@ -31,6 +31,8 @@ type Spec struct {
 type Process struct {
 	// Terminal creates an interactive terminal for the container.
 	Terminal bool `json:"terminal,omitempty"`
+	// InitialConsoleSize contains the initial h,w of the console.
+	InitialConsoleSize [2]uint `json:"initialConsoleSize"`


Hello, @maintainers, how about use a more verbose structure here?

type ConsoleWindowSize struct { Height uint `json:"height"` Width uint `json:"width"` }

+1 for explicit height/width. Although probably no need for the specific name. type Box would give us something we could use in other places, although I can't think of other consumers now.

+1 for this change, @jhowardmsft WDYT?

Agreed. Updated using Box, and updated the docs accordingly

lowenna · 2016-09-18T03:46:51Z

@wking @laijs @hqhq Updated.

hqhq · 2016-09-18T04:02:44Z

config.md

@@ -95,6 +95,7 @@ See links for details about [mountvol](http://ss64.com/nt/mountvol.html) and [Se
 **`process`** (object, required) configures the container process.

 * **`terminal`** (bool, optional) specifies whether you want a terminal attached to that process, defaults to false.
+* **`initialConsoleSize`** (box, optional) specifies the initial console size of the terminal if attached.


I prefer naming as consoleWindowSize as @laijs suggested, because we might want to update process console size and it seems weird to assign new config to the field with initial prefixed.

@hqhq - @laijs explicitly said this was needed on Linux, so I don't understand your comment? As for initial, this is important - it's the how the console is initialised for the console start, so I don't agree with your other suggestion.

Yes Linux has tty console size, but the Window in the name consoleWindowSize doesn't mean Windows operate system, it just reflect semantics from http://man7.org/linux/man-pages/man4/tty_ioctl.4.html (section Get and set window size).

And I don't think we need to specify initial for the usage of initialization, otherwise we'll call InitialArgs, InitialRlimits, etc.. Not to mention that they can also be used in non-initialization stages.

wking · 2016-09-18T07:06:00Z

On Sat, Sep 17, 2016 at 11:24:02PM -0700, Qiang Huang wrote:

Not to mention that they can also be used in non-initialization
stages.

I'm fine dropping initial, but thought I'd point out that the current
spec is not supporter of join-and-tweak. See the in-flight #540
trying to clarify that position, and the open #305 in favor of
allowing join-and-tweak. I'm personally in favor of allowing
join-and-tweak, but filed #540 because I'd rather have clearer wording
on the no-tweaking restriction while we want for the consensus to
shift towards allowing join-and-tweak.

hqhq · 2016-09-18T07:28:45Z

@wking Yeah I plan to put this back on table post-1.0, but we'll support it in runc whether it's in spec or not, so we better keep it looks reasonable for update.

lowenna · 2016-09-18T07:34:58Z

Are there not already many fields which are not configurable post create? I don't care though, if consensus is to remove 'initial', so be it. Imo it seems pretty descriptive of the purpose though.

wking · 2016-09-18T08:11:41Z

On Sun, Sep 18, 2016 at 12:34:59AM -0700, John Howard wrote:

Are there not already many fields which are not configurable post
create?

It may be helpful to think about this in three classes:

a. Initial setup. The resource (e.g. a console) was just created and
needs to be configured. That's what most/all of the current config
settings are about.
b. Out-of-band tweaking. Users can do whatever they want outside of
the runtime, e.g. dragging their console around to resize it.
c. Runtime tweaking. A subsequent runtime invocation updates an
existing resource. This is what #305 and #540 are talking about.

It's probably good to remind users that most settings are not set in
stone, and that (b) and maybe eventually (c) are possible (one example
where this isn't true is the user map 1, which is “write once and
you're stuck with it” 2). But I don't think prefixing properties
with “initial” is the most efficient way to remind them.

 > An attempt to write more than once to a uid_map file in a user
 > namespace fails with the error EPERM.  Similar rules apply for
 > gid_map files.

lowenna · 2016-09-19T18:02:44Z

@wking. Thanks. The current commit f2bc43b should reflect the changes based on all feedback to date.

I would be inclined to say that adding something like opencontainers/image-spec#311 belongs in a separate PR rather than polluting this one. I'm happy to put that together later today.

wking · 2016-09-19T18:11:30Z

On Mon, Sep 19, 2016 at 11:02:46AM -0700, John Howard wrote:

I would be inclined to say that adding something like
opencontainers/image-spec#311 belongs in a
separate PR rather than polluting this one. I'm happy to put that
together later today.

That's fine with me. Leaving the consoleSize-unset behavior implicit
for an afternoon is fine. And perhaps the
opencontainers/image-spec#311 port lands first, in which case we can
reroll this to explicitly make the consoleSize-unset case unspecified.

f2bc43b looks good to me, except the commit summary should now be:

config: Add consoleSize to process

lowenna · 2016-09-19T18:17:09Z

Ah yes, of course. Commit summary and PR title updated.

wking · 2016-09-19T18:21:11Z

55956b8 and a punt on porting opencontainers/image-spec#311 looks good to me.

hqhq · 2016-09-20T01:03:28Z

LGTM

lowenna · 2016-09-20T02:26:24Z

@tianon - sorry, it's gone through a few revisions since your last LGTMs. Any chance of a further review? Thanks ❤️

mrunalp · 2016-09-20T21:17:29Z

LGTM

tianon · 2016-09-20T21:27:33Z

Nice, I like the introduction of the box type to avoid having to just "know" what order the arguments go in. 👍

LGTM
(although appears to now have a conflict in config.md)

wking · 2016-09-20T21:34:38Z

On Tue, Sep 20, 2016 at 02:27:34PM -0700, Tianon Gravi wrote:

(although appears to now have a conflict in config.md)

#574 is probably causing a lot of trivial conflicts.

lowenna · 2016-09-20T21:58:31Z

Rebased. Yes, it was #574 (optional --> OPTIONAL)

wking · 2016-09-20T22:09:38Z

config.md

@@ -95,6 +95,7 @@ See links for details about [mountvol](http://ss64.com/nt/mountvol.html) and [Se
 **`process`** (object, REQUIRED) configures the container process.

 * **`terminal`** (bool, OPTIONAL) specifies whether you want a terminal attached to that process, defaults to false.
+* **`consoleSize`** (box, OPTIONAL) specifies the console size of the terminal if attached.


It's pretty clear from the Go and JSON Schema what “box” means here. But it's the Markdown that's normative. I'd recommend either inlining the defintion here:

consoleSize (object, OPTIONAL) specifies the console size of the terminal if attached.
Contains the following properties:

height (uint, REQUIRED) specifies the height of the console.

width (uint, REQUIRED) specifies the width of the console.

Or defining box in a separate section at the end of this file:

Box

A geometric box.
This object has the following fields:

height …

Updated using the first notation from your suggestions. (a502caf)

Signed-off-by: John Howard <jhoward@microsoft.com>

wking · 2016-09-20T22:55:23Z

a502caf looks good to me (like most of the previous iterations of this PR ;).

hqhq · 2016-09-21T02:32:52Z

LGTM

mrunalp · 2016-09-21T19:29:22Z

LGTM

…r unset The old language is from a502caf (config: Add consoleSize to process, 2016-09-14, opencontainers#563), where nobody commented on the "if attached" wording [1]. But reading the old line now, it's not clear to me what consoleSize means when terminal is not true. This commit explicitly declares consoleSize unspecified in that condition, so runtimes are free to do what they want short of erroring out. I considered making the property undefined or requiring it to be unset, but those seemed too strict given our permissive "MUST ignore unknown properties" extensibility requirement. [1]: opencontainers#563 Signed-off-by: W. Trevor King <wking@tremily.us>

…r unset The old language is from a502caf (config: Add consoleSize to process, 2016-09-14, opencontainers#563), where nobody commented on the "if attached" wording [1]. But reading the old line now, it's not clear to me what consoleSize means when terminal is not true. This commit explicitly declares consoleSize ignored in that condition. I'd rather have made it unspecified, so runtimes are free to do what they want short of erroring out, but Michael wanted the more specific "ignored" [2]. I considered making the property undefined or requiring it to be unset, but those seemed too strict given our permissive "MUST ignore unknown properties" extensibility requirement. [1]: opencontainers#563 [2]: opencontainers#863 (comment) Signed-off-by: W. Trevor King <wking@tremily.us>

philips reviewed Sep 14, 2016

View reviewed changes

lowenna force-pushed the jjh/initialconsolesize branch from be80d0d to 47a03c6 Compare September 14, 2016 21:36

wking suggested changes Sep 15, 2016

View reviewed changes

wking reviewed Sep 15, 2016

View reviewed changes

lowenna force-pushed the jjh/initialconsolesize branch from 47a03c6 to c86f32c Compare September 15, 2016 16:41

lowenna force-pushed the jjh/initialconsolesize branch from c86f32c to 2a7b7c6 Compare September 15, 2016 17:28

tianon reviewed Sep 15, 2016

View reviewed changes

This was referenced Sep 15, 2016

Windows: OCI process struct and console size to uint moby/moby#26579

Merged

ConsoleSize --> uint microsoft/hcsshim#67

Merged

laijs reviewed Sep 17, 2016

View reviewed changes

lowenna force-pushed the jjh/initialconsolesize branch from 2a7b7c6 to 0fa739c Compare September 17, 2016 22:18

lowenna force-pushed the jjh/initialconsolesize branch from 0fa739c to dcf55f5 Compare September 17, 2016 22:21

laijs reviewed Sep 18, 2016

View reviewed changes

lowenna force-pushed the jjh/initialconsolesize branch from dcf55f5 to 0efe3b8 Compare September 18, 2016 03:46

hqhq reviewed Sep 18, 2016

View reviewed changes

lowenna force-pushed the jjh/initialconsolesize branch from f2bc43b to 55956b8 Compare September 19, 2016 18:16

lowenna changed the title ~~Windows: Add consoleSize to process~~ config: Add consoleSize to process Sep 19, 2016

lowenna mentioned this pull request Sep 19, 2016

README: Define 'unspecified', 'undefined', and 'implementation-defined' #575

Merged

This was referenced Sep 20, 2016

Container create or start should accept height/width params moby/moby#10341

Closed

Docker number of lines in terminal changing inside docker moby/moby#25450

Closed

lowenna mentioned this pull request Sep 20, 2016

Windows: OCI HVRuntime and LayerPaths to options moby/moby#26746

Merged

lowenna force-pushed the jjh/initialconsolesize branch from 55956b8 to f31b19f Compare September 20, 2016 21:57

wking reviewed Sep 20, 2016

View reviewed changes

config: Add consoleSize to process

a502caf

Signed-off-by: John Howard <jhoward@microsoft.com>

lowenna force-pushed the jjh/initialconsolesize branch from f31b19f to a502caf Compare September 20, 2016 22:19

mrunalp merged commit c356a80 into opencontainers:master Sep 21, 2016

wking mentioned this pull request Sep 22, 2016

specs-go/config: Make consoleSize a pointer #581

Closed

vbatts mentioned this pull request Dec 7, 2016

Version bump for v1.0.0-rc3 #642

Merged

wking mentioned this pull request May 26, 2017

config: Explicily make consoleSize unspecified if terminal is false or unset #863

Merged

config: Add consoleSize to process #563

config: Add consoleSize to process #563

Conversation

lowenna commented Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Sep 14, 2016

lowenna commented Sep 14, 2016

tianon commented Sep 15, 2016 • edited by caniszczyk 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

tianon commented Sep 15, 2016 • edited by caniszczyk Loading

wking commented Sep 15, 2016 via email

anuthan commented Sep 15, 2016

lowenna commented Sep 15, 2016

wking commented Sep 15, 2016

tianon Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tianon commented Sep 15, 2016 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowenna commented Sep 17, 2016

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

lowenna commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Sep 18, 2016

hqhq commented Sep 18, 2016

lowenna commented Sep 18, 2016

wking commented Sep 18, 2016

lowenna commented Sep 19, 2016

wking commented Sep 19, 2016

lowenna commented Sep 19, 2016

wking commented Sep 19, 2016 via email

hqhq commented Sep 20, 2016 • edited by caniszczyk Loading

lowenna commented Sep 20, 2016

mrunalp commented Sep 20, 2016 • edited by caniszczyk Loading

tianon commented Sep 20, 2016 • edited by caniszczyk Loading

wking commented Sep 20, 2016

lowenna commented Sep 20, 2016

wking Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Box

lowenna Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

wking commented Sep 20, 2016 via email

hqhq commented Sep 21, 2016 • edited by caniszczyk Loading

mrunalp commented Sep 21, 2016 • edited by caniszczyk Loading

lowenna commented Sep 14, 2016 •

edited

Loading

tianon commented Sep 15, 2016 •

edited by caniszczyk

Loading

tianon commented Sep 15, 2016 •

edited by caniszczyk

Loading

tianon Sep 15, 2016 •

edited

Loading

tianon commented Sep 15, 2016 •

edited by caniszczyk

Loading

hqhq commented Sep 20, 2016 •

edited by caniszczyk

Loading

mrunalp commented Sep 20, 2016 •

edited by caniszczyk

Loading

tianon commented Sep 20, 2016 •

edited by caniszczyk

Loading

wking Sep 20, 2016 •

edited

Loading

lowenna Sep 20, 2016 •

edited

Loading

hqhq commented Sep 21, 2016 •

edited by caniszczyk

Loading

mrunalp commented Sep 21, 2016 •

edited by caniszczyk

Loading