Apply review comments

Signed-off-by: Antoine Bartuccio <antoine.bartuccio@vates.tech>

Author: klmp200

lib/vm.py

@@ -549,252 +549,6 @@ def test_snapshot_on_running_vm(self) -> None:
         finally:
             snapshot.destroy(verify=True)
 
-    def test_vdi_revert(self) -> None:
-        # Successives reverts to ensure the
-        # snapshot tree stays consistent
-        # Created snapshot Tree
-        # snap 1 -> snap 2
-        #   |-----> snap 3
-        # Revert order: 1 -> 2 -> 3 -> 1 -> 1
-        vm = self.clone()
-        snapshots = []
-        if not vm.is_running():
-            vm.start()
-        vm.wait_for_vm_running_and_ssh_up()
-        try:
-            snap_1 = vm.snapshot()
-            snapshots.append(snap_1)
-
-            snap_2_file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(snap_2_file)
-            snap_2 = vm.snapshot()
-            snapshots.append(snap_2)
-
-            snap_1.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {snap_2_file}')
-
-            snap_3_file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(snap_3_file)
-            snap_3 = vm.snapshot()
-            snapshots.append(snap_3)
-
-            snap_2.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {snap_3_file}')
-            vm.ssh(f'test -f {snap_2_file}')
-
-            snap_3.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {snap_2_file}')
-            vm.ssh(f'test -f {snap_3_file}')
-
-            snap_1.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {snap_2_file}')
-            vm.ssh(f'test ! -f {snap_3_file}')
-
-            tmp_file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(tmp_file)
-            snap_1.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {snap_2_file}')
-            vm.ssh(f'test ! -f {snap_3_file}')
-            vm.ssh(f'test ! -f {tmp_file}')
-
-        finally:
-            for snap in snapshots:
-                snap.destroy(verify=True)
-            vm.destroy()
-
-    def test_vdi_revert_cbt(self):
-        snapshots = []
-        vm = self.clone()
-        sr = vm.get_sr()
-
-        if not vm.is_running():
-            vm.start()
-        vm.wait_for_vm_running_and_ssh_up()
-        snap_no_cbt_file = f"/root/{uuid.uuid4()}"
-        vm.ssh_touch_file(snap_no_cbt_file)
-        snap_no_cbt = vm.snapshot()
-        snapshots.append(snap_no_cbt)
-
-        try:
-            vm.vdis[0].enable_cbt()
-            assert len(vm.vdis) == 1
-            snap_cbt_file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(snap_cbt_file)
-            snap_cbt = vm.snapshot()
-            snapshots.append(snap_cbt)
-
-            before_revert_file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(before_revert_file)
-
-            # CBT -> CBT
-            snap_cbt.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test -f {snap_cbt_file}')
-            assert vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-            # CBT -> no CBT
-            snap_no_cbt.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test ! -f {snap_cbt_file}')
-            assert not vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-            # no CBT -> CBT
-            snap_cbt.revert()
-            vm.start()
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test -f {snap_cbt_file}')
-            assert vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-        finally:
-            for snap in snapshots:
-                snap.destroy(verify=True)
-            vm.destroy()
-
-    def test_vdi_revert_journal_cbt(self, request: pytest.FixtureRequest, fistpoint: str, host: Host | None = None):
-        from lib.fistpoint import FistPoint # noqa: I001 Avoid circular import
-        host_uuid = host.uuid if host else None
-
-        request.getfixturevalue("exit_on_fistpoint")
-        vm = self.clone()
-        sr = vm.get_sr()
-
-        snapshots = []
-        if not vm.is_running():
-            vm.start(host_uuid)
-        vm.wait_for_vm_running_and_ssh_up()
-
-        snap_no_cbt_file = f"/root/{uuid.uuid4()}"
-        vm.ssh_touch_file(snap_no_cbt_file)
-        snap_no_cbt = vm.snapshot()
-        snapshots.append(snap_no_cbt)
-
-        vm.vdis[0].enable_cbt()
-        snap_cbt_file = f"/root/{uuid.uuid4()}"
-        vm.ssh_touch_file(snap_cbt_file)
-        snap_cbt = vm.snapshot()
-        snapshots.append(snap_cbt)
-
-        before_revert_file = f"/root/{uuid.uuid4()}"
-        vm.ssh_touch_file(before_revert_file)
-        try:
-            # CBT -> CBT
-            with FistPoint(vm.host, fistpoint), pytest.raises(commands.SSHCommandFailed):
-                snap_cbt.revert()
-            vm.start(host_uuid)
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test -f {snap_cbt_file}')
-            assert vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-            # CBT -> no CBT
-            with FistPoint(vm.host, fistpoint), pytest.raises(commands.SSHCommandFailed):
-                snap_no_cbt.revert()
-            vm.start(host_uuid)
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test -f {snap_cbt_file}')
-            assert vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-            # no CBT -> CBT
-            snap_no_cbt.revert()
-            with FistPoint(vm.host, fistpoint), pytest.raises(commands.SSHCommandFailed):
-                snap_cbt.revert()
-            vm.start(host_uuid)
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {before_revert_file}')
-            vm.ssh(f'test -f {snap_no_cbt_file}')
-            vm.ssh(f'test ! -f {snap_cbt_file}')
-            assert not vm.vdis[0].is_cbt_enabled()
-            assert VDI(snap_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-            assert not VDI(snap_no_cbt.vdi_uuids()[0], sr=sr).is_cbt_enabled()
-
-        finally:
-            for snap in snapshots:
-                snap.destroy(verify=True)
-            vm.destroy()
-
-    def test_vdi_revert_journal(self, request: pytest.FixtureRequest, fistpoint: str, host: Host | None = None):
-        from lib.fistpoint import FistPoint # noqa: I001 Avoid circular import
-        request.getfixturevalue("exit_on_fistpoint")
-        vm = self.clone()
-        snap = vm.snapshot()
-        host_uuid = host.uuid if host else None
-        if not vm.is_running():
-            vm.start(host_uuid)
-        vm.wait_for_vm_running_and_ssh_up()
-        try:
-            file = f"/root/{uuid.uuid4()}"
-            vm.ssh_touch_file(file)
-
-            with FistPoint(host if host else vm.host, fistpoint), pytest.raises(commands.SSHCommandFailed):
-                snap.revert()
-
-            vm.start(host_uuid)
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test -f {file}')
-
-            snap.revert()
-            vm.start(host_uuid)
-            vm.wait_for_vm_running_and_ssh_up()
-            vm.ssh(f'test ! -f {file}')
-        finally:
-            snap.destroy(verify=True)
-            vm.destroy()
-
-    def test_critical_journal_revert(self, request: pytest.FixtureRequest, host: Host, fistpoint: str):
-        from lib.fistpoint import FistPoint # noqa: I001 Avoid circular import
-        vm = self.clone()
-        sr = vm.get_sr()
-        request.getfixturevalue("exit_on_fistpoint")
-        snap = vm.snapshot()
-        sr.param_set("other-config", "false", "auto-scan") # Avoid journals being run during tests unexpectedly
-        try:
-            with FistPoint(vm.host, fistpoint), pytest.raises(commands.SSHCommandFailed):
-                snap.revert()
-
-            with pytest.raises(
-                commands.SSHCommandFailed,
-                check=lambda e: (
-                    "The SR is not available [opterr=Critical journals are pending. A scan is required.]" in e.stdout
-                ),
-            ):
-                vm.start(on=host.uuid)
-        finally:
-            sr.scan() # Force journals to be processed
-            sr.param_remove("other-config", "auto-scan")
-            snap.destroy(verify=True)
-            vm.destroy()
-
     def get_messages(self, name: str) -> List[str]:
         args: dict[str, str | bool | dict[str, str]] = {
             'obj-uuid': self.uuid,

tests/storage/cephfs/test_cephfs_sr.py

@@ -4,13 +4,18 @@
 import time
 
 from lib.commands import SSHCommandFailed
-from lib.common import vm_image, wait_for
+from lib.common import Defer, vm_image, wait_for
 from lib.host import Host
 from lib.pool import Pool
 from lib.sr import SR
 from lib.vdi import VDI
 from lib.vm import VM
 from tests.storage import vdi_is_open
+from tests.storage.storage import (
+    check_critical_journal_revert,
+    check_vdi_revert,
+    check_vdi_revert_journal,
+)
 
 # Requirements:
 # - one XCP-ng host >= 8.2
@@ -47,8 +52,8 @@ def test_snapshot(self, vm_on_cephfs_sr: VM) -> None:
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
-    def test_revert(self, vm_on_cephfs_sr: VM) -> None:
-        vm_on_cephfs_sr.test_vdi_revert()
+    def test_revert(self, vm_on_cephfs_sr: VM, defer: Defer) -> None:
+        check_vdi_revert(defer, vm_on_cephfs_sr)
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
@@ -60,13 +65,15 @@ def test_revert(self, vm_on_cephfs_sr: VM) -> None:
             "FileSR_revert_create_dest",
         ]
     )
