You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by jo...@apache.org on 2016/12/16 02:27:25 UTC

[1/4] mesos git commit: Stout: Added helper for determining a runtime directory.

Repository: mesos
Updated Branches:
  refs/heads/master c1578bb74 -> 8d2fe7e09


Stout: Added helper for determining a runtime directory.

This is a continuation of the fix in:
https://reviews.apache.org/r/54336/

The new helper `os::var()` returns `/var` on POSIX and (usually)
`C:\\ProgramData` on Windows.  On Windows, this uses a COM API to look
up the correct location for persistent, app-local (but not per user)
variable data.

The correct place for variable runtime data on Windows is not
necessarily in a temporary directory, but in the analogous location
`ProgramData`.  Thus we need a platform-agnostic way to refer to `/var`.

NOTE: The call to `ShGetKnownFolder` is not RAII because it is a C API,
and the ATL `CComHeapPtr` class is not used in this commit due to
Windows header issues.  Thus the buffer allocated by the C API is freed
immediately after the data is copied into a `std::wstring`.  Because the
Windows API returns a UTF-16 string, and Unicode characters are valid
in Windows path names, we have to correctly convert it to UTF-8 using
`<codecvt>`.

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


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

Branch: refs/heads/master
Commit: 3b61aaea36c778a936c645cd4a7270f8498ef6bd
Parents: c1578bb
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Dec 15 15:38:14 2016 -0800
Committer: Joseph Wu <jo...@apache.org>
Committed: Thu Dec 15 16:39:32 2016 -0800

----------------------------------------------------------------------
 3rdparty/stout/include/stout/posix/os.hpp   |  6 +++++
 3rdparty/stout/include/stout/windows.hpp    |  5 ++++
 3rdparty/stout/include/stout/windows/os.hpp | 32 +++++++++++++++++++++++-
 3 files changed, 42 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/3b61aaea/3rdparty/stout/include/stout/posix/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/posix/os.hpp b/3rdparty/stout/include/stout/posix/os.hpp
index 05d27a0..397c2d6 100644
--- a/3rdparty/stout/include/stout/posix/os.hpp
+++ b/3rdparty/stout/include/stout/posix/os.hpp
@@ -445,6 +445,12 @@ inline Option<std::string> which(
 }
 
 
