Windows: Changed command executor to use Subprocess. By encapsulating the job object logic inside a (Windows-only) libprocess actor, we're able to reuse `Subprocess` for launching tasks on Windows. This allows us to remove the entirety of `launchTaskWindows` and instead reuse `launchTaskPosix`, which just uses `subprocess`. This also fixes the `CommitSuicideOnTaskFailure` test, which is now enabled.
Much of the code in this commit will be moved in a subsequent commit, as there is no longer any need to separate the Posix/Windows launch paths in the command executor. Review: https://reviews.apache.org/r/57975/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/c51ebf50 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/c51ebf50 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/c51ebf50 Branch: refs/heads/master Commit: c51ebf508d9aaed2d1c75c46e72148df8aa6bb7b Parents: cb23605 Author: Andrew Schwartzmeyer <[email protected]> Authored: Mon Apr 3 14:19:10 2017 -0700 Committer: Joseph Wu <[email protected]> Committed: Tue Apr 4 16:45:16 2017 -0700 ---------------------------------------------------------------------- src/launcher/CMakeLists.txt | 2 +- src/launcher/executor.cpp | 33 +-------- src/launcher/executor.hpp | 6 -- src/launcher/posix/executor.cpp | 12 +++- src/launcher/posix/executor.hpp | 3 + src/launcher/windows/executor.cpp | 107 ------------------------------ src/launcher/windows/executor.hpp | 37 ----------- src/tests/default_executor_tests.cpp | 2 +- 8 files changed, 18 insertions(+), 184 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/CMakeLists.txt ---------------------------------------------------------------------- diff --git a/src/launcher/CMakeLists.txt b/src/launcher/CMakeLists.txt index f63f544..9df593c 100644 --- a/src/launcher/CMakeLists.txt +++ b/src/launcher/CMakeLists.txt @@ -34,7 +34,7 @@ if (NOT WIN32) else (NOT WIN32) set(EXECUTOR_EXECUTABLE_SRC ${EXECUTOR_EXECUTABLE_SRC} - windows/executor.cpp + posix/executor.cpp ) endif (NOT WIN32) http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/executor.cpp ---------------------------------------------------------------------- diff --git a/src/launcher/executor.cpp b/src/launcher/executor.cpp index bc69beb..04546a1 100644 --- a/src/launcher/executor.cpp +++ b/src/launcher/executor.cpp @@ -145,21 +145,9 @@ public: capabilities(_capabilities), frameworkId(_frameworkId), executorId(_executorId), - lastTaskStatus(None()) - { -#ifdef __WINDOWS__ - processHandle = INVALID_HANDLE_VALUE; -#endif - } + lastTaskStatus(None()) {} - virtual ~CommandExecutor() - { -#ifdef __WINDOWS__ - if (processHandle != INVALID_HANDLE_VALUE) { - ::CloseHandle(processHandle); - } -#endif // __WINDOWS__ - } + virtual ~CommandExecutor() = default; void connected() { @@ -515,7 +503,6 @@ protected: cout << "Starting task " << taskId.get() << endl; -#ifndef __WINDOWS__ pid = launchTaskPosix( command, launcherDir, @@ -525,19 +512,6 @@ protected: sandboxDirectory, workingDirectory, capabilities); -#else - // A Windows process is started using the `CREATE_SUSPENDED` flag - // and is part of a job object. While the process handle is kept - // open the reap function will work. - PROCESS_INFORMATION processInformation = launchTaskWindows( - command, - rootfs); - - pid = processInformation.dwProcessId; - ::ResumeThread(processInformation.hThread); - CloseHandle(processInformation.hThread); - processHandle = processInformation.hProcess; -#endif cout << "Forked command at " << pid << endl; @@ -1005,9 +979,6 @@ private: Option<Timer> killGracePeriodTimer; pid_t pid; -#ifdef __WINDOWS__ - HANDLE processHandle; -#endif Duration shutdownGracePeriod; Option<KillPolicy> killPolicy; Option<FrameworkInfo> frameworkInfo; http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/executor.hpp ---------------------------------------------------------------------- diff --git a/src/launcher/executor.hpp b/src/launcher/executor.hpp index c7c134a..8da63f3 100644 --- a/src/launcher/executor.hpp +++ b/src/launcher/executor.hpp @@ -18,12 +18,6 @@ #define __LAUNCHER_EXECUTOR_HPP__ -// For readability, we minimize the number of #ifdef blocks in the code by -// splitting platform specific system calls into separate directories. -#ifdef __WINDOWS__ -#include "launcher/windows/executor.hpp" -#else #include "launcher/posix/executor.hpp" -#endif // __WINDOWS__ #endif // __LAUNCHER_EXECUTOR_HPP__ http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/posix/executor.cpp ---------------------------------------------------------------------- diff --git a/src/launcher/posix/executor.cpp b/src/launcher/posix/executor.cpp index 7c4ef10..9eba24f 100644 --- a/src/launcher/posix/executor.cpp +++ b/src/launcher/posix/executor.cpp @@ -17,6 +17,9 @@ #include <iostream> #include <process/subprocess.hpp> +#ifdef __WINDOWS__ +#include <process/windows/jobobject.hpp> +#endif // __WINDOWS__ #include <stout/os.hpp> #include <stout/protobuf.hpp> @@ -88,6 +91,7 @@ pid_t launchTaskPosix( ? workingDirectory.get() : sandboxDirectory.get()); +#ifndef __WINDOWS__ // TODO(jieyu): If the task has a rootfs, the executor itself will // be running as root. Its sandbox is owned by root as well. In // order for the task to be able to access to its sandbox, we need @@ -102,6 +106,7 @@ pid_t launchTaskPosix( user.get() + ": " + chown.error()); } } +#endif // __WINDOWS__ } launchInfo.mutable_environment()->CopyFrom(environment); @@ -130,6 +135,11 @@ pid_t launchTaskPosix( argv[0] = MESOS_CONTAINERIZER; argv[1] = MesosContainerizerLaunch::NAME; + vector<process::Subprocess::ParentHook> parentHooks; +#ifdef __WINDOWS__ + parentHooks.emplace_back(Subprocess::ParentHook::CREATE_JOB()); +#endif // __WINDOWS__ + Try<Subprocess> s = subprocess( path::join(launcherDir, MESOS_CONTAINERIZER), argv, @@ -139,7 +149,7 @@ pid_t launchTaskPosix( &launchFlags, None(), None(), - {}, + parentHooks, {Subprocess::ChildHook::SETSID()}); if (s.isError()) { http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/posix/executor.hpp ---------------------------------------------------------------------- diff --git a/src/launcher/posix/executor.hpp b/src/launcher/posix/executor.hpp index 2dd9766..65a2401 100644 --- a/src/launcher/posix/executor.hpp +++ b/src/launcher/posix/executor.hpp @@ -22,6 +22,9 @@ #include <mesos/mesos.hpp> #include <stout/option.hpp> +#ifdef __WINDOWS__ +#include <stout/windows.hpp> +#endif // __WINDOWS__ namespace mesos { namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/windows/executor.cpp ---------------------------------------------------------------------- diff --git a/src/launcher/windows/executor.cpp b/src/launcher/windows/executor.cpp deleted file mode 100644 index b51fde7..0000000 --- a/src/launcher/windows/executor.cpp +++ /dev/null @@ -1,107 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "launcher/windows/executor.hpp" - -#include <iostream> - -#include <stout/os.hpp> -#include <stout/strings.hpp> -#include <stout/windows.hpp> - -#include <stout/os/close.hpp> - -using std::cout; -using std::cerr; -using std::endl; -using std::string; -using std::vector; - -namespace mesos { -namespace internal { - -PROCESS_INFORMATION launchTaskWindows( - const CommandInfo& command, - Option<string>& rootfs) -{ - PROCESS_INFORMATION processInfo; - ::ZeroMemory(&processInfo, sizeof(PROCESS_INFORMATION)); - - STARTUPINFO startupInfo; - ::ZeroMemory(&startupInfo, sizeof(STARTUPINFO)); - startupInfo.cb = sizeof(STARTUPINFO); - - string executable; - string commandLine = command.value(); - - if (command.shell()) { - // Use Windows shell (`cmd.exe`). Look for it in the system folder. - char systemDir[MAX_PATH]; - if (!::GetSystemDirectory(systemDir, MAX_PATH)) { - // No way to recover from this, safe to exit the process. - abort(); - } - - executable = path::join(systemDir, os::Shell::name); - - // `cmd.exe` needs to be used in conjunction with the `/c` parameter. - // For compliance with C-style applications, `cmd.exe` should be passed - // as `argv[0]`. - // TODO(alexnaparu): Quotes are probably needed after `/c`. - commandLine = - strings::join(" ", os::Shell::arg0, os::Shell::arg1, commandLine); - } else { - // Not a shell command, execute as-is. - executable = command.value(); - - // TODO(jieyu): Consider allowing os::stringify_args to take - // `command.arguments()` directly. - commandLine = os::stringify_args(os::raw::Argv(command.arguments())); - } - - cout << commandLine << endl; - - // There are many wrappers on `CreateProcess` that are more user-friendly, - // but they don't return the PID of the child process. - BOOL createProcessResult = ::CreateProcess( - executable.empty() ? nullptr : executable.c_str(), // Module to load. - (LPSTR) commandLine.c_str(), // Command line. - nullptr, // Default security attributes. - nullptr, // Default primary thread security attributes. - TRUE, // Inherited parent process handles. - CREATE_SUSPENDED, // Create suspended so we can wrap in job object. - nullptr, // Use parent's environment. - nullptr, // Use parent's current directory. - &startupInfo, // STARTUPINFO pointer. - &processInfo); // PROCESS_INFORMATION pointer. - - if (!createProcessResult) { - ABORT("launchTaskWindows: CreateProcess failed with error code " + - GetLastError()); - } - - Try<HANDLE> job = os::create_job(processInfo.dwProcessId); - // The job handle is not closed. The job lifetime is equal or lower - // than the process lifetime. - if (job.isError()) { - abort(); - } - - return processInfo; -} - -} // namespace internal { -} // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/launcher/windows/executor.hpp ---------------------------------------------------------------------- diff --git a/src/launcher/windows/executor.hpp b/src/launcher/windows/executor.hpp deleted file mode 100644 index 6f02912..0000000 --- a/src/launcher/windows/executor.hpp +++ /dev/null @@ -1,37 +0,0 @@ -// Licensed to the Apache Software Foundation (ASF) under one -// or more contributor license agreements. See the NOTICE file -// distributed with this work for additional information -// regarding copyright ownership. The ASF licenses this file -// to you under the Apache License, Version 2.0 (the -// "License"); you may not use this file except in compliance -// with the License. You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef __LAUNCHER_WINDOWS_EXECUTOR_HPP__ -#define __LAUNCHER_WINDOWS_EXECUTOR_HPP__ - -#include <string> - -#include <mesos/mesos.hpp> - -#include <stout/option.hpp> -#include <stout/windows.hpp> - -namespace mesos { -namespace internal { - -PROCESS_INFORMATION launchTaskWindows( - const CommandInfo& command, - Option<std::string>& rootfs); - -} // namespace internal { -} // namespace mesos { - -#endif // __LAUNCHER_WINDOWS_EXECUTOR_HPP__ http://git-wip-us.apache.org/repos/asf/mesos/blob/c51ebf50/src/tests/default_executor_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/default_executor_tests.cpp b/src/tests/default_executor_tests.cpp index 370733d..22af7e9 100644 --- a/src/tests/default_executor_tests.cpp +++ b/src/tests/default_executor_tests.cpp @@ -934,7 +934,7 @@ TEST_P(DefaultExecutorTest, ROOT_ContainerStatusForTask) // This test verifies that the default executor commits suicide when the only // task in the task group exits with a non-zero status code. -TEST_P_TEMP_DISABLED_ON_WINDOWS(DefaultExecutorTest, CommitSuicideOnTaskFailure) +TEST_P(DefaultExecutorTest, CommitSuicideOnTaskFailure) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master);
