fixes 1492: tty initial size error #1529

lifubang · 2018-11-21T13:46:02Z

Signed-off-by: Lifubang lifubang@acmcoder.com

For Ubuntu 16.04.2 LTS (GNU/Linux 4.4.0-62-generic x86_64), Docker version 17.12.0-ce, build c97c6d6 / 18.03.0-ce, build 0520e24
When I run: docker exec -it ubuntu /bin/bash
The tty's size is small than window's size.

root@iZ2zeesy3n96esegm7tue0Z:~# docker exec -it ubuntu /bin/bash
root@97b3e229a701:/# ls -a
.   .dockerenv  bin   dev  home  lib64  mnt  proc  run   srv  tmp  var
..  .r          boot  etc  lib   media  opt  root  sbin  sys  usr

After I fix the bug:

root@iZ2zeesy3n96esegm7tue0Z:~# ./docker-linux-amd64 exec -it ubuntu /bin/bash
root@97b3e229a701:/# ls -a
.  ..  .dockerenv  .r  bin  boot  dev  etc  home  lib  lib64  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var

If not fix this bug, it's too inconvenient for me to use docker exec -it.

- What I did

Fix tty initial size error
I think it's a bug of dockerd, but for previous dockerd edition, I think we can fix it in cli.
Fix when we use --debug flag, the first shell prompt don't return to the head of line when we got an error in func resizeTtyTo!

- How I did it
~~func MonitorTtySize in file cli/command/container/tty.go~~
~~I move the call resizeTty() to last and run it asynchronously.~~

Because in sometimes when dockercli call tty resize, the exec have not finished yet. Then we got a message "Error response from daemon: no such exec", but we ignore it, so we need to throw this error and deal with it after 10 milliseconds.
Add \r when logrus.Debugf.

- How to verify it
It's convenient for me to use docker exec -it.
I don't need to resize the window.

- Description for the changelog
fixes tty initial size error

- I have no more cute animal now
Before fix:
vim /etc/mime.types

After fix:
vim /etc/mime.types

codecov-io · 2018-11-21T13:50:57Z

Codecov Report

Merging #1529 into master will decrease coverage by 0.77%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1529      +/-   ##
==========================================
- Coverage   56.07%   55.29%   -0.78%     
==========================================
  Files         306      289      -17     
  Lines       20950    19388    -1562     
==========================================
- Hits        11747    10721    -1026     
+ Misses       8354     7968     -386     
+ Partials      849      699     -150

lifubang · 2018-11-21T13:55:56Z

@thaJeztah it is the duplicate copy of #1380, PTAL.

kolyshkin · 2018-11-21T17:39:35Z

cli/command/container/tty.go

