Updating all references to os::shell. For more details see MESOS-3142.
Review: https://reviews.apache.org/r/36979 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/6affd37e Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/6affd37e Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/6affd37e Branch: refs/heads/master Commit: 6affd37e51e22e2339f8bfcccb010e1f61c1cb90 Parents: 8c8c239 Author: Marco Massenzio <[email protected]> Authored: Wed Aug 12 11:55:52 2015 -0700 Committer: Benjamin Hindman <[email protected]> Committed: Fri Aug 14 10:12:06 2015 -0700 ---------------------------------------------------------------------- src/hdfs/hdfs.hpp | 68 +++++++++----------- src/master/main.cpp | 31 ++------- .../isolators/network/port_mapping.cpp | 22 ++----- src/tests/containerizer/isolator_tests.cpp | 10 ++- src/tests/containerizer/port_mapping_tests.cpp | 30 ++++++--- 5 files changed, 64 insertions(+), 97 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/6affd37e/src/hdfs/hdfs.hpp ---------------------------------------------------------------------- diff --git a/src/hdfs/hdfs.hpp b/src/hdfs/hdfs.hpp index a070c32..18f1723 100644 --- a/src/hdfs/hdfs.hpp +++ b/src/hdfs/hdfs.hpp @@ -70,12 +70,15 @@ struct HDFS CHECK_SOME(command); - Try<int> status = os::shell(NULL, command.get() + " 2>&1"); + // We are piping stderr to stdout so that we can see the error (if + // any) in the logs emitted by `os::shell()` in case of failure. + Try<std::string> out = os::shell(command.get() + " 2>&1"); - if(status.isError()) { - return Error(status.error()); + if (out.isError()) { + return Error(out.error()); } - return status.get() == 0; + + return true; } Try<bool> exists(std::string path) @@ -88,13 +91,15 @@ struct HDFS CHECK_SOME(command); - Try<int> status = os::shell(NULL, command.get() + " 2>&1"); + // We are piping stderr to stdout so that we can see the error (if + // any) in the logs emitted by `os::shell()` in case of failure. + Try<std::string> out = os::shell(command.get() + " 2>&1"); - if (status.isError()) { - return Error(status.error()); + if (out.isError()) { + return Error(out.error()); } - return status.get() == 0; + return true; } Try<Bytes> du(std::string path) @@ -107,18 +112,21 @@ struct HDFS CHECK_SOME(command); - std::ostringstream output; - - Try<int> status = os::shell(&output, command.get() + " 2>&1"); + // We are piping stderr to stdout so that we can see the error (if + // any) in the logs emitted by `os::shell()` in case of failure. + // + // TODO(marco): this was the existing logic, but not sure it is + // actually needed. + Try<std::string> out = os::shell(command.get() + " 2>&1"); - if (status.isError()) { - return Error("HDFS du failed: " + status.error()); + if (out.isError()) { + return Error("HDFS du failed: " + out.error()); } - const std::vector<std::string>& s = strings::split(output.str(), " "); + const std::vector<std::string>& s = strings::split(out.get(), " "); if (s.size() != 2) { return Error("HDFS du returned an unexpected number of results: '" + - output.str() + "'"); + out.get() + "'"); } Result<size_t> size = numify<size_t>(s[0]); @@ -141,14 +149,10 @@ struct HDFS CHECK_SOME(command); - std::ostringstream output; - - Try<int> status = os::shell(&output, command.get() + " 2>&1"); + Try<std::string> out = os::shell(command.get()); - if (status.isError()) { - return Error(status.error()); - } else if (status.get() != 0) { - return Error(command.get() + "\n" + output.str()); + if (out.isError()) { + return Error(out.error()); } return Nothing(); @@ -171,14 +175,10 @@ struct HDFS CHECK_SOME(command); - std::ostringstream output; + Try<std::string> out = os::shell(command.get()); - Try<int> status = os::shell(&output, command.get() + " 2>&1"); - - if (status.isError()) { - return Error(status.error()); - } else if (status.get() != 0) { - return Error(command.get() + "\n" + output.str()); + if (out.isError()) { + return Error(out.error()); } return Nothing(); @@ -194,14 +194,10 @@ struct HDFS CHECK_SOME(command); - std::ostringstream output; - - Try<int> status = os::shell(&output, command.get() + " 2>&1"); + Try<std::string> out = os::shell(command.get()); - if (status.isError()) { - return Error(status.error()); - } else if (status.get() != 0) { - return Error(command.get() + "\n" + output.str()); + if (out.isError()) { + return Error(out.error()); } return Nothing(); http://git-wip-us.apache.org/repos/asf/mesos/blob/6affd37e/src/master/main.cpp ---------------------------------------------------------------------- diff --git a/src/master/main.cpp b/src/master/main.cpp index e024a2e..bafc605 100644 --- a/src/master/main.cpp +++ b/src/master/main.cpp @@ -200,36 +200,13 @@ int main(int argc, char** argv) } if (ip_discovery_command.isSome()) { - ostringstream out; + Try<string> ipAddress = os::shell(ip_discovery_command.get()); - // TODO(marco): Modify os::shell to be used in a less verbose - // fashion, see MESOS-3142. - Try<int> status = os::shell(&out, ip_discovery_command.get()); - - if (status.isError()) { - EXIT(EXIT_FAILURE) - << "Could not execute '" << ip_discovery_command.get() - << "': " << status.error(); - } else if (WIFSIGNALED(status.get())) { - EXIT(EXIT_FAILURE) - << "Running '" << ip_discovery_command.get() - << "' was interruped by signal '" - << strsignal(WTERMSIG(status.get())) << "'"; - } else if (WEXITSTATUS(status.get()) != EXIT_SUCCESS) { - EXIT(EXIT_FAILURE) - << "Failed to discover Master IP using '" - << ip_discovery_command.get() << "'; the script was either not found " - << "or exited with an error code.\n" - << "Error code (127 typically indicates command not found): " - << stringify(WEXITSTATUS(status.get())); + if (ipAddress.isError()) { + EXIT(EXIT_FAILURE) << ipAddress.error(); } - const string ipAddress = strings::trim(out.str()); - - LOG(INFO) << "Configuring Mesos master to listen on IP '" - << ipAddress << "'"; - - os::setenv("LIBPROCESS_IP", ipAddress); + os::setenv("LIBPROCESS_IP", strings::trim(ipAddress.get())); } else if (ip.isSome()) { os::setenv("LIBPROCESS_IP", ip.get()); } http://git-wip-us.apache.org/repos/asf/mesos/blob/6affd37e/src/slave/containerizer/isolators/network/port_mapping.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/isolators/network/port_mapping.cpp b/src/slave/containerizer/isolators/network/port_mapping.cpp index 12b3ea7..b7b69c2 100644 --- a/src/slave/containerizer/isolators/network/port_mapping.cpp +++ b/src/slave/containerizer/isolators/network/port_mapping.cpp @@ -1091,22 +1091,14 @@ Try<Isolator*> PortMappingIsolatorProcess::create(const Flags& flags) // Check the availability of a few Linux commands that we will use. // We use the blocking os::shell here because 'create' will only be // invoked during initialization. - Try<int> checkCommandTc = os::shell(NULL, "tc filter show"); + Try<string> checkCommandTc = os::shell("tc filter show"); if (checkCommandTc.isError()) { return Error("Check command 'tc' failed: " + checkCommandTc.error()); - } else if (checkCommandTc.get() != 0) { - return Error( - "Check command 'tc' failed: non-zero exit code: " + - stringify(checkCommandTc.get())); } - Try<int> checkCommandIp = os::shell(NULL, "ip link show"); + Try<string> checkCommandIp = os::shell("ip link show"); if (checkCommandIp.isError()) { return Error("Check command 'ip' failed: " + checkCommandIp.error()); - } else if (checkCommandIp.get() != 0) { - return Error( - "Check command 'ip' failed: non-zero exit code: " + - stringify(checkCommandIp.get())); } Try<Resources> resources = Resources::parse( @@ -1574,8 +1566,7 @@ Try<Isolator*> PortMappingIsolatorProcess::create(const Flags& flags) // issues for the shell command 'mount --make-rslave' inside the // container. It's OK to use the blocking os::shell here because // 'create' will only be invoked during initialization. - Try<int> mount = os::shell( - NULL, + Try<string> mount = os::shell( "mount --bind %s %s", PORT_MAPPING_BIND_MOUNT_ROOT().c_str(), PORT_MAPPING_BIND_MOUNT_ROOT().c_str()); @@ -1584,17 +1575,12 @@ Try<Isolator*> PortMappingIsolatorProcess::create(const Flags& flags) return Error( "Failed to self bind mount '" + PORT_MAPPING_BIND_MOUNT_ROOT() + "': " + mount.error()); - } else if (mount.get() != 0) { - return Error( - "Failed to self bind mount '" + PORT_MAPPING_BIND_MOUNT_ROOT() + - "': non-zero exit code: " + stringify(mount.get())); } } // Mark the mount point PORT_MAPPING_BIND_MOUNT_ROOT() as // recursively shared. - Try<int> mountShared = os::shell( - NULL, + Try<string> mountShared = os::shell( "mount --make-rshared %s", PORT_MAPPING_BIND_MOUNT_ROOT().c_str()); http://git-wip-us.apache.org/repos/asf/mesos/blob/6affd37e/src/tests/containerizer/isolator_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/isolator_tests.cpp b/src/tests/containerizer/isolator_tests.cpp index dd1ae22..fdb7064 100644 --- a/src/tests/containerizer/isolator_tests.cpp +++ b/src/tests/containerizer/isolator_tests.cpp @@ -1250,20 +1250,18 @@ TYPED_TEST(UserCgroupIsolatorTest, ROOT_CGROUPS_UserCgroup) // // Our 'grep' will only select the 'memory' line and then 'awk' will // output 'memory/mesos/b7410ed8-c85b-445e-b50e-3a1698d0e18c'. - ostringstream output; - Try<int> status = os::shell( - &output, + Try<string> grepOut = os::shell( "grep '" + path::join("/", flags.cgroups_root) + "' /proc/" + - stringify(pid) + "/cgroup | awk -F ':' '{print $2$3}'"); + stringify(pid) + "/cgroup | awk -F ':' '{print $2$3}'"); - ASSERT_SOME(status); + ASSERT_SOME(grepOut); // Kill the dummy child process. ::kill(pid, SIGKILL); int exitStatus; EXPECT_NE(-1, ::waitpid(pid, &exitStatus, 0)); - vector<string> cgroups = strings::tokenize(output.str(), "\n"); + vector<string> cgroups = strings::tokenize(grepOut.get(), "\n"); ASSERT_FALSE(cgroups.empty()); foreach (const string& cgroup, cgroups) { http://git-wip-us.apache.org/repos/asf/mesos/blob/6affd37e/src/tests/containerizer/port_mapping_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/port_mapping_tests.cpp b/src/tests/containerizer/port_mapping_tests.cpp index 3c9b7c8..d3526b8 100644 --- a/src/tests/containerizer/port_mapping_tests.cpp +++ b/src/tests/containerizer/port_mapping_tests.cpp @@ -191,14 +191,14 @@ public: << "new libnl library, or disable this test case\n" << "-------------------------------------------------------------"; - ASSERT_SOME_EQ(0, os::shell(NULL, "which nc")) + ASSERT_SOME(os::shell("which nc")) << "-------------------------------------------------------------\n" << "We cannot run any PortMappingIsolatorTests because 'nc'\n" << "could not be found. You can either install 'nc', or disable\n" << "this test case\n" << "-------------------------------------------------------------"; - ASSERT_SOME_EQ(0, os::shell(NULL, "which arping")) + ASSERT_SOME(os::shell("which arping")) << "-------------------------------------------------------------\n" << "We cannot run some PortMappingIsolatorTests because 'arping'\n" << "could not be found. You can either isntall 'arping', or\n" @@ -838,24 +838,24 @@ TEST_F(PortMappingIsolatorTest, ROOT_NC_HostToContainerUDP) // Send to 'localhost' and 'port'. ostringstream command2; command2 << "printf hello1 | nc -w1 -u localhost " << validPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command2.str().c_str())); + ASSERT_SOME(os::shell(command2.str())); // Send to 'localhost' and 'invalidPort'. The command should return // successfully because UDP is stateless but no data could be sent. ostringstream command3; command3 << "printf hello2 | nc -w1 -u localhost " << invalidPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command3.str().c_str())); + ASSERT_SOME(os::shell(command3.str())); // Send to 'public IP' and 'port'. ostringstream command4; command4 << "printf hello3 | nc -w1 -u " << hostIP << " " << validPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command4.str().c_str())); + ASSERT_SOME(os::shell(command4.str())); // Send to 'public IP' and 'invalidPort'. The command should return // successfully because UDP is stateless but no data could be sent. ostringstream command5; command5 << "printf hello4 | nc -w1 -u " << hostIP << " " << invalidPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command5.str().c_str())); + ASSERT_SOME(os::shell(command5.str())); EXPECT_SOME_EQ("hello1", os::read(trafficViaLoopback)); EXPECT_SOME_EQ("hello3", os::read(trafficViaPublic)); @@ -955,24 +955,34 @@ TEST_F(PortMappingIsolatorTest, ROOT_NC_HostToContainerTCP) // Send to 'localhost' and 'port'. ostringstream command2; command2 << "printf hello1 | nc localhost " << validPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command2.str().c_str())); + ASSERT_SOME(os::shell(command2.str())); // Send to 'localhost' and 'invalidPort'. This should fail because TCP // connection couldn't be established.. ostringstream command3; command3 << "printf hello2 | nc localhost " << invalidPort; - ASSERT_SOME_EQ(256, os::shell(NULL, command3.str().c_str())); + Try<string> invalid = os::shell(command3.str()); + ASSERT_ERROR(invalid); + + // When the above command fails, it prints an error message that + // ends with the error exit code, which happens to be 256. + EXPECT_TRUE(strings::contains(invalid.error(), "256")); // Send to 'public IP' and 'port'. ostringstream command4; command4 << "printf hello3 | nc " << hostIP << " " << validPort; - ASSERT_SOME_EQ(0, os::shell(NULL, command4.str().c_str())); + ASSERT_SOME(os::shell(command4.str())); // Send to 'public IP' and 'invalidPort'. This should fail because TCP // connection couldn't be established. ostringstream command5; command5 << "printf hello4 | nc " << hostIP << " " << invalidPort; - ASSERT_SOME_EQ(256, os::shell(NULL, command5.str().c_str())); + Try<string> connect = os::shell(command5.str()); + ASSERT_ERROR(connect); + + // As above, we check that the error message contains the + // expected error code. + EXPECT_TRUE(strings::contains(connect.error(), "256")); EXPECT_SOME_EQ("hello1", os::read(trafficViaLoopback)); EXPECT_SOME_EQ("hello3", os::read(trafficViaPublic));
