Merge pull request #242 from xcp-ng/install-driven-enhancements

Install-driven enhancements

README.md

@@ -4,23 +4,22 @@ Note: this is a perpertual work in progress. If you encounter any obstacles or b
 
 ## Main requirements
 * python >= 3.5
-* pytest >= 5.4 (python3 version)
-* xo-cli >= 0.17.0 installed, in the PATH, and registered to an instance of XO that will be used during the tests
+* packages as listed in requirements/base.txt
+* extra test-specific requirements are documented in the test file
+  "Requirements" header
 
 ### Quick install (python requirements)
 
 Install the python requirements using pip:
 
 ```
 $ pip install -r requirements/base.txt
-
 ```
 
 Additionally, for dev dependencies (things like the linter / style checker):
 
 ```
 $ pip install -r requirements/dev.txt
-
 ```
 
 ## Other requirements

conftest.py

@@ -19,20 +19,14 @@
 # need to import them in the global conftest.py so that they are recognized as fixtures.
 from pkgfixtures import formatted_and_mounted_ext4_disk, sr_disk_wiped
 
-# *** Support for incremental tests in test classes ***
-# From https://stackoverflow.com/questions/12411431/how-to-skip-the-rest-of-tests-in-the-class-if-one-has-failed
-def pytest_runtest_makereport(item, call):
-    if "incremental" in item.keywords:
-        if call.excinfo is not None:
-            parent = item.parent
-            parent._previousfailed = item
+# Do we cache VMs?
+try:
+    from data import CACHE_IMPORTED_VM
+except ImportError:
+    CACHE_IMPORTED_VM = False
+assert CACHE_IMPORTED_VM in [True, False]
 
