Skip to content
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

Change created virt-v2v conversion output to -o kubeVirt #778

Closed
wants to merge 11 commits into from

Conversation

bkhizgiy
Copy link
Member

@bkhizgiy bkhizgiy commented Mar 5, 2024

Currently, the output produced by virt-v2v is in libvirt XML format. However, since we are working with KubeVirt VMs and in light of recent changes by the virt-v2v team to add missing fields to the YAML output, we are switching the output format to -o kubevirt. This change will allow us to use this information to create the VM in OpenShift and capture some missing fields about the VMs. As part of this update, if a VM name is not valid, we will generate a new name and pass it to the conversion pod, where it will be updated as part of the virt-v2v command, as well as within the migration controller.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 5.26316% with 72 lines in your changes missing coverage. Please review.

Project coverage is 16.30%. Comparing base (786259f) to head (b36899c).

Files Patch % Lines
pkg/controller/plan/util/kubevirtvmparser.go 0.00% 32 Missing ⚠️
virt-v2v/cold/entrypoint.go 15.38% 19 Missing and 3 partials ⚠️
pkg/controller/plan/kubevirt.go 0.00% 13 Missing ⚠️
pkg/controller/plan/migration.go 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #778   +/-   ##
=======================================
  Coverage   16.30%   16.30%           
=======================================
  Files         103      103           
  Lines       19431    19443   +12     
=======================================
+ Hits         3168     3171    +3     
- Misses      15982    15988    +6     
- Partials      281      284    +3     
Flag Coverage Δ
unittests 16.30% <5.26%> (+<0.01%) ⬆️

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.

virt-v2v/cold/entrypoint.go Outdated Show resolved Hide resolved
pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved
@ahadas ahadas added this to the 2.6.0 milestone Mar 7, 2024
virt-v2v/cold/entrypoint.go Outdated Show resolved Hide resolved
@bkhizgiy bkhizgiy changed the title OVA: workaround for virt-v2v firmware detection wip: OVA: workaround for virt-v2v firmware detection Mar 10, 2024
@bkhizgiy bkhizgiy changed the title wip: OVA: workaround for virt-v2v firmware detection WIP: OVA: workaround for virt-v2v firmware detection Mar 10, 2024
pkg/controller/plan/adapter/ova/kubevirtvmparser.go Outdated Show resolved Hide resolved
pkg/controller/plan/kubevirt.go Outdated Show resolved Hide resolved
pkg/controller/plan/adapter/ova/kubevirtvmparser.go Outdated Show resolved Hide resolved
virt-v2v/cold/entrypoint.go Outdated Show resolved Hide resolved
virt-v2v/cold/entrypoint.go Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Mar 12, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
4.7% Duplication on New Code

See analysis details on SonarCloud

@ahadas ahadas removed this from the 2.6.0 milestone Mar 14, 2024
@ahadas ahadas linked an issue Apr 9, 2024 that may be closed by this pull request
@bkhizgiy bkhizgiy force-pushed the workarround branch 2 times, most recently from 87e02e6 to 405fc92 Compare July 23, 2024 11:54
@bkhizgiy bkhizgiy changed the title WIP: OVA: workaround for virt-v2v firmware detection Change created virt-v2v conversion output to -o kubeVirt Jul 24, 2024
@bkhizgiy bkhizgiy force-pushed the workarround branch 3 times, most recently from 45e6697 to 7fa1768 Compare July 31, 2024 09:07
Copy link
Member

@liranr23 liranr23 left a comment

Choose a reason for hiding this comment

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

Please add/fix the commits messages, I can't tell why the newName is in use.

pkg/controller/plan/kubevirt.go Show resolved Hide resolved
pkg/controller/plan/util/kubevirtvmparser.go Outdated Show resolved Hide resolved
virt-v2v/cold/entrypoint.go Show resolved Hide resolved
virt-v2v/cold/entrypoint.go Show resolved Hide resolved
Copy link
Member

@ahadas ahadas left a comment

Choose a reason for hiding this comment

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

looks good to me in general

err = liberr.Wrap(err)
return
}
case api.VSphere:
Copy link
Member

Choose a reason for hiding this comment

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

so wouldn't it be missing for vSphere?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, this function is called during the conversion phase in the migration process, so both vSphere and OVA providers enter here. Instead of applying it only to vSphere, it will apply to OVA as well.

environment = append(environment,
core.EnvVar{
Name: "V2V_NewName",
Value: vm.NewName,
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't we set it only when a new name is set? you check if V2V_NewName is set in the entrypoint of the conversion pod

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using GetEnv in the entrypoint function, which returns false both for unset and empty variables, so it's working correctly. However, I'll add a check here as well.

As part of switching the virt-v2v conversion output to -o kubevirt,
we need to ensure that the converted VM name meets DNS1123 requirements
otherwise, the conversion will fail.
This change involves passing the newly generated VM name to the
conversion pod as an environment variable and updating it as part of the
virt-v2v command execution

Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
@bkhizgiy
Copy link
Member Author

bkhizgiy commented Sep 2, 2024

@mnecas @yaacov There was a conflicts with some of the recent changes. Can you please verify that nothing is missing with the switch to the YAML file? Thanks!

As part of the virt-v2v migration, we inspect the OS type and disk paths.
Until now, this has been done with the XML output file. Due to the change
to YAML format, this process needs to be adapted as well.
Since we cannot use external libraries in the virt-v2v directory,
it was implemented using bufio.

Signed-off-by: Bella Khizgiyaev <[email protected]>
Copy link

sonarcloud bot commented Sep 2, 2024

@yaacov
Copy link
Member

yaacov commented Sep 3, 2024

Can you please verify that nothing is missing with the switch to the YAML file? Thanks!

The xml to yaml converion in entrypoint file, looks good to me 👍

@mnecas
Copy link
Member

mnecas commented Sep 18, 2024

Continuing the PR in the #1020
I have rebased it with the latest virt-v2v wrapper changes and made some additional changes
Closing

@mnecas mnecas closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OVA: Consume firmware type from virt-v2v kubvirt configuration
6 participants