[albertoflorez]

Summary

In VMware environments, the same network can be represented by multiple port groups (e.g., on different distributed switches or different sites). Previously, the network mapping logic had two limitations that prevented this scenario from working correctly:

1. A NAD could only reference a single source port group via the source-portgroup annotation, so only one port group would match automatically.
2. The network_attachment_definition override was silently ignored when the annotation-based lookup returned no results — the template fell back to type: pod instead of using the explicitly configured NAD.

Changes

• match_portgroup filter plugin (plugins/filter/match_portgroup.py): New custom Ansible filter that replaces the JMESPath exact-match query. It splits the source-portgroup annotation by comma and checks if the network name is in the resulting list. Fully backward compatible with single-value annotations.
• portgroups (list) support (roles/network_mgmt/templates/manual-lb/nad.yaml.j2): The manual NAD template now accepts portgroups (list) alongside the existing portgroup (string). When a list is provided, values are joined with commas in the annotation. portgroups takes precedence if both are defined.
• Fix pod fallback bug (roles/mtv_management/templates/networkmap_map.yml.j2): The template now checks for an explicit network_attachment_definition in overrides before falling back to type: pod. This applies to both vSphere and oVirt paths.
• Updated matching logic (roles/mtv_management/tasks/_mtv_network_map_process_network.yml): Replaced json_query with the new match_portgroup filter for both VMware and oVirt NAD lookups.
• Documentation (roles/network_mgmt/defaults/main.yml, roles/mtv_management/defaults/main.yml): Added usage examples for portgroups list and multi-portgroup override patterns.

Why this matters

Without this change, migrating VMs connected to different port groups that represent the same network required manual workarounds and could silently result in VMs being mapped to the pod network instead of the intended multus NAD — potentially breaking network connectivity after migration.

Testing

• Verified NAD creation with portgroups (list) generates the correct comma-separated source-portgroup annotation
• Verified backward compatibility: existing inventories using portgroup (string) continue to work without changes
• Verified the pod fallback fix: overrides with network_attachment_definition now correctly produce type: multus even when the annotation-based lookup returns no matches
• Tested end-to-end with a real VMware environment using two port groups mapped to the same NAD

Closes #90

[stevefulme1]

Review

Solid PR — real bug fix, well-scoped, backward compatible, good description. Logic is correct for standard cases. A few observations:

Suggestions

1. Unit tests for match_portgroup filter plugin

New filter with branching logic (namespace filtering, comma splitting, exception handling) on a correctness-critical path — wrong match means VM on wrong network. Ansible filter plugins are straightforward to test with pytest. Would be good to cover at minimum: single portgroup match, multi-portgroup match, namespace filtering, no-match, and malformed input.

2. Extract duplicated Jinja2 conditional in networkmap_map.yml.j2

The pod-fallback condition on lines 5 and 25 is ~300 chars of nested boolean logic, duplicated for both vSphere and oVirt paths:

{% elif ('network_destination_type' in ... and ... == "pod") or (mtv_management_mtv_network_networkattachmentdefinition | length == 0 and ('network_attachment_definition' not in ... or ... | default('', true) | trim | length == 0)) %}

Extracting to a set variable (e.g., __should_use_pod) would improve readability and reduce risk of the two copies diverging over time.

3. Comma-in-portgroup-name edge case (match_portgroup.py:57-58)

split(",") means a portgroup name containing a literal comma would be misparsed. Likely rare, but worth a note in the DOCUMENTATION block so users are aware of the delimiter choice.

4. Minor: portgroups vs portgroup precedence

When both portgroup (string) and portgroups (list) are defined in a NAD entry, the list wins silently. Documented in defaults — just flagging that a validation warning or mutual exclusivity check could prevent user confusion.

Summary

No blockers. The core logic and the pod-fallback fix look correct. Main ask is test coverage for the new filter plugin.

[stevefulme1]

just fix the slight thing on my review and we can go ahead and approve.

[albertoflorez]

Thanks for the review — all four points addressed.

1. Unit tests for match_portgroup (d7d06c9)

Added tests/unit/plugins/filter/test_match_portgroup.py — 44 tests covering the cases you asked for: single portgroup match, multi-portgroup match, namespace filtering, no-match, and malformed input. Also covered a few edge cases worth locking down given the correctness risk: case sensitivity, substring non-matching (VLAN1000 must not match VLAN100), whitespace around comma separators, empty entries in the annotation, and that the filter returns the original objects without mutating the input.

The test loads the plugin by path with importlib rather than importing ansible_collections.infra.openshift_virtualization_migration.plugins.filter.match_portgroup, because the repo is not checked out under an ansible_collections/ tree and that import fails when running pytest from the repo root. It works either way, including inside the tree ansible-test builds.

2. Duplicated Jinja2 conditional (0cd53fa)

Went a bit further than extracting __should_use_pod: the vSphere and oVirt branches were identical apart from two variables, so the template now sets __network_overrides and __source_network per provider and shares a single body. The conditions are named (__destination_type, __override_nad, __should_use_pod) with a comment explaining the pod fallback. 42 lines with two copies → 35 with one.

To confirm the refactor is behaviour-preserving I rendered the old and new templates over 126 combinations (3 providers × 14 override shapes × 3 discovered-NAD states): byte-identical output in every case, including the unrecognised-provider case.

3. Comma-in-portgroup-name edge case (88093a9)

Documented in the DOCUMENTATION block and the docstring, including the workaround (map the portgroup explicitly via the network_attachment_definition override). It is also now rejected at validation time rather than only documented — see below.

4. portgroups vs portgroup precedence (c27c8ee)

Rather than documenting the silent precedence, I made it an error. roles/network_mgmt/tasks/manual.yml now asserts that the two keys are mutually exclusive, and separately that portgroups is a list (not a string or mapping) whose entries contain no commas — which closes point 3 on the input side too. Both asserts were exercised against good and bad inputs. roles/network_mgmt/README.md is regenerated by docsible, hence the extra churn in that commit.

---

One thing worth flagging, since the main ask was test coverage: CI does not currently run unit tests. _test-stage.yml in openshift_virtualization_migration_ci runs ansible-lint, tox (linters only, via tox.ini) and tox --ansible (which ends in ansible-test integration). Nothing invokes pytest or ansible-test units, and tests/unit appears only in lint_exclude. These are the first unit tests in the collection, so the gap isn't specific to this PR.

That's a change in the shared CI repo, so I'd rather not bundle it here. Happy to open an issue against openshift_virtualization_migration_ci if you think it's worth wiring up — let me know.

[stevefulme1]

LGTM