-def pytest_runtest_setup(item):
-    previousfailed = getattr(item.parent, "_previousfailed", None)
-    if previousfailed is not None:
-        pytest.skip("previous test failed (%s)" % previousfailed.name)
-
-# *** End of: Support for incremental tests ***
+# pytest hooks
 
 def pytest_addoption(parser):
     parser.addoption(
@@ -85,6 +79,42 @@ def pytest_configure(config):
     global_config.ignore_ssh_banner = config.getoption('--ignore-ssh-banner')
     global_config.ssh_output_max_lines = int(config.getoption('--ssh-output-max-lines'))
 
+def pytest_generate_tests(metafunc):
+    if "vm_ref" in metafunc.fixturenames:
+        vms = metafunc.config.getoption("vm")
+        if not vms:
+            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")
+
+def pytest_collection_modifyitems(items, config):
+    # Automatically mark tests based on fixtures they require.
+    # Check pytest.ini or pytest --markers for marker descriptions.
+
+    markable_fixtures = [
+        'uefi_vm',
+        'unix_vm',
+        'windows_vm',
+        'hostA2',
+        'hostB1',
+        'sr_disk',
+        'sr_disk_4k'
+    ]
+
+    for item in items:
+        fixturenames = getattr(item, 'fixturenames', ())
+        for fixturename in markable_fixtures:
+            if fixturename in fixturenames:
+                item.add_marker(fixturename)
+
+        if 'vm_ref' not in fixturenames:
+            item.add_marker('no_vm')
+
+        if item.get_closest_marker('multi_vms'):
+            # multi_vms implies small_vm
+            item.add_marker('small_vm')
+
+# fixtures
+
 def setup_host(hostname_or_ip):
     pool = Pool(hostname_or_ip)
     h = pool.master
@@ -302,19 +332,12 @@ def vm_ref(request):
 
 @pytest.fixture(scope="module")
 def imported_vm(host, vm_ref):
-    # Do we cache VMs?
-    try:
-        from data import CACHE_IMPORTED_VM
-    except ImportError:
-        CACHE_IMPORTED_VM = False
-    assert CACHE_IMPORTED_VM in [True, False]
-
     if is_uuid(vm_ref):
         vm_orig = VM(vm_ref, host)
         name = vm_orig.name()
         logging.info(">> Reuse VM %s (%s) on host %s" % (vm_ref, name, host))
     else:
-        vm_orig = host.import_vm(vm_ref, host.main_sr(), use_cache=CACHE_IMPORTED_VM)
+        vm_orig = host.import_vm(vm_ref, host.main_sr_uuid(), use_cache=CACHE_IMPORTED_VM)
 
     if CACHE_IMPORTED_VM:
         # Clone the VM before running tests, so that the original VM remains untouched
@@ -332,18 +355,22 @@ def imported_vm(host, vm_ref):
         vm.destroy(verify=True)
 
 @pytest.fixture(scope="module")
-def running_vm(imported_vm):
+def started_vm(imported_vm):
     vm = imported_vm
-
     # may be already running if we skipped the import to use an existing VM
     if not vm.is_running():
         vm.start()
     wait_for(vm.is_running, '> Wait for VM running')
-    wait_for(vm.try_get_and_store_ip, "> Wait for VM IP")
-    wait_for(vm.is_ssh_up, "> Wait for VM SSH up")
+    wait_for(vm.try_get_and_store_ip, "> Wait for VM IP", timeout_secs=5 * 60)
     return vm
     # no teardown
 
+@pytest.fixture(scope="module")
+def running_vm(started_vm):
+    vm = started_vm
+    wait_for(vm.is_ssh_up, "> Wait for VM SSH up")
+    return vm
+
 @pytest.fixture(scope='module')
 def unix_vm(imported_vm):
     vm = imported_vm
@@ -415,37 +442,3 @@ def second_network(pytestconfig, host):
     if network_uuid == host.management_network():
         pytest.fail("--second-network must NOT be the management network")
     return network_uuid
-
-def pytest_generate_tests(metafunc):
-    if "vm_ref" in metafunc.fixturenames:
-        vms = metafunc.config.getoption("vm")
-        if not vms:
-            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")
-
-def pytest_collection_modifyitems(items, config):
-    # Automatically mark tests based on fixtures they require.
-    # Check pytest.ini or pytest --markers for marker descriptions.
-
-    markable_fixtures = [
-        'uefi_vm',
-        'unix_vm',
-        'windows_vm',
-        'hostA2',
-        'hostB1',
-        'sr_disk',
-        'sr_disk_4k'
-    ]
-
-    for item in items:
-        fixturenames = getattr(item, 'fixturenames', ())
-        for fixturename in markable_fixtures:
-            if fixturename in fixturenames:
-                item.add_marker(fixturename)
-
-        if 'vm_ref' not in fixturenames:
-            item.add_marker('no_vm')
-
-        if item.get_closest_marker('multi_vms'):
-            # multi_vms implies small_vm
-            item.add_marker('small_vm')

lib/basevm.py

@@ -2,7 +2,7 @@
 
 import lib.commands as commands
 
-from lib.common import _param_get, _param_remove, _param_set
+from lib.common import _param_add, _param_clear, _param_get, _param_remove, _param_set
 from lib.sr import SR
 
 class BaseVM:
@@ -16,13 +16,24 @@ def __init__(self, uuid, host):
         self.host = host
 
     def param_get(self, param_name, key=None, accept_unknown_key=False):
-        return _param_get(self.host, BaseVM.xe_prefix, self.uuid, param_name, key, accept_unknown_key)
+        return _param_get(self.host, self.xe_prefix, self.uuid,
+                          param_name, key, accept_unknown_key)
 
     def param_set(self, param_name, value, key=None):
-        _param_set(self.host, BaseVM.xe_prefix, self.uuid, param_name, value, key)
+        _param_set(self.host, self.xe_prefix, self.uuid,
+                   param_name, value, key)
 
     def param_remove(self, param_name, key, accept_unknown_key=False):
-        _param_remove(self.host, BaseVM.xe_prefix, self.uuid, param_name, key, accept_unknown_key)
+        _param_remove(self.host, self.xe_prefix, self.uuid,
+                      param_name, key, accept_unknown_key)
+
+    def param_add(self, param_name, value, key=None):
+        _param_add(self.host, self.xe_prefix, self.uuid,
+                   param_name, value, key)
+
+    def param_clear(self, param_name):
+        _param_clear(self.host, self.xe_prefix, self.uuid,
+                     param_name)
 
     def name(self):
         return self.param_get('name-label')

lib/commands.py

@@ -26,7 +26,7 @@ def __init__(self, returncode, stdout, cmd):
 class LocalCommandFailed(BaseCommandFailed):
     def __init__(self, returncode, stdout, cmd):
         msg_end = f": {stdout}" if stdout else "."
-        super(SSHCommandFailed, self).__init__(
+        super(LocalCommandFailed, self).__init__(
             returncode, stdout, cmd,
             f'Local command ({cmd}) failed with return code {returncode}{msg_end}'
         )
@@ -65,8 +65,8 @@ def _ellide_log_lines(log):
 OUPUT_LOGGER.addHandler(OUTPUT_HANDLER)
 OUTPUT_HANDLER.setFormatter(logging.Formatter('%(message)s'))
 
-def _ssh(hostname_or_ip, cmd, check=True, simple_output=True, suppress_fingerprint_warnings=True,
-         background=False, target_os='linux', decode=True, options=[]):
+def _ssh(hostname_or_ip, cmd, check, simple_output, suppress_fingerprint_warnings,
+         background, target_os, decode, options):
     opts = list(options)
     opts.append('-o "BatchMode yes"')
     if suppress_fingerprint_warnings:
@@ -205,6 +205,7 @@ def sftp(hostname_or_ip, cmds, check=True, suppress_fingerprint_warnings=True):
 
 def local_cmd(cmd, check=True, decode=True):
     """ Run a command locally on tester end. """
+    logging.debug("[local] %s", (cmd,))
     res = subprocess.run(
         cmd,
         stdout=subprocess.PIPE,

lib/host.py

@@ -200,21 +200,31 @@ def xo_server_reconnect(self):
         # is not enough to guarantee that the host object exists yet.
         wait_for(lambda: xo_object_exists(self.uuid), "Wait for XO to know about HOST %s" % self.uuid)
 
+    @staticmethod
+    def vm_cache_key(uri):
+        return f"[Cache for {uri}]"
+
+    def cached_vm(self, uri, sr_uuid):
+        assert sr_uuid, "A SR UUID is necessary to use import cache"
+        cache_key = self.vm_cache_key(uri)
+        # Look for an existing cache VM
+        vm_uuids = safe_split(self.xe('vm-list', {'name-description': cache_key}, minimal=True), ',')
+
+        for vm_uuid in vm_uuids:
+            vm = VM(vm_uuid, self)
+            # Make sure the VM is on the wanted SR.
+            # Assumption: if the first disk is on the SR, the VM is.
+            # If there's no VDI at all, then it is virtually on any SR.
+            if not vm.vdi_uuids() or vm.get_sr().uuid == sr_uuid:
+                logging.info(f"Reusing cached VM {vm.uuid} for {uri}")
+                return vm
+        logging.info("Could not find a VM in cache with key %r", cache_key)
+
     def import_vm(self, uri, sr_uuid=None, use_cache=False):
         if use_cache:
-            assert sr_uuid, "A SR UUID is necessary to use import cache"
-            cache_key = f"[Cache for {uri}]"
-            # Look for an existing cache VM
-            vm_uuids = safe_split(self.xe('vm-list', {'name-description': cache_key}, minimal=True), ',')
-
-            for vm_uuid in vm_uuids:
-                vm = VM(vm_uuid, self)
-                # Make sure the VM is on the wanted SR.
-                # Assumption: if the first disk is on the SR, the VM is.
-                # If there's no VDI at all, then it is virtually on any SR.
-                if not vm.vdi_uuids() or vm.get_sr().uuid == sr_uuid:
-                    logging.info(f"Reusing cached VM {vm.uuid} for {uri}")
-                    return vm
+            vm = self.cached_vm(uri, sr_uuid)
+            if vm:
+                return vm
 
         params = {}
         msg = "Import VM %s" % uri
@@ -227,14 +237,14 @@ def import_vm(self, uri, sr_uuid=None, use_cache=False):
             params['sr-uuid'] = sr_uuid
         logging.info(msg)
         vm_uuid = self.xe('vm-import', params)
-        logging.info("VM UUID: %s" % vm_uuid)
         vm_name = prefix_object_name(self.xe('vm-param-get', {'uuid': vm_uuid, 'param-name': 'name-label'}))
         vm = VM(vm_uuid, self)
         vm.param_set('name-label', vm_name)
         # Set VM VIF networks to the host's management network
         for vif in vm.vifs():
             vif.move(self.management_network())
         if use_cache:
+            cache_key = self.vm_cache_key(uri)
             logging.info(f"Marking VM {vm.uuid} as cached")
             vm.param_set('name-description', cache_key)
         return vm
@@ -460,7 +470,7 @@ def local_vm_srs(self):
                 srs.append(sr)
         return srs
 
-    def main_sr(self):
+    def main_sr_uuid(self):
         """ Main SR is either the default SR, or the first local SR, depending on data.py's DEFAULT_SR. """
         try:
             from data import DEFAULT_SR
@@ -480,6 +490,7 @@ def main_sr(self):
         else:
             sr_uuid = self.pool.param_get('default-SR')
             assert sr_uuid, f"DEFAULT_SR='default' so there must be a default SR on the pool of host {self}"
+        assert sr_uuid != "<not in database>"
         return sr_uuid
 
     def hostname(self):
@@ -509,6 +520,7 @@ def join_pool(self, pool):
             lambda: master.xe('host-param-get', {'uuid': self.uuid, 'param-name': 'enabled'}),
             f"Wait for pool {master} to see joined host {self} as enabled."
         )
+        self.pool = pool
 
     def activate_smapi_driver(self, driver):
         sm_plugins = self.ssh(['grep', '[[:space:]]*sm-plugins[[:space:]]*=[[:space:]]*', XAPI_CONF_FILE]).splitlines()

lib/vm.py

@@ -122,7 +122,7 @@ def wait_for_os_booted(self):
         # waiting for the IP:
         # - allows to make sure the OS actually started (on VMs that have the management agent)
         # - allows to store the IP for future use in the VM object
-        wait_for(self.try_get_and_store_ip, "Wait for VM IP")
+        wait_for(self.try_get_and_store_ip, "Wait for VM IP", timeout_secs=5 * 60)
         # now wait also for the management agent to have started
         wait_for(self.is_management_agent_up, "Wait for management agent up")
 
@@ -322,7 +322,7 @@ def tools_version(self):
         return "{major}.{minor}.{micro}-{build}".format(**version_dict)
 
     def file_exists(self, filepath):
-        """ Test that the file at filepath exists. """
+        """Returns True if the file exists, otherwise returns False."""
         return self.ssh_with_result(['test', '-f', filepath]).returncode == 0
 
     def detect_package_manager(self):
@@ -334,10 +334,15 @@ def detect_package_manager(self):
         else:
             return PackageManagerEnum.UNKNOWN
 
-    def mount_guest_tools_iso(self):
-        self.host.xe('vm-cd-insert', {'uuid': self.uuid, 'cd-name': 'guest-tools.iso'})
+    def insert_cd(self, vdi_name):
+        logging.info("Insert CD %r in VM %s", vdi_name, self.uuid)
+        self.host.xe('vm-cd-insert', {'uuid': self.uuid, 'cd-name': vdi_name})
+
+    def insert_guest_tools_iso(self):
+        self.insert_cd('guest-tools.iso')
 
-    def unmount_guest_tools_iso(self):
+    def eject_cd(self):
+        logging.info("Ejecting CD from VM %s", self.uuid)
         self.host.xe('vm-cd-eject', {'uuid': self.uuid})
 
     # *** Common reusable test fragments
@@ -429,10 +434,6 @@ def clear_uefi_variables(self):
         """
         self.param_remove('NVRAM', 'EFI-variables')
 
-    def file_exists(self, filepath):
-        """Returns True if the file exists, otherwise returns False."""
-        return self.ssh_with_result(['test', '-f', filepath]).returncode == 0
-
     def sign_bins(self):
         for f in self.get_all_efi_bins():
             self.sign(f)
@@ -476,7 +477,6 @@ def clone(self):
         name = self.name() + '_clone_for_tests'
         logging.info("Clone VM")
         uuid = self.host.xe('vm-clone', {'uuid': self.uuid, 'new-name-label': name})
-        logging.info("New VM: %s (%s)" % (uuid, name))
         return VM(uuid, self.host)
 
     def install_uefi_certs(self, auths):