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)

Conflicts:
	src/test/pybind/test_rbd.py [ commit d7fd66ec9944 ("librbd: add
	  rbd_clone4() API to take parent snapshot by ID") not in reef ]

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 0040d1e67e5b82e00f7aacbe011634e02c0105bd..77c1088f0385308a96e046408db74ca5d81d0213 100644 (file)
--- a/src/test/pybind/test_rbd.py
+++ b/src/test/pybind/test_rbd.py
@@ -33,6 +33,7 @@ from rbd import (RBD, Group, Image, ImageNotFound, InvalidArgument, ImageExists,
                  RBD_MIRROR_IMAGE_MODE_JOURNAL, RBD_MIRROR_IMAGE_MODE_SNAPSHOT,
                  RBD_LOCK_MODE_EXCLUSIVE, RBD_OPERATION_FEATURE_GROUP,
                  RBD_OPERATION_FEATURE_CLONE_CHILD,
+                 RBD_SNAP_NAMESPACE_TYPE_GROUP,
                  RBD_SNAP_NAMESPACE_TYPE_TRASH,
                  RBD_SNAP_NAMESPACE_TYPE_MIRROR,
                  RBD_IMAGE_MIGRATION_STATE_PREPARED, RBD_CONFIG_SOURCE_CONFIG,
@@ -2805,7 +2806,7 @@ class TestGroups(object):
         eq([snap_name], [snap['name'] for snap in self.group.list_snaps()])
 
         for snap in self.image.list_snaps():
-            eq(rbd.RBD_SNAP_NAMESPACE_TYPE_GROUP, snap['namespace'])
+            eq(RBD_SNAP_NAMESPACE_TYPE_GROUP, snap['namespace'])
             info = snap['group']
             eq(group_name, info['group_name'])
             eq(snap_name, info['group_snap_name'])
@@ -2871,33 +2872,187 @@ class TestGroups(object):
         eq([], list(self.group.list_snaps()))
 
     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):