librbd: track in-progress migration aborting operation

We want to prevent the destination image from being used while an
abort is in-progress. Test that the image has no watchers prior to
permitting the abort, switch the migration state to ABORTING, and
treat the image as read-only if the migration state is ABORTING.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>
(cherry picked from commit d848b4f1c083bd9f3e6eac3e186c9e7df2e22004)

diff --git a/src/cls/rbd/cls_rbd_types.cc b/src/cls/rbd/cls_rbd_types.cc
index 5f3c40b049f2f312a8d6b6696ef295b88958be70..b6a02496bcb5feaf3333a44ab15bc40a45db844d 100644 (file)
--- a/src/cls/rbd/cls_rbd_types.cc
+++ b/src/cls/rbd/cls_rbd_types.cc
@@ -1201,6 +1201,9 @@ std::ostream& operator<<(std::ostream& os,
   case MIGRATION_STATE_EXECUTED:
     os << "executed";
     break;
+  case MIGRATION_STATE_ABORTING:
+    os << "aborting";
+    break;
   default:
     os << "unknown (" << static_cast<uint32_t>(migration_state) << ")";
     break;

diff --git a/src/cls/rbd/cls_rbd_types.h b/src/cls/rbd/cls_rbd_types.h
index 5ea37caf7d45ceaa93dd3b3b974fc7d84e9d7dde..ab77735f8f906f4588038afe3daa0b18b9baf16a 100644 (file)
--- a/src/cls/rbd/cls_rbd_types.h
+++ b/src/cls/rbd/cls_rbd_types.h
@@ -926,6 +926,7 @@ enum MigrationState {
   MIGRATION_STATE_PREPARED = 2,
   MIGRATION_STATE_EXECUTING = 3,
   MIGRATION_STATE_EXECUTED = 4,
+  MIGRATION_STATE_ABORTING = 5,
 };
 
 inline void encode(const MigrationState &state, bufferlist& bl) {

diff --git a/src/include/rbd/librbd.h b/src/include/rbd/librbd.h
index c06c6017118ef1ced13c212ba4d3d89eaea7bd9f..495e904a697fcb380b26cf4cc4c2e0895b67582e 100644 (file)
--- a/src/include/rbd/librbd.h
+++ b/src/include/rbd/librbd.h
@@ -325,6 +325,7 @@ typedef enum {
   RBD_IMAGE_MIGRATION_STATE_PREPARED = 2,
   RBD_IMAGE_MIGRATION_STATE_EXECUTING = 3,
   RBD_IMAGE_MIGRATION_STATE_EXECUTED = 4,
+  RBD_IMAGE_MIGRATION_STATE_ABORTING = 5,
 } rbd_image_migration_state_t;
 
 typedef struct {

diff --git a/src/librbd/api/Migration.cc b/src/librbd/api/Migration.cc
index a5c22418293932fc3504e6f675c8c5ff0b6caa8a..a2cdabe1d589d0259f32142b595939f9f86c04a2 100644 (file)
--- a/src/librbd/api/Migration.cc
+++ b/src/librbd/api/Migration.cc
@@ -791,8 +791,36 @@ int Migration<I>::abort() {
     ldout(m_cct, 1) << "failed to open destination image: " << cpp_strerror(r)
                     << dendl;
   } else {
-    ldout(m_cct, 10) << "relinking children" << dendl;
+    BOOST_SCOPE_EXIT_TPL(&dst_image_ctx) {
+      if (dst_image_ctx != nullptr) {
+        dst_image_ctx->state->close();
+      }
+    } BOOST_SCOPE_EXIT_END;
+
+    std::list<obj_watch_t> watchers;
+    int flags = librbd::image::LIST_WATCHERS_FILTER_OUT_MY_INSTANCE |
+                librbd::image::LIST_WATCHERS_FILTER_OUT_MIRROR_INSTANCES;
+    C_SaferCond on_list_watchers;
+    auto list_watchers_request = librbd::image::ListWatchersRequest<I>::create(
+        *dst_image_ctx, flags, &watchers, &on_list_watchers);
+    list_watchers_request->send();
+    r = on_list_watchers.wait();
+    if (r < 0) {
+      lderr(m_cct) << "failed listing watchers:" << cpp_strerror(r) << dendl;
+      return r;
+    }
+    if (!watchers.empty()) {
+      lderr(m_cct) << "image has watchers - cannot abort migration" << dendl;
+      return -EBUSY;
+    }
 
+    // ensure destination image is now read-only
+    r = set_state(cls::rbd::MIGRATION_STATE_ABORTING, "");
+    if (r < 0) {
+      return r;
+    }
+
+    ldout(m_cct, 10) << "relinking children" << dendl;
     r = relink_children(dst_image_ctx, m_src_image_ctx);
     if (r < 0) {
       return r;
@@ -800,12 +828,6 @@ int Migration<I>::abort() {
 
     ldout(m_cct, 10) << "removing dst image snapshots" << dendl;
 
-    BOOST_SCOPE_EXIT_TPL(&dst_image_ctx) {
-      if (dst_image_ctx != nullptr) {
-        dst_image_ctx->state->close();
-      }
-    } BOOST_SCOPE_EXIT_END;
-
     std::vector<librbd::snap_info_t> snaps;
     r = Snapshot<I>::list(dst_image_ctx, snaps);
     if (r < 0) {
@@ -853,7 +875,7 @@ int Migration<I>::abort() {
                    << m_dst_io_ctx.get_pool_name() << "/" << m_dst_image_name
                    << " (" << m_dst_image_id << ")': " << cpp_strerror(r)
                    << dendl;
-      // not fatal
+      return r;
     }
   }
 

diff --git a/src/librbd/image/RefreshRequest.cc b/src/librbd/image/RefreshRequest.cc
index 560b22ee6106b0bde65e54fb4ce9dfa4c4c65541..590242c353e06a4c9163eead768b5730ec6d4488 100644 (file)
--- a/src/librbd/image/RefreshRequest.cc
+++ b/src/librbd/image/RefreshRequest.cc
@@ -124,13 +124,29 @@ Context *RefreshRequest<I>::handle_get_migration_header(int *result) {
   case cls::rbd::MIGRATION_HEADER_TYPE_DST:
     ldout(cct, 1) << this << " " << __func__ << ": migrating from: "
                   << m_migration_spec << dendl;
-    if (m_migration_spec.state != cls::rbd::MIGRATION_STATE_PREPARED &&
-        m_migration_spec.state != cls::rbd::MIGRATION_STATE_EXECUTING &&
-        m_migration_spec.state != cls::rbd::MIGRATION_STATE_EXECUTED) {
+    switch (m_migration_spec.state) {
+    case cls::rbd::MIGRATION_STATE_PREPARING:
       ldout(cct, 5) << this << " " << __func__ << ": current migration state: "
                     << m_migration_spec.state << ", retrying" << dendl;
       send();
       return nullptr;
+    case cls::rbd::MIGRATION_STATE_PREPARED:
+    case cls::rbd::MIGRATION_STATE_EXECUTING:
+    case cls::rbd::MIGRATION_STATE_EXECUTED:
+      break;
+    case cls::rbd::MIGRATION_STATE_ABORTING:
+      if (!m_read_only) {
+        lderr(cct) << this << " " << __func__ << ": migration is being aborted"
+                   << dendl;
+        *result = -EROFS;
+        return m_on_finish;
+      }
+      break;
+    default:
+      lderr(cct) << this << " " << __func__ << ": migration is in an "
+                 << "unexpected state" << dendl;
+      *result = -EINVAL;
+      return m_on_finish;
     }
     break;
   default:
@@ -1301,8 +1317,14 @@ void RefreshRequest<I>::apply() {
     m_image_ctx.operations_disabled = (
       (m_op_features & ~RBD_OPERATION_FEATURES_ALL) != 0ULL);
     m_image_ctx.group_spec = m_group_spec;
-    if (get_migration_info(&m_image_ctx.parent_md,
-                           &m_image_ctx.migration_info)) {
+
+    bool migration_info_valid;
+    int r = get_migration_info(&m_image_ctx.parent_md,
+                               &m_image_ctx.migration_info,
+                               &migration_info_valid);
+    ceph_assert(r == 0); // validated in refresh parent step
+
+    if (migration_info_valid) {
       for (auto it : m_image_ctx.migration_info.snap_map) {
         migration_reverse_snap_seq[it.second.front()] = it.first;
       }
@@ -1313,7 +1335,7 @@ void RefreshRequest<I>::apply() {
 
     librados::Rados rados(m_image_ctx.md_ctx);
     int8_t require_osd_release;
-    int r = rados.get_min_compatible_osd(&require_osd_release);
+    r = rados.get_min_compatible_osd(&require_osd_release);
     if (r == 0 && require_osd_release >= CEPH_RELEASE_OCTOPUS) {
       m_image_ctx.enable_sparse_copyup = true;
     }
@@ -1417,7 +1439,13 @@ template <typename I>
 int RefreshRequest<I>::get_parent_info(uint64_t snap_id,
                                        ParentImageInfo *parent_md,
                                        MigrationInfo *migration_info) {
-  if (get_migration_info(parent_md, migration_info)) {
+  bool migration_info_valid;
+  int r = get_migration_info(parent_md, migration_info, &migration_info_valid);
+  if (r < 0) {
+    return r;
+  }
+
+  if (migration_info_valid) {
     return 0;
   } else if (snap_id == CEPH_NOSNAP) {
     *parent_md = m_parent_md;
@@ -1436,17 +1464,24 @@ int RefreshRequest<I>::get_parent_info(uint64_t snap_id,
 }
 
 template <typename I>
-bool RefreshRequest<I>::get_migration_info(ParentImageInfo *parent_md,
-                                           MigrationInfo *migration_info) {
+int RefreshRequest<I>::get_migration_info(ParentImageInfo *parent_md,
+                                          MigrationInfo *migration_info,
+                                          bool* migration_info_valid) {
+  CephContext *cct = m_image_ctx.cct;
   if (m_migration_spec.header_type != cls::rbd::MIGRATION_HEADER_TYPE_DST ||
       (m_migration_spec.state != cls::rbd::MIGRATION_STATE_PREPARED &&
-       m_migration_spec.state != cls::rbd::MIGRATION_STATE_EXECUTING)) {
-    ceph_assert(m_migration_spec.header_type ==
-                    cls::rbd::MIGRATION_HEADER_TYPE_SRC ||
-                m_migration_spec.pool_id == -1 ||
-                m_migration_spec.state == cls::rbd::MIGRATION_STATE_EXECUTED);
+       m_migration_spec.state != cls::rbd::MIGRATION_STATE_EXECUTING &&
+       m_migration_spec.state != cls::rbd::MIGRATION_STATE_ABORTING)) {
+    if (m_migration_spec.header_type != cls::rbd::MIGRATION_HEADER_TYPE_SRC &&
+        m_migration_spec.pool_id != -1 &&
+        m_migration_spec.state != cls::rbd::MIGRATION_STATE_EXECUTED) {
+      lderr(cct) << this << " " << __func__ << ": invalid migration spec"
+                 << dendl;
+      return -EINVAL;
+    }
 
-    return false;
+    *migration_info_valid = false;
+    return 0;
   }
 
   parent_md->spec.pool_id = m_migration_spec.pool_id;
@@ -1483,10 +1518,11 @@ bool RefreshRequest<I>::get_migration_info(ParentImageInfo *parent_md,
   *migration_info = {m_migration_spec.pool_id, m_migration_spec.pool_namespace,
                      m_migration_spec.image_name, m_migration_spec.image_id, {},
                      overlap, m_migration_spec.flatten};
+  *migration_info_valid = true;
 
   deep_copy::util::compute_snap_map(m_image_ctx.cct, 0, CEPH_NOSNAP, {},
                                     snap_seqs, &migration_info->snap_map);
-  return true;
+  return 0;
 }
 
 } // namespace image

diff --git a/src/librbd/image/RefreshRequest.h b/src/librbd/image/RefreshRequest.h
index 4e314c7e6980c635ef632c7172d5d7f72fa1c427..6970a9b45626cd8e62759335cc45add3372f8084 100644 (file)
--- a/src/librbd/image/RefreshRequest.h
+++ b/src/librbd/image/RefreshRequest.h
@@ -258,8 +258,9 @@ private:
   void apply();
   int get_parent_info(uint64_t snap_id, ParentImageInfo *parent_md,
                       MigrationInfo *migration_info);
-  bool get_migration_info(ParentImageInfo *parent_md,
-                          MigrationInfo *migration_info);
+  int get_migration_info(ParentImageInfo *parent_md,
+                         MigrationInfo *migration_info,
+                         bool* migration_info_valid);
 };
 
 } // namespace image

diff --git a/src/pybind/rbd/rbd.pyx b/src/pybind/rbd/rbd.pyx
index 72da0ee9fbb35a9216ba1a1b35791e3ebd0c2a32..70d032b478913f4e30940ad47466f0bf5aef6f9c 100644 (file)
--- a/src/pybind/rbd/rbd.pyx
+++ b/src/pybind/rbd/rbd.pyx
@@ -273,6 +273,7 @@ cdef extern from "rbd/librbd.h" nogil:
         _RBD_IMAGE_MIGRATION_STATE_PREPARED "RBD_IMAGE_MIGRATION_STATE_PREPARED"
         _RBD_IMAGE_MIGRATION_STATE_EXECUTING "RBD_IMAGE_MIGRATION_STATE_EXECUTING"
         _RBD_IMAGE_MIGRATION_STATE_EXECUTED "RBD_IMAGE_MIGRATION_STATE_EXECUTED"
+        _RBD_IMAGE_MIGRATION_STATE_ABORTING "RBD_IMAGE_MIGRATION_STATE_ABORTING"
 
     ctypedef struct rbd_image_migration_status_t:
         int64_t source_pool_id
@@ -790,6 +791,7 @@ RBD_IMAGE_MIGRATION_STATE_PREPARING = _RBD_IMAGE_MIGRATION_STATE_PREPARING
 RBD_IMAGE_MIGRATION_STATE_PREPARED = _RBD_IMAGE_MIGRATION_STATE_PREPARED
 RBD_IMAGE_MIGRATION_STATE_EXECUTING = _RBD_IMAGE_MIGRATION_STATE_EXECUTING
 RBD_IMAGE_MIGRATION_STATE_EXECUTED = _RBD_IMAGE_MIGRATION_STATE_EXECUTED
+RBD_IMAGE_MIGRATION_STATE_ABORTING = _RBD_IMAGE_MIGRATION_STATE_ABORTING
 
 RBD_CONFIG_SOURCE_CONFIG = _RBD_CONFIG_SOURCE_CONFIG
 RBD_CONFIG_SOURCE_POOL = _RBD_CONFIG_SOURCE_POOL

diff --git a/src/test/librbd/test_Migration.cc b/src/test/librbd/test_Migration.cc
index 19512036ad365f70b0b780f9bd4efce6680bfdf5..38fdabe52ecfd5387eb3243842e6ccd26a821e38 100644 (file)
--- a/src/test/librbd/test_Migration.cc
+++ b/src/test/librbd/test_Migration.cc
@@ -1107,6 +1107,15 @@ TEST_F(TestMigration, SnapTrimBeforePrepare)
   migration_commit(m_ioctx, m_image_name);
 }
 
+TEST_F(TestMigration, AbortInUseImage) {
+  migration_prepare(m_ioctx, m_image_name);
+  migration_status(RBD_IMAGE_MIGRATION_STATE_PREPARED);
+
+  librbd::NoOpProgressContext no_op;
+  EXPECT_EQ(-EBUSY, librbd::api::Migration<>::abort(m_ioctx, m_ictx->name,
+                                                    no_op));
+}
+
 TEST_F(TestMigration, CloneV1Parent)
 {
   const uint32_t CLONE_FORMAT = 1;

diff --git a/src/tools/rbd/action/Status.cc b/src/tools/rbd/action/Status.cc
index 75afdd7743117558a82457fbc8598e8cdc1b3b0b..0a599e7f0e04d1f8bb714165554b045c2e816d4c 100644 (file)
--- a/src/tools/rbd/action/Status.cc
+++ b/src/tools/rbd/action/Status.cc
@@ -79,6 +79,9 @@ static int do_show_status(librados::IoCtx& io_ctx, const std::string &image_name
       case RBD_IMAGE_MIGRATION_STATE_EXECUTED:
         migration_state = "executed";
         break;
+      case RBD_IMAGE_MIGRATION_STATE_ABORTING:
+        migration_state = "aborting";
+        break;
       default:
         migration_state = "unknown";
       }