git commit: Fixed Command Executor when shell is false.

[tn...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master ae8a51be6 -> 36a26c3ca


Fixed Command Executor when shell is false.

Review: https://reviews.apache.org/r/26622


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/36a26c3c
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/36a26c3c
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/36a26c3c

Branch: refs/heads/master
Commit: 36a26c3ca7b5a11df0b62540cdd7ad4c58987774
Parents: ae8a51b
Author: R.B. Boyer <ar...@nexusvector.net>
Authored: Sat Nov 1 00:49:33 2014 -0700
Committer: Timothy Chen <tn...@gmail.com>
Committed: Sat Nov 1 00:52:45 2014 -0700

----------------------------------------------------------------------
 src/slave/slave.cpp       |  28 ++++++++--
 src/tests/slave_tests.cpp | 123 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/36a26c3c/src/slave/slave.cpp
----------------------------------------------------------------------
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 96fb5f7..8aa3524 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -2674,15 +2674,33 @@ ExecutorInfo Slave::getExecutorInfo(
     executor.set_name("Command Executor " + name);
     executor.set_source(task.task_id().value());
 
-    // Copy the CommandInfo to get the URIs and environment, but
-    // update it to invoke 'mesos-executor' (unless we couldn't
-    // resolve 'mesos-executor' via 'realpath', in which case just
-    // echo the error and exit).
-    executor.mutable_command()->MergeFrom(task.command());
+    // Copy [uris, environment, container, user] fields from the
+    // CommandInfo to get the URIs and environment, and use them
+    // to invoke 'mesos-executor'.
+    executor.mutable_command()->mutable_uris()->MergeFrom(
+        task.command().uris());
+
+    if (task.command().has_environment()) {
+        executor.mutable_command()->mutable_environment()->MergeFrom(
+            task.command().environment());
+    }
+
+    if (task.command().has_container()) {
+        executor.mutable_command()->mutable_container()->MergeFrom(
+            task.command().container());
+    }
+
+    if (task.command().has_user()) {
+        executor.mutable_command()->set_user(task.command().user());
+    }
 
     Result<string> path = os::realpath(
         path::join(flags.launcher_dir, "mesos-executor"));
 
+    // Strongly enforce a specific shell setting for launching
+    // command executor.
+    executor.mutable_command()->set_shell(true);
+
     if (path.isSome()) {
       executor.mutable_command()->set_value(path.get());
     } else {

http://git-wip-us.apache.org/repos/asf/mesos/blob/36a26c3c/src/tests/slave_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/slave_tests.cpp b/src/tests/slave_tests.cpp
index a1bd00c..d2cbaf8 100644
--- a/src/tests/slave_tests.cpp
+++ b/src/tests/slave_tests.cpp
@@ -393,6 +393,129 @@ TEST_F(SlaveTest, MesosExecutorWithOverride)
 }
 
 
+// Test that we don't let task arguments bleed over as
+// mesos-executor args. For more details of this see MESOS-1873.
+//
+// This assumes the ability to execute '/bin/echo --author'.
+TEST_F(SlaveTest, MesosExecutorCommandTaskWithArgsList)
+{
+  Try<PID<Master> > master = StartMaster();
+  ASSERT_SOME(master);
+
+  // Need flags for 'executor_registration_timeout'.
+  slave::Flags flags = CreateSlaveFlags();
+  flags.isolation = "posix/cpu,posix/mem";
+
+  Try<MesosContainerizer*> containerizer =
+    MesosContainerizer::create(flags, false);
+  CHECK_SOME(containerizer);
+
+  Try<PID<Slave> > slave = StartSlave(containerizer.get());
+  ASSERT_SOME(slave);
+
+  MockScheduler sched;
+  MesosSchedulerDriver driver(
+      &sched, DEFAULT_FRAMEWORK_INFO, master.get(), DEFAULT_CREDENTIAL);
+
+  EXPECT_CALL(sched, registered(&driver, _, _))
+    .Times(1);
+
+  Future<vector<Offer> > offers;
+  EXPECT_CALL(sched, resourceOffers(&driver, _))
+    .WillOnce(FutureArg<1>(&offers))
+    .WillRepeatedly(Return()); // Ignore subsequent offers.
+
+  driver.start();
+
+  AWAIT_READY(offers);
+  EXPECT_NE(0u, offers.get().size());
+
+  // Launch a task with the command executor.
+  TaskInfo task;
+  task.set_name("");
+  task.mutable_task_id()->set_value("1");
+  task.mutable_slave_id()->MergeFrom(offers.get()[0].slave_id());
+  task.mutable_resources()->MergeFrom(offers.get()[0].resources());
+
+  // Command executor will run as user running test.
+  CommandInfo command;
+  command.set_shell(false);
+  command.set_value("/bin/echo");
+  command.add_arguments("/bin/echo");
+  command.add_arguments("--author");
+
+  task.mutable_command()->MergeFrom(command);
+
+  vector<TaskInfo> tasks;
+  tasks.push_back(task);
+
+  Future<TaskStatus> statusRunning;
+  Future<TaskStatus> statusFinished;
+  EXPECT_CALL(sched, statusUpdate(&driver, _))
+    .WillOnce(FutureArg<1>(&statusRunning))
+    .WillOnce(FutureArg<1>(&statusFinished));
+
+  driver.launchTasks(offers.get()[0].id(), tasks);
+
+  AWAIT_READY(statusRunning);
+  EXPECT_EQ(TASK_RUNNING, statusRunning.get().state());
+
+  AWAIT_READY(statusFinished);
+  EXPECT_EQ(TASK_FINISHED, statusFinished.get().state());
+
+  driver.stop();
+  driver.join();
+
+  Shutdown(); // Must shutdown before 'containerizer' gets deallocated.
+}
+
+
+// Don't let args from the CommandInfo struct bleed over into
+// mesos-executor forking. For more details of this see MESOS-1873.
+TEST_F(SlaveTest, GetExecutorInfo)
+{
+  // Create a thin dummy Slave to access underlying getExecutorInfo().
+  // Testing this method should not necessarily require an integration
+  // test as with most other methods here.
+  slave::Flags flags = CreateSlaveFlags();
+  TestContainerizer containerizer;
+  StandaloneMasterDetector detector;
+  Files files;
+  slave::StatusUpdateManager updateManager(flags);
+
+  slave::GarbageCollector gc;
+  Slave slave(flags, &detector, &containerizer, &files, &gc, &updateManager);
+
+  FrameworkID frameworkId;
+  frameworkId.set_value("20141010-221431-251662764-60288-32120-0000");
+
+  // Launch a task with the command executor.
+  TaskInfo task;
+  task.set_name("task");
+  task.mutable_task_id()->set_value("1");
+  task.mutable_slave_id()->set_value(
+      "20141010-221431-251662764-60288-32120-0001");
+  task.mutable_resources()->MergeFrom(
+      Resources::parse("cpus:0.1;mem:32").get());
+
+  CommandInfo command;
+  command.set_shell(false);
+  command.set_value("/bin/echo");
+  command.add_arguments("/bin/echo");
+  command.add_arguments("--author");
+
+  task.mutable_command()->MergeFrom(command);
+
+  const ExecutorInfo& executor = slave.getExecutorInfo(frameworkId, task);
+
+  // Now assert that it actually is running mesos-executor without any
+  // bleedover from the command we intend on running.
+  EXPECT_TRUE(executor.command().shell());
+  EXPECT_FALSE(executor.command().has_container());
+  EXPECT_EQ(0, executor.command().arguments_size());
+  EXPECT_NE(string::npos, executor.command().value().find("mesos-executor"));
+}
+
 // This test runs a command without the command user field set. The
 // command will verify the assumption that the command is run as the
 // slave user (in this case, root).