Merge pull request ceph#63149 from ajarr/wip-ajarr-fix-mirror-image-get-mode

librbd/api/Mirror: return EINVAL from image_get_mode() when the image is disabled for mirroring

Reviewed-by: Ilya Dryomov <[email protected]>
Reviewed-by: Afreen Misbah <[email protected]>

Author: idryomov

PendingReleaseNotes

@@ -156,6 +156,21 @@
   Related trackers:
    - https://tracker.ceph.com/issues/67777
 
+* RBD: Fetching the mirroring mode of an image is invalid if the image is
+  disabled for mirroring. The public APIs -- C++ `mirror_image_get_mode()`,
+  C `rbd_mirror_image_get_mode()`, and Python `Image.mirror_image_get_mode()`
+  -- will return EINVAL when mirroring is disabled.
+
+* RBD: Promoting an image is invalid if the image is not enabled for mirroring.
+  The public APIs -- C++ `mirror_image_promote()`,
+  C `rbd_mirror_image_promote()`, and Python `Image.mirror_image_promote()` --
+  will return EINVAL instead of ENOENT when mirroring is not enabled.
+
+* RBD: Requesting a resync on an image is invalid if the image is not enabled
+  for mirroring. The public APIs -- C++ `mirror_image_resync()`,
+  C `rbd_mirror_image_resync()`, and Python `Image.mirror_image_resync()` --
+  will return EINVAL instead of ENOENT when mirroring is not enabled.
+
 >=19.2.1
 
 * CephFS: Command `fs subvolume create` now allows tagging subvolumes through option

src/librbd/api/Mirror.cc

