Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vincentwoo opened this issue Jan 26, 2015 · 20 comments

Comments

@vincentwoo
Copy link
Contributor

@vincentwoo vincentwoo commented Jan 26, 2015

It is bizarre to me that there is a containers/resize endpoint, but that those parameters are not available to be set on container creation or start. In addition, calling resize doesn't even work - you need to restart the container as well, making the whole flow for creating a TTY of a certain size nigh impossible in the usual Docker flow.

@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Jan 26, 2015

For the resize endpoint, don't you need to signal SIGWINCH to tell the process that the TTY has been resized? Or doesn't that work?

EDIT: Also, can you include the execdriver you are using (just paste the output of docker info)?

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jan 26, 2015

My understanding was that resize SIGWINCHed on its own. Despite that, the docs read: "The container must be restarted for the resize to take effect."

I am on the latest stable docker:

vwoo@ubuntu:~/coderpad-tty/docker$ docker --version
Docker version 1.4.1, build 5bc2ff8
vwoo@ubuntu:~/coderpad-tty/docker$ docker info
Containers: 2
Images: 164
Storage Driver: aufs
 Root Dir: /var/lib/docker/aufs
 Dirs: 168
Execution Driver: native-0.2
Kernel Version: 3.11.0-12-generic
Operating System: Ubuntu 13.10
CPUs: 2
Total Memory: 3.767 GiB
Name: ubuntu
ID: 6LST:RRCU:BKCS:Z73Y:HOMA:UUY7:RHNL:YRDY:52H6:IGID:SL3G:57BR
Username: coderpad
Registry: [https://index.docker.io/v1/]
@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Jan 26, 2015

@vincentwoo Looking at the code for the terminal resizing (called from the execdriver which I assumed was native), I don't think you're right about that. Unless the relevant ioctl syscall (TIOCSWINSZ) signals the program (which I severely doubt) then you'd need to signal it yourself.

But all of this is besides the point, as the docs do say that you need to restart the container for the resize to work -- so it's not that "calling resize doesn't even work" it's just part of the functionality to restart the container.

But I think that restarting the container shouldn't be necessary for the TTY resize to be applied (I reckon restarting the container is to ensure that all processes in a container have their TTYs resized)...

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jan 26, 2015

Okay, I can do some digging about manually sending the signal to my proc. Definitely not happy from a UX perspective about having to do so, though.

How difficult would adding width/height to container creation be? It seems like a huge timesaver. Building containers such that they can have a restart applied to them with no consequences is a huge operational burden.

@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Jan 26, 2015

@vincentwoo It wouldn't be hard at all, it'd be a fairly small patch and I could knock it out in an hour or so (getting it merged might be annoying, since it's an API change so a whole bunch of people need to LGTM it). Right now I'm wondering why on earth you need to restart a container to apply the TTY resize ... the resize endpoint should (IMO) SIGWINCH all of the processes in a container.

Also, I'm fairly sure that you can just do docker kill --signal="WINCH" <container>.

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jan 26, 2015

I am personally having a lot of trouble getting either WINCHing or resizeing to work. I've yet to see either actually take and affect a terminal session.

@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Jan 27, 2015

resize (without WINCH works fine for me):

[terminal 1] % docker info
Containers: 12
Images: 50
Storage Driver: devicemapper
 Pool Name: docker-254:2-1575127-pool
 Pool Blocksize: 65.54 kB
 Data file: /var/lib/docker/devicemapper/devicemapper/data
 Metadata file: /var/lib/docker/devicemapper/devicemapper/metadata
 Data Space Used: 897.8 MB
 Data Space Total: 107.4 GB
 Metadata Space Used: 3.416 MB
 Metadata Space Total: 2.147 GB
 Library Version: 1.02.92 (2014-11-28)
Execution Driver: native-0.2
Kernel Version: 3.18.2-2-ARCH
Operating System: Arch Linux
CPUs: 2
Total Memory: 3.668 GiB
Name: kirari
ID: UC7W:T7MS:FUZ7:VMBC:ZOQ5:GOTJ:IABI:Q3KO:W364:MLAH:TQLM:MXB4
Username: cyphar
Registry: [https://index.docker.io/v1/]
WARNING: No swap limit support
[terminal 1] % docker run -it ubuntu /bin/bash
[terminal 1] $ stty -a
speed 38400 baud; rows 27; columns 104; line = 0;
[ ... snip ... ]
[terminal 2] % socat unix-connect:/run/docker.sock stdio
POST /v1.16/containers/081a9d2bc3a96ac8eb7fd4cadf173d344ffe893e9b481a7c250eb76c6cacbb61/resize?h=27&w=10 HTTP/1.1

HTTP/1.1 200 OK
Date: Tue, 27 Jan 2015 14:27:39 GMT
Content-Length: 0
Content-Type: text/plain; charset=utf-8
[terminal 1] $ stty -a
speed 38400 baud; rows 27; columns 10; line = 0;
[ ... snip ... ]

So it works for me (ignore the weird tagging of different terminals, this was done in a tmux session which is hard to express in a flat format).

I also want to point out that docker run doesn't restart the container after doing a resize (neither does docker attach and others), implying that there's no need to update the /containers/.../create endpoint (because you can just use the resize one).

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jan 31, 2015

Upon further investigation, it seems that you were right. I was just having trouble verifying with an actual console that exceeded the width set by resize. Can we update the docs to reflect not needing to restart to resize? Additionally, I still think having width and height be a property of container creation or run is a good idea. Doing an extra call just to set some minor config seems a bit wrong.

@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Feb 1, 2015

@vincentwoo Since internally, neither the start nor create endpoints support it, why should the create one support it? IMO it's a config option that is not really related to starting or creating a container.

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Feb 4, 2015

@cyphar Certainly I think this relates to plausibly at least create. create takes a Tty option, so it doesn't seem terribly farfetched that configuring the size of that selfsame tty should be part of the API.

@LK4D4

This comment has been minimized.

Copy link
Contributor

@LK4D4 LK4D4 commented Sep 15, 2016

@cyphar @vincentwoo do you think this still actual and worth to do? Thanks!

@LK4D4 LK4D4 added the area/runtime label Sep 15, 2016
@thaJeztah

This comment has been minimized.

Copy link
Member

@thaJeztah thaJeztah commented Sep 20, 2016

Looks like this was duplicated by #25450

ping @mlaventure were you working on that?

@thaJeztah

This comment has been minimized.

Copy link
Member

@thaJeztah thaJeztah commented Sep 20, 2016

Oh, well, reading again, it's slightly different, but related (I think)

@cyphar

This comment has been minimized.

Copy link
Contributor

@cyphar cyphar commented Sep 20, 2016

This has actually popped up again inside the runtime-spec (opencontainers/runtime-spec#563). Basically, since Windows requires the ability to set the console size on first start, we might end up adding it for all platforms.

@LK4D4 see above.

@mlaventure

This comment has been minimized.

Copy link
Contributor

@mlaventure mlaventure commented Sep 20, 2016

@thaJeztah It's related indeed, assigning myself to it too. Once runc is updated and vendor'ed in, I'll make the necessary changes (it will also require changes in containerd)

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Feb 7, 2017

@mlaventure any developments on this front? I'd still definitely like this. I set width/height with stty inside the container these days, understandably annoying.

@mlaventure

This comment has been minimized.

Copy link
Contributor

@mlaventure mlaventure commented Feb 8, 2017

@vincentwoo sorry, no. I have been focusing on the new containerd

I'll try and see if I can squeeze a bit of time to look at this in a near future.

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jun 15, 2017

@mlaventure ping! This is causing me headaches again, would really love a quick patch :)

@vincentwoo

This comment has been minimized.

Copy link
Contributor Author

@vincentwoo vincentwoo commented Jun 15, 2017

(also, latest docs still read "Resize the TTY for a container. You must restart the container for the resize to take effect.")

@mlaventure

This comment has been minimized.

Copy link
Contributor

@mlaventure mlaventure commented Jun 15, 2017

Sorry, really haven't had the time to tackle this. Not sure when I'll have some time. I added it to my TODO for docker-ce 17.07, but not sure I'll get the time to work on it.

Anyone willing to do so, should make a PR 😇

webframp added a commit to webframp/kitchen-dokken that referenced this issue May 6, 2018
docker exec doesn't supply terminal width and height values to containers
currently. This should really be fixed upstream but this is workaround for the
issue until then. There's a 2 year old github issue for it so I doubt it's a
priority right now.

Before:
```
sme@megatron  ~  stty size
48 151

sme@megatron  ~  kitchen login

root@dokken:/# stty size
0 0
```

After:
```
sme@megatron  ~  stty size
48 151

sme@megatron  ~  kitchen login

root@dokken:/# stty size
48 151
```

refs:
- moby/moby#33794
- moby/moby#10341

Signed-off-by: Sean Escriva <sean.escriva@gmail.com>
webframp added a commit to webframp/kitchen-dokken that referenced this issue May 6, 2018
docker exec doesn't supply terminal width and height values to containers
currently. This should really be fixed upstream but this is workaround for the
issue until then. There's a 2 year old github issue for it so I doubt it's a
priority right now.

Before:
```
sme@megatron ~ stty size
48 151

sme@megatron ~ kitchen login

root@dokken:/# stty size
0 0
```

After:
```
sme@megatron ~ stty size
48 151

sme@megatron ~ kitchen login

root@dokken:/# stty size
48 151
```

refs:
- moby/moby#33794
- moby/moby#10341

Signed-off-by: Sean Escriva <sean.escriva@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.