Skip to content

MTV-2707 | xcopy-popualtor: Pass the vmId to the populator #2002

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 26, 2025

Conversation

rgolangh
Copy link
Contributor

Motivation
Up until now the populator was deducing the name of the VM from the path
of the vmdk file. That approach breaks when 1. a VM is renamed 2. the
disk is a shared disk 3. there are more vm's with same name on another
DC. Probably more cases exists.

Modification
Forkflift will add the vmId to the VsphereXcopyVolumePopulator resource,
which now have a new field, VmId. From there on that is passed all the
way to the command line argument --source-vm-id and there its used to
find the ESX by VM ID.

Result
The migration runs exactly the same, there are no visible changes, or UX
changes, this is all transparent to the user.
Renamed or imported VMs or any of the cases mentioned above will migrate
without any problems.

https://issues.redhat.com/browse/MTV-2707

Signed-off-by: Roy Golan rgolan@redhat.com

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 9.31%. Comparing base (f1fe5d0) to head (e73e238).
Report is 432 commits behind head on main.

Files with missing lines Patch % Lines
cmd/populator-controller/populator-controller.go 0.00% 1 Missing ⚠️
pkg/controller/plan/adapter/vsphere/builder.go 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##             main   #2002      +/-   ##
=========================================
- Coverage   15.45%   9.31%   -6.15%     
=========================================
  Files         112     360     +248     
  Lines       23377   48652   +25275     
=========================================
+ Hits         3613    4530     +917     
- Misses      19479   43757   +24278     
- Partials      285     365      +80     
Flag Coverage Δ
unittests 9.31% <0.00%> (-6.15%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -72,7 +72,7 @@ func (c *VSphereClient) RunEsxCommand(ctx context.Context, host *object.HostSyst
return res.Values, nil
}

func (c *VSphereClient) GetEsxByVm(ctx context.Context, vmName string) (*object.HostSystem, error) {
func (c *VSphereClient) GetEsxByVm(ctx context.Context, vmId string) (*object.HostSystem, error) {
finder := find.NewFinder(c.Client.Client, true)

//FIXME - need to trace the VM by the datastore, which we should have because
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix that already - maybe we donh't need to set a datacenter if we get the vm by id. anyhow, get rid of the datacednter part because there may be a case where there is no datacenger

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold need to change a small thing with the default DC (remove it)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold cancel
This is ready and working

Motivation
Up until now the populator was deducing the name of the VM from the path
of the vmdk file. That approach breaks when 1. a VM is renamed 2. the
disk is a shared disk 3. there are more vm's with same name on another
DC. Probably more cases exists.

Modification
Forkflift will add the vmId to the VsphereXcopyVolumePopulator resource,
which now have a new field, VmId. From there on that is passed all the
way to the command line argument --source-vm-id and there its used to
find the ESX by VM ID.

Result
The migration runs exactly the same, there are no visible changes, or UX
changes, this is all transparent to the user.
Renamed or imported VMs or any of the cases mentioned above will migrate
without any problems.

https://issues.redhat.com/browse/MTV-2707

Signed-off-by: Roy Golan <rgolan@redhat.com>
Copy link

@mnecas mnecas merged commit d5b6ce2 into kubev2v:main Jun 26, 2025
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants