-
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
MTV-1717 | Wait for DV status to not be paused #1243
base: main
Are you sure you want to change the base?
Conversation
Issue: When CDI is managing lot of DVs at once it can take some time before the CDI reconciles the DVs status and start the import. In the function `updateCopyProgress` we check the DV status and if it is paused we do continue and as next phase we have snapshot removal. So if the DV is still in paused state and we start the cutover and MTV skips over the phase thinking it already finished as the DV is in paused. Fix: Add WaitForDataVolumesStatus phase to wait until the DVs do not have the paused status. Ref: https://issues.redhat.com/browse/MTV-1717 https://github.com/kubev2v/forklift/blob/ea83264881b3dcd4a88dd627c1ca75da2483f408/pkg/controller/plan/migration.go#L1573-L1575 Signed-off-by: Martin Necas <mnecas@redhat.com>
Quality Gate passedIssues Measures |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1243 +/- ##
=======================================
Coverage 15.63% 15.63%
=======================================
Files 112 112
Lines 23105 23149 +44
=======================================
+ Hits 3612 3620 +8
- Misses 19208 19241 +33
- Partials 285 288 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -1073,7 +1093,7 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) { | |||
|
|||
switch vm.Phase { | |||
case AddCheckpoint: | |||
vm.Phase = CopyDisks | |||
vm.Phase = WaitForDataVolumesStatus | |||
case AddFinalCheckpoint: | |||
vm.Phase = Finalize |
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.
AddFinalCheckpoint needs to move to the WaitForFinalDataVolumesStatus phase as well.
@@ -1259,6 +1279,15 @@ func (r *Migration) execute(vm *plan.VMStatus) (err error) { | |||
return | |||
} | |||
|
|||
func (r *Migration) hasPausedDv(dvs []ExtendedDataVolume) bool { | |||
for _, dv := range dvs { |
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.
Is there any risk of a race in the opposite direction if the DV precopy finishes and returns to the paused state before the forklift controller leaves this phase?
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.
Questions inline.
Issue: When CDI is managing lot of DVs at once it can take some time before the CDI reconciles the DVs status and start the import. In the function
updateCopyProgress
we check the DV status and if it is paused we do continue and as next phase we have snapshot removal. So if the DV is still in paused state and we start the cutover and MTV skips over the phase thinking it already finished as the DV is in paused.Fix: Add WaitForDataVolumesStatus phase to wait until the DVs do not have the paused status.
Ref: https://issues.redhat.com/browse/MTV-1717
forklift/pkg/controller/plan/migration.go
Lines 1573 to 1575 in ea83264