librbd: disallow group snap rollback if memberships don't match

Before proceeding with group rollback, ensure that the set of images
that took part in the group snapshot matches the set of images that are
currently part of the group.  Otherwise, because we preserve affected
snapshots when an image is removed from the group, data loss can ensue
where an image gets rolled back while part of another group or not part
of any group but long repurposed for something else.

Similarly, ensure that the group snapshot is complete.

Fixes: https://tracker.ceph.com/issues/66300
Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
(cherry picked from commit f212a9ca5b9238b80ae5c728a7bf99366ed910a1)

diff --git a/src/cls/rbd/cls_rbd_types.h b/src/cls/rbd/cls_rbd_types.h
index c8d2cb871e4439f2fcae584e36803ca6d721024f..c1d64805ae429984fcac1e8fe6cd07635b5aa3ac 100644 (file)
--- a/src/cls/rbd/cls_rbd_types.h
+++ b/src/cls/rbd/cls_rbd_types.h
@@ -374,6 +374,7 @@ struct GroupImageSpec {
 
   std::string image_key();
 
+  bool operator==(const GroupImageSpec&) const = default;
 };
 WRITE_CLASS_ENCODER(GroupImageSpec);
 

diff --git a/src/librbd/api/Group.cc b/src/librbd/api/Group.cc
index 06d38fe85007354afb67bc8f0ce12b321e400755..f7aebf0e6b3eab35b58b58f1b19a0c182594eeff 100644 (file)
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -1263,6 +1263,35 @@ int Group<I>::snap_rollback(librados::IoCtx& group_ioctx,
     return -ENOENT;
   }
 
+  if (group_snap->state != cls::rbd::GROUP_SNAPSHOT_STATE_COMPLETE) {
+    lderr(cct) << "group snapshot is not complete" << dendl;
+    return -EINVAL;
+  }
+
+  std::vector<cls::rbd::GroupImageSpec> rollback_images;
+  for (const auto& snap : group_snap->snaps) {
+    rollback_images.emplace_back(snap.image_id, snap.pool);
+  }
+
+  std::vector<cls::rbd::GroupImageStatus> images;
+  r = group_image_list(group_ioctx, group_name, &images);
+  if (r < 0) {
+    return r;
+  }
+
+  std::vector<cls::rbd::GroupImageSpec> current_images;
+  for (const auto& image : images) {
+    if (image.state == cls::rbd::GROUP_IMAGE_LINK_STATE_ATTACHED) {
+      current_images.push_back(image.spec);
+    }
+  }
+
+  if (rollback_images != current_images) {
+    lderr(cct) << "group snapshot membership does not match group membership"
+               << dendl;
+    return -EINVAL;
+  }
+
   string group_header_oid = util::group_header_name(group_id);
   r = group_snap_rollback_by_record(group_ioctx, *group_snap, group_id,
                                     group_header_oid, pctx);

diff --git a/src/test/pybind/test_rbd.py b/src/test/pybind/test_rbd.py
index e5618fd66f5945f884cb19592fa69bcf75fa2510..16f2b1a776fb66f66309efa6363d3843010334c2 100644 (file)
--- a/src/test/pybind/test_rbd.py
+++ b/src/test/pybind/test_rbd.py
@@ -3052,33 +3052,187 @@ class TestGroups(object):
         self.rbd.remove(ioctx, clone_name)
 
     def test_group_snap_rollback(self):
-        eq([], list(self.group.list_images()))
-        self.group.add_image(ioctx, image_name)
-        with Image(ioctx, image_name) as image:
-            image.write(b'\0' * 256, 0)
+        for _ in range(1, 3):
+            create_image()
+            self.image_names.append(image_name)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            image.write(b'1' * 256, 0)
+        with Image(ioctx, self.image_names[1]) as image:
+            image.write(b'2' * 256, 0)
+        with Image(ioctx, self.image_names[2]) as image:
+            image.write(b'3' * 256, 0)
+        self.group.add_image(ioctx, self.image_names[0])
+        snap_name1 = get_temp_snap_name()
+        self.group.create_snap(snap_name1)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            image.write(b'4' * 256, 0)
+        with Image(ioctx, self.image_names[1]) as image:
+            image.write(b'5' * 256, 0)
+        with Image(ioctx, self.image_names[2]) as image:
+            image.write(b'6' * 256, 0)
+        self.group.add_image(ioctx, self.image_names[1])
+        snap_name2 = get_temp_snap_name()
+        self.group.create_snap(snap_name2)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            image.write(b'7' * 256, 0)
+        with Image(ioctx, self.image_names[1]) as image:
+            image.write(b'8' * 256, 0)
+        with Image(ioctx, self.image_names[2]) as image:
+            image.write(b'9' * 256, 0)
+        self.group.add_image(ioctx, self.image_names[2])
+        snap_name3 = get_temp_snap_name()
+        self.group.create_snap(snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            image.write(b'a' * 256, 0)
+        with Image(ioctx, self.image_names[1]) as image:
+            image.write(b'b' * 256, 0)
+        with Image(ioctx, self.image_names[2]) as image:
+            image.write(b'c' * 256, 0)
+
+        for i in range(0, 3):
+            self.group.remove_image(ioctx, self.image_names[i])
+        with Image(ioctx, self.image_names[0]) as image:
+            image_snaps = list(image.list_snaps())
+            assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP,
+                                                             RBD_SNAP_NAMESPACE_TYPE_GROUP,
+                                                             RBD_SNAP_NAMESPACE_TYPE_GROUP]
+        with Image(ioctx, self.image_names[1]) as image:
+            image_snaps = list(image.list_snaps())
+            assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP,
+                                                             RBD_SNAP_NAMESPACE_TYPE_GROUP]
+        with Image(ioctx, self.image_names[2]) as image:
+            image_snaps = list(image.list_snaps())
+            assert [s['namespace'] for s in image_snaps] == [RBD_SNAP_NAMESPACE_TYPE_GROUP]
+
+        # group = []
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            read = image.read(0, 256)
+            assert read == b'a' * 256
+        with Image(ioctx, self.image_names[1]) as image:
             read = image.read(0, 256)
