You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by kl...@apache.org on 2017/03/01 18:07:36 UTC
[1/4] mesos git commit: Renamed 'SubprocessInfo' to 'ContainerIO' in
'ContainerLogger'.
Repository: mesos
Updated Branches:
refs/heads/master fbdbf08fa -> 0d717e08e
Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.
Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.
Review: https://reviews.apache.org/r/57121/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/cbb99ce0
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/cbb99ce0
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/cbb99ce0
Branch: refs/heads/master
Commit: cbb99ce0a60c08eb0156461a8c3c3ef5ce8646ca
Parents: fbdbf08
Author: Kevin Klues <kl...@gmail.com>
Authored: Wed Mar 1 09:55:13 2017 -0800
Committer: Kevin Klues <kl...@gmail.com>
Committed: Wed Mar 1 09:55:13 2017 -0800
----------------------------------------------------------------------
include/mesos/slave/container_logger.hpp | 16 ++++++------
src/slave/container_loggers/lib_logrotate.cpp | 14 +++++------
src/slave/container_loggers/lib_logrotate.hpp | 2 +-
src/slave/container_loggers/sandbox.cpp | 14 +++++------
src/slave/container_loggers/sandbox.hpp | 2 +-
src/slave/containerizer/docker.cpp | 12 ++++-----
.../containerizer/mesos/io/switchboard.cpp | 26 ++++++++++----------
.../containerizer/mesos/io/switchboard.hpp | 2 +-
src/tests/container_logger_tests.cpp | 4 +--
9 files changed, 46 insertions(+), 46 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/include/mesos/slave/container_logger.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/slave/container_logger.hpp b/include/mesos/slave/container_logger.hpp
index a3f619b..2ede5f2 100644
--- a/include/mesos/slave/container_logger.hpp
+++ b/include/mesos/slave/container_logger.hpp
@@ -55,7 +55,7 @@ public:
* A collection of `process::subprocess` arguments which the container logger
* can influence. See `ContainerLogger::prepare`.
*/
- struct SubprocessInfo
+ struct ContainerIO
{
/**
* Describes how the container logger redirects I/O for stdout/stderr.
@@ -127,12 +127,12 @@ public:
* How to redirect the stdout of the executable.
* See `process::Subprocess::IO`.
*/
- IO out = SubprocessInfo::IO::FD(STDOUT_FILENO);
+ IO out = IO::FD(STDOUT_FILENO);
/**
* Similar to `out`, except this describes how to redirect stderr.
*/
- IO err = SubprocessInfo::IO::FD(STDERR_FILENO);
+ IO err = IO::FD(STDERR_FILENO);
};
/**
@@ -162,10 +162,10 @@ public:
*
* The container logger is given some of the arguments which the containerizer
* will use to launch a container. The container logger should return a
- * `SubprocessInfo` which tells the containerizer how to handle the stdout
- * and stderr of the subprocess. The container logger can modify the fields
- * within the `SubprocessInfo` as much as necessary, with some exceptions;
- * see the struct `SubprocessInfo` above.
+ * `ContainerIO` which tells the containerizer how to handle the stdout
+ * and stderr of the container. The container logger can modify the fields
+ * within the `ContainerIO` as much as necessary, with some exceptions;
+ * see the struct `ContainerIO` above.
*
* NOTE: The container logger should not lose stdout/stderr if the agent
* fails over. Additionally, if the container logger is stateful, the logger
@@ -178,7 +178,7 @@ public:
* executor's sandbox, such as persistent state between agent failovers.
* NOTE: All files in the sandbox are exposed via the `/files` endpoint.
*/
- virtual process::Future<SubprocessInfo> prepare(
+ virtual process::Future<ContainerIO> prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
const Option<std::string>& user) = 0;
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/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 b257f48..5dc7d53 100644
--- a/src/slave/container_loggers/lib_logrotate.cpp
+++ b/src/slave/container_loggers/lib_logrotate.cpp
@@ -59,7 +59,7 @@ namespace mesos {
namespace internal {
namespace logger {
-using SubprocessInfo = ContainerLogger::SubprocessInfo;
+using ContainerIO = ContainerLogger::ContainerIO;
class LogrotateContainerLoggerProcess :
@@ -71,7 +71,7 @@ public:
// Spawns two subprocesses that read from their stdin and write to
// "stdout" and "stderr" files in the sandbox. The subprocesses will rotate
// the files according to the configured maximum size and number of files.
- Future<SubprocessInfo> prepare(
+ Future<ContainerIO> prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
const Option<std::string>& user)
@@ -252,10 +252,10 @@ public:
}
// NOTE: The ownership of these FDs is given to the caller of this function.
- ContainerLogger::SubprocessInfo info;
- info.out = SubprocessInfo::IO::FD(outfds.write.get());
- info.err = SubprocessInfo::IO::FD(errfds.write.get());
- return info;
+ ContainerIO io;
+ io.out = ContainerIO::IO::FD(outfds.write.get());
+ io.err = ContainerIO::IO::FD(errfds.write.get());
+ return io;
}
protected:
@@ -285,7 +285,7 @@ Try<Nothing> LogrotateContainerLogger::initialize()
}
-Future<SubprocessInfo> LogrotateContainerLogger::prepare(
+Future<ContainerIO> LogrotateContainerLogger::prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
const Option<std::string>& user)
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/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 e37d99c..5e22ef8 100644
--- a/src/slave/container_loggers/lib_logrotate.hpp
+++ b/src/slave/container_loggers/lib_logrotate.hpp
@@ -195,7 +195,7 @@ public:
// This is a noop. The logrotate container logger has nothing to initialize.
virtual Try<Nothing> initialize();
- virtual process::Future<mesos::slave::ContainerLogger::SubprocessInfo>
+ virtual process::Future<mesos::slave::ContainerLogger::ContainerIO>
prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/src/slave/container_loggers/sandbox.cpp
----------------------------------------------------------------------
diff --git a/src/slave/container_loggers/sandbox.cpp b/src/slave/container_loggers/sandbox.cpp
index b55e089..ec0b532 100644
--- a/src/slave/container_loggers/sandbox.cpp
+++ b/src/slave/container_loggers/sandbox.cpp
@@ -45,7 +45,7 @@ namespace mesos {
namespace internal {
namespace slave {
-using SubprocessInfo = ContainerLogger::SubprocessInfo;
+using ContainerIO = ContainerLogger::ContainerIO;
class SandboxContainerLoggerProcess :
@@ -55,17 +55,17 @@ public:
SandboxContainerLoggerProcess()
: ProcessBase(process::ID::generate("sandbox-logger")) {}
- process::Future<ContainerLogger::SubprocessInfo> prepare(
+ process::Future<ContainerLogger::ContainerIO> prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
const Option<std::string>& user)
{
- ContainerLogger::SubprocessInfo info;
+ ContainerIO io;
- info.out = SubprocessInfo::IO::PATH(path::join(sandboxDirectory, "stdout"));
- info.err = SubprocessInfo::IO::PATH(path::join(sandboxDirectory, "stderr"));
+ io.out = ContainerIO::IO::PATH(path::join(sandboxDirectory, "stdout"));
+ io.err = ContainerIO::IO::PATH(path::join(sandboxDirectory, "stderr"));
- return info;
+ return io;
}
};
@@ -90,7 +90,7 @@ Try<Nothing> SandboxContainerLogger::initialize()
}
-Future<ContainerLogger::SubprocessInfo>
+Future<ContainerLogger::ContainerIO>
SandboxContainerLogger::prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/src/slave/container_loggers/sandbox.hpp
----------------------------------------------------------------------
diff --git a/src/slave/container_loggers/sandbox.hpp b/src/slave/container_loggers/sandbox.hpp
index 4ec090c..9042cd5 100644
--- a/src/slave/container_loggers/sandbox.hpp
+++ b/src/slave/container_loggers/sandbox.hpp
@@ -58,7 +58,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::SubprocessInfo>
+ virtual process::Future<mesos::slave::ContainerLogger::ContainerIO>
prepare(
const ExecutorInfo& executorInfo,
const std::string& sandboxDirectory,
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/src/slave/containerizer/docker.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp
index 823a39e..c4bdbca 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -1324,7 +1324,7 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer(
container->user)
.then(defer(
self(),
- [=](const ContainerLogger::SubprocessInfo& subprocessInfo)
+ [=](const ContainerLogger::ContainerIO& containerIO)
-> Future<Docker::Container> {
Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
container->container,
@@ -1351,8 +1351,8 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer(
// ExecutorInfo, or the docker executor.
Future<Option<int>> run = docker->run(
runOptions.get(),
- subprocessInfo.out,
- subprocessInfo.err);
+ containerIO.out,
+ containerIO.err);
// It's possible that 'run' terminates before we're able to
// obtain an 'inspect' result. It's also possible that 'run'
@@ -1463,7 +1463,7 @@ Future<pid_t> DockerContainerizerProcess::launchExecutorProcess(
}))
.then(defer(
self(),
- [=](const ContainerLogger::SubprocessInfo& subprocessInfo)
+ [=](const ContainerLogger::ContainerIO& containerIO)
-> Future<pid_t> {
// NOTE: The child process will be blocked until all hooks have been
// executed.
@@ -1513,8 +1513,8 @@ Future<pid_t> DockerContainerizerProcess::launchExecutorProcess(
path::join(flags.launcher_dir, MESOS_DOCKER_EXECUTOR),
argv,
Subprocess::PIPE(),
- subprocessInfo.out,
- subprocessInfo.err,
+ containerIO.out,
+ containerIO.err,
&launchFlags,
environment,
None(),
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/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 820d53b..c9e6f63 100644
--- a/src/slave/containerizer/mesos/io/switchboard.cpp
+++ b/src/slave/containerizer/mesos/io/switchboard.cpp
@@ -290,7 +290,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::prepare(
Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
const ContainerID& containerId,
const ContainerConfig& containerConfig,
- const ContainerLogger::SubprocessInfo& loggerInfo)
+ const ContainerLogger::ContainerIO& loggerIO)
{
// On windows, we do not yet support running an io switchboard
// server, so we must error out if it is required.
@@ -310,31 +310,31 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
ContainerIO* out = launchInfo.mutable_out();
ContainerIO* err = launchInfo.mutable_err();
- switch (loggerInfo.out.type()) {
+ switch (loggerIO.out.type()) {
#ifndef __WINDOWS__
- case ContainerLogger::SubprocessInfo::IO::Type::FD:
+ case ContainerLogger::ContainerIO::IO::Type::FD:
out->set_type(ContainerIO::FD);
- out->set_fd(loggerInfo.out.fd().get());
+ out->set_fd(loggerIO.out.fd().get());
break;
#endif
- case ContainerLogger::SubprocessInfo::IO::Type::PATH:
+ case ContainerLogger::ContainerIO::IO::Type::PATH:
out->set_type(ContainerIO::PATH);
- out->set_path(loggerInfo.out.path().get());
+ out->set_path(loggerIO.out.path().get());
break;
default:
UNREACHABLE();
}
- switch (loggerInfo.err.type()) {
+ switch (loggerIO.err.type()) {
#ifndef __WINDOWS__
- case ContainerLogger::SubprocessInfo::IO::Type::FD:
+ case ContainerLogger::ContainerIO::IO::Type::FD:
err->set_type(ContainerIO::FD);
- err->set_fd(loggerInfo.err.fd().get());
+ err->set_fd(loggerIO.err.fd().get());
break;
#endif
- case ContainerLogger::SubprocessInfo::IO::Type::PATH:
+ case ContainerLogger::ContainerIO::IO::Type::PATH:
err->set_type(ContainerIO::PATH);
- err->set_path(loggerInfo.err.path().get());
+ err->set_path(loggerIO.err.path().get());
break;
default:
UNREACHABLE();
@@ -588,8 +588,8 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
path::join(flags.launcher_dir, IOSwitchboardServer::NAME),
{IOSwitchboardServer::NAME},
Subprocess::PATH("/dev/null"),
- loggerInfo.out,
- loggerInfo.err,
+ loggerIO.out,
+ loggerIO.err,
&switchboardFlags,
environment,
None(),
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/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 5b1fa25..7189602 100644
--- a/src/slave/containerizer/mesos/io/switchboard.hpp
+++ b/src/slave/containerizer/mesos/io/switchboard.hpp
@@ -105,7 +105,7 @@ private:
process::Future<Option<mesos::slave::ContainerLaunchInfo>> _prepare(
const ContainerID& containerId,
const mesos::slave::ContainerConfig& containerConfig,
- const mesos::slave::ContainerLogger::SubprocessInfo& loggerInfo);
+ const mesos::slave::ContainerLogger::ContainerIO& loggerIO);
process::Future<process::http::Connection> _connect(
const ContainerID& containerId) const;
http://git-wip-us.apache.org/repos/asf/mesos/blob/cbb99ce0/src/tests/container_logger_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/container_logger_tests.cpp b/src/tests/container_logger_tests.cpp
index 5f2a7b0..b0cd83a 100644
--- a/src/tests/container_logger_tests.cpp
+++ b/src/tests/container_logger_tests.cpp
@@ -115,7 +115,7 @@ public:
// All output is redirected to STDOUT_FILENO and STDERR_FILENO.
EXPECT_CALL(*this, prepare(_, _, _))
- .WillRepeatedly(Return(mesos::slave::ContainerLogger::SubprocessInfo()));
+ .WillRepeatedly(Return(mesos::slave::ContainerLogger::ContainerIO()));
}
virtual ~MockContainerLogger() {}
@@ -124,7 +124,7 @@ public:
MOCK_METHOD3(
prepare,
- Future<mesos::slave::ContainerLogger::SubprocessInfo>(
+ Future<mesos::slave::ContainerLogger::ContainerIO>(
const ExecutorInfo&, const string&, const Option<string>&));
};
[2/4] mesos git commit: Added STDIN IO to
'ContainerLogger::ContainerIO' class.
Posted by kl...@apache.org.
Added STDIN IO to 'ContainerLogger::ContainerIO' class.
Added STDIN IO to 'ContainerLogger::ContainerIO' class.
Review: https://reviews.apache.org/r/57122/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/3aa7e1b6
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/3aa7e1b6
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/3aa7e1b6
Branch: refs/heads/master
Commit: 3aa7e1b67074eb67794bf2e4c18ff24d2ecf37f3
Parents: cbb99ce
Author: Kevin Klues <kl...@gmail.com>
Authored: Wed Mar 1 09:55:18 2017 -0800
Committer: Kevin Klues <kl...@gmail.com>
Committed: Wed Mar 1 09:55:18 2017 -0800
----------------------------------------------------------------------
include/mesos/slave/container_logger.hpp | 6 ++++++
1 file changed, 6 insertions(+)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/3aa7e1b6/include/mesos/slave/container_logger.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/slave/container_logger.hpp b/include/mesos/slave/container_logger.hpp
index 2ede5f2..e0cf320 100644
--- a/include/mesos/slave/container_logger.hpp
+++ b/include/mesos/slave/container_logger.hpp
@@ -124,6 +124,12 @@ public:
};
/**
+ * How to redirect the stdin of the executable.
+ * See `process::Subprocess::IO`.
+ */
+ IO in = IO::FD(STDIN_FILENO);
+
+ /**
* How to redirect the stdout of the executable.
* See `process::Subprocess::IO`.
*/
[3/4] mesos git commit: Fixed ContainerLogger / IOSwitchboard FD
leaks.
Posted by kl...@apache.org.
Fixed ContainerLogger / IOSwitchboard FD leaks.
Previously, the containizer launch path would leak FDs if the
containerizer launch path failed between successfully calling
prepare() on either the ContainerLogger (in the case of the Docker
containerizer) or the IOSwitchboard (in the case of the mesos
containerizer) and forking the actual container.
These components relied on the Subprocess call inside launcher->fork()
to close these FDS on their behalf. If the containerizer launch path
failed somewhere between calling prepare() and making this fork()
call, these FDs would never be closed.
In the case of the IOSwitchboard, this would lead to deadlock in the
destroy path because the future returned by the IOSwitchboard's
cleanup function would never be satisfied. The IOSwitchboard doesn't
shutdown until the FDs it allocates to the container have been closed.
This commit fixes this problem by updating the
ContainerLogger::ContainerIO::FD abstraction to change the way it
manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
label and forcing the launcher->fork() call to deal with closing the
FDs once it's forked a new subprocess, we now do things slightly
differently now.
We now keep the default DUP label on each FD (instead fo changing it
to OWNED) to cause launcher->fork() to dup it before mapping it onto
the stdin/stdout/stderr of the subprocess. It then only closes the
duped FD, leaving the original one open.
In doing so, it's now the containerizer's responsibility to ensure
that these FDs are closed properly (whether that's between a
successful prepare() call and launcher->fork()) or after
launcher->fork() has completed successfully). While this has the
potential to complicate things slightly on the SUCCESS path,
at least it is now the containerizers's responsibility to close these
FDS in *all* cases, rather than splitting that responsibility across
components.
In order to simplify this, we've also modified the
ContainerLogger::ContainerIO::FD abstraction to hold a Shared
pointer to its underlying file descriptor and (optionally) close it on
destruction. With this, we can ensure that all file descriptors
created through this abstraction will be automatically closed onced
their final reference goes out of scope (even if its been copied
around several times).
In essence, this releases the containerizer from the burden of manually
closing these FDS itself. So long as it holds the final reference to
these FDs (which it does), they will be automatically closed along
*any* path out of containerizer->launch(). These are exactly the
semantics we want to achieve.
In the case of the the ContainerLogger, ownership of these FDs (and
thus their final reference) is passed to the containerizer in the
ContainerIO struct returned by prepare(). In the case of the
IOSwitchboard, we had to add a new API call to transfer ownership
(since it is an isolator and prepare() can only return a protobuf),
but the end result is the same.
Review: https://reviews.apache.org/r/56195/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/48c376a5
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/48c376a5
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/48c376a5
Branch: refs/heads/master
Commit: 48c376a5fdbee4569e4af9144139d638c7924548
Parents: 3aa7e1b
Author: Kevin Klues <kl...@gmail.com>
Authored: Wed Mar 1 09:55:21 2017 -0800
Committer: Kevin Klues <kl...@gmail.com>
Committed: Wed Mar 1 09:55:21 2017 -0800
----------------------------------------------------------------------
include/mesos/slave/container_logger.hpp | 65 ++++++++----
include/mesos/slave/containerizer.proto | 21 ----
src/slave/containerizer/mesos/containerizer.cpp | 78 +++-----------
src/slave/containerizer/mesos/containerizer.hpp | 1 +
.../containerizer/mesos/io/switchboard.cpp | 106 +++++++++----------
.../containerizer/mesos/io/switchboard.hpp | 17 +++
6 files changed, 127 insertions(+), 161 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/48c376a5/include/mesos/slave/container_logger.hpp
----------------------------------------------------------------------
diff --git a/include/mesos/slave/container_logger.hpp b/include/mesos/slave/container_logger.hpp
index e0cf320..f759640 100644
--- a/include/mesos/slave/container_logger.hpp
+++ b/include/mesos/slave/container_logger.hpp
@@ -25,6 +25,7 @@
#include <process/future.hpp>
#include <process/subprocess.hpp>
+#include <process/shared.hpp>
#include <stout/try.hpp>
#include <stout/nothing.hpp>
@@ -80,25 +81,19 @@ public:
static IO PATH(const std::string& path)
{
- return IO(Type::PATH, None(), path);
+ return IO(Type::PATH, path);
}
- static IO FD(int_fd fd)
+ static IO FD(int_fd fd, bool closeOnDestruction = true)
{
- return IO(Type::FD, fd, None());
+ return IO(Type::FD, fd, closeOnDestruction);
}
operator process::Subprocess::IO () const
{
switch (type_) {
case Type::FD:
- // NOTE: The FD is not duplicated and will be closed (as
- // seen by the agent process) when the container is
- // spawned. This shifts the burden of FD-lifecycle
- // management into the Containerizer.
- return process::Subprocess::FD(
- fd_.get(),
- process::Subprocess::IO::OWNED);
+ return process::Subprocess::FD(*fd_->get());
case Type::PATH:
return process::Subprocess::PATH(path_.get());
default:
@@ -106,20 +101,46 @@ public:
}
}
- Type type() const { return type_; }
- Option<int_fd> fd() const { return fd_; }
- Option<std::string> path() const { return path_; }
-
private:
- IO(Type _type,
- const Option<int_fd>& _fd,
- const Option<std::string>& _path)
+ // 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_(_fd),
+ fd_(None()),
path_(_path) {}
Type type_;
- Option<int_fd> fd_;
+ Option<process::Shared<FDWrapper>> fd_;
Option<std::string> path_;
};
@@ -127,18 +148,18 @@ public:
* How to redirect the stdin of the executable.
* See `process::Subprocess::IO`.
*/
- IO in = IO::FD(STDIN_FILENO);
+ 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);
+ IO out = IO::FD(STDOUT_FILENO, false);
/**
* Similar to `out`, except this describes how to redirect stderr.
*/
- IO err = IO::FD(STDERR_FILENO);
+ IO err = IO::FD(STDERR_FILENO, false);
};
/**
http://git-wip-us.apache.org/repos/asf/mesos/blob/48c376a5/include/mesos/slave/containerizer.proto
----------------------------------------------------------------------
diff --git a/include/mesos/slave/containerizer.proto b/include/mesos/slave/containerizer.proto
index 76fde8a..c30b1fc 100644
--- a/include/mesos/slave/containerizer.proto
+++ b/include/mesos/slave/containerizer.proto
@@ -150,21 +150,6 @@ message ContainerConfig {
}
-// This message is used to describe how to handle standard
-// input/output/error for the container.
-message ContainerIO {
- enum Type {
- UNKNOWN = 0;
- FD = 1; // File descriptor. Ownership will be transferred.
- PATH = 2; // File path.
- }
-
- optional Type type = 1;
- optional int32 fd = 2;
- optional string path = 3;
-}
-
-
/**
* Protobuf returned by Isolator::prepare(). The command is executed
* by the Launcher in the containerized context.
@@ -209,12 +194,6 @@ message ContainerLaunchInfo {
// The rlimits are set while launching the container.
optional RLimitInfo rlimits = 8;
- // I/O information about the container to be launched. If not set,
- // the container will inherit the agent standard input/output/error.
- optional ContainerIO in = 11;
- optional ContainerIO out = 12;
- optional ContainerIO err = 13;
-
// (POSIX only) The slave path of the pseudo terminal.
optional string tty_slave_path = 14;
}
http://git-wip-us.apache.org/repos/asf/mesos/blob/48c376a5/src/slave/containerizer/mesos/containerizer.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.cpp b/src/slave/containerizer/mesos/containerizer.cpp
index d2b4f75..e99db1b 100644
--- a/src/slave/containerizer/mesos/containerizer.cpp
+++ b/src/slave/containerizer/mesos/containerizer.cpp
@@ -129,9 +129,9 @@ using mesos::modules::ModuleManager;
using mesos::slave::ContainerClass;
using mesos::slave::ContainerConfig;
-using mesos::slave::ContainerIO;
using mesos::slave::ContainerLaunchInfo;
using mesos::slave::ContainerLimitation;
+using mesos::slave::ContainerLogger;
using mesos::slave::ContainerState;
using mesos::slave::ContainerTermination;
using mesos::slave::Isolator;
@@ -1095,9 +1095,13 @@ Future<bool> MesosContainerizerProcess::launch(
if (!containerConfig.has_container_info() ||
!containerConfig.container_info().mesos().has_image()) {
return prepare(containerId, None())
+ .then(defer(self(), [this, containerId] () {
+ return ioSwitchboard->extractContainerIO(containerId);
+ }))
.then(defer(self(),
&Self::_launch,
containerId,
+ lambda::_1,
environment,
slaveId,
checkpoint));
@@ -1111,9 +1115,13 @@ Future<bool> MesosContainerizerProcess::launch(
.then(defer(self(),
[=](const ProvisionInfo& provisionInfo) -> Future<bool> {
return prepare(containerId, provisionInfo)
+ .then(defer(self(), [this, containerId] () {
+ return ioSwitchboard->extractContainerIO(containerId);
+ }))
.then(defer(self(),
&Self::_launch,
containerId,
+ lambda::_1,
environment,
slaveId,
checkpoint));
@@ -1242,6 +1250,7 @@ Future<Nothing> MesosContainerizerProcess::fetch(
Future<bool> MesosContainerizerProcess::_launch(
const ContainerID& containerId,
+ const Option<ContainerLogger::ContainerIO>& containerIO,
const map<string, string>& environment,
const SlaveID& slaveId,
bool checkpoint)
@@ -1256,6 +1265,7 @@ Future<bool> MesosContainerizerProcess::_launch(
return Failure("Container is being destroyed during preparing");
}
+ CHECK(containerIO.isSome());
CHECK_EQ(container->state, PREPARING);
CHECK_READY(container->launchInfos);
@@ -1302,21 +1312,6 @@ Future<bool> MesosContainerizerProcess::_launch(
return Failure("Multiple isolators specify rlimits");
}
- if (isolatorLaunchInfo->has_in() &&
- launchInfo.has_in()) {
- return Failure("Multiple isolators specify stdin");
- }
-
- if (isolatorLaunchInfo->has_out() &&
- launchInfo.has_out()) {
- return Failure("Multiple isolators specify stdout");
- }
-
- if (isolatorLaunchInfo->has_err() &&
- launchInfo.has_err()) {
- return Failure("Multiple isolators specify stderr");
- }
-
if (isolatorLaunchInfo->has_tty_slave_path() &&
launchInfo.has_tty_slave_path()) {
return Failure("Multiple isolators specify tty");
@@ -1325,51 +1320,6 @@ Future<bool> MesosContainerizerProcess::_launch(
launchInfo.MergeFrom(isolatorLaunchInfo.get());
}
- // Determine the I/O for the container.
- // TODO(jieyu): Close 'fd' on error before 'launcher->fork'.
- Option<Subprocess::IO> in;
- Option<Subprocess::IO> out;
- Option<Subprocess::IO> err;
-
- if (launchInfo.has_in()) {
- switch (launchInfo.in().type()) {
- case ContainerIO::FD:
- in = Subprocess::FD(launchInfo.in().fd(), Subprocess::IO::OWNED);
- break;
- case ContainerIO::PATH:
- in = Subprocess::PATH(launchInfo.in().path());
- break;
- default:
- break;
- }
- }
-
- if (launchInfo.has_out()) {
- switch (launchInfo.out().type()) {
- case ContainerIO::FD:
- out = Subprocess::FD(launchInfo.out().fd(), Subprocess::IO::OWNED);
- break;
- case ContainerIO::PATH:
- out = Subprocess::PATH(launchInfo.out().path());
- break;
- default:
- break;
- }
- }
-
- if (launchInfo.has_err()) {
- switch (launchInfo.err().type()) {
- case ContainerIO::FD:
- err = Subprocess::FD(launchInfo.err().fd(), Subprocess::IO::OWNED);
- break;
- case ContainerIO::PATH:
- err = Subprocess::PATH(launchInfo.err().path());
- break;
- default:
- break;
- }
- }
-
// Remove duplicated entries in enter and clone namespaces.
set<int> enterNamespaces(
launchInfo.enter_namespaces().begin(),
@@ -1608,9 +1558,9 @@ Future<bool> MesosContainerizerProcess::_launch(
containerId,
argv[0],
argv,
- in.isSome() ? in.get() : Subprocess::FD(STDIN_FILENO),
- out.isSome() ? out.get() : Subprocess::FD(STDOUT_FILENO),
- err.isSome() ? err.get() : Subprocess::FD(STDERR_FILENO),
+ containerIO->in,
+ containerIO->out,
+ containerIO->err,
nullptr,
launchEnvironment,
// 'enterNamespaces' will be ignored by PosixLauncher.
http://git-wip-us.apache.org/repos/asf/mesos/blob/48c376a5/src/slave/containerizer/mesos/containerizer.hpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/mesos/containerizer.hpp b/src/slave/containerizer/mesos/containerizer.hpp
index 10a9b57..8010a08 100644
--- a/src/slave/containerizer/mesos/containerizer.hpp
+++ b/src/slave/containerizer/mesos/containerizer.hpp
@@ -235,6 +235,7 @@ private:
process::Future<bool> _launch(
const ContainerID& containerId,
+ const Option<mesos::slave::ContainerLogger::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/48c376a5/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 c9e6f63..895f619 100644
--- a/src/slave/containerizer/mesos/io/switchboard.cpp
+++ b/src/slave/containerizer/mesos/io/switchboard.cpp
@@ -101,7 +101,6 @@ using process::network::internal::SocketImpl;
using mesos::slave::ContainerConfig;
using mesos::slave::ContainerClass;
-using mesos::slave::ContainerIO;
using mesos::slave::ContainerLaunchInfo;
using mesos::slave::ContainerLimitation;
using mesos::slave::ContainerLogger;
@@ -264,6 +263,7 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::prepare(
{
// In local mode, the container will inherit agent's stdio.
if (local) {
+ containerIOs[containerId] = ContainerLogger::ContainerIO();
return None();
}
@@ -305,42 +305,10 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
containerConfig.container_info().has_tty_info();
if (!IOSwitchboard::requiresServer(containerConfig)) {
- ContainerLaunchInfo launchInfo;
+ CHECK(!containerIOs.contains(containerId));
+ containerIOs[containerId] = loggerIO;
- ContainerIO* out = launchInfo.mutable_out();
- ContainerIO* err = launchInfo.mutable_err();
-
- switch (loggerIO.out.type()) {
-#ifndef __WINDOWS__
- case ContainerLogger::ContainerIO::IO::Type::FD:
- out->set_type(ContainerIO::FD);
- out->set_fd(loggerIO.out.fd().get());
- break;
-#endif
- case ContainerLogger::ContainerIO::IO::Type::PATH:
- out->set_type(ContainerIO::PATH);
- out->set_path(loggerIO.out.path().get());
- break;
- default:
- UNREACHABLE();
- }
-
- switch (loggerIO.err.type()) {
-#ifndef __WINDOWS__
- case ContainerLogger::ContainerIO::IO::Type::FD:
- err->set_type(ContainerIO::FD);
- err->set_fd(loggerIO.err.fd().get());
- break;
-#endif
- case ContainerLogger::ContainerIO::IO::Type::PATH:
- err->set_type(ContainerIO::PATH);
- err->set_path(loggerIO.err.path().get());
- break;
- default:
- UNREACHABLE();
- }
-
- return launchInfo;
+ return ContainerLaunchInfo();
}
#ifndef __WINDOWS__
@@ -351,10 +319,16 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
" '" + stringify(containerId) + "'");
}
- // Return the set of fds that should be sent to the
- // container and dup'd onto its stdin/stdout/stderr.
+ // We need this so we can return the
+ // `tty_slave_path` if there is one.
ContainerLaunchInfo launchInfo;
+ // We assign this variable to an entry in the `containerIOs` hashmap
+ // 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;
+
// Manually construct pipes instead of using `Subprocess::PIPE`
// so that the ownership of the FDs is properly represented. The
// `Subprocess` spawned below owns one end of each pipe and will
@@ -443,14 +417,9 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
stdoutFromFd = master;
stderrFromFd = master;
- launchInfo.mutable_in()->set_type(ContainerIO::FD);
- launchInfo.mutable_in()->set_fd(slave.get());
-
- launchInfo.mutable_out()->set_type(ContainerIO::FD);
- launchInfo.mutable_out()->set_fd(slave.get());
-
- launchInfo.mutable_err()->set_type(ContainerIO::FD);
- launchInfo.mutable_err()->set_fd(slave.get());
+ containerIO.in = ContainerLogger::ContainerIO::IO::FD(slave.get());
+ containerIO.out = containerIO.in;
+ containerIO.err = containerIO.in;
launchInfo.set_tty_slave_path(slavePath.get());
} else {
@@ -491,14 +460,9 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
stdoutFromFd = outfds[0];
stderrFromFd = errfds[0];
- launchInfo.mutable_in()->set_type(ContainerIO::FD);
- launchInfo.mutable_in()->set_fd(infds[0]);
-
- launchInfo.mutable_out()->set_type(ContainerIO::FD);
- launchInfo.mutable_out()->set_fd(outfds[1]);
-
- launchInfo.mutable_err()->set_type(ContainerIO::FD);
- launchInfo.mutable_err()->set_fd(errfds[1]);
+ containerIO.in = ContainerLogger::ContainerIO::IO::FD(infds[0]);
+ containerIO.out = ContainerLogger::ContainerIO::IO::FD(outfds[1]);
+ containerIO.err = ContainerLogger::ContainerIO::IO::FD(errfds[1]);
}
// Make sure all file descriptors opened have CLOEXEC set.
@@ -653,6 +617,9 @@ Future<Option<ContainerLaunchInfo>> IOSwitchboard::_prepare(
containerId,
lambda::_1))));
+ // Populate the `containerIOs` hashmap.
+ containerIOs[containerId] = containerIO;
+
return launchInfo;
#endif // __WINDOWS__
}
@@ -722,6 +689,29 @@ Future<http::Connection> IOSwitchboard::_connect(
}
+Future<Option<ContainerLogger::ContainerIO>> IOSwitchboard::extractContainerIO(
+ const ContainerID& containerId)
+{
+ return dispatch(self(), [this, containerId]() {
+ return _extractContainerIO(containerId);
+ });
+}
+
+
+Future<Option<ContainerLogger::ContainerIO>> IOSwitchboard::_extractContainerIO(
+ const ContainerID& containerId)
+{
+ if (!containerIOs.contains(containerId)) {
+ return None();
+ }
+
+ ContainerLogger::ContainerIO containerIO = containerIOs[containerId];
+ containerIOs.erase(containerId);
+
+ return containerIO;
+}
+
+
Future<ContainerLimitation> IOSwitchboard::watch(
const ContainerID& containerId)
{
@@ -806,6 +796,14 @@ Future<Nothing> IOSwitchboard::cleanup(
// DISCARDED cases as well.
return await(list<Future<Option<int>>>{status}).then(
defer(self(), [this, containerId]() -> Future<Nothing> {
+ // We need to call `_extractContainerIO` here in case the
+ // `IOSwitchboard` still holds a reference to the container's
+ // `ContainerIO` struct. We don't care about its value at this
+ // point. We just need to extract it out of the hashmap (if
+ // it's in there) so it can drop out of scope and all open
+ // file descriptors will be closed.
+ _extractContainerIO(containerId);
+
// We only remove the 'containerId from our info struct once
// we are sure that the I/O switchboard has shutdown. If we
// removed it any earlier, attempts to connect to the I/O
http://git-wip-us.apache.org/repos/asf/mesos/blob/48c376a5/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 7189602..83ed029 100644
--- a/src/slave/containerizer/mesos/io/switchboard.hpp
+++ b/src/slave/containerizer/mesos/io/switchboard.hpp
@@ -79,6 +79,11 @@ public:
process::Future<process::http::Connection> connect(
const ContainerID& containerId) const;
+ // 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);
+
// Helper function that returns `true` if `IOSwitchboardServer`
// needs to be enabled for the given `ContainerConfig`. It must
// be enabled for `DEBUG` containers and ones that need `TTYInfo`.
@@ -110,6 +115,9 @@ private:
process::Future<process::http::Connection> _connect(
const ContainerID& containerId) const;
+ process::Future<Option<mesos::slave::ContainerLogger::ContainerIO>>
+ _extractContainerIO(const ContainerID& containerID);
+
#ifndef __WINDOWS__
void reaped(
const ContainerID& containerId,
@@ -120,6 +128,15 @@ private:
bool local;
process::Owned<mesos::slave::ContainerLogger> logger;
hashmap<ContainerID, process::Owned<Info>> infos;
+
+ // We use a separate hashmap to hold the `ContainerIO` for each
+ // container because we need to maintain this information even in
+ // the case were we only instantiate the logger and never spawn an
+ // `IOSwitchbaordProcess`. Also, the lifetime of the `ContainerIO`
+ // 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;
};
[4/4] mesos git commit: Pulled 'ContainerIO' out of the
'ContainerLogger'.
Posted by kl...@apache.org.
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/0d717e08
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/0d717e08
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/0d717e08
Branch: refs/heads/master
Commit: 0d717e08e90a38d9d17864b9d51c45ea0b35b3a5
Parents: 48c376a
Author: Kevin Klues <kl...@gmail.com>
Authored: Wed Mar 1 09:55:25 2017 -0800
Committer: Kevin Klues <kl...@gmail.com>
Committed: Wed Mar 1 09:55:25 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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/src/slave/containerizer/docker.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp
index c4bdbca..dfab262 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>
@@ -72,6 +73,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;
@@ -1324,7 +1326,7 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer(
container->user)
.then(defer(
self(),
- [=](const ContainerLogger::ContainerIO& containerIO)
+ [=](const ContainerIO& containerIO)
-> Future<Docker::Container> {
Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
container->container,
@@ -1463,7 +1465,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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/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/0d717e08/src/tests/container_logger_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/container_logger_tests.cpp b/src/tests/container_logger_tests.cpp
index b0cd83a..54e5b29 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>&));
};