@@ -34,19 +34,48 @@ func resizeTtyTo(ctx context.Context, client client.ContainerAPIClient, id strin
 	}

 	if err != nil {
-		logrus.Debugf("Error resize: %s", err)
+		logrus.Debugf("Error resize: %s\r", err)


What is the reason for adding \r here?

@kolyshkin Nice to see you again ^_^
Thanks for your review. It's because that:

root@iZ2zeesy3n96esegm7tue0Z:~# docker --debug exec -it k8s_nginxgateway_apigateway-p46ch_default_2de895e0-e8a8-11e8-badf-00163e0af11c_0 /bin/bash DEBU[0000] Error resize: Error response from daemon: no such exec root@apigateway-p46ch:/# ls bin dev home lib64 mnt proc run srv t usr boot etc lib media opt root sbin sys tmp var root@apigateway-p46ch:/#

When we use --debug, the first shell prompt don't return to the head of line!

Nice catch! I've definitely seen it in the past.

Maybe add a comment why \r is there to the patch description, so anyone doing git blame can find the reason why.

No code change, modify patch description now. Thanks.

thaJeztah

oh, noticed I had some comments pending that I didn't post yet.

Still have to do a full review though (and I see some comments were still pending?)

thaJeztah · 2018-11-21T18:42:03Z

cli/command/container/client_test.go

+	inspectFunc         func(string) (types.ContainerJSON, error)
+	execInspectFunc     func(execID string) (types.ContainerExecInspect, error)
+	execCreateFunc      func(container string, config types.ExecConfig) (types.IDResponse, error)
+	createContainerFunc func(config *container.Config,


Looks like this signature did not change; perhaps you can revert this formatting change to keep the change focussed on the actual changes? Or did gofmt force this wrap?

Because in circleci:lint, there is an error:
cli/command/container/client_test.go:18::warning: line is 201 characters (lll),
so I need to force this wrap, or else ci will get an error.

ah! hm that's a pity (as it's not related to this change 😞)

thaJeztah · 2018-11-21T18:51:46Z

cli/command/container/tty.go


-	resizeTty()
+// safeResizeTty is to resize the tty's size safely
+func safeResizeTty(ctx context.Context, cli command.Cli, id string, isExec bool, resizeTtyFunc func(ctx context.Context, cli command.Cli, id string, isExec bool) error) {


safeResizeTty is a bit confusing for a name. Perhaps retryResizeTTY (but I'm open to better suggestions)?

Yes, I change it to initTtySize, because this function is used to init the tty's size.

thaJeztah · 2019-02-02T17:57:51Z

cli/command/container/tty.go

+	}
+	err := rttyFunc(ctx, cli, id, isExec)
+	if err != nil {
+		retryTimes := 5


The loop looked a bit complicated to me; wondering; would this work? (pls double-check; it's just a very quick draft)

go func() { var err error for retry := 0; retry < 5; retry++ { time.Sleep(10 * time.Millisecond) err = rttyFunc(ctx, cli, id, isExec) if err == nil { break } } if err != nil { fmt.Fprintln(cli.Err(), "Resize tty error, we'll use a default size. You can resize the tty size manually.") } }()

I will test this code.

Congratulations, this quick draft works very well.

silvin-lubecki · 2019-02-11T10:11:17Z

cli/command/container/tty.go

+			var err error
+			for retry := 0; retry < 5; retry++ {
+				time.Sleep(10 * time.Millisecond)
+				err = rttyFunc(ctx, cli, id, isExec)


nit:

if err = rttyFunc(ctx, cli, id, isExec); err == nil{ break }

Thanks. I also changed the line 54:

cli/cli/command/container/tty.go

Line 54 in 5017d5f

if err := rttyFunc(ctx, cli, id, isExec); err != nil {

silvin-lubecki · 2019-02-11T13:39:09Z

cli/command/container/tty.go

+				}
+			}
+			if err != nil {
+				fmt.Fprintln(cli.Err(), "Resize tty error, we'll use a default size. You can resize the tty size manually.")


What about fmt.Println(cli.Err(), "error resizing tty, falling back to default size") ?

Ah, yes, good suggestion; perhaps "failed to" or "unable to" (I think we generally use failed to for such things)

Suggested change

fmt.Fprintln(cli.Err(), "Resize tty error, we'll use a default size. You can resize the tty size manually.")

fmt.Fprintln(cli.Err(), "failed to resize tty, using default size")

Yes, but I can't use commit suggestion button in this conversation because I also need to update the test case.

Signed-off-by: Lifubang <lifubang@acmcoder.com> Signed-off-by: lifubang <lifubang@acmcoder.com>

thaJeztah

LGTM, thanks!

silvin-lubecki

LGTM, thank you @lifubang 👍 !

GordonTheTurtle added the status/0-triage label Nov 21, 2018

thaJeztah added impact/changelog status/2-code-review and removed status/0-triage labels Nov 21, 2018

kolyshkin reviewed Nov 21, 2018

View reviewed changes

lifubang force-pushed the ttyexecresize branch 2 times, most recently from a3a34a1 to c2205f7 Compare November 21, 2018 23:06

thaJeztah requested changes Feb 2, 2019

View reviewed changes

thaJeztah mentioned this pull request Feb 2, 2019

Strange terminal behaviour inside docker moby/moby#33794

Closed

thaJeztah reviewed Feb 2, 2019

View reviewed changes

lifubang force-pushed the ttyexecresize branch 3 times, most recently from 6076ddd to 37f0d7c Compare February 11, 2019 08:08

silvin-lubecki reviewed Feb 11, 2019

View reviewed changes

lifubang force-pushed the ttyexecresize branch from 37f0d7c to 5017d5f Compare February 11, 2019 10:27

silvin-lubecki reviewed Feb 11, 2019

View reviewed changes

tty initial size error

3fbffc6

Signed-off-by: Lifubang <lifubang@acmcoder.com> Signed-off-by: lifubang <lifubang@acmcoder.com>

lifubang force-pushed the ttyexecresize branch from 5017d5f to 3fbffc6 Compare February 12, 2019 01:15

thaJeztah approved these changes Feb 12, 2019

View reviewed changes

silvin-lubecki approved these changes Feb 12, 2019

View reviewed changes

silvin-lubecki merged commit 7f612bf into docker:master Feb 12, 2019

GordonTheTurtle added this to the 19.03.0 milestone Feb 12, 2019

thaJeztah mentioned this pull request Feb 21, 2019

Docker Exec does not resize terminal on container docker/for-linux#314

Closed

3 tasks

thaJeztah mentioned this pull request Mar 26, 2019

[18.09 backport] fixes 1492: tty initial size error #1775

Merged

adongy mentioned this pull request Feb 11, 2020

Python 3.8 support celery/celery#5761

Closed

thaJeztah mentioned this pull request Apr 12, 2022

investigate terminal resize issues, and (if possible) set size on creation #3554

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes 1492: tty initial size error #1529

fixes 1492: tty initial size error #1529

lifubang commented Nov 21, 2018 •

edited

Loading

codecov-io commented Nov 21, 2018 •

edited

Loading

lifubang commented Nov 21, 2018

kolyshkin Nov 21, 2018

lifubang Nov 21, 2018 •

edited

Loading

kolyshkin Nov 21, 2018

lifubang Nov 21, 2018

thaJeztah left a comment

thaJeztah Nov 21, 2018

lifubang Feb 11, 2019 •

edited

Loading

thaJeztah Feb 11, 2019

thaJeztah Nov 21, 2018

lifubang Feb 11, 2019

thaJeztah Feb 2, 2019

lifubang Feb 11, 2019

lifubang Feb 11, 2019

silvin-lubecki Feb 11, 2019

lifubang Feb 11, 2019

silvin-lubecki Feb 11, 2019

thaJeztah Feb 11, 2019

lifubang Feb 12, 2019

thaJeztah left a comment

silvin-lubecki left a comment

	fmt.Fprintln(cli.Err(), "Resize tty error, we'll use a default size. You can resize the tty size manually.")
	fmt.Fprintln(cli.Err(), "failed to resize tty, using default size")

fixes 1492: tty initial size error #1529

fixes 1492: tty initial size error #1529

Conversation

lifubang commented Nov 21, 2018 • edited Loading

codecov-io commented Nov 21, 2018 • edited Loading

Codecov Report

lifubang commented Nov 21, 2018

Choose a reason for hiding this comment

lifubang Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifubang Feb 11, 2019 • 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

thaJeztah left a comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

lifubang commented Nov 21, 2018 •

edited

Loading

codecov-io commented Nov 21, 2018 •

edited

Loading

lifubang Nov 21, 2018 •

edited

Loading

lifubang Feb 11, 2019 •

edited

Loading