Add test for rolling pool update with VM availability check on updated host #310

rushikeshjadhav · 2025-05-21T12:17:40Z

Added test_linstor_sr_pool_update for rolling pool update with VM availability check.

It performs

A rolling update and reboot of all hosts in a LINSTOR SR pool (starting with the master).
Verifies VM can start and shutdown successfully on each host after update.
Ensures SR remains usable throughout the process.

stormi · 2025-05-22T17:19:34Z

tests/storage/linstor/test_linstor_sr.py

The tricky part here is ensuring that we do have packages to test the rolling pool update. That means starting with a pool which has an older version of the packages compared to the ones we want to test. That also likely means moving this kind of test to a specific job, or have an orchestrator test (or fixtures) that prepares a nested pool for this test.

See with @ydirson, as I think there are common challenges with the installation and upgrade tests.

So should we limit this to xcp-ng-linstor-testing packages to keep it in linstor tests? Along side of main update tests, this can be specific for linstor related testing package tests. For this test pool_with_linstor fixture will have to run without enablerepo="xcp-ng-linstor-testing", so that we can reliably test "upgrade" from main repo linstor vs new one.

Larger installation and upgrade tests can happen in different job.

# repoquery -a --repoid=xcp-ng-linstor-testing linstor-common-0:1.29.2-1.el7_9.noarch linstor-controller-0:1.29.2-1.el7_9.noarch linstor-satellite-0:1.29.2-1.el7_9.noarch

# repoquery -a --repoid=xcp-ng-linstor linstor-common-0:1.29.0-1.el7_9.noarch linstor-controller-0:1.29.0-1.el7_9.noarch linstor-satellite-0:1.29.0-1.el7_9.noarch

@stormi @ydirson can you check if the latest approach of using a marker upgrade_test to conditionally select testing repo is useful?

See my other comments, I don't really grasp what you tried to achieve with that.

The tricky part here is ensuring that we do have packages to test the rolling pool update. That means starting with a pool which has an older version of the packages compared to the ones we want to test. That also likely means moving this kind of test to a specific job, or have an orchestrator test (or fixtures) that prepares a nested pool for this test.

