Add coalesce integrity tests on most SR types #356

glehmann · 2025-10-02T14:31:28Z

Not sure if others should be covered as well.
I've removed the coalesce tests based on dom0, because they seemed redundant, but we could also keep them. Just let me know.

stormi · 2025-10-02T14:34:54Z

tests/storage/storage.py

+
+
+def install_randstream(vm: 'VM'):
+    vm.ssh("wget -nv https://github.com/xcp-ng/randstream/releases/download/0.3.1/randstream-0.3.1-x86_64-unknown-linux-musl.tar.gz -O - | tar -xzC /usr/bin/ ./randstream")  # noqa: E501


~~Something we should package instead?~~ It's inside the VM

We could add it to the Alpine image, but it seems simpler this way

Nambrok · 2025-10-02T15:08:49Z

lib/sr.py

        return self._type

-    def create_vdi(self, name_label: str, virtual_size: int = 64, image_format: Optional[str] = None) -> VDI:
+    def create_vdi(self, name_label: str, virtual_size: int | str = 64, image_format: Optional[str] = None) -> VDI:


I would prefer to choose between int or string. String make sense with how you want to use it 1GiB, so we could replace the default value to be a string and check caller to change them to use a string.

I would agree if create_vdi() wasn't used in some untyped code. For now, using it this way seems safer in order not to break existing code.

Nambrok · 2025-10-02T15:15:09Z

tests/storage/ext/conftest.py

-def vdi_on_ext_sr(ext_sr):
-    vdi = ext_sr.create_vdi('EXT-local-VDI-test')
+def vdi_on_ext_sr(ext_sr: 'SR'):
+    vdi = ext_sr.create_vdi('EXT-local-VDI-test', virtual_size='1GiB')


We should change it in the default value of the function if you intend to change it everywhere :)

Indeed it could be better. I haven't checked if it's used somewhere else, where it wouldn't make sense to require a larger disk though

It seems ok, I'll use 1GiB by default

Nambrok · 2025-10-08T08:06:01Z

tests/storage/ext/test_ext_sr.py

 from lib.fistpoint import FistPoint
-from lib.vdi import VDI
 from tests.storage import try_to_create_sr_with_missing_device, vdi_is_open
+from tests.storage.storage import install_randstream, operation_on_vdi, wait_for_vdi_coalesce


Could you add them in the tests/storage/__init__.py import to avoid the double storage like it's done for vdi_is_open above.

Nambrok · 2025-10-08T08:23:34Z

tests/storage/ext/test_ext_sr.py

+        try:
+            vm.start()
+            vm.wait_for_vm_running_and_ssh_up()
+            install_randstream(vm)


It would be best to do it in a fixture

That would make us either:

start the vm, install randstream and stop the vm

attach/detach the vdi on the running vm
Is that ok?

Your fixture could give the VM powered on

and attaching a VDI to a running VM if it has the PV drivers is ok

Nambrok · 2025-10-08T08:27:56Z

tests/storage/ext/test_ext_sr.py

+            vm.wait_for_vm_running_and_ssh_up()
+            install_randstream(vm)
+            vm.ssh("randstream generate -v /dev/xvdb")
+            vm.ssh("randstream validate -v --expected-checksum 65280014 /dev/xvdb")


Where are you getting the expected checksum that is hardcoded?
There is a seed on the second generate call a few lines later but not here.

I generate a first stream with the same size and copy/paste it.
This run uses the default seed (0). The second one uses a different seed to write something different on the disk.

Nambrok · 2025-10-08T08:29:32Z

tests/storage/ext/test_ext_sr.py

+            new_vdi = operation_on_vdi(vm.host, vdi.uuid, vdi_op)
+            vm.ssh("randstream generate -v --seed 1 --size 128Mi /dev/xvdb")
+            vm.ssh("randstream validate -v --expected-checksum ad2ca9af /dev/xvdb")
+            vm.host.xe("vdi-destroy", {"uuid": new_vdi.uuid})


Can't you just do new_vdi.destroy() like you do in the finally?

Nambrok · 2025-10-08T08:30:15Z

tests/storage/lvm/test_lvm_sr.py

 from lib.fistpoint import FistPoint
-from lib.vdi import VDI
 from tests.storage import try_to_create_sr_with_missing_device, vdi_is_open
+from tests.storage.storage import install_randstream, operation_on_vdi, wait_for_vdi_coalesce


Same here, adding the import in __init__.py

Wescoeur · 2025-10-08T16:16:37Z

lib/sr.py

        return self._type

