----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68810/#review209034 -----------------------------------------------------------
src/launcher/executor.cpp Lines 520 (patched) <https://reviews.apache.org/r/68810/#comment293304> ttySlavePath.isNone() src/slave/containerizer/mesos/io/switchboard.cpp Line 446 (original), 446-448 (patched) <https://reviews.apache.org/r/68810/#comment293303> This applies to default executor as well. You should only do this for command executor. I.e., ``` if (containerConfig.has_task_info()) { launchInfo.mutable_command()->add_arguments( "--tty_slave_path=" + slavePath.get()); } ``` - Jie Yu On Sept. 22, 2018, 12:36 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68810/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2018, 12:36 a.m.) > > > Review request for mesos, Gilbert Song and Jie Yu. > > > Bugs: MESOS-8978 > https://issues.apache.org/jira/browse/MESOS-8978 > > > Repository: mesos > > > Description > ------- > > Previously, we would unconditionally call 'setsid()' in the command > executor whenever we launched a process. This causes the process it > launches to start a new session and NOT inherit the controlling TTY > from it's parent. This obviously causes problems, if the goal of > attaching a TTY to a task is so that we can actually give it control > of that TTY while it is executing. > > Interestingly, even if process does not control its TTY, it can still > read and write from the file descriptors associated with it (it just > can't receive any signals associated with it, such as SIGWINCH, NOHUP, > etc.). This is why things "appeared" to mostly work until this point > because stdin/stdout/stderr were all being redirected properly to it. > > Where we saw problems was with trying to 'attach' to an already > running process launched via the command executor using the > ATTACH_NESTED_CONTAINER_INPUT/OUTPUT calls. If you ran something like > 'bash', you would not be able to do job control, and you could not > resize the screen properly when running things like 'vim'. > > This commit fixes this issue by checking to see if a TTY has been > attached to a tasng launched by the command executor, and skipping the > 'setsid()' call when forking it. We considered a number of alternative > approaches, but finally settled on this one since it was the least > invasive. I.e. only tasks launched with a TTY (which is a failry new > concept in Mesos) will be affected by this change; the semantics of > all traditional tasks launched via the command executor will go > unchanged. > > Note: this problem does not exists for the default executor because > the agent does the job of launching all containers, and there is no > executor sitting between the containerizer and the actual process of a > task intervening to call an extra 'setsid()'. This is why containers > launched via LAUNCH_NESTED_CONTAINER_SESSION have always worked as > expected. > > > Diffs > ----- > > src/launcher/executor.cpp 86fb87dfbaa4daba22ec491ca5fca0e562797521 > src/slave/containerizer/mesos/io/switchboard.cpp > 52b0e521ed1c651c90b3a3df7c4df576288bf400 > > > Diff: https://reviews.apache.org/r/68810/diff/1/ > > > Testing > ------- > > $ make -j 40 check > > > Thanks, > > Kevin Klues > >
