Support multi-arch cpu_arch in py_config and schedulable_nodes#4972
Closed
hmeir wants to merge 1 commit into
Closed
Support multi-arch cpu_arch in py_config and schedulable_nodes#4972hmeir wants to merge 1 commit into
hmeir wants to merge 1 commit into
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 56 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-5
requested review from
OhadRevah,
rnetser and
vsibirsk
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4972 +/- ##
==========================================
- Coverage 98.67% 98.66% -0.02%
==========================================
Files 25 25
Lines 2487 2465 -22
==========================================
- Hits 2454 2432 -22
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tests/install_upgrade_operators/hco_enablement_golden_image_updates/conftest.py`:
- Around line 132-136: The fixture default_common_templates_related_resources
currently declares worker_architectures as a direct parameter, but
worker_architectures is only defined in the multiarch/conftest and thus not
available to sibling tests; either move the worker_architectures fixture into
tests/install_upgrade_operators/hco_enablement_golden_image_updates/conftest.py
so it’s globally available to those tests, or remove worker_architectures from
default_common_templates_related_resources’ signature and instead fetch it at
runtime inside that fixture (use request.getfixturevalue("worker_architectures")
with a try/except to handle the case where it’s not present) so the fixture no
longer has an unresolved dependency. Ensure you update
default_common_templates_related_resources to reference worker_architectures
only when successfully obtained.
In
`@tests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/conftest.py`:
- Around line 33-34: The test currently patches
hyperconverged_resource_scope_class.spec.FEATURE_GATES by assigning a map with
only ENABLE_MULTI_ARCH_BOOT_IMAGE_IMPORT which can remove other gates; instead
update/merge the existing featureGates map so only
ENABLE_MULTI_ARCH_BOOT_IMAGE_IMPORT is changed. Locate
hyperconverged_resource_scope_class and the places that set FEATURE_GATES (lines
referencing FEATURE_GATES and ENABLE_MULTI_ARCH_BOOT_IMAGE_IMPORT) and change
the patch logic to read the existing spec.featureGates, copy/merge it,
set/override ENABLE_MULTI_ARCH_BOOT_IMAGE_IMPORT to the desired boolean, then
write back the merged map so other gates are preserved.
- Around line 29-119: All new pytest fixtures in this file lack type hints; add
proper typing annotations for parameters and return values (and import needed
names from typing) so they comply with mypy strict rules. Specifically, annotate
disabled_multiarch_feature_gate and enabled_multiarch_feature_gate as returning
Iterator[None] (or Generator[None, None, None]), worker_architectures as
returning Set[str], control_plane_architecture with the appropriate return type
from get_control_plane_architecture (e.g., str), single_arch_node_placement as
Iterator[None], and both hco_with_custom_unsupported_arch_template and
hco_with_custom_no_arch_annotation_template with the correct Iterator/Generator
return type; also add type annotations to their parameters (e.g., admin_client,
hco_namespace, hyperconverged_resource_scope_function, etc.) where those fixture
types are known. Ensure you import typing names (Iterator, Generator, Set) and
any project-specific types used for fixtures.
- Line 71: Before computing single_arch from worker_architectures, guard against
an empty list to avoid a bare IndexError: add an explicit check/assert that
worker_architectures is not empty and raise a clear error (e.g., AssertionError
or ValueError with a descriptive message about missing worker architectures or
cluster/setup failure) before calling sorted(worker_architectures)[0]; this
change should be applied around the single_arch assignment in conftest.py where
worker_architectures is used.
In
`@tests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/utils.py`:
- Line 49: The deletion of template["status"] can raise KeyError when the key is
absent; update the removal to be safe by using a conditional or a safe pop
(e.g., check "if 'status' in template: del template['status']" or use
template.pop('status', None)) so that the code in utils.py that manipulates the
template variable no longer hard-fails when status is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 09db8301-cdeb-4dfb-81cb-b600adc0b48f
📒 Files selected for processing (13)
tests/conftest.pytests/install_upgrade_operators/constants.pytests/install_upgrade_operators/crypto_policy/constants.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/conftest.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/conftest.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/test_multiarch_golden_images_support.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/multiarch/utils.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/test_custom_golden_images_namespace.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/utils.pyutilities/hco.pyutilities/monitoring.pyutilities/pytest_utils.pyutilities/unittests/test_pytest_utils.py
hmeir
force-pushed
the
iuo-multi-arch-automation
branch
from
f0f9c71 to
396f636
Compare
openshift-virtualization-qe-bot-3
removed
the
changes-requested-coderabbitai[bot]
label
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
hmeir
force-pushed
the
iuo-multi-arch-automation
branch
from
396f636 to
7c92583
Compare
hmeir
force-pushed
the
iuo-multi-arch-automation
branch
from
64dcc7d to
8453f60
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
rnetser
requested changes
May 25, 2026
Collaborator
rnetser
left a comment
rnetser
left a comment
There was a problem hiding this comment.
such PRs should be split to smaller ones:
- refactor - moving code to be reused
- add additional py_config data
- new tests
Comment on lines
+562
to
+568
| def filter_multiarch_tests(items: list[Item], config: Config) -> list[Item]: | ||
| if py_config.get("cluster_type") == MULTIARCH: | ||
| return items | ||
| discard_tests, items_to_return = remove_tests_from_list(items=items, filter_str="multiarch") | ||
| if discard_tests: | ||
| config.hook.pytest_deselected(items=discard_tests) | ||
| return items_to_return |
Collaborator
There was a problem hiding this comment.
@hmeir please move, as commented by coderabbit, to pytest_utils.
| cpu_arch = cpu_arch_option.split(",") if cpu_arch_option else list(get_cluster_architecture()) | ||
|
|
||
| if len(cpu_arch) > 1: | ||
| py_config["cpu_arch"] = cpu_arch # stored as list for multiarch runs |
Collaborator
There was a problem hiding this comment.
py_config["cpu_arch"] is expected to be a sting; it cannot be set here as a list
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conftest.py`:
- Line 36: The PR description template used in conftest.py is missing required
headings; update the template or PR-body generation logic (where MULTIARCH is
referenced) to restore these exact sections: "##### What this PR does / why we
need it:" (must contain meaningful content or fail the check), "##### Which
issue(s) this PR fixes:", "##### Special notes for reviewer:", and "#####
jira-ticket:"; ensure the generator populates or enforces non-empty content for
the first heading and that the validation check treats absence as HIGH severity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f451c589-dff3-4fa0-a10f-b2962f925dc8
📒 Files selected for processing (4)
conftest.pytests/conftest.pyutilities/pytest_utils.pyutilities/unittests/test_pytest_utils.py
This was referenced May 25, 2026
Store cpu_arch as list in py_config for multi-arch runs. Fix schedulable_nodes to normalize cpu_arch to list and use 'in' membership check. Update cluster info log to handle list cpu_arch display. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
rnetser
reviewed
May 26, 2026
Comment on lines
+562
to
+568
| def filter_multiarch_tests(items: list[Item], config: Config) -> list[Item]: | ||
| if py_config.get("cluster_type") == MULTIARCH: | ||
| return items | ||
| discard_tests, items_to_return = remove_tests_from_list(items=items, filter_str="multiarch") | ||
| if discard_tests: | ||
| config.hook.pytest_deselected(items=discard_tests) | ||
| return items_to_return |
| f"\tCNI type: {get_cluster_cni_type(admin_client=admin_client)}\n" | ||
| f"\tWorkers type: {workers_type}\n" | ||
| f"\tCluster CPU Architecture: {nodes_cpu_architecture}\n" | ||
| f"\tCluster CPU Architecture: {nodes_cpu_architecture or ', '.join(sorted(get_cluster_architecture()))}\n" |
Collaborator
There was a problem hiding this comment.
why not use py_config["cluster_arch"]?
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4934. Overlapping filestests/conftest.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filesconftest.py |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filestests/conftest.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Enable py_config and collection infrastructure to handle multi-arch
cluster runs with `--cpu-arch=amd64,arm64'.
Which issue(s) this PR fixes:
cpu_archas list inpy_configfor multi-arch runs so fixtures can read declared architecturesschedulable_nodesto normalize cpu_arch to list and useinmembership check (handles both str and list[str])
filter_multiarch_teststo deselect multiarch-marked tests onhomogeneous clusters (fixes pytest-check-x86 tox collection failure)
Special notes for reviewer:
Multiarch doc says:
jira-ticket:
NONE