From 0a2cc1e163fea2d6c1bb020e112eef5a7044b347 Mon Sep 17 00:00:00 2001 From: Ilya Dryomov Date: Sun, 19 Jun 2022 12:12:01 +0200 Subject: [PATCH] mgr/rbd_support: always rescan image mirror snapshots on refresh Establishing a watch on rbd_mirroring object and skipping rescanning image mirror snapshots on periodic refresh unless rbd_mirroring object gets notified in the interim is flawed. rbd_mirroring object is notified when mirroring is enabled or disabled on some image (including when the image is removed), but it is not notified when images are promoted or demoted. However, load_pool_images() discards images that are not primary at the time of the scan. If the image is promoted later, no snapshots are created even if the schedule is in place. This happens regardless of whether the schedule is added before or after the promotion. This effectively reverts commit 69259c8d3722 ("mgr/rbd_support: make mirror_snapshot_schedule rescan only updated pools"). An alternative fix could be to stop discarding non-primary images (i.e. drop if not info['primary']: continue check added in commit d39eb283c5ce ("mgr/rbd_support: mirror snapshot schedule should skip non-primary images")), but that would clutter the queue and therefore "rbd mirror snapshot schedule status" output with bogus entries. Performing a rescan roughly every 60 seconds should be manageable: currently it amounts to a single mirror_image_status_list request, followed by mirror_image_get, get_snapcontext and snapshot_get requests for each snapshot-based mirroring enabled image and concluded by a single dir_list request. Among these, per-image get_snapcontext and snapshot_get requests are necessary for determining primaryness. Fixes: https://tracker.ceph.com/issues/53914 Signed-off-by: Ilya Dryomov (cherry picked from commit 7fb4fdbed0b908a2105ac44deca48f2170e46fe5) Conflicts: src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py [ commit e4a16e261370 ("mgr/rbd_support: add type annotation") not in octopus ] --- qa/workunits/rbd/cli_generic.sh | 16 +- .../rbd_support/mirror_snapshot_schedule.py | 138 +----------------- 2 files changed, 17 insertions(+), 137 deletions(-) diff --git a/qa/workunits/rbd/cli_generic.sh b/qa/workunits/rbd/cli_generic.sh index 32c45790fa603..a1723509586e2 100755 --- a/qa/workunits/rbd/cli_generic.sh +++ b/qa/workunits/rbd/cli_generic.sh @@ -1304,6 +1304,20 @@ test_mirror_snapshot_schedule() { test "$(rbd mirror snapshot schedule status -p rbd2/ns1 --image test1 --format xml | $XMLSTARLET sel -t -v '//scheduled_images/image/image')" = 'rbd2/ns1/test1' + rbd mirror image demote rbd2/ns1/test1 + for i in `seq 12`; do + rbd mirror snapshot schedule status | grep 'rbd2/ns1/test1' || break + sleep 10 + done + rbd mirror snapshot schedule status | expect_fail grep 'rbd2/ns1/test1' + + rbd mirror image promote rbd2/ns1/test1 + for i in `seq 12`; do + rbd mirror snapshot schedule status | grep 'rbd2/ns1/test1' && break + sleep 10 + done + rbd mirror snapshot schedule status | grep 'rbd2/ns1/test1' + rbd mirror snapshot schedule add 1h 00:15 test "$(rbd mirror snapshot schedule ls)" = 'every 1h starting at 00:15:00' rbd mirror snapshot schedule ls -R | grep 'every 1h starting at 00:15:00' @@ -1325,11 +1339,11 @@ test_mirror_snapshot_schedule() { test "$(rbd mirror snapshot schedule ls -p rbd2/ns1 --image test1)" = 'every 1m' rbd rm rbd2/ns1/test1 - for i in `seq 12`; do rbd mirror snapshot schedule status | grep 'rbd2/ns1/test1' || break sleep 10 done + rbd mirror snapshot schedule status | expect_fail grep 'rbd2/ns1/test1' rbd mirror snapshot schedule remove test "$(rbd mirror snapshot schedule ls -R --format json)" = "[]" diff --git a/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py b/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py index eb10d0710b0f3..536ee3d16c999 100644 --- a/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py +++ b/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py @@ -11,8 +11,6 @@ from threading import Condition, Lock, Thread from .common import get_rbd_pools from .schedule import LevelSpec, Interval, StartTime, Schedule, Schedules -MIRRORING_OID = "rbd_mirroring" - def namespace_validator(ioctx): mode = rbd.RBD().mirror_mode_get(ioctx) if mode != rbd.RBD_MIRROR_MODE_IMAGE: @@ -24,125 +22,6 @@ def image_validator(image): if mode != rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT: raise rbd.InvalidArgument("Invalid mirror image mode") -class Watchers: - - lock = Lock() - - def __init__(self, handler): - self.rados = handler.module.rados - self.log = handler.log - self.watchers = {} - self.updated = {} - self.error = {} - self.epoch = {} - - def __del__(self): - self.unregister_all() - - def _clean_watcher(self, pool_id, namespace, watch_id): - assert self.lock.locked() - - del self.watchers[pool_id, namespace] - self.updated.pop(watch_id, None) - self.error.pop(watch_id, None) - self.epoch.pop(watch_id, None) - - def check(self, pool_id, namespace, epoch): - error = None - with self.lock: - watch = self.watchers.get((pool_id, namespace)) - if watch is not None: - error = self.error.get(watch.get_id()) - if not error: - updated = self.updated[watch.get_id()] - self.updated[watch.get_id()] = False - self.epoch[watch.get_id()] = epoch - return updated - if error: - self.unregister(pool_id, namespace) - - if self.register(pool_id, namespace): - return self.check(pool_id, namespace, epoch) - else: - return True - - def register(self, pool_id, namespace): - - def callback(notify_id, notifier_id, watch_id, data): - self.log.debug("watcher {}: got notify {} from {}".format( - watch_id, notify_id, notifier_id)) - - with self.lock: - self.updated[watch_id] = True - - def error_callback(watch_id, error): - self.log.debug("watcher {}: got errror {}".format( - watch_id, error)) - - with self.lock: - self.error[watch_id] = error - - try: - ioctx = self.rados.open_ioctx2(int(pool_id)) - ioctx.set_namespace(namespace) - watch = ioctx.watch(MIRRORING_OID, callback, error_callback) - except rados.ObjectNotFound: - self.log.debug( - "{}/{}/{} watcher not registered: object not found".format( - pool_id, namespace, MIRRORING_OID)) - return False - - self.log.debug("{}/{}/{} watcher {} registered".format( - pool_id, namespace, MIRRORING_OID, watch.get_id())) - - with self.lock: - self.watchers[pool_id, namespace] = watch - self.updated[watch.get_id()] = True - return True - - def unregister(self, pool_id, namespace): - - with self.lock: - watch = self.watchers[pool_id, namespace] - - watch_id = watch.get_id() - - try: - watch.close() - - self.log.debug("{}/{}/{} watcher {} unregistered".format( - pool_id, namespace, MIRRORING_OID, watch_id)) - - except rados.Error as e: - self.log.debug( - "exception when unregistering {}/{} watcher: {}".format( - pool_id, namespace, e)) - - with self.lock: - self._clean_watcher(pool_id, namespace, watch_id) - - def unregister_all(self): - with self.lock: - watchers = list(self.watchers) - - for pool_id, namespace in watchers: - self.unregister(pool_id, namespace) - - def unregister_stale(self, current_epoch): - with self.lock: - watchers = list(self.watchers) - - for pool_id, namespace in watchers: - with self.lock: - watch = self.watchers[pool_id, namespace] - if self.epoch.get(watch.get_id()) == current_epoch: - continue - - self.log.debug("{}/{}/{} watcher {} stale".format( - pool_id, namespace, MIRRORING_OID, watch_id)) - - self.unregister(pool_id, namespace) - class CreateSnapshotRequests: @@ -439,7 +318,6 @@ class MirrorSnapshotScheduleHandler: self.thread.start() def _cleanup(self): - self.watchers.unregister_all() self.create_snapshot_requests.wait_for_pending() def run(self): @@ -464,7 +342,6 @@ class MirrorSnapshotScheduleHandler: def init_schedule_queue(self): self.queue = {} self.images = {} - self.watchers = Watchers(self) self.refresh_images() self.log.debug("MirrorSnapshotScheduleHandler: queue is initialized") @@ -486,13 +363,11 @@ class MirrorSnapshotScheduleHandler: self.load_schedules() if not self.schedules: self.log.debug("MirrorSnapshotScheduleHandler: no schedules") - self.watchers.unregister_all() self.images = {} self.queue = {} self.last_refresh_images = datetime.now() return self.REFRESH_DELAY_SECONDS - epoch = int(datetime.now().strftime('%s')) images = {} for pool_id, pool_name in get_rbd_pools(self.module).items(): @@ -500,17 +375,16 @@ class MirrorSnapshotScheduleHandler: LevelSpec.from_pool_spec(pool_id, pool_name)): continue with self.module.rados.open_ioctx2(int(pool_id)) as ioctx: - self.load_pool_images(ioctx, epoch, images) + self.load_pool_images(ioctx, images) with self.lock: self.refresh_queue(images) self.images = images - self.watchers.unregister_stale(epoch) self.last_refresh_images = datetime.now() return self.REFRESH_DELAY_SECONDS - def load_pool_images(self, ioctx, epoch, images): + def load_pool_images(self, ioctx, images): pool_id = str(ioctx.get_pool_id()) pool_name = ioctx.get_pool_name() images[pool_id] = {} @@ -527,14 +401,6 @@ class MirrorSnapshotScheduleHandler: pool_name, namespace)) images[pool_id][namespace] = {} ioctx.set_namespace(namespace) - updated = self.watchers.check(pool_id, namespace, epoch) - if not updated: - self.log.debug("load_pool_images: {}/{} not updated".format( - pool_name, namespace)) - with self.lock: - images[pool_id][namespace] = \ - self.images[pool_id][namespace] - continue mirror_images = dict(rbd.RBD().mirror_image_info_list( ioctx, rbd.RBD_MIRROR_IMAGE_MODE_SNAPSHOT)) if not mirror_images: -- 2.39.5