+inline Try<std::string> var()
+{
+  return "/var";
+}
+
+
 // Create pipes for interprocess communication.
 inline Try<Nothing> pipe(int pipe_fd[2])
 {

http://git-wip-us.apache.org/repos/asf/mesos/blob/3b61aaea/3rdparty/stout/include/stout/windows.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/windows.hpp b/3rdparty/stout/include/stout/windows.hpp
index d89c709..e641c46 100644
--- a/3rdparty/stout/include/stout/windows.hpp
+++ b/3rdparty/stout/include/stout/windows.hpp
@@ -50,6 +50,11 @@
 #error "Mesos doesn't currently support the `_UNICODE` Windows header constant"
 #endif // _UNICODE
 
+// Similarly, the Windows API uses `PWSTR` to mean `wchar_t*`,
+// but rather than assuming this is true, it should be asserted.
+static_assert(std::is_same<PWSTR, wchar_t*>::value,
+              "Expected `PWSTR` to be of type `wchar_t*`.");
+
 // An RAII `HANDLE`.
 class SharedHandle : public std::shared_ptr<void>
 {

http://git-wip-us.apache.org/repos/asf/mesos/blob/3b61aaea/3rdparty/stout/include/stout/windows/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/windows/os.hpp b/3rdparty/stout/include/stout/windows/os.hpp
index 51c8dcf..5cd9254 100644
--- a/3rdparty/stout/include/stout/windows/os.hpp
+++ b/3rdparty/stout/include/stout/windows/os.hpp
@@ -15,11 +15,13 @@
 
 #include <direct.h>
 #include <io.h>
-#include <TlHelp32.h>
 #include <Psapi.h>
+#include <shlobj.h>
+#include <TlHelp32.h>
 
 #include <sys/utime.h>
 
+#include <codecvt>
 #include <list>
 #include <map>
 #include <set>
@@ -720,6 +722,34 @@ inline Try<Nothing> kill_job(pid_t pid)
 }
 
 
+inline Try<std::string> var()
+{
+  wchar_t* var_folder = nullptr;
+
+  // Retrieves the directory of `ProgramData` using the default options.
+  // NOTE: The location of `ProgramData` is fixed and so does not
+  // depend on the current user.
+  if (::SHGetKnownFolderPath(
+          FOLDERID_ProgramData,
+          KF_FLAG_DEFAULT,
+          nullptr,
+          &var_folder) // `PWSTR` is `typedef wchar_t*`.
+      != S_OK) {
+    return WindowsError("os::var: Call to `SHGetKnownFolderPath` failed");
+  }
+
+  // Convert `wchar_t*` to `wstring`.
+  std::wstring wvar_folder(var_folder);
+
+  // Free the buffer allocated by `SHGetKnownFolderPath`.
+  CoTaskMemFree(static_cast<void*>(var_folder));
+
+  // Convert UTF-16 `wstring` to UTF-8 `string`.
+  std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>, wchar_t> converter;
+  return converter.to_bytes(wvar_folder);
+}
+
+
 // Create pipes for interprocess communication. Since the pipes cannot
 // be used directly by Posix `read/write' functions they are wrapped
 // in file descriptors, a process-local concept.


[2/4] mesos git commit: Used `os::var()` for the default value in agent `--runtime_dir`.

Posted by jo...@apache.org.
Used `os::var()` for the default value in agent `--runtime_dir`.

This commit improves the logic of `Flags::runtime_dir` to use
`os::var()` for encapsulation of `/var` vs `C:\\ProgramData`.

The flags still contain some platform specific `#ifdef`-ing as the
desired runtime path is structured differently.  On POSIX, the
default is `/var/run/mesos`.  On Windows, the default is
`C:\\ProgramData\mesos\runtime`.  Notice that the relative position of
"run(time)" is flipped.

The permission check for the default path is fixed to check actual
access permissions to the directory, instead of assuming permissions
based on comparison of usernames.

Stylistically we chose a bit of duplication as a trade-off for enhanced
readability and for auto-formatting to work properly.

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


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

Branch: refs/heads/master
Commit: a0f5caa2f1562a0d7f0247fd1940ed76e5b0f878
Parents: 3b61aae
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Dec 15 15:54:40 2016 -0800
Committer: Joseph Wu <jo...@apache.org>
Committed: Thu Dec 15 16:50:01 2016 -0800

----------------------------------------------------------------------
 src/slave/flags.cpp | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/a0f5caa2/src/slave/flags.cpp
----------------------------------------------------------------------
diff --git a/src/slave/flags.cpp b/src/slave/flags.cpp
index a1dfbf9..5fb91fb 100644
--- a/src/slave/flags.cpp
+++ b/src/slave/flags.cpp
@@ -217,22 +217,29 @@ mesos::internal::slave::Flags::Flags()
       "not across reboots). This directory will be cleared on reboot.\n"
       "(Example: `/var/run/mesos`)",
       []() -> string {
+        Try<std::string> var = os::var();
+        if (var.isSome()) {
 #ifdef __WINDOWS__
-        // TODO(josephw): After adding a platform-dependent helper
-        // for determining the "var" directory, consider removing
-        // this `#ifdef`.
-        return path::join(os::temp(), "mesos", "runtime");
+          const std::string prefix(var.get());
 #else
-        Result<string> user = os::user();
-        CHECK_SOME(user);
-
-        // TODO(andschwa): Check for permissions instead of user.
-        if (user.get() == "root") {
-          return path::join("/var", "run", "mesos");
-        } else {
-          return path::join(os::temp(), "mesos", "runtime");
-        }
+          const std::string prefix(path::join(var.get(), "run"));
+#endif // __WINDOWS__
+
+          // We check for access on the prefix because the remainder
+          // of the directory structure is created by the agent later.
+          Try<bool> access = os::access(prefix, R_OK | W_OK);
+          if (access.isSome() && access.get()) {
+#ifdef __WINDOWS__
+            return path::join(prefix, "mesos", "runtime");
+#else
+            return path::join(prefix, "mesos");
 #endif // __WINDOWS__
+          }
+        }
+
+        // We provide a fallback path for ease of use in case `os::var()`
+        // errors or if the directory is not accessible.
+        return path::join(os::temp(), "mesos", "runtime");
       }());
 
   add(&Flags::launcher_dir, // TODO(benh): This needs a better name.


[4/4] mesos git commit: Windows: Added trivially passable tests to build.

Posted by jo...@apache.org.
Windows: Added trivially passable tests to build.

The tests in these files will build, run, and pass on Windows without
any further changes.

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


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

Branch: refs/heads/master
Commit: 8d2fe7e09eaa6c8270d5398c86b4b52c29d172f7
Parents: a22c733
Author: Alex Clemmer <cl...@gmail.com>
Authored: Thu Dec 15 12:43:44 2016 -0800
Committer: Joseph Wu <jo...@apache.org>
Committed: Thu Dec 15 17:15:09 2016 -0800

----------------------------------------------------------------------
 src/tests/CMakeLists.txt | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/8d2fe7e0/src/tests/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/src/tests/CMakeLists.txt b/src/tests/CMakeLists.txt
index 6aab511..a1e9944 100644
--- a/src/tests/CMakeLists.txt
+++ b/src/tests/CMakeLists.txt
@@ -88,12 +88,31 @@ endif (LINUX)
 set(MESOS_TESTS_SRC
   ${MESOS_TESTS_SRC}
   ${MESOS_TESTS_UTILS_SRC}
+  attributes_tests.cpp
   authentication_tests.cpp
+  exception_tests.cpp
+  executor_http_api_tests.cpp
+  http_authentication_tests.cpp
+  http_fault_tolerance_tests.cpp
+  master_maintenance_tests.cpp
+  master_slave_reconciliation_tests.cpp
+  protobuf_io_tests.cpp
+  resources_tests.cpp
+  scheduler_driver_tests.cpp
+  scheduler_event_call_tests.cpp
+  scheduler_http_api_tests.cpp
+  slave_validation_tests.cpp
+  sorter_tests.cpp
+  uri_tests.cpp
+  values_tests.cpp
+  zookeeper_url_tests.cpp
   )
 
 set(MESOS_TESTS_SRC
   ${MESOS_TESTS_SRC}
+  common/http_tests.cpp
   common/recordio_tests.cpp
+  common/type_utils_tests.cpp
   )
 
 if (NOT WIN32)
@@ -101,7 +120,6 @@ if (NOT WIN32)
     ${MESOS_TESTS_SRC}
     anonymous_tests.cpp
     api_tests.cpp
-    attributes_tests.cpp
     authorization_tests.cpp
     command_executor_tests.cpp
     container_logger_tests.cpp
@@ -111,8 +129,6 @@ if (NOT WIN32)
     disk_quota_tests.cpp
     dynamic_weights_tests.cpp
     examples_tests.cpp
-    exception_tests.cpp
-    executor_http_api_tests.cpp
     fault_tolerance_tests.cpp
     fetcher_cache_tests.cpp
     fetcher_tests.cpp
@@ -122,16 +138,12 @@ if (NOT WIN32)
     health_check_tests.cpp
     hierarchical_allocator_tests.cpp
     hook_tests.cpp
-    http_authentication_tests.cpp
-    http_fault_tolerance_tests.cpp
     log_tests.cpp
     logging_tests.cpp
     master_allocator_tests.cpp
     master_authorization_tests.cpp
     master_contender_detector_tests.cpp
-    master_maintenance_tests.cpp
     master_quota_tests.cpp
-    master_slave_reconciliation_tests.cpp
     master_tests.cpp
     master_validation_tests.cpp
     metrics_tests.cpp
@@ -141,7 +153,6 @@ if (NOT WIN32)
     paths_tests.cpp
     persistent_volume_endpoints_tests.cpp
     persistent_volume_tests.cpp
-    protobuf_io_tests.cpp
     protobuf_utils_tests.cpp
     rate_limiting_tests.cpp
     reconciliation_tests.cpp
@@ -149,31 +160,20 @@ if (NOT WIN32)
     reservation_endpoints_tests.cpp
     reservation_tests.cpp
     resource_offers_tests.cpp
-    resources_tests.cpp
     role_tests.cpp
-    scheduler_driver_tests.cpp
-    scheduler_event_call_tests.cpp
-    scheduler_http_api_tests.cpp
     scheduler_tests.cpp
     slave_authorization_tests.cpp
     slave_recovery_tests.cpp
     slave_tests.cpp
-    slave_validation_tests.cpp
-    sorter_tests.cpp
     state_tests.cpp
     status_update_manager_tests.cpp
     teardown_tests.cpp
     uri_fetcher_tests.cpp
-    uri_tests.cpp
-    values_tests.cpp
-    zookeeper_url_tests.cpp
     )
 
   set(MESOS_TESTS_SRC
     ${MESOS_TESTS_SRC}
     common/command_utils_tests.cpp
-    common/http_tests.cpp
-    common/type_utils_tests.cpp
     )
 
   set(MESOS_TESTS_SRC


[3/4] mesos git commit: Windows: Implemented `os::user()'.

Posted by jo...@apache.org.
Windows: Implemented `os::user()'.

This adds a basic implementation for `os::user` that returns a string.
For now, this string should only be used to populate the `user` field
in protobufs, such as the `FrameworkInfo` protobuf.  No other usage
of `os::user` is supported.

This replaces review: https://reviews.apache.org/r/53706/

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


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

Branch: refs/heads/master
Commit: a22c733ae0b3c4e457c64b7eaf8770dee4461f42
Parents: a0f5caa
Author: Daniel Pravat <dp...@outlook.com>
Authored: Thu Dec 15 17:05:50 2016 -0800
Committer: Joseph Wu <jo...@apache.org>
Committed: Thu Dec 15 17:15:09 2016 -0800

----------------------------------------------------------------------
 3rdparty/stout/cmake/StoutConfigure.cmake      |  1 +
 3rdparty/stout/include/stout/os/windows/su.hpp | 42 ++++++++++++++++++++-
 3rdparty/stout/tests/os_tests.cpp              | 13 +++++++
 3 files changed, 55 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/a22c733a/3rdparty/stout/cmake/StoutConfigure.cmake
----------------------------------------------------------------------
diff --git a/3rdparty/stout/cmake/StoutConfigure.cmake b/3rdparty/stout/cmake/StoutConfigure.cmake
index 7e483aa..d8da0f0 100644
--- a/3rdparty/stout/cmake/StoutConfigure.cmake
+++ b/3rdparty/stout/cmake/StoutConfigure.cmake
@@ -111,6 +111,7 @@ if (WIN32)
     ${ZLIB_LFLAG}
     ws2_32
     Mswsock
+    Secur32
     )
 else (WIN32)
   set(STOUT_LIBS

http://git-wip-us.apache.org/repos/asf/mesos/blob/a22c733a/3rdparty/stout/include/stout/os/windows/su.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/os/windows/su.hpp b/3rdparty/stout/include/stout/os/windows/su.hpp
index 1bb7096..573fdef 100644
--- a/3rdparty/stout/include/stout/os/windows/su.hpp
+++ b/3rdparty/stout/include/stout/os/windows/su.hpp
@@ -18,6 +18,7 @@
 #define __STOUT_OS_WINDOWS_SU_HPP__
 
 #include <string>
+#include <vector>
 
 #include <stout/error.hpp>
 #include <stout/nothing.hpp>
@@ -26,6 +27,22 @@
 
 #include <stout/windows.hpp>
 
+// Include for `GetUserNameEx`. `SECURITY_WIN32` or `SECURITY_KERNEL` must be
+// defined to include `SecExt.h`, which defines `GetUserNameEx` (the choice
+// depends on whether you want the functions defined to be usermode or kernel
+// operations). We include `security.h` instead of `SecExt.h` because comments
+// in this header indicate that it should only be included from `security.h`.
+// Finally, we `#undef` to avoid accidentally interfering with Windows headers
+// that might be sensitive to `SECURITY_WIN32`.
+#if defined(SECURITY_WIN32) || defined(SECURITY_KERNEL)
+#include <security.h>
+#else
+#define SECURITY_WIN32
+#include <security.h>
+#undef SECURITY_WIN32
+#endif // SECURITY_WIN32 || SECURITY_KERNEL
+
+
 namespace os {
 
 // NOTE: We delete these functions because they are not meaningful on Windows.
@@ -50,9 +67,32 @@ inline Result<uid_t> getuid(const Option<std::string>& user = None()) = delete;
 inline Result<gid_t> getgid(const Option<std::string>& user = None()) = delete;
 
 
+// Returns the SAM account name for the current user. This username is
+// unprocessed, meaning it contains punctuation, possibly including '\'.
+// NOTE: The `uid` parameter is unsupported on Windows, and will result in an
+// error.
 inline Result<std::string> user(Option<uid_t> uid = None())
 {
-  return WindowsError(ERROR_NOT_SUPPORTED);
+  if (uid.isSome()) {
+    return Error(
+        "os::user: Retrieving user information via uid "
+        "is not supported on Windows");
+  }
+
+  EXTENDED_NAME_FORMAT username_format = NameSamCompatible;
+  ULONG buffer_size = 0;
+  if (::GetUserNameEx(username_format, nullptr, &buffer_size) == FALSE) {
+    if (::GetLastError() != ERROR_MORE_DATA) {
+      return WindowsError("os::user: Failed to get buffer size for username");
+    }
+  }
+
+  std::vector<TCHAR> user_name(buffer_size);
+  if (::GetUserNameEx(username_format, &user_name[0], &buffer_size) == FALSE) {
+    return WindowsError("os::user: Failed to get username from OS");
+  }
+
+  return std::string(&user_name[0]);
 }
 
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/a22c733a/3rdparty/stout/tests/os_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os_tests.cpp b/3rdparty/stout/tests/os_tests.cpp
index bed1449..8d2005b 100644
--- a/3rdparty/stout/tests/os_tests.cpp
+++ b/3rdparty/stout/tests/os_tests.cpp
@@ -719,6 +719,19 @@ TEST_F(OsTest, User)
 #endif // __WINDOWS__
 
 
+TEST_F(OsTest, TrivialUser)
+{
+  const Result<string> user1 = os::user();
+  ASSERT_SOME(user1);
+  ASSERT_NE("", user1.get());
+
+#ifdef __WINDOWS__
+  const Result<string> user2 = os::user(INT_MAX);
+  ASSERT_ERROR(user2);
+#endif // __WINDOWS__
+}
+
+
 // TODO(hausdorff): Look into enabling this on Windows. Right now,
 // `LD_LIBRARY_PATH` doesn't exist on Windows, so `setPaths` doesn't work. See
 // MESOS-5940.