mgr/rbd_support: avoid losing a schedule on load vs add race

If load_schedules() (i.e. periodic refresh) races with add_schedule()
invoked by the user for a fresh image, that image's schedule may get
lost until the next rebuild (not refresh!) of the queue:

1. periodic refresh invokes load_schedules()
2. load_schedules() creates a new Schedules instance and loads
   schedules from rbd_mirror_snapshot_schedule object
3. add_schedule() is invoked for a new image (an image that isn't
   present in self.images) by the user
4. before load_schedules() can grab self.lock, add_schedule() commits
   the new schedule to rbd_mirror_snapshot_schedule object and adds it
   to self.schedules
5. load_schedules() grabs self.lock and reassigns self.schedules with
   Schedules instance that is now stale
6. periodic refresh invokes load_pool_images() which discovers the new
   image; eventually it is added to self.images
7. periodic refresh invokes refresh_queue() which attempts to enqueue()
   the new image; this fails because a matching schedule isn't present

The next periodic refresh recovers the discarded schedule from
rbd_mirror_snapshot_schedule object but no attempt to enqueue() that
image is made since it is already "known" at that point.  Despite the
schedule being in place, no snapshots are created until the queue is
rebuilt from scratch or rbd_support module is reloaded.

To fix that, extend self.lock critical sections so that add_schedule()
and remove_schedule() can't get stepped on by load_schedules().

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

Conflicts:
	src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py [ commit
	  e4a16e261370 ("mgr/rbd_support: add type annotation") not in
	  pacific ]
	src/pybind/mgr/rbd_support/trash_purge_schedule.py [ ditto ]

diff --git a/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py b/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py
index a5516da66c09e992fcad4349b7c0aaceb57d3054..eb10d0710b0f3f02963672c091cd3254e9bfae7f 100644 (file)
--- a/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py
+++ b/src/pybind/mgr/rbd_support/mirror_snapshot_schedule.py
@@ -473,8 +473,7 @@ class MirrorSnapshotScheduleHandler:
 
         schedules = Schedules(self)
         schedules.load(namespace_validator, image_validator)
-        with self.lock:
-            self.schedules = schedules
+        self.schedules = schedules
 
     def refresh_images(self):
         elapsed = (datetime.now() - self.last_refresh_images).total_seconds()
@@ -483,9 +482,8 @@ class MirrorSnapshotScheduleHandler:
 
         self.log.debug("MirrorSnapshotScheduleHandler: refresh_images")
 
-        self.load_schedules()
-
         with self.lock:
+            self.load_schedules()
             if not self.schedules:
                 self.log.debug("MirrorSnapshotScheduleHandler: no schedules")
                 self.watchers.unregister_all()
@@ -565,25 +563,24 @@ class MirrorSnapshotScheduleHandler:
                     pool_name, e))
 
     def rebuild_queue(self):
-        with self.lock:
-            now = datetime.now()
+        now = datetime.now()
 
-            # don't remove from queue "due" images
-            now_string = datetime.strftime(now, "%Y-%m-%d %H:%M:00")
+        # don't remove from queue "due" images
+        now_string = datetime.strftime(now, "%Y-%m-%d %H:%M:00")
 
-            for schedule_time in list(self.queue):
-                if schedule_time > now_string:
-                    del self.queue[schedule_time]
+        for schedule_time in list(self.queue):
+            if schedule_time > now_string:
+                del self.queue[schedule_time]
 
-            if not self.schedules:
-                return
+        if not self.schedules:
+            return
 
-            for pool_id in self.images:
-                for namespace in self.images[pool_id]:
-                    for image_id in self.images[pool_id][namespace]:
-                        self.enqueue(now, pool_id, namespace, image_id)
+        for pool_id in self.images:
+            for namespace in self.images[pool_id]:
+                for image_id in self.images[pool_id][namespace]:
+                    self.enqueue(now, pool_id, namespace, image_id)
 
-            self.condition.notify()
+        self.condition.notify()
 
     def refresh_queue(self, current_images):
         now = datetime.now()
@@ -661,11 +658,10 @@ class MirrorSnapshotScheduleHandler:
             "MirrorSnapshotScheduleHandler: add_schedule: level_spec={}, interval={}, start_time={}".format(
                 level_spec.name, interval, start_time))
 
+        # TODO: optimize to rebuild only affected part of the queue
         with self.lock:
             self.schedules.add(level_spec, interval, start_time)
