linstor: test VM startup on disk failure by Kuruyia · Pull Request #595 · xcp-ng/xcp-ng-tests

Kuruyia · 2026-06-22T13:42:24Z

This adds a test to the LINSTOR SR test suite to make sure that a with a VDI on a shared LINSTOR SR can still start and shut down when a physical disk of that SR has failed.

The test does the following:

Fails a physical disk of the LINSTOR SR pool on a random host.
Ensures a VM can still start up and shut down on all hosts.

This uses a device mapper to avoid relying on the capabilities of underlying block device.

Supersedes #312.

Kuruyia · 2026-06-22T13:42:39Z

Leaving this PR as draft for now since this test requires starting in a clean environment (with no LINSTOR SR already set-up) since it needs to set up the dm-flakey device mapper under the LVM volume. I'd like to open a discussion here on how to implement this properly.

I would have hoped that placing this test in its own sub-package (tests/storage/linstor/unhealthy/) would be enough for the clean up of the parent package (tests/storage/linstor/) fixtures to occur, but this is not the case.

@glehmann I believe this is what #558 was trying to fix for the create_destroy tests, but I've observed that (at least, in the LINSTOR SR tests) the instances of the package-scoped fixtures in the main package are the same used in the sub-packages.

glehmann · 2026-06-22T15:10:46Z

I would have hoped that placing this test in its own sub-package (tests/storage/linstor/unhealthy/) would be enough for the clean up of the parent package (tests/storage/linstor/) fixtures to occur, but this is not the case.

Have you seen that in pkgfixtures.py?

# Due to a bug in the way pytest handles the setup and teardown of package-scoped fixtures,
# we moved the following fixtures out of the main conftest.py.
# To workaround the bug, the fixture must be imported either in a package's own conftest.py,
# or directly in a test module. Then the fixtures will truly be handled as package-scoped.
# Reference: https://github.com/pytest-dev/pytest/issues/8189

It might be worth trying if it helps

@glehmann I believe this is what #558 was trying to fix for the create_destroy tests, but I've observed that (at least, in the LINSTOR SR tests) the instances of the package-scoped fixtures in the main package are the same used in the sub-packages.

The case is a bit different in #558: we want the fixtures to not be there when running the create/destroy tests.

Kuruyia · 2026-06-23T09:40:39Z

Have you seen that in pkgfixtures.py?

# Due to a bug in the way pytest handles the setup and teardown of package-scoped fixtures,
# we moved the following fixtures out of the main conftest.py.
# To workaround the bug, the fixture must be imported either in a package's own conftest.py,
# or directly in a test module. Then the fixtures will truly be handled as package-scoped.
# Reference: https://github.com/pytest-dev/pytest/issues/8189

It might be worth trying if it helps

I tried the following:

Create a tests/storage/linstor/pkgfixtures.py file.
Move all package-scoped fixtures from tests/storage/linstor/conftest.py into tests/storage/linstor/pkgfixtures.py.
Import all those fixtures back in tests/storage/linstor/conftest.py.
Import all those fixtures in tests/storage/linstor/unhealthy/conftest.py.

But pytest comes up with the following plan:

[...]
  TEARDOWN P linstor_sr
  TEARDOWN P storage_pool_name
  TEARDOWN P linstor_redundancy
  TEARDOWN P pool_with_linstor
  TEARDOWN P _linstor_config
  TEARDOWN P lvm_disks
  TEARDOWN P lvm_disk_paths
  TEARDOWN P flakey_unused_512B_disk
  TEARDOWN P linstor_sr
  TEARDOWN P linstor_redundancy
  TEARDOWN P pool_with_linstor
  TEARDOWN P _linstor_config
  TEARDOWN P pool_with_saved_yum_state
  TEARDOWN P storage_pool_name
  TEARDOWN P lvm_disks
  TEARDOWN P lvm_disk_paths
[...]

As you can see, it just duplicates teardown for those package-scoped fixtures. I think pytest performs teardown on package-scoped fixtures when all tests from the package and sub-packages ran, but not when it starts running tests of a sub-package.

Maybe the "simpler" approach here would be to extract the bulk of the logic for creating the LVM volume and LINSTOR SR into their own files in lib/? This way I can have completely separate fixtures in the main package and this new sub-package, while minimizing code duplication.

The case is a bit different in #558: we want the fixtures to not be there when running the create/destroy tests.

