Merge pull request #8375 from rjfd/wip-15265

librbd: disable image mirroring when image is removed

Reviewed-by: Jason Dillaman <dillaman@redhat.com>

src/librbd/internal.cc

@@ -280,8 +280,20 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force) {
   std::set<cls::journal::Client> clients;
   std::string header_oid;
 
+  int r = cls_client::mirror_image_get(&ictx->md_ctx, ictx->id,
+      &mirror_image_internal);
+  if (r < 0 && r != -ENOENT) {
+    lderr(cct) << "cannot disable mirroring: " << cpp_strerror(r) << dendl;
+    return r;
+  } else if (r == -ENOENT) {
+    // mirroring is not enabled for this image
+    ldout(cct, 20) << "ignoring disable command: mirroring is not enabled "
+      "for this image" << dendl;
+    return 0;
+  }
+
   bool is_primary;
-  int r = Journal<>::is_tag_owner(ictx, &is_primary);
+  r = Journal<>::is_tag_owner(ictx, &is_primary);
   if (r < 0) {
     lderr(cct) << "cannot disable mirroring: failed to check tag ownership: "
       << cpp_strerror(r) << dendl;
@@ -297,19 +309,6 @@ int mirror_image_disable_internal(ImageCtx *ictx, bool force) {
     goto remove_mirroring_image;
   }
 
-  r = cls_client::mirror_image_get(&ictx->md_ctx, ictx->id,
-      &mirror_image_internal);
-  if (r < 0 && r != -ENOENT) {
-    lderr(cct) << "cannot disable mirroring: " << cpp_strerror(r) << dendl;
-    return r;
-  }
-  else if (r == -ENOENT) {
-    // mirroring is not enabled for this image
-    ldout(cct, 20) << "ignoring disable command: mirroring is not enabled "
-      "for this image" << dendl;
-    return 0;
-  }
-
   mirror_image_internal.state =
     cls::rbd::MirrorImageState::MIRROR_IMAGE_STATE_DISABLING;
   r = cls_client::mirror_image_set(&ictx->md_ctx, ictx->id,
@@ -1943,6 +1942,17 @@ remove_mirroring_image:
 	return r;
       }
 
+      if (!old_format) {
+        r = mirror_image_disable_internal(ictx, false);
+        if (r < 0) {
+          lderr(cct) << "error disabling image mirroring: " << cpp_strerror(r)
+                     << dendl;
+          ictx->owner_lock.put_read();
+          ictx->state->close();
+          return r;
+        }
+      }
+
       ictx->owner_lock.put_read();
       ictx->state->close();
 

src/test/librbd/test_mirroring.cc

@@ -26,7 +26,6 @@
 #include <boost/assign/list_of.hpp>
 #include <utility>
 #include <vector>
-#include <tuple>
 
 void register_test_mirroring() {
 }
@@ -69,10 +68,6 @@ public:
     ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, sizeof(mirror_image)));
     ASSERT_EQ(mirror_state, mirror_image.state);
 
-    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE));
-    if (mirror_state != RBD_MIRROR_IMAGE_DISABLED) {
-      ASSERT_EQ(0, image.mirror_image_disable(false));
-    }
     ASSERT_EQ(0, image.close());
     ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str()));
     ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED));
@@ -98,10 +93,6 @@ public:
     ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, sizeof(mirror_image)));
     ASSERT_EQ(mirror_state, mirror_image.state);
 
-    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE));
-    if (mirror_state != RBD_MIRROR_IMAGE_DISABLED) {
-      ASSERT_EQ(0, image.mirror_image_disable(false));
-    }
     ASSERT_EQ(0, image.close());
     ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str()));
     ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED));
@@ -122,10 +113,6 @@ public:
     ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, sizeof(mirror_image)));
     ASSERT_EQ(mirror_state, mirror_image.state);
 
-    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE));
-    if (mirror_state != RBD_MIRROR_IMAGE_DISABLED) {
-      ASSERT_EQ(0, image.mirror_image_disable(false));
-    }
     ASSERT_EQ(0, image.close());
     ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str()));
     ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED));
@@ -154,10 +141,6 @@ public:
     ASSERT_EQ(0, image.mirror_image_get_info(&mirror_image, sizeof(mirror_image)));
     ASSERT_EQ(mirror_state, mirror_image.state);
 
-    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE));
-    if (mirror_state != RBD_MIRROR_IMAGE_DISABLED) {
-      ASSERT_EQ(0, image.mirror_image_disable(false));
-    }
     ASSERT_EQ(0, image.close());
     ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str()));
     ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED));
@@ -198,22 +181,27 @@ public:
       ASSERT_EQ(mirror_state, mirror_image.state);
 
       ASSERT_EQ(0, image.close());
+      ASSERT_EQ(0, m_rbd.remove(m_ioctx, img_name_str.c_str()));
+    }
   }
 
-    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_IMAGE));
+  void check_remove_image(rbd_mirror_mode_t mirror_mode, uint64_t features,
-    for (const auto& tuple : images) {
+                          bool enable_mirroring) {
-      std::string img_name;
-      rbd_mirror_image_state_t mirror_state;
-      std::tie(img_name, mirror_state) = tuple;
 
+    ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, mirror_mode));
+
+    int order = 20;
+    ASSERT_EQ(0, m_rbd.create2(m_ioctx, image_name.c_str(), 4096, features,
+              &order));
     librbd::Image image;
-      ASSERT_EQ(0, m_rbd.open(m_ioctx, image, img_name.c_str()));
+    ASSERT_EQ(0, m_rbd.open(m_ioctx, image, image_name.c_str()));
-      if (mirror_state != RBD_MIRROR_IMAGE_DISABLED) {
+
-        ASSERT_EQ(0, image.mirror_image_disable(false));
+    if (enable_mirroring) {
-      }
+      ASSERT_EQ(0, image.mirror_image_enable());
-      ASSERT_EQ(0, image.close());
-      ASSERT_EQ(0, m_rbd.remove(m_ioctx, img_name.c_str()));
     }
+
+    image.close();
+    ASSERT_EQ(0, m_rbd.remove(m_ioctx, image_name.c_str()));
     ASSERT_EQ(0, m_rbd.mirror_mode_set(m_ioctx, RBD_MIRROR_MODE_DISABLED));
   }
 
@@ -454,3 +442,21 @@ TEST_F(TestMirroring, MirrorModeSet_ImageMode_To_DisabledMode) {
   check_mirroring_on_mirror_mode_set(RBD_MIRROR_MODE_DISABLED, states_vec);
 }
 
+TEST_F(TestMirroring, RemoveImage_With_MirrorImageEnabled) {
+  check_remove_image(RBD_MIRROR_MODE_IMAGE,
+                     RBD_FEATURE_EXCLUSIVE_LOCK | RBD_FEATURE_JOURNALING,
+                     true);
+}
+
+TEST_F(TestMirroring, RemoveImage_With_MirrorImageDisabled) {
+  check_remove_image(RBD_MIRROR_MODE_IMAGE,
+                     RBD_FEATURE_EXCLUSIVE_LOCK | RBD_FEATURE_JOURNALING,
+                     false);
+}
+
+TEST_F(TestMirroring, RemoveImage_With_ImageWithoutJournal) {
+  check_remove_image(RBD_MIRROR_MODE_IMAGE,
+                     RBD_FEATURE_EXCLUSIVE_LOCK,
+                     false);
+}
+