-            eq(read, b'\0' * 256)
+            assert read == b'b' * 256
+        with Image(ioctx, self.image_names[2]) as image:
+            read = image.read(0, 256)
+            assert read == b'c' * 256
 
-        global snap_name
-        eq([], list(self.group.list_snaps()))
-        self.group.create_snap(snap_name)
-        eq([snap_name], [snap['name'] for snap in self.group.list_snaps()])
+        # group = [img0]
+        self.group.add_image(ioctx, self.image_names[0])
+        self.group.rollback_to_snap(snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
 
-        with Image(ioctx, image_name) as image:
-            data = rand_data(256)
-            image.write(data, 0)
+        with Image(ioctx, self.image_names[0]) as image:
+            read = image.read(0, 256)
+            assert read == b'1' * 256
+        with Image(ioctx, self.image_names[1]) as image:
+            read = image.read(0, 256)
+            assert read == b'b' * 256
+        with Image(ioctx, self.image_names[2]) as image:
+            read = image.read(0, 256)
+            assert read == b'c' * 256
+
+        # group = [img1]
+        self.group.remove_image(ioctx, self.image_names[0])
+        self.group.add_image(ioctx, self.image_names[1])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        # group = [img2]
+        self.group.remove_image(ioctx, self.image_names[1])
+        self.group.add_image(ioctx, self.image_names[2])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        # group = [img0 img1]
+        self.group.remove_image(ioctx, self.image_names[2])
+        # re-add in reverse order to test that order doesn't matter
+        self.group.add_image(ioctx, self.image_names[1])
+        self.group.add_image(ioctx, self.image_names[0])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        self.group.rollback_to_snap(snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
             read = image.read(0, 256)
-            eq(read, data)
+            assert read == b'4' * 256
+        with Image(ioctx, self.image_names[1]) as image:
+            read = image.read(0, 256)
+            assert read == b'5' * 256
+        with Image(ioctx, self.image_names[2]) as image:
+            read = image.read(0, 256)
+            assert read == b'c' * 256
+
+        # group = [img0 img2]
+        self.group.remove_image(ioctx, self.image_names[1])
+        self.group.add_image(ioctx, self.image_names[2])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        # group = [img1 img2]
+        self.group.remove_image(ioctx, self.image_names[0])
+        self.group.add_image(ioctx, self.image_names[1])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        # group = [img0 img1 img2]
+        self.group.add_image(ioctx, self.image_names[0])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        self.group.rollback_to_snap(snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            read = image.read(0, 256)
+            assert read == b'7' * 256
+        with Image(ioctx, self.image_names[1]) as image:
+            read = image.read(0, 256)
+            assert read == b'8' * 256
+        with Image(ioctx, self.image_names[2]) as image:
+            read = image.read(0, 256)
+            assert read == b'9' * 256
 
-        self.group.rollback_to_snap(snap_name)
-        with Image(ioctx, image_name) as image:
+        # group = [img0 img1]
+        self.group.remove_image(ioctx, self.image_names[2])
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name1)
+        self.group.rollback_to_snap(snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            read = image.read(0, 256)
+            assert read == b'4' * 256
+        with Image(ioctx, self.image_names[1]) as image:
+            read = image.read(0, 256)
+            assert read == b'5' * 256
+        with Image(ioctx, self.image_names[2]) as image:
             read = image.read(0, 256)
-            eq(read, b'\0' * 256)
+            assert read == b'9' * 256
 
-        self.group.remove_image(ioctx, image_name)
-        eq([], list(self.group.list_images()))
-        self.group.remove_snap(snap_name)
-        eq([], list(self.group.list_snaps()))
+        # group = [img0]
+        self.group.remove_image(ioctx, self.image_names[1])
+        self.group.rollback_to_snap(snap_name1)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name2)
+        assert_raises(InvalidArgument, self.group.rollback_to_snap, snap_name3)
+
+        with Image(ioctx, self.image_names[0]) as image:
+            read = image.read(0, 256)
+            assert read == b'1' * 256
+        with Image(ioctx, self.image_names[1]) as image:
+            read = image.read(0, 256)
+            assert read == b'5' * 256
+        with Image(ioctx, self.image_names[2]) as image:
+            read = image.read(0, 256)
+            assert read == b'9' * 256
 
 class TestMigration(object):