I believe both problems are somewhat related?

Only tested on the LINSTOR SR tests - I observed that if a test, that uses a fixture that creates a LINSTOR SR, runs before the create_destroy tests, then when it's the turn of the create_destroy tests, they will fail immediately complaining that python-linstor is already installed (although those tests would have also failed down the line because the LINSTOR SR was already created).

My understanding of #558 was that teardown of package-scoped fixtures was supposed to be performed when entering a sub-package, which would solve this problem. Is that correct?

This can be easily replicated by:

Creating a new tests/storage/linstor/awesome_tests/ sub-package (note that it alphabetically comes before tests/storage/linstor/create_destroy/ so it runs first).
Creating a no-op test inside of this sub-package that uses the linstor_sr fixture.

This extracts the `host_devices` function of the `lvm_disks` fixture in the LINSTOR SR tests into its own fixture. This allows getting the paths to the physical disks used in the LVM volume in other fixtures and the tests, and overriding those paths in sub-packages. Signed-off-by: Alexandre Sollier <alexandre.sollier@vates.tech>

This moves out the regular LINSTOR SR tests into its own `tests/storage/linstor/regular` package to remove all tests from the `linstor` package. Package-scoped fixtures were also moved to their own `pkgfixtures.py` file and are explicitly imported into each `conftest.py` file of the sub-packages. This is done because we need to make sure the setup and teardown of those fixtures execute when crossing a sub-package boundary, which does not happen when a package wants to use a fixture from a parent package. This follows how the problem has been fixed in the root `conftest.py` file. Signed-off-by: Alexandre Sollier <alexandre.sollier@vates.tech>

Kuruyia · 2026-07-09T15:30:27Z

As seen with @glehmann yesterday, I split the LINSTOR SR tests into separate sub-packages to work around the issue of package-scoped fixtures not doing the setup/teardown sequence when crossing a sub-package boundary when they are declared in the parent package conftest.py file. This is similar to what's already being done for the root package-scoped fixtures.

Marking this PR as ready for review since this was the last blocking topic for me.

CI seems to be currently failing for reasons unrelated to this PR.

glehmann · 2026-07-10T13:47:05Z

    return wait_for(fn, msg, timeout_secs, retry_delay_secs, True)

+def run_with_timeout(fn: Callable[[], Any], timeout_secs: int = 2 * 60) -> None:
+    queue = multiprocessing.Queue()


Suggested change

queue = multiprocessing.Queue()

queue: multiprocessing.Queue[Exception] = multiprocessing.Queue()

glehmann · 2026-07-10T13:47:06Z

+                vm.wait_for_os_booted()
+                vm.shutdown(verify=True)
+        finally:
+            flakey_unused_512B_disk[random_host].repair()


Using defer(lambda: flakey_unused_512B_disk[random_host].repair()) line 36 would avoid the try/finally block and keep the disk in its failed state in the debugger, in case of failure

glehmann · 2026-07-10T13:47:06Z

+    ) -> None:
+        sr = linstor_sr
+        vm = vm_on_linstor_sr
+        random_host = random.choice(sr.pool.hosts)


Introducing randomness might make the test less stable. Is there a reason to pick the host randomly?

I have no reasons myself, this is more of an artifact from the previous PR. The test should be successful on all hosts equally.

