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

MTV-1536 | Use CDI for disk transfer in cold migration feature toggle #1171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mnecas
Copy link
Member

@mnecas mnecas commented Nov 5, 2024

Issue:
This is alternative to #1109 which removes the virt-v2v disk transfer and replaces it with te CDI. The main dissadvatage of that solution is that in case customer will have problems with the new CDI method there won't be any easy way how to go back, to the virt-v2v.

Fix:
This change adds a new feature toggle feature_vsphere_cold_cdi in the forklift controller. By default the feature will be enabled and in case there will be problems such as disk corruption the user can disable it.

@mnecas mnecas requested a review from yaacov as a code owner November 5, 2024 07:40
@codecov-commenter
Copy link

codecov-commenter commented Nov 5, 2024

⚠️ 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 6 lines in your changes missing coverage. Please review.

Project coverage is 15.66%. Comparing base (9de3139) to head (53d631c).

Files with missing lines Patch % Lines
pkg/controller/plan/migration.go 0.00% 2 Missing ⚠️
pkg/controller/plan/adapter/vsphere/builder.go 0.00% 1 Missing ⚠️
pkg/controller/plan/adapter/vsphere/client.go 0.00% 1 Missing ⚠️
pkg/controller/plan/kubevirt.go 0.00% 1 Missing ⚠️
pkg/controller/plan/scheduler/vsphere/scheduler.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    #1171      +/-   ##
==========================================
- Coverage   15.71%   15.66%   -0.05%     
==========================================
  Files         112      112              
  Lines       23052    23052              
==========================================
- Hits         3623     3612      -11     
- Misses      19142    19155      +13     
+ Partials      287      285       -2     
Flag Coverage Δ
unittests 15.66% <0.00%> (-0.05%) ⬇️

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.

Copy link
Contributor

@mansam mansam left a comment

Choose a reason for hiding this comment

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

I think that parsing the environment variable should probably be done in the settings/features.go package, as is done for the other feature flags.

@mnecas
Copy link
Member Author

mnecas commented Nov 5, 2024

I think that parsing the environment variable should probably be done in the settings/features.go package, as is done for the other feature flags.

100% thanks!

switch source.Type() {
case VSphere:
return !p.Spec.Warm && destination.IsHost(), nil
return !p.Spec.Warm && destination.IsHost() && !useCdiForColdMigration, nil
Copy link
Member

Choose a reason for hiding this comment

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

LGTM, I have concerns about the naming and the comments
a. the default now is to use CDI, virt v2v transport is the exception, so useCdiForColdMigration checks if use the default settings, maybe we should check for the exception case useVirtV2vTransport , it will be easier to remove once we fill safe about using CDI and remove the virt v2v transport , WDYT ?
b. another concern is about the name VSphereColdLocal it now checks for 'ova' or 'cold + local + "exception for using virt v2v"' so the name + comment may need to change ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks good point, posted latest patch ptal

Issue:
This is alternative to kubev2v#1109
which removes the virt-v2v disk transfer and replaces it with te CDI.
The main dissadvatage of that solution is that in case customer will
have problems with the new CDI method there won't be any easy way how to
go back, to the virt-v2v.

Fix:
This change adds a new feature toggle `feature_vsphere_cold_cdi` in the
forklift controller. By default the feature will be enabled and in case
there will be problems such as disk corruption the user can disable it.

Signed-off-by: Martin Necas <[email protected]>
Copy link

sonarqubecloud bot commented Nov 6, 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.

4 participants