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

fixes 1492: tty initial size error #1380

Closed
wants to merge 0 commits into from

Conversation

lifubang
Copy link
Contributor

@lifubang lifubang commented Sep 19, 2018

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

fixes #1492.

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.

- 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.

- 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
sk j _1irw za2kc78iody5

After fix:
vim /etc/mime.types
x z0 r5jg_i4h x6m2bny 4

@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #1380 into master will decrease coverage by 0.32%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1380      +/-   ##
==========================================
- Coverage   55.23%   54.91%   -0.33%     
==========================================
  Files         289      292       +3     
  Lines       19374    19287      -87     
==========================================
- Hits        10702    10591     -111     
- Misses       7977     8034      +57     
+ Partials      695      662      -33

@kolyshkin
Copy link
Contributor

LGTM aside from a small nitpick

@kolyshkin
Copy link
Contributor

@lifubang can you please merge your commits? It's easier to review that way (in case the second commit fixes the first one etc.)

@lifubang
Copy link
Contributor Author

lifubang commented Oct 9, 2018

@kolyshkin I have find the exact error when I met this situation.
The fault is "Error response from daemon: no such exec".
Because when dockercli call tty resize, the exec have ** not ** finished yet.

So we need to deal with this error.

@lifubang
Copy link
Contributor Author

lifubang commented Oct 9, 2018

@kolyshkin I have update the commit message:
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.

@lifubang
Copy link
Contributor Author

lifubang commented Oct 9, 2018

@kolyshkin @thaJeztah @vdemeester PTAL

@lifubang lifubang changed the title fixes tty initial size error fixes 1492: tty initial size error Oct 31, 2018
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, but added this to today's maintainers meeting to have a quick check with other maintainers.

thanks!

resizeTty()
err := resizeTty()
if err != nil {
go func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussing in the maintainers meeting with @cpuguy83 and @kolyshkin - and we're wondering if we should use a retry-loop (with a maximum number of retries) instead.

This retry will make it less racy, but it's still possible that if fails; a loop would probably prevent that situation

@lifubang wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if there is a retry-loop (with a maximum number of retries), I think it will be better.
How about the result of the maintainers' discussion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you update the PR to use a retry loop, it should be ready to go, and we can merge.

Not sure if we can write a test for this; I think that would be complicated, so I'm ok with merging without a test, but if you can think of a way to test this, that would be great (but again; that's not a blocker for me)

Copy link
Contributor Author

@lifubang lifubang Nov 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's very hard to add a test case, because there are no return value in this func, so I let a debug info here. I'll think about it.

err = resizeTty()
if err != nil && retry < retryTimes {
retry++
logrus.Debugf("Resize tty has retried %d times now\r", retry)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a bit noisy; also, since this is the CLI, I think this will be printed on stdout / stderr, not logged (I think).

I think we should either remove the debug, or only print a debug message if it failed to do the resize after retryTimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I also add a small test case.

@lifubang lifubang force-pushed the ttyexecresize branch 5 times, most recently from 06fd12f to 6bd526c Compare November 15, 2018 03:36
@lifubang lifubang closed this Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

initial windows size of docker exec -it is too small
5 participants