net: Nad live-update non-migratable VM test#5212
Conversation
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? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds DataVolume-backed VM spec support, reusable NAD patching, a non-migratable VM path, and a negative test that switches a secondary bridge NAD and verifies VLAN-specific connectivity outcomes. ChangesDataVolume-backed non-migratable VM NAD change test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)libs/net/vmspec.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.12][ERROR]: unable to find a config; path tests/network/l2_bridge/nad_ref_change/conftest.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path tests/network/l2_bridge/nad_ref_change/lib_helpers.py┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.13][ERROR]: unable to find a config; path
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 |
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-6
requested review from
nirdothan,
rnetser and
servolkov
azhivovk
changed the title
openshift-virtualization-qe-bot-6
requested review from
RoniKishner,
frenzyfriday,
orelmisan and
yossisegev
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/vm/factory.py`:
- Around line 44-47: The condition on line 44 incorrectly suppresses the default
containerdisk insertion whenever any DataVolume is present, breaking the ability
to use DataVolumes as additional disks alongside a boot disk. Replace the logic
that checks for any DataVolume with a check that determines whether a boot disk
or containerdisk already exists. The containerdisk should be inserted by default
unless a boot disk is already present, allowing DataVolumes to coexist as
additional disks. Modify the condition in the containerdisk_storage function
call to verify the absence of an actual boot disk rather than the absence of any
DataVolume.
In `@tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py`:
- Around line 244-248: The wait_for_vmi_condition_status call needs to be
anchored to the pre-patch resource state to ensure it validates the
RestartRequired condition is set as a result of the current patch operation, not
from a stale previous state. Capture the resourceVersion of
non_migratable_under_test_vm before calling patch_nad_references, then pass this
resourceVersion to the wait_for_vmi_condition_status call so it waits for a
condition transition that occurs after the patch, not a pre-existing condition
state.
In `@tests/network/libs/nad_ref.py`:
- Around line 27-40: The patch_nad_references function contains duplicate
network mutation logic that is already present in update_nad_references,
specifically the block that iterates through networks and updates the
multus.networkName based on the nad_name_by_net mapping. Extract this common
logic (the deepcopy, the for loop iterating through networks, and the
conditional update of network.multus.networkName) into a separate shared helper
function, then refactor both patch_nad_references and update_nad_references to
call this new helper function instead of duplicating the code.
🪄 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: 70873300-8c28-46c1-a952-0a736d2105fb
📒 Files selected for processing (6)
libs/vm/factory.pylibs/vm/spec.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.pytests/network/libs/nad_ref.py
azhivovk
force-pushed
the
nad-ref-non-migratable
branch
from
524eef8 to
3ab53f3
Compare
|
@jpeimer Hi, please review the DV related changes |
openshift-virtualization-qe-bot
added
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
|
Change: rebase upon main and resolve conflicts |
|
Change: move NAD update helper to vmspec |
| nad_names: list[str], | ||
| ip_addresses: list[list[str]], | ||
| iface_names: list[str], | ||
| data_volume: DataVolume, |
There was a problem hiding this comment.
It's not clear what we expect to get here.
There are at least two kinds of relationships between the VM and the DV:
- VM can have a dataVolumeTemplate (a dictionary) in its spec, so the VM creation will trigger a DataVolume creation, and the VM will own this DataVolume
- VM can use an existing DV that was created by someone else
In this case, data_volume arg is used for the dataVolumeTemplate, so it makes more sense to get the dictionary, and not the DataVolume object.
There was a problem hiding this comment.
the function caller constructs a DataVolume describing what storage they want. The function decides to embed it as a template - this is an implementation detail. This all is about clear function API - accept the typed object.
There was a problem hiding this comment.
The function is using this argument as a dataVolumeTemplate, so it needs to explicitly say it expects the data_volume_template (dictionary), and not a DataVolume object.
There was a problem hiding this comment.
I implemented the second approach
VM can use an existing DV that was created by someone else
Now we create DV separately and the VM references it
That way we can avoid usage of raw dicts and use the DataVolume OCP resource
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Creating a DataVolume separately will make your life much harder.
VM with dataVolumeTemplate is a much more common use.
this is an example how they pass such data to VM: direct DataVolume usage
This still adds a dataVolumeTemplate to the VM spec.
There was a problem hiding this comment.
Hi @rnetser can you share your thoughts?
@servolkov requested to avoid using raw dicts (as we do now in net tests) and create dataclasses
This led to implement usage of DataVolume OCP resource: DV is first created and then referenced by the VM but @jpeimer asks to use DataVolumeTemplate
Using DataVolumeTemplate will require addition of dataclasses to avoid using raw dicts
WDYT?
There was a problem hiding this comment.
This still adds a dataVolumeTemplate to the VM spec.
@jpeimer very well, I don't mind. But I was talking about what they use as a function parameter, not about implementation (as well as the topic here).
Using DataVolumeTemplate will require addition of dataclasses to avoid using raw dicts
not necessary. See your very first implementation.
|
Change: rebase upon main to merge fix of pre-commit and tox errors |
|
Change: use |
| runcmd=runcmd, | ||
| ) | ||
| dv_name = data_volume_template["metadata"]["name"] | ||
| vm.body["spec"]["template"]["spec"]["volumes"].append({"name": dv_name, "dataVolume": {"name": dv_name}}) |
There was a problem hiding this comment.
a raw dict manipulation bypasses the typed spec system that the rest of the our codebase uses. I think you should:
- restore
DataVolumeRefandVolume.dataVolumeinlibs/vm/spec.py- they were in the previous iteration and they're correct: they map to real KubeVirt API fields and follow the same pattern - extract
_bridge_vm_specfromtwo_secondary_bridge_vmso both builders share spec construction - accept
data_volume: DataVolumeinstead ofdata_volume_template: dict[str, Any]-DataVolumeenforces at the type level that only aDataVolumecan be passed, a raw dict throws away the type, the schema, and the API identity.
There was a problem hiding this comment.
Now we create DV separately and the VM references it
That way we can avoid usage of raw dicts and use the DataVolume OCP resource
#5212 (comment)
|
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 `@tests/network/l2_bridge/nad_ref_change/lib_helpers.py`:
- Around line 161-186: The docstring in the helper that builds the Fedora bridge
VM spec is inaccurate and should be updated to match the implementation. Clarify
that the function uses data_volume_storage(data_volume.name) to reference an
already-created DataVolume by name and adds it via add_volume_disk to the VM
spec, rather than creating a dataVolumeTemplate or relying on the VM controller
to provision it. Keep the description aligned with _bridge_vm_spec,
data_volume_storage, and fedora_vm so readers understand the fixture depends on
an existing deployed DataVolume.
🪄 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: 0bdace48-2a5b-4bdf-825c-5bee5b90337a
📒 Files selected for processing (6)
libs/net/vmspec.pylibs/vm/spec.pylibs/vm/vm.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
|
Change: edit docstring |
servolkov
left a comment
servolkov
left a comment
There was a problem hiding this comment.
The PR description looks outdated. In general LGTM, a few cosmetic comments:
| ) -> None: | ||
| """Verify baseline connectivity before the NAD reference change. | ||
|
|
||
| Asserts that the non-migratable under-test VM can reach the reference VM on VLAN-A |
There was a problem hiding this comment.
NIT
avoid mentioning implementation details in such fixtures, the configuration may change (VLAN A and so on). IMO all this docstring is redundant.
| namespace=namespace, | ||
| name=name, | ||
| client=client, | ||
| spec=_bridge_vm_spec( |
There was a problem hiding this comment.
NIT
it's better to put it in a variable for readability.
| ) | ||
|
|
||
|
|
||
| def assert_baseline_connectivity( |
There was a problem hiding this comment.
Since it is used as a helper for the precondition, consider poll_baseline_connectivity or verify_beseline_connectivity, not assert_.
| with DataVolume( | ||
| name="non-migratable-dv", | ||
| namespace=namespace.name, | ||
| source_dict={"blank": {}}, | ||
| access_modes=DataVolume.AccessMode.RWO, | ||
| size="20Gi", | ||
| storage_class=py_config["default_storage_class"], | ||
| api_name="storage", | ||
| client=unprivileged_client, | ||
| ) as dv: | ||
| dv.wait_for_dv_success() | ||
| yield dv |
There was a problem hiding this comment.
How will this work with a WaitForFirstConsumer storage?
There was a problem hiding this comment.
Can you please elaborate?
I need to remove dv.wait_for_dv_success() before yielding the DV, since the VM fixture already guarantees the storage was successfully mounted before the test runs?
There was a problem hiding this comment.
When we create a DV with WaitForFirstConsumer storage, it stays pending until the first consumer (pod/virt-launcher pod) comes and consumes it. But then other issues may come up (PVC and VM scheduled on different nodes / DV and VM architecture mismatch etc). So it's much better and safer to create a VM with dataVolumeTemplate instead of using a pre-created DV.
| ) | ||
|
|
||
|
|
||
| def data_volume_storage(name: str) -> tuple[SpecDisk, Volume]: |
There was a problem hiding this comment.
It's not very clear what data_volume_storage means.
There was a problem hiding this comment.
Renamed to disk_and_volume_for_data_volume
I created it to improve readability of disk and volume creation
| nad_names: list[str], | ||
| ip_addresses: list[list[str]], | ||
| iface_names: list[str], | ||
| data_volume: DataVolume, |
There was a problem hiding this comment.
Creating a DataVolume separately will make your life much harder.
VM with dataVolumeTemplate is a much more common use.
this is an example how they pass such data to VM: direct DataVolume usage
This still adds a dataVolumeTemplate to the VM spec.
|
Change: apply changes after CR
|
and disk-volume helper The VM spec lacked typed support for volumes backed by a DataVolume, forcing callers to manipulate vm.body["spec"] directly and bypass the typed spec layer. DataVolumeRef maps the KubeVirt dataVolume volume source to the same typed pattern used by ContainerDisk and CloudInitNoCloud. disk_and_volume_for_data_volume follows the same factory convention as containerdisk_storage and cloudinitdisk_storage. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
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 318-322: In vmspec’s network update flow, avoid masking a broken
template by defaulting `deepcopy(vm.template_spec.networks)` to an empty list
and then calling `vm.set_networks`; instead, fail fast if the template networks
are missing or invalid. Update the helper that rewrites networks so it only
proceeds when a matching Multus network exists for an entry in
`nad_name_by_net`, and if none of the NAD mappings can be applied, return an
error rather than letting the caller time out with `MigrationRequired` or
`RestartRequired`. Use the `vm.set_networks` path and the
`nad_name_by_net`/`network.multus` checks to locate the fix.
🪄 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: 2c0a9573-2923-4478-baa0-a4638224afe1
📒 Files selected for processing (6)
libs/net/vmspec.pylibs/vm/spec.pylibs/vm/vm.pytests/network/l2_bridge/nad_ref_change/conftest.pytests/network/l2_bridge/nad_ref_change/lib_helpers.pytests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
|
Change: use DV template |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5285. Overlapping fileslibs/vm/vm.py |
A NAD reference change on a non-live-migratable VM cannot be applied without a restart. The expected behavior is that KubeVirt sets a RestartRequired condition and the VM retains connectivity to the original VLAN until restarted. A RWO DataVolume is embedded as a dataVolumeTemplate so the VM owns and creates it. Each DV is bound to exactly one VM; creating it inside the helper ensures the DV is scheduled on the same node as the VM so the RWO PVC can be claimed. Quarantined via CNV-87878: the RestartRequired condition is not yet wired up in KubeVirt, so the wait step will remain skipped until the fix lands and the quarantine is lifted. Signed-off-by: Asia Khromov <azhivovk@redhat.com> Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Change: DV creation in non-migratable VM helper |
|
/verified Dual-stack BM with cnv-4.23: Test is not collected New test is passing when run without waiting for Regression due to helper |
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
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
# Ensure we are on the PR branch clone; derive merge-base against base branch.
base_ref=$(gh pr view "$pr" --repo "$repo" --json baseRefName --jq '.baseRefName')
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
git fetch --quiet origin "$base_ref"
merge_base=$(git merge-base "origin/$base_ref" HEAD)
echo "BASE_REF=$base_ref"
echo "HEAD_SHA=$head_sha"
echo "MERGE_BASE=$merge_base"
echo
echo "=== Changed files ==="
git diff --name-only "$merge_base"...HEAD
echo
echo "=== First changed file and first changed line ==="
first_file=$(git diff --name-only "$merge_base"...HEAD | head -n1)
echo "FIRST_FILE=$first_file"
python - <<'PY' "$merge_base" "$first_file"
import re, subprocess, sys
base, path = sys.argv[1], sys.argv[2]
patch = subprocess.check_output(["git", "diff", "--unified=0", f"{base}...HEAD", "--", path], text=True)
line = None
for ln in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', ln)
if m:
start = int(m.group(1))
# first changed content line on RIGHT side is the hunk start
line = start
break
if line is None:
raise SystemExit("Could not determine first changed line")
print(f"FIRST_LINE={line}")
PY
first_line=$(python - <<'PY' "$merge_base" "$first_file"
import re, subprocess, sys
base, path = sys.argv[1], sys.argv[2]
patch = subprocess.check_output(["git", "diff", "--unified=0", f"{base}...HEAD", "--", path], text=True)
for ln in patch.splitlines():
m = re.match(r'^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@', ln)
if m:
print(m.group(1))
break
PY
)
echo
echo "=== Modified symbols in libs and changed test helpers ==="
for f in libs/net/vmspec.py libs/vm/spec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py; do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --view expanded || true
echo "DIFF SYMBOL LINES:"
git diff --unified=0 "$merge_base"...HEAD -- "$f" | grep -nE '^[+-](def |class |`@pytest`\.fixture|[[:space:]]+[A-Za-z_][A-Za-z0-9_]*\s*:)' || true
echo
fi
done
echo
echo "=== Direct symbol searches in tests and conftest ==="
# Modified / introduced symbols based on summaries and expected diff
symbols=(
patch_nad_references
update_nad_references
wait_for_vmi_condition_status
verify_baseline_connectivity
non_migratable_bridge_vm
disk_and_volume_for_data_volume
DataVolumeRef
)
for sym in "${symbols[@]}"; do
echo "--- symbol: $sym"
rg -n -C2 --glob 'tests/**' --glob 'tests/**/conftest.py' --glob 'tests/**/*.py' "\\b${sym}\\b" tests || true
echo
done
echo
echo "=== Same-file callers / transitive expansion ==="
for f in libs/net/vmspec.py libs/vm/vm.py tests/network/l2_bridge/nad_ref_change/lib_helpers.py tests/network/l2_bridge/nad_ref_change/conftest.py; do
echo "--- $f"
rg -n -C2 'patch_nad_references|update_nad_references|verify_baseline_connectivity|non_migratable_bridge_vm|disk_and_volume_for_data_volume|DataVolumeRef' "$f" || true
echo
done
echo
echo "=== Changed file content slices ==="
sed -n '1,260p' tests/network/l2_bridge/nad_ref_change/conftest.py
echo
sed -n '1,260p' tests/network/l2_bridge/nad_ref_change/lib_helpers.py
echo
sed -n '1,320p' tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py
echo
sed -n '1,240p' libs/net/vmspec.py
echo
sed -n '1,220p' libs/vm/spec.py
echo
sed -n '1,220p' libs/vm/vm.py
echo
echo "=== Smoke and gating files ==="
echo "--- smoke"
rg -l '`@pytest`\.mark\.smoke' tests || true
echo "--- gating"
rg -l '`@pytest`\.mark\.gating' tests || true
echo
echo "=== Intersections / hierarchy checks for smoke and gating ==="
python - <<'PY'
from pathlib import Path
import subprocess, re
# Collect changed symbols and observed affected files manually from rg-based searches
def run(cmd):
return subprocess.check_output(cmd, text=True)
smoke_files = [p for p in run(["rg", "-l", r"`@pytest`\.mark\.smoke", "tests"]).splitlines() if p.strip()]
gating_files = [p for p in run(["rg", "-l", r"`@pytest`\.mark\.gating", "tests"]).splitlines() if p.strip()]
# Candidate affected files based on direct known usages from this PR
affected = set()
for pattern in [
r"\bverify_baseline_connectivity\b",
r"\bnon_migratable_bridge_vm\b",
r"\bpatch_nad_references\b",
]:
out = run(["rg", "-l", pattern, "tests"])
affected.update([p for p in out.splitlines() if p.strip()])
# Conftest hierarchy check for smoke files using modified symbols
symbols = ["patch_nad_references", "verify_baseline_connectivity", "non_migratable_bridge_vm"]
hier_hits = {}
for smoke in smoke_files:
p = Path(smoke).parent
checked = []
while True:
cf = p / "conftest.py"
if cf.exists():
txt = cf.read_text()
for s in symbols:
if re.search(rf"\b{s}\b", txt):
hier_hits.setdefault(smoke, []).append((str(cf), s))
checked.append(str(cf))
if str(p) == ".":
break
p = p.parent
print("SMOKE_FILES_COUNT", len(smoke_files))
print("GATING_FILES_COUNT", len(gating_files))
print("AFFECTED_DIRECT", sorted(affected))
print("SMOKE_HIER_HITS", hier_hits)
print("AFFECTED_GATING", sorted(set(gating_files) & affected))
PY
echo
echo "=== Build test plan body ==="
python - <<'PY' > /tmp/test_plan_comment.txt
from pathlib import Path
import subprocess, re, sys
def run(*cmd):
return subprocess.check_output(list(cmd), text=True)
# Verify actual dependency paths from repo content
conftest = Path("tests/network/l2_bridge/nad_ref_change/conftest.py").read_text()
helpers = Path("tests/network/l2_bridge/nad_ref_change/lib_helpers.py").read_text()
test_file = Path("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py").read_text()
# Determine smoke/gating usage
smoke_files = [p for p in run("rg", "-l", r"`@pytest`\.mark\.smoke", "tests").splitlines() if p.strip()]
gating_files = [p for p in run("rg", "-l", r"`@pytest`\.mark\.gating", "tests").splitlines() if p.strip()]
affected_tests = []
paths = []
# Direct affected tests from changed test file
if "test_non_migratable_vm_nad_change_not_applied" in test_file:
affected_tests.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied")
paths.append("test_non_migratable_vm_nad_change_not_applied → non_migratable_baseline_connectivity fixture → verify_baseline_connectivity() / non_migratable_under_test_vm fixture → non_migratable_bridge_vm() → libs.vm.vm.disk_and_volume_for_data_volume()")
# Same file changed tests using shared baseline helper
if "verify_baseline_connectivity" in conftest:
affected_tests.append("tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_migratable_vm_nad_change_applied")
paths.append("test_migratable_vm_nad_change_applied → baseline_connectivity fixture → verify_baseline_connectivity()")
# patch_nad_references is only used in this changed test file
if "patch_nad_references" in test_file:
paths.append("test_non_migratable_vm_nad_change_not_applied → libs.net.vmspec.patch_nad_references()")
# Determine smoke impact: require verified path to smoke file or smoke conftest hierarchy usage
smoke_true = False
smoke_reason = ""
symbols = ["patch_nad_references", "verify_baseline_connectivity", "non_migratable_bridge_vm"]
for smoke in smoke_files:
p = Path(smoke).parent
while True:
cf = p / "conftest.py"
if cf.exists():
txt = cf.read_text()
for s in symbols:
if re.search(rf"\b{s}\b", txt):
smoke_true = True
smoke_reason = f"{smoke} → {cf} → {s}"
break
if smoke_true or str(p) == ".":
break
p = p.parent
if smoke_true:
break
# Determine gating impact: any gating-marked file in affected set
affected_set = set()
for pat in [r"\bverify_baseline_connectivity\b", r"\bnon_migratable_bridge_vm\b", r"\bpatch_nad_references\b"]:
out = run("rg", "-l", pat, "tests")
affected_set.update([p for p in out.splitlines() if p.strip()])
gating_hits = sorted(set(gating_files) & affected_set)
gating_true = bool(gating_hits)
gating_reason = ""
if gating_true:
# build specific path for first gating hit if available
g = gating_hits[0]
if g == "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py":
gating_reason = "tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied (gating) → non_migratable_baseline_connectivity / non_migratable_under_test_vm → verify_baseline_connectivity() / non_migratable_bridge_vm() / patch_nad_references()"
else:
gating_reason = g
comment = []
comment.append("**Test Execution Plan**")
comment.append("")
comment.append(f"- **Run smoke tests:** {'True' if smoke_true else 'False'}" + (f" — {smoke_reason}" if smoke_true else " — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols."))
comment.append(f"- **Run gating tests:** {'True' if gating_true else 'False'}" + (f" — {gating_reason}" if gating_true else ""))
comment.append("")
comment.append("**Affected tests to run**")
for t in affected_tests:
marker = " (gating)" if "non_migratable" in t else ""
comment.append(f"- `{t}`{marker}")
comment.append("")
comment.append("**Real tests (cluster required)**")
comment.append("Error path (the fix):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`")
comment.append("Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. When KubeVirt wires `RestartRequired`, re-enable the wait and expect that condition before connectivity checks.")
comment.append("")
comment.append("Happy path (regression):")
comment.append("`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_migratable_vm_nad_change_applied`")
comment.append("Expected: the normal migratable NAD update path still applies live and connectivity switches as expected.")
print("\n".join(comment))
PY
cat /tmp/test_plan_comment.txt
echo
echo "=== Post inline review comment ==="
# Try submit as a single COMMENT review with one inline comment.
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
-f body='' \
-f event='COMMENT' \
-f commit_id="$head_sha" \
-f "comments[0][path]=$first_file" \
-F "comments[0][line]=$first_line" \
-f "comments[0][side]=RIGHT" \
-F "comments[0][body]=`@openshift-virtualization-qe-bot-3` $(cat /tmp/test_plan_comment.txt)" \
>/tmp/review_response.json || {
echo "Inline review creation failed, retrying with issue comment fallback" >&2
gh pr comment "$pr" --repo "$repo" --body "`@openshift-virtualization-qe-bot-3` $(cat /tmp/test_plan_comment.txt)"
}
echo "DONE"Length of output: 85114 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
head_sha=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')
first_file="libs/net/vmspec.py"
first_line="309"
echo "=== Inspect direct callers and markers for localnet NAD ref change ==="
sed -n '1,260p' tests/network/localnet/nad_ref_change/test_nad_ref_change.py
echo
echo "--- smoke marker in affected files"
rg -n '`@pytest`\.mark\.smoke|`@pytest`\.mark\.gating' \
tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py \
tests/network/localnet/nad_ref_change/test_nad_ref_change.py || true
cat > /tmp/test_plan_comment.txt <<'EOF'
**Test Execution Plan**
- **Run smoke tests:** False — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols in `libs/net/vmspec.py`, `libs/vm/spec.py`, `libs/vm/vm.py`, or the changed NAD-ref-change helpers/fixtures.
- **Run gating tests:** False — no gating-marked test file appears in the affected set traced from `patch_nad_references()`, `update_nad_references()`, `verify_baseline_connectivity()`, `non_migratable_bridge_vm()`, and `disk_and_volume_for_data_volume()`.
**Affected tests to run**
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_vm_state_iface_info_preserved`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_connectivity`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_two_networks`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Dependency paths:
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied` → `non_migratable_baseline_connectivity` / `non_migratable_under_test_vm` fixtures in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` / `non_migratable_bridge_vm()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py` → `disk_and_volume_for_data_volume()` in `libs/vm/vm.py`, plus direct call to `patch_nad_references()` in `libs/net/vmspec.py`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::*` → `baseline_connectivity` fixture in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py`; the tests also call `update_nad_references()` in `libs/net/vmspec.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` → direct call(s) to `update_nad_references()` in `libs/net/vmspec.py`, which now delegate to `patch_nad_references()`
**Real tests (cluster required)**
Error path (the fix):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, the condition wait should also pass before the connectivity assertions.
Happy path (regression):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`
Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.
Additional regression for the refactored shared helper:
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.
EOF
python - <<'PY'
import json, pathlib
body = pathlib.Path("/tmp/test_plan_comment.txt").read_text()
payload = {
"body": "",
"event": "COMMENT",
"commit_id": "440085587c13fb65d210f9684f3d781d8e5841fd",
"comments": [
{
"path": "libs/net/vmspec.py",
"line": 309,
"side": "RIGHT",
"body": body,
}
],
}
pathlib.Path("/tmp/review_payload.json").write_text(json.dumps(payload))
print(body)
PY
echo
echo "=== Post inline review comment via raw JSON ==="
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/pulls/$pr/reviews" \
--input /tmp/review_payload.jsonLength of output: 12236 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="RedHatQE/openshift-virtualization-tests"
pr="5212"
cat > /tmp/test_plan_comment.txt <<'EOF'
**Test Execution Plan**
- **Run smoke tests:** False — no verified dependency path from any smoke-marked test or parent `conftest.py` to the modified symbols in `libs/net/vmspec.py`, `libs/vm/spec.py`, `libs/vm/vm.py`, or the changed NAD-ref-change helpers/fixtures.
- **Run gating tests:** False — no gating-marked test file appears in the affected set traced from `patch_nad_references()`, `update_nad_references()`, `verify_baseline_connectivity()`, `non_migratable_bridge_vm()`, and `disk_and_volume_for_data_volume()`.
**Affected tests to run**
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_vm_state_iface_info_preserved`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_connectivity`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::test_two_networks`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Dependency paths:
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied` → `non_migratable_baseline_connectivity` / `non_migratable_under_test_vm` fixtures in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` / `non_migratable_bridge_vm()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py` → `disk_and_volume_for_data_volume()` in `libs/vm/vm.py`, plus direct call to `patch_nad_references()` in `libs/net/vmspec.py`
- `tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange::*` → `baseline_connectivity` fixture in `tests/network/l2_bridge/nad_ref_change/conftest.py` → `verify_baseline_connectivity()` in `tests/network/l2_bridge/nad_ref_change/lib_helpers.py`; the tests also call `update_nad_references()` in `libs/net/vmspec.py`
- `tests/network/localnet/nad_ref_change/test_nad_ref_change.py` → direct call(s) to `update_nad_references()` in `libs/net/vmspec.py`, which now delegate to `patch_nad_references()`
**Real tests (cluster required)**
Error path (the fix):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::test_non_migratable_vm_nad_change_not_applied`
Expected: the VM NAD patch does not apply live; connectivity stays on VLAN-A and not VLAN-B. Once KubeVirt wires `RestartRequired`, the condition wait should also pass before the connectivity assertions.
Happy path (regression):
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/l2_bridge/nad_ref_change/test_nad_ref_change.py::TestRunningVMLinuxBridgeVlanChange`
Expected: the normal migratable NAD update path still applies live and connectivity follows the updated NAD references.
Additional regression for the refactored shared helper:
`uv run pytest --verbose -rs -s -o log_cli=true --leftovers-collector --tb=native --skip-deprecated-api-test --cluster-sanity-skip-storage-check tests/network/localnet/nad_ref_change/test_nad_ref_change.py`
Expected: the localnet NAD reference update path still applies live after `update_nad_references()` was refactored to call `patch_nad_references()`.
EOF
python - <<'PY'
import json, pathlib
payload = {"body": pathlib.Path("/tmp/test_plan_comment.txt").read_text()}
pathlib.Path("/tmp/issue_comment_payload.json").write_text(json.dumps(payload))
PY
gh api \
--method POST \
-H "Accept: application/vnd.github+json" \
"/repos/$repo/issues/$pr/comments" \
--input /tmp/issue_comment_payload.jsonLength of output: 4498
CRITICAL: I could not post the required inline review comment because the GitHub integration for this run does not have write access to the PR. All three write paths failed with
That rule exists to ensure the plan is attached to the actual diff, but here the permission boundary is the blocker, not the analysis. Here is the exact test execution plan that should be posted on Test Execution Plan
Affected tests to run
Dependency paths:
Real tests (cluster required) Happy path (regression): Additional regression for the refactored shared helper: |
|
@coderabbitai I ran the new test without the condition waiting to make sure it'll pass once we unblock it |
|
Tip For best results, initiate chat on the files or code changes.
The linked verification comment covers the regression path I was concerned about as well. 🐇 |
10 participants
What this PR does / why we need it:
Adds
test_non_migratable_vm_nad_change_not_applied: verifies that updating the NAD reference on a non-migratable VM does not apply live and the VM reports aRestartRequiredcondition and retains connectivity to the original VLAN-A.To support this, the PR also:
containerdisk
Which issue(s) this PR fixes: -
Special notes for reviewer:
The test is quarantined via @pytest.mark.jira("CNV-87878", run=False): the
RestartRequiredcondition is not yet wired up in KubeVirt.jira-ticket: https://redhat.atlassian.net/browse/CNV-89745
Summary by CodeRabbit
Summary
New Features
Bug Fixes
Tests