From e1e6898572a560cdf00cab7f50ab4f260ba376fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Prchl=C3=ADk?= Date: Wed, 12 Jun 2024 17:07:20 +0200 Subject: [PATCH] Add support for multiple disks to testcloud plugin (#2767) ``` TMT_SHOW_TRACEBACK=1 tmt -vv run plans --default \ provision --how virtual --image fedora-39 \ --hardware 'disk[1].size=20GB' \ --hardware 'disk[2].size=15GB' \ login -s provision \ finish ``` Fixes https://github.com/teemtee/tmt/issues/2765 Co-authored-by: Martin Hoyer --- docs/releases.rst | 5 + tests/provision/virtual.testcloud/test.sh | 37 ++++- tests/unit/test_hardware.py | 43 ++++++ tmt/hardware.py | 17 ++- tmt/steps/provision/__init__.py | 75 ++++++++++- tmt/steps/provision/testcloud.py | 156 +++++++++++++++------- 6 files changed, 271 insertions(+), 62 deletions(-) diff --git a/docs/releases.rst b/docs/releases.rst index 5e020e94ac..b27d7a05cf 100644 --- a/docs/releases.rst +++ b/docs/releases.rst @@ -12,6 +12,11 @@ The :ref:`/spec/tests/duration` now supports multiplication. Added option ``--failed-only`` to the test attributes, enabling rerunning failed tests from previous runs. +The :ref:`virtual` provision +plugin gains support for adding multiple disks to guests, by adding +the corresponding ``disk[N].size`` +:ref:`HW requirements`. + tmt-1.33 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tests/provision/virtual.testcloud/test.sh b/tests/provision/virtual.testcloud/test.sh index 81f31390bd..a0353edd42 100755 --- a/tests/provision/virtual.testcloud/test.sh +++ b/tests/provision/virtual.testcloud/test.sh @@ -21,11 +21,11 @@ rlJournalStart rlAssertGrep "memory: 2048 MB" "$run/log.txt" rlAssertGrep "disk: 10 GB" "$run/log.txt" rlAssertGrep "effective hardware: variant #1: memory: == 2048 MB" "$run/log.txt" - rlAssertGrep "effective hardware: variant #1: disk.size: == 10 GB" "$run/log.txt" + rlAssertGrep "effective hardware: variant #1: disk\\[0\\].size: == 10 GB" "$run/log.txt" rlAssertGrep "memory: set to '2048 MB' because of 'memory: == 2048 MB'" "$run/log.txt" - rlAssertGrep "disk\\[0\\].size: set to '10 GB' because of 'disk.size: == 10 GB'" "$run/log.txt" + rlAssertGrep "disk\\[0\\].size: set to '10 GB' because of 'disk\\[0\\].size: == 10 GB'" "$run/log.txt" rlAssertGrep "final domain memory: 2048000" "$run/log.txt" - rlAssertGrep "final domain disk size: 10" "$run/log.txt" + rlAssertGrep "final domain root disk size: 10" "$run/log.txt" fi rlPhaseEnd @@ -43,11 +43,11 @@ rlJournalStart rlAssertGrep "memory: 2049 MB" "$run/log.txt" rlAssertGrep "disk: 11 GB" "$run/log.txt" rlAssertGrep "effective hardware: variant #1: memory: == 2049 MB" "$run/log.txt" - rlAssertGrep "effective hardware: variant #1: disk.size: == 11 GB" "$run/log.txt" + rlAssertGrep "effective hardware: variant #1: disk\\[0\\].size: == 11 GB" "$run/log.txt" rlAssertGrep "memory: set to '2049 MB' because of 'memory: == 2049 MB'" "$run/log.txt" - rlAssertGrep "disk\\[0\\].size: set to '11 GB' because of 'disk.size: == 11 GB'" "$run/log.txt" + rlAssertGrep "disk\\[0\\].size: set to '11 GB' because of 'disk\\[0\\].size: == 11 GB'" "$run/log.txt" rlAssertGrep "final domain memory: 2049000" "$run/log.txt" - rlAssertGrep "final domain disk size: 11" "$run/log.txt" + rlAssertGrep "final domain root disk size: 11" "$run/log.txt" fi rlPhaseEnd @@ -59,6 +59,31 @@ rlJournalStart fi rlPhaseEnd + rlPhaseStartTest "Provision a guest with multiple disks" + rlRun "cp $SRC_PLAN ." + + if ! rlRun "tmt run -i $run --scratch --all \ + provision -h virtual.testcloud \ + --hardware disk[1].size=22GB \ + --hardware disk[2].size=33GB \ + --connection system"; then + rlRun "cat $run/log.txt" 0 "Dump log.txt" + else + rlAssertGrep "effective hardware: variant #1: disk\\[0\\].size: == 10 GB" "$run/log.txt" + rlAssertGrep "effective hardware: variant #1: disk\\[1\\].size: == 22 GB" "$run/log.txt" + rlAssertGrep "effective hardware: variant #1: disk\\[2\\].size: == 33 GB" "$run/log.txt" + + rlAssertGrep "disk\\[0\\].size: set to '10 GB' because of 'disk\\[0\\].size: == 10 GB'" "$run/log.txt" + rlAssertGrep "disk\\[1\\].size: set to '22 GB' because of 'disk\\[1\\].size: == 22 GB'" "$run/log.txt" + rlAssertGrep "disk\\[2\\].size: set to '33 GB' because of 'disk\\[2\\].size: == 33 GB'" "$run/log.txt" + + rlAssertGrep "final domain root disk size: 10" "$run/log.txt" + rlAssertGrep "final domain disk #0 size: 10" "$run/log.txt" + rlAssertGrep "final domain disk #1 size: 22" "$run/log.txt" + rlAssertGrep "final domain disk #2 size: 33" "$run/log.txt" + fi + rlPhaseEnd + rlPhaseStartCleanup rlRun "popd" rlRun "rm -r $tmp" 0 "Remove tmp directory" diff --git a/tests/unit/test_hardware.py b/tests/unit/test_hardware.py index 1706c415a8..79ba7b37a8 100644 --- a/tests/unit/test_hardware.py +++ b/tests/unit/test_hardware.py @@ -4,6 +4,8 @@ import pytest import tmt.hardware +import tmt.steps +import tmt.steps.provision import tmt.utils from tmt.hardware import Hardware @@ -87,6 +89,47 @@ def test_constraint_components_pattern(value: str, expected: tuple[Any, Any]) -> assert match.groups() == expected +@pytest.mark.parametrize( + ('spec', 'expected_exc', 'expected_message'), + [ + ( + ('disk[1].size=15GB', 'disk.size=20GB'), + tmt.utils.SpecificationError, + r"^Hardware requirement 'disk\.size=20GB' lacks entry index \(disk\[N\]\)\.$" + ), + ( + ('network[1].type=eth', 'network.type=eth'), + tmt.utils.SpecificationError, + r"^Hardware requirement 'network\.type=eth' lacks entry index \(network\[N\]\)\.$" + ), + ( + ('disk=20GB',), + tmt.utils.SpecificationError, + r"^Hardware requirement 'disk=20GB' lacks child property \(disk\[N\].M\)\.$" + ), + ( + ('network=eth',), + tmt.utils.SpecificationError, + r"^Hardware requirement 'network=eth' lacks child property \(network\[N\].M\)\.$" + ), + ], + ids=[ + 'disk.size lacks index', + 'network.size lacks index', + 'disk lacks child property', + 'network lacks child property', + ] + ) +def test_normalize_invalid_hardware( + spec: tmt.hardware.Spec, + expected_exc: type[Exception], + expected_message: str, + root_logger + ) -> None: + with pytest.raises(expected_exc, match=expected_message): + tmt.steps.provision.normalize_hardware('', spec, root_logger) + + FULL_HARDWARE_REQUIREMENTS = """ boot: method: bios diff --git a/tmt/hardware.py b/tmt/hardware.py index d6890e36f2..fa8bb0fa34 100644 --- a/tmt/hardware.py +++ b/tmt/hardware.py @@ -154,6 +154,21 @@ class Operator(enum.Enum): $ # must match the whole string """, re.VERBOSE) + +#: A list of constraint names that operate over sequence of entities. +INDEXABLE_CONSTRAINTS: tuple[str, ...] = ( + 'disk', + 'network' + ) + +#: A list of constraint names that do not have child properties. +CHILDLESS_CONSTRAINTS: tuple[str, ...] = ( + 'arch', + 'memory', + 'hostname' + ) + + # Type of the operator callable. The operators accept two arguments, and return # a boolean evaluation of relationship of their two inputs. OperatorHandlerType = Callable[[Any, Any], bool] @@ -595,7 +610,7 @@ def printable_name(self) -> str: names: list[str] = [] - if components.peer_index: + if components.peer_index is not None: names.append(f'{components.name.replace("_", "-")}[{components.peer_index}]') else: diff --git a/tmt/steps/provision/__init__.py b/tmt/steps/provision/__init__.py index 77234a2078..f54c63acaa 100644 --- a/tmt/steps/provision/__init__.py +++ b/tmt/steps/provision/__init__.py @@ -1,5 +1,4 @@ import ast -import collections import dataclasses import datetime import enum @@ -493,25 +492,93 @@ def normalize_hardware( # From command line if isinstance(raw_hardware, (list, tuple)): - merged: collections.defaultdict[str, Any] = collections.defaultdict(dict) + merged: dict[str, Any] = {} for raw_datum in raw_hardware: components = tmt.hardware.ConstraintComponents.from_spec(raw_datum) - if components.name == 'cpu' and components.child_name == 'flag': + if components.name not in tmt.hardware.CHILDLESS_CONSTRAINTS \ + and components.child_name is None: + raise tmt.utils.SpecificationError( + f"Hardware requirement '{raw_datum}' lacks " + f"child property ({components.name}[N].M).") + + if components.name in tmt.hardware.INDEXABLE_CONSTRAINTS \ + and components.peer_index is None: + raise tmt.utils.SpecificationError( + f"Hardware requirement '{raw_datum}' lacks " + f"entry index ({components.name}[N]).") + + if components.peer_index is not None: + # This should not happen, the test above already ruled + # out `child_name` being `None`, but mypy does not know + # everything is fine. + assert components.child_name is not None # narrow type + + if components.name not in merged: + merged[components.name] = [] + + # Calculate the number of placeholders needed. + placeholders = components.peer_index - len(merged[components.name]) + 1 + + # Fill in empty spots between the existing ones and the + # one we're adding with placeholders. + if placeholders > 0: + merged[components.name].extend([{} for _ in range(placeholders)]) + + merged[components.name][components.peer_index][components.child_name] = \ + f'{components.operator} {components.value}' + + elif components.name == 'cpu' and components.child_name == 'flag': + if components.name not in merged: + merged[components.name] = [] + if 'flag' not in merged['cpu']: merged['cpu']['flag'] = [] merged['cpu']['flag'].append(f'{components.operator} {components.value}') elif components.child_name: + if components.name not in merged: + merged[components.name] = [] + merged[components.name][components.child_name] = \ f'{components.operator} {components.value}' else: + if components.name not in merged: + merged[components.name] = [] + merged[components.name] = f'{components.operator} {components.value}' - return tmt.hardware.Hardware.from_spec(dict(merged)) + # Very crude, we will need something better to handle `and` and + # `or` and nesting. + def _drop_placeholders(data: dict[str, Any]) -> dict[str, Any]: + new_data: dict[str, Any] = {} + + for key, value in data.items(): + if isinstance(value, list): + new_data[key] = [] + + for item in value: + if isinstance(item, dict) and not item: + continue + + new_data[key].append(item) + + else: + new_data[key] = value + + return new_data + + # TODO: if the index matters - and it does, because `disk[0]` is + # often a "root disk" - we need sparse list. Cannot prune + # placeholders now, because it would turn `disk[1]` into `disk[0]`, + # overriding whatever was set for the root disk. + # https://github.com/teemtee/tmt/issues/3004 for tracking. + # merged = _drop_placeholders(merged) + + return tmt.hardware.Hardware.from_spec(merged) # From fmf return tmt.hardware.Hardware.from_spec(raw_hardware) diff --git a/tmt/steps/provision/testcloud.py b/tmt/steps/provision/testcloud.py index d6f707b378..9681c649bf 100644 --- a/tmt/steps/provision/testcloud.py +++ b/tmt/steps/provision/testcloud.py @@ -1,11 +1,14 @@ +import collections import dataclasses import datetime +import itertools import os import platform import re import threading import types +from collections.abc import Iterator from typing import TYPE_CHECKING, Any, Optional, Union, cast import click @@ -431,6 +434,103 @@ def _apply_hw_tpm( domain.tpm_configuration = TPMConfiguration() +def _apply_hw_disk_size( + hardware: Optional[tmt.hardware.Hardware], + domain: 'DomainConfiguration', + logger: tmt.log.Logger) -> None: + """ Apply ``disk`` constraint to given VM domain """ + + final_size: 'Size' = DEFAULT_DISK + + def _generate_disk_filepaths() -> Iterator[Path]: + """ Generate paths to use for files representing VM storage """ + + # Start with the path already decided by testcloud... + yield Path(domain.local_disk) + + # ... and use it as a basis for remaining paths. + for i in itertools.count(1, 1): + yield Path(f'{domain.local_disk}.{i}') + + disk_filepath_generator = _generate_disk_filepaths() + + if not hardware or not hardware.constraint: + logger.debug( + 'disk[0].size', + f"set to '{final_size}' because of no constraints", + level=4) + + domain.storage_devices = [ + QCow2StorageDevice( + str(next(disk_filepath_generator)), + int(final_size.to('GB').magnitude)) + ] + + return + + variant = hardware.constraint.variant() + + # Collect all `disk.size` constraints, ignore the rest. + disk_size_constraints = [ + constraint + for constraint in variant + if isinstance(constraint, tmt.hardware.SizeConstraint) + and constraint.expand_name().name == 'disk' + and constraint.expand_name().child_name == 'size'] + + if not disk_size_constraints: + logger.debug( + 'disk[0].size', + f"set to '{final_size}' because of no 'disk.size' constraints", + level=4) + + domain.storage_devices = [ + QCow2StorageDevice( + str(next(disk_filepath_generator)), + int(final_size.to('GB').magnitude)) + ] + + return + + # Now sort them into groups by their `peer_index`, i.e. `disk[0]`, + # `disk[1]` and so on. + by_peer_index: dict[int, list[tmt.hardware.SizeConstraint]] = collections.defaultdict(list) + + for constraint in disk_size_constraints: + if constraint.operator not in ( + tmt.hardware.Operator.EQ, + tmt.hardware.Operator.GTE, + tmt.hardware.Operator.LTE): + raise ProvisionError( + f"Cannot apply hardware requirement '{constraint}', operator not supported.") + + components = constraint.expand_name() + + assert components.peer_index is not None # narrow type + + by_peer_index[components.peer_index].append(constraint) + + # Process each disk and its constraints, construct the + # corresponding storage device, and the last constraint wins + # & sets its size. + for peer_index in sorted(by_peer_index.keys()): + final_size = DEFAULT_DISK + + for constraint in by_peer_index[peer_index]: + logger.debug( + f'disk[{peer_index}].size', + f"set to '{constraint.value}' because of '{constraint}'", + level=4) + + final_size = constraint.value + + domain.storage_devices.append( + QCow2StorageDevice( + str(next(disk_filepath_generator)), + int(final_size.to('GB').magnitude)) + ) + + class GuestTestcloud(tmt.GuestSsh): """ Testcloud Instance @@ -690,53 +790,6 @@ def _apply_hw_memory(self, domain: 'DomainConfiguration') -> None: domain.memory_size = int(constraint.value.to('kB').magnitude) - def _apply_hw_disk_size(self, domain: 'DomainConfiguration') -> 'QCow2StorageDevice': - """ Apply ``disk`` constraint to given VM domain """ - - final_size: 'Size' = DEFAULT_DISK - - if not self.hardware or not self.hardware.constraint: - self.debug( - 'disk[0].size', - f"set to '{final_size}' because of no constraints", - level=4) - - return QCow2StorageDevice(domain.local_disk, int(final_size.to('GB').magnitude)) - - variant = self.hardware.constraint.variant() - - disk_size_constraints = [ - constraint - for constraint in variant - if isinstance(constraint, tmt.hardware.SizeConstraint) - and constraint.expand_name().name == 'disk' - and constraint.expand_name().child_name == 'size'] - - if not disk_size_constraints: - self.debug( - 'disk[0].size', - f"set to '{final_size}' because of no 'disk.size' constraints", - level=4) - - return QCow2StorageDevice(domain.local_disk, int(final_size.to('GB').magnitude)) - - for constraint in disk_size_constraints: - if constraint.operator not in ( - tmt.hardware.Operator.EQ, - tmt.hardware.Operator.GTE, - tmt.hardware.Operator.LTE): - raise ProvisionError( - f"Cannot apply hardware requirement '{constraint}', operator not supported.") - - self.debug( - 'disk[0].size', - f"set to '{constraint.value}' because of '{constraint}'", - level=4) - - final_size = constraint.value - - return QCow2StorageDevice(domain.local_disk, int(final_size.to('GB').magnitude)) - def _apply_hw_arch(self, domain: 'DomainConfiguration', kvm: bool, legacy_os: bool) -> None: if self.arch == "x86_64": domain.system_architecture = X86_64ArchitectureConfiguration( @@ -826,11 +879,14 @@ def start(self) -> None: self._logger.debug('effective hardware', line, level=4) self._apply_hw_memory(self._domain) - storage_image = self._apply_hw_disk_size(self._domain) + _apply_hw_disk_size(self.hardware, self._domain, self._logger) _apply_hw_tpm(self.hardware, self._domain, self._logger) self.debug('final domain memory', str(self._domain.memory_size)) - self.debug('final domain disk size', str(storage_image.size)) + self.debug('final domain root disk size', str(self._domain.storage_devices[0].size)) + + for i, device in enumerate(self._domain.storage_devices): + self.debug(f'final domain disk #{i} size', str(device.size)) # Is this a CoreOS? self._domain.coreos = self.is_coreos @@ -852,8 +908,6 @@ def start(self) -> None: else: raise tmt.utils.ProvisionError("Only system, or session connection is supported.") - self._domain.storage_devices.append(storage_image) - if not self._domain.coreos: seed_disk = RawStorageDevice(self._domain.seed_path) self._domain.storage_devices.append(seed_disk)