Fix s390x test compatibility issues#4305
Open
chandramerla wants to merge 1 commit intoRedHatQE:mainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds S390X-aware test logic and configuration, skips EFI-requiring preferences on S390X, broadens test scope by removing S390X-only markers, and enhances IP resolution with a timeout-retry mechanism in the network utility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-4
requested review from
jpeimer,
kgoldbla and
yossisegev
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4305 +/- ##
==========================================
+ Coverage 98.63% 98.65% +0.02%
==========================================
Files 25 25
Lines 2420 2459 +39
==========================================
+ Hits 2387 2426 +39
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-4
requested review from
Ahmad-Hafe,
Anatw,
RoniKishner,
azhivovk,
dalia-frank,
dshchedr,
geetikakay,
josemacassan,
kshvaika,
servolkov and
vsibirsk
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
chandramerla
force-pushed
the
s390x-test-fixes
branch
from
2a236a8 to
2d924df
Compare
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: 1
♻️ Duplicate comments (1)
utilities/network.py (1)
764-770:⚠️ Potential issue | 🟠 MajorHIGH:
_resolve_ipcan fail before retry logic has a chance to retryAt
Line 766(vm.vmi.interfaces[0]) andLine 768(podIPsiteration), transient missing data can raiseIndexError/AttributeError/TypeError. That turns a temporary “IP not ready yet” state into an immediate failure, which defeats the new timeout-based polling behavior.Proposed fix
def _resolve_ip(): - if vm: - addr_list = vm.vmi.interfaces[0].get("ipAddresses") or [] - else: - addr_list = [ip_addr["ip"] for ip_addr in virt_handler_pod.instance.status.podIPs] + try: + if vm: + interfaces = vm.vmi.interfaces or [] + addr_list = interfaces[0].get("ipAddresses") if interfaces else [] + addr_list = addr_list or [] + else: + pod_ips = virt_handler_pod.instance.status.podIPs or [] + addr_list = [ip_addr.get("ip") for ip_addr in pod_ips if ip_addr.get("ip")] + except (AttributeError, IndexError, TypeError): + return None ip_list = [ip for ip in addr_list if get_valid_ip_address(dst_ip=ip, family=family)] return ip_list[0] if ip_list else None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utilities/network.py` around lines 764 - 770, The helper _resolve_ip currently accesses vm.vmi.interfaces[0] and iterates virt_handler_pod.instance.status.podIPs directly which can raise IndexError/AttributeError/TypeError and bypass retry logic; update _resolve_ip to defensively access those attributes (check for None, missing keys, empty lists) and wrap the extraction logic in a try/except that catches IndexError/AttributeError/TypeError, returning None on failure so the outer polling can retry; reference vm, vm.vmi.interfaces, virt_handler_pod.instance.status.podIPs, get_valid_ip_address and family when making these safe checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/infrastructure/instance_types/test_common_vm_preference.py`:
- Around line 60-64: Add explicit type hints to the helper function
_preference_requires_efi: annotate the parameters (e.g., client: Any,
preference_name: str) and the return type (-> bool) and import Any from typing
if needed; keep the implementation unchanged but update the function signature
to include these annotations to improve readability in tests.
---
Duplicate comments:
In `@utilities/network.py`:
- Around line 764-770: The helper _resolve_ip currently accesses
vm.vmi.interfaces[0] and iterates virt_handler_pod.instance.status.podIPs
directly which can raise IndexError/AttributeError/TypeError and bypass retry
logic; update _resolve_ip to defensively access those attributes (check for
None, missing keys, empty lists) and wrap the extraction logic in a try/except
that catches IndexError/AttributeError/TypeError, returning None on failure so
the outer polling can retry; reference vm, vm.vmi.interfaces,
virt_handler_pod.instance.status.podIPs, get_valid_ip_address and family when
making these safe checks.
🪄 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: 8e50a842-3e41-4d13-92af-a27609ead15b
📒 Files selected for processing (7)
tests/conftest.pytests/global_config_s390x.pytests/infrastructure/instance_types/test_common_vm_preference.pytests/infrastructure/instance_types/test_vm_with_instance_and_pref.pytests/network/connectivity/test_pod_network.pytests/storage/cdi_clone/test_clone.pyutilities/network.py
💤 Files with no reviewable changes (2)
- tests/infrastructure/instance_types/test_vm_with_instance_and_pref.py
- tests/storage/cdi_clone/test_clone.py
- Fix centos DataSource name: use os_name="centos-stream" to generate correct "centos-stream9" DataSource name instead of "centos9" - Add retry logic to get_ip_from_vm_or_virt_handler_pod with TimeoutSampler to handle guest agent IP reporting timing - Skip x86-specific clock timers (hpet, pit, kvm, hyperv, rtc) in VirtualMachinePreference fixture on s390x - Remove s390x marker from test_validate_clock_values (x86-only) - Skip EFI/OVMF preferences on s390x in common VM preference tests - Override nic_models_matrix for s390x to exclude unsupported e1000e - Remove s390x marker from test_successful_clone_of_large_image (Windows image not available for s390x) Made-with: Cursor
chandramerla
force-pushed
the
s390x-test-fixes
branch
from
2d924df to
7eef80e
Compare
openshift-virtualization-qe-bot
added
commented-coderabbitai[bot]
tox:verify-tc-requirement-polarion:running
Member
Author
|
@rnetser Could you please help review these small test fixes? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/conftest.py`:
- Around line 2223-2228: The check incorrectly compares
get_cluster_architecture() (which returns a set) to S390X; change the guard to
test membership (e.g., if S390X in get_cluster_architecture()) so the s390x
branch runs and avoids adding common_preference_dict.setdefault("clock",
{})["preferredTimer"] = request.param["clock_preferred_timer"]; update the
conditional that uses get_cluster_architecture(), S390X, common_preference_dict,
and request.param["clock_preferred_timer"] accordingly.
In `@tests/infrastructure/instance_types/test_common_vm_preference.py`:
- Around line 67-73: The variable is_s390x is incorrectly computed by equality
against get_cluster_architecture(); since that function returns a singleton set,
change the check to a membership test (e.g., is_s390x = S390X in
get_cluster_architecture()) so the s390x EFI/OVMF skip branch (which calls
_preference_requires_efi(client, preference_name) and logs via LOGGER) actually
runs for s390x clusters.
🪄 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: b8828b87-7642-4402-b0cf-a05bf8a90428
📒 Files selected for processing (7)
tests/conftest.pytests/global_config_s390x.pytests/infrastructure/instance_types/test_common_vm_preference.pytests/infrastructure/instance_types/test_vm_with_instance_and_pref.pytests/network/connectivity/test_pod_network.pytests/storage/cdi_clone/test_clone.pyutilities/network.py
💤 Files with no reviewable changes (2)
- tests/infrastructure/instance_types/test_vm_with_instance_and_pref.py
- tests/storage/cdi_clone/test_clone.py
Comment on lines
+2223
to
+2228
| if get_cluster_architecture() == S390X: | ||
| LOGGER.info( | ||
| "Skipping preferredTimer in VirtualMachinePreference: x86-specific timers not available on s390x" | ||
| ) | ||
| else: | ||
| common_preference_dict.setdefault("clock", {})["preferredTimer"] = request.param["clock_preferred_timer"] |
Contributor
There was a problem hiding this comment.
HIGH: This s390x guard is comparing the wrong types.
Line 2223 compares get_cluster_architecture() to S390X, but this helper returns a set in this repo. That makes the condition false, so preferredTimer is still added on s390x and the fixture keeps generating x86-only clock timers there.
Proposed fix
- if get_cluster_architecture() == S390X:
+ if S390X in get_cluster_architecture():
LOGGER.info(
"Skipping preferredTimer in VirtualMachinePreference: x86-specific timers not available on s390x"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/conftest.py` around lines 2223 - 2228, The check incorrectly compares
get_cluster_architecture() (which returns a set) to S390X; change the guard to
test membership (e.g., if S390X in get_cluster_architecture()) so the s390x
branch runs and avoids adding common_preference_dict.setdefault("clock",
{})["preferredTimer"] = request.param["clock_preferred_timer"]; update the
conditional that uses get_cluster_architecture(), S390X, common_preference_dict,
and request.param["clock_preferred_timer"] accordingly.
Comment on lines
+67
to
+73
| is_s390x = get_cluster_architecture() == S390X | ||
| for preference_name in preferences: | ||
| # TODO remove arm64 skip when openshift-virtualization-tests support arm64 | ||
| if all(suffix not in preference_name for suffix in ["virtio", "arm64"]): | ||
| if is_s390x and _preference_requires_efi(client=client, preference_name=preference_name): | ||
| LOGGER.info(f"Skipping preference {preference_name}: EFI/OVMF not available on s390x") | ||
| continue |
Contributor
There was a problem hiding this comment.
HIGH: is_s390x is always false here.
Line 67 makes the same set-to-string comparison, so the new EFI/OVMF skip path never runs on s390x. The test will still try to boot preferences that require EFI, which is the compatibility issue this PR is meant to avoid.
Proposed fix
- is_s390x = get_cluster_architecture() == S390X
+ is_s390x = S390X in get_cluster_architecture()🧰 Tools
🪛 Ruff (0.15.7)
[warning] 72-72: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/infrastructure/instance_types/test_common_vm_preference.py` around
lines 67 - 73, The variable is_s390x is incorrectly computed by equality against
get_cluster_architecture(); since that function returns a singleton set, change
the check to a membership test (e.g., is_s390x = S390X in
get_cluster_architecture()) so the s390x EFI/OVMF skip branch (which calls
_preference_requires_efi(client, preference_name) and logs via LOGGER) actually
runs for s390x clusters.
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
5 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.
Short description:
When trying to move to fedora43 for s390x and trying to run affected tests, have come across some issues, whose fixes are in this PR
More details:
What this PR does / why we need it:
Fixes some test issues for s390x arch.
Which issue(s) this PR fixes:
Special notes for reviewer:
Please review changes from respective files as you are more familiar or making fixes there -
@aditi-sharma-1 . tests/infrastructure/instance_types/test_common_vm_preference.py , tests/infrastructure/instance_types/test_vm_with_instance_and_pref.py, utilities/pytest_utils.py, utilities/unittests/test_pytest_utils.py
@HarshithaMS005 tests/network/connectivity/test_pod_network.py , utilities/network.py
@Davo911 tests/storage/cdi_clone/test_clone.py
jira-ticket:
Summary by CodeRabbit
Tests
Bug Fixes / Improvements