-    def test_revert_journal(self, vm_on_cephfs_sr: VM, request: pytest.FixtureRequest, fistpoint: str):
-        vm_on_cephfs_sr.test_vdi_revert_journal(request, fistpoint, vm_on_cephfs_sr.host.pool.master)
+    def test_revert_journal(self, vm_on_cephfs_sr: VM, defer: Defer, exit_on_fistpoint: None, fistpoint: str):
+        check_vdi_revert_journal(defer, vm_on_cephfs_sr, fistpoint, vm_on_cephfs_sr.host.pool.master)
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
-    def test_critical_journal_revert(self, vm_on_cephfs_sr: VM, request: pytest.FixtureRequest, hostA2: Host) -> None:
-        vm_on_cephfs_sr.test_critical_journal_revert(request, hostA2, "FileSR_revert_create_src")
+    def test_critical_journal_revert(
+        self, vm_on_cephfs_sr: VM, defer: Defer, exit_on_fistpoint: None, hostA2: Host
+    ) -> None:
+        check_critical_journal_revert(defer, vm_on_cephfs_sr, hostA2, "FileSR_revert_create_src")
 
     # *** tests with reboots (longer tests).
 

tests/storage/ext/test_ext_sr.py

@@ -23,6 +23,12 @@
     vdi_is_open,
     xva_export_import,
 )
