Refactor OS matrix to support different architectures #983
Conversation
## Walkthrough
This update introduces multi-architecture support for test image configurations, adding dedicated handling for `arm64` and `s390x` architectures alongside `x86_64`. It refactors OS matrix generation into reusable utilities, modularizes image metadata into dataclasses, and updates test configs, fixtures, and tox environments to leverage these changes. Numerous dictionary accesses are made safer using `.get()`. Documentation and pytest markers are updated accordingly.
## Changes
| Files / Groups | Change Summary |
|-----------------------------------------------------------------------------------------------------|---------------|
| `utilities/constants.py`, `libs/infra/images.py`, `utilities/os_utils.py` | Refactored image and OS matrix handling: introduced architecture-specific classes, dataclasses for image metadata, and utilities for OS matrix generation. Added support for `arm64` and `s390x`. |
| `tests/global_config.py` | Removed all statically defined OS matrices and related constants; simplified configuration. |
| `tests/global_config_x86.py`, `tests/global_config_arm64.py`, `tests/global_config_s390x.py` | Added/updated architecture-specific global config files using new OS matrix utilities and image constants. |
| `tests/global_config_aws.py`, `tests/global_config_ibm_spectrum_sc.py`, `tests/global_config_interop.py`, `tests/global_config_interop_sno.py`, `tests/global_config_lvms.py`, `tests/global_config_nfs.py`, `tests/global_config_oci.py`, `tests/global_config_rh_it.py`, `tests/global_config_sno.py`, `tests/global_config_sno_hpp.py` | Updated to load new architecture-specific config files (`global_config_x86.py`). |
| `conftest.py`, `tests/conftest.py`, `tests/os_params.py`, `utilities/virt.py`, `utilities/pytest_utils.py` | Updated to use safer `.get()` dictionary access, improved error handling, and robust fixture/config logic. |
| `tests/storage/cdi_import/test_import_http.py`, `tests/storage/cdi_upload/test_upload_virtctl.py`, `tests/virt/node/hotplug/test_cpu_memory_hotplug.py` | Introduced local variables for OS dicts, replaced direct config access with `.get()`. |
| `tests/virt/cluster/common_templates/windows/test_windows_custom_options.py`, `tests/virt/cluster/longevity_tests/constants.py`, `tests/virt/node/general/test_networkinterfacemultiqueue_feature.py`, `tests/virt/node/general/test_oom.py`, `tests/virt/node/general/test_windows_vtpm_bitlocker.py`, `tests/virt/node/general/test_wsl2.py`, `tests/virt/node/gpu/gpu_pci_passthrough/test_windows_vm_with_gpu_pci_passthrough.py`, `tests/virt/node/gpu/vgpu/test_windows_vm_with_vgpu.py`, `tests/virt/node/high_performance_vm/test_high_performance_templates.py`, `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`, `tests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.py` | Replaced `os.path.join` with f-string path concatenation for image paths; removed unused `os` imports. |
| Many test files (e.g., `tests/storage/test_hotplug.py`, `tests/scale/test_scale_benchmark.py`, `tests/virt/cluster/common_templates/general/test_base_custom_templates.py`, etc.) | Replaced direct dictionary key access with `.get()` for safer value retrieval from config/OS dicts. |
| `pytest.ini` | Added `x86_64` and `s390x` markers; removed default test config from `addopts`. |
| `tox.ini` | Split `pytest-check` into `pytest-check-x86` and `pytest-check-arm64` environments; added template for `pytest-check-s390x`. |
| `docs/GETTING_STARTED.md` | Updated documentation to mention `arm64` and `s390x` image architecture support. |
## Possibly related PRs
- RedHatQE/openshift-virtualization-tests#930: Related to ARM64 test coverage enhancements for RHEL10 instance types and test markers.
- RedHatQE/openshift-virtualization-tests#915: Related to OS matrix data handling and test configuration updates.
- RedHatQE/openshift-virtualization-tests#1142: Fixes flakiness in metrics tests by adding a retry mechanism, related to test reliability improvements.
## Suggested labels
`verified`, `size/L`, `can-be-merged`, `lgtm-dshchedr`, `approved-rnetser`, `lgtm-rnetser`
## Suggested reviewers
- dshchedr
- vsibirsk
- RoniKishner📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
PR will be approved when the following conditions are met:
Approvers and Reviewers
Supported /retest check runs
Supported labels
|
ghost
requested a review
from 
ghost
mentioned this pull request
polarion-jenkins
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
utilities/constants.py (1)
158-160: Minor grammar & clarity.
f"{arch} architecture in not supported"→f"{arch} architecture is not supported"- raise ValueError(f"{arch} architecture in not supported") + raise ValueError(f"{arch} architecture is not supported")utilities/os_utils.py (3)
155-166: Inconsistent error message (winvswindows).The function accepts
"windows"but the exception advertises"win".
Align them to avoid user confusion.- else: - raise ValueError(f"Unsupported OS: {os_name}. Supported: rhel, win, fedora") + else: + raise ValueError( + f"Unsupported OS: {os_name}. Supported values: 'rhel', 'windows', 'fedora', 'centos'" + )
173-197: No guard for an emptysupported_operating_systemslist.Calling
generate_os_matrix_dict("rhel", [])silently returns[], which is probably unintended.
Fail fast to surface configuration errors.- unsupported_versions = [] + if not supported_operating_systems: + raise ValueError("supported_operating_systems must contain at least one version") + + unsupported_versions = []
185-188: Template labels fall back to constants but ignore per-OS overrides.If a version overrides only
WORKLOAD_STRbut notFLAVOR_STR, the current logic overrides both.
Switch to independentdict.getcalls to allow partial overrides:- WORKLOAD_STR: base_version_dict.get(WORKLOAD_STR, base_dict[WORKLOAD_STR]), - FLAVOR_STR: base_version_dict.get(FLAVOR_STR, base_dict[FLAVOR_STR]), + WORKLOAD_STR: base_version_dict.get(WORKLOAD_STR, base_dict.get(WORKLOAD_STR)), + FLAVOR_STR: base_version_dict.get(FLAVOR_STR, base_dict.get(FLAVOR_STR)),tests/global_config_x86.py (1)
78-78: Remove unnecessary noqa comment.If you've implemented the suggested fixes above, this noqa comment can be removed as the
configvariable will be properly initialized.- config[_dir] = locals()[_dir] # noqa: F821 + config[_dir] = locals()[_dir]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)conftest.py(1 hunks)tests/conftest.py(1 hunks)tests/global_config.py(1 hunks)tests/global_config_x86.py(1 hunks)tests/os_params.py(1 hunks)utilities/constants.py(1 hunks)utilities/os_utils.py(1 hunks)utilities/virt.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/os_params.py (1)
utilities/virt.py (1)
get_windows_os_dict(1552-1564)
conftest.py (1)
utilities/infra.py (1)
generate_latest_os_dict(196-214)
tests/conftest.py (1)
utilities/storage.py (1)
get_test_artifact_server_url(596-617)
utilities/os_utils.py (1)
libs/vm/spec.py (1)
Template(16-18)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
🔇 Additional comments (16)
README.md (1)
92-92: Documentation update for s390x architecture support looks good.The addition of s390x as a supported architecture with the "currently work in progress" note aligns well with the broader changes in this PR to support different architectures in the OS matrix.
tests/os_params.py (3)
7-9: Good improvement to defensive coding practices.The use of
.get()with default empty dictionaries provides better error handling for missing keys in the configuration, making the code more robust against KeyError exceptions.
12-21: Consistent application of safer dictionary access.The consistent use of the
.get()method across all Windows OS configuration access points is a good practice, especially when interacting with configuration data that might be incomplete.
23-25: Maintaining consistency for Fedora configuration access.The changes follow the same pattern of safer access through
.get()method for Fedora configurations, ensuring a consistent approach throughout the file.utilities/virt.py (2)
1553-1564: Enhanced error handling for Windows OS configuration.The function now safely checks for the existence of the Windows OS matrix in the configuration and provides a graceful fallback (empty dictionary) when it's not present. This is a good improvement to prevent runtime exceptions.
1568-1579: Improved robustness for RHEL OS configuration retrieval.Similarly to the Windows OS function, this change adds proper checks for RHEL OS matrix existence and provides a graceful fallback when missing. The consistent approach across both OS matrix functions is commendable.
tests/conftest.py (1)
1884-1891: Improved error handling in rhel_latest_os_params fixture.The change safely handles the possibility of a missing "latest_rhel_os_dict" key in py_config using the walrus operator with get(), preventing KeyError exceptions during fixture setup and returning an empty dictionary instead.
conftest.py (2)
588-593: Safer handling of OS matrix configuration.These changes use the walrus operator with
.get()to safely handle cases where OS matrix keys might be missing in py_config, preventing potential KeyError exceptions during session startup.
595-606: Added defensive checks when updating OS matrices.Good addition of null checks before attempting to generate latest OS dictionaries, ensuring the code only updates matrices when they exist, preventing potential errors during OS matrix generation.
tests/global_config.py (1)
181-188: Verify CentOS Stream 10 entries really exist.
centos-stream10images and templates are listed but are still in Early Preview upstream.
Please confirm:• A matching DataImportCron template lives in the cluster
• Thecentos.stream10preference exists in common-templatesOtherwise this entry will break nightly jobs.
tests/global_config_x86.py (6)
20-30: RHEL OS matrix definition looks good.The OS matrix for RHEL is well-defined using the new
generate_os_matrix_dictutility function. The supported operating systems list contains specific versions which aligns with the refactoring goal to support different architectures.
32-42: Windows OS matrix definition looks good.The Windows OS matrix is correctly defined with a comprehensive list of supported versions. The use of the centralized
generate_os_matrix_dictfunction ensures consistency across different OS matrices.
44-46: Fedora and CentOS OS matrix definitions look good.Both OS matrices are concisely defined with their respective supported versions. The single-line style is appropriate for these simpler definitions with only one supported OS version each.
48-59: Special RHEL 10 instance type matrix is well-defined.The specialized instance type matrix for RHEL 10 includes all necessary attributes with clear structure. This provides good configuration for tests that require specific instance type settings.
61-66: Efficient latest OS dictionary retrieval.The use of tuple unpacking for the return value of
get_latest_os_dict_listis clear and concise. This correctly retrieves the latest OS dictionaries for all defined OS matrices.
72-77: Appropriate filtering in config construction.The type checking and key exclusion logic for constructing the config dictionary is well-implemented. This ensures that only relevant configuration values are included in the final config dictionary.
polarion-jenkins
added
tox:verify-bugs-are-open:failed
polarion-jenkins
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-983 published |
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
New Features
arm64ands390xarchitectures (s390x as work in progress) in test configuration and documentation.Bug Fixes
Refactor
Documentation
Chores