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>

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 afa5b0aff032b495a3dc1edfc84ae0ea50d8ce0a..942426c95680a298f9ee03622434504b4928f8ec 100644 (file)
--- a/src/librbd/api/Group.cc
+++ b/src/librbd/api/Group.cc
@@ -1281,6 +1281,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 5f52888f8eca0af3ff6775efd04c5f819dfe0787..45aab84d745b3e3ab78a0c9314fb333a5962cd45 100644 (file)
--- a/src/test/pybind/test_rbd.py
+++ b/src/test/pybind/test_rbd.py
@@ -3057,33 +3057,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):