crush: CrushTester::test_with_crushtool: use SubProcess to spawn crus…

…htool

Fixes and improvements this brings:

* Use _exit(2) instead of exit(2) if exec(3) failed (does not call the atexit
  functions, removing asock and pid files in the child process).
* Close all parent descriptors before exec(3).
* Log crushtool stderr.
* SubProcess is covered by unit tests.

Signed-off-by: Mykola Golub <mgolub@mirantis.com>

src/common/config_opts.h

@@ -25,6 +25,7 @@ OPTION(mon_host, OPT_STR, "")
 OPTION(lockdep, OPT_BOOL, false)
 OPTION(run_dir, OPT_STR, "/var/run/ceph")       // the "/var/run/ceph" dir, created on daemon startup
 OPTION(admin_socket, OPT_STR, "$run_dir/$cluster-$name.asok") // default changed by common_preinit()
+OPTION(crushtool, OPT_STR, "crushtool") // crushtool utility path
 
 OPTION(daemonize, OPT_BOOL, false) // default changed by common_preinit()
 OPTION(pid_file, OPT_STR, "") // default changed by common_preinit()
@@ -252,7 +253,6 @@ OPTION(mon_osd_min_down_reporters, OPT_INT, 1)   // number of OSDs who need to r
 OPTION(mon_osd_min_down_reports, OPT_INT, 3)     // number of times a down OSD must be reported for it to count
 OPTION(mon_osd_force_trim_to, OPT_INT, 0)   // force mon to trim maps to this point, regardless of min_last_epoch_clean (dangerous, use with care)
 OPTION(mon_mds_force_trim_to, OPT_INT, 0)   // force mon to trim mdsmaps to this point (dangerous, use with care)
-OPTION(crushtool, OPT_STR, "crushtool")
 
 // dump transactions
 OPTION(mon_debug_dump_transactions, OPT_BOOL, false)

src/crush/CrushTester.cc

@@ -1,15 +1,13 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
+// vim: ts=8 sw=2 smarttab
 
 #include "include/stringify.h"
 #include "CrushTester.h"
 
 #include <algorithm>
 #include <stdlib.h>
-/* fork */
-#include <unistd.h>
-/* waitpid */
-#include <sys/types.h>
-#include <sys/wait.h>
-#include <common/errno.h>
+
+#include <common/SubProcess.h>
 
 void CrushTester::set_device_weight(int dev, float f)
 {
@@ -356,81 +354,35 @@ void CrushTester::write_integer_indexed_scalar_data_string(vector<string> &dst,
   dst.push_back( data_buffer.str() );
 }
 
-int CrushTester::test_with_crushtool(const string& crushtool,
+int CrushTester::test_with_crushtool(const char *crushtool_cmd,
                                      int timeout)
 {
-  string timeout_string = stringify(timeout);
-  vector<const char *> cmd_args;
-  cmd_args.push_back("timeout");
-  cmd_args.push_back(timeout_string.c_str());
-  cmd_args.push_back(crushtool.c_str());
-  cmd_args.push_back("-i");
-  cmd_args.push_back("-");
-  cmd_args.push_back("--test");
-  cmd_args.push_back(NULL);
-
-  int pipefds[2];
-  if (::pipe(pipefds) == -1) {
-    int r = errno;
-    err << "error creating pipe: " << cpp_strerror(r) << "\n";
-    return -r;
-  }
-
-  int fpid = fork();
-  if (fpid < 0) {
-    int r = errno;
-    err << "unable to fork(): " << cpp_strerror(r);
-    ::close(pipefds[0]);
-    ::close(pipefds[1]);
-    return -r;
-  } else if (fpid == 0) {
-    ::close(pipefds[1]);
-    ::dup2(pipefds[0], STDIN_FILENO);
-    ::close(pipefds[0]);
-    ::close(1);
-    ::close(2);
-    int r = execvp(cmd_args[0], (char * const *)&cmd_args[0]);
-    if (r < 0)
-      exit(errno);
-    // we should never reach this
-    exit(EINVAL);
+  SubProcess crushtool("timeout", true, false, true);
+  crushtool.add_cmd_args(stringify(timeout).c_str(),
+			 crushtool_cmd, "-i", "-", "--test", NULL);
+  int ret = crushtool.spawn();
+  if (ret != 0) {
+    err << "failed run crushtool: " << crushtool.err();
+    return ret;
   }
-  ::close(pipefds[0]);
 
   bufferlist bl;
   ::encode(crush, bl);
-  bl.write_fd(pipefds[1]);
-  ::close(pipefds[1]);
-
-  int status;
-  int r = waitpid(fpid, &status, 0);
-  assert(r == fpid);
-
-  if (!WIFEXITED(status)) {
-    assert(WIFSIGNALED(status));
-    err << "error testing crush map\n";
-    return -EINVAL;
+  bl.write_fd(crushtool.stdin());
+  crushtool.close_stdin();
+  bl.clear();
+  ret = bl.read_fd(crushtool.stderr(), 100 * 1024);
+  if (ret < 0) {
+    err << "failed read from crushtool: " << cpp_strerror(-ret);
+    return ret;
   }
-
-  r = WEXITSTATUS(status);
-  if (r == 0) {
-    // major success!
-    return 0;
-  }
-  if (r == 124) {
-    // the test takes longer than timeout and was interrupted
-    return -EINTR;
-  }
-
-  if (r == ENOENT) {
-    err << "unable to find " << cmd_args << " to test the map";
-    return -ENOENT;
+  bl.write_stream(err);
+  if (crushtool.join() != 0) {
+    err << crushtool.err();
+    return -EINVAL;
   }
 
-  // something else entirely happened
-  // log it and consider an invalid crush map
-  err << "error running crushmap through crushtool: " << cpp_strerror(r);
-  return -r;
+  return 0;
 }
 
 int CrushTester::test()

src/crush/CrushTester.h

@@ -334,8 +334,8 @@ class CrushTester {
   }
 
   int test();
-  int test_with_crushtool(const string& crushtool,
-			  int timeout);
+  int test_with_crushtool(const char *crushtool_cmd = "crushtool",
+			  int timeout = 0);
 };
 
 #endif

src/mon/OSDMonitor.cc

@@ -4663,7 +4663,7 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
     dout(10) << " testing map" << dendl;
     stringstream ess;
     CrushTester tester(crush, ess);
-    int r = tester.test_with_crushtool(g_conf->crushtool,
+    int r = tester.test_with_crushtool(g_conf->crushtool.c_str(),
 				       g_conf->mon_lease);
     if (r < 0) {
       if (r == -EINTR) {