-
-        # TODO: optimize to rebuild only affected part of the queue
-        self.rebuild_queue()
+            self.rebuild_queue()
         return 0, "", ""
 
     def remove_schedule(self, level_spec, interval, start_time):
@@ -673,11 +669,10 @@ class MirrorSnapshotScheduleHandler:
             "MirrorSnapshotScheduleHandler: remove_schedule: level_spec={}, interval={}, start_time={}".format(
                 level_spec.name, interval, start_time))
 
+        # TODO: optimize to rebuild only affected part of the queue
         with self.lock:
             self.schedules.remove(level_spec, interval, start_time)
-
-        # TODO: optimize to rebuild only affected part of the queue
-        self.rebuild_queue()
+            self.rebuild_queue()
         return 0, "", ""
 
     def list(self, level_spec):

diff --git a/src/pybind/mgr/rbd_support/trash_purge_schedule.py b/src/pybind/mgr/rbd_support/trash_purge_schedule.py
index 08f8ab3af120daa368844153e466c9424666193b..bf5d8ae70815a5700c33a46a26f9876f7669721f 100644 (file)
--- a/src/pybind/mgr/rbd_support/trash_purge_schedule.py
+++ b/src/pybind/mgr/rbd_support/trash_purge_schedule.py
@@ -71,8 +71,7 @@ class TrashPurgeScheduleHandler:
 
         schedules = Schedules(self)
         schedules.load()
-        with self.lock:
-            self.schedules = schedules
+        self.schedules = schedules
 
     def refresh_pools(self):
         elapsed = (datetime.now() - self.last_refresh_pools).total_seconds()
@@ -81,9 +80,8 @@ class TrashPurgeScheduleHandler:
 
         self.log.debug("TrashPurgeScheduleHandler: refresh_pools")
 
-        self.load_schedules()
-
         with self.lock:
+            self.load_schedules()
             if not self.schedules:
                 self.log.debug("TrashPurgeScheduleHandler: no schedules")
                 self.pools = {}
@@ -127,24 +125,23 @@ class TrashPurgeScheduleHandler:
             pools[pool_id][namespace] = pool_name
 
     def rebuild_queue(self):
-        with self.lock:
-            now = datetime.now()
+        now = datetime.now()
 
-            # don't remove from queue "due" images
-            now_string = datetime.strftime(now, "%Y-%m-%d %H:%M:00")
+        # don't remove from queue "due" images
+        now_string = datetime.strftime(now, "%Y-%m-%d %H:%M:00")
 
-            for schedule_time in list(self.queue):
-                if schedule_time > now_string:
-                    del self.queue[schedule_time]
+        for schedule_time in list(self.queue):
+            if schedule_time > now_string:
+                del self.queue[schedule_time]
 
-            if not self.schedules:
-                return
+        if not self.schedules:
+            return
 
-            for pool_id, namespaces in self.pools.items():
-                for namespace in namespaces:
-                    self.enqueue(now, pool_id, namespace)
+        for pool_id, namespaces in self.pools.items():
+            for namespace in namespaces:
+                self.enqueue(now, pool_id, namespace)
 
-            self.condition.notify()
+        self.condition.notify()
 
     def refresh_queue(self, current_pools):
         now = datetime.now()
@@ -218,11 +215,10 @@ class TrashPurgeScheduleHandler:
             "TrashPurgeScheduleHandler: add_schedule: level_spec={}, interval={}, start_time={}".format(
                 level_spec.name, interval, start_time))
 
+        # TODO: optimize to rebuild only affected part of the queue
         with self.lock:
             self.schedules.add(level_spec, interval, start_time)
-
-        # TODO: optimize to rebuild only affected part of the queue
-        self.rebuild_queue()
+            self.rebuild_queue()
         return 0, "", ""
 
     def remove_schedule(self, level_spec, interval, start_time):
@@ -230,11 +226,10 @@ class TrashPurgeScheduleHandler:
             "TrashPurgeScheduleHandler: remove_schedule: level_spec={}, interval={}, start_time={}".format(
                 level_spec.name, interval, start_time))
 
+        # TODO: optimize to rebuild only affected part of the queue
         with self.lock:
             self.schedules.remove(level_spec, interval, start_time)
-
-        # TODO: optimize to rebuild only affected part of the queue
-        self.rebuild_queue()
+            self.rebuild_queue()
         return 0, "", ""
 
     def list(self, level_spec):