Repository navigation
fix: revert multus_network_name to use class_plan_config for VM lookups - #592
Merged
myakove merged 4 commits intoJul 13, 2026
Merged
Conversation
PR RedHatQE#583 changed the multus_network_name fixture to use prepared_plan (cloned VM names) instead of class_plan_config (original template names) for network lookups. This broke all RHV tests because RHV clones VMs FROM templates — cloned VMs are VMs, not templates. The provider's get_vm_or_template_networks() calls get_template_by_name(), which searches the oVirt templates service and raises NotFoundError for cloned VM names. Reverting to class_plan_config is safe because cloned VMs always inherit the source VM/template networks — no code path in the codebase adds or modifies network assignments after cloning. The original template names are what all providers need for network lookups.
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
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. |
PR Summary by QodoFix RHV regression by using class_plan_config for Multus network lookups
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review by Qodo
Context used✅ Compliance rules (platform):
97 rules 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Copy-offload configs use a placeholder VM name ("xcopy-template-test")
that gets overridden from .providers.json at runtime. Apply the same
override in multus_network_name before network lookup.
Extracts the override logic into apply_copyoffload_vm_name_override()
to eliminate duplication between multus_network_name and prepared_plan.
redhat-qe-bot1
added
size/S
and removed
lgtm-coderabbitai[bot]
commented-qodo-code-review[bot]
size/XS
labels
|
Code review by qodo was updated up to the latest commit 38b4f59 |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/copyoffload_migration.py (1)
58-81: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winType annotation
list[dict[str, str | bool]]is too narrow for heterogeneous VM config dicts.The VM config dicts are heterogeneous structures loaded from plan config — they contain values beyond
strandbool(e.g., nested dicts, lists, integers). The downstream caller inconftest.py:1020types this aslist[dict[str, Any]], creating a mismatch at the call site. UsingAnyhere is justified because the dict is a heterogeneous config object, not a uniform mapping. Per coding guidelines,Anyshould be avoided "unless justified" — this is a justified case.🔧 Proposed fix for type annotation
def apply_copyoffload_vm_name_override( - virtual_machines: list[dict[str, str | bool]], + virtual_machines: list[dict[str, Any]], source_provider: BaseProvider, ) -> None: """Override placeholder VM names with the real VM name from copy-offload config. Copy-offload test configs use placeholder names (e.g., "xcopy-template-test") that must be resolved to the real template name from the provider's copyoffload_config. Args: - virtual_machines (list[dict[str, str | bool]]): List of VM config dicts to update in place. + virtual_machines (list[dict[str, Any]]): List of VM config dicts to update in place. source_provider (BaseProvider): Source provider that may have copyoffload_config. """🤖 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 `@utilities/copyoffload_migration.py` around lines 58 - 81, The type annotation on apply_copyoffload_vm_name_override is too restrictive for the heterogeneous VM config objects passed in from the caller. Update the virtual_machines parameter from a narrow dict value union to a more flexible Any-based mapping so it matches the list[dict[str, Any]] shape used upstream in conftest.py and accurately reflects the mixed config data stored in each VM entry. Keep the rest of the function behavior the same while preserving the in-place name override logic.
🤖 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.
Outside diff comments:
In `@utilities/copyoffload_migration.py`:
- Around line 58-81: The type annotation on apply_copyoffload_vm_name_override
is too restrictive for the heterogeneous VM config objects passed in from the
caller. Update the virtual_machines parameter from a narrow dict value union to
a more flexible Any-based mapping so it matches the list[dict[str, Any]] shape
used upstream in conftest.py and accurately reflects the mixed config data
stored in each VM entry. Keep the rest of the function behavior the same while
preserving the in-place name override logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85166765-4383-4480-a8e6-b8bf9aec4339
📒 Files selected for processing (1)
utilities/copyoffload_migration.py
|
Code review by qodo was updated up to the latest commit 405104e |
Collaborator
Author
|
/verified |
Collaborator
Author
|
/approve |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
Collaborator
Author
|
/cherry-pick v2.11 |
|
Cherry-picked PR fix: revert multus_network_name to use class_plan_config for VM lookups into v2.11: #600 |
redhat-qe-bot1
pushed a commit
that referenced
this pull request
Jul 14, 2026
…ps (#592) (#600) * fix: revert multus_network_name to use class_plan_config for VM lookups PR #583 changed the multus_network_name fixture to use prepared_plan (cloned VM names) instead of class_plan_config (original template names) for network lookups. This broke all RHV tests because RHV clones VMs FROM templates — cloned VMs are VMs, not templates. The provider's get_vm_or_template_networks() calls get_template_by_name(), which searches the oVirt templates service and raises NotFoundError for cloned VM names. Reverting to class_plan_config is safe because cloned VMs always inherit the source VM/template networks — no code path in the codebase adds or modifies network assignments after cloning. The original template names are what all providers need for network lookups. * fix: add copy-offload VM name override to multus_network_name Copy-offload configs use a placeholder VM name ("xcopy-template-test") that gets overridden from .providers.json at runtime. Apply the same override in multus_network_name before network lookup. Extracts the override logic into apply_copyoffload_vm_name_override() to eliminate duplication between multus_network_name and prepared_plan. * fix: use specific type for VM config dict in apply_copyoffload_vm_name_override Co-authored-by: Marián Krčmárik <mkrcmari@redhat.com>
1 of 3 tasks
This was referenced Aug 17, 2026
This was referenced Oct 12, 2026
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
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.
PR #583 changed the multus_network_name fixture to use prepared_plan
(cloned VM names) instead of class_plan_config (original template names)
for network lookups. This broke all RHV tests because RHV clones VMs
FROM templates — cloned VMs are VMs, not templates. The provider's
get_vm_or_template_networks() calls get_template_by_name(), which
searches the oVirt templates service and raises NotFoundError for
cloned VM names.
Reverting to class_plan_config is safe because cloned VMs always
inherit the source VM/template networks — no code path in the
codebase adds or modifies network assignments after cloning.
For copy-offload tests, the config uses a placeholder VM name
("xcopy-template-test") that gets overridden from .providers.json
at runtime. This override is now also applied in multus_network_name
via a shared helper to ensure the network lookup uses the real
VM name.
Also extracts the copy-offload VM name override logic into
apply_copyoffload_vm_name_override() to eliminate duplication
between multus_network_name and prepared_plan fixtures
Summary by CodeRabbit
Summary by CodeRabbit