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 #2765

Co-authored-by: Martin Hoyer <[email protected]>

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<plugins/provision/virtual.testcloud>` provision
+plugin gains support for adding multiple disks to guests, by adding
+the corresponding ``disk[N].size``
+:ref:`HW requirements</spec/hardware/disk>`.
+
 
 tmt-1.33
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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"

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

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:

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)