MTV-5674 | Add per-network guestIPConfig to NetworkMap#6916
Open
Hazanel wants to merge 1 commit into
Open
Conversation
Contributor
Author
|
Hey @coderabbitai, please take a look |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details
gh pr view 6916 --repo kubev2v/forklift --json title,body,files,additions,deletions,commits
echo "=== PR Files Changed ==="
gh pr view 6916 --repo kubev2v/forklift --json files --jq '.files[].path'Length of output: 4061 🏁 Script executed: #!/bin/bash
# Get the PR diff
gh pr diff 6916 --repo kubev2v/forkliftLength of output: 49452
Review: MTV-5674 | Add per-network guestIPConfig to NetworkMapThe overall design is clean and well-tested. A few things are worth addressing: 🔴 High:
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6916 +/- ##
==========================================
- Coverage 15.45% 10.75% -4.70%
==========================================
Files 112 521 +409
Lines 23377 62120 +38743
==========================================
+ Hits 3613 6683 +3070
- Misses 19479 54868 +35389
- Partials 285 569 +284
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:
|
📝 WalkthroughWalkthroughThis PR extends the NetworkMap CRD to support per-network guest IP configuration, allowing fine-grained control over static IP preservation for individual mapped networks. The implementation adds guestIPConfig enum field (preserve/dhcp/none) to network destinations, introduces mode-resolution logic, and integrates the feature into Hyper-V and vSphere migration builders. ChangesGuest IP Configuration Support for Network Mappings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/controller/plan/adapter/vsphere/builder.go (1)
327-365: ⚡ Quick winRefactor
mapMacStaticIpsto reduce complexity in this hot path.Line 327 is now above the configured complexity threshold (25 vs 15). Extract NIC eligibility and gateway resolution into small helpers to keep this path maintainable and avoid recurring quality-gate noise.
Refactor sketch
func (r *Builder) mapMacStaticIps(vm *model.VM, modeByMAC map[string]string) (ipMap string, err error) { - // existing complex loop... + isWindowsFlag := isWindows(vm) + var configurations []string + for _, guestNetwork := range vm.GuestNetworks { + if !shouldIncludeNICForPreserve(guestNetwork.MAC, modeByMAC) { + continue + } + if isWindowsFlag && guestNetwork.Origin != string(types.NetIpConfigInfoIpAddressOriginManual) { + continue + } + gateway := findMatchingGateway(vm.GuestIpStacks, guestNetwork) + configurations = append(configurations, formatStaticIPConfig(guestNetwork, gateway)) + } + return strings.Join(configurations, "_"), nil }As per coding guidelines "Clarity and Simplicity: Write idiomatic Go. Code should be self-documenting. Complexity should be justified."
🤖 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 `@pkg/controller/plan/adapter/vsphere/builder.go` around lines 327 - 365, The mapMacStaticIps function is too complex; extract the NIC eligibility checks and gateway resolution into small helpers to simplify the hot path. Create a helper like shouldIncludeNIC(guestNetwork *model.GuestNetwork, modeByMAC map[string]string, isWindows bool) that encapsulates the preserve-mode check and the Windows-origin logic, and another helper resolveGateway(vm *model.VM, guestNetwork *model.GuestNetwork) string that iterates vm.GuestIpStacks and returns the matching gateway (or ""), then simplify mapMacStaticIps to call these helpers, build the dns/configuration string as before, and append trimmed configurations; keep function signature of mapMacStaticIps unchanged.pkg/controller/plan/adapter/hyperv/builder_test.go (1)
102-270: ⚡ Quick winAdd direct tests for
hasMultipleStaticIPsPerNICmode filtering.This suite validates
ResolveNICModes/mapMacStaticIps, but the changed counting path inhasMultipleStaticIPsPerNICis still untested. Please add a focused case proving that Windows multi-IP detection ignores NICs resolved asdhcp/none.Proposed test addition
+ Context("hasMultipleStaticIPsPerNIC with guestIPConfig filtering", func() { + It("ignores multiple IPs on non-preserve NICs", func() { + vm := &model.VM{ + GuestOS: "Windows Server 2019", + GuestNetworks: []hyperv.GuestNetwork{ + {MAC: "00:15:5D:01:02:03", IP: "172.29.3.193", Origin: hyperv.OriginManual}, + {MAC: "00:15:5D:01:02:03", IP: "172.29.3.194", Origin: hyperv.OriginManual}, + }, + } + + Expect(hasMultipleStaticIPsPerNIC(vm, map[string]string{ + "00:15:5D:01:02:03": "dhcp", + })).To(BeFalse()) + + Expect(hasMultipleStaticIPsPerNIC(vm, map[string]string{ + "00:15:5D:01:02:03": "preserve", + })).To(BeTrue()) + }) + })As per coding guidelines: "Coverage: Strive for high test coverage on critical logic paths."
🤖 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 `@pkg/controller/plan/adapter/hyperv/builder_test.go` around lines 102 - 270, Add a unit test that directly exercises hasMultipleStaticIPsPerNIC to cover the branch where NICs resolved as "dhcp" or "none" are ignored: create a VM with multiple NICs and guest/static IP setups, call planbase.ResolveNICModes (or mapMacStaticIps) to produce modes and the mac->static IP mapping, then assert hasMultipleStaticIPsPerNIC returns false when only DHCP/none NICs would otherwise contribute and true when a NIC resolved as "preserve" has multiple static IPs; refer to hasMultipleStaticIPsPerNIC, ResolveNICModes, mapMacStaticIps and nicRefsFromVM to locate the logic to test.pkg/controller/map/network/network_test.go (1)
257-407: 🏗️ Heavy liftThese tests re-implement validation logic instead of testing it.
The new
GuestIPConfig validationspecs manually loop throughnm.Spec.Mapand manually callSetCondition, so they can pass even ifvalidateDestination()wiring regresses. Please test the production path (or extract a shared helper used by both validation and tests) to avoid self-fulfilling tests.As per coding guidelines: "Coverage: Strive for high test coverage on critical logic paths."
🤖 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 `@pkg/controller/map/network/network_test.go` around lines 257 - 407, Tests are re-implementing validation logic instead of exercising the production validator; replace the manual loop and direct SetCondition calls with a call to the real validation path (e.g. invoke validateDestination/ValidateDestination or the exported validation helper used by the controller against the NetworkMap instance) so the test uses the same code under test, then assert on nm.Status.HasCondition(GuestIPConfigNotValid) and HasBlockerCondition as before; if no exported validator exists, extract the guestIPConfig-checking logic into a shared helper (used by validateDestination and tests) and call that helper here instead.
🤖 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 `@pkg/controller/plan/adapter/base/network.go`:
- Around line 35-43: When resolving mode := string(pair.GuestIPConfig) for each
nic, do not let plan-level preserveStaticIPs force "preserve" for destinations
with type "ignored": if pair.GuestIPConfig is empty and the destination type is
"ignored" set mode to api.GuestIPConfigNone explicitly (instead of using
preserveStaticIPs -> api.GuestIPConfigPreserve). Locate the branch that sets
mode when pair.GuestIPConfig == "" and add a check for destination.type ==
"ignored" (or the corresponding enum/constant) to assign api.GuestIPConfigNone
before considering preserveStaticIPs; keep using nic.MAC and modes[nic.MAC] =
mode unchanged.
---
Nitpick comments:
In `@pkg/controller/map/network/network_test.go`:
- Around line 257-407: Tests are re-implementing validation logic instead of
exercising the production validator; replace the manual loop and direct
SetCondition calls with a call to the real validation path (e.g. invoke
validateDestination/ValidateDestination or the exported validation helper used
by the controller against the NetworkMap instance) so the test uses the same
code under test, then assert on nm.Status.HasCondition(GuestIPConfigNotValid)
and HasBlockerCondition as before; if no exported validator exists, extract the
guestIPConfig-checking logic into a shared helper (used by validateDestination
and tests) and call that helper here instead.
In `@pkg/controller/plan/adapter/hyperv/builder_test.go`:
- Around line 102-270: Add a unit test that directly exercises
hasMultipleStaticIPsPerNIC to cover the branch where NICs resolved as "dhcp" or
"none" are ignored: create a VM with multiple NICs and guest/static IP setups,
call planbase.ResolveNICModes (or mapMacStaticIps) to produce modes and the
mac->static IP mapping, then assert hasMultipleStaticIPsPerNIC returns false
when only DHCP/none NICs would otherwise contribute and true when a NIC resolved
as "preserve" has multiple static IPs; refer to hasMultipleStaticIPsPerNIC,
ResolveNICModes, mapMacStaticIps and nicRefsFromVM to locate the logic to test.
In `@pkg/controller/plan/adapter/vsphere/builder.go`:
- Around line 327-365: The mapMacStaticIps function is too complex; extract the
NIC eligibility checks and gateway resolution into small helpers to simplify the
hot path. Create a helper like shouldIncludeNIC(guestNetwork
*model.GuestNetwork, modeByMAC map[string]string, isWindows bool) that
encapsulates the preserve-mode check and the Windows-origin logic, and another
helper resolveGateway(vm *model.VM, guestNetwork *model.GuestNetwork) string
that iterates vm.GuestIpStacks and returns the matching gateway (or ""), then
simplify mapMacStaticIps to call these helpers, build the dns/configuration
string as before, and append trimmed configurations; keep function signature of
mapMacStaticIps unchanged.
🪄 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: CHILL
Plan: Pro
Run ID: 1c02b41a-ee33-4700-80f1-8d2c76dfca17
📒 Files selected for processing (13)
operator/.downstream_manifestsoperator/.upstream_manifestsoperator/config/crd/bases/forklift.konveyor.io_networkmaps.yamlpkg/apis/forklift/v1beta1/mapping.gopkg/controller/map/network/network_test.gopkg/controller/map/network/validation.gopkg/controller/plan/adapter/base/network.gopkg/controller/plan/adapter/base/network_test.gopkg/controller/plan/adapter/hyperv/builder.gopkg/controller/plan/adapter/hyperv/builder_test.gopkg/controller/plan/adapter/hyperv/hyperv_suite_test.gopkg/controller/plan/adapter/vsphere/builder.gopkg/controller/plan/adapter/vsphere/builder_test.go
Contributor
Author
|
thanks @coderabbitai, what about the nadpool logic, wdyt? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat pkg/controller/plan/adapter/base/network.go
echo "=== TESTS ==="
cat pkg/controller/plan/adapter/base/network_test.goLength of output: 24801
✅ What works wellPool-per-VM isolation: Cross-network NAD deduplication: The pool is shared across all source networks within one VM invocation. So if Deterministic ordering: Pool key safety: The key is
🟡 One subtle edge case: pool-exhausted NIC gets "preserve" treatmentWhen there are more NICs than NAD rows for a source network (pool exhausted), the NIC is skipped — it doesn't appear in if mode, found := modeByMAC[mac]; found && mode != string(api.GuestIPConfigPreserve) {
continue // skip this NIC
}
// fallthrough → include this NIC in static IP configSo a pool-exhausted NIC falls into the In practice this is rare (it requires more NICs than NAD rows, which 🟢 Minor:
|
Hazanel
force-pushed
the
mtv-5268-guest-ip-config
branch
from
12eea74 to
9d133eb
Compare
Contributor
Author
|
@coderabbitai, please take a look |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Recent commits ==="
gh pr view 6916 --repo kubev2v/forklift --json commits --jq '.commits[-5:] | .[] | {oid: .oid[0:8], message: .messageHeadline, date: .committedDate}'
echo ""
echo "=== PR status ==="
gh pr view 6916 --repo kubev2v/forklift --json state,reviews,reviewDecision,mergeable,statusCheckRollup --jq '{state, reviewDecision, mergeable, checks: [.statusCheckRollup[]? | {name:.name, state:.state}]}'
echo ""
echo "=== Files changed ==="
gh pr view 6916 --repo kubev2v/forklift --json files --jq '.files[] | {path: .path, additions, deletions}'Length of output: 3143
Open items from previous reviews
CI is still pending so I can't report on that yet. Let me know if you want me to dive deeper into any of the items above, or if any of them have been addressed outside the current commit! 🧠 Learnings used |
Contributor
Author
|
@coderabbitai, the PR has changed , changes were amended to the PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Recent commits ==="
gh pr view 6916 --repo kubev2v/forklift --json commits --jq '.commits[-8:] | .[] | {oid: .oid[0:8], message: .messageHeadline, date: .committedDate}'
echo ""
echo "=== Files changed ==="
gh pr view 6916 --repo kubev2v/forklift --json files --jq '.files[] | {path: .path, additions, deletions}'
echo ""
echo "=== PR diff ==="
gh pr diff 6916 --repo kubev2v/forkliftLength of output: 41179
✅ All previous concerns addressed
🟡 One lingering minor noteThe 🟢 Notable improvementThe rename from Overall the PR is in great shape! 🐇 🧠 Learnings used |
Introduce a guestIPConfig field (preserve/dhcp/none) on NetworkPair entries in the NetworkMap CRD, allowing per-NIC control over whether static IPs are preserved during migration. This overrides the plan-level preserveStaticIPs flag on a per-network basis. Changes: - Add GuestIPConfig type and field to NetworkPair in mapping.go - Add validation rejecting guestIPConfig on ignored destinations - Add ResolveNICModes() and HasPreserveMode() helpers in base package - Modify vSphere and HyperV builders to filter mapMacStaticIps by mode Ref: https://redhat.atlassian.net/browse/MTV-5674 Resolves: MTV-5674 Signed-off-by: Elad Hazan <ehazan@redhat.com>
Hazanel
force-pushed
the
mtv-5268-guest-ip-config
branch
from
9d133eb to
bbea2f2
Compare
Contributor
Author
|
Thanks @coderabbitai, I just addressed your final minor note |
|
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Contributor
Author
|
Test plan and conversion pod's log are attached to the ticket |
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
2 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.
Introduce a guestIPConfig field (preserve/dhcp/none) on NetworkPair entries in the NetworkMap CRD, allowing per-NIC control over whether static IPs are preserved during migration. This overrides the plan-level preserveStaticIPs flag on a per-network basis.
Changes:
Ref: https://redhat.atlassian.net/browse/MTV-5674
Resolves: MTV-5674