Set initial console size based on process spec #1275

williammartin · 2017-01-16T16:21:13Z

Signed-off-by: Petar Petrov pppepito86@gmail.com

This commit hopefully resolves issue #1269, setting the initial console size for TTYs based on the ConsoleSize properties in the process spec.

Two thoughts about this PR that would be good to get feedback on: Firstly, the height and width are passed through as separate primitives and turned into a term.WinSize right before use. We weren't sure of the pros and cons of this approach vs the term package all the way up the stack, or the runtime-spec Box or even whether we should define another process config struct.

Secondly, the bats test would ideally read the master fd from the $CONSOLE_SOCKET and read output directly from a runc exec invocation calling stty -a but there seems to be no convention for doing this and working around the existing recvtty infrastructure to allow these kind of tests seemed like a fairly sizable change, that might be best as its own PR.

williammartin · 2017-01-16T17:33:46Z

Looks like the build failed with some bad packaging. Can't see how this is anything to do with us - is it possible to kick off again?

cyphar · 2017-01-17T02:31:19Z

I've re-kicked off the CI and the tests now pass.

justincormack · 2017-01-23T10:54:03Z

utils_linux.go

@@ -81,6 +81,8 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) {
 		Label:           p.SelinuxLabel,
 		NoNewPrivileges: &p.NoNewPrivileges,
 		AppArmorProfile: p.ApparmorProfile,
+		ConsoleWidth:    uint16(p.ConsoleSize.Width),
+		ConsoleHeight:   uint16(p.ConsoleSize.Height),


This will silently take the lowest 16 bits of the specified value, which could be confusing. I think it is better to pass and store as int everywhere and error on console creation if it is too big (in future other platforms could have different limits).

Yeh this seems reasonable. Have updated the PR - let me know if you think this is an appropriate solution. Thanks.

justincormack · 2017-01-23T10:55:58Z

libcontainer/console_linux.go

@@ -5,18 +5,23 @@ import (
 	"os"
 	"syscall"
 	"unsafe"
+
+	"github.com/docker/docker/pkg/term"


I think there is a requirement to not depend on docker/docker to avoid circular dependencies. May be betetr to copy the one function into this repo.

Yeh we wondered about this but thought we saw it elsewhere (maybe some legacy that was needing to be removed, or just a mistake). Have just done a little copying and refactoring in the updated PR.

williammartin · 2017-01-27T10:40:46Z

The build failed because of the flake fixed by: #1237

container_linux.go:247: starting container process caused "process_linux.go:272: running exec setns process for init caused \"exit status 26\""

Can someone kick off again please.

caniszczyk · 2017-01-27T13:03:54Z

cc: @crosbymichael since it's on janky

cyphar · 2017-01-27T13:12:27Z

For future reference, anyone inside the #docker-maintainers channel can restart the Janky jobs using the message !rebuild runc#<pr number>.

crosbymichael · 2017-02-04T00:28:48Z

@williammartin just wondering, why do you have to do a resize at this point in time? Why can't you resize after receiving the master pty?

williammartin · 2017-02-04T00:32:05Z

@crosbymichael Just to be clear on your question, when you say "receiving the master pty", you mean when the caller of runc receives it on the --console-socket rather than when runc exec gets it from runc init?

crosbymichael · 2017-02-04T00:33:19Z

When the caller of runc gets it

williammartin · 2017-02-06T20:46:34Z

Well, it's intuitive that if this is specified in the runtime-spec, that the runtime should apply it before the process starts. More importantly, software that expects particular tty sizings should expect it to be set.

I'm wondering if part of the pain here is because exec doesn't have a create/start split i.e. right now the process might be finished by the time I receive the master pty. I'd need some way to let runc know "ok, I've set the tty size, carry on", which feels less intuitive and more complicated than this PR.

crosbymichael · 2017-02-06T21:07:45Z

Well if your issue is with exec that is kinda a separate problem because consoles don't work at all right now because its too fast.

See #1302

williammartin · 2017-02-07T00:08:32Z

While it's true that the console stuff is currently broken, it still seems intuitive that the runtime should set the specification for a caller before the process starts. Do you have some other thoughts on solving that or is your feeling that this should be punted until #1302 resolves in some manner?

crosbymichael · 2017-02-07T00:13:17Z

Yes I agree that it should be set before the process starts. I think it would be better to solve this how I want to fix the race for the exec case, if we can block until we receive the pty master, resize it, then close that connection causing runc to start the process I think that is a better flow than having to provide console sizes through runc.

To the branch await-prs-from-138131099 in the fork in cloudfoundry-incubator. We can get back onto a released version of runc when the following PRs are merged: * opencontainers/runc#1237 * opencontainers/runc#1275 dadoo was the main guardian component that had to change here, to accomodate changes in runc for processes that have TTYs. [#138131099]

craigfurman · 2017-07-20T15:50:34Z

@crosbymichael now that #1302 is resolved, do you have any thoughts on this?

craigfurman · 2017-09-27T15:14:38Z

This PR is a little smaller after our most recent rebase now that runc uses a pty library. Does this change anything?

Craig & @jszroberto

cyphar

I think this might be able to solve moby/moby#25450 (at least partially). This looks alright (bar my comments that need to be addressed), but I'd like to know whether @crosbymichael is okay with this new version of the patch (I don't remember what it used to look like to be honest).

cyphar · 2017-10-02T07:33:34Z

utils_linux.go

+	if p.ConsoleSize != nil {
+		consoleWidth = p.ConsoleSize.Width
+		consoleHeight = p.ConsoleSize.Height
+	}


The defaults of 0 don't make sense given that you unconditionally set the console size when creating it. I don't know what the kernel will do when you tell it to resize a console to 0x0, but from my quick reading I expect that it isn't a no-op.

cyphar · 2017-10-02T07:33:58Z

libcontainer/process.go

@@ -47,6 +47,10 @@ type Process struct {
 	// ExtraFiles specifies additional open files to be inherited by the container
 	ExtraFiles []*os.File

+	// Initial sizings for the console
+	ConsoleWidth  uint
+	ConsoleHeight uint


Since we have to use uint16 in the Resize why not just make these uint16 in the first place?

cyphar · 2017-10-02T07:35:03Z

tests/integration/tty.bats

+	wait_for_container 15 1 test_busybox
+
+	# make sure we're running
+	testcontainer test_busybox running


I believe we've stopped using this pattern in tests (wait_for_container and testcontainer), but I might be mistaken.

cyphar · 2017-10-02T07:38:30Z

tests/integration/tty.bats

+	"args": [
+		"/bin/sh",
+		"-c",
+		"/bin/stty -a > $BATS_TMPDIR/tty-info"


$BATS_TMPDIR is on the host not in the container. Since it's generally /tmp this won't cause obvious issues, but just use /tmp inside the container.

cyphar · 2017-10-02T07:38:42Z

tests/integration/tty.bats

+{
+	"args": [
+	    "/bin/cat",
+	    "$BATS_TMPDIR/tty-info"


cyphar · 2017-10-02T07:39:11Z

tests/integration/tty.bats

+
+	# clean process.jsons
+	rm $BATS_TMPDIR/write-tty-info.json
+	rm $BATS_TMPDIR/read-tty-info.json


Also you could use process substitution to remove the need for temporary files, but that's not really a big issue.

Signed-off-by: Will Martin <wmartin@pivotal.io> Signed-off-by: Petar Petrov <pppepito86@gmail.com> Signed-off-by: Ed King <eking@pivotal.io> Signed-off-by: Roberto Jimenez Sanchez <jszroberto@gmail.com> Signed-off-by: Thomas Godkin <tgodkin@pivotal.io>

BooleanCat · 2017-10-04T13:48:44Z

@crosbymichael We have made the suggested changes above, let us know what you think.

Tom and @jszroberto

crosbymichael · 2017-10-04T15:30:53Z

LGTM

Thanks @williammartin

cyphar · 2017-10-04T15:33:04Z

LGTM. Thanks all.

@crosbymichael

Set initial console size based on process spec LGTMs: @crosbymichael @cyphar Closes #1275

williammartin · 2017-10-04T15:42:27Z

Thanks all

teddyking mentioned this pull request Jan 18, 2017

Add support for setting console size for runc exec -d -tty -console-socket ... #1269

Closed

justincormack reviewed Jan 23, 2017

View reviewed changes

williammartin force-pushed the 1269-initial-console-sizing branch 2 times, most recently from c6ceb09 to b3f43f4 Compare January 26, 2017 13:48

craigfurman force-pushed the 1269-initial-console-sizing branch from b3f43f4 to c1f9411 Compare April 20, 2017 10:59

williammartin mentioned this pull request May 2, 2017

runc:[2:INIT] processes not progressing when calling runc exec #1432

Closed

craigfurman force-pushed the 1269-initial-console-sizing branch from c1f9411 to 8cc84aa Compare July 25, 2017 10:42

karampok force-pushed the 1269-initial-console-sizing branch from 8cc84aa to 00d99ae Compare September 26, 2017 13:56

cyphar requested changes Oct 2, 2017

View reviewed changes

BooleanCat force-pushed the 1269-initial-console-sizing branch from 00d99ae to 605dc5c Compare October 4, 2017 13:28

cyphar approved these changes Oct 4, 2017

View reviewed changes

cyphar merged commit 605dc5c into opencontainers:master Oct 4, 2017

cyphar added a commit that referenced this pull request Oct 4, 2017

merge branch 'pr-1275'

dc1552a

Set initial console size based on process spec LGTMs: @crosbymichael @cyphar Closes #1275

cyphar mentioned this pull request Nov 16, 2017

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

Closed

cyphar mentioned this pull request Feb 24, 2018

VERSION: release v1.0.0-rc5 #1739

Merged

MoritzKn mentioned this pull request Feb 11, 2022

Terminal Size Zero in Docker Run moby/moby#43229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set initial console size based on process spec #1275

Set initial console size based on process spec #1275

williammartin commented Jan 16, 2017

williammartin commented Jan 16, 2017

cyphar commented Jan 17, 2017

justincormack Jan 23, 2017

williammartin Jan 26, 2017

justincormack Jan 23, 2017

williammartin Jan 26, 2017

williammartin commented Jan 27, 2017

caniszczyk commented Jan 27, 2017

cyphar commented Jan 27, 2017

crosbymichael commented Feb 4, 2017

williammartin commented Feb 4, 2017 •

edited

Loading

crosbymichael commented Feb 4, 2017

williammartin commented Feb 6, 2017

crosbymichael commented Feb 6, 2017

williammartin commented Feb 7, 2017 •

edited

Loading

crosbymichael commented Feb 7, 2017

craigfurman commented Jul 20, 2017

craigfurman commented Sep 27, 2017

cyphar left a comment

cyphar Oct 2, 2017

cyphar Oct 2, 2017

cyphar Oct 2, 2017

cyphar Oct 2, 2017

cyphar Oct 2, 2017

cyphar Oct 2, 2017

BooleanCat commented Oct 4, 2017

crosbymichael commented Oct 4, 2017 •

edited by caniszczyk

Loading

cyphar commented Oct 4, 2017 •

edited by caniszczyk

Loading

williammartin commented Oct 4, 2017

Set initial console size based on process spec #1275

Set initial console size based on process spec #1275

Conversation

williammartin commented Jan 16, 2017

williammartin commented Jan 16, 2017

cyphar commented Jan 17, 2017

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

williammartin commented Jan 27, 2017

caniszczyk commented Jan 27, 2017

cyphar commented Jan 27, 2017

crosbymichael commented Feb 4, 2017

williammartin commented Feb 4, 2017 • edited Loading

crosbymichael commented Feb 4, 2017

williammartin commented Feb 6, 2017

crosbymichael commented Feb 6, 2017

williammartin commented Feb 7, 2017 • edited Loading

crosbymichael commented Feb 7, 2017

craigfurman commented Jul 20, 2017

craigfurman commented Sep 27, 2017

cyphar left a 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

BooleanCat commented Oct 4, 2017

crosbymichael commented Oct 4, 2017 • edited by caniszczyk Loading

cyphar commented Oct 4, 2017 • edited by caniszczyk Loading

williammartin commented Oct 4, 2017

williammartin commented Feb 4, 2017 •

edited

Loading

williammartin commented Feb 7, 2017 •

edited

Loading

crosbymichael commented Oct 4, 2017 •

edited by caniszczyk

Loading

cyphar commented Oct 4, 2017 •

edited by caniszczyk

Loading