We can either fix this to e.g. the first host of the list, or fail the disk and do the test on all hosts one-by-one if we want to be thorough (if we're ready to pay the extra time complexity, which would be $n^2$ in total with $n$ being the number of hosts).

Using the first host is fine IMO

glehmann · 2026-07-10T13:47:06Z

+        logging.info(f'Repairing device {self._device.path} on {self._host.hostname_or_ip}')
+
+        self._apply_dm_table(self._build_dm_table(False))
+        cmd_res = self._host.ssh('linstor -m --controllers `xe host-list params=address --minimal` r l -n `hostname` --props DrbdOptions/SkipDisk') # noqa: E501


You can just split the line instead of disabling the E501 rule

Suggested change

cmd_res = self._host.ssh('linstor -m --controllers `xe host-list params=address --minimal` r l -n `hostname` --props DrbdOptions/SkipDisk') # noqa: E501

cmd_res = self._host.ssh('linstor -m --controllers `xe host-list params=address --minimal` r l'

' -n `hostname` --props DrbdOptions/SkipDisk')

This adds the `run_with_timeout` helper function that can be used to call blocking functions with a timeout to avoid the tests potentially hanging indefinitely. Signed-off-by: Alexandre Sollier <alexandre.sollier@vates.tech>

Kuruyia · 2026-07-20T07:55:46Z

Marking this as draft again since more extensive testing shows that this test sometimes ends up in a split-brain on the DRBD side.

This adds a test to the LINSTOR SR test suite to make sure that a VM with a VDI on a shared LINSTOR SR can still start and shut down properly when a physical disk of that SR has failed. The test does the following: - Fails a physical disk of the LINSTOR SR pool on a random host. - Ensures a VM can still start up and shut down on all hosts. This uses a device mapper to avoid relying on the capabilities of the underlying block device. Signed-off-by: Alexandre Sollier <alexandre.sollier@vates.tech>

Kuruyia self-assigned this Jun 22, 2026

Kuruyia mentioned this pull request Jun 22, 2026

Add disk failure test to validate SR and VM resilience #312

Closed

glehmann mentioned this pull request Jun 22, 2026

Fix diff merge algorithm pairing deletes with wrong inserts across sections jennings/pulldash#247

Closed

Kuruyia added 2 commits July 9, 2026 14:19

Kuruyia force-pushed the aso/linstor_unhealthy branch from 42217f2 to 73b6b8b Compare July 9, 2026 15:23

Kuruyia marked this pull request as ready for review July 9, 2026 15:30

Kuruyia requested review from a team as code owners July 9, 2026 15:30

glehmann reviewed Jul 10, 2026

View reviewed changes

lib: add run_with_timeout helper

46b1c18

This adds the `run_with_timeout` helper function that can be used to call blocking functions with a timeout to avoid the tests potentially hanging indefinitely. Signed-off-by: Alexandre Sollier <alexandre.sollier@vates.tech>

Kuruyia force-pushed the aso/linstor_unhealthy branch from 73b6b8b to 904bb05 Compare July 10, 2026 14:28

glehmann approved these changes Jul 15, 2026

View reviewed changes

Kuruyia force-pushed the aso/linstor_unhealthy branch from 904bb05 to a53257f Compare July 20, 2026 07:51

Kuruyia marked this pull request as draft July 20, 2026 07:52

Kuruyia force-pushed the aso/linstor_unhealthy branch from a53257f to b5d6c9d Compare July 20, 2026 14:59

Kuruyia force-pushed the aso/linstor_unhealthy branch from b5d6c9d to 73b7044 Compare July 20, 2026 15:01

Navigation Menu

Uh oh!

linstor: test VM startup on disk failure - #595

linstor: test VM startup on disk failure#595
Kuruyia wants to merge 4 commits into
masterfrom
aso/linstor_unhealthy

Kuruyia commented Jun 22, 2026

Uh oh!

Kuruyia commented Jun 22, 2026

Uh oh!

glehmann commented Jun 22, 2026

Uh oh!

Kuruyia commented Jun 23, 2026

Uh oh!

Kuruyia commented Jul 9, 2026

Uh oh!

glehmann Jul 10, 2026

Uh oh!

glehmann Jul 10, 2026

Uh oh!

glehmann Jul 10, 2026

Uh oh!

Kuruyia Jul 10, 2026

Uh oh!

glehmann Jul 15, 2026

Uh oh!

Uh oh!

glehmann Jul 10, 2026

Uh oh!

Kuruyia commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	queue = multiprocessing.Queue()
	queue: multiprocessing.Queue[Exception] = multiprocessing.Queue()

	cmd_res = self._host.ssh('linstor -m --controllers `xe host-list params=address --minimal` r l -n `hostname` --props DrbdOptions/SkipDisk') # noqa: E501
	cmd_res = self._host.ssh('linstor -m --controllers `xe host-list params=address --minimal` r l'
	' -n `hostname` --props DrbdOptions/SkipDisk')

Uh oh!

Conversation

Kuruyia commented Jun 22, 2026

Uh oh!

Kuruyia commented Jun 22, 2026

Uh oh!

glehmann commented Jun 22, 2026

Uh oh!

Kuruyia commented Jun 23, 2026

Uh oh!

Kuruyia commented Jul 9, 2026

Uh oh!

glehmann Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

glehmann Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

glehmann Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Kuruyia Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

glehmann Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glehmann Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Kuruyia commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants