Net multi arch regression#4810
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:
📝 WalkthroughWalkthroughAdds an optional ChangesArchitecture-aware VM configuration and test refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
/build-and-push-container |
|
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-2
requested review from
EdDev,
RoniKishner,
dshchedr,
frenzyfriday,
nirdothan,
orelmisan and
servolkov
|
/build-and-push-container |
|
/retest build-container |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4810 published |
1 similar comment
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4810 published |
|
/wip |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Modified symbols in libs/vm/factory.py ==="
git diff origin/main...HEAD --unified=0 -- libs/vm/factory.py | grep '^[+-]def '
echo ""
echo "=== Modified fields/lines in libs/vm/spec.py ==="
git diff origin/main...HEAD --unified=0 -- libs/vm/spec.py
echo ""
echo "=== Same-file callers of fedora_image in libs/vm/factory.py ==="
rg -n "fedora_image" libs/vm/factory.py
echo ""
echo "=== Direct callers of fedora_image in tests/ and libs/ ==="
rg -rn "fedora_image\b" tests/ libs/ --type py -l
echo ""
echo "=== Transitive: functions that call fedora_image (same file) ==="
rg -n "fedora_vm\|fedora_image" libs/vm/factory.py
echo ""
echo "=== Files calling fedora_vm in tests/ and conftest files ==="
rg -rn "fedora_vm\b" tests/ --type py -l
echo ""
echo "=== Smoke test files ==="
rg -rn "`@pytest.mark.smoke`" tests/ --type py -l
echo ""
echo "=== Gating test files ==="
rg -rn "`@pytest.mark.gating`" tests/ --type py -l
echo ""
echo "=== Do gating files import from libs.vm? ==="
GATING_FILES=$(rg -rn "`@pytest.mark.gating`" tests/ --type py -l)
for f in $GATING_FILES; do
if rg -q "from libs\.vm|import libs\.vm" "$f" 2>/dev/null; then
echo "GATING+LIBS.VM MATCH: $f"
rg -n "from libs\.vm|import libs\.vm" "$f"
fi
done
echo "Gating libs.vm check done"
echo ""
echo "=== Do smoke files import from libs.vm? ==="
SMOKE_FILES=$(rg -rn "`@pytest.mark.smoke`" tests/ --type py -l)
for f in $SMOKE_FILES; do
if rg -q "from libs\.vm|import libs\.vm" "$f" 2>/dev/null; then
echo "SMOKE+LIBS.VM MATCH: $f"
rg -n "from libs\.vm|import libs\.vm" "$f"
fi
done
echo "Smoke libs.vm check done"
echo ""
echo "=== Conftest files along fedora_vm caller paths - libs.vm imports ==="
FEDORA_VM_FILES=$(rg -rn "fedora_vm\b" tests/ --type py -l)
for f in $FEDORA_VM_FILES; do
dir=$(dirname "$f")
while [ "$dir" != "." ] && [ "$dir" != "/" ]; do
conftest="$dir/conftest.py"
if [ -f "$conftest" ]; then
if rg -q "from libs\.vm|import libs\.vm" "$conftest" 2>/dev/null; then
echo "CONFTEST WITH LIBS.VM: $conftest (from caller: $f)"
fi
fi
dir=$(dirname "$dir")
done
done
echo "Conftest check done"
echo ""
echo "=== Check multiarch_excluded in current diff ==="
git diff origin/main...HEAD | grep -i "multiarch_excluded" || echo "NOT FOUND in diff"
echo ""
echo "=== PR changed files ==="
git diff origin/main...HEAD --name-onlyLength of output: 8371 🏁 Script executed: #!/bin/bash
echo "=== tests/network/localnet/conftest.py - libs.vm imports and fedora_vm usage ==="
rg -n "from libs\.vm|import libs\.vm|fedora_vm" tests/network/localnet/conftest.py
echo ""
echo "=== tests/network/localnet/liblocalnet.py - libs.vm imports and fedora_vm usage ==="
rg -n "from libs\.vm|import libs\.vm|fedora_vm" tests/network/localnet/liblocalnet.py
echo ""
echo "=== tests/virt/node/owner_references/test_vm_owner_references.py - gating + fedora_vm check ==="
rg -n "fedora_vm\|from libs\.vm\|import libs\.vm" tests/virt/node/owner_references/test_vm_owner_references.py | head -10
echo ""
echo "=== tests/virt/cluster/vm_cloning/test_vm_cloning.py - gating + fedora_vm check ==="
rg -n "fedora_vm\|from libs\.vm\|import libs\.vm" tests/virt/cluster/vm_cloning/test_vm_cloning.py | head -10
echo ""
echo "=== Conftest files in the directory hierarchy of gating fedora_vm callers ==="
# Check if tests/virt/node/owner_references/ or its ancestors have a conftest importing libs.vm.factory
for dir in "tests/virt/node/owner_references" "tests/virt/node" "tests/virt" "tests"; do
conftest="$dir/conftest.py"
if [ -f "$conftest" ]; then
if rg -q "from libs\.vm\.factory\|from libs\.vm import factory\|fedora_image\|_fill_vm_spec_defaults" "$conftest" 2>/dev/null; then
echo "FACTORY MATCH in conftest: $conftest"
fi
if rg -q "from libs\.vm|import libs\.vm" "$conftest" 2>/dev/null; then
echo "LIBS.VM import in: $conftest"
rg -n "from libs\.vm|import libs\.vm" "$conftest"
fi
fi
done
echo ""
echo "=== Confirm: does libs/vm/factory.py fedora_image change affect VMISpec.architecture? ==="
grep -n "architecture\|_fill_vm_spec_defaults\|fedora_image\|vmi_spec" libs/vm/factory.py | head -20Length of output: 3577 Test Execution Plan
Run smoke tests: False Verified: none of the 6 smoke-marked files reside under any conftest directory that imports Run gating tests: False Dependency trace confirmed:
Affected tests to run:
Both tests verified by the author on arm64 multi-arch, amd64 multi-arch, and homogeneous clusters — all passed. |
servolkov
left a comment
servolkov
left a comment
There was a problem hiding this comment.
I think the description contains outdated info (like excluded tests). Pls. reflect accordingly what has been done in the PR.
|
/verified Verified by running the tests on 3 different setups, making sure the tests are passing and that the VM spec is created with the expected
|
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== PR changed files ==="
git diff origin/main...HEAD --name-only
echo ""
echo "=== First changed line info for libs/vm/factory.py ==="
git diff origin/main...HEAD -- libs/vm/factory.py | head -30
echo ""
echo "=== Modified symbols in libs/vm/factory.py ==="
git diff origin/main...HEAD --unified=0 -- libs/vm/factory.py | grep '^[+-][^+-]' | head -20
echo ""
echo "=== Smoke test files ==="
rg -rn "`@pytest.mark.smoke`" tests/ --type py -l
echo ""
echo "=== Gating test files ==="
rg -rn "`@pytest.mark.gating`" tests/ --type py -l
echo ""
echo "=== Direct callers of fedora_image in tests/ ==="
rg -rn "fedora_image\b" tests/ --type py -l
echo ""
echo "=== Same-file callers of _fill_vm_spec_defaults ==="
rg -n "_fill_vm_spec_defaults\|fedora_image" libs/vm/factory.py
echo ""
echo "=== Do smoke files import libs.vm? ==="
SMOKE_FILES=$(rg -rn "`@pytest.mark.smoke`" tests/ --type py -l)
for f in $SMOKE_FILES; do
if rg -q "from libs\.vm|import libs\.vm" "$f" 2>/dev/null; then
echo "SMOKE+LIBS.VM: $f"
fi
done
echo "Smoke check done"
echo ""
echo "=== Do gating files import libs.vm? ==="
GATING_FILES=$(rg -rn "`@pytest.mark.gating`" tests/ --type py -l)
for f in $GATING_FILES; do
if rg -q "from libs\.vm|import libs\.vm" "$f" 2>/dev/null; then
echo "GATING+LIBS.VM: $f"
fi
done
echo "Gating check done"
echo ""
echo "=== Get commit SHA for PR HEAD to use for inline comment ==="
git rev-parse HEAD
echo ""
echo "=== Get the exact line numbers changed in libs/vm/factory.py ==="
git diff origin/main...HEAD --unified=0 -- libs/vm/factory.py | grep "^@@"
echo ""
echo "=== libs/vm/factory.py diff ==="
git diff origin/main...HEAD -- libs/vm/factory.pyLength of output: 8995 🏁 Script executed: #!/bin/bash
# Post inline review comment on libs/vm/factory.py line 4 (first changed line)
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4810/reviews \
-X POST \
-H "Accept: application/vnd.github+json" \
--field event="COMMENT" \
--field commit_id="5494c258197a8eca64a77a744c9893305cf2e02d" \
--field body="" \
--field "comments[][path]=libs/vm/factory.py" \
--field "comments[][line]=4" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False**
Verified: none of the 6 smoke-marked files (`tests/virt/node/general/test_container_disk_vm.py`, `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py`, `tests/storage/cdi_upload/test_upload_virtctl.py`, `tests/install_upgrade_operators/csv/test_csv.py`, `tests/infrastructure/instance_types/supported_os/test_rhel_os.py`, `tests/storage/cdi_clone/test_clone.py`) import from \`libs.vm\`. No conftest in their directory hierarchy imports \`libs.vm.factory\` or \`libs.vm.spec\`.
**Run gating tests: False**
Verified: all 46 gating-marked files checked — none import from \`libs.vm\`. The \`_fill_vm_spec_defaults\` and \`fedora_image\` symbols have no direct or transitive callers in gating tests.
**Affected tests to run:**
| Path | Reason |
|---|---|
| \`tests/network/user_defined_network/ip_specification/test_ip_specification.py::TestVMWithExplicitIPAddressSpecification::test_vm_is_started_with_successful_connectivity\` | Guest interface name read dynamically from \`vmi.interfaces[0].interfaceName\`; cloud-init key \`\"eth0\"\` → \`\"primary\"\`; \`_fill_vm_spec_defaults\` now sets \`vmi_spec.architecture\` from \`py_config[\"cpu_arch\"]\` on MULTIARCH clusters |
| \`tests/network/user_defined_network/ip_specification/test_ip_specification.py::TestVMWithExplicitIPAddressSpecification::test_ip_address_is_preserved_over_power_cycle\` | Same logic changes as above |
Both tests verified by the author across arm64 multi-arch, amd64 multi-arch, and homogeneous clusters — all passed."
echo "Exit code: $?"Length of output: 4408 |
|
/verified Verified by running the tests on 3 different setups, making sure the tests are passing and that the VM spec is created with the expected
|
Add arcitecture support to VM specs. This change includes accessing `constants.Images` rather than importing `Images` at module level to ensure the updated Images on run-time is referenced. Retrieve guest interface name dynamically Signed-off-by: Yossi Segev <ysegev@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
/verified Verified by running the tests on 3 different setups, making sure the tests are passing and that the VM spec is created with the expected
|
|
Clean rebase detected — no code changes compared to previous head ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Current HEAD SHA ==="
git rev-parse HEAD
echo ""
echo "=== First changed line in libs/vm/factory.py ==="
git diff origin/main...HEAD --unified=0 -- libs/vm/factory.py | grep "^@@" | head -3
echo ""
echo "=== PR changed files ==="
git diff origin/main...HEAD --name-only
echo ""
echo "=== Post inline review comment ==="
HEAD_SHA=$(git rev-parse HEAD)
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4810/reviews \
-X POST \
-H "Accept: application/vnd.github+json" \
--field event="COMMENT" \
--field commit_id="$HEAD_SHA" \
--field body="" \
--field "comments[][path]=libs/vm/factory.py" \
--field "comments[][line]=4" \
--field "comments[][side]=RIGHT" \
--field "comments[][body]=**Test Execution Plan**
**Run smoke tests: False**
Verified: none of the 6 smoke-marked files (`tests/virt/node/general/test_container_disk_vm.py`, `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py`, `tests/storage/cdi_upload/test_upload_virtctl.py`, `tests/install_upgrade_operators/csv/test_csv.py`, `tests/infrastructure/instance_types/supported_os/test_rhel_os.py`, `tests/storage/cdi_clone/test_clone.py`) import from \`libs.vm\`. Full hierarchy scan of conftest files in their directory chains confirmed no import of \`libs.vm.factory\` or \`libs.vm.spec\`.
**Run gating tests: False**
Verified: all gating-marked files checked — none import from \`libs.vm\`. The modified symbols (\`_fill_vm_spec_defaults\` arch defaulting logic, \`VMISpec.architecture\` field) have no direct or transitive callers in any gating test.
**Affected tests to run:**
| Path | Reason |
|---|---|
| \`tests/network/user_defined_network/ip_specification/test_ip_specification.py::TestVMWithExplicitIPAddressSpecification::test_vm_is_started_with_successful_connectivity\` | Guest interface name read dynamically from \`vmi.interfaces[0].interfaceName\`; cloud-init key \`\"eth0\"\` → \`\"primary\"\`; \`_fill_vm_spec_defaults\` sets \`vmi_spec.architecture\` from \`py_config[\"cpu_arch\"]\` on MULTIARCH clusters |
| \`tests/network/user_defined_network/ip_specification/test_ip_specification.py::TestVMWithExplicitIPAddressSpecification::test_ip_address_is_preserved_over_power_cycle\` | Same logic changes as above |
Both tests verified by the PR author on arm64 multi-arch, amd64 multi-arch, and homogeneous clusters — all passed."
echo "Exit code: $?"Length of output: 5230 |
|
/approve |
|
smoke tests are not needed; override |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-4810. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
|
/cherry-pick cnv-4.22 |
|
Cherry-picked PR Net multi arch regression into cnv-4.22: #5267 |
15 participants
Short description:
Adjust network tests to be able to run on specific arch on multi-arch clusters, and also handle instabilities that was exposed while working on multi-architecture support.
More details:
This change includes accessing
constants.Imagesrather than importingImagesat module level to ensure the updated Images on run-time isreferenced.
What this PR does / why we need it:
This is required for testing regression of network tests on multi-arch clusters.
jira-ticket:
https://redhat.atlassian.net/browse/CNV-80548