improve network lookup for vmware source imports #70
Conversation
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
ibrokethecloud
force-pushed
the
tweak-vmware-network-lookup
branch
from
0807758 to
cd068e6
Compare
… vm's to allow importing of VM's using Dswitch backed networks change how network name lookup is perform when importing vmware based vm's to allow importing of VM's using Dswitch backed networks
ibrokethecloud
force-pushed
the
tweak-vmware-network-lookup
branch
from
cd068e6 to
2ff4763
Compare
votdev
reviewed
Mar 10, 2025
| case *types.VirtualEthernetCardNetworkBackingInfo: | ||
| obj := b | ||
| summary = obj.DeviceName | ||
| } |
There was a problem hiding this comment.
The return variable is already set to empty, but to simplify reading the code I would add a default case to the switch statement. In this case it is totally clear what happens.
Suggested change
| } | |
| default: | |
| summary = "" | |
| } |
innobead
previously approved these changes
Mar 10, 2025
|
waiting on feedback from end user who is running a dev build to validate the changes |
ibrokethecloud
dismissed stale reviews from starbops, innobead, and votdev
via
5eadbfd
votdev
reviewed
May 6, 2025
| @@ -92,4 +92,5 @@ const ( | |||
| VirtualMachineImageFailed condition.Cond = "VirtualMachineImageFailed" | |||
| VirtualMachineExportFailed condition.Cond = "VMExportFailed" | |||
| VirtualMachineMigrationFailed ImportStatus = "VMMigrationFailed" | |||
| SkipPreflightChecks string = "harvesterhci.io/skip-preflight-checks" | |||
There was a problem hiding this comment.
Could you explain why you prefer using an annotation over adding a new field to VirtualMachineImportSpec?
votdev
reviewed
May 6, 2025
| @@ -91,6 +93,11 @@ func NewClient(ctx context.Context, endpoint string, dc string, secret *corev1.S | |||
| vmwareClient.ctx = ctx | |||
| vmwareClient.Client = c | |||
| vmwareClient.dc = dc | |||
| networkMap, err := GenerateNetworkMapByRef(ctx, c.Client) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("error generating network map during client initialisation: %v", err) | |||
There was a problem hiding this comment.
Suggested change
| return nil, fmt.Errorf("error generating network map during client initialisation: %v", err) | |
| return nil, fmt.Errorf("error generating network map during client initialisation: %w", err) |
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.
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Problem:
Imports from vmware source where the VM is using a network backed by a Dswitch get mapped to pod network and not the correct target vlan in harvester.
This happens because of the way summary of a nic is represented when using a dswitch backed network.
Solution:
Add additional logic in the vmware import process to identify backing device for a nic and process network names accordingly.
In case of dswitched backed networks we identify the PortGroupKey for nic and use the same to lookup actual vm network name.
Related Issue:
harvester/harvester#7774
Test plan: