From 2335a30f4815cea4c30ebdf9595ed151c61e1bdb Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Fri, 13 Jun 2025 18:01:05 +0200 Subject: [PATCH 1/5] linstor: stop removing python-linstor package This particular package will be removed by the `saved_yum_state` fixture. If removing this code breaks other tests, then it is likely the latter that need fixing. Signed-off-by: Yann Dirson --- tests/storage/linstor/conftest.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/storage/linstor/conftest.py b/tests/storage/linstor/conftest.py index afb09da4d..2dba789db 100644 --- a/tests/storage/linstor/conftest.py +++ b/tests/storage/linstor/conftest.py @@ -77,15 +77,6 @@ def install_linstor(host): yield pool - # Need to remove this package as we have separate run of `test_create_sr_without_linstor` - # for `thin` and `thick` `provisioning_type`. - def remove_linstor(host): - logging.info(f"Cleaning up python-linstor from host {host}...") - host.yum_remove(["python-linstor"]) - - with concurrent.futures.ThreadPoolExecutor() as executor: - executor.map(remove_linstor, pool.hosts) - @pytest.fixture(scope='package') def linstor_sr(pool_with_linstor, provisioning_type, storage_pool_name): sr = pool_with_linstor.master.sr_create('linstor', 'LINSTOR-SR-test', { From 3ca7752e01a981e91bf0774b971277ad522b0c42 Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Fri, 13 Jun 2025 17:53:08 +0200 Subject: [PATCH 2/5] test_create_sr_without_linstor: more idiomatic handling of exceptions --- tests/storage/linstor/test_linstor_sr.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/storage/linstor/test_linstor_sr.py b/tests/storage/linstor/test_linstor_sr.py index 7dc6f4597..10e6662a9 100644 --- a/tests/storage/linstor/test_linstor_sr.py +++ b/tests/storage/linstor/test_linstor_sr.py @@ -22,19 +22,16 @@ def test_create_sr_without_linstor(self, host, lvm_disks, provisioning_type, sto # This test must be the first in the series in this module assert not host.is_package_installed('python-linstor'), \ "linstor must not be installed on the host at the beginning of the tests" - try: + with pytest.raises(SSHCommandFailed): sr = host.sr_create('linstor', 'LINSTOR-SR-test', { 'group-name': storage_pool_name, 'redundancy': '1', 'provisioning': provisioning_type }, shared=True) - try: + # if exception was not raised, cleanup + # FIXME: ignoring all exceptions looks like a problem here? + with contextlib.suppress(Exception): sr.destroy() - except Exception: - pass - assert False, "SR creation should not have succeeded!" - except SSHCommandFailed as e: - logging.info("SR creation failed, as expected: {}".format(e)) def test_create_and_destroy_sr(self, pool_with_linstor, provisioning_type, storage_pool_name): # Create and destroy tested in the same test to leave the host as unchanged as possible From 0f82d1e22bb3530db6cac8755f1c2fa20f12f91c Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Mon, 16 Jun 2025 18:21:54 +0200 Subject: [PATCH 3/5] linstor: fix requirements description Signed-off-by: Yann Dirson --- tests/storage/linstor/test_linstor_sr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/storage/linstor/test_linstor_sr.py b/tests/storage/linstor/test_linstor_sr.py index 10e6662a9..a7e05c233 100644 --- a/tests/storage/linstor/test_linstor_sr.py +++ b/tests/storage/linstor/test_linstor_sr.py @@ -8,7 +8,7 @@ from tests.storage import vdi_is_open # Requirements: -# - two or more XCP-ng hosts >= 8.2 with additional unused disk(s) for the SR +# - one pool of 3 or more XCP-ng hosts >= 8.2 with additional unused disk(s) for the SR # - access to XCP-ng RPM repository from the host class TestLinstorSRCreateDestroy: From d2bc7a53e4940ccd9dbf744db878176cf5e446df Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Fri, 13 Jun 2025 17:25:26 +0200 Subject: [PATCH 4/5] WIP linstor: use a fixture to check linstor is not installed "assert" here will show the test as FAILED, where in fact the test was not run. This makes it a fixture, so at least the test is just marked ERROR when that happens. FIXME: python-linstor still sounds a strange package to test for --- tests/storage/linstor/conftest.py | 6 ++++++ tests/storage/linstor/test_linstor_sr.py | 5 ++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/tests/storage/linstor/conftest.py b/tests/storage/linstor/conftest.py index 2dba789db..b839f9eb3 100644 --- a/tests/storage/linstor/conftest.py +++ b/tests/storage/linstor/conftest.py @@ -99,3 +99,9 @@ def vm_on_linstor_sr(host, linstor_sr, vm_ref): yield vm logging.info("<< Destroy VM") vm.destroy(verify=True) + +@pytest.fixture(scope='module') +def host_without_linstor(host): + # FIXME: is that really the package to test? + assert not host.is_package_installed('python-linstor'), \ + "linstor must not be installed on the host at the beginning of the tests" diff --git a/tests/storage/linstor/test_linstor_sr.py b/tests/storage/linstor/test_linstor_sr.py index a7e05c233..3207b48d2 100644 --- a/tests/storage/linstor/test_linstor_sr.py +++ b/tests/storage/linstor/test_linstor_sr.py @@ -18,10 +18,9 @@ class TestLinstorSRCreateDestroy: and VM import. """ - def test_create_sr_without_linstor(self, host, lvm_disks, provisioning_type, storage_pool_name): + def test_create_sr_without_linstor(self, host_without_linstor, lvm_disks, provisioning_type, storage_pool_name): # This test must be the first in the series in this module - assert not host.is_package_installed('python-linstor'), \ - "linstor must not be installed on the host at the beginning of the tests" + host = host_without_linstor with pytest.raises(SSHCommandFailed): sr = host.sr_create('linstor', 'LINSTOR-SR-test', { 'group-name': storage_pool_name, From be8e849606124547228c795426a92736f8a6bd6c Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Fri, 13 Jun 2025 18:01:21 +0200 Subject: [PATCH 5/5] WIP linstor: remarks --- tests/storage/linstor/conftest.py | 14 ++++++++++++++ tests/storage/linstor/test_linstor_sr.py | 1 + 2 files changed, 15 insertions(+) diff --git a/tests/storage/linstor/conftest.py b/tests/storage/linstor/conftest.py index b839f9eb3..0dbcc68c8 100644 --- a/tests/storage/linstor/conftest.py +++ b/tests/storage/linstor/conftest.py @@ -43,17 +43,30 @@ def lvm_disks(host, sr_disks_for_all_hosts, provisioning_type): @pytest.fixture(scope="package") def storage_pool_name(provisioning_type): + # FIXME: this needs an explanation return GROUP_NAME if provisioning_type == "thick" else STORAGE_POOL_NAME +# FIXME why having this feature of session scope? Shouldn't it make +# it impossible to run both thin and thick tests in the same session? @pytest.fixture(params=["thin"], scope="session") def provisioning_type(request): return request.param +# FIXME: this feature has scope "package" but even test_linsor_sr file +# includes tests that need *not* to have linstor installed. Currently +# pool_with_saved_yum_state's setup is being run for both thin and +# thick, but with a single teardown at the end (which is even not what +# --setup-plan claims it will do)? Is there even a way to make this +# work, with pool_with_saved_yum_state being of scope package anyway? +# Possibly by grouping packages with identical package reqs into +# searate sub-packages? @pytest.fixture(scope='package') def pool_with_linstor(hostA2, lvm_disks, pool_with_saved_yum_state): import concurrent.futures pool = pool_with_saved_yum_state + # FIXME must check we have at least 3 hosts - hostA3 or a simple check here? + def check_linstor_installed(host): if host.is_package_installed(LINSTOR_PACKAGE): raise Exception( @@ -69,6 +82,7 @@ def install_linstor(host): host.yum_install([LINSTOR_PACKAGE], enablerepo="xcp-ng-linstor-testing") # Needed because the linstor driver is not in the xapi sm-plugins list # before installing the LINSTOR packages. + # FIXME: why multipathd? host.ssh(["systemctl", "restart", "multipathd"]) host.restart_toolstack(verify=True) diff --git a/tests/storage/linstor/test_linstor_sr.py b/tests/storage/linstor/test_linstor_sr.py index 3207b48d2..6df0a9ad3 100644 --- a/tests/storage/linstor/test_linstor_sr.py +++ b/tests/storage/linstor/test_linstor_sr.py @@ -20,6 +20,7 @@ class TestLinstorSRCreateDestroy: def test_create_sr_without_linstor(self, host_without_linstor, lvm_disks, provisioning_type, storage_pool_name): # This test must be the first in the series in this module + # FIXME: why would it be? host = host_without_linstor with pytest.raises(SSHCommandFailed): sr = host.sr_create('linstor', 'LINSTOR-SR-test', {