feat(coalesce): Add tests for coalesce #290

Nambrok · 2025-04-02T08:47:46Z

The tests create a VDI, connect it to Dom0 then use the tapdev to write random data somewhere in it.
It then compare the data of the VDI to the original data we have to see if it has changed.
The test create snapshot/clone before deleting them and waiting for the coalesce to have happened by observing sm-config:vhd-parent before checking the integrity of the data.

Also add --vdi-type parameter that default to vhd to prepare to tests multiple VDI types, i.e. qcow2.

conftest.py

lib/host.py

Nambrok · 2025-04-02T15:13:13Z

For the failing test: waiting for #291 to be resolved.

stormi · 2025-04-02T15:16:51Z

conftest.py

+        action="append",
+        default=[],
+        help="Format of VDI to execute tests on."
+        "Example: vhd,qcow2"


Making this a CLI parameter means you're deporting the test job definition outside pytest. When rishi wanted to do the same for thin vs thick in the context of XOSTOR tests, we asked him to rework the tests so they actually test both formats.

It's to allow to test only one type by hand, the objectives for tests is to keep the default which at the moment is vhd but will be ["vhd", "qcow2"] eventually.

pytest already has test selection mechanisms:

path to test files

-k

-m

As long as the parameter is not necessary in jobs.py, fine by me. We won't have SR types that only support VHD or only support QCOW2 at some point?

The parameter default to VHD when not given meaning it's not needed to change jobs.py. But I will need to add the new coalesce tests in jobs.py either way and it's the only test at the moment that will use vdi-type
I'm not sure to see how the other selection mechanisms from pytest could work in this case.

At some point to enable "qcow2", this will be required to make modifications in jobs.py

@Nambrok while doing similar for thin vs thick, we did

@pytest.fixture(params=["thin"], scope="session") def provisioning_type(request): return request.param

So here in this case, if we do -

@pytest.fixture(params=["vhd"], scope="session") def vdi_image_format(request): return request.param

and

@pytest.fixture(scope='package') def ext_sr(host, sr_disk, vdi_image_format): """ An EXT SR on first host. """ sr = host.sr_create('ext', "EXT-local-SR-test", { 'device': '/dev/' + sr_disk, 'preferred-image-formats': vdi_image_format }) yield sr # teardown sr.destroy()

the tests can be upgraded for new types in future for supported SR.

As the whole SR tests needs to be tested for both vhd & qcow2, it makes it suitable per SR tests package.

I'm fine with both approach, to get this added.

We can just change the default parameter value in my case, but it also allows to run only one type of image_format when started manually.
Your method always run both.

https://github.com/xcp-ng/xcp-ng-tests/pull/290/files#diff-a31c7ed5d35f5ed8233994868c54d625b18e6bacb6794344c4531e62bd9dde59R111
We can just add qcow2 on this line to add qcow2 to also run by default without modifying jobs.py.

Works, I think we can move ahead with this merge.

Your method always run both.

Well, there is still -k and -m for filtering as Sam mentioned. We may also want to avoid doing similar things different ways.

stormi · 2025-04-02T15:22:34Z

conftest.py

@@ -75,6 +75,13 @@ def pytest_addoption(parser):
             "4KiB blocksize to be formatted and used in storage tests. "
             "Set it to 'auto' to let the fixtures auto-detect available disks."
    )
