andrijapanicsb commented on code in PR #14256:
URL: https://github.com/apache/cloudstack/pull/14256#discussion_r4186232806
##########
server/src/main/java/org/apache/cloudstack/vm/VmwareCbtMigrationManagerImpl.java:
##########
@@ -2695,6 +2695,22 @@ private boolean
hasActiveMigrationOnSameConvertHost(VmwareCbtMigrationVO migrati
}
private void sendCleanupCommand(VmwareCbtMigrationVO migration, boolean
failOnCleanupError, int waitSeconds) {
+ // Import may finish after cancellation and record the VM without
changing the Cancelled state.
+ // Its target disks now belong to that VM, so neither cancel nor
delete may remove them.
+ // This does not prevent source snapshot cleanup or deletion of the
migration record.
Review Comment:
Thanks, @DaanHoogland . I’m keeping these comments. They document a
non-obvious cancellation/import interaction and explain why cleanup must
preserve the imported VM’s disks. If anything in that explanation is
technically inaccurate, please point it out; the deletion suggestions don’t
identify an error.
Given that the earlier PR was reverted over concerns about independent
testing, it is rather surprising to see review time spent removing explanations
instead of helping close that testing gap. With prebuilt packages available, an
independent CBT migration test would be considerably more useful than making
the source four comment lines shorter.
If you have time to contribute further, could you help verify that workflow
and report any functional issues? That would address the concern that actually
held this work back.
##########
server/src/main/java/org/apache/cloudstack/vm/VmwareCbtMigrationManagerImpl.java:
##########
@@ -2695,6 +2695,22 @@ private boolean
hasActiveMigrationOnSameConvertHost(VmwareCbtMigrationVO migrati
}
private void sendCleanupCommand(VmwareCbtMigrationVO migration, boolean
failOnCleanupError, int waitSeconds) {
+ // Import may finish after cancellation and record the VM without
changing the Cancelled state.
+ // Its target disks now belong to that VM, so neither cancel nor
delete may remove them.
+ // This does not prevent source snapshot cleanup or deletion of the
migration record.
+ Long importedVmId = migration.getVmId();
+ if (importedVmId == null) {
+ // Re-read before cleanup: an import may have recorded its VM
since this caller loaded the migration.
Review Comment:
Thanks, @DaanHoogland . I’m keeping these comments. They document a
non-obvious cancellation/import interaction and explain why cleanup must
preserve the imported VM’s disks. If anything in that explanation is
technically inaccurate, please point it out; the deletion suggestions don’t
identify an error.
Given that the earlier PR was reverted over concerns about independent
testing, it is rather surprising to see review time spent removing explanations
instead of helping close that testing gap. With prebuilt packages available, an
independent CBT migration test would be considerably more useful than making
the source four comment lines shorter.
If you have time to contribute further, could you help verify that workflow
and report any functional issues? That would address the concern that actually
held this work back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
