From 985cae644712c8f26ad45ed131f923fe05e0cb1b Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Fri, 14 Jun 2024 14:04:39 +0200 Subject: [PATCH] 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 (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 ] --- src/cls/rbd/cls_rbd_types.h | 1 + src/librbd/api/Group.cc | 29 ++++++ src/test/pybind/test_rbd.py | 197 ++++++++++++++++++++++++++++++++---- 3 files changed, 206 insertions(+), 21 deletions(-) diff --git a/src/cls/rbd/cls_rbd_types.h b/src/cls/rbd/cls_rbd_types.h index c8d2cb871e443..c1d64805ae429 100644 --- 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 06d38fe850073..f7aebf0e6b3ea 100644 --- a/src/librbd/api/Group.cc +++ b/src/librbd/api/Group.cc @@ -1263,6 +1263,35 @@ int Group::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 rollback_images; + for (const auto& snap : group_snap->snaps) { + rollback_images.emplace_back(snap.image_id, snap.pool); + } + + std::vector images; + r = group_image_list(group_ioctx, group_name, &images); + if (r < 0) { + return r; + } + + std::vector 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 0040d1e67e5b8..77c1088f03853 100644 --- 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): -- 2.39.5