crush: reset bucket->h.items[i] when removing tree item

* crush: so we don't see the reference after the removing, this keeps
  check_item_loc() happy, and move_bucket() use check_item_loc() to see if
  the removed bucket disappears after the removal.
* test: also add unittest_crush_wrapper::CrushWrapper.insert_item

Fixes: http://tracker.ceph.com/issues/16525
Signed-off-by: Kefu Chai <kchai@redhat.com>

diff --git a/src/crush/builder.c b/src/crush/builder.c
index 387c8beceb85f32c756e464c180098638bf3b7cc..9331f6d8bada9a369bb57b6dbdf43e5b41380316 100644 (file)
--- a/src/crush/builder.c
+++ b/src/crush/builder.c
@@ -1015,7 +1015,8 @@ int crush_remove_tree_bucket_item(struct crush_bucket_tree *bucket, int item)
 
                if (bucket->h.items[i] != item)
                        continue;
-               
+
+               bucket->h.items[i] = 0;
                node = crush_calc_tree_node(i);
                weight = bucket->node_weights[node];
                bucket->node_weights[node] = 0;

diff --git a/src/test/crush/CrushWrapper.cc b/src/test/crush/CrushWrapper.cc
index 1d8527441e7b0ea2618caa7df883390c34f92960..035be924c3da7f59416e3ca99453525474020ae4 100644 (file)
--- a/src/test/crush/CrushWrapper.cc
+++ b/src/test/crush/CrushWrapper.cc
@@ -708,6 +708,48 @@ TEST(CrushWrapper, insert_item) {
   delete c;
 }
 
+TEST(CrushWrapper, remove_item) {
+  auto *c = new CrushWrapper;
+
+  const int ROOT_TYPE = 2;
+  c->set_type_name(ROOT_TYPE, "root");
+  const int HOST_TYPE = 1;
+  c->set_type_name(HOST_TYPE, "host");
+  const int OSD_TYPE = 0;
+  c->set_type_name(OSD_TYPE, "osd");
+
+  {
+    int root;
+    ASSERT_EQ(0, c->add_bucket(0, CRUSH_BUCKET_STRAW, CRUSH_HASH_RJENKINS1,
+                              ROOT_TYPE, 0, NULL, NULL, &root));
+    c->set_item_name(root, "root0");
+  }
+
+  {
+    int host;
+    c->add_bucket(0, CRUSH_BUCKET_TREE, CRUSH_HASH_RJENKINS1,
+                 HOST_TYPE, 0, NULL, NULL, &host);
+    c->set_item_name(host, "host0");
+  }
+
+  const int num_osd = 12;
+  {
+    map<string, string> loc = {{"root", "root0"},
+                              {"host", "host0"}};
+    string name{"osd."};
+    for (int item = 0; item < num_osd; item++) {
+      ASSERT_EQ(0, c->insert_item(g_ceph_context, item, 1.0,
+                                 name + to_string(item), loc));
+    }
+  }
+  const int item_to_remove = num_osd / 2;
+  map<string, string> loc;
+  loc.insert(c->get_immediate_parent(item_to_remove));
+  ASSERT_EQ(0, c->remove_item(g_ceph_context, item_to_remove, true));
+  float weight;
+  EXPECT_FALSE(c->check_item_loc(g_ceph_context, item_to_remove, loc, &weight));
+}
+
 TEST(CrushWrapper, item_bucket_names) {
   CrushWrapper *c = new CrushWrapper;
   int index = 123;