From 3baff21fe4a9442cfce6a7be1f3f209fce7e8b30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20Prchl=C3=ADk?= Date: Mon, 13 May 2024 13:13:20 +0200 Subject: [PATCH] Simplify parsing of HW requirements and add missing ones It turned out some requirements were left out and nothing was parsing them. The "maximal" unit test was incorrect and did not report this. The patch * adds parsing for missing requirement groups (`gpu`, `device`), * several individual requirements were also missing, * simplifies parsing of groups of constraints, providing helpers to avoid repetition, * and fixes the test guarding this. --- tests/unit/provision/mrack/test_hw.py | 90 ++++---- tests/unit/test_hardware.py | 181 +++++++++++---- tmt/hardware.py | 317 +++++++++++++++----------- 3 files changed, 358 insertions(+), 230 deletions(-) diff --git a/tests/unit/provision/mrack/test_hw.py b/tests/unit/provision/mrack/test_hw.py index 2365a03a22..27ae4c2375 100644 --- a/tests/unit/provision/mrack/test_hw.py +++ b/tests/unit/provision/mrack/test_hw.py @@ -20,6 +20,8 @@ operator_to_beaker_op, ) +from ...test_hardware import FULL_HARDWARE_REQUIREMENTS + @pytest.mark.parametrize( ('operator', 'value', 'expected'), @@ -42,52 +44,7 @@ def test_operator_to_beaker_op( def test_maximal_constraint(root_logger: Logger) -> None: - hw_spec = """ - boot: - method: bios - compatible: - distro: - - rhel-7 - - rhel-8 - cpu: - sockets: "<= 1" - cores: 2 - threads: ">= 8" - cores-per-socket: "= 2" - threads-per-core: "== 4" - processors: "> 8" - model: 62 - model-name: "!~ Haswell" - family: "< 6" - family-name: Skylake - flag: - - avx - - "= avx2" - - "!= smep" - disk: - - size: 40 GiB - model-name: "PERC H310" - - size: 120 GiB - driver: mpt3sas - gpu: - device-name: G86 [Quadro NVS 290] - hostname: "~ .*.foo.redhat.com" - memory: 8 GiB - network: - - type: eth - - type: eth - tpm: - version: "2.0" - virtualization: - is-supported: true - is-virtualized: false - hypervisor: "~ xen" - zcrypt: - adapter: "CEX8C" - mode: "CCA" - """ - - hw = Hardware.from_spec(tmt.utils.yaml_to_dict(textwrap.dedent(hw_spec))) + hw = Hardware.from_spec(tmt.utils.yaml_to_dict(textwrap.dedent(FULL_HARDWARE_REQUIREMENTS))) assert hw.constraint is not None result = constraint_to_beaker_filter(hw.constraint, root_logger) @@ -135,6 +92,8 @@ def test_maximal_constraint(root_logger: Logger) -> None: }, {'or': []}, {'or': []}, + {'or': []}, + {'or': []}, { 'not': { @@ -146,6 +105,7 @@ def test_maximal_constraint(root_logger: Logger) -> None: } }, }, + {'or': []}, { 'and': [ { @@ -176,6 +136,15 @@ def test_maximal_constraint(root_logger: Logger) -> None: } ] }, + { + 'and': [ + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []} + ] + }, { 'system': { 'memory': { @@ -199,8 +168,8 @@ def test_maximal_constraint(root_logger: Logger) -> None: { 'disk': { 'model': { - '_op': '==', - '_value': 'PERC H310' + '_op': 'like', + '_value': 'WD 100G%' } } } @@ -220,7 +189,7 @@ def test_maximal_constraint(root_logger: Logger) -> None: 'key_value': { '_key': 'BOOTDISK', '_op': '==', - '_value': 'mpt3sas' + '_value': 'virtblk' } } ] @@ -229,9 +198,18 @@ def test_maximal_constraint(root_logger: Logger) -> None: }, { 'and': [ - {'or': []}, + { + 'and': [ + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []} + ] + }, {'or': []} - ] + ], }, { 'hostname': { @@ -240,6 +218,16 @@ def test_maximal_constraint(root_logger: Logger) -> None: } }, {'or': []}, + { + 'and': [ + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []}, + {'or': []} + ] + }, + {'or': []}, { 'and': [ { diff --git a/tests/unit/test_hardware.py b/tests/unit/test_hardware.py index ff4bd4982d..1706c415a8 100644 --- a/tests/unit/test_hardware.py +++ b/tests/unit/test_hardware.py @@ -87,59 +87,144 @@ def test_constraint_components_pattern(value: str, expected: tuple[Any, Any]) -> assert match.groups() == expected +FULL_HARDWARE_REQUIREMENTS = """ + boot: + method: bios + compatible: + distro: + - rhel-7 + - rhel-8 + cpu: + sockets: "<= 1" + cores: 2 + threads: ">= 8" + cores-per-socket: "= 2" + threads-per-core: "== 4" + processors: "> 8" + model: 62 + model-name: "!~ Haswell" + family: "< 6" + family-name: Skylake + vendor-name: "~ Intel.*" + vendor: == 0x8086 + stepping: "!= 10" + flag: + - avx + - "= avx2" + - "!= smep" + disk: + - size: 40 GiB + model-name: "~ WD 100G.*" + - size: 120 GiB + driver: virtblk + gpu: + device-name: G86 [Quadro NVS 290] + device: "97" + vendor-name: 'Nvidia' + vendor: 0x10de + driver: "~radeon" + hostname: "~ .*.foo.redhat.com" + location: + lab-controller: "!= lab-1.bar.redhat.com" + memory: 8 GiB + network: + - type: eth + vendor: "!= 0x79" + vendor-name: ~ ^Broadcom + device-name: ~ ^NetXtreme II BCM + device: 1657 + driver: iwlwifi + - type: eth + system: + vendor: 0x413C + vendor-name: "~ Dell.*" + model: 79 + model-name: "~ PowerEdge R750" + numa-nodes: "< 4" + tpm: + version: "2.0" + virtualization: + is-supported: true + is-virtualized: false + hypervisor: "~ xen" + zcrypt: + adapter: "CEX8C" + mode: "CCA" +""" + + def test_parse_maximal_constraint() -> None: - hw_spec = """ - boot: - method: bios - compatible: - distro: - - rhel-7 - - rhel-8 - cpu: - sockets: "<= 1" - cores: 2 - threads: ">= 8" - cores-per-socket: "= 2" - threads-per-core: "== 4" - processors: "> 8" - model: 62 - model-name: "!~ Haswell" - family: "< 6" - family-name: Skylake - flag: - - avx - - "= avx2" - - "!= smep" - disk: - - size: 40 GiB - - size: 120 GiB - gpu: - device-name: G86 [Quadro NVS 290] - hostname: "~ .*.foo.redhat.com" - location: - lab-controller: "!= lab-1.bar.redhat.com" - memory: 8 GiB - network: - - type: eth - - type: eth - system: - vendor: 0x413C - vendor-name: "~ Dell.*" - model: 79 - model-name: "~ PowerEdge R750" - numa-nodes: "< 4" - tpm: - version: "2.0" - virtualization: - is-supported: true - is-virtualized: false - hypervisor: "~ xen" + hw_spec_out = """ + and: + - boot.method: contains bios + - and: + - compatible.distro: contains rhel-7 + - compatible.distro: contains rhel-8 + - and: + - cpu.processors: '> 8' + - cpu.sockets: <= 1 + - cpu.cores: == 2 + - cpu.threads: '>= 8' + - cpu.cores-per-socket: == 2 + - cpu.threads-per-core: == 4 + - cpu.model: == 62 + - cpu.family: < 6 + - cpu.vendor: == 32902 + - cpu.stepping: '!= 10' + - cpu.family-name: == Skylake + - cpu.model-name: '!~ Haswell' + - cpu.vendor-name: ~ Intel.* + - and: + - cpu.flag: contains avx + - cpu.flag: contains avx2 + - cpu.flag: not contains smep + - and: + - gpu.vendor: == 4318 + - gpu.device: == 97 + - gpu.vendor-name: == Nvidia + - gpu.device-name: == G86 [Quadro NVS 290] + - gpu.driver: ~ radeon + - memory: == 8 GiB + - and: + - and: + - disk[0].size: == 40 GiB + - disk[0].model-name: ~ WD 100G.* + - and: + - disk[1].size: == 120 GiB + - disk[1].driver: == virtblk + - and: + - and: + - network[0].vendor: '!= 121' + - network[0].device: == 1657 + - network[0].vendor-name: ~ ^Broadcom + - network[0].device-name: ~ ^NetXtreme II BCM + - network[0].driver: == iwlwifi + - network[0].type: == eth + - network[1].type: == eth + - hostname: ~ .*.foo.redhat.com + - location.lab-controller: '!= lab-1.bar.redhat.com' + - and: + - system.vendor: == 16700 + - system.vendor-name: ~ Dell.* + - system.model: == 79 + - system.numa-nodes: < 4 + - system.model-name: ~ PowerEdge R750 + - tpm.version: == 2.0 + - and: + - virtualization.is-virtualized: == False + - virtualization.is-supported: == True + - virtualization.hypervisor: ~ xen + - and: + - zcrypt.adapter: == CEX8C + - zcrypt.mode: == CCA """ - hw = parse_hw(hw_spec) + hw = parse_hw(FULL_HARDWARE_REQUIREMENTS) assert hw.constraint is not None + print(hw.to_spec()) print(tmt.utils.dict_to_yaml(hw.constraint.to_spec())) + print(textwrap.dedent(hw_spec_out)) - assert hw.to_spec() == tmt.utils.yaml_to_dict(hw_spec) + assert tmt.utils.dict_to_yaml(hw.constraint.to_spec()) == textwrap.dedent(hw_spec_out).lstrip() diff --git a/tmt/hardware.py b/tmt/hardware.py index 624413e46e..0771c8b6a5 100644 --- a/tmt/hardware.py +++ b/tmt/hardware.py @@ -50,7 +50,7 @@ import tmt.log import tmt.utils -from tmt.utils import SpecBasedContainer +from tmt.utils import SpecBasedContainer, SpecificationError if TYPE_CHECKING: from pint import Quantity @@ -698,11 +698,26 @@ def from_specification( original_constraint: Optional['Constraint[Any]'] = None, allowed_operators: Optional[list[Operator]] = None ) -> T: + + def _cast_number(raw_value: Any) -> int: + if isinstance(raw_value, int): + return raw_value + + if isinstance(raw_value, str): + raw_value = raw_value.strip() + + if raw_value.startswith('0x'): + return int(raw_value, base=16) + + return int(raw_value) + + raise SpecificationError(f"Could not convert '{raw_value}' to a number.") + return cls._from_specification( name, raw_value, as_quantity=False, - as_cast=int, + as_cast=_cast_number, original_constraint=original_constraint, allowed_operators=allowed_operators ) @@ -875,6 +890,106 @@ def wrapper(spec: Spec, index: int) -> BaseConstraint: return wrapper +def _parse_number_constraints( + spec: Spec, + prefix: str, + constraint_keys: tuple[str, ...]) -> list[BaseConstraint]: + """ Parse number-like constraints defined by a given set of keys """ + + return [ + NumberConstraint.from_specification( + f'{prefix}.{constraint_name.replace("-", "_")}', + str(spec[constraint_name]), + allowed_operators=[ + Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE]) + for constraint_name in constraint_keys + if constraint_name in spec + ] + + +def _parse_size_constraints( + spec: Spec, + prefix: str, + constraint_keys: tuple[str, ...]) -> list[BaseConstraint]: + """ Parse size-like constraints defined by a given set of keys """ + + return [ + SizeConstraint.from_specification( + f'{prefix}.{constraint_name}', + str(spec[constraint_name]), + allowed_operators=[ + Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE]) + for constraint_name in constraint_keys + if constraint_name in spec + ] + + +def _parse_text_constraints( + spec: Spec, + prefix: str, + constraint_keys: tuple[str, ...], + allowed_operators: Optional[tuple[Operator, ...]] = None) -> list[BaseConstraint]: + """ Parse text-like constraints defined by a given set of keys """ + + allowed_operators = allowed_operators or ( + Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH) + + return [ + TextConstraint.from_specification( + f'{prefix}.{constraint_name.replace("-", "_")}', + str(spec[constraint_name]), + allowed_operators=list(allowed_operators)) + for constraint_name in constraint_keys + if constraint_name in spec + ] + + +def _parse_flag_constraints( + spec: Spec, + prefix: str, + constraint_keys: tuple[str, ...]) -> list[BaseConstraint]: + """ Parse flag-like constraints defined by a given set of keys """ + + return [ + FlagConstraint.from_specification( + f'{prefix}.{constraint_name.replace("-", "_")}', + spec[constraint_name], + allowed_operators=[Operator.EQ, Operator.NEQ]) + for constraint_name in constraint_keys + if constraint_name in spec + ] + + +def _parse_device_core( + spec: Spec, + device_prefix: str = 'device', + include_driver: bool = True, + include_device: bool = True) -> And: + """ + Parse constraints shared across device classes. + + :param spec: raw constraint block specification. + :returns: block representation as :py:class:`BaseConstraint` or one of its subclasses. + """ + + group = And() + + number_constraints: tuple[str, ...] = ('vendor',) + text_constraints: tuple[str, ...] = ('vendor-name',) + + if include_device: + number_constraints = (*number_constraints, 'device') + text_constraints = (*text_constraints, 'device-name') + + if include_driver: + text_constraints = (*text_constraints, 'driver') + + group.constraints += _parse_number_constraints(spec, device_prefix, number_constraints) + group.constraints += _parse_text_constraints(spec, device_prefix, text_constraints) + + return group + + @ungroupify def _parse_boot(spec: Spec) -> BaseConstraint: """ @@ -914,29 +1029,11 @@ def _parse_virtualization(spec: Spec) -> BaseConstraint: group = And() - if 'is-virtualized' in spec: - group.constraints += [ - FlagConstraint.from_specification( - 'virtualization.is_virtualized', - spec['is-virtualized'], - allowed_operators=[Operator.EQ, Operator.NEQ]) - ] - - if 'is-supported' in spec: - group.constraints += [ - FlagConstraint.from_specification( - 'virtualization.is_supported', - spec['is-supported'], - allowed_operators=[Operator.EQ, Operator.NEQ]) - ] - - if 'hypervisor' in spec: - group.constraints += [ - TextConstraint.from_specification( - 'virtualization.hypervisor', - spec['hypervisor'], - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - ] + group.constraints += _parse_flag_constraints(spec, + 'virtualization', + ('is-virtualized', + 'is-supported')) + group.constraints += _parse_text_constraints(spec, 'virtualization', ('hypervisor',)) return group @@ -973,13 +1070,10 @@ def _parse_cpu(spec: Spec) -> BaseConstraint: group = And() - group.constraints += [ - NumberConstraint.from_specification( - f'cpu.{constraint_name.replace("-", "_")}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE]) - for constraint_name in ( + group.constraints += _parse_number_constraints( + spec, + 'cpu', + ( 'processors', 'sockets', 'cores', @@ -987,23 +1081,21 @@ def _parse_cpu(spec: Spec) -> BaseConstraint: 'cores-per-socket', 'threads-per-core', 'model', - 'family' + 'family', + 'vendor', + 'stepping' ) - if constraint_name in spec - ] + ) - group.constraints += [ - TextConstraint.from_specification( - f'cpu.{constraint_name.replace("-", "_")}', - str(spec[constraint_name]), - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - for constraint_name in ( + group.constraints += _parse_text_constraints( + spec, + 'cpu', + ( 'family-name', 'model-name', 'vendor-name' ) - if constraint_name in spec - ] + ) if 'flag' in spec: flag_group = And() @@ -1024,6 +1116,18 @@ def _parse_cpu(spec: Spec) -> BaseConstraint: return group +@ungroupify +def _parse_device(spec: Spec) -> BaseConstraint: + """ + Parse a device-related constraints. + + :param spec: raw constraint block specification. + :returns: block representation as :py:class:`BaseConstraint` or one of its subclasses. + """ + + return _parse_device_core(spec) + + @ungroupify_indexed def _parse_disk(spec: Spec, disk_index: int) -> BaseConstraint: """ @@ -1036,35 +1140,9 @@ def _parse_disk(spec: Spec, disk_index: int) -> BaseConstraint: group = And() - group.constraints += [ - SizeConstraint.from_specification( - f'disk[{disk_index}].{constraint_name}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE]) - for constraint_name in ('size',) - if constraint_name in spec - ] - - group.constraints += [ - TextConstraint.from_specification( - f'disk[{disk_index}].{constraint_name.replace("-", "_")}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - for constraint_name in ('model-name',) - if constraint_name in spec - ] - - group.constraints += [ - TextConstraint.from_specification( - f'disk[{disk_index}].{constraint_name}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - for constraint_name in ('driver',) - if constraint_name in spec - ] + group.constraints += _parse_size_constraints(spec, f'disk[{disk_index}]', ('size',)) + group.constraints += _parse_text_constraints(spec, + f'disk[{disk_index}]', ('model-name', 'driver')) return group @@ -1092,6 +1170,18 @@ def _parse_disks(spec: Spec) -> BaseConstraint: return group +@ungroupify +def _parse_gpu(spec: Spec) -> BaseConstraint: + """ + Parse a gpu-related constraints. + + :param spec: raw constraint block specification. + :returns: block representation as :py:class:`BaseConstraint` or one of its subclasses. + """ + + return _parse_device_core(spec, device_prefix='gpu') + + @ungroupify_indexed def _parse_network(spec: Spec, network_index: int) -> BaseConstraint: """ @@ -1102,16 +1192,8 @@ def _parse_network(spec: Spec, network_index: int) -> BaseConstraint: :returns: block representation as :py:class:`BaseConstraint` or one of its subclasses. """ - group = And() - - group.constraints += [ - TextConstraint.from_specification( - f'network[{network_index}].{constraint_name}', - str(spec[constraint_name]), - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - for constraint_name in ('type',) - if constraint_name in spec - ] + group = _parse_device_core(spec, f'network[{network_index}]') + group.constraints += _parse_text_constraints(spec, f'network[{network_index}]', ('type',)) return group @@ -1144,33 +1226,20 @@ def _parse_system(spec: Spec) -> BaseConstraint: :returns: block representation as :py:class:`BaseConstraint` or one of its subclasses. """ - group = And() + group = _parse_device_core( + spec, + device_prefix='system', + include_driver=False, + include_device=False) - group.constraints += [ - NumberConstraint.from_specification( - f'system.{constraint_name.replace("-", "_")}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE]) - for constraint_name in ('vendor', 'model', 'numa-nodes') - if constraint_name in spec - ] - - group.constraints += [ - TextConstraint.from_specification( - f'system.{constraint_name.replace("-", "_")}', - str(spec[constraint_name]), - allowed_operators=[ - Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - for constraint_name in ('model-name', 'vendor-name') - if constraint_name in spec - ] + group.constraints += _parse_number_constraints(spec, 'system', ('model', 'numa-nodes')) + group.constraints += _parse_text_constraints(spec, 'system', ('model-name',)) return group -TPM_VERSION_ALLOWED_OPERATORS: list[Operator] = [ - Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE] +TPM_VERSION_ALLOWED_OPERATORS: tuple[Operator, ...] = ( + Operator.EQ, Operator.NEQ, Operator.LT, Operator.LTE, Operator.GT, Operator.GTE) @ungroupify @@ -1184,13 +1253,11 @@ def _parse_tpm(spec: Spec) -> BaseConstraint: group = And() - if 'version' in spec: - group.constraints += [ - TextConstraint.from_specification( - 'tpm.version', - spec['version'], - allowed_operators=TPM_VERSION_ALLOWED_OPERATORS) - ] + group.constraints += _parse_text_constraints(spec, + 'tpm', + ('version', + ), + allowed_operators=TPM_VERSION_ALLOWED_OPERATORS) return group @@ -1235,19 +1302,7 @@ def _parse_zcrypt(spec: Spec) -> BaseConstraint: group = And() - if 'adapter' in spec: - group.constraints += [ - TextConstraint.from_specification( - 'zcrypt.adapter', - spec['adapter'], - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH])] - - if 'mode' in spec: - group.constraints += [ - TextConstraint.from_specification( - 'zcrypt.mode', - spec['mode'], - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH])] + group.constraints += _parse_text_constraints(spec, 'zcrypt', ('adapter', 'mode')) return group @@ -1263,13 +1318,7 @@ def _parse_location(spec: Spec) -> BaseConstraint: group = And() - if 'lab-controller' in spec: - group.constraints += [ - TextConstraint.from_specification( - 'location.lab_controller', - spec['lab-controller'], - allowed_operators=[Operator.EQ, Operator.NEQ, Operator.MATCH, Operator.NOTMATCH]) - ] + group.constraints += _parse_text_constraints(spec, 'location', ('lab-controller',)) return group @@ -1301,6 +1350,12 @@ def _parse_generic_spec(spec: Spec) -> BaseConstraint: if 'cpu' in spec: group.constraints += [_parse_cpu(spec['cpu'])] + if 'device' in spec: + group.constraints += [_parse_device(spec['device'])] + + if 'gpu' in spec: + group.constraints += [_parse_gpu(spec['gpu'])] + if 'memory' in spec: group.constraints += [_parse_memory(spec)]