Skip to content

Storage Tests for qcow2 vdi image format #339

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Storage Tests for qcow2 vdi image format #339

wants to merge 5 commits into from
+134 −24

Conversation

rushikeshjadhav
Copy link
Contributor

@rushikeshjadhav rushikeshjadhav commented Jul 8, 2025 •
edited
Loading

Incorporating image-format fixture into following SR types for storage tests

  • ext
  • lvm
  • xfs
  • zfs
  • largeblock

@rushikeshjadhav rushikeshjadhav marked this pull request as draft July 8, 2025 12:26
@rushikeshjadhav rushikeshjadhav requested a review from Nambrok July 10, 2025 12:21
@rushikeshjadhav rushikeshjadhav force-pushed the storage-qcow2 branch 2 times, most recently from 1d35ab2 to da6c5d5 Compare September 18, 2025 17:35
@rushikeshjadhav rushikeshjadhav marked this pull request as ready for review September 18, 2025 17:41
@rushikeshjadhav rushikeshjadhav mentioned this pull request Sep 18, 2025
Closed
Millefeuille42
Millefeuille42 requested changes Sep 23, 2025
View reviewed changes
@glehmann glehmann self-requested a review September 30, 2025 13:06
Wescoeur
Wescoeur requested changes Sep 30, 2025
View reviewed changes
tests/storage/zfsvol/test_zfsvol_sr.py Outdated
Comment on lines 26 to 29
sr = host.sr_create('zfs-vol', "ZFS-local-SR-test", {
'device': '/dev/' + sr_disk_wiped,
'preferred-image-formats': image_format
}, verify=True)
Copy link
Member

@Wescoeur Wescoeur Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test it @rushikeshjadhav ? We don't support QCOW2 with ZFS vol driver (SMAPIv3 plugin).
So we must not add image_format for this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, although the test passes, its volume is not of qcow2 format. Will remove it and add additional checks for qcow2 VDI validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added additional check for qcow2 VDI validation as well with test_vdi_image_format.

@rushikeshjadhav rushikeshjadhav force-pushed the storage-qcow2 branch 3 times, most recently from 38db6d5 to 79264c0 Compare October 1, 2025 14:32
@rushikeshjadhav rushikeshjadhav marked this pull request as draft October 1, 2025 14:37
glehmann
glehmann reviewed Oct 1, 2025
View reviewed changes
@rushikeshjadhav rushikeshjadhav marked this pull request as ready for review October 2, 2025 12:35
@glehmann
Copy link
Member

glehmann commented Oct 2, 2025

The CI workflow is not run on the PR. We should run it before merging.

@glehmann
Copy link
Member

glehmann commented Oct 2, 2025

There are a few problems to fix:

❯ mypy --install-types --non-interactive lib/ conftest.py pkgfixtures.py tests/
  pyright lib/ conftest.py pkgfixtures.py
  ruff check lib/ tests/
  flake8
tests/install/test.py:88: error: Value of type "object" is not indexable  [index]
tests/install/test.py:342: error: Value of type "object" is not indexable  [index]
Found 2 errors in 1 file (checked 191 source files)
WARNING: there is a new pyright version available (v1.1.402 -> v1.1.406).
Please install the new version or set PYRIGHT_PYTHON_FORCE_VERSION to `latest`

0 errors, 0 warnings, 0 informations
All checks passed!
./tests/storage/largeblock/conftest.py:17:19: E128 continuation line under-indented for visual indent

@glehmann
Copy link
Member

glehmann commented Oct 2, 2025

@rushikeshjadhav the CI should run when using a branch directly in this repository.

stormi
stormi reviewed Oct 2, 2025
View reviewed changes
Copy link
Member

@stormi stormi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically installing packages that support qcow2 could have been a good idea, but here it might cause more friction than it avoids.

@rushikeshjadhav
Copy link
Contributor Author

@rushikeshjadhav the CI should run when using a branch directly in this repository.

I have pushed to https://github.com/xcp-ng/xcp-ng-tests/tree/storage-qcow2, will check on CI.

@rushikeshjadhav
Copy link
Contributor Author

@rushikeshjadhav the CI should run when using a branch directly in this repository.

I have pushed to https://github.com/xcp-ng/xcp-ng-tests/tree/storage-qcow2, will check on CI.

This is found to be working. Kindly see @Wescoeur @Nambrok if it can be merged.

Nambrok
Nambrok requested changes Oct 14, 2025
View reviewed changes
tests/storage/zfs/test_zfs_sr.py Outdated
assert not vdi_is_open(vdi_on_zfs_sr)

def test_vdi_image_format(self, vdi_on_zfs_sr: VDI, image_format: str):
assert vdi_on_zfs_sr.get_image_format() == image_format
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against it but it will not work on a "normal" sm version. The image-format sm-config key is only available with a QCOW2 sm version.
In case of a normal sm, it will give you None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. We would like the tests to pass with current sm if the configuration asks to just test for VHD.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Nambrok, do we plan to add the sm-config key to the next SM version? If not, is there a reason to remove it?

If we don’t want to continue using the sm-config key, would reading the header of a VHD or QCOW2 file from the actual file or volume be a suitable alternative?

This seems an important test, as in current sm, the driver creates a VHD even when requested for a QCOW2 file, and all tests pass, which they shouldn’t.

Copy link
Member

@stormi stormi Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just skip the test if sm doesn't support QCOW2? We need to think in two timelines at once. The current one, with sm not supporting QCOW2 nor image-format, and the future one, where they're supported. If we want to merge this PR to master, we need both to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, have made changes to support both SM and skip test if it's not applicable.

tests/storage/xfs/conftest.py Outdated
@pytest.fixture(scope='package')
def xfs_sr(unused_512B_disks: dict[Host, list[Host.BlockDeviceInfo]], host_with_xfsprogs: Host
def xfs_sr(unused_512B_disks: dict[Host,
list[Host.BlockDeviceInfo]],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer the type to be on the same line as the variable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glehmann glehmann glehmann approved these changes

@Millefeuille42 Millefeuille42 Millefeuille42 approved these changes

@Wescoeur Wescoeur Awaiting requested review from Wescoeur

@Nambrok Nambrok Awaiting requested review from Nambrok

@stormi stormi Awaiting requested review from stormi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@rushikeshjadhav @glehmann @stormi @Wescoeur @Nambrok @Millefeuille42