+from tests.storage.storage import (
+    check_vdi_revert,
+    check_vdi_revert_cbt,
+    check_vdi_revert_journal,
+    check_vdi_revert_journal_cbt,
+)
 
 # Requirements:
 # - one XCP-ng host with an additional unused disk for the SR
@@ -64,18 +70,18 @@ def test_snapshot(self, vm_on_ext_sr: VM) -> None:
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
-    def test_revert(self, vm_on_ext_sr: VM) -> None:
-        vm_on_ext_sr.test_vdi_revert()
+    def test_revert(self, vm_on_ext_sr: VM, defer: Defer) -> None:
+        check_vdi_revert(defer, vm_on_ext_sr)
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
-    def test_revert_cbt(self, vm_on_ext_sr: VM) -> None:
-        vm_on_ext_sr.test_vdi_revert_cbt()
+    def test_revert_cbt(self, vm_on_ext_sr: VM, defer: Defer) -> None:
+        check_vdi_revert_cbt(defer, vm_on_ext_sr)
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
-    def test_revert_journal_cbt(self, vm_on_ext_sr: VM, request: pytest.FixtureRequest):
-        vm_on_ext_sr.test_vdi_revert_journal_cbt(request, "FileSR_revert_create_src")
+    def test_revert_journal_cbt(self, vm_on_ext_sr: VM, defer: Defer, exit_on_fistpoint: None):
+        check_vdi_revert_journal_cbt(defer, vm_on_ext_sr, "FileSR_revert_create_src")
 
     @pytest.mark.small_vm
     @pytest.mark.big_vm
@@ -87,8 +93,8 @@ def test_revert_journal_cbt(self, vm_on_ext_sr: VM, request: pytest.FixtureReque
             "FileSR_revert_create_dest",
         ]
     )
-    def test_revert_journal(self, vm_on_ext_sr: VM, request: pytest.FixtureRequest, fistpoint: str):
-        vm_on_ext_sr.test_vdi_revert_journal(request, fistpoint)
+    def test_revert_journal(self, vm_on_ext_sr: VM, defer: Defer, exit_on_fistpoint: None, fistpoint: str):
+        check_vdi_revert_journal(defer, vm_on_ext_sr, fistpoint)
 
     @pytest.mark.small_vm
     @pytest.mark.parametrize("vdi_op", ["snapshot", "clone"])