What we want to do depends on what we want to test. If we're testing that a rolling update works, one very important parameter is which version we're updating from, which raises the question of which such updates are supported (and incidentally, what does the product to, and what we should check, for unsupported updates. Need input from @Wescoeur and @Nambrok here.

My feeling here is that we want to setup specific pools first (nested, I'd say), and setup the linstor cluster with the specific version (a test parameter) that we want to upgrade from.

As to which repo to take the update from, we may want to allow devs to select it (ie have control on what they want to test), so I'd rather have this simply controlled by data.py.

I think we want to test progressive updates to not complicate our lives for the moment. The advantage of nested pools allows us to have an image at a given version and to easily rollback (and of course to select any LINSTOR version). Now this does not allow us to test on "real machines", if we want to do without nested ones, it would have to save the initial /var/lib/linstor folder as well as several config files, it is not necessarily easy to do...

If we don't have a lot of time for now, the nested solution "should" be ok.

stormi · 2025-05-22T17:20:35Z

tests/storage/linstor/test_linstor_sr.py

+                h.install_updates()
+                h.reboot(verify=True)
+                vm.start(on=h.uuid)
+                vm.wait_for_os_booted()
+                vm.shutdown(verify=True)


This doesn't look like the proper way to upgrade a pool with linstor. They don't support rolling upgrade, so you have to update all satellites and restart them, at some point. @Wescoeur has this documented somewhere.

Yes, it's https://docs.xcp-ng.org/management/updates/#rolling-pool-update-rpu I have updated the code considering above.

… from testing repo Signed-off-by: Rushikesh Jadhav <[email protected]>

…po in pool_with_linstor This mechanism allows the test to start with our without testing repo packages Signed-off-by: Rushikesh Jadhav <[email protected]>

Tests that want to test linstor upgrade can enablerepo during execution Signed-off-by: Rushikesh Jadhav <[email protected]>

…ity check. - Uses `@pytest.mark.upgrade_test` to mark need of upgrade during the test - Updates of all hosts in a LINSTOR SR pool (starting with the master) - Reboots updated hosts - Verifies VM can start and shutdown successfully on each host after update - Ensures SR remains usable throughout the process Signed-off-by: Rushikesh Jadhav <[email protected]>

ydirson

In addition, please format the commit message as per the standard converntions

ydirson · 2025-06-13T13:15:10Z

tests/storage/linstor/conftest.py

+def pytest_configure(config):
+    config._linstor_upgrade_test = False
+
+def pytest_collection_modifyitems(config, items):


a note that those are pytest hooks (and, for the 2nd one, when it is called) it would be useful to the reader

ydirson · 2025-06-13T13:22:21Z

tests/storage/linstor/conftest.py

    import concurrent.futures
+
+    dont_use_testing_repo = request.config._linstor_upgrade_test


It is not clear at first read what this does: this line should have an explanation for future readers of the file, and the commit message should give an overview. And using a variable with a negative name does not help: something like use_testing_repo = not request.config._linstor_upgrade_test would be more readable - but that would not be enough in itself to understand the logic.

That's especially hard to follow, as the extra testing repo seems to be configured only for the upgrade test.

ydirson · 2025-06-13T13:23:23Z

lib/host.py

        logging.info("Install updates on host %s" % self)
-        return self.ssh(['yum', 'update', '-y'])
+        enablerepo_cmd = ['--enablerepo=%s' % enablerepo] if enablerepo is not None else []


that is not a command, should not be named _cmd

ydirson · 2025-06-13T13:24:57Z

lib/host.py

@@ -359,9 +359,10 @@ def pool_has_vm(self, vm_uuid, vm_type='vm'):
        else:
            return self.xe('vm-list', {'uuid': vm_uuid}, minimal=True) == vm_uuid

-    def install_updates(self):
+    def install_updates(self, enablerepo=None):


enablerepo sounds like a bool. extra_repo, maybe?
Also adding a type hint would help

ydirson · 2025-06-13T13:26:07Z

pytest.ini

@@ -5,6 +5,7 @@ markers =
    default_vm: mark a test with a default VM in case no --vm parameter was given.

    # *** Markers used to select tests at collect stage ***
+    upgrade_test: mark a test which will upgrade packages from testing repo


this line does not warrant a separate commit, it logically belongs to the next commit.

ydirson · 2025-06-13T13:36:23Z

tests/storage/linstor/test_linstor_sr.py

See my other comments, I don't really grasp what you tried to achieve with that.

ydirson · 2025-06-13T13:38:13Z

tests/storage/linstor/test_linstor_sr.py

+        3. Reboot all hosts.
+        4. Sequentially ensure that the VM can start on all hosts.
+        """
+        import concurrent.futures, threading


not much reason not to do that at top of file

ydirson · 2025-06-13T13:43:04Z

pytest.ini

@@ -5,6 +5,7 @@ markers =
    default_vm: mark a test with a default VM in case no --vm parameter was given.

    # *** Markers used to select tests at collect stage ***
+    upgrade_test: mark a test which will upgrade packages from testing repo


From what I gather this is about an "update" (ie using yum) not an "upgrade" (which uses the installer ISO)

ydirson · 2025-06-13T13:50:54Z

tests/storage/linstor/test_linstor_sr.py

The tricky part here is ensuring that we do have packages to test the rolling pool update. That means starting with a pool which has an older version of the packages compared to the ones we want to test. That also likely means moving this kind of test to a specific job, or have an orchestrator test (or fixtures) that prepares a nested pool for this test.

What we want to do depends on what we want to test. If we're testing that a rolling update works, one very important parameter is which version we're updating from, which raises the question of which such updates are supported (and incidentally, what does the product to, and what we should check, for unsupported updates. Need input from @Wescoeur and @Nambrok here.

My feeling here is that we want to setup specific pools first (nested, I'd say), and setup the linstor cluster with the specific version (a test parameter) that we want to upgrade from.

As to which repo to take the update from, we may want to allow devs to select it (ie have control on what they want to test), so I'd rather have this simply controlled by data.py.

ydirson · 2025-06-13T13:52:09Z

tests/storage/linstor/test_linstor_sr.py

+            if host.has_updates(enablerepo="xcp-ng-linstor-testing"):
+                host.install_updates(enablerepo="xcp-ng-linstor-testing")


we don't want a test for updates just doing nothing when by mistake it was given no updates, that should be a test error

rushikeshjadhav requested review from Nambrok, ydirson and stormi May 21, 2025 15:13

stormi reviewed May 22, 2025

View reviewed changes

rushikeshjadhav force-pushed the feat-storage-linstor-619 branch from 25af1c3 to 2ec6fe5 Compare May 27, 2025 13:42

rushikeshjadhav added 4 commits May 27, 2025 23:46

Define 'upgrade_test' marker for tests that validate package upgrades…

ae48061

… from testing repo Signed-off-by: Rushikesh Jadhav <[email protected]>

Add upgrade_test marker detection and conditionally select testing re…

199a1f0

…po in pool_with_linstor This mechanism allows the test to start with our without testing repo packages Signed-off-by: Rushikesh Jadhav <[email protected]>

Add enablerepo support to yum update and check-update in Host methods

a4eb408

Tests that want to test linstor upgrade can enablerepo during execution Signed-off-by: Rushikesh Jadhav <[email protected]>

rushikeshjadhav force-pushed the feat-storage-linstor-619 branch from 2ec6fe5 to f2a8994 Compare May 27, 2025 18:28

ydirson requested changes Jun 13, 2025

View reviewed changes

rushikeshjadhav marked this pull request as draft June 16, 2025 10:22

		import concurrent.futures

		dont_use_testing_repo = request.config._linstor_upgrade_test

		if host.has_updates(enablerepo="xcp-ng-linstor-testing"):
		host.install_updates(enablerepo="xcp-ng-linstor-testing")

Add test for rolling pool update with VM availability check on updated host #310

Are you sure you want to change the base?

Add test for rolling pool update with VM availability check on updated host #310

Uh oh!

Conversation

rushikeshjadhav commented May 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushikeshjadhav May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydirson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rushikeshjadhav May 27, 2025 •

edited

Loading