+    parser.addoption(
+        "--image-format",


When you work in the context of storage everyday, maybe it's a clear name, but otherwise it might be too generic a name. Both image and format can be many things.

It's why I added vdi-image-format as suggestion.
But I wonder if we should not use the naming volume-image-format instead.

No preference.

And vdi-type looked consistent with sm's terminology 🤔

# xe vdi-list params=type | sort | uniq type ( RO) : CBT metadata type ( RO) : HA statefile type ( RO) : Redo log type ( RO) : User

Not really consistent. In this example here, type is completly different.

On the XAPI side, we have a vdi_type stored in the sm-config attribute. Here you can have the VHD/AIO value. However there are weird situations like: sm-config (MRO) : type: raw; vdi_type: aio.

Regarding the new QCOW2 format we discussed internally to use a new attribute image-format. type is completely ambiguous depending on what you are talking about and the context in which it is used.

In the SMAPIv3, vdi type is completly removed (same for VDI, we use volume instead). And image-format is used.

glehmann · 2025-04-02T15:56:49Z

For the failing test: waiting for #291 to be resolved.

I thought it would take a bit longer before it showed again!

rushikeshjadhav · 2025-07-07T10:30:20Z

conftest.py

+        if len(image_format) == 0:
+            image_format = ["vhd"] # Not giving image-format will default to doing tests on vhd


"vhd,qcow2" are considered as one single option by pytest when passing like --image-format=vhd,qcow2 thus need to convert it into list

image_format = image_format[0].split(",") if image_format else ["vhd"]

Indeed, but it's because different input are made into a list so --image-format=vhd --image-format=qcow2 => ["vhd", "qcow2"].
Some fixture are doing both but I didn't explicitely, we could do both.

I think it's cleaner to just use --image-format=vhd --image-format=qcow2 like this, but then example given in --help needs fixing, as it does suggest the other way.

lib/basevm.py

lib/host.py

ydirson

First pass for the big first commit

ydirson · 2025-07-28T15:23:35Z

conftest.py

+        action="append",
+        default=[],
+        help="Format of VDI to execute tests on."
+        "Example: vhd,qcow2"


Your method always run both.

Well, there is still -k and -m for filtering as Sam mentioned. We may also want to avoid doing similar things different ways.

ydirson · 2025-07-28T15:25:24Z

conftest.py

+        if len(image_format) == 0:
+            image_format = ["vhd"] # Not giving image-format will default to doing tests on vhd


I think it's cleaner to just use --image-format=vhd --image-format=qcow2 like this, but then example given in --help needs fixing, as it does suggest the other way.

tests/storage/coalesce/conftest.py

ydirson · 2025-07-28T15:41:21Z

tests/storage/coalesce/conftest.py

+    logging.info(f"<< Unplugging VDI {vdi_uuid} from Dom0")
+    host.xe("vbd-unplug", {"uuid": vbd_uuid})


IIRC vbd-unplug says/does nothing if already unplugged, wouldn't it be useful to first assert that the VBD is indeed plugged?

I'm a little afraid of potential side effects personally, but I have nothing against a warning log. On the other hand, we don't check whether the VDI to unplug is in the self.vdis list. 🤔

I'm a little afraid of potential side effects

I'm myself a bit afraid of what can happen if we proceed while the system is not in the expected state

Expected state being VBD unplugged so we can then remove it. Calling it and it doing nothing is not a problem, if it errors it will be raised.

tests/storage/coalesce/conftest.py

tests/storage/coalesce/test_coalesce.py

vdi.py: add getting the image_format if available sr.py: add choosing a image_format on vdi creation sr.py add get_type to get the type of the SR Signed-off-by: Damien Thenot <[email protected]>

Add a fixture `shared_sr` to get a shared SR Also modify local_sr_* function to log the SR type Signed-off-by: Damien Thenot <[email protected]>

ydirson · 2025-07-31T10:10:08Z

lib/basevm.py

+            # Doesn't work with Dom0 since `vm-disk-list` doesn't work on it so we create empty list
+            if e.stdout == "Error: No matching VMs found":
+                logging.info("Couldn't get disks list. We are Dom0. Continuing...")


Maybe rather something like "Couldn't get disks list (maybe we are dom0?). Continuing..." ?

ydirson · 2025-07-31T10:11:16Z

lib/basevm.py

-    def vdi_uuids(self, sr_uuid=None):
+    def vdi_uuids(self, sr_uuid=None) -> List[str]:


could also type sr_uuid

ydirson · 2025-07-31T10:16:36Z

lib/vm.py

+    def connect_vdi(self, vdi: VDI, device: str = "autodetect") -> str:
+        logging.info(f">> Plugging VDI {vdi.uuid} on VM {self.uuid}")
+        vbd_uuid = self.host.xe("vbd-create", {
+            "vdi-uuid": vdi.uuid,
+            "vm-uuid": self.uuid,
+            "device": device,
+        })
+        self.host.xe("vbd-plug", {"uuid": vbd_uuid})
+
+        self.vdis.append(vdi)
+
+        return vbd_uuid


if vbd-plug fails we may want to remove the vbd?

It looks like 2 fixtures for create and plug would be a good fit, except that it would not allow to store self.vdis, but I don't see why we need to store it (note: this advocates for finer commit granularity, where we have a better opportunity to explain why we're adding such a feature)... and creating a VDI object requires at least on XAPI call, which can make this sensibly slower especially for a VM with many VDIs

