Merge pull request ceph#62484 from idryomov/wip-70640

mgr/rbd_support: always parse interval and start_time in Schedules::remove()

Reviewed-by: Ramana Raja <[email protected]>

Author: idryomov

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'
 

src/pybind/mgr/rbd_support/schedule.py

@@ -510,19 +510,21 @@ def add(self,
 
     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)