You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by ha...@apache.org on 2017/02/18 10:20:27 UTC
mesos git commit: Refactored `Docker::run` to make it only aware of
docker cli options.
Repository: mesos
Updated Branches:
refs/heads/master 82bc0b9c3 -> 8561b96a7
Refactored `Docker::run` to make it only aware of docker cli options.
This patch creates a wrapper struct for all recognizable docker cli
options, and separate logic of creating these options to a different
common function.
This also enables us to overcome gmock's 10 argument limit.
No logic change happens in this refactoring patch.
Review: https://reviews.apache.org/r/54821
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/8561b96a
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/8561b96a
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/8561b96a
Branch: refs/heads/master
Commit: 8561b96a71d6371a5d16911b23e866bbfadf3ee3
Parents: 82bc0b9
Author: Zhitao Li <zh...@gmail.com>
Authored: Sat Feb 18 17:51:49 2017 +0800
Committer: Haosdent Huang <ha...@apache.org>
Committed: Sat Feb 18 17:58:07 2017 +0800
----------------------------------------------------------------------
src/docker/docker.cpp | 383 +++++++++++--------
src/docker/docker.hpp | 81 +++-
src/docker/executor.cpp | 38 +-
src/slave/containerizer/docker.cpp | 18 +-
.../docker_containerizer_tests.cpp | 69 ++--
src/tests/containerizer/docker_tests.cpp | 80 ++--
src/tests/mock_docker.cpp | 2 +-
src/tests/mock_docker.hpp | 29 +-
8 files changed, 439 insertions(+), 261 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/docker/docker.cpp
----------------------------------------------------------------------
diff --git a/src/docker/docker.cpp b/src/docker/docker.cpp
index 9a454ec..075b86d 100755
--- a/src/docker/docker.cpp
+++ b/src/docker/docker.cpp
@@ -495,7 +495,7 @@ Try<Docker::Image> Docker::Image::create(const JSON::Object& json)
}
-Future<Option<int>> Docker::run(
+Try<Docker::RunOptions> Docker::RunOptions::create(
const ContainerInfo& containerInfo,
const CommandInfo& commandInfo,
const string& name,
@@ -503,49 +503,34 @@ Future<Option<int>> Docker::run(
const string& mappedDirectory,
const Option<Resources>& resources,
const Option<map<string, string>>& env,
- const Option<vector<Device>>& devices,
- const process::Subprocess::IO& _stdout,
- const process::Subprocess::IO& _stderr) const
+ const Option<vector<Device>>& devices)
{
if (!containerInfo.has_docker()) {
- return Failure("No docker info found in container info");
+ return Error("No docker info found in container info");
}
const ContainerInfo::DockerInfo& dockerInfo = containerInfo.docker();
- vector<string> argv;
- argv.push_back(path);
- argv.push_back("-H");
- argv.push_back(socket);
- argv.push_back("run");
-
- if (dockerInfo.privileged()) {
- argv.push_back("--privileged");
- }
+ RunOptions options;
+ options.privileged = dockerInfo.privileged();
if (resources.isSome()) {
// TODO(yifan): Support other resources (e.g. disk).
Option<double> cpus = resources.get().cpus();
if (cpus.isSome()) {
- uint64_t cpuShare =
+ options.cpuShares =
std::max((uint64_t) (CPU_SHARES_PER_CPU * cpus.get()), MIN_CPU_SHARES);
- argv.push_back("--cpu-shares");
- argv.push_back(stringify(cpuShare));
}
Option<Bytes> mem = resources.get().mem();
if (mem.isSome()) {
- Bytes memLimit = std::max(mem.get(), MIN_MEMORY);
- argv.push_back("--memory");
- argv.push_back(stringify(memLimit.bytes()));
+ options.memory = std::max(mem.get(), MIN_MEMORY);
}
}
- string environmentVariables;
-
if (env.isSome()) {
foreachpair (const string& key, const string& value, env.get()) {
- environmentVariables += key + "=" + value + "\n";
+ options.env[key] = value;
}
}
@@ -556,42 +541,11 @@ Future<Option<int>> Docker::run(
// Skip to avoid duplicate environment variables.
continue;
}
- environmentVariables += variable.name() + "=" + variable.value() + "\n";
- }
-
- environmentVariables += "MESOS_SANDBOX=" + mappedDirectory + "\n";
- environmentVariables += "MESOS_CONTAINER_NAME=" + name + "\n";
-
- Try<string> environmentFile_ = os::mktemp();
- if (environmentFile_.isError()) {
- return Failure("Failed to create temporary docker environment "
- "file: " + environmentFile_.error());
- }
-
- const string& environmentFile = environmentFile_.get();
-
- Try<int_fd> fd = os::open(
- environmentFile,
- O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC,
- S_IRUSR | S_IWUSR);
-
- if (fd.isError()) {
- return Failure(
- "Failed to open file '" + environmentFile + "': " + fd.error());
- }
-
- Try<Nothing> write = os::write(fd.get(), environmentVariables);
-
- os::close(fd.get());
-
- if (write.isError()) {
- return Failure(
- "Failed to write docker environment file to '" + environmentFile +
- "': " + write.error());
+ options.env[variable.name()] = variable.value();
}
- argv.push_back("--env-file");
- argv.push_back(environmentFile);
+ options.env["MESOS_SANDBOX"] = mappedDirectory;
+ options.env["MESOS_CONTAINER_NAME"] = name;
Option<string> volumeDriver;
foreach (const Volume& volume, containerInfo.volumes()) {
@@ -609,7 +563,7 @@ Future<Option<int>> Docker::run(
// volume in the sandbox.
if (!path::absolute(volume.host_path()) &&
!path::absolute(volume.container_path())) {
- return Failure(
+ return Error(
"Both host_path '" + volume.host_path() + "' " +
"and container_path '" + volume.container_path() + "' " +
"of a volume are relative");
@@ -628,7 +582,7 @@ Future<Option<int>> Docker::run(
switch (volume.mode()) {
case Volume::RW: volumeConfig += ":rw"; break;
case Volume::RO: volumeConfig += ":ro"; break;
- default: return Failure("Unsupported volume mode");
+ default: return Error("Unsupported volume mode");
}
} else if (volume.has_source()) {
if (volume.source().type() != Volume::Source::DOCKER_VOLUME) {
@@ -646,7 +600,7 @@ Future<Option<int>> Docker::run(
if (volumeDriver.isSome() &&
volumeDriver.get() != currentDriver) {
- return Failure("Only one volume driver is supported");
+ return Error("Only one volume driver is supported");
}
volumeDriver = currentDriver;
@@ -655,101 +609,79 @@ Future<Option<int>> Docker::run(
switch (volume.mode()) {
case Volume::RW: volumeConfig += ":rw"; break;
case Volume::RO: volumeConfig += ":ro"; break;
- default: return Failure("Unsupported volume mode");
+ default: return Error("Unsupported volume mode");
}
} else {
- return Failure("Host path or volume source is required");
+ return Error("Host path or volume source is required");
}
- argv.push_back("-v");
- argv.push_back(volumeConfig);
+ options.volumes.push_back(volumeConfig);
}
// Mapping sandbox directory into the container mapped directory.
- argv.push_back("-v");
- argv.push_back(sandboxDirectory + ":" + mappedDirectory);
+ options.volumes.push_back(sandboxDirectory + ":" + mappedDirectory);
// TODO(gyliu513): Deprecate this after the release cycle of 1.0.
// It will be replaced by Volume.Source.DockerVolume.driver.
if (dockerInfo.has_volume_driver()) {
if (volumeDriver.isSome() &&
volumeDriver.get() != dockerInfo.volume_driver()) {
- return Failure("Only one volume driver per task is supported");
+ return Error("Only one volume driver per task is supported");
}
volumeDriver = dockerInfo.volume_driver();
}
- if (volumeDriver.isSome()) {
- argv.push_back("--volume-driver=" + volumeDriver.get());
- }
-
- const string& image = dockerInfo.image();
+ options.volumeDriver = volumeDriver;
- argv.push_back("--net");
- string network;
switch (dockerInfo.network()) {
- case ContainerInfo::DockerInfo::HOST: network = "host"; break;
- case ContainerInfo::DockerInfo::BRIDGE: network = "bridge"; break;
- case ContainerInfo::DockerInfo::NONE: network = "none"; break;
+ case ContainerInfo::DockerInfo::HOST: options.network = "host"; break;
+ case ContainerInfo::DockerInfo::BRIDGE: options.network = "bridge"; break;
+ case ContainerInfo::DockerInfo::NONE: options.network = "none"; break;
case ContainerInfo::DockerInfo::USER: {
- // User defined networks require docker version >= 1.9.0.
- Try<Nothing> validateVer = validateVersion(Version(1, 9, 0));
-
- if (validateVer.isError()) {
- return Failure("User defined networks require Docker "
- "version 1.9.0 or higher");
- }
-
if (containerInfo.network_infos_size() == 0) {
- return Failure("No network info found in container info");
+ return Error("No network info found in container info");
}
if (containerInfo.network_infos_size() > 1) {
- return Failure("Only a single network can be defined in Docker run");
+ return Error("Only a single network can be defined in Docker run");
}
const NetworkInfo& networkInfo = containerInfo.network_infos(0);
if(!networkInfo.has_name()){
- return Failure("No network name found in network info");
+ return Error("No network name found in network info");
}
- network = networkInfo.name();
+ options.network = networkInfo.name();
break;
}
- default: return Failure("Unsupported Network mode: " +
- stringify(dockerInfo.network()));
+ default: return Error("Unsupported Network mode: " +
+ stringify(dockerInfo.network()));
}
- argv.push_back(network);
-
if (containerInfo.has_hostname()) {
- if (network == "host") {
- return Failure("Unable to set hostname with host network mode");
+ if (options.network.isSome() && options.network.get() == "host") {
+ return Error("Unable to set hostname with host network mode");
}
- argv.push_back("--hostname");
- argv.push_back(containerInfo.hostname());
- }
-
- foreach (const Parameter& parameter, dockerInfo.parameters()) {
- argv.push_back("--" + parameter.key() + "=" + parameter.value());
+ options.hostname = containerInfo.hostname();
}
if (dockerInfo.port_mappings().size() > 0) {
- if (network == "host" || network == "none") {
- return Failure("Port mappings are only supported for bridge and "
- "user-defined networks");
+ if (options.network.isSome() &&
+ (options.network.get() == "host" || options.network.get() == "none")) {
+ return Error("Port mappings are only supported for bridge and "
+ "user-defined networks");
}
if (!resources.isSome()) {
- return Failure("Port mappings require resources");
+ return Error("Port mappings require resources");
}
Option<Value::Ranges> portRanges = resources.get().ports();
if (!portRanges.isSome()) {
- return Failure("Port mappings require port resources");
+ return Error("Port mappings require port resources");
}
foreach (const ContainerInfo::DockerInfo::PortMapping& mapping,
@@ -764,105 +696,235 @@ Future<Option<int>> Docker::run(
}
if (!found) {
- return Failure("Port [" + stringify(mapping.host_port()) + "] not " +
- "included in resources");
+ return Error("Port [" + stringify(mapping.host_port()) + "] not " +
+ "included in resources");
}
- string portMapping = stringify(mapping.host_port()) + ":" +
- stringify(mapping.container_port());
+ Docker::PortMapping portMapping;
+ portMapping.hostPort = mapping.host_port();
+ portMapping.containerPort = mapping.container_port();
if (mapping.has_protocol()) {
- portMapping += "/" + strings::lower(mapping.protocol());
+ portMapping.protocol = mapping.protocol();
}
- argv.push_back("-p");
- argv.push_back(portMapping);
+ options.portMappings.push_back(portMapping);
}
}
if (devices.isSome()) {
- foreach (const Device& device, devices.get()) {
- if (!device.hostPath.absolute()) {
- return Failure("Device path '" + device.hostPath.string() + "'"
- " is not an absolute path");
- }
-
- string permissions;
- permissions += device.access.read ? "r" : "";
- permissions += device.access.write ? "w" : "";
- permissions += device.access.mknod ? "m" : "";
-
- // Docker doesn't handle this case (it fails by saying
- // that an absolute path is not being provided).
- if (permissions.empty()) {
- return Failure("At least one access required for --devices:"
- " none specified for"
- " '" + device.hostPath.string() + "'");
- }
+ options.devices = devices.get();
+ }
- // Note that docker silently does not handle default devices
- // passed in with restricted permissions (e.g. /dev/null), so
- // we don't bother checking this case either.
+ options.name = name;
- argv.push_back("--device=" +
- device.hostPath.string() + ":" +
- device.containerPath.string() + ":" +
- permissions);
- }
+ foreach (const Parameter& parameter, dockerInfo.parameters()) {
+ options.additionalOptions.push_back(
+ "--" + parameter.key() + "=" + parameter.value());
}
+ options.image = dockerInfo.image();
+
if (commandInfo.shell()) {
// We override the entrypoint if shell is enabled because we
// assume the user intends to run the command within a shell
// and not the default entrypoint of the image. View MESOS-1770
// for more details.
- argv.push_back("--entrypoint");
-
#ifdef __WINDOWS__
- argv.push_back("cmd");
+ options.entrypoint = "cmd";
#else
- argv.push_back("/bin/sh");
+ options.entrypoint = "/bin/sh";
#endif // __WINDOWS__
}
- argv.push_back("--name");
- argv.push_back(name);
- argv.push_back(image);
-
if (commandInfo.shell()) {
if (!commandInfo.has_value()) {
- return Failure("Shell specified but no command value provided");
+ return Error("Shell specified but no command value provided");
}
// The Docker CLI only supports a single word for overriding the
// entrypoint, so we must specify `-c` (or `/c` on Windows)
// for the other parts of the command.
#ifdef __WINDOWS__
- argv.push_back("/c");
+ options.arguments.push_back("/c");
#else
- argv.push_back("-c");
+ options.arguments.push_back("-c");
#endif // __WINDOWS__
- argv.push_back(commandInfo.value());
+ options.arguments.push_back(commandInfo.value());
} else {
if (commandInfo.has_value()) {
- argv.push_back(commandInfo.value());
+ options.arguments.push_back(commandInfo.value());
}
foreach (const string& argument, commandInfo.arguments()) {
- argv.push_back(argument);
+ options.arguments.push_back(argument);
}
}
- string cmd = strings::join(" ", argv);
+ return options;
+}
- LOG(INFO) << "Running " << cmd;
- map<string, string> environment = os::environment();
+Future<Option<int>> Docker::run(
+ const Docker::RunOptions& options,
+ const process::Subprocess::IO& _stdout,
+ const process::Subprocess::IO& _stderr) const
+{
+ vector<string> argv;
+ argv.push_back(path);
+ argv.push_back("-H");
+ argv.push_back(socket);
+ argv.push_back("run");
- // NOTE: This is non-relevant to pick up a docker config file,
- // which is necessary for private registry.
- environment["HOME"] = sandboxDirectory;
+ if (options.privileged) {
+ argv.push_back("--privileged");
+ }
+
+ if (options.cpuShares.isSome()) {
+ argv.push_back("--cpu-shares");
+ argv.push_back(stringify(options.cpuShares.get()));
+ }
+
+ if (options.memory.isSome()) {
+ argv.push_back("--memory");
+ argv.push_back(stringify(options.memory->bytes()));
+ }
+
+ string environmentVariables;
+
+ foreachpair(const string& key, const string& value, options.env) {
+ environmentVariables += key + "=" + value + "\n";
+ }
+
+ Try<string> environmentFile_ = os::mktemp();
+ if (environmentFile_.isError()) {
+ return Failure("Failed to create temporary docker environment "
+ "file: " + environmentFile_.error());
+ }
+
+ const string& environmentFile = environmentFile_.get();
+
+ Try<int_fd> fd = os::open(
+ environmentFile,
+ O_WRONLY | O_CREAT | O_TRUNC | O_CLOEXEC,
+ S_IRUSR | S_IWUSR);
+
+ if (fd.isError()) {
+ return Failure(
+ "Failed to open file '" + environmentFile + "': " + fd.error());
+ }
+
+ Try<Nothing> write = os::write(fd.get(), environmentVariables);
+
+ os::close(fd.get());
+
+ if (write.isError()) {
+ return Failure(
+ "Failed to write docker environment file to '" + environmentFile +
+ "': " + write.error());
+ }
+
+ argv.push_back("--env-file");
+ argv.push_back(environmentFile);
+
+ foreach(const string& volume, options.volumes) {
+ argv.push_back("-v");
+ argv.push_back(volume);
+ }
+
+ if (options.volumeDriver.isSome()) {
+ argv.push_back("--volume-driver=" + options.volumeDriver.get());
+ }
+
+ if (options.network.isSome()) {
+ const string& network = options.network.get();
+ argv.push_back("--net");
+ argv.push_back(network);
+
+ if (network != "host" &&
+ network != "bridge" &&
+ network != "none") {
+ // User defined networks require docker version >= 1.9.0.
+ Try<Nothing> validateVer = validateVersion(Version(1, 9, 0));
+
+ if (validateVer.isError()) {
+ return Failure("User defined networks require Docker "
+ "version 1.9.0 or higher");
+ }
+ }
+ }
+
+ if (options.hostname.isSome()) {
+ argv.push_back("--hostname");
+ argv.push_back(options.hostname.get());
+ }
+
+ foreach (const Docker::PortMapping& mapping, options.portMappings) {
+ argv.push_back("-p");
+
+ string portMapping = stringify(mapping.hostPort) + ":" +
+ stringify(mapping.containerPort);
+
+ if (mapping.protocol.isSome()) {
+ portMapping += "/" + strings::lower(mapping.protocol.get());
+ }
+
+ argv.push_back(portMapping);
+ }
+
+ foreach (const Device& device, options.devices) {
+ if (!device.hostPath.absolute()) {
+ return Failure("Device path '" + device.hostPath.string() + "'"
+ " is not an absolute path");
+ }
+
+ string permissions;
+ permissions += device.access.read ? "r" : "";
+ permissions += device.access.write ? "w" : "";
+ permissions += device.access.mknod ? "m" : "";
+
+ // Docker doesn't handle this case (it fails by saying
+ // that an absolute path is not being provided).
+ if (permissions.empty()) {
+ return Failure("At least one access required for --devices:"
+ " none specified for"
+ " '" + device.hostPath.string() + "'");
+ }
+
+ // Note that docker silently does not handle default devices
+ // passed in with restricted permissions (e.g. /dev/null), so
+ // we don't bother checking this case either.
+ argv.push_back(
+ "--device=" +
+ device.hostPath.string() + ":" +
+ device.containerPath.string() + ":" +
+ permissions);
+ }
+
+ if (options.entrypoint.isSome()) {
+ argv.push_back("--entrypoint");
+ argv.push_back(options.entrypoint.get());
+ }
+
+ if (options.name.isSome()) {
+ argv.push_back("--name");
+ argv.push_back(options.name.get());
+ }
+
+ foreach (const string& option, options.additionalOptions) {
+ argv.push_back(option);
+ }
+
+ argv.push_back(options.image);
+
+ foreach(const string& argument, options.arguments) {
+ argv.push_back(argument);
+ }
+
+ string cmd = strings::join(" ", argv);
+
+ LOG(INFO) << "Running " << cmd;
Try<Subprocess> s = subprocess(
path,
@@ -870,8 +932,7 @@ Future<Option<int>> Docker::run(
Subprocess::PATH("/dev/null"),
_stdout,
_stderr,
- nullptr,
- environment);
+ nullptr);
if (s.isError()) {
return Failure("Failed to create subprocess '" + cmd + "': " + s.error());
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/docker/docker.hpp
----------------------------------------------------------------------
diff --git a/src/docker/docker.hpp b/src/docker/docker.hpp
index 314fcb4..1737b78 100644
--- a/src/docker/docker.hpp
+++ b/src/docker/docker.hpp
@@ -79,6 +79,13 @@ public:
} access;
};
+ struct PortMapping
+ {
+ uint32_t hostPort;
+ uint32_t containerPort;
+ Option<std::string> protocol;
+ };
+
class Container
{
public:
@@ -142,6 +149,68 @@ public:
environment(_environment) {}
};
+ // See https://docs.docker.com/engine/reference/run for a complete
+ // explanation of each option.
+ class RunOptions
+ {
+ public:
+ static Try<RunOptions> create(
+ const mesos::ContainerInfo& containerInfo,
+ const mesos::CommandInfo& commandInfo,
+ const std::string& containerName,
+ const std::string& sandboxDirectory,
+ const std::string& mappedDirectory,
+ const Option<mesos::Resources>& resources = None(),
+ const Option<std::map<std::string, std::string>>& env = None(),
+ const Option<std::vector<Device>>& devices = None());
+
+ // "--privileged" option.
+ bool privileged;
+
+ // "--cpu-shares" option.
+ Option<uint64_t> cpuShares;
+
+ // "--memory" option.
+ Option<Bytes> memory;
+
+ // Environment variable overrides. These overrides will be passed
+ // to docker container through "--env-file" option.
+ std::map<std::string, std::string> env;
+
+ // "--volume" option.
+ std::vector<std::string> volumes;
+
+ // "--volume-driver" option.
+ Option<std::string> volumeDriver;
+
+ // "--network" option.
+ Option<std::string> network;
+
+ // "--hostname" option.
+ Option<std::string> hostname;
+
+ // Port mappings for "-p" option.
+ std::vector<PortMapping> portMappings;
+
+ // "--device" option.
+ std::vector<Device> devices;
+
+ // "--entrypoint" option.
+ Option<std::string> entrypoint;
+
+ // "--name" option.
+ Option<std::string> name;
+
+ // Additional docker options passed through containerizer.
+ std::vector<std::string> additionalOptions;
+
+ // "IMAGE[:TAG|@DIGEST]" part of docker run.
+ std::string image;
+
+ // Arguments for docker run.
+ std::vector<std::string> arguments;
+ };
+
// Performs 'docker run IMAGE'. Returns the exit status of the
// container. Note that currently the exit status may correspond
// to the exit code from a failure of the docker client or daemon
@@ -154,19 +223,11 @@ public:
//
// [1]: https://github.com/docker/docker/pull/14012
virtual process::Future<Option<int>> run(
- const mesos::ContainerInfo& containerInfo,
- const mesos::CommandInfo& commandInfo,
- const std::string& containerName,
- const std::string& sandboxDirectory,
- const std::string& mappedDirectory,
- const Option<mesos::Resources>& resources = None(),
- const Option<std::map<std::string, std::string>>& env = None(),
- const Option<std::vector<Device>>& devices = None(),
+ const RunOptions& options,
const process::Subprocess::IO& _stdout =
process::Subprocess::FD(STDOUT_FILENO),
const process::Subprocess::IO& _stderr =
- process::Subprocess::FD(STDERR_FILENO))
- const;
+ process::Subprocess::FD(STDERR_FILENO)) const;
// Returns the current docker version.
virtual process::Future<Version> version() const;
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/docker/executor.cpp
----------------------------------------------------------------------
diff --git a/src/docker/executor.cpp b/src/docker/executor.cpp
index d8c72b0..0db2556 100644
--- a/src/docker/executor.cpp
+++ b/src/docker/executor.cpp
@@ -154,13 +154,7 @@ public:
CHECK(task.container().type() == ContainerInfo::DOCKER);
- // We're adding task and executor resources to launch docker since
- // the DockerContainerizer updates the container cgroup limits
- // directly and it expects it to be the sum of both task and
- // executor resources. This does leave to a bit of unaccounted
- // resources for running this executor, but we are assuming
- // this is just a very small amount of overcommit.
- run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
task.container(),
task.command(),
containerName,
@@ -168,7 +162,30 @@ public:
mappedDirectory,
task.resources() + task.executor().resources(),
taskEnvironment,
- None(), // No extra devices.
+ None() // No extra devices.
+ );
+
+ if (runOptions.isError()) {
+ TaskStatus status;
+ status.mutable_task_id()->CopyFrom(task.task_id());
+ status.set_state(TASK_FAILED);
+ status.set_message(
+ "Failed to create docker run options: " + runOptions.error());
+
+ driver->sendStatusUpdate(status);
+
+ _stop();
+ return;
+ }
+
+ // We're adding task and executor resources to launch docker since
+ // the DockerContainerizer updates the container cgroup limits
+ // directly and it expects it to be the sum of both task and
+ // executor resources. This does leave to a bit of unaccounted
+ // resources for running this executor, but we are assuming
+ // this is just a very small amount of overcommit.
+ run = docker->run(
+ runOptions.get(),
Subprocess::FD(STDOUT_FILENO),
Subprocess::FD(STDERR_FILENO));
@@ -454,6 +471,11 @@ private:
CHECK_SOME(driver);
driver.get()->sendStatusUpdate(taskStatus);
+ _stop();
+ }
+
+ void _stop()
+ {
// A hack for now ... but we need to wait until the status update
// is sent to the slave before we shut ourselves down.
// TODO(tnachen): Remove this hack and also the same hack in the
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/slave/containerizer/docker.cpp
----------------------------------------------------------------------
diff --git a/src/slave/containerizer/docker.cpp b/src/slave/containerizer/docker.cpp
index 406e560..ed1cbf1 100644
--- a/src/slave/containerizer/docker.cpp
+++ b/src/slave/containerizer/docker.cpp
@@ -1322,10 +1322,7 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer(
self(),
[=](const ContainerLogger::SubprocessInfo& subprocessInfo)
-> Future<Docker::Container> {
- // Start the executor in a Docker container.
- // This executor could either be a custom executor specified by an
- // ExecutorInfo, or the docker executor.
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
container->container,
container->command,
containerName,
@@ -1333,7 +1330,18 @@ Future<Docker::Container> DockerContainerizerProcess::launchExecutorContainer(
flags.sandbox_directory,
container->resources,
container->environment,
- None(), // No extra devices.
+ None() // No extra devices.
+ );
+
+ if (runOptions.isError()) {
+ return Failure(runOptions.error());
+ }
+
+ // Start the executor in a Docker container.
+ // This executor could either be a custom executor specified by an
+ // ExecutorInfo, or the docker executor.
+ Future<Option<int>> run = docker->run(
+ runOptions.get(),
subprocessInfo.out,
subprocessInfo.err);
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/tests/containerizer/docker_containerizer_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/docker_containerizer_tests.cpp b/src/tests/containerizer/docker_containerizer_tests.cpp
index 31d63b1..921ca32 100644
--- a/src/tests/containerizer/docker_containerizer_tests.cpp
+++ b/src/tests/containerizer/docker_containerizer_tests.cpp
@@ -1186,7 +1186,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover)
CommandInfo commandInfo;
commandInfo.set_value("sleep 1000");
- docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
container1,
@@ -1194,14 +1194,21 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_Recover)
flags.sandbox_directory,
resources);
- Future<Option<int>> orphanRun =
- docker->run(
- containerInfo,
- commandInfo,
- container2,
- flags.work_dir,
- flags.sandbox_directory,
- resources);
+ ASSERT_SOME(runOptions);
+
+ docker->run(runOptions.get());
+
+ Try<Docker::RunOptions> orphanOptions = Docker::RunOptions::create(
+ containerInfo,
+ commandInfo,
+ container2,
+ flags.work_dir,
+ flags.sandbox_directory,
+ resources);
+
+ ASSERT_SOME(orphanOptions);
+
+ Future<Option<int>> orphanRun = docker->run(orphanOptions.get());
ASSERT_TRUE(
exists(docker, slaveId, containerId, ContainerState::RUNNING));
@@ -1319,7 +1326,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_KillOrphanContainers)
CommandInfo commandInfo;
commandInfo.set_value("sleep 1000");
- docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
container1,
@@ -1327,14 +1334,21 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_KillOrphanContainers)
flags.sandbox_directory,
resources);
- Future<Option<int>> orphanRun =
- docker->run(
- containerInfo,
- commandInfo,
- container2,
- flags.work_dir,
- flags.sandbox_directory,
- resources);
+ ASSERT_SOME(runOptions);
+
+ docker->run(runOptions.get());
+
+ Try<Docker::RunOptions> orphanOptions = Docker::RunOptions::create(
+ containerInfo,
+ commandInfo,
+ container2,
+ flags.work_dir,
+ flags.sandbox_directory,
+ resources);
+
+ ASSERT_SOME(orphanOptions);
+
+ Future<Option<int>> orphanRun = docker->run(orphanOptions.get());
ASSERT_TRUE(
exists(docker, slaveId, containerId, ContainerState::RUNNING));
@@ -1501,14 +1515,15 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_SkipRecoverMalformedUUID)
CommandInfo commandInfo;
commandInfo.set_value("sleep 1000");
- Future<Option<int>> run =
- docker->run(
- containerInfo,
- commandInfo,
- container,
- flags.work_dir,
- flags.sandbox_directory,
- resources);
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
+ containerInfo,
+ commandInfo,
+ container,
+ flags.work_dir,
+ flags.sandbox_directory,
+ resources);
+
+ Future<Option<int>> run = docker->run(runOptions.get());
ASSERT_TRUE(
exists(docker, slaveId, containerId, ContainerState::RUNNING));
@@ -3802,7 +3817,7 @@ TEST_F(DockerContainerizerTest, ROOT_DOCKER_DockerInspectDiscard)
Invoke((MockDocker*) docker.get(),
&MockDocker::_inspect)));
- EXPECT_CALL(*mockDocker, run(_, _, _, _, _, _, _, _, _, _))
+ EXPECT_CALL(*mockDocker, run(_, _, _))
.WillOnce(Return(Failure("Run failed")));
Owned<MasterDetector> detector = master.get()->createDetector();
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/tests/containerizer/docker_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/containerizer/docker_tests.cpp b/src/tests/containerizer/docker_tests.cpp
index 9667d43..db9355f 100644
--- a/src/tests/containerizer/docker_tests.cpp
+++ b/src/tests/containerizer/docker_tests.cpp
@@ -126,8 +126,7 @@ TEST_F(DockerTest, ROOT_DOCKER_interface)
CommandInfo commandInfo;
commandInfo.set_value("sleep 120");
- // Start the container.
- Future<Option<int>> status = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -135,6 +134,11 @@ TEST_F(DockerTest, ROOT_DOCKER_interface)
"/mnt/mesos/sandbox",
resources);
+ ASSERT_SOME(runOptions);
+
+ // Start the container.
+ Future<Option<int>> status = docker->run(runOptions.get());
+
Future<Docker::Container> inspect =
docker->inspect(containerName, Seconds(1));
@@ -208,9 +212,7 @@ TEST_F(DockerTest, ROOT_DOCKER_interface)
EXPECT_NE("/" + containerName, container.name);
}
- // Start the container again, this time we will do a "rm -f"
- // directly, instead of stopping and rm.
- status = docker->run(
+ runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -218,6 +220,12 @@ TEST_F(DockerTest, ROOT_DOCKER_interface)
"/mnt/mesos/sandbox",
resources);
+ ASSERT_SOME(runOptions);
+
+ // Start the container again, this time we will do a "rm -f"
+ // directly, instead of stopping and rm.
+ status = docker->run(runOptions.get());
+
inspect = docker->inspect(containerName, Seconds(1));
AWAIT_READY(inspect);
@@ -278,8 +286,7 @@ TEST_F(DockerTest, ROOT_DOCKER_kill)
CommandInfo commandInfo;
commandInfo.set_value("sleep 120");
- // Start the container, kill it, and expect it to terminate.
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -287,6 +294,11 @@ TEST_F(DockerTest, ROOT_DOCKER_kill)
"/mnt/mesos/sandbox",
resources);
+ ASSERT_SOME(runOptions);
+
+ // Start the container, kill it, and expect it to terminate.
+ Future<Option<int>> run = docker->run(runOptions.get());
+
// Note that we cannot issue the kill until we know that the
// run has been processed. We check for this by waiting for
// a successful 'inspect' result.
@@ -351,14 +363,14 @@ TEST_F(DockerTest, ROOT_DOCKER_CheckCommandWithShell)
CommandInfo commandInfo;
commandInfo.set_shell(true);
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
"testContainer",
"dir",
"/mnt/mesos/sandbox");
- ASSERT_TRUE(run.isFailed());
+ ASSERT_ERROR(runOptions);
}
@@ -397,7 +409,7 @@ TEST_F(DockerTest, ROOT_DOCKER_CheckPortResource)
Resources resources =
Resources::parse("ports:[9998-9999];ports:[10001-11000]").get();
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -405,15 +417,14 @@ TEST_F(DockerTest, ROOT_DOCKER_CheckPortResource)
"/mnt/mesos/sandbox",
resources);
- // Port should be out side of the provided ranges.
- AWAIT_EXPECT_FAILED(run);
+ ASSERT_ERROR(runOptions);
resources = Resources::parse("ports:[9998-9999];ports:[10000-11000]").get();
Try<string> directory = environment->mkdtemp();
ASSERT_SOME(directory);
- run = docker->run(
+ runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -421,6 +432,10 @@ TEST_F(DockerTest, ROOT_DOCKER_CheckPortResource)
"/mnt/mesos/sandbox",
resources);
+ ASSERT_SOME(runOptions);
+
+ Future<Option<int>> run = docker->run(runOptions.get());
+
AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
}
@@ -491,13 +506,15 @@ TEST_F(DockerTest, ROOT_DOCKER_MountRelativeHostPath)
const string testFile = path::join(directory.get(), "test_file");
EXPECT_SOME(os::write(testFile, "data"));
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
NAME_PREFIX + "-mount-relative-host-path-test",
directory.get(),
"/mnt/mesos/sandbox");
+ Future<Option<int>> run = docker->run(runOptions.get());
+
AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
}
@@ -534,13 +551,16 @@ TEST_F(DockerTest, ROOT_DOCKER_MountAbsoluteHostPath)
commandInfo.set_shell(true);
commandInfo.set_value("ls /tmp/test_file");
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
NAME_PREFIX + "-mount-absolute-host-path-test",
directory.get(),
"/mnt/mesos/sandbox");
+ ASSERT_SOME(runOptions);
+
+ Future<Option<int>> run = docker->run(runOptions.get());
AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
}
@@ -578,13 +598,17 @@ TEST_F(DockerTest, ROOT_DOCKER_MountRelativeContainerPath)
commandInfo.set_shell(true);
commandInfo.set_value("ls /mnt/mesos/sandbox/tmp/test_file");
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
NAME_PREFIX + "-mount-relative-container-path-test",
directory.get(),
"/mnt/mesos/sandbox");
+ ASSERT_SOME(runOptions);
+
+ Future<Option<int>> run = docker->run(runOptions.get());
+
AWAIT_EXPECT_WEXITSTATUS_EQ(0, run);
}
@@ -621,14 +645,14 @@ TEST_F(DockerTest, ROOT_DOCKER_MountRelativeHostPathRelativeContainerPath)
const string testFile = path::join(directory.get(), "test_file");
EXPECT_SOME(os::write(testFile, "data"));
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
NAME_PREFIX + "-mount-relative-host-path/container-path-test",
directory.get(),
"/mnt/mesos/sandbox");
- ASSERT_TRUE(run.isFailed());
+ ASSERT_ERROR(runOptions);
}
@@ -790,7 +814,7 @@ TEST_F(DockerTest, ROOT_DOCKER_NVIDIA_GPU_DeviceAllow)
vector<Docker::Device> devices = { nvidiaCtl };
- Future<Option<int>> status = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -800,6 +824,10 @@ TEST_F(DockerTest, ROOT_DOCKER_NVIDIA_GPU_DeviceAllow)
None(),
devices);
+ ASSERT_SOME(runOptions);
+
+ Future<Option<int>> status = docker->run(runOptions.get());
+
AWAIT_EXPECT_WEXITSTATUS_EQ(0, status);
}
@@ -844,7 +872,7 @@ TEST_F(DockerTest, ROOT_DOCKER_NVIDIA_GPU_InspectDevices)
vector<Docker::Device> devices = { nvidiaCtl };
- Future<Option<int>> status = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
containerName,
@@ -854,6 +882,10 @@ TEST_F(DockerTest, ROOT_DOCKER_NVIDIA_GPU_InspectDevices)
None(),
devices);
+ ASSERT_SOME(runOptions);
+
+ Future<Option<int>> status = docker->run(runOptions.get());
+
Future<Docker::Container> container =
docker->inspect(containerName, Milliseconds(1));
@@ -897,14 +929,14 @@ TEST_F(DockerTest, ROOT_DOCKER_ConflictingVolumeDriversInMultipleVolumes)
CommandInfo commandInfo;
commandInfo.set_shell(false);
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
"testContainer",
"dir",
"/mnt/mesos/sandbox");
- ASSERT_TRUE(run.isFailed());
+ ASSERT_ERROR(runOptions);
}
@@ -932,14 +964,14 @@ TEST_F(DockerTest, ROOT_DOCKER_ConflictingVolumeDrivers)
CommandInfo commandInfo;
commandInfo.set_shell(false);
- Future<Option<int>> run = docker->run(
+ Try<Docker::RunOptions> runOptions = Docker::RunOptions::create(
containerInfo,
commandInfo,
"testContainer",
"dir",
"/mnt/mesos/sandbox");
- ASSERT_TRUE(run.isFailed());
+ ASSERT_ERROR(runOptions);
}
} // namespace tests {
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/tests/mock_docker.cpp
----------------------------------------------------------------------
diff --git a/src/tests/mock_docker.cpp b/src/tests/mock_docker.cpp
index 02b6065..81a14ca 100644
--- a/src/tests/mock_docker.cpp
+++ b/src/tests/mock_docker.cpp
@@ -54,7 +54,7 @@ MockDocker::MockDocker(
EXPECT_CALL(*this, stop(_, _, _))
.WillRepeatedly(Invoke(this, &MockDocker::_stop));
- EXPECT_CALL(*this, run(_, _, _, _, _, _, _, _, _, _))
+ EXPECT_CALL(*this, run(_, _, _))
.WillRepeatedly(Invoke(this, &MockDocker::_run));
EXPECT_CALL(*this, inspect(_, _))
http://git-wip-us.apache.org/repos/asf/mesos/blob/8561b96a/src/tests/mock_docker.hpp
----------------------------------------------------------------------
diff --git a/src/tests/mock_docker.hpp b/src/tests/mock_docker.hpp
index 829a760..f58211d 100644
--- a/src/tests/mock_docker.hpp
+++ b/src/tests/mock_docker.hpp
@@ -58,17 +58,10 @@ public:
const Option<JSON::Object>& config = None());
virtual ~MockDocker();
- MOCK_CONST_METHOD10(
+ MOCK_CONST_METHOD3(
run,
process::Future<Option<int>>(
- const mesos::ContainerInfo&,
- const mesos::CommandInfo&,
- const std::string&,
- const std::string&,
- const std::string&,
- const Option<mesos::Resources>&,
- const Option<std::map<std::string, std::string>>&,
- const Option<std::vector<Device>>&,
+ const Docker::RunOptions& options,
const process::Subprocess::IO&,
const process::Subprocess::IO&));
@@ -98,26 +91,12 @@ public:
const Option<Duration>&));
process::Future<Option<int>> _run(
- const mesos::ContainerInfo& containerInfo,
- const mesos::CommandInfo& commandInfo,
- const std::string& name,
- const std::string& sandboxDirectory,
- const std::string& mappedDirectory,
- const Option<mesos::Resources>& resources,
- const Option<std::map<std::string, std::string>>& env,
- const Option<std::vector<Device>>& devices,
+ const Docker::RunOptions& runOptions,
const process::Subprocess::IO& _stdout,
const process::Subprocess::IO& _stderr) const
{
return Docker::run(
- containerInfo,
- commandInfo,
- name,
- sandboxDirectory,
- mappedDirectory,
- resources,
- env,
- devices,
+ runOptions,
_stdout,
_stderr);
}