Repository: mesos Updated Branches: refs/heads/master d22a3d701 -> 3f60c9857
Changed default executor tests to not use pipes for synchronization. Some tests of nested container functionality used pipes passed to launched tasks to detect whether a task has actually started executing the workload (`TASK_RUNNING` updates might be sent before the task workload is actually started). Once we avoid leaking unspecified file descriptors into forked processes, this test setup will be broken. In this patch we replace the use of pipes for synchronization with HTTP requests to an actor running in the tests, or wait on other observable side effects. Review: https://reviews.apache.org/r/67398/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/a666047c Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/a666047c Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/a666047c Branch: refs/heads/master Commit: a666047c9324a0b24b26fa8d89b3fdb73537f44f Parents: d22a3d7 Author: Benjamin Bannier <benjamin.bann...@mesosphere.io> Authored: Mon Jun 25 20:08:43 2018 +0200 Committer: Benjamin Bannier <bbann...@apache.org> Committed: Mon Jun 25 20:08:43 2018 +0200 ---------------------------------------------------------------------- src/tests/check_tests.cpp | 41 +---- .../nested_mesos_containerizer_tests.cpp | 156 ++++++------------- 2 files changed, 54 insertions(+), 143 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/a666047c/src/tests/check_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/check_tests.cpp b/src/tests/check_tests.cpp index d48febf..73ea5a9 100644 --- a/src/tests/check_tests.cpp +++ b/src/tests/check_tests.cpp @@ -1971,47 +1971,22 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( .WillOnce(FutureArg<1>(&updateCheckResult)) .WillRepeatedly(Return()); // Ignore subsequent updates. - // Default executor delegates launching both the task and its check to the - // agent. To avoid a race, we explicitly synchronize. - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); - - const std::array<int_fd, 2>& pipes = pipes_.get(); - const string filename = "nested_inherits_work_dir"; - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'echo', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - v1::CommandInfo command; - command.set_shell(false); - command.set_value("/bin/bash"); - command.add_arguments("bash"); - command.add_arguments("-c"); - command.add_arguments( - "touch " + filename + ";echo running >&" + - stringify(pipes[1]) + ";sleep 1000"); - - v1::TaskInfo taskInfo = v1::createTask(agentId, resources, command); + v1::TaskInfo taskInfo = v1::createTask( + agentId, + resources, + v1::createCommandInfo( + strings::format("touch %s; sleep 1000", filename).get())); v1::CheckInfo* checkInfo = taskInfo.mutable_check(); checkInfo->set_type(v1::CheckInfo::COMMAND); checkInfo->set_delay_seconds(0); checkInfo->set_interval_seconds(0); - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'read', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - v1::CommandInfo* checkCommand = - checkInfo->mutable_command()->mutable_command(); - checkCommand->set_shell(false); - checkCommand->set_value("/bin/bash"); - checkCommand->add_arguments("bash"); - checkCommand->add_arguments("-c"); - checkCommand->add_arguments( - "read INPUT <&" + stringify(pipes[0]) + ";ls " + filename); + // Wait in a busy loop until the file has been created. + checkInfo->mutable_command()->mutable_command()->CopyFrom( + v1::createCommandInfo("while [ -f " + filename + "]; do :; done")); v1::TaskGroupInfo taskGroup; taskGroup.add_tasks()->CopyFrom(taskInfo); http://git-wip-us.apache.org/repos/asf/mesos/blob/a666047c/src/tests/containerizer/nested_mesos_containerizer_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/nested_mesos_containerizer_tests.cpp b/src/tests/containerizer/nested_mesos_containerizer_tests.cpp index 6050e6e..b80e40b 100644 --- a/src/tests/containerizer/nested_mesos_containerizer_tests.cpp +++ b/src/tests/containerizer/nested_mesos_containerizer_tests.cpp @@ -14,6 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +#include <sys/stat.h> #include <sys/wait.h> #include <map> @@ -452,23 +453,11 @@ TEST_F(NestedMesosContainerizerTest, ContainerID containerId; containerId.set_value(id::UUID::random().toString()); - // Use a pipe to pass parent's MESOS_SANDBOX value to a child container. - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); + string pipe = path::join(sandbox.get(), "pipe"); + ASSERT_EQ(0, ::mkfifo(pipe.c_str(), 0700)); - const std::array<int_fd, 2>& pipes = pipes_.get(); - - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'echo', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - CommandInfo command; - command.set_shell(false); - command.set_value("/bin/bash"); - command.add_arguments("bash"); - command.add_arguments("-c"); - command.add_arguments( - "echo $MESOS_SANDBOX >&" + stringify(pipes[1]) + ";" + "sleep 1000"); + CommandInfo command = + createCommandInfo("echo ${MESOS_SANDBOX} > " + pipe + " ; sleep 1000"); ExecutorInfo executor = createExecutorInfo("executor", command, "cpus:1"); @@ -486,30 +475,16 @@ TEST_F(NestedMesosContainerizerTest, AWAIT_ASSERT_EQ(Containerizer::LaunchResult::SUCCESS, launch); - // Wait for the parent container to start running its task - // before launching a debug container inside it. - AWAIT_READY(process::io::poll(pipes[0], process::io::READ)); - close(pipes[1]); - - // Launch a nested debug container that compares MESOS_SANDBOX + // Launch a nested debug container that compares `MESOS_SANDBOX` // it sees with the one its parent sees. { ContainerID nestedContainerId; nestedContainerId.mutable_parent()->CopyFrom(containerId); nestedContainerId.set_value(id::UUID::random().toString()); - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'read', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - CommandInfo nestedCommand; - nestedCommand.set_shell(false); - nestedCommand.set_value("/bin/bash"); - nestedCommand.add_arguments("bash"); - nestedCommand.add_arguments("-c"); - nestedCommand.add_arguments( - "read PARENT_SANDBOX <&" + stringify(pipes[0]) + ";" - "[ ${PARENT_SANDBOX} == ${MESOS_SANDBOX} ] && exit 0 || exit 1;"); + CommandInfo nestedCommand = createCommandInfo( + "read PARENT_SANDBOX < " + pipe + ";" + "[ ${PARENT_SANDBOX} = ${MESOS_SANDBOX} ] && exit 0 || exit 1;"); Future<Containerizer::LaunchResult> launchNested = containerizer->launch( nestedContainerId, @@ -529,8 +504,6 @@ TEST_F(NestedMesosContainerizerTest, ASSERT_SOME(waitNested.get()); ASSERT_TRUE(waitNested.get()->has_status()); EXPECT_WEXITSTATUS_EQ(0, waitNested.get()->status()); - - close(pipes[0]); } // Destroy the containerizer with all associated containers. @@ -573,27 +546,15 @@ TEST_F(NestedMesosContainerizerTest, containerId.set_value(id::UUID::random().toString()); // Use a pipe to synchronize with the top-level container. - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); - - const std::array<int_fd, 2>& pipes = pipes_.get(); + string pipe = path::join(sandbox.get(), "pipe"); + ASSERT_EQ(0, ::mkfifo(pipe.c_str(), 0700)); const string filename = "nested_inherits_work_dir"; - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'echo', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - CommandInfo command; - command.set_shell(false); - command.set_value("/bin/bash"); - command.add_arguments("bash"); - command.add_arguments("-c"); - command.add_arguments( - "touch " + filename + ";echo running >&" + - stringify(pipes[1]) + ";sleep 1000"); - - ExecutorInfo executor = createExecutorInfo("executor", command, "cpus:1"); + ExecutorInfo executor = createExecutorInfo( + "executor", + "touch " + filename + "; echo running > " + pipe + "; sleep 1000", + "cpus:1"); Try<string> directory = environment->mkdtemp(); ASSERT_SOME(directory); @@ -616,9 +577,9 @@ TEST_F(NestedMesosContainerizerTest, // Wait for the parent container to start running its task // before launching a debug container inside it. - AWAIT_READY(process::io::poll(pipes[0], process::io::READ)); - close(pipes[1]); - close(pipes[0]); + Result<string> read = os::read(pipe); + ASSERT_SOME(read); + ASSERT_EQ("running\n", read.get()); Future<ContainerStatus> status = containerizer->status(containerId); AWAIT_READY(status); @@ -1015,19 +976,17 @@ TEST_F(NestedMesosContainerizerTest, ASSERT_EQ(1u, offers->size()); // Use a pipe to synchronize with the top-level container. - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); - - const std::array<int_fd, 2>& pipes = pipes_.get(); + string pipe = path::join(sandbox.get(), "pipe"); + ASSERT_EQ(0, ::mkfifo(pipe.c_str(), 0700)); - // Launch a command task within the `alpine` docker image and - // synchronize its launch with the launch of a debug container below. + // Launch a command task within the `alpine` docker image. TaskInfo task = createTask( offers->front().slave_id(), offers->front().resources(), - "echo running >&" + stringify(pipes[1]) + ";" + "sleep 1000"); + "echo running > /tmp/pipe; sleep 1000"); - task.mutable_container()->CopyFrom(createContainerInfo("alpine")); + task.mutable_container()->CopyFrom(createContainerInfo( + "alpine", {createVolumeHostPath("/tmp", sandbox.get(), Volume::RW)})); Future<TaskStatus> statusStarting; Future<TaskStatus> statusRunning; @@ -1044,12 +1003,11 @@ TEST_F(NestedMesosContainerizerTest, AWAIT_READY_FOR(statusRunning, Seconds(120)); ASSERT_EQ(TASK_RUNNING, statusRunning->state()); - close(pipes[1]); - // Wait for the parent container to start running its task // before launching a debug container inside it. - AWAIT_READY(process::io::poll(pipes[0], process::io::READ)); - close(pipes[0]); + Result<string> read = os::read(pipe); + ASSERT_SOME(read); + ASSERT_EQ("running\n", read.get()); ASSERT_TRUE(statusRunning->has_slave_id()); ASSERT_TRUE(statusRunning->has_container_status()); @@ -1383,23 +1341,14 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentExit) ContainerID containerId; containerId.set_value(id::UUID::random().toString()); - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); + string pipe = path::join(sandbox.get(), "pipe"); + ASSERT_EQ(0, ::mkfifo(pipe.c_str(), 0700)); - const std::array<int_fd, 2>& pipes = pipes_.get(); - - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'read', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - CommandInfo command; - command.set_shell(false); - command.set_value("/bin/bash"); - command.add_arguments("bash"); - command.add_arguments("-c"); - command.add_arguments("read key <&" + stringify(pipes[0])); - - ExecutorInfo executor = createExecutorInfo("executor", command, "cpus:1"); + // We launch a blocking `read` after which we return with a non-success code. + ExecutorInfo executor = createExecutorInfo( + "executor", + createCommandInfo("read < " + pipe + " && exit 1"), + "cpus:1"); Try<string> directory = environment->mkdtemp(); ASSERT_SOME(directory); @@ -1410,8 +1359,6 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentExit) map<string, string>(), None()); - close(pipes[0]); // We're never going to read. - AWAIT_ASSERT_EQ(Containerizer::LaunchResult::SUCCESS, launch); // Now launch nested container. @@ -1432,7 +1379,8 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentExit) Future<Option<ContainerTermination>> nestedWait = containerizer->wait( nestedContainerId); - close(pipes[1]); // Force the 'read key' to exit! + // Write to the fifo to unblock the `read` in the parent container. + os::write(pipe, ""); AWAIT_READY(wait); ASSERT_SOME(wait.get()); @@ -1477,25 +1425,14 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentSigterm) ContainerID containerId; containerId.set_value(id::UUID::random().toString()); - // Use a pipe to synchronize with the top-level container. - Try<std::array<int_fd, 2>> pipes_ = os::pipe(); - ASSERT_SOME(pipes_); - - const std::array<int_fd, 2>& pipes = pipes_.get(); - - // NOTE: We use a non-shell command here to use 'bash -c' to execute - // the 'echo', which deals with the file descriptor, because of a bug - // in ubuntu dash. Multi-digit file descriptor is not supported in - // ubuntu dash, which executes the shell command. - CommandInfo command; - command.set_shell(false); - command.set_value("/bin/bash"); - command.add_arguments("bash"); - command.add_arguments("-c"); - command.add_arguments( - "echo running >&" + stringify(pipes[1]) + ";" + "sleep 1000"); + // Use a fifo to synchronize with the top-level container. + string pipe = path::join(sandbox.get(), "pipe"); + ASSERT_EQ(0, ::mkfifo(pipe.c_str(), 0700)); - ExecutorInfo executor = createExecutorInfo("executor", command, "cpus:1"); + ExecutorInfo executor = createExecutorInfo( + "executor", + createCommandInfo("echo running > " + pipe + "; sleep 1000"), + "cpus:1"); Try<string> directory = environment->mkdtemp(); ASSERT_SOME(directory); @@ -1508,8 +1445,6 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentSigterm) AWAIT_ASSERT_EQ(Containerizer::LaunchResult::SUCCESS, launch); - close(pipes[1]); - // Now launch nested container. ContainerID nestedContainerId; nestedContainerId.mutable_parent()->CopyFrom(containerId); @@ -1534,8 +1469,9 @@ TEST_F(NestedMesosContainerizerTest, ROOT_CGROUPS_ParentSigterm) // Wait for the parent container to start running its executor // process before sending it a signal. - AWAIT_READY(process::io::poll(pipes[0], process::io::READ)); - close(pipes[0]); + Result<string> read = os::read(pipe); + ASSERT_SOME(read); + ASSERT_EQ("running\n", read.get()); ASSERT_EQ(0, os::kill(status->executor_pid(), SIGTERM));