ydirson · 2025-07-31T10:29:21Z

tests/storage/coalesce/conftest.py

+@pytest.fixture(scope="module")
+def tapdev(local_sr_on_hostA1, vdi_with_vbd_on_dom0):
+    """
+    A tapdev is a blockdevice allowing access to a VDI from the Dom0.
+
+    It is usually used to give access to the VDI to Qemu for emulating devices
+    before PV driver are loaded in the guest.
+    """


That does not really explain how it is useful for testing.

Maybe naming it like tapdev_on_vdi_on_local_sr would be sufficient.

It should be mentioned somewhere that tapdev is used for a better test performance, to avoid instantiating a new VM to access the vdi

ydirson · 2025-07-31T10:36:56Z

tests/storage/coalesce/test_coalesce.py

+        assert compare_data(host, tapdev, data_file_on_host, offset, length)
+
+    @pytest.mark.parametrize("vdi_op", ["snapshot", "clone"])
+    def test_coalesce(self, host: "Host", tapdev: str, vdi_with_vbd_on_dom0: "VDI", data_file_on_host: str, vdi_op):


Having to use vdi_with_vbd_on_dom0 to get access to the VDI behind tapdev is confusing.
Maybe a TapDev object to encapsulate all relevant info would improve this?

This will have to change because tapdev like this won't be available in the future, it's something made available for the moment but will be lost with removing the blktap2 kernel patch in 9.0. We already have plan to redo this part.
I might had a way to obtain an access from Dom0 directly on the VDI object.

stormi

Almost good to merge for me, as we need to move on and I don't think remaining unadressed comments add too much technical debt.

However, I'd prefer if, before the merge, some of the remaining comments were addressed.

And this has to be run on CI before the merge.

glehmann · 2025-08-19T13:52:30Z

lib/sr.py

@@ -152,13 +154,21 @@ def is_shared(self):
                                                            {'uuid': self.uuid, 'param-name': 'shared'}))
        return self._is_shared

-    def create_vdi(self, name_label, virtual_size=64):
+    def get_type(self) -> str:
+        if self._type is None:


We could use functools.lru_cache decorator to simplify the caching (more a note for the future).

I'm not sure lru_cache is relevant here, as it's an LRU cache that's used and updated based on the parameters given to the function: here, the cache will contain a maximum of one value, as we only pass self as an argument. It's a little overkill. 😄

There is @functools.cache in this case :)

IMHO I think having own decorator to handle 0 arg (const_cache?) or continuing with an explicit private variable is better. @functools.cache uses a dict in its implementation to deal with arguments. (I think I don't like scripted languages with a tendency to consume more resources where we wouldn't do that in a binary :p )

The decorator is only applied at (byte-)compile time, so a clever python could get rid of the dict and incur zero runtime cost. No clue if it does 😉

glehmann · 2025-08-19T13:56:37Z

lib/host.py

+    def get_dom0_uuid(self):
+        return self.inventory["CONTROL_DOMAIN_UUID"]
+
+    def get_dom0_VM(self) -> VM:


get_dom0_vm to avoid mixing case

