-
Notifications
You must be signed in to change notification settings - Fork 6
feat(coalesce): Add tests for coalesce #290
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,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", | ||
action="append", | ||
default=[], | ||
help="Format of VDI to execute tests on." | ||
"Example: vhd,qcow2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pytest already has test selection mechanisms:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as the parameter is not necessary in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At some point to enable "qcow2", this will be required to make modifications in @Nambrok while doing similar for
So here in this case, if we do -
and
the tests can be upgraded for new types in future for supported SR. As the whole SR tests needs to be tested for both I'm fine with both approach, to get this added. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/xcp-ng/xcp-ng-tests/pull/290/files#diff-a31c7ed5d35f5ed8233994868c54d625b18e6bacb6794344c4531e62bd9dde59R111 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works, I think we can move ahead with this merge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Well, there is still |
||
) | ||
|
||
def pytest_configure(config): | ||
global_config.ignore_ssh_banner = config.getoption('--ignore-ssh-banner') | ||
|
@@ -106,6 +113,12 @@ def pytest_generate_tests(metafunc): | |
vms = [None] # no --vm parameter does not mean skip the test, for us, it means use the default | ||
metafunc.parametrize("vm_ref", vms, indirect=True, scope="module") | ||
|
||
if "image_format" in metafunc.fixturenames: | ||
image_format = metafunc.config.getoption("image_format") | ||
if len(image_format) == 0: | ||
image_format = ["vhd"] # Not giving image-format will default to doing tests on vhd | ||
Comment on lines
+118
to
+119
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "vhd,qcow2" are considered as one single option by pytest when passing like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, but it's because different input are made into a list so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's cleaner to just use |
||
metafunc.parametrize("image_format", image_format, scope="session") | ||
|
||
def pytest_collection_modifyitems(items, config): | ||
# Automatically mark tests based on fixtures they require. | ||
# Check pytest.ini or pytest --markers for marker descriptions. | ||
|
@@ -304,14 +317,21 @@ def host_no_ipv6(host): | |
if is_ipv6(host.hostname_or_ip): | ||
pytest.skip(f"This test requires an IPv4 XCP-ng") | ||
|
||
@pytest.fixture(scope="session") | ||
def shared_sr(host): | ||
sr = host.pool.first_shared_sr() | ||
assert sr, "No shared SR available on hosts" | ||
logging.info(">> Shared SR on host present: {} of type {}".format(sr.uuid, sr.get_type())) | ||
yield sr | ||
|
||
@pytest.fixture(scope='session') | ||
def local_sr_on_hostA1(hostA1): | ||
""" A local SR on the pool's master. """ | ||
srs = hostA1.local_vm_srs() | ||
assert len(srs) > 0, "a local SR is required on the pool's master" | ||
# use the first local SR found | ||
sr = srs[0] | ||
logging.info(">> local SR on hostA1 present : %s" % sr.uuid) | ||
logging.info(">> local SR on hostA1 present: {} of type {}".format(sr.uuid, sr.get_type())) | ||
yield sr | ||
|
||
@pytest.fixture(scope='session') | ||
|
@@ -321,7 +341,7 @@ def local_sr_on_hostA2(hostA2): | |
assert len(srs) > 0, "a local SR is required on the pool's second host" | ||
# use the first local SR found | ||
sr = srs[0] | ||
logging.info(">> local SR on hostA2 present : %s" % sr.uuid) | ||
logging.info(">> local SR on hostA2 present: {} of type {}".format(sr.uuid, sr.get_type())) | ||
yield sr | ||
|
||
@pytest.fixture(scope='session') | ||
|
@@ -331,7 +351,7 @@ def local_sr_on_hostB1(hostB1): | |
assert len(srs) > 0, "a local SR is required on the second pool's master" | ||
# use the first local SR found | ||
sr = srs[0] | ||
logging.info(">> local SR on hostB1 present : %s" % sr.uuid) | ||
logging.info(">> local SR on hostB1 present: {} of type {}".format(sr.uuid, sr.get_type())) | ||
yield sr | ||
|
||
@pytest.fixture(scope='session') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,24 @@ | |
import time | ||
|
||
import lib.commands as commands | ||
from lib.common import prefix_object_name, safe_split, strtobool, wait_for, wait_for_not | ||
from lib.common import ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure why this is reformatted |
||
prefix_object_name, | ||
safe_split, | ||
strtobool, | ||
wait_for, | ||
wait_for_not, | ||
) | ||
from lib.vdi import VDI | ||
|
||
from typing import Optional | ||
|
||
class SR: | ||
def __init__(self, uuid, pool): | ||
self.uuid = uuid | ||
self.pool = pool | ||
self._is_shared = None # cached value for is_shared() | ||
self._main_host = None # cached value for main_host() | ||
self._type = None # cache value for get_type() | ||
|
||
def pbd_uuids(self): | ||
return safe_split(self.pool.master.xe('pbd-list', {'sr-uuid': self.uuid}, minimal=True)) | ||
|
@@ -152,13 +161,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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO I think having own decorator to handle 0 arg (const_cache?) or continuing with an explicit private variable is better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😉 |
||
self._type = self.pool.master.xe("sr-param-get", {"uuid": self.uuid, "param-name": "type"}) | ||
return self._type | ||
|
||
def create_vdi(self, name_label: str, virtual_size: int = 64, image_format: Optional[str] = None) -> VDI: | ||
logging.info("Create VDI %r on SR %s", name_label, self.uuid) | ||
vdi_uuid = self.pool.master.xe('vdi-create', { | ||
args = { | ||
'name-label': prefix_object_name(name_label), | ||
'virtual-size': str(virtual_size), | ||
'sr-uuid': self.uuid | ||
}) | ||
'sr-uuid': self.uuid, | ||
} | ||
if image_format: | ||
args["sm-config:image-format"] = image_format | ||
vdi_uuid = self.pool.master.xe('vdi-create', args) | ||
return VDI(vdi_uuid, sr=self) | ||
|
||
def run_quicktest(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
import pytest | ||
|
||
import logging | ||
|
||
from lib.vdi import VDI | ||
|
||
MAX_LENGTH = 1 * 1024 * 1024 * 1024 # 1GiB | ||
|
||
@pytest.fixture(scope="module") | ||
def vdi_on_local_sr(host, local_sr_on_hostA1, image_format): | ||
sr = local_sr_on_hostA1 | ||
vdi = sr.create_vdi("testVDI", MAX_LENGTH, image_format=image_format) | ||
logging.info(">> Created VDI {} of type {}".format(vdi.uuid, image_format)) | ||
|
||
yield vdi | ||
|
||
logging.info("<< Destroying VDI {}".format(vdi.uuid)) | ||
vdi.destroy() | ||
|
||
@pytest.fixture(scope="module") | ||
def vdi_with_vbd_on_dom0(host, vdi_on_local_sr): | ||
dom0 = host.get_dom0_vm() | ||
dom0.connect_vdi(vdi_on_local_sr) | ||
|
||
yield vdi_on_local_sr | ||
|
||
dom0.disconnect_vdi(vdi_on_local_sr) | ||
|
||
@pytest.fixture(scope="function") | ||
def data_file_on_host(host): | ||
filename = "/root/data.bin" | ||
logging.info(f">> Creating data file {filename} on host") | ||
size = 1 * 1024 * 1024 # 1MiB | ||
assert size <= MAX_LENGTH, "Size of the data file bigger than the VDI size" | ||
|
||
host.ssh(["dd", "if=/dev/urandom", f"of={filename}", f"bs={size}", "count=1"]) | ||
|
||
yield filename | ||
|
||
logging.info("<< Deleting data file") | ||
host.ssh(["rm", filename]) | ||
|
||
@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. | ||
""" | ||
Comment on lines
+43
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does not really explain how it is useful for testing. Maybe naming it like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be mentioned somewhere that tapdev is used for a better test performance, to avoid instantiating a new VM to access the vdi |
||
sr_uuid = local_sr_on_hostA1.uuid | ||
vdi_uuid = vdi_with_vbd_on_dom0.uuid | ||
yield f"/dev/sm/backend/{sr_uuid}/{vdi_uuid}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andformat
can be many things.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And
vdi-type
looked consistent withsm
's terminology 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really consistent. In this example here, type is completly different.
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
.type
is completely ambiguous depending on what you are talking about and the context in which it is used.volume
instead). Andimage-format
is used.