Skip to content

Handle large output in v2 shim with TTY#3743

Merged
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:v2blocking
Oct 11, 2019
Merged

Handle large output in v2 shim with TTY#3743
estesp merged 1 commit intocontainerd:masterfrom
crosbymichael:v2blocking

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Oct 11, 2019

Reized the I/O buffers to align with the size of the kernel buffers with fifos
and move the close aspect of the console to key off of the stdin closing.

Fixes #3738

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

estesp
estesp approved these changes Oct 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

Would it be worth adding a comment about why "4096" is special and why it shouldn't be changed (sounds like matching some kernel buffer is important?)

@estesp
Copy link
Copy Markdown
Member

estesp commented Oct 11, 2019

Hmm.. go test race detector doesn't like the new test though?

WARNING: DATA RACE
Read at 0x00c001955e78 by goroutine 148:
  github.com/containerd/containerd.TestContainerExecLargeOutputWithTTY()

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

estesp
estesp reviewed Oct 11, 2019
View reviewed changes
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

cpuguy83
cpuguy83 approved these changes Oct 11, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

Confirmed test fails (hangs) against master.
Also manually verified normal TTY behavior is still good.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

@crosbymichael
Handle large output in v2 shim with TTY
f8cca26 
Reized the I/O buffers to align with the size of the kernel buffers with fifos
and move the close aspect of the console to key off of the stdin closing.

Fixes containerd#3738

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Oct 11, 2019

Build succeeded.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 11, 2019

Codecov Report

Merging #3743 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3743   +/-   ##
=======================================
  Coverage   41.98%   41.98%           
=======================================
  Files         131      131           
  Lines       14528    14528           
=======================================
  Hits         6099     6099           
  Misses       7521     7521           
  Partials      908      908
Flag Coverage Δ
#linux 45.39% <ø> (ø) ⬆️
#windows 37.1% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6af355f...f8cca26. Read the comment docs.

@estesp estesp merged commit 57cfc90 into containerd:master Oct 11, 2019
@estesp estesp mentioned this pull request Oct 14, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Feb 14, 2020
Merged
@ningmingxiao ningmingxiao mentioned this pull request Sep 17, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Large output of processes with TTY gets occasionally truncated

5 participants

@crosbymichael @estesp @codecov-io @cpuguy83 @fuweid