glehmann · 2025-08-19T13:58:11Z

lib/sr.py


 import lib.commands as commands
-from lib.common import prefix_object_name, safe_split, strtobool, wait_for, wait_for_not
+from lib.common import (


not sure why this is reformatted

glehmann · 2025-08-19T14:36:37Z

tests/storage/coalesce/test_coalesce.py

+    from lib.vdi import VDI
+
+class Test:
+    def test_write_data(self, host: "Host", tapdev: str, data_file_on_host: str):


I don't understand the goal of that test—is it somehow related to the coalesce feature?

glehmann · 2025-08-19T15:04:24Z

tests/storage/coalesce/conftest.py

+@pytest.fixture(scope="module")
+def tapdev(local_sr_on_hostA1, vdi_with_vbd_on_dom0):
+    """
+    A tapdev is a blockdevice allowing access to a VDI from the Dom0.
+
+    It is usually used to give access to the VDI to Qemu for emulating devices
+    before PV driver are loaded in the guest.
+    """


It should be mentioned somewhere that tapdev is used for a better test performance, to avoid instantiating a new VM to access the vdi

glehmann · 2025-08-19T15:05:50Z

tests/storage/coalesce/test_coalesce.py

+        assert compare_data(host, tapdev, data_file_on_host, offset, length)
+
+    @pytest.mark.parametrize("vdi_op", ["snapshot", "clone"])
+    def test_coalesce(self, host: "Host", tapdev: str, vdi_with_vbd_on_dom0: "VDI", data_file_on_host: str, vdi_op):


We could avoid generating a file on the dom0 if we:

generate a file with random content on the vdi directly;

compute the check sum and retrieve it locally

trigger the coalesce

recompute the check sum and verify it's the same as before

ydirson

Looks good for merging

glehmann · 2025-08-20T08:52:49Z

5 approvals—not something we see every day 😄

lib/vm.py

Add a list of vdis object to a VM Add get_dom0_vm in host.py Add function to connect/disconnect a VDI from a VM Add a path to `Snapshot.revert()` to reset the VDIs list of the VM object that created it since a revert would change the VDI UUIDs of the VM. Signed-off-by: Damien Thenot <[email protected]>

Add a vdi-type parameter to parametrize the vdi_type fixture, it defaults to vhd at the moment, will be used to run test on others vdi types, e.g. qcow2. The tests create a VDI, connect it to Dom0 then use the tapdev to write random data somewhere in it. It then compares the data of the VDI to the original data we have to see if it has changed. The test creates snapshot/clone before deleting them and waiting for the coalesce to have happened by observing sm-config:vhd-parent before checking the integrity of the data. Signed-off-by: Damien Thenot <[email protected]>

Nambrok force-pushed the dtt-coalesce branch from f398d35 to 53efb2a Compare April 2, 2025 11:14

Nambrok requested a review from Wescoeur April 2, 2025 11:16

Nambrok marked this pull request as ready for review April 2, 2025 11:16

Nambrok force-pushed the dtt-coalesce branch from 53efb2a to d8516eb Compare April 2, 2025 12:46

Wescoeur requested changes Apr 2, 2025

View reviewed changes

conftest.py Outdated Show resolved Hide resolved

lib/host.py Outdated Show resolved Hide resolved

Nambrok force-pushed the dtt-coalesce branch 4 times, most recently from 72580ab to 8cdbea9 Compare April 2, 2025 14:44

Nambrok requested review from Wescoeur and AnthoineB April 2, 2025 14:44

Nambrok force-pushed the dtt-coalesce branch from 8cdbea9 to 93a7a23 Compare April 2, 2025 14:48

stormi reviewed Apr 2, 2025

View reviewed changes

Nambrok force-pushed the dtt-coalesce branch from 93a7a23 to c7753f9 Compare April 9, 2025 14:34

AnthoineB approved these changes Apr 28, 2025

View reviewed changes

Nambrok force-pushed the dtt-coalesce branch 2 times, most recently from 2ec1e33 to d5ec0a3 Compare May 20, 2025 12:01

Nambrok force-pushed the dtt-coalesce branch 2 times, most recently from f07746f to ca0d349 Compare June 4, 2025 16:40

Nambrok force-pushed the dtt-coalesce branch 3 times, most recently from 4f33a49 to b96297e Compare July 3, 2025 07:47

rushikeshjadhav reviewed Jul 7, 2025

View reviewed changes

Nambrok requested a review from ydirson July 9, 2025 12:20

Nambrok force-pushed the dtt-coalesce branch 2 times, most recently from dc0ecdd to 46374df Compare July 24, 2025 09:57

Wescoeur approved these changes Jul 24, 2025

View reviewed changes

lib/basevm.py Outdated Show resolved Hide resolved

lib/host.py Outdated Show resolved Hide resolved

lib/host.py Outdated Show resolved Hide resolved

ydirson requested changes Jul 28, 2025

View reviewed changes

Nambrok force-pushed the dtt-coalesce branch 4 times, most recently from a9e09ed to add8109 Compare July 29, 2025 08:47

Nambrok added 2 commits July 29, 2025 11:33

feat(image_format): support image_format

0d957e7

vdi.py: add getting the image_format if available sr.py: add choosing a image_format on vdi creation sr.py add get_type to get the type of the SR Signed-off-by: Damien Thenot <[email protected]>

Add shared_sr + log type of SR

1d1e036

Add a fixture `shared_sr` to get a shared SR Also modify local_sr_* function to log the SR type Signed-off-by: Damien Thenot <[email protected]>

Nambrok force-pushed the dtt-coalesce branch 5 times, most recently from 5287d97 to 84f281d Compare July 29, 2025 10:05

ydirson requested changes Jul 31, 2025

View reviewed changes

Nambrok force-pushed the dtt-coalesce branch 3 times, most recently from befcbf5 to d4099df Compare August 7, 2025 14:51

stormi approved these changes Aug 7, 2025

View reviewed changes

glehmann approved these changes Aug 19, 2025

View reviewed changes

ydirson approved these changes Aug 19, 2025

View reviewed changes

Nambrok force-pushed the dtt-coalesce branch from d4099df to 259e737 Compare August 21, 2025 08:28

ydirson approved these changes Aug 21, 2025

View reviewed changes

lib/vm.py Show resolved Hide resolved

lib/vm.py Outdated Show resolved Hide resolved

Nambrok force-pushed the dtt-coalesce branch 2 times, most recently from 21673d2 to 1b92265 Compare August 21, 2025 11:49

Nambrok force-pushed the dtt-coalesce branch from 1b92265 to 8932208 Compare August 28, 2025 14:30

Nambrok added 2 commits August 28, 2025 16:38

Nambrok force-pushed the dtt-coalesce branch from 8932208 to 2019971 Compare August 28, 2025 14:38

ydirson merged commit 7d55cf3 into master Aug 28, 2025
9 checks passed

ydirson deleted the dtt-coalesce branch August 28, 2025 14:56

		if len(image_format) == 0:
		image_format = ["vhd"] # Not giving image-format will default to doing tests on vhd

		logging.info(f"<< Unplugging VDI {vdi_uuid} from Dom0")
		host.xe("vbd-unplug", {"uuid": vbd_uuid})

		def vdi_uuids(self, sr_uuid=None):
		def vdi_uuids(self, sr_uuid=None) -> List[str]:

feat(coalesce): Add tests for coalesce #290

feat(coalesce): Add tests for coalesce #290

Uh oh!

Conversation

Nambrok commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Nambrok commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

stormi Apr 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!

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 commented Apr 2, 2025

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!

Uh oh!

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!

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!

Uh oh!

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!

Nambrok commented Apr 2, 2025 •

edited

Loading

Nambrok commented Apr 2, 2025 •

edited

Loading

stormi Apr 2, 2025 •

edited

Loading