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 <idryomov@gmail.com>
(cherry picked from commit bcd676356231c97c8275e6c03becc2c58497f20d)

diff --git a/qa/workunits/rbd/cli_generic.sh b/qa/workunits/rbd/cli_generic.sh
index df03e84462fff414b375aa74e1034d60319b27ef..e4a7d1a1e02d35f3908cd93d946deae5c8011c6f 100755 (executable)
--- a/qa/workunits/rbd/cli_generic.sh
+++ b/qa/workunits/rbd/cli_generic.sh
@@ -1163,6 +1163,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'
@@ -1261,9 +1267,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
@@ -1338,6 +1348,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'
@@ -1403,9 +1419,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 c6ce99182afd474b8aa1836d36ede1432420d105..4968700ad82aec3dc13684781381f57129823dd7 100644 (file)
--- 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)