@@ -311,37 +311,29 @@ const char *pool_or_namespace(I *ictx) {
 
 struct C_ImageGetInfo : public Context {
   mirror_image_info_t *mirror_image_info;
-  mirror_image_mode_t *mirror_image_mode;
   Context *on_finish;
 
   cls::rbd::MirrorImage mirror_image;
   mirror::PromotionState promotion_state = mirror::PROMOTION_STATE_PRIMARY;
   std::string primary_mirror_uuid;
 
-  C_ImageGetInfo(mirror_image_info_t *mirror_image_info,
-                 mirror_image_mode_t *mirror_image_mode,  Context *on_finish)
-    : mirror_image_info(mirror_image_info),
-      mirror_image_mode(mirror_image_mode), on_finish(on_finish) {
+  C_ImageGetInfo(mirror_image_info_t *mirror_image_info, Context *on_finish)
+    : mirror_image_info(mirror_image_info), on_finish(on_finish) {
   }
 
   void finish(int r) override {
+    // Suppress ENOENT returned by GetInfoRequest when mirroring is
+    // disabled -- mirror_image.state will indicate that anyway.
     if (r < 0 && r != -ENOENT) {
       on_finish->complete(r);
       return;
     }
 
-    if (mirror_image_info != nullptr) {
-      mirror_image_info->global_id = mirror_image.global_image_id;
-      mirror_image_info->state = static_cast<rbd_mirror_image_state_t>(
-        mirror_image.state);
-      mirror_image_info->primary = (
-        promotion_state == mirror::PROMOTION_STATE_PRIMARY);
-    }
-
-    if (mirror_image_mode != nullptr) {
-      *mirror_image_mode =
-        static_cast<rbd_mirror_image_mode_t>(mirror_image.mode);
-    }
+    mirror_image_info->global_id = mirror_image.global_image_id;
+    mirror_image_info->state = static_cast<mirror_image_state_t>(
+      mirror_image.state);
+    mirror_image_info->primary = (
+      promotion_state == mirror::PROMOTION_STATE_PRIMARY);
 
     on_finish->complete(0);
   }
@@ -357,7 +349,7 @@ struct C_ImageGetGlobalStatus : public C_ImageGetInfo {
       const std::string &image_name,
       mirror_image_global_status_t *mirror_image_global_status,
       Context *on_finish)
-    : C_ImageGetInfo(&mirror_image_global_status->info, nullptr, on_finish),
+    : C_ImageGetInfo(&mirror_image_global_status->info, on_finish),
       image_name(image_name),
       mirror_image_global_status(mirror_image_global_status) {
   }
@@ -384,6 +376,36 @@ struct C_ImageGetGlobalStatus : public C_ImageGetInfo {
   }
 };
 
+struct C_ImageGetMode : public Context {
+  mirror_image_mode_t *mirror_image_mode;
+  Context *on_finish;
+
+  cls::rbd::MirrorImage mirror_image;
+  mirror::PromotionState promotion_state = mirror::PROMOTION_STATE_PRIMARY;
+  std::string primary_mirror_uuid;
+
+  C_ImageGetMode(mirror_image_mode_t *mirror_image_mode,  Context *on_finish)
+    : mirror_image_mode(mirror_image_mode), on_finish(on_finish) {
+  }
+
+  void finish(int r) override {
+    // Suppress ENOENT returned by GetInfoRequest when mirroring is
+    // disabled -- mirror_image.state will indicate that anyway.
+    if (r < 0 && r != -ENOENT) {
+      on_finish->complete(r);
+      return;
+    } else if (mirror_image.state == cls::rbd::MIRROR_IMAGE_STATE_DISABLED) {
+      on_finish->complete(-EINVAL);
+      return;
+    }
+
+    *mirror_image_mode =
+      static_cast<mirror_image_mode_t>(mirror_image.mode);
+
+    on_finish->complete(0);
+  }
+};
+
 template <typename I>
 struct C_ImageSnapshotCreate : public Context {
   I *ictx;
@@ -793,11 +815,14 @@ int Mirror<I>::image_resync(I *ictx) {
   req->send();
 
   r = get_info_ctx.wait();
-  if (r < 0) {
+  if (r < 0 && r != -ENOENT) {
+    lderr(cct) << "failed to retrieve mirroring state, cannot resync: "
+               << cpp_strerror(r) << dendl;
     return r;
-  }
-
-  if (promotion_state == mirror::PROMOTION_STATE_PRIMARY) {
+  } else if (mirror_image.state != cls::rbd::MIRROR_IMAGE_STATE_ENABLED) {
+    lderr(cct) << "mirroring is not enabled, cannot resync" << dendl;
+    return -EINVAL;
+  } else if (promotion_state == mirror::PROMOTION_STATE_PRIMARY) {
     lderr(cct) << "image is primary, cannot resync to itself" << dendl;
     return -EINVAL;
   }
@@ -858,7 +883,7 @@ void Mirror<I>::image_get_info(I *ictx, mirror_image_info_t *mirror_image_info,
         return;
       }
 
-      auto ctx = new C_ImageGetInfo(mirror_image_info, nullptr, on_finish);
+      auto ctx = new C_ImageGetInfo(mirror_image_info, on_finish);
       auto req = mirror::GetInfoRequest<I>::create(*ictx, &ctx->mirror_image,
                                                    &ctx->promotion_state,
                                                    &ctx->primary_mirror_uuid,
@@ -895,7 +920,7 @@ void Mirror<I>::image_get_info(librados::IoCtx& io_ctx,
   ldout(cct, 20) << "pool_id=" << io_ctx.get_id() << ", image_id=" << image_id
                  << dendl;
 
-  auto ctx = new C_ImageGetInfo(mirror_image_info, nullptr, on_finish);
+  auto ctx = new C_ImageGetInfo(mirror_image_info, on_finish);
   auto req = mirror::GetInfoRequest<I>::create(io_ctx, op_work_queue, image_id,
                                                &ctx->mirror_image,
                                                &ctx->promotion_state,
@@ -924,7 +949,7 @@ void Mirror<I>::image_get_mode(I *ictx, mirror_image_mode_t *mode,
   CephContext *cct = ictx->cct;
   ldout(cct, 20) << "ictx=" << ictx << dendl;
 
-  auto ctx = new C_ImageGetInfo(nullptr, mode, on_finish);
+  auto ctx = new C_ImageGetMode(mode, on_finish);
   auto req = mirror::GetInfoRequest<I>::create(*ictx, &ctx->mirror_image,
                                                &ctx->promotion_state,
                                                &ctx->primary_mirror_uuid, ctx);

src/librbd/mirror/PromoteRequest.cc

@@ -45,7 +45,7 @@ void PromoteRequest<I>::handle_get_info(int r) {
   CephContext *cct = m_image_ctx.cct;
   ldout(cct, 20) << "r=" << r << dendl;
 
-  if (r < 0) {
+  if (r < 0 && r != -ENOENT) {
     lderr(cct) << "failed to retrieve mirroring state: " << cpp_strerror(r)
                << dendl;
     finish(r);

src/pybind/mgr/dashboard/controllers/rbd.py

@@ -235,8 +235,11 @@ def create(self, image_spec, snapshot_name, mirrorImageSnapshot):
 
         def _create_snapshot(ioctx, img, snapshot_name):
             mirror_info = img.mirror_image_get_info()
-            mirror_mode = img.mirror_image_get_mode()
-            if (mirror_info['state'] == rbd.RBD_MIRROR_IMAGE_ENABLED and mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT) and mirrorImageSnapshot:  # noqa E501 #pylint: disable=line-too-long
+            mirror_mode = None
+            if mirror_info['state'] == rbd.RBD_MIRROR_IMAGE_ENABLED:
+                mirror_mode = img.mirror_image_get_mode()
+
+            if (mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT) and mirrorImageSnapshot:
                 img.mirror_image_create_snapshot()
             else:
                 img.create_snap(snapshot_name)

src/pybind/mgr/dashboard/controllers/rbd_mirroring.py

@@ -241,8 +241,14 @@ class ReplayingData(NamedTuple):
 
 def _get_mirror_mode(ioctx, image_name):
     with rbd.Image(ioctx, image_name) as img:
-        mirror_mode = img.mirror_image_get_mode()
+        mirror_mode = None
         mirror_mode_str = 'Disabled'
+        try:
+            mirror_mode = img.mirror_image_get_mode()
+        except rbd.InvalidArgument:
+            # Suppress exception raised when mirroring is disabled
+            pass
+
         if mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_JOURNAL:
             mirror_mode_str = 'journal'
         elif mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT:

src/pybind/mgr/dashboard/services/rbd.py

@@ -305,8 +305,13 @@ def _rbd_image(cls, ioctx, pool_name, namespace, image_name,  # pylint: disable=
         with rbd.Image(ioctx, image_name) as img:
             stat = img.stat()
             mirror_info = img.mirror_image_get_info()
-            mirror_mode = img.mirror_image_get_mode()
-            if mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_JOURNAL and mirror_info['state'] != rbd.RBD_MIRROR_IMAGE_DISABLED:  # noqa E501 #pylint: disable=line-too-long
+            mirror_mode = None
+            if mirror_info['state'] != rbd.RBD_MIRROR_IMAGE_DISABLED:
+                mirror_mode = img.mirror_image_get_mode()
+            else:
+                stat['mirror_mode'] = 'Disabled'
+
+            if mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_JOURNAL:
                 stat['mirror_mode'] = 'journal'
             elif mirror_mode == rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT:
                 stat['mirror_mode'] = 'snapshot'
@@ -315,8 +320,6 @@ def _rbd_image(cls, ioctx, pool_name, namespace, image_name,  # pylint: disable=
                 for scheduled_image in schedule_status['scheduled_images']:
                     if scheduled_image['image'] == get_image_spec(pool_name, namespace, image_name):
                         stat['schedule_info'] = scheduled_image
-            else:
-                stat['mirror_mode'] = 'Disabled'
 
             stat['name'] = image_name
 
@@ -364,10 +367,8 @@ def _rbd_image(cls, ioctx, pool_name, namespace, image_name,  # pylint: disable=
                 if snap['namespace'] == rbd.RBD_SNAP_NAMESPACE_TYPE_TRASH:
                     continue
 
-                try:
-                    snap['mirror_mode'] = MIRROR_IMAGE_MODE(img.mirror_image_get_mode()).name
-                except ValueError as ex:
-                    raise DashboardException(f'Unknown RBD Mirror mode: {ex}')
+                if mirror_mode:
+                    snap['mirror_mode'] = mirror_mode
 
                 snap['timestamp'] = "{}Z".format(
                     img.get_snap_timestamp(snap['id']).isoformat())

src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py

@@ -113,95 +113,101 @@ def handle_open_image(self,
             self.finish(image_spec)
             return
 
-        self.get_mirror_mode(image_spec, image)
+        self.get_mirror_info(image_spec, image)
 
-    def get_mirror_mode(self, image_spec: ImageSpec, image: rbd.Image) -> None:
+    def get_mirror_info(self, image_spec: ImageSpec, image: rbd.Image) -> None:
         pool_id, namespace, image_id = image_spec
 
-        self.log.debug("CreateSnapshotRequests.get_mirror_mode: {}/{}/{}".format(
+        self.log.debug("CreateSnapshotRequests.get_mirror_info: {}/{}/{}".format(
             pool_id, namespace, image_id))
 
-        def cb(comp: rados.Completion, mode: Optional[int]) -> None:
-            self.handle_get_mirror_mode(image_spec, image, comp, mode)
+        def cb(comp: rados.Completion, info: Optional[Dict[str, Union[str, int]]]) -> None:
+            self.handle_get_mirror_info(image_spec, image, comp, info)
 
         try:
-            image.aio_mirror_image_get_mode(cb)
+            image.aio_mirror_image_get_info(cb)
         except Exception as e:
             self.log.error(
-                "exception when getting mirror mode for {}/{}/{}: {}".format(
+                "exception when getting mirror info for {}/{}/{}: {}".format(
                     pool_id, namespace, image_id, e))
             self.close_image(image_spec, image)
 
-    def handle_get_mirror_mode(self,
+    def handle_get_mirror_info(self,
                                image_spec: ImageSpec,
                                image: rbd.Image,
                                comp: rados.Completion,
-                               mode: Optional[int]) -> None:
+                               info: Optional[Dict[str, Union[str, int]]]) -> None:
         pool_id, namespace, image_id = image_spec
 
         self.log.debug(
-            "CreateSnapshotRequests.handle_get_mirror_mode {}/{}/{}: r={} mode={}".format(
-                pool_id, namespace, image_id, comp.get_return_value(), mode))
+            "CreateSnapshotRequests.handle_get_mirror_info {}/{}/{}: r={} info={}".format(
+                pool_id, namespace, image_id, comp.get_return_value(), info))
 
-        if mode is None:
-            if comp.get_return_value() != -errno.ENOENT:
-                self.log.error(
-                    "error when getting mirror mode for {}/{}/{}: {}".format(
-                        pool_id, namespace, image_id, comp.get_return_value()))
+        if info is None:
+            self.log.error(
+                "error when getting mirror info for {}/{}/{}: {}".format(
+                    pool_id, namespace, image_id, comp.get_return_value()))
             self.close_image(image_spec, image)
             return
 
-        if mode != rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT:
+        if info['state'] != rbd.RBD_MIRROR_IMAGE_ENABLED:
             self.log.debug(
-                "CreateSnapshotRequests.handle_get_mirror_mode: {}/{}/{}: {}".format(
+                "CreateSnapshotRequests.handle_get_mirror_info: {}/{}/{}: {}".format(
                     pool_id, namespace, image_id,
-                    "snapshot mirroring is not enabled"))
+                    "mirroring is not enabled"))
             self.close_image(image_spec, image)
             return
 
-        self.get_mirror_info(image_spec, image)
+        if not info['primary']:
+            self.log.debug(
+                "CreateSnapshotRequests.handle_get_mirror_info: {}/{}/{}: {}".format(
+                    pool_id, namespace, image_id,
+                    "is not primary"))
+            self.close_image(image_spec, image)
+            return
 
-    def get_mirror_info(self, image_spec: ImageSpec, image: rbd.Image) -> None:
+        self.get_mirror_mode(image_spec, image)
+
+    def get_mirror_mode(self, image_spec: ImageSpec, image: rbd.Image) -> None:
         pool_id, namespace, image_id = image_spec
 
-        self.log.debug("CreateSnapshotRequests.get_mirror_info: {}/{}/{}".format(
+        self.log.debug("CreateSnapshotRequests.get_mirror_mode: {}/{}/{}".format(
             pool_id, namespace, image_id))
 
-        def cb(comp: rados.Completion, info: Optional[Dict[str, Union[str, int]]]) -> None:
-            self.handle_get_mirror_info(image_spec, image, comp, info)
+        def cb(comp: rados.Completion, mode: Optional[int]) -> None:
+            self.handle_get_mirror_mode(image_spec, image, comp, mode)
 
         try:
-            image.aio_mirror_image_get_info(cb)
+            image.aio_mirror_image_get_mode(cb)
         except Exception as e:
             self.log.error(
-                "exception when getting mirror info for {}/{}/{}: {}".format(
+                "exception when getting mirror mode for {}/{}/{}: {}".format(
                     pool_id, namespace, image_id, e))
             self.close_image(image_spec, image)
 
-    def handle_get_mirror_info(self,
+    def handle_get_mirror_mode(self,
                                image_spec: ImageSpec,
                                image: rbd.Image,
                                comp: rados.Completion,
-                               info: Optional[Dict[str, Union[str, int]]]) -> None:
+                               mode: Optional[int]) -> None:
         pool_id, namespace, image_id = image_spec
 
         self.log.debug(
-            "CreateSnapshotRequests.handle_get_mirror_info {}/{}/{}: r={} info={}".format(
-                pool_id, namespace, image_id, comp.get_return_value(), info))
+            "CreateSnapshotRequests.handle_get_mirror_mode {}/{}/{}: r={} mode={}".format(
+                pool_id, namespace, image_id, comp.get_return_value(), mode))
 
-        if info is None:
-            if comp.get_return_value() != -errno.ENOENT:
-                self.log.error(
-                    "error when getting mirror info for {}/{}/{}: {}".format(
-                        pool_id, namespace, image_id, comp.get_return_value()))
+        if mode is None:
+            self.log.error(
+                "error when getting mirror mode for {}/{}/{}: {}".format(
+                    pool_id, namespace, image_id, comp.get_return_value()))
             self.close_image(image_spec, image)
             return
 
-        if not info['primary']:
+        if mode != rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT:
             self.log.debug(
-                "CreateSnapshotRequests.handle_get_mirror_info: {}/{}/{}: {}".format(
+                "CreateSnapshotRequests.handle_get_mirror_mode: {}/{}/{}: {}".format(
                     pool_id, namespace, image_id,
-                    "is not primary"))
+                    "not enabled for snapshot mirroring"))
             self.close_image(image_spec, image)
             return