Merge pull request ceph#4726 from tchaikov/wip-11680-check-empty-crus…

…hmap

mon: check new crush for unknown name/type

Reviewed-by: Loic Dachary <[email protected]>

src/crush/CrushTester.cc

@@ -3,10 +3,11 @@
 
 #include "include/stringify.h"
 #include "CrushTester.h"
+#include "CrushTreeDumper.h"
 
 #include <algorithm>
 #include <stdlib.h>
-
+#include <boost/lexical_cast.hpp>
 #include <common/SubProcess.h>
 
 void CrushTester::set_device_weight(int dev, float f)
@@ -354,10 +355,11 @@ void CrushTester::write_integer_indexed_scalar_data_string(vector<string> &dst,
   dst.push_back( data_buffer.str() );
 }
 
-int CrushTester::test_with_crushtool(const char *crushtool_cmd, int timeout)
+int CrushTester::test_with_crushtool(const char *crushtool_cmd, int max_id, int timeout)
 {
   SubProcessTimed crushtool(crushtool_cmd, true, false, true, timeout);
-  crushtool.add_cmd_args("-i", "-", "--test", NULL);
+  string opt_max_id = boost::lexical_cast<string>(max_id);
+  crushtool.add_cmd_args("-i", "-", "--test", "--check", opt_max_id.c_str(), NULL);
   int ret = crushtool.spawn();
   if (ret != 0) {
     err << "failed run crushtool: " << crushtool.err();
@@ -383,6 +385,58 @@ int CrushTester::test_with_crushtool(const char *crushtool_cmd, int timeout)
   return 0;
 }
 
+namespace {
+  class BadCrushMap : public std::runtime_error {
+  public:
+    int item;
+    BadCrushMap(const char* msg, int id)
+      : std::runtime_error(msg), item(id) {}
+  };
+  // throws if any node in the crush fail to print
+  class CrushWalker : public CrushTreeDumper::Dumper<void> {
+    typedef void DumbFormatter;
+    typedef CrushTreeDumper::Dumper<DumbFormatter> Parent;
+    unsigned max_id;
+  public:
+    CrushWalker(const CrushWrapper *crush, unsigned max_id)
+      : Parent(crush), max_id(max_id) {}
+    void dump_item(const CrushTreeDumper::Item &qi, DumbFormatter *) {
+      int type = -1;
+      if (qi.is_bucket()) {
+	if (!crush->get_item_name(qi.id)) {
+	  throw BadCrushMap("unknown item name", qi.id);
+	}
+	type = crush->get_bucket_type(qi.id);
+      } else {
+	if (max_id > 0 && qi.id >= max_id) {
+	  throw BadCrushMap("item id too large", qi.id);
+	}
+	type = 0;
+      }
+      if (!crush->get_type_name(type)) {
+	throw BadCrushMap("unknown type name", qi.id);
+      }
+    }
+  };
+}
+
+bool CrushTester::check_name_maps(unsigned max_id) const
+{
+  CrushWalker crush_walker(&crush, max_id);
+  try {
+    // walk through the crush, to see if its self-contained
+    crush_walker.dump(NULL);
+    // and see if the maps is also able to handle straying OSDs, whose id >= 0.
+    // "ceph osd tree" will try to print them, even they are not listed in the
+    // crush map.
+    crush_walker.dump_item(CrushTreeDumper::Item(0, 0, 0), NULL);
+  } catch (const BadCrushMap& e) {
+    err << e.what() << ": item#" << e.item << std::endl;
+    return false;
+  }
+  return true;
+}
+
 int CrushTester::test()
 {
   if (min_rule < 0 || max_rule < 0) {

src/crush/CrushTester.h

@@ -333,8 +333,17 @@ class CrushTester {
     min_rule = max_rule = rule;
   }
 
+  /**
+   * check if any bucket/nodes is referencing an unknown name or type
+   * @param max_id rejects any non-bucket items with id less than this number,
+   *               pass 0 to disable this check
+   * @return false if an dangling name/type is referenced or an item id is too
+   *         large, true otherwise
+   */
+  bool check_name_maps(unsigned max_id = 0) const;
   int test();
   int test_with_crushtool(const char *crushtool_cmd = "crushtool",
+			  int max_id = -1,
 			  int timeout = 0);
 };
 

src/mon/OSDMonitor.cc

@@ -4858,6 +4858,7 @@ bool OSDMonitor::prepare_command_impl(MMonCommand *m,
     // then we would consistently trigger an election before the command
     // finishes, having a flapping monitor unable to hold quorum.
     int r = tester.test_with_crushtool(g_conf->crushtool.c_str(),
+				       osdmap.get_max_osd(),
 				       g_conf->mon_lease);
     if (r < 0) {
       derr << "error on crush map: " << ess.str() << dendl;

src/test/cli/crushtool/check-names.empty.crushmap.txt

@@ -0,0 +1,11 @@
+# begin crush map
+
+# devices
+
+# types
+
+# buckets
+
+# rules
+
+# end crush map

src/test/cli/crushtool/check-names.empty.t

@@ -0,0 +1,4 @@
+  $ crushtool -c "$TESTDIR/check-names.empty.crushmap.txt" -o "$TESTDIR/check-names.empty.crushmap"
+  $ crushtool -i "$TESTDIR/check-names.empty.crushmap" --check-names
+  unknown type name: item#0
+  $ rm -f "$TESTDIR/check-names.empty.crushmap"

src/test/cli/crushtool/check-names.max-id.t

@@ -0,0 +1,7 @@
+  $ crushtool -i "$TESTDIR/simple.template" --add-item 0 1.0 device0 --loc host host0 --loc cluster cluster0 -o check-names.crushmap > /dev/null
+  $ crushtool -i check-names.crushmap       --add-item 1 1.0 device1 --loc host host0 --loc cluster cluster0 -o check-names.crushmap > /dev/null
+  $ crushtool -i check-names.crushmap --check 2
+  $ crushtool -i check-names.crushmap       --add-item 2 1.0 device2 --loc host host0 --loc cluster cluster0 -o check-names.crushmap > /dev/null
+  $ crushtool -i check-names.crushmap --check 2
+  item id too large: item#2
+  $ crushtool -i check-names.crushmap --check

src/test/cli/crushtool/help.t

@@ -59,6 +59,7 @@
   Options for the display/test stage
 
      --tree                print map summary as a tree
+     --check [max_id]      check if any item is referencing an unknown name/type
      -i mapfn --show-location id
                            show location for given device id
      -i mapfn --test       test a range of inputs on the map

src/test/mon/osd-crush.sh

@@ -205,6 +205,19 @@ function TEST_crush_rename_bucket() {
     ./ceph osd crush rename-bucket nonexistent something 2>&1 | grep "Error ENOENT" || return 1
 }
 
+function TEST_crush_reject_empty() {
+    local dir=$1
+    run_mon $dir a || return 1
+    # should have at least one OSD
+    run_osd $dir 0 || return 1
+
+    local empty_map=$dir/empty_map
+    :> $empty_map.txt
+    ./crushtool -c $empty_map.txt -o $empty_map.map || return 1
+    expect_failure $dir "Error EINVAL" \
+        ./ceph osd setcrushmap -i $empty_map.map || return 1
+}
+
 main osd-crush "$@"
 
 # Local Variables:

src/tools/crushtool.cc

@@ -165,6 +165,7 @@ void usage()
   cout << "Options for the display/test stage\n";
   cout << "\n";
   cout << "   --tree                print map summary as a tree\n";
+  cout << "   --check [max_id]      check if any item is referencing an unknown name/type\n";
   cout << "   -i mapfn --show-location id\n";
   cout << "                         show location for given device id\n";
   cout << "   -i mapfn --test       test a range of inputs on the map\n";
@@ -226,6 +227,8 @@ int main(int argc, const char **argv)
   std::string infn, srcfn, outfn, add_name, remove_name, reweight_name;
   bool compile = false;
   bool decompile = false;
+  bool check = false;
+  int max_id = -1;
   bool test = false;
   bool display = false;
   bool tree = false;
@@ -311,6 +314,8 @@ int main(int argc, const char **argv)
     } else if (ceph_argparse_witharg(args, i, &val, "-c", "--compile", (char*)NULL)) {
       srcfn = val;
       compile = true;
+    } else if (ceph_argparse_witharg(args, i, &max_id, err, "--check", (char*)NULL)) {
+      check = true;
     } else if (ceph_argparse_flag(args, i, "-t", "--test", (char*)NULL)) {
       test = true;
     } else if (ceph_argparse_witharg(args, i, &full_location, err, "--show-location", (char*)NULL)) {
@@ -497,15 +502,15 @@ int main(int argc, const char **argv)
     }
   }
 
-  if (test && !display && !write_to_file) {
+  if (test && !check && !display && !write_to_file) {
     cerr << "WARNING: no output selected; use --output-csv or --show-X" << std::endl;
   }
 
   if (decompile + compile + build > 1) {
     cerr << "cannot specify more than one of compile, decompile, and build" << std::endl;
     exit(EXIT_FAILURE);
   }
-  if (!compile && !decompile && !build && !test && !reweight && !adjust && !tree &&
+  if (!check && !compile && !decompile && !build && !test && !reweight && !adjust && !tree &&
       add_item < 0 && full_location < 0 &&
       remove_name.empty() && reweight_name.empty()) {
     cerr << "no action specified; -h for help" << std::endl;
@@ -823,6 +828,12 @@ int main(int argc, const char **argv)
     }
   }
 
+  if (check) {
+    if (!tester.check_name_maps(max_id)) {
+      exit(1);
+    }
+  }
+
   if (test) {
     if (tester.get_output_utilization_all() ||
 	tester.get_output_utilization())