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 <[email protected]> Authored: Sat Feb 18 17:51:49 2017 +0800 Committer: Haosdent Huang <[email protected]> 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); }
