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.

Sign up for GitHub

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

Jump to bottom

Support terminal resizing for exec/attach/run #25273

Merged
merged 1 commit into from
Jul 14, 2016
Merged

Support terminal resizing for exec/attach/run #25273

merged 1 commit into from
Jul 14, 2016

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented May 6, 2016 

Add support for terminal resizing for exec, attach, and run. Note that for Docker, exec sessions
inherit the environment from the primary process, so if the container was created with tty=false,
that means the exec session's TERM variable will default to "dumb". Users can override this by
setting TERM=xterm (or whatever is appropriate) to get the correct "smart" terminal behavior.

Fixes #13585

@ncdc
Copy link
Member Author

ncdc commented May 6, 2016

@kubernetes/rh-cluster-infra @lavalamp @smarterclayton @liggitt

@ncdc
Copy link
Member Author

ncdc commented May 6, 2016

@stefwalter @kubernetes/rh-ux @kubernetes/kubectl FYI

@ncdc ncdc mentioned this pull request May 6, 2016
Closed
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 6, 2016
@ncdc ncdc added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 6, 2016
@ncdc ncdc force-pushed the exec-sigwinch branch from 091d53c to 2d83975 Compare May 6, 2016 19:33
@ncdc
Copy link
Member Author

ncdc commented May 6, 2016

I'm going to need to figure out how to support Windows as it doesn't do SIGWINCH. It looks like https://github.com/nsf/termbox-go can do it, but I don't think we need/want to pull that in as it does a lot more than what we strictly require.

@maclof
Copy link
Contributor

maclof commented May 7, 2016

@ncdc docker seems to just do this within a go routine in a for loop, https://github.com/docker/docker/blob/7fd53f7c711474791ce4292326e0b1dc7d4d6b0f/api/client/utils.go#L118

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2016
@ncdc ncdc force-pushed the exec-sigwinch branch from 2d83975 to a440854 Compare May 16, 2016 19:55
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2016
@ncdc ncdc mentioned this pull request May 17, 2016
Merged
@ncdc ncdc force-pushed the exec-sigwinch branch 2 times, most recently from 83ea429 to e8ea557 Compare May 18, 2016 14:57
@ncdc
Copy link
Member Author

ncdc commented May 18, 2016

@sttts feel free to review :-)

sttts
sttts reviewed May 18, 2016
View reviewed changes
pkg/client/unversioned/remotecommand/v1.go
}
}

func (e *streamProtocolV1) stream(conn streamCreator) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

e is a strange abbreviation for streamProtocolV1

Copy link
Member Author

Choose a reason for hiding this comment

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

It might have been an executor before. It probably is worth renaming.

@ncdc ncdc force-pushed the exec-sigwinch branch from e8ea557 to 9681600 Compare May 18, 2016 18:43
sttts
sttts reviewed May 18, 2016
View reviewed changes
pkg/kubelet/server/remotecommand/websocket.go
return channels
}

func createReadChannel(real bool) wsstream.ChannelType {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not actually creating anything.

@ncdc ncdc force-pushed the exec-sigwinch branch from 9681600 to 5b050d0 Compare May 18, 2016 20:19
@csrwng
Copy link
Contributor

csrwng commented May 20, 2016

@ncdc this is what I see in windows...
https://gist.github.com/csrwng/12ce01cca4055c25255b91fc127cef7a

However, in general, terminal support in a windows command prompt is bad. Character codes for backspace, clear, etc don't work. Maybe it's just a matter of setting the value for TERM, but so far the values I tried haven't worked (ansi, xterm, pcansi)

@csrwng
Copy link
Contributor

csrwng commented May 20, 2016

On a PowerShell window, TERM=xterm works. However, same issue with the resizing as on a regular command window.

smarterclayton
smarterclayton reviewed Jul 6, 2016
View reviewed changes
pkg/util/term/resizeevents_windows.go
"k8s.io/kubernetes/pkg/util/runtime"
)

func monitorResizeEvents(fd uintptr, resizeEvents chan<- Size, stop chan struct{}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

godoc on these functions

@ncdc ncdc mentioned this pull request Jul 7, 2016
Closed
@ncdc ncdc force-pushed the exec-sigwinch branch 2 times, most recently from 82bfa32 to 583b838 Compare July 7, 2016 16:15
@yuvipanda
Copy link
Contributor

(I'll be happy to cherry-pick and deploy this on our cluster when it's merged :D)

@smarterclayton
Copy link
Contributor

Ok, I want soak time on this. Squash and LGTM and I'll label it.

Support terminal resizing for exec/attach/run
3b21a99 
Add support for terminal resizing for exec, attach, and run. Note that for Docker, exec sessions
inherit the environment from the primary process, so if the container was created with tty=false,
that means the exec session's TERM variable will default to "dumb". Users can override this by
setting TERM=xterm (or whatever is appropriate) to get the correct "smart" terminal behavior.
@ncdc
Copy link
Member Author

ncdc commented Jul 13, 2016

Squashed

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 13, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 13, 2016

GCE e2e build/test passed for commit 3b21a99.

@sttts
Copy link
Contributor

sttts commented Jul 14, 2016

@k8s-bot unit test this issue: #28941

1 similar comment
@ncdc
Copy link
Member Author

ncdc commented Jul 14, 2016

@k8s-bot unit test this issue: #28941

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 14, 2016

GCE e2e build/test passed for commit 3b21a99.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5017214 into kubernetes:master Jul 14, 2016
@yujuhong yujuhong mentioned this pull request Jul 25, 2016
Closed
25 tasks
@yujuhong
Copy link
Contributor

/cc @kubernetes/sig-node

@yujuhong yujuhong mentioned this pull request Jul 25, 2016
Closed
@yuvipanda
Copy link
Contributor

Do you think this could possibly be cherry-picked onto 1.3.4? I can't seem to rebase easily..

k8s-github-robot pushed a commit that referenced this pull request Aug 21, 2016
Merge pull request #26541 from sttts/sttts-kubectl-exec-rc
7272cd0 
Automatic merge from submit-queue

Return container command exit codes in kubectl run/exec

Fixes #26424
Based on #25273.

TODO:
- [x] add e2e tests
- [x] investigate `kubectl run` exit code for `--restart=Never` (compare issue #24533 and PR #25253)
- [x] document exit codes
@janetkuo janetkuo mentioned this pull request Sep 7, 2016
Closed
@ncdc ncdc deleted the exec-sigwinch branch February 13, 2017 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@smarterclayton smarterclayton

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.