[WIP]Add vnic_info metric test for NAD reference change#5090
Closed
OhadRevah wants to merge 8 commits into
Closed
Conversation
These modules have no dependency on test-specific code. Moving them to libs/net/ makes them importable from any test suite without cross-package imports. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Asia Khromov <azhivovk@redhat.com>
Add cluster_vlan_iterator() to libs/net/cluster.py. The underlying VLAN list is read from py_config once via a cached helper; each call returns a fresh generator so every fixture invocation starts from the beginning. Add next_vlan_index_number fixture to tests/conftest.py using this helper so any test suite can allocate cluster VLANs without cross-package imports. Network tests continue to use their own fixture in tests/network/conftest.py and will be adjusted in a follow-up. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Asia Khromov <azhivovk@redhat.com>
Wait condition helpers use vm.vmi which exists on both BaseVirtualMachine and VirtualMachineForTests. Widen the type to VirtualMachine so any subclass can be passed without a type error. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Asia Khromov <azhivovk@redhat.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
📝 WalkthroughWalkthroughThis PR consolidates test-local helpers into ChangesLibrary Migration and NAD Swap Test Enable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The PR combines import consolidation (repetitive but low-risk), library helper addition with clear logic (NAD-swap workflow), fixture provisioning (straightforward fixture patterns), and test enabling (validation logic refactor adds modest complexity). Multiple files affected, but changes follow consistent patterns. The Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
OhadRevah
changed the title
|
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
Anatw,
EdDev,
albarker-rh,
azhivovk,
dshchedr,
nirdothan,
rlobillo,
rnetser,
servolkov and
yossisegev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
echo "=== Get current PR head SHA ==="
HEAD_SHA=$(gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5090 --jq '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Smoke-marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Gating-marked test files ==="
rg -l '`@pytest.mark.gating`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Callers of cluster_vlan_iterator ==="
rg -n 'cluster_vlan_iterator' --type py || echo "NONE"
echo ""
echo "=== Callers of wait_for_vmi_condition_status / wait_for_no_vmi_condition in tests/ ==="
rg -n 'wait_for_vmi_condition_status\|wait_for_no_vmi_condition' tests/ --type py || echo "NONE"
echo ""
echo "=== update_nad_references callers ==="
rg -n 'update_nad_references' --type py || echo "NONE"
echo ""
echo "=== Post inline comment ==="
gh api \
--method POST \
repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments \
--field body='**Test Execution Plan**
**Run smoke tests: False**
Verified: `rg -l "`@pytest.mark.smoke`" tests/` returns files only in `tests/storage/` and `tests/virt/`. None of those files or their conftest hierarchy import `cluster_vlan_iterator`, `next_vlan_index_number`, `wait_for_vmi_condition_status`, or `wait_for_no_vmi_condition`. No traceable dependency path exists.
**Run gating tests: False**
Verified: `rg -l "`@pytest.mark.gating`" tests/` returns no files that import or transitively call any modified symbol.
**Affected tests to run**
New test (primary deliverable):
- `tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap`
Import-path-only refactors (`libnncp`: `tests.network.libs.*` → `libs.net.*`) — collection verification only (`pytest --collect-only`) for:
- `tests/network/bgp/`
- `tests/network/l2_bridge/`
- `tests/network/localnet/`' \
--field commit_id="$HEAD_SHA" \
--field path='libs/net/cluster.py' \
--field position=3
echo "Done"Length of output: 10121 🧠 Learnings used |
Contributor
Author
|
/verified All tests pass, build 5580 |
Contributor
|
/lgtm |
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5135. Overlapping filestests/utils.py |
Move update_nad_references to libs/net/vmspec.py to eliminate identical copies in tests/utils.py and tests/network/l2_bridge/nad_ref_change/lib_helpers.py. The function is used by both network and observability team directories, so it belongs in a shared module per project conventions. assisted by: claude code claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ohad <orevah@redhat.com>
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 `@libs/net/vmspec.py`:
- Around line 316-321: The code currently masks errors by defaulting networks to
[] and silently skipping unmatched nad_name_by_net keys; instead, fail fast:
remove the "or []" fallback so deepcopy(vm.template_spec.networks) must be
present and raise a clear exception if vm.template_spec.networks is None/empty,
and before mutating, validate that every network.name in the copied networks
exists in nad_name_by_net (raise if any missing) so you don't apply a
destructive/no-op patch; then proceed to update network.multus.networkName, call
vm.set_networks(networks=networks) and wait_for_vmi_condition_status as before.
🪄 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: 39ea1de5-c3a2-4b79-80e0-8444934af42e
📒 Files selected for processing (4)
libs/net/vmspec.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/observability/metrics/conftest.py
💤 Files with no reviewable changes (1)
- tests/network/l2_bridge/nad_ref_change/lib_helpers.py
Signed-off-by: Ohad <orevah@redhat.com>
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5090 published |
|
/verified All tests passed for PR #5090. Execution details |
Contributor
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: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
HEAD_SHA=$(gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5090 --jq '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Smoke-marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Gating-marked test files ==="
rg -l '`@pytest.mark.gating`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Callers of cluster_vlan_iterator in tests/ ==="
rg -n 'cluster_vlan_iterator' tests/ --type py || echo "NONE"
echo ""
echo "=== Callers of wait_for_vmi_condition_status / wait_for_no_vmi_condition in tests/ ==="
rg -n 'wait_for_vmi_condition_status\|wait_for_no_vmi_condition' tests/ --type py || echo "NONE"
echo ""
echo "=== update_nad_references callers in tests/ ==="
rg -n 'update_nad_references' tests/ --type py || echo "NONE"
echo ""
echo "=== Post inline comment ==="
gh api \
--method POST \
repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments \
--field body='**Test Execution Plan**
**Run smoke tests: False**
Verified: `rg -l "`@pytest.mark.smoke`" tests/` returns files only in `tests/storage/` and `tests/virt/`. None of those files or their conftest hierarchy import `cluster_vlan_iterator`, `next_vlan_index_number`, `wait_for_vmi_condition_status`, or `wait_for_no_vmi_condition`. No traceable dependency path exists.
**Run gating tests: False**
Verified: `rg -l "`@pytest.mark.gating`" tests/` returns no files that import or transitively call any modified symbol (`cluster_vlan_iterator`, `wait_for_vmi_condition_status`, `wait_for_no_vmi_condition`, `update_nad_references`).
**Affected tests to run**
New test (primary deliverable of this PR):
- `tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap`
Import-path-only refactors (`libnncp`: `tests.network.libs.*` → `libs.net.*`) — collection verification only:Length of output: 6766 🏁 Script executed: #!/bin/bash
set -e
echo "=== Check gating tests for next_vlan_index_number ==="
rg -n 'next_vlan_index_number' tests/ --type py || echo "NONE"
echo ""
echo "=== Post inline comment using python to avoid quoting issues ==="
python3 - <<'PYEOF'
import subprocess, json, sys
body = """**Test Execution Plan**
**Run smoke tests: False**
Verified: smoke-marked files are in `tests/storage/` and `tests/virt/`. The `next_vlan_index_number` fixture (module-scope, non-autouse) is only consumed in `tests/observability/metrics/conftest.py`. No smoke test requests it, and no smoke conftest hierarchy imports `cluster_vlan_iterator`. No traceable dependency path.
**Run gating tests: False**
Verified: `next_vlan_index_number` is not used in any gating-marked test. `wait_for_vmi_condition_status`/`wait_for_no_vmi_condition` have zero callers in `tests/`. `update_nad_references` callers (`tests/network/l2_bridge/nad_ref_change/`, `tests/network/localnet/nad_ref_change/`) are not gating-marked. No traceable dependency path.
**Affected tests to run**
New test (primary deliverable of this PR):
- `tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap`
Import-path-only refactors (`libnncp`: `tests.network.libs.*` -> `libs.net.*`) — collection verification only (`pytest --collect-only`) for:
- `tests/network/bgp/`
- `tests/network/l2_bridge/`
- `tests/network/localnet/`"""
payload = {
"body": body,
"commit_id": "1ea7591a62a5ee1f129f6afe7de1ceafb30df08b",
"path": "libs/net/cluster.py",
"position": 3
}
result = subprocess.run(
["gh", "api", "--method", "POST",
"repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments",
"--input", "-"],
input=json.dumps(payload).encode(),
capture_output=True
)
print("STDOUT:", result.stdout.decode()[:500])
print("STDERR:", result.stderr.decode()[:500])
print("RC:", result.returncode)
PYEOFLength of output: 3136 |
Anatw
reviewed
Jun 9, 2026
Anatw
left a comment
Anatw
left a comment
Contributor
There was a problem hiding this comment.
Thank you, Ohad!
I stopped the review after the 5th commit. Will continue after the commits are squashed, it is hard to review.
|
Clean rebase detected — no code changes compared to previous head ( |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5090 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2461 -26
==========================================
- Hits 2454 2428 -26
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/observability/metrics/utils.py (1)
601-609:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Order-dependent VNIC validation can fail even when a matching metric exists.
At Line 602, the code validates only
result[0]. If Prometheus returns multiple series, the first one may not be the target interface, causing false timeout/failure. Match against any returned metric entry and succeed when one fully matches.Suggested fix
- if sample and (result := sample.get("data", {}).get("result")): - vnic_info_metric_result = result[0].get("metric") - mismatch_vnic_info = {} - for info, expected_value in vnic_info_to_compare.items(): - actual_value = vnic_info_metric_result.get(info) - if actual_value != expected_value: - mismatch_vnic_info[info] = {f"Expected: {expected_value}", f"Actual: {actual_value}"} - if not mismatch_vnic_info: - return + if sample and (result := sample.get("data", {}).get("result")): + mismatch_vnic_info = None + for metric_result in result: + vnic_info_metric_result = metric_result.get("metric", {}) + current_mismatch = {} + for info, expected_value in vnic_info_to_compare.items(): + actual_value = vnic_info_metric_result.get(info) + if actual_value != expected_value: + current_mismatch[info] = { + f"Expected: {expected_value}", + f"Actual: {actual_value}", + } + if not current_mismatch: + return + mismatch_vnic_info = current_mismatch🤖 Prompt for 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. In `@tests/observability/metrics/utils.py` around lines 601 - 609, The validation currently inspects only result[0], which is order-dependent; modify the logic that processes sample.get("data", {}).get("result") to iterate over all entries in result and for each entry compare its metric (vnic_info_metric_result) against vnic_info_to_compare, returning early if any entry fully matches; if none match, build a mismatch_vnic_info (or aggregate per-entry differences) and proceed with the failure path as before. Ensure you update the loop around result and keep variable names vnic_info_metric_result, mismatch_vnic_info and vnic_info_to_compare so the change is localized and easy to review.tests/observability/metrics/test_vms_metrics.py (1)
47-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Fail fast in
get_last_transition_timeinstead of returning implicitNoneOn Line 48, if
status.conditionsis missing or READY is absent, this function falls through with implicitNone. That delays the real failure until the timeout loop and obscures root cause. Raise immediately with context.Proposed fix
def get_last_transition_time(vm): - for condition in vm.instance.get("status", {}).get("conditions"): + conditions = vm.instance.get("status", {}).get("conditions") + if not conditions: + raise ValueError(f"VM {vm.name} has no status.conditions") + + for condition in conditions: if condition.get("type") == vm.Condition.READY: last_transition_time = condition.get("lastTransitionTime") return int((datetime.strptime(last_transition_time, "%Y-%m-%dT%H:%M:%SZ").replace(tzinfo=UTC)).timestamp()) + raise ValueError(f"VM {vm.name} has no READY condition")As per coding guidelines: “No defensive programming. Fail-fast and don't hide bugs with fake defaults.” Ruff also flags this path with RET503.
🤖 Prompt for 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. In `@tests/observability/metrics/test_vms_metrics.py` around lines 47 - 51, The get_last_transition_time function currently can return None when status.conditions is missing or no READY condition is present; change it to fail fast by explicitly checking that vm.instance.get("status", {}).get("conditions") exists and is a sequence and raise a ValueError with context if not, then iterate conditions to find the one where condition.get("type") == vm.Condition.READY, parse its "lastTransitionTime" as before and return the timestamp, and if no READY condition is found raise a ValueError that includes vm identifying information so the caller sees the real cause instead of getting an implicit None.Sources: Coding guidelines, Linters/SAST tools
🤖 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 `@libs/net/vmspec.py`:
- Around line 315-320: The code allows continuing to patch and wait even when
nad_name_by_net is empty; before performing the patch/wait logic (after
computing nad_name_by_net and before using updated_names/networks), add a
fail-fast check: if nad_name_by_net is empty, raise a ValueError (or appropriate
exception) with a clear message including vm.name so the caller gets an
immediate, actionable error instead of a no-op timeout. Locate the logic that
uses nad_name_by_net/updated_names/networks in this function (the block that
computes resource_version, networks, updated_names) and insert the validation
there.
In `@tests/conftest.py`:
- Line 2783: The line assigns bridge using an unchecked access with a forbidden
"# type: ignore"; remove the suppression and make the typing explicit by
retrieving the interface into a properly annotated variable or casting/asserting
its type before accessing .name (e.g., use a typed local like interface:
InterfaceType = bridge_nncp.desired_state_spec.interfaces[0] or use
cast(InterfaceType, bridge_nncp.desired_state_spec.interfaces[0]) and then set
bridge = interface.name), or add a runtime assert to satisfy the type checker;
target the bridge_nncp.desired_state_spec.interfaces[0].name access and the
bridge variable so the fix replaces the ignore with an explicit type
annotation/cast/assert.
- Line 2764: The test fixture accesses hosts_common_available_ports[-1] without
checking for emptiness; update the fixture setup to first guard that
hosts_common_available_ports is non-empty (e.g., if not
hosts_common_available_ports: raise a clear exception or pytest.skip with a
descriptive message) before constructing
libnncp.Port(name=hosts_common_available_ports[-1]); ensure the check is placed
immediately before the port=[libnncp.Port(...)] usage so the failure is fast and
the error message references hosts_common_available_ports and the fixture that
builds the port list.
In `@tests/utils.py`:
- Around line 787-789: The helper get_vlan_index_number lacks type hints and a
docstring: add type annotations (e.g., def get_vlan_index_number(vlans_list:
Sequence[int]) -> Iterator[int]) and import typing names (Sequence, Iterator) as
needed, and add a Google-style docstring explaining that the function yields
VLAN indices from vlans_list one by one and will raise ValueError when the list
is exhausted (include the ValueError message contract and an example of
usage/side effect). Ensure the docstring documents Args, Yields, and Raises
sections and keep the ValueError message consistent with the existing f-string.
---
Outside diff comments:
In `@tests/observability/metrics/test_vms_metrics.py`:
- Around line 47-51: The get_last_transition_time function currently can return
None when status.conditions is missing or no READY condition is present; change
it to fail fast by explicitly checking that vm.instance.get("status",
{}).get("conditions") exists and is a sequence and raise a ValueError with
context if not, then iterate conditions to find the one where
condition.get("type") == vm.Condition.READY, parse its "lastTransitionTime" as
before and return the timestamp, and if no READY condition is found raise a
ValueError that includes vm identifying information so the caller sees the real
cause instead of getting an implicit None.
In `@tests/observability/metrics/utils.py`:
- Around line 601-609: The validation currently inspects only result[0], which
is order-dependent; modify the logic that processes sample.get("data",
{}).get("result") to iterate over all entries in result and for each entry
compare its metric (vnic_info_metric_result) against vnic_info_to_compare,
returning early if any entry fully matches; if none match, build a
mismatch_vnic_info (or aggregate per-entry differences) and proceed with the
failure path as before. Ensure you update the loop around result and keep
variable names vnic_info_metric_result, mismatch_vnic_info and
vnic_info_to_compare so the change is localized and easy to review.
🪄 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: 2031c349-6636-4453-a04f-f726d1fddf59
📒 Files selected for processing (13)
libs/net/vmspec.pytests/conftest.pytests/network/conftest.pytests/network/l2_bridge/conftest.pytests/network/l2_bridge/migration_stuntime/conftest.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/localnet/liblocalnet.pytests/network/localnet/nad_ref_change/test_nad_ref_change.pytests/network/utils.pytests/observability/metrics/conftest.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.pytests/utils.py
💤 Files with no reviewable changes (3)
- tests/network/conftest.py
- tests/network/utils.py
- tests/network/l2_bridge/conftest.py
Move update_nad_references to libs/net/vmspec.py to eliminate identical copies in tests/utils.py, tests/network/l2_bridge/nad_ref_change/lib_helpers.py, and tests/network/libs/nad_ref.py. The function is used across network and observability team directories, so it belongs in a shared module. Also adds fail-fast validation: raises ValueError for missing networks, non-Multus networks, and unmatched interface names. assisted by: claude code claude-opus-4-6 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Ohad <orevah@redhat.com>
|
/build-and-push-container |
|
Verification failed for PR #5090. Execution details |
|
/build-and-push-container |
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5090 published |
|
/verified All tests passed for PR #5090. Execution details |
Contributor
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: |
|
|
||
| from tests.network.libs.apimachinery import dict_normalization_for_dataclass | ||
| from libs.net.apimachinery import dict_normalization_for_dataclass | ||
|
|
Contributor
There was a problem hiding this comment.
Test Execution Plan
Run smoke tests: False
Verified: smoke-marked files are in tests/storage/, tests/virt/, tests/install_upgrade_operators/, and tests/infrastructure/. None of those files or their conftest hierarchy import cluster_vlan_iterator, next_vlan_index_number, wait_for_vmi_condition_status, wait_for_no_vmi_condition, or update_nad_references. No traceable dependency path.
Run gating tests: False
Verified: gating-marked network test files (tests/network/nmstate/, tests/network/connectivity/, tests/network/localnet/test_default_bridge.py, etc.) have zero imports of any modified symbol. update_nad_references callers (tests/network/l2_bridge/nad_ref_change/, tests/network/localnet/nad_ref_change/) carry no gating marker.
Affected tests to run
New test (primary deliverable of this PR):
tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap
Import-path-only refactors (libnncp: tests.network.libs.* -> libs.net.*) — collection verification only:
pytest --collect-only tests/network/bgp/pytest --collect-only tests/network/l2_bridge/pytest --collect-only tests/network/localnet/
Real tests (cluster required)
tests/conftest.py promotes bridge_nncp (package-scope), bridge_nad_a, bridge_nad_b, vlan_index_number, and vlans_list (session-scope) from per-directory conftest files to the root conftest. These run at session/setup time and must be verified on a real cluster:
Happy path (new observability test end-to-end):
pytest tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap -s
Expected: VM starts attached to bridge_nad_a; NAD swap triggers live migration; kubevirt_vm_vnic_info / kubevirt_vmi_vnic_info labels update to reflect bridge_nad_b.
Regression (existing NAD-ref-change tests still collect after fixture relocation):
pytest --collect-only tests/network/l2_bridge/nad_ref_change/ tests/network/localnet/nad_ref_change/
Expected: session collects without import errors; vlan_index_number resolves from tests/conftest.py (moved from tests/network/conftest.py).
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -e
HEAD_SHA=$(gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5090 --jq '.head.sha')
echo "HEAD SHA: $HEAD_SHA"
echo ""
echo "=== Verify smoke files ==="
rg -l '`@pytest.mark.smoke`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Verify gating files ==="
rg -l '`@pytest.mark.gating`' tests/ --type py || echo "NONE FOUND"
echo ""
echo "=== Callers of wait_for_vmi_condition_status / wait_for_no_vmi_condition in tests/ ==="
rg -n 'wait_for_vmi_condition_status\|wait_for_no_vmi_condition' tests/ --type py || echo "NONE"
echo ""
echo "=== update_nad_references callers ==="
rg -n 'update_nad_references' tests/ --type py || echo "NONE"
echo ""
echo "=== bridge_nncp / vlans_list callers outside conftest ==="
rg -n 'bridge_nncp\|vlans_list\|bridge_nad_a\|bridge_nad_b\|vlan_index_number' tests/ --type py -l || echo "NONE"
echo ""
echo "=== Post inline comment ==="
python3 - <<'PYEOF'
import subprocess, json, sys
HEAD_SHA_RESULT = subprocess.run(
["gh", "api", "repos/RedHatQE/openshift-virtualization-tests/pulls/5090", "--jq", ".head.sha"],
capture_output=True, text=True
)
head_sha = HEAD_SHA_RESULT.stdout.strip()
print(f"Using SHA: {head_sha}")
body = """**Test Execution Plan**
**Run smoke tests: False**
Verified: `rg -l '`@pytest.mark.smoke`' tests/` returns files only in `tests/storage/` and `tests/virt/`. None of those files or their conftest hierarchy import `cluster_vlan_iterator`, `next_vlan_index_number`, `wait_for_vmi_condition_status`, `wait_for_no_vmi_condition`, or `update_nad_references`. No traceable dependency path.
**Run gating tests: False**
Verified: `rg -l '`@pytest.mark.gating`' tests/` returns no files that import or transitively call any modified symbol. `update_nad_references` callers (`tests/network/l2_bridge/nad_ref_change/`, `tests/network/localnet/nad_ref_change/`) carry no gating marker.
**Affected tests to run**
New test (primary deliverable of this PR):
- `tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap`
Import-path-only refactors (`libnncp`: `tests.network.libs.*` → `libs.net.*`) — collection verification only:
- `pytest --collect-only tests/network/bgp/`
- `pytest --collect-only tests/network/l2_bridge/`
- `pytest --collect-only tests/network/localnet/`
**Real tests (cluster required)**
`tests/conftest.py` adds new session/package-scoped fixtures (`bridge_nncp`, `bridge_nad_a`, `bridge_nad_b`, `vlan_index_number`, `vlans_list`) that run at collection/setup time. Verify on a real cluster:
Happy path (new fixtures provision correctly):
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap -s`
Expected: VM starts with `bridge_nad_a`, NAD swap triggers live migration, metric labels update to reflect `bridge_nad_b`.
Regression (existing bridge/VLAN tests still collect and run):
`pytest --collect-only tests/network/l2_bridge/nad_ref_change/`
Expected: session collects without import errors; `vlan_index_number` fixture resolves from `tests/conftest.py` (moved from `tests/network/conftest.py`)."""
payload = {
"body": body,
"commit_id": head_sha,
"path": "libs/net/cluster.py",
"position": 3
}
result = subprocess.run(
["gh", "api", "--method", "POST",
"repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments",
"--input", "-"],
input=json.dumps(payload).encode(),
capture_output=True
)
print("STDOUT:", result.stdout.decode()[:800])
print("STDERR:", result.stderr.decode()[:500])
print("RC:", result.returncode)
PYEOFLength of output: 8439 🏁 Script executed: #!/bin/bash
set -e
echo "=== PR files sorted ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5090/files \
--jq 'sort_by(.filename) | [.[] | {filename: .filename, status: .status, additions: .additions, patch: .patch}]' \
2>/dev/null
echo ""
echo "=== Check gating network tests for modified symbol imports ==="
rg -n 'cluster_vlan_iterator\|next_vlan_index_number\|wait_for_vmi_condition_status\|wait_for_no_vmi_condition\|update_nad_references\|bridge_nncp\|vlans_list\|bridge_nad_a\|bridge_nad_b\|vlan_index_number' \
tests/network/nmstate/test_connectivity_after_nmstate_changes.py \
tests/network/network_service/test_service_config_manifest.py \
tests/network/user_defined_network/test_user_defined_network.py \
tests/network/localnet/test_default_bridge.py \
tests/network/kubemacpool/test_kubemacpool.py \
tests/network/flat_overlay/test_flat_overlay.py \
tests/network/connectivity/test_ovs_linux_bridge.py \
tests/network/connectivity/test_pod_network.py \
tests/network/migration/test_masquerade_connectivity_after_migration.py 2>/dev/null || echo "NONE"Length of output: 29120 🏁 Script executed: #!/bin/bash
python3 - <<'PYEOF'
import subprocess, json
head_sha = "30dbdcd6507f995f4192e44a9702c3e61577cd4c"
body = """**Test Execution Plan**
**Run smoke tests: False**
Verified: smoke-marked files are in `tests/storage/`, `tests/virt/`, `tests/install_upgrade_operators/`, and `tests/infrastructure/`. None of those files or their conftest hierarchy import `cluster_vlan_iterator`, `next_vlan_index_number`, `wait_for_vmi_condition_status`, `wait_for_no_vmi_condition`, or `update_nad_references`. No traceable dependency path.
**Run gating tests: False**
Verified: gating-marked network test files (`tests/network/nmstate/`, `tests/network/connectivity/`, `tests/network/localnet/test_default_bridge.py`, etc.) have zero imports of any modified symbol. `update_nad_references` callers (`tests/network/l2_bridge/nad_ref_change/`, `tests/network/localnet/nad_ref_change/`) carry no gating marker.
**Affected tests to run**
New test (primary deliverable of this PR):
- `tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap`
Import-path-only refactors (`libnncp`: `tests.network.libs.*` -> `libs.net.*`) — collection verification only:
- `pytest --collect-only tests/network/bgp/`
- `pytest --collect-only tests/network/l2_bridge/`
- `pytest --collect-only tests/network/localnet/`
**Real tests (cluster required)**
`tests/conftest.py` promotes `bridge_nncp` (package-scope), `bridge_nad_a`, `bridge_nad_b`, `vlan_index_number`, and `vlans_list` (session-scope) from per-directory conftest files to the root conftest. These run at session/setup time and must be verified on a real cluster:
Happy path (new observability test end-to-end):
`pytest tests/observability/metrics/test_vms_metrics.py::TestVmVnicInfo::test_metric_kubevirt_vm_vnic_info_after_nad_swap -s`
Expected: VM starts attached to `bridge_nad_a`; NAD swap triggers live migration; `kubevirt_vm_vnic_info` / `kubevirt_vmi_vnic_info` labels update to reflect `bridge_nad_b`.
Regression (existing NAD-ref-change tests still collect after fixture relocation):
`pytest --collect-only tests/network/l2_bridge/nad_ref_change/ tests/network/localnet/nad_ref_change/`
Expected: session collects without import errors; `vlan_index_number` resolves from `tests/conftest.py` (moved from `tests/network/conftest.py`)."""
# Try libs/net/nodenetworkconfigurationpolicy.py position 6 (the added import line)
payload = {
"body": body,
"commit_id": head_sha,
"path": "libs/net/nodenetworkconfigurationpolicy.py",
"position": 6
}
result = subprocess.run(
["gh", "api", "--method", "POST",
"repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments",
"--input", "-"],
input=json.dumps(payload).encode(),
capture_output=True
)
print("STDOUT:", result.stdout.decode()[:800])
print("STDERR:", result.stderr.decode()[:500])
print("RC:", result.returncode)
if result.returncode != 0:
print("\n--- Retrying with vmspec.py position 1 ---")
payload2 = {
"body": body,
"commit_id": head_sha,
"path": "libs/net/vmspec.py",
"position": 1
}
result2 = subprocess.run(
["gh", "api", "--method", "POST",
"repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments",
"--input", "-"],
input=json.dumps(payload2).encode(),
capture_output=True
)
print("STDOUT:", result2.stdout.decode()[:800])
print("STDERR:", result2.stderr.decode()[:500])
print("RC:", result2.returncode)
if result2.returncode != 0:
print("\n--- Retrying with tests/conftest.py position 1 ---")
payload3 = {
"body": body,
"commit_id": head_sha,
"path": "tests/conftest.py",
"position": 1
}
result3 = subprocess.run(
["gh", "api", "--method", "POST",
"repos/RedHatQE/openshift-virtualization-tests/pulls/5090/comments",
"--input", "-"],
input=json.dumps(payload3).encode(),
capture_output=True
)
print("STDOUT:", result3.stdout.decode()[:800])
print("STDERR:", result3.stderr.decode()[:500])
print("RC:", result3.returncode)
PYEOFLength of output: 5107 |
azhivovk
suggested changes
Jun 11, 2026
azhivovk
left a comment
azhivovk
left a comment
Contributor
There was a problem hiding this comment.
The commits are a bit messy but I hope the comments are clear
The main necessary change is to adjust the test to the infra commits I pushed
| ) | ||
|
|
||
|
|
||
| def update_nad_references(vm: BaseVirtualMachine, nad_name_by_net: dict[str, str]) -> None: |
Contributor
There was a problem hiding this comment.
All defensive checks are redundant - you can use the helper as is
We create the NADs which we use and if there's a problem with resource creation we'll get an API error
| yield vm | ||
|
|
||
|
|
||
| def get_vlan_index_number(vlans_list: list[int]) -> Generator[int]: |
Contributor
There was a problem hiding this comment.
You can drop the VLAN fixtures changes, we don't need to move it - you can use the new implementation
next(cluster_vlan_ids)
| @@ -13,6 +13,7 @@ | |||
| import tempfile | |||
Contributor
There was a problem hiding this comment.
This conftest is messy and carries many unrelated fixtures
@rnetser suggested to create tests/fixtures/<module.py> and use the fixtures as plugins
You can use the NNCP and NADs fixtures in the metrics conftest where you need
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5090. |
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
11 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:
Implements test_metric_kubevirt_vm_vnic_info_after_nad_swap — verifies that kubevirt_vm_vnic_info and kubevirt_vmi_vnic_info metric labels
update correctly after swapping the VM's secondary network NAD reference and completing the triggered live migration.
Which issue(s) this PR fixes:
Special notes for reviewer:
Based on azhivovk:obs-metrics-nad-ref-vnic-info with vlan_index_number renamed to next_vlan_index_number to match the root conftest fixture in #5072.
jira-ticket:
https://redhat.atlassian.net/browse/CNV-85094
Summary by CodeRabbit
New Features
Tests