Fix s390x test compatibility issues#4305
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces architecture-aware test handling for s390x clusters by adding conditional logic to fixtures and tests. It adds a dependency on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.66% +0.03%
==========================================
Files 25 25
Lines 2420 2476 +56
==========================================
+ Hits 2387 2443 +56
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
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4365. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4402. Overlapping filestests/conftest.py |
|
Hey @chandramerla just thinking out loud: in order to get things moving, what do you think about moving the timeout related changes to another PR? |
chandramerla
force-pushed
the
s390x-test-fixes
branch
from
7eef80e to
09fc814
Compare
openshift-virtualization-qe-bot-4
requested review from
acinko-rh,
ema-aka-young,
frenzyfriday,
nirdothan,
orelmisan and
rnetser
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/infrastructure/instance_types/test_common_vm_preference.py (1)
60-78:⚠️ Potential issue | 🔴 CriticalCRITICAL:
is_s390xis alwaysFalse, so the EFI skip path never runs — and reviewer asked foris_s390x_cluster.
get_cluster_architecture()returns aset[str](perutilities/architecture.py), soget_cluster_architecture() == S390Xcompares a set against the string"s390x"and is permanentlyFalse. The new EFI/OVMF skip block on s390x is therefore unreachable, and the test will still try to start VMs with EFI-required preferences on s390x — defeating the purpose of this PR.rnetser asked for the existing
is_s390x_clusterfixture (defined intests/conftest.pylines 2743–2745) to be used. Sincerun_general_vm_preferencesis a plain helper, the cleanest fix is to push the architecture check up to the test methods (which can request the fixture) and pass it down:🔧 Suggested fix
-def run_general_vm_preferences(client, namespace, preferences): - is_s390x = get_cluster_architecture() == S390X +def run_general_vm_preferences(client, namespace, preferences, is_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 start_vm_with_cluster_preference( client=client, preference_name=preference_name, namespace_name=namespace.name, )And update each call site to request the
is_s390x_clusterfixture and forward it, e.g.:- def test_common_vm_preference_windows(self, unprivileged_client, namespace): + def test_common_vm_preference_windows(self, unprivileged_client, namespace, is_s390x_cluster): run_general_vm_preferences( client=unprivileged_client, namespace=namespace, preferences=[pref for pref in VM_PREFERENCES_LIST["windows"] if pref not in {"windows.2k3", "windows.xp"}], + is_s390x=is_s390x_cluster, )(apply analogously to
test_common_vm_preference_linuxandtest_common_vm_preference_dpdk).After this, the
get_cluster_architectureimport at line 10 and theS390Ximport at line 11 are no longer needed in this file.Based on learnings,
get_cluster_architecture()returns a singleton set, not a string, so equality againstS390Xis never true; and maintainer rnetser requested theis_s390x_clusterfixture be used instead of re-deriving the check.🤖 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 60 - 78, run_general_vm_preferences currently computes is_s390x using get_cluster_architecture() == S390X which always yields False; move the cluster-architecture decision into the test(s) and accept a boolean parameter is_s390x_cluster instead of deriving it inside the helper: change run_general_vm_preferences signature to accept is_s390x_cluster (and keep _preference_requires_efi as-is), update callers (e.g., test_common_vm_preference_linux, test_common_vm_preference_dpdk, test_common_vm_preference_windows) to request the existing is_s390x_cluster fixture and pass it through, and then remove the now-unused get_cluster_architecture and S390X imports from this file.tests/conftest.py (1)
2197-2203:⚠️ Potential issue | 🔴 CriticalCRITICAL: s390x guard is dead code — comparison can never be true, and maintainer asked for the existing fixture.
get_cluster_architecture()returns aset[str](seeutilities/architecture.py), soget_cluster_architecture() == S390Xcompares a set against the string"s390x"and is alwaysFalse. On s390x theelsebranch still runs andpreferredTimer(an x86-only timer) is set on theVirtualMachinePreference— exactly the failure this PR is meant to fix.Additionally, this file already defines the canonical
is_s390x_clusterfixture at lines 2743–2745, and reviewer rnetser explicitly asked for it to be used here. Wire it through the fixture parameters instead of re-implementing the check (and re-implementing it incorrectly):🔧 Suggested fix
`@pytest.fixture`(scope="class") -def common_vm_preference_param_dict(request): +def common_vm_preference_param_dict(request, is_s390x_cluster): common_preference_dict = { "name": request.param["name"], "client": request.param.get("client"), "teardown": request.param.get("teardown", True), "yaml_file": request.param.get("yaml_file"), } if request.param.get("clock_timezone") or request.param.get("clock_utc_seconds_offset"): common_preference_dict["clock"] = { "preferredClockOffset": { "timezone": request.param.get("clock_timezone"), "utc": {"offsetSeconds": request.param.get("clock_utc_seconds_offset")}, } } if request.param.get("clock_preferred_timer"): - if get_cluster_architecture() == S390X: + if is_s390x_cluster: 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"]If
is_s390x_clusteris adopted, theget_cluster_architectureimport added at line 77 and theS390Ximport on line 113 also become unused here and should be removed.Based on learnings,
get_cluster_architecture()returns a singleton set in this repo, so equality against theS390Xstring is never true; and maintainer rnetser requested theis_s390x_clusterfixture be used instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` around lines 2197 - 2203, Replace the broken architecture check that compares get_cluster_architecture() to S390X with the existing is_s390x_cluster fixture: add is_s390x_cluster as a parameter to the fixture/function that contains the shown block, then change the condition to only set common_preference_dict.setdefault("clock", {})["preferredTimer"] when request.param.get("clock_preferred_timer") is truthy AND is_s390x_cluster is False; keep the LOGGER.info branch for the s390x case. After that remove the now-unused imports get_cluster_architecture and S390X from the top of the file. Ensure references to request.param and common_preference_dict remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/conftest.py`:
- Around line 2197-2203: Replace the broken architecture check that compares
get_cluster_architecture() to S390X with the existing is_s390x_cluster fixture:
add is_s390x_cluster as a parameter to the fixture/function that contains the
shown block, then change the condition to only set
common_preference_dict.setdefault("clock", {})["preferredTimer"] when
request.param.get("clock_preferred_timer") is truthy AND is_s390x_cluster is
False; keep the LOGGER.info branch for the s390x case. After that remove the
now-unused imports get_cluster_architecture and S390X from the top of the file.
Ensure references to request.param and common_preference_dict remain unchanged.
In `@tests/infrastructure/instance_types/test_common_vm_preference.py`:
- Around line 60-78: run_general_vm_preferences currently computes is_s390x
using get_cluster_architecture() == S390X which always yields False; move the
cluster-architecture decision into the test(s) and accept a boolean parameter
is_s390x_cluster instead of deriving it inside the helper: change
run_general_vm_preferences signature to accept is_s390x_cluster (and keep
_preference_requires_efi as-is), update callers (e.g.,
test_common_vm_preference_linux, test_common_vm_preference_dpdk,
test_common_vm_preference_windows) to request the existing is_s390x_cluster
fixture and pass it through, and then remove the now-unused
get_cluster_architecture and S390X imports from this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1c1425c-bdbb-463a-8975-e7f680ff5709
📒 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
chandramerla
left a comment
chandramerla
left a comment
There was a problem hiding this comment.
Thanks for the review and sorry for getting back late on this.
Now, I've addressed the review comments and testing the changes locally
|
|
||
|
|
||
| def run_general_vm_preferences(client, namespace, preferences): | ||
| is_s390x = get_cluster_architecture() == S390X |
| Check connectivity | ||
| """ | ||
| dst_ip = get_ip_from_vm_or_virt_handler_pod(family=ip_family, vm=pod_net_running_vmb) | ||
| dst_ip = get_ip_from_vm_or_virt_handler_pod(family=ip_family, vm=pod_net_running_vmb, wait_timeout=TIMEOUT_90SEC) |
There was a problem hiding this comment.
AFAIR I've seen this when running the tests locally - not getting the IP always probably as during startup it takes some time to initialize.
Anyway reverted from this PR for now. I will cross check if this is required and raise a separate PR if it is the case, with more details.
| } | ||
| if request.param.get("clock_preferred_timer"): | ||
| common_preference_dict.setdefault("clock", {})["preferredTimer"] = request.param["clock_preferred_timer"] | ||
| if get_cluster_architecture() == S390X: |
- Fix centos DataSource name: use os_name="centos-stream" to generate correct "centos-stream9" DataSource name instead of "centos9" - 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
|
Clean rebase detected — no code changes compared to previous head ( |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4455. Overlapping filestests/conftest.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4725. Overlapping filestests/global_config_s390x.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4605. Overlapping filestests/storage/cdi_clone/test_clone.py |
|
/lgtm |
|
@chandramerla thanks, please, update the PR description to match the PR content. After you recent fixes some of the lines should be deleted, like "Fix centos DataSource name" and "Add retry logic to get_ip_from_vm_or_virt_handler_pod". Also please don't forget to add |
11 participants
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