-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Quality Gate passedIssues Measures |
87e02e6
to
405fc92
Compare
45e6697
to
7fa1768
Compare
There was a problem hiding this 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.
8c38ec3
to
b36899c
Compare
There was a problem hiding this 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: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
pkg/controller/plan/kubevirt.go
Outdated
environment = append(environment, | ||
core.EnvVar{ | ||
Name: "V2V_NewName", | ||
Value: vm.NewName, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
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]>
Signed-off-by: Bella Khizgiyaev <[email protected]>
b36899c
to
afff014
Compare
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]>
afff014
to
ed26a11
Compare
Quality Gate passedIssues Measures |
The xml to yaml converion in entrypoint file, looks good to me 👍 |
Continuing the PR in the #1020 |
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.