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

"osc exec" results in a dumb terminal and no tty #2284

Closed
brenton opened this issue May 15, 2015 · 9 comments
Closed

"osc exec" results in a dumb terminal and no tty #2284

brenton opened this issue May 15, 2015 · 9 comments
Assignees
Labels
component/kubernetes kind/bug Categorizes issue or PR as related to a bug. priority/P3

Comments

@brenton
Copy link
Contributor

brenton commented May 15, 2015

It's important to the users coming from 2.x that osc exec be roughly the same experience as ssh in earlier versions. This issue is meant to track progress towards making this a reality for GA.

Current State

Right now we're using nsenter under the covers. As best I can tell nsenter doesn't actually create a tty nor have the ability to. In addition a dumb terminal is set up by default. This means:

  • Using 'less' on a file will result in the following output:

      root@nginx:/var/log# less yum.log 
      WARNING: terminal is not fully functional
      yum.log  (press RETURN)
    

    This could be addressed today by setting a reasonable value for $TERM here: https://github.com/openshift/origin/blob/master/Godeps/_workspace/src/github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/dockertools/manager.go#L994. I hardcoded TERM=xterm in my environment and that mostly worked. The trick with this approach is that it would be more correct to automatically set this based on the terminal the user is actually using.

    It's important to add that simply connecting with osc exec and setting TERM=xterm does not work correctly today in both Kubernetes and Origin. Try doing so, running less and then using the up and down arrows and you'll see the problem. Even if Docker exec doesn't set a default terminal in docker-1.6.0-6.el7.x86_64 it's doing something to make things work as expected.

  • Without a TTY things like screen and tmux won't work. I suspect we'll get complaints about that. The workaround right now is to run script /dev/null -c tmux to trick the executable you want to run. That really seems like a hack.

@brenton brenton added the kind/bug Categorizes issue or PR as related to a bug. label May 15, 2015
@brenton
Copy link
Contributor Author

brenton commented May 15, 2015

@ncdc, you might want to follow along here too.

@brenton
Copy link
Contributor Author

brenton commented May 15, 2015

@smarterclayton, what labels/milestone should we put on this?

@smarterclayton
Copy link
Contributor

P2, bug, component/kubelet

----- Original Message -----

@smarterclayton, what labels/milestone should we put on this?


Reply to this email directly or view it on GitHub:
#2284 (comment)

@smarterclayton
Copy link
Contributor

We need dockerexec in our builds and packages stat. We probably want to vendor it into the all in one binary since I think 99% of the things it calls are already part of our import tree.

@ncdc
Copy link
Contributor

ncdc commented May 18, 2015

@smarterclayton last I read, upstream just wanted docker exec and wasn't interested in our dockerexec. How do you want to handle this? I'd like to avoid permanent UPSTREAM commits that add support for dockerexec if at all possible.

@smarterclayton
Copy link
Contributor

Upstream needs to support arbitrary executors, and we should get our stuff as a plugin in the upstream code.

On May 18, 2015, at 10:27 AM, Andy Goldstein notifications@github.com wrote:

@smarterclayton last I read, upstream just wanted docker exec and wasn't interested in our dockerexec. How do you want to handle this? I'd like to avoid permanent UPSTREAM commits that add support for dockerexec if at all possible.


Reply to this email directly or view it on GitHub.

@danmcp danmcp assigned ncdc and unassigned mrunalp May 19, 2015
@smarterclayton smarterclayton modified the milestone: 0.6.0 May 20, 2015
@ncdc
Copy link
Contributor

ncdc commented May 21, 2015

@smarterclayton @mrunalp FYI I tried to swap in our dockerexec late last night. 1st issue I hit was its attempt to get the hostname file - this exists in the infra container's directory, but not the target container's. I commented that out, and the next thing I got was a panic trying to terminate the spawned process (apparently the initial spawning failed, so it tried to kill it, and that panicked). I'm starting to dig into the details this morning.

@ncdc
Copy link
Contributor

ncdc commented Feb 1, 2016

Upstream issue: kubernetes/kubernetes#13585

@ncdc
Copy link
Contributor

ncdc commented Feb 1, 2016

Yes, we need to add SIGWINCH support. That is the only way to fix. There was an upstream community member who was interested in adding support, but I haven't seen any updates in a while. We'll get around to it eventually but there are higher priority items at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/kubernetes kind/bug Categorizes issue or PR as related to a bug. priority/P3
Projects
None yet
Development

No branches or pull requests

5 participants