Pulled 'ContainerIO' out of the 'ContainerLogger'. Pulled 'ContainerIO' out of the 'ContainerLogger'.
Review: https://reviews.apache.org/r/57176/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/a62d0155 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/a62d0155 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/a62d0155 Branch: refs/heads/1.2.x Commit: a62d0155f93c7e603674a3aab673f5c916828a15 Parents: 356cf67 Author: Kevin Klues <[email protected]> Authored: Wed Mar 1 09:55:25 2017 -0800 Committer: Kevin Klues <[email protected]> Committed: Wed Mar 1 10:31:43 2017 -0800 ---------------------------------------------------------------------- include/mesos/slave/container_logger.hpp | 112 +---------------- include/mesos/slave/containerizer.hpp | 124 +++++++++++++++++++ src/slave/container_loggers/lib_logrotate.cpp | 5 +- src/slave/container_loggers/lib_logrotate.hpp | 4 +- src/slave/container_loggers/sandbox.cpp | 10 +- src/slave/container_loggers/sandbox.hpp | 4 +- src/slave/containerizer/docker.cpp | 6 +- src/slave/containerizer/mesos/containerizer.cpp | 3 +- src/slave/containerizer/mesos/containerizer.hpp | 2 +- .../containerizer/mesos/io/switchboard.cpp | 21 ++-- .../containerizer/mesos/io/switchboard.hpp | 12 +- src/tests/container_logger_tests.cpp | 5 +- 12 files changed, 163 insertions(+), 145 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/include/mesos/slave/container_logger.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/slave/container_logger.hpp b/include/mesos/slave/container_logger.hpp index f759640..e8f8838 100644 --- a/include/mesos/slave/container_logger.hpp +++ b/include/mesos/slave/container_logger.hpp @@ -23,6 +23,8 @@ #include <mesos/mesos.hpp> +#include <mesos/slave/containerizer.hpp> + #include <process/future.hpp> #include <process/subprocess.hpp> #include <process/shared.hpp> @@ -53,116 +55,6 @@ class ContainerLogger { public: /** - * A collection of `process::subprocess` arguments which the container logger - * can influence. See `ContainerLogger::prepare`. - */ - struct ContainerIO - { - /** - * Describes how the container logger redirects I/O for stdout/stderr. - * See `process::Subprocess::IO`. - * - * NOTE: This wrapper prevents the container logger from redirecting I/O - * via a `Subprocess::PIPE`. This is restricted because logging must not - * be affected by the status of the agent process: - * * A `Subprocess::PIPE` will require the agent process to regularly - * read and empty the pipe. The agent does not do this. If the pipe - * fills up, the write-end of the pipe may become blocked on IO. - * * Logging must continue even if the agent dies. - */ - class IO - { - public: - enum class Type - { - FD, - PATH - }; - - static IO PATH(const std::string& path) - { - return IO(Type::PATH, path); - } - - static IO FD(int_fd fd, bool closeOnDestruction = true) - { - return IO(Type::FD, fd, closeOnDestruction); - } - - operator process::Subprocess::IO () const - { - switch (type_) { - case Type::FD: - return process::Subprocess::FD(*fd_->get()); - case Type::PATH: - return process::Subprocess::PATH(path_.get()); - default: - UNREACHABLE(); - } - } - - private: - // A simple abstraction to wrap an FD and (optionally) close it - // on destruction. We know that we never copy instances of this - // class once they are instantiated, so it's OK to call - // `close()` in the destructor since only one reference will - // ever exist to it. - class FDWrapper - { - public: - FDWrapper(int_fd _fd, bool _closeOnDestruction) - : fd(_fd), closeOnDestruction(_closeOnDestruction) {} - - ~FDWrapper() { - CHECK(fd >= 0); - if (closeOnDestruction) { - close(fd); - } - } - - operator int_fd() const { return fd; } - - private: - FDWrapper(const FDWrapper& fd) = delete; - - int_fd fd; - bool closeOnDestruction; - }; - - IO(Type _type, int_fd _fd, bool closeOnDestruction) - : type_(_type), - fd_(new FDWrapper(_fd, closeOnDestruction)), - path_(None()) {} - - IO(Type _type, const std::string& _path) - : type_(_type), - fd_(None()), - path_(_path) {} - - Type type_; - Option<process::Shared<FDWrapper>> fd_; - Option<std::string> path_; - }; - - /** - * How to redirect the stdin of the executable. - * See `process::Subprocess::IO`. - */ - IO in = IO::FD(STDIN_FILENO, false); - - /** - * How to redirect the stdout of the executable. - * See `process::Subprocess::IO`. - */ - IO out = IO::FD(STDOUT_FILENO, false); - - /** - * Similar to `out`, except this describes how to redirect stderr. - */ - IO err = IO::FD(STDERR_FILENO, false); - }; - - /** * Create and initialize a container logger instance of the given type, * specified by the `container_logger` agent flag. If the type is not * specified, a default container logger instance will be created. http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/include/mesos/slave/containerizer.hpp ---------------------------------------------------------------------- diff --git a/include/mesos/slave/containerizer.hpp b/include/mesos/slave/containerizer.hpp index d0096b9..838a8c3 100644 --- a/include/mesos/slave/containerizer.hpp +++ b/include/mesos/slave/containerizer.hpp @@ -17,7 +17,131 @@ #ifndef __MESOS_SLAVE_CONTAINERIZER_HPP__ #define __MESOS_SLAVE_CONTAINERIZER_HPP__ +#include <string> + +#include <process/shared.hpp> +#include <process/subprocess.hpp> + +#include <stout/option.hpp> + // ONLY USEFUL AFTER RUNNING PROTOC. #include <mesos/slave/containerizer.pb.h> +namespace mesos { +namespace slave { + +/** + * An abstraction around the IO classes used to redirect + * stdin/stdout/stderr to/from a container by the containerizer. + */ +struct ContainerIO +{ + /** + * Describes how the containerizer redirects I/O for + * stdin/stdout/stderr of a container. + * See `process::Subprocess::IO`. + * + * NOTE: This wrapper prevents the containerizer from redirecting I/O + * via a `Subprocess::PIPE`. This is restricted because the IO of a + * container must not be affected by the status of the agent process: + * * A `Subprocess::PIPE` will require the agent process to regularly + * read and empty the pipe. The agent does not do this. If the pipe + * fills up, the write-end of the pipe may become blocked on IO. + * * Redirection must continue even if the agent dies. + */ + class IO + { + public: + enum class Type + { + FD, + PATH + }; + + static IO PATH(const std::string& path) + { + return IO(Type::PATH, path); + } + + static IO FD(int_fd fd, bool closeOnDestruction = true) + { + return IO(Type::FD, fd, closeOnDestruction); + } + + operator process::Subprocess::IO () const + { + switch (type_) { + case Type::FD: + return process::Subprocess::FD(*fd_->get()); + case Type::PATH: + return process::Subprocess::PATH(path_.get()); + default: + UNREACHABLE(); + } + } + + private: + // A simple abstraction to wrap an FD and (optionally) close it + // on destruction. We know that we never copy instances of this + // class once they are instantiated, so it's OK to call + // `close()` in the destructor since only one reference will + // ever exist to it. + class FDWrapper + { + public: + FDWrapper(int_fd _fd, bool _closeOnDestruction) + : fd(_fd), closeOnDestruction(_closeOnDestruction) {} + + ~FDWrapper() { + CHECK(fd >= 0); + if (closeOnDestruction) { + close(fd); + } + } + + operator int_fd() const { return fd; } + + private: + FDWrapper(const FDWrapper& fd) = delete; + + int_fd fd; + bool closeOnDestruction; + }; + + IO(Type _type, int_fd _fd, bool closeOnDestruction) + : type_(_type), + fd_(new FDWrapper(_fd, closeOnDestruction)), + path_(None()) {} + + IO(Type _type, const std::string& _path) + : type_(_type), + fd_(None()), + path_(_path) {} + + Type type_; + Option<process::Shared<FDWrapper>> fd_; + Option<std::string> path_; + }; + + /** + * How to redirect the stdin of the executable. + * See `process::Subprocess::IO`. + */ + IO in = IO::FD(STDIN_FILENO, false); + + /** + * How to redirect the stdout of the executable. + * See `process::Subprocess::IO`. + */ + IO out = IO::FD(STDOUT_FILENO, false); + + /** + * Similar to `out`, except this describes how to redirect stderr. + */ + IO err = IO::FD(STDERR_FILENO, false); +}; + +} // namespace slave { +} // namespace mesos { + #endif // __MESOS_SLAVE_CONTAINERIZER_HPP__ http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/container_loggers/lib_logrotate.cpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/lib_logrotate.cpp b/src/slave/container_loggers/lib_logrotate.cpp index 5dc7d53..acaad38 100644 --- a/src/slave/container_loggers/lib_logrotate.cpp +++ b/src/slave/container_loggers/lib_logrotate.cpp @@ -24,6 +24,7 @@ #include <mesos/module/container_logger.hpp> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <process/dispatch.hpp> #include <process/future.hpp> @@ -54,14 +55,12 @@ using namespace mesos; using namespace process; using mesos::slave::ContainerLogger; +using mesos::slave::ContainerIO; namespace mesos { namespace internal { namespace logger { -using ContainerIO = ContainerLogger::ContainerIO; - - class LogrotateContainerLoggerProcess : public Process<LogrotateContainerLoggerProcess> { http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/container_loggers/lib_logrotate.hpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/lib_logrotate.hpp b/src/slave/container_loggers/lib_logrotate.hpp index 5e22ef8..561ddb6 100644 --- a/src/slave/container_loggers/lib_logrotate.hpp +++ b/src/slave/container_loggers/lib_logrotate.hpp @@ -20,6 +20,7 @@ #include <stdio.h> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <stout/bytes.hpp> #include <stout/flags.hpp> @@ -195,8 +196,7 @@ public: // This is a noop. The logrotate container logger has nothing to initialize. virtual Try<Nothing> initialize(); - virtual process::Future<mesos::slave::ContainerLogger::ContainerIO> - prepare( + virtual process::Future<mesos::slave::ContainerIO> prepare( const ExecutorInfo& executorInfo, const std::string& sandboxDirectory, const Option<std::string>& user); http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/container_loggers/sandbox.cpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/sandbox.cpp b/src/slave/container_loggers/sandbox.cpp index ec0b532..b01e359 100644 --- a/src/slave/container_loggers/sandbox.cpp +++ b/src/slave/container_loggers/sandbox.cpp @@ -21,6 +21,7 @@ #include <mesos/mesos.hpp> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <process/dispatch.hpp> #include <process/future.hpp> @@ -40,14 +41,12 @@ using namespace process; using mesos::slave::ContainerLogger; +using mesos::slave::ContainerIO; namespace mesos { namespace internal { namespace slave { -using ContainerIO = ContainerLogger::ContainerIO; - - class SandboxContainerLoggerProcess : public Process<SandboxContainerLoggerProcess> { @@ -55,7 +54,7 @@ public: SandboxContainerLoggerProcess() : ProcessBase(process::ID::generate("sandbox-logger")) {} - process::Future<ContainerLogger::ContainerIO> prepare( + process::Future<ContainerIO> prepare( const ExecutorInfo& executorInfo, const std::string& sandboxDirectory, const Option<std::string>& user) @@ -90,8 +89,7 @@ Try<Nothing> SandboxContainerLogger::initialize() } -Future<ContainerLogger::ContainerIO> -SandboxContainerLogger::prepare( +Future<ContainerIO> SandboxContainerLogger::prepare( const ExecutorInfo& executorInfo, const std::string& sandboxDirectory, const Option<std::string>& user) http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/container_loggers/sandbox.hpp ---------------------------------------------------------------------- diff --git a/src/slave/container_loggers/sandbox.hpp b/src/slave/container_loggers/sandbox.hpp index 9042cd5..fb06a70 100644 --- a/src/slave/container_loggers/sandbox.hpp +++ b/src/slave/container_loggers/sandbox.hpp @@ -24,6 +24,7 @@ #include <mesos/mesos.hpp> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <process/future.hpp> #include <process/owned.hpp> @@ -58,8 +59,7 @@ public: // Tells the subprocess to redirect the executor/task's stdout and stderr // to separate "stdout" and "stderr" files in the sandbox. // The `path`, `argv`, and `environment` are not changed. - virtual process::Future<mesos::slave::ContainerLogger::ContainerIO> - prepare( + virtual process::Future<mesos::slave::ContainerIO> prepare( const ExecutorInfo& executorInfo, const std::string& sandboxDirectory, const Option<std::string>& user); http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/containerizer/docker.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp index 979cdfe..c438ec3 100644 --- a/src/slave/containerizer/docker.cpp +++ b/src/slave/containerizer/docker.cpp @@ -21,6 +21,7 @@ #include <vector> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <process/check.hpp> #include <process/collect.hpp> @@ -71,6 +72,7 @@ using std::string; using std::vector; using mesos::slave::ContainerLogger; +using mesos::slave::ContainerIO; using mesos::slave::ContainerTermination; using mesos::internal::slave::state::SlaveState; @@ -1319,7 +1321,7 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer( container->user) .then(defer( self(), - [=](const ContainerLogger::ContainerIO& containerIO) + [=](const ContainerIO& containerIO) -> Future<Docker::Container> { // Start the executor in a Docker container. // This executor could either be a custom executor specified by an @@ -1432,7 +1434,7 @@ Future<pid_t> DockerContainerizerProcess::launchExecutorProcess( })) .then(defer( self(), - [=](const ContainerLogger::ContainerIO& containerIO) + [=](const ContainerIO& containerIO) -> Future<pid_t> { // NOTE: The child process will be blocked until all hooks have been // executed. http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/containerizer/mesos/containerizer.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp index e99db1b..b001d02 100644 --- a/src/slave/containerizer/mesos/containerizer.cpp +++ b/src/slave/containerizer/mesos/containerizer.cpp @@ -132,6 +132,7 @@ using mesos::slave::ContainerConfig; using mesos::slave::ContainerLaunchInfo; using mesos::slave::ContainerLimitation; using mesos::slave::ContainerLogger; +using mesos::slave::ContainerIO; using mesos::slave::ContainerState; using mesos::slave::ContainerTermination; using mesos::slave::Isolator; @@ -1250,7 +1251,7 @@ Future<Nothing> MesosContainerizerProcess::fetch( Future<bool> MesosContainerizerProcess::_launch( const ContainerID& containerId, - const Option<ContainerLogger::ContainerIO>& containerIO, + const Option<ContainerIO>& containerIO, const map<string, string>& environment, const SlaveID& slaveId, bool checkpoint) http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/containerizer/mesos/containerizer.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp index 8010a08..09f94cc 100644 --- a/src/slave/containerizer/mesos/containerizer.hpp +++ b/src/slave/containerizer/mesos/containerizer.hpp @@ -235,7 +235,7 @@ private: process::Future<bool> _launch( const ContainerID& containerId, - const Option<mesos::slave::ContainerLogger::ContainerIO>& containerIO, + const Option<mesos::slave::ContainerIO>& containerIO, const std::map<std::string, std::string>& environment, const SlaveID& slaveId, bool checkpoint); http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/containerizer/mesos/io/switchboard.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/io/switchboard.cpp b/src/slave/containerizer/mesos/io/switchboard.cpp index 895f619..27395f9 100644 --- a/src/slave/containerizer/mesos/io/switchboard.cpp +++ b/src/slave/containerizer/mesos/io/switchboard.cpp @@ -104,6 +104,7 @@ using mesos::slave::ContainerClass; using mesos::slave::ContainerLaunchInfo; using mesos::slave::ContainerLimitation; using mesos::slave::ContainerLogger; +using mesos::slave::ContainerIO; using mesos::slave::ContainerState; using mesos::slave::Isolator; @@ -263,7 +264,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::prepare( { // In local mode, the container will inherit agent's stdio. if (local) { - containerIOs[containerId] = ContainerLogger::ContainerIO(); + containerIOs[containerId] = ContainerIO(); return None(); } @@ -290,7 +291,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::prepare( Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare( const ContainerID& containerId, const ContainerConfig& containerConfig, - const ContainerLogger::ContainerIO& loggerIO) + const ContainerIO& loggerIO) { // On windows, we do not yet support running an io switchboard // server, so we must error out if it is required. @@ -327,7 +328,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare( // at the bottom of this function. We declare it here so we can // populate it throughout this function and only store it back to // the hashmap once we know this function has succeeded. - ContainerLogger::ContainerIO containerIO; + ContainerIO containerIO; // Manually construct pipes instead of using `Subprocess::PIPE` // so that the ownership of the FDs is properly represented. The @@ -417,7 +418,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare( stdoutFromFd = master; stderrFromFd = master; - containerIO.in = ContainerLogger::ContainerIO::IO::FD(slave.get()); + containerIO.in = ContainerIO::IO::FD(slave.get()); containerIO.out = containerIO.in; containerIO.err = containerIO.in; @@ -460,9 +461,9 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare( stdoutFromFd = outfds[0]; stderrFromFd = errfds[0]; - containerIO.in = ContainerLogger::ContainerIO::IO::FD(infds[0]); - containerIO.out = ContainerLogger::ContainerIO::IO::FD(outfds[1]); - containerIO.err = ContainerLogger::ContainerIO::IO::FD(errfds[1]); + containerIO.in = ContainerIO::IO::FD(infds[0]); + containerIO.out = ContainerIO::IO::FD(outfds[1]); + containerIO.err = ContainerIO::IO::FD(errfds[1]); } // Make sure all file descriptors opened have CLOEXEC set. @@ -689,7 +690,7 @@ Future<http::Connection> IOSwitchboard::_connect( } -Future<Option<ContainerLogger::ContainerIO>> IOSwitchboard::extractContainerIO( +Future<Option<ContainerIO>> IOSwitchboard::extractContainerIO( const ContainerID& containerId) { return dispatch(self(), [this, containerId]() { @@ -698,14 +699,14 @@ Future<Option<ContainerLogger::ContainerIO>> IOSwitchboard::extractContainerIO( } -Future<Option<ContainerLogger::ContainerIO>> IOSwitchboard::_extractContainerIO( +Future<Option<ContainerIO>> IOSwitchboard::_extractContainerIO( const ContainerID& containerId) { if (!containerIOs.contains(containerId)) { return None(); } - ContainerLogger::ContainerIO containerIO = containerIOs[containerId]; + ContainerIO containerIO = containerIOs[containerId]; containerIOs.erase(containerId); return containerIO; http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/slave/containerizer/mesos/io/switchboard.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/io/switchboard.hpp b/src/slave/containerizer/mesos/io/switchboard.hpp index 83ed029..520a6ef 100644 --- a/src/slave/containerizer/mesos/io/switchboard.hpp +++ b/src/slave/containerizer/mesos/io/switchboard.hpp @@ -81,8 +81,8 @@ public: // Transfer ownership of a `ContainerIO` struct for a given // container out of the `IOSwitchboard` and into the caller. - process::Future<Option<mesos::slave::ContainerLogger::ContainerIO>> - extractContainerIO(const ContainerID& containerID); + process::Future<Option<mesos::slave::ContainerIO>> extractContainerIO( + const ContainerID& containerID); // Helper function that returns `true` if `IOSwitchboardServer` // needs to be enabled for the given `ContainerConfig`. It must @@ -110,13 +110,13 @@ private: process::Future<Option<mesos::slave::ContainerLaunchInfo>> _prepare( const ContainerID& containerId, const mesos::slave::ContainerConfig& containerConfig, - const mesos::slave::ContainerLogger::ContainerIO& loggerIO); + const mesos::slave::ContainerIO& loggerIO); process::Future<process::http::Connection> _connect( const ContainerID& containerId) const; - process::Future<Option<mesos::slave::ContainerLogger::ContainerIO>> - _extractContainerIO(const ContainerID& containerID); + process::Future<Option<mesos::slave::ContainerIO>> _extractContainerIO( + const ContainerID& containerID); #ifndef __WINDOWS__ void reaped( @@ -136,7 +136,7 @@ private: // is shorter lived than the `Info` struct, as it should be removed // from this hash as soon as ownership is transferred out of the // `IOSwitchboard` via a call to `extractContainerIO()`. - hashmap<ContainerID, mesos::slave::ContainerLogger::ContainerIO> containerIOs; + hashmap<ContainerID, mesos::slave::ContainerIO> containerIOs; }; http://git-wip-us.apache.org/repos/asf/mesos/blob/a62d0155/src/tests/container_logger_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/container_logger_tests.cpp b/src/tests/container_logger_tests.cpp index 0a19ac2..c1495f2 100644 --- a/src/tests/container_logger_tests.cpp +++ b/src/tests/container_logger_tests.cpp @@ -21,6 +21,7 @@ #include <gmock/gmock.h> #include <mesos/slave/container_logger.hpp> +#include <mesos/slave/containerizer.hpp> #include <process/clock.hpp> #include <process/future.hpp> @@ -115,7 +116,7 @@ public: // All output is redirected to STDOUT_FILENO and STDERR_FILENO. EXPECT_CALL(*this, prepare(_, _, _)) - .WillRepeatedly(Return(mesos::slave::ContainerLogger::ContainerIO())); + .WillRepeatedly(Return(mesos::slave::ContainerIO())); } virtual ~MockContainerLogger() {} @@ -124,7 +125,7 @@ public: MOCK_METHOD3( prepare, - Future<mesos::slave::ContainerLogger::ContainerIO>( + Future<mesos::slave::ContainerIO>( const ExecutorInfo&, const string&, const Option<string>&)); };
