From: Ilya Dryomov Date: Tue, 25 Mar 2025 08:13:27 +0000 (+0100) Subject: mgr/rbd_support: always parse interval and start_time in Schedules::remove() X-Git-Tag: v20.3.0~259^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=bcd676356231c97c8275e6c03becc2c58497f20d;p=ceph.git mgr/rbd_support: always parse interval and start_time in Schedules::remove() Commit 1b62447071a9 ("mgr/rbd_support: fix schedule remove") addressed the issue that it was concerned with in a rather suboptimal way: instead of moving the parsing of interval and start_time upfront to be able to bail early, it wrapped from_string() constructors with try/finally and left the conditional behavior in place. Fixes: https://tracker.ceph.com/issues/70640 Signed-off-by: Ilya Dryomov --- diff --git a/qa/workunits/rbd/cli_generic.sh b/qa/workunits/rbd/cli_generic.sh index 0ceb9ff54cf31..bd2a50d7c4915 100755 --- a/qa/workunits/rbd/cli_generic.sh +++ b/qa/workunits/rbd/cli_generic.sh @@ -1161,6 +1161,12 @@ test_trash_purge_schedule() { expect_fail rbd trash purge schedule ls test "$(rbd trash purge schedule ls -R --format json)" = "[]" + # check that remove fails when pool exists but no schedules are in place + expect_fail rbd trash purge schedule remove dummy + expect_fail rbd trash purge schedule remove 1d dummy + expect_fail rbd trash purge schedule remove -p rbd dummy + expect_fail rbd trash purge schedule remove -p rbd 1d dummy + rbd trash purge schedule add -p rbd 1d 01:30 rbd trash purge schedule ls -p rbd | grep 'every 1d starting at 01:30' @@ -1259,9 +1265,13 @@ test_trash_purge_schedule() { # Negative tests rbd trash purge schedule add 2m expect_fail rbd trash purge schedule add -p rbd dummy + expect_fail rbd trash purge schedule add -p rbd 1d dummy expect_fail rbd trash purge schedule add dummy + expect_fail rbd trash purge schedule add 1d dummy expect_fail rbd trash purge schedule remove -p rbd dummy + expect_fail rbd trash purge schedule remove -p rbd 1d dummy expect_fail rbd trash purge schedule remove dummy + expect_fail rbd trash purge schedule remove 1d dummy rbd trash purge schedule ls -p rbd | grep 'every 1d starting at 01:30' rbd trash purge schedule ls | grep 'every 2m' rbd trash purge schedule remove -p rbd 1d 01:30 @@ -1336,6 +1346,12 @@ test_mirror_snapshot_schedule() { test "$(rbd mirror image status rbd2/ns1/test1 | grep -c mirror.primary)" = '1' + # check that remove fails when image exists but no schedules are in place + expect_fail rbd mirror snapshot schedule remove dummy + expect_fail rbd mirror snapshot schedule remove 1h dummy + expect_fail rbd mirror snapshot schedule remove -p rbd2/ns1 --image test1 dummy + expect_fail rbd mirror snapshot schedule remove -p rbd2/ns1 --image test1 1h dummy + rbd mirror snapshot schedule add -p rbd2/ns1 --image test1 1m expect_fail rbd mirror snapshot schedule ls rbd mirror snapshot schedule ls -R | grep 'rbd2 *ns1 *test1 *every 1m' @@ -1401,9 +1417,13 @@ test_mirror_snapshot_schedule() { # Negative tests expect_fail rbd mirror snapshot schedule add dummy + expect_fail rbd mirror snapshot schedule add 1h dummy expect_fail rbd mirror snapshot schedule add -p rbd2/ns1 --image test1 dummy + expect_fail rbd mirror snapshot schedule add -p rbd2/ns1 --image test1 1h dummy expect_fail rbd mirror snapshot schedule remove dummy + expect_fail rbd mirror snapshot schedule remove 1h dummy expect_fail rbd mirror snapshot schedule remove -p rbd2/ns1 --image test1 dummy + expect_fail rbd mirror snapshot schedule remove -p rbd2/ns1 --image test1 1h dummy test "$(rbd mirror snapshot schedule ls)" = 'every 1h starting at 00:15:00' test "$(rbd mirror snapshot schedule ls -p rbd2/ns1 --image test1)" = 'every 1m' diff --git a/src/pybind/mgr/rbd_support/schedule.py b/src/pybind/mgr/rbd_support/schedule.py index c6ce99182afd4..4968700ad82ae 100644 --- a/src/pybind/mgr/rbd_support/schedule.py +++ b/src/pybind/mgr/rbd_support/schedule.py @@ -510,19 +510,21 @@ class Schedules: def remove(self, level_spec: LevelSpec, - interval: Optional[str], - start_time: Optional[str]) -> None: + interval_str: Optional[str], + start_time_str: Optional[str]) -> None: + # from_string() may raise, so call it before popping the schedule (and + # unconditionally to ensure that invalid interval or start time always + # leads to an error) + interval = Interval.from_string(interval_str) if interval_str else None + start_time = StartTime.from_string(start_time_str) schedule = self.schedules.pop(level_spec.id, None) if schedule: if interval is None: schedule = None else: - try: - schedule.remove(Interval.from_string(interval), - StartTime.from_string(start_time)) - finally: - if schedule: - self.schedules[level_spec.id] = schedule + schedule.remove(interval, start_time) + if schedule: + self.schedules[level_spec.id] = schedule if not schedule: del self.level_specs[level_spec.id] self.save(level_spec, schedule)