-    def create_vdi(self, name_label: str, virtual_size: int = 64, image_format: Optional[str] = None) -> VDI:
+    def create_vdi(self, name_label: str, virtual_size: int | str = '1GiB', image_format: Optional[str] = None) -> VDI:


I'm not a fan of type unions except in special cases. Here, I think we should define an alias because I'm afraid we'll have to specify the union in several places in the tests in the future like resize tests.

For example:

Size = int | str ... def create_vdi(self, name_label: str, virtual_size: Size = '1GiB', image_format: Optional[str] = None) -> VDI:

Otherwise it's fine to use: 2**30 directly.

I'm not sure why it would propagate to other types, and 2**30 is not clear at all for me.

Would you be ok if I define some constants KiB, GiB, TiB that can be used as 2 * GiB, instead of passing a string for virtual_size?

Signed-off-by: Gaëtan Lehmann <[email protected]>

So we have something to write to. 1GiB seems small enough to not impact performance or request all the resources Signed-off-by: Gaëtan Lehmann <[email protected]>

Signed-off-by: Gaëtan Lehmann <[email protected]>

glehmann requested review from Nambrok and Wescoeur October 2, 2025 14:31

glehmann changed the title ~~Add coalesce integrity test on most SR types~~ Add coalesce integrity tests on most SR types Oct 2, 2025

stormi reviewed Oct 2, 2025

View reviewed changes

Nambrok reviewed Oct 2, 2025

View reviewed changes

Nambrok requested changes Oct 8, 2025

View reviewed changes

glehmann force-pushed the gln/coalesce branch 3 times, most recently from 31d71e1 to 5ff2af9 Compare October 8, 2025 15:38

Wescoeur reviewed Oct 8, 2025

View reviewed changes

glehmann mentioned this pull request Oct 8, 2025

Test disk integrity after migration #358

Open

Allow to connect/disconnect a VDI even when the VM is not running

fb949a3

Signed-off-by: Gaëtan Lehmann <[email protected]>

glehmann force-pushed the gln/coalesce branch from 5ff2af9 to bb23281 Compare October 9, 2025 06:00

glehmann added 13 commits October 9, 2025 10:13

storage: add some type annotations

c413ac5

Signed-off-by: Gaëtan Lehmann <[email protected]>

lib: add KiB, GiB and TiB constants

eff4625

Signed-off-by: Gaëtan Lehmann <[email protected]>

storage: use 1GiB disk size by default

a0fecd6

So we have something to write to. 1GiB seems small enough to not impact performance or request all the resources Signed-off-by: Gaëtan Lehmann <[email protected]>

storage: add utility functions for coalesce tests

3453e8f

Signed-off-by: Gaëtan Lehmann <[email protected]>

nfs: add coalesce tests

dde0fe0

Signed-off-by: Gaëtan Lehmann <[email protected]>

ext: add coalesce tests

843f718

Signed-off-by: Gaëtan Lehmann <[email protected]>

xfs: add coalesce tests

c3c2e81

Signed-off-by: Gaëtan Lehmann <[email protected]>

lvm: add coalesce tests

5597036

Signed-off-by: Gaëtan Lehmann <[email protected]>

zfs: add coalesce tests

6e0378a

Signed-off-by: Gaëtan Lehmann <[email protected]>

zfsvol: add coalesce tests

6863c04

Signed-off-by: Gaëtan Lehmann <[email protected]>

lvmoiscsi: add coalesce tests

63794f5

Signed-off-by: Gaëtan Lehmann <[email protected]>

Remove dom0 coalesce tests

2e156aa

Signed-off-by: Gaëtan Lehmann <[email protected]>

Fix non-expanded values in disconnect_vdi() assert

b50bda8

Signed-off-by: Gaëtan Lehmann <[email protected]>

glehmann force-pushed the gln/coalesce branch from bb23281 to b50bda8 Compare October 9, 2025 08:19



		def install_randstream(vm: 'VM'):
		vm.ssh("wget -nv https://github.com/xcp-ng/randstream/releases/download/0.3.1/randstream-0.3.1-x86_64-unknown-linux-musl.tar.gz -O - \| tar -xzC /usr/bin/ ./randstream") # noqa: E501

Add coalesce integrity tests on most SR types #356

Are you sure you want to change the base?

Add coalesce integrity tests on most SR types #356

Conversation

glehmann commented Oct 2, 2025

Uh oh!

stormi Oct 2, 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!

glehmann Oct 8, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stormi Oct 2, 2025 •

edited

Loading

glehmann Oct 8, 2025 •

edited

Loading