mesos-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From klue...@apache.org
Subject [mesos] 02/02: Removed call to 'setsid()' in command executor if TTY attached.
Date Tue, 02 Oct 2018 11:13:40 GMT
This is an automated email from the ASF dual-hosted git repository.

klueska pushed a commit to branch 1.6.x
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit 69c79efe98d55e476ddd1ca6cfdc120a88e7a97d
Author: Kevin Klues <klueska@gmail.com>
AuthorDate: Tue Oct 2 11:41:12 2018 +0200

    Removed call to 'setsid()' in command executor if TTY attached.
    
    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,
    SIGHUP, 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 task 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.
    
    Review: https://reviews.apache.org/r/68810/
---
 src/launcher/executor.cpp                        | 22 +++++++++++++++++++---
 src/slave/containerizer/mesos/io/switchboard.cpp |  7 +++++++
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp
index 541ca5b..3f606e5 100644
--- a/src/launcher/executor.cpp
+++ b/src/launcher/executor.cpp
@@ -131,6 +131,7 @@ public:
       const Option<Environment>& _taskEnvironment,
       const Option<CapabilityInfo>& _effectiveCapabilities,
       const Option<CapabilityInfo>& _boundingCapabilities,
+      const Option<string>& _ttySlavePath,
       const FrameworkID& _frameworkId,
       const ExecutorID& _executorId,
       const Duration& _shutdownGracePeriod)
@@ -155,6 +156,7 @@ public:
       taskEnvironment(_taskEnvironment),
       effectiveCapabilities(_effectiveCapabilities),
       boundingCapabilities(_boundingCapabilities),
+      ttySlavePath(_ttySlavePath),
       frameworkId(_frameworkId),
       executorId(_executorId),
       lastTaskStatus(None()) {}
@@ -406,7 +408,8 @@ protected:
       const Option<string>& sandboxDirectory,
       const Option<string>& workingDirectory,
       const Option<CapabilityInfo>& effectiveCapabilities,
-      const Option<CapabilityInfo>& boundingCapabilities)
+      const Option<CapabilityInfo>& boundingCapabilities,
+      const Option<string>& ttySlavePath)
   {
     // Prepare the flags to pass to the launch process.
     slave::MesosContainerizerLaunch::Flags launchFlags;
@@ -499,6 +502,11 @@ protected:
         [](pid_t pid) { return os::set_job_kill_on_close_limit(pid); }));
 #endif // __WINDOWS__
 
+    vector<process::Subprocess::ChildHook> childHooks;
+    if (ttySlavePath.isNone()) {
+      childHooks.emplace_back(Subprocess::ChildHook::SETSID());
+    }
+
     Try<Subprocess> s = subprocess(
         path::join(launcherDir, MESOS_CONTAINERIZER),
         argv,
@@ -509,7 +517,7 @@ protected:
         None(),
         None(),
         parentHooks,
-        {Subprocess::ChildHook::SETSID()});
+        childHooks);
 
     if (s.isError()) {
       ABORT("Failed to launch '" + commandString + "': " + s.error());
@@ -673,7 +681,8 @@ protected:
         sandboxDirectory,
         workingDirectory,
         effectiveCapabilities,
-        boundingCapabilities);
+        boundingCapabilities,
+        ttySlavePath);
 
     LOG(INFO) << "Forked command at " << pid.get();
 
@@ -1203,6 +1212,7 @@ private:
   Option<Environment> taskEnvironment;
   Option<CapabilityInfo> effectiveCapabilities;
   Option<CapabilityInfo> boundingCapabilities;
+  Option<string> ttySlavePath;
   const FrameworkID frameworkId;
   const ExecutorID executorId;
   Owned<MesosBase> mesos;
@@ -1262,6 +1272,10 @@ public:
         "bounding_capabilities",
         "The bounding set of capabilities the command can use.");
 
+    add(&Flags::tty_slave_path,
+        "tty_slave_path",
+        "A path to the slave end of the attached TTY if there is one.");
+
     add(&Flags::launcher_dir,
         "launcher_dir",
         "Directory path of Mesos binaries.",
@@ -1279,6 +1293,7 @@ public:
   Option<Environment> task_environment;
   Option<mesos::CapabilityInfo> effective_capabilities;
   Option<mesos::CapabilityInfo> bounding_capabilities;
+  Option<string> tty_slave_path;
   string launcher_dir;
 };
 
@@ -1356,6 +1371,7 @@ int main(int argc, char** argv)
           flags.task_environment,
           flags.effective_capabilities,
           flags.bounding_capabilities,
+          flags.tty_slave_path,
           frameworkId,
           executorId,
           shutdownGracePeriod));
diff --git a/src/slave/containerizer/mesos/io/switchboard.cpp b/src/slave/containerizer/mesos/io/switchboard.cpp
index e0a95ed..3089bea 100644
--- a/src/slave/containerizer/mesos/io/switchboard.cpp
+++ b/src/slave/containerizer/mesos/io/switchboard.cpp
@@ -448,6 +448,13 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
     containerIO.err = containerIO.in;
 
     launchInfo.set_tty_slave_path(slavePath.get());
+
+    // The command executor requires the `tty_slave_path`
+    // to also be passed as a command line argument.
+    if (containerConfig.has_task_info()) {
+      launchInfo.mutable_command()->add_arguments(
+            "--tty_slave_path=" + slavePath.get());
+    }
   } else {
     Try<std::array<int_fd, 2>> infds_ = os::pipe();
     if (infds_.isError()) {


Mime
View raw message