Skip to content

Commit fd16736

Browse files
andrijapanicsbandrijapanicsb
committed
Protect imported VM disks from VMware CBT migration cleanup
Skip target cleanup on cancel and delete when an imported VM is recorded, including imports that completed after cancellation. Re-read the migration before cleanup to protect VM references recorded since the caller loaded it. Keep source snapshot cleanup and migration record deletion independent. Add regression coverage for both cleanup entry points, all migration states, stale caller records, and record-only deletion with source cleanup. This guard does not resolve imports racing after the final read. Signed-off-by: andrijapanicsb <andrija.panic@gmail.com>
1 parent 2a3598a commit fd16736

2 files changed

Lines changed: 135 additions & 0 deletions

File tree

‎server/src/main/java/org/apache/cloudstack/vm/VmwareCbtMigrationManagerImpl.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2695,6 +2695,22 @@ private boolean hasActiveMigrationOnSameConvertHost(VmwareCbtMigrationVO migrati
26952695
}
26962696

26972697
private void sendCleanupCommand(VmwareCbtMigrationVO migration, boolean failOnCleanupError, int waitSeconds) {
2698+
// Import may finish after cancellation and record the VM without changing the Cancelled state.
2699+
// Its target disks now belong to that VM, so neither cancel nor delete may remove them.
2700+
// This does not prevent source snapshot cleanup or deletion of the migration record.
2701+
Long importedVmId = migration.getVmId();
2702+
if (importedVmId == null) {
2703+
// Re-read before cleanup: an import may have recorded its VM since this caller loaded the migration.
2704+
VmwareCbtMigrationVO storedMigration = vmwareCbtMigrationDao.findById(migration.getId());
2705+
if (storedMigration != null) {
2706+
importedVmId = storedMigration.getVmId();
2707+
}
2708+
}
2709+
if (importedVmId != null) {
2710+
LOGGER.info("Skipping target disk cleanup for VMware CBT migration {} because it references imported VM {}.",
2711+
migration.getUuid(), importedVmId);
2712+
return;
2713+
}
26982714
if (!hasCleanupTargetDisks(migration)) {
26992715
return;
27002716
}

‎server/src/test/java/org/apache/cloudstack/vm/VmwareCbtMigrationDeletePolicyTest.java‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,21 @@
1616
// under the License.
1717
package org.apache.cloudstack.vm;
1818

19+
import java.util.Collections;
20+
21+
import org.apache.cloudstack.api.command.admin.vm.DeleteVmwareCbtMigrationCmd;
22+
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
1923
import org.junit.Assert;
2024
import org.junit.Test;
25+
import org.mockito.Mockito;
26+
import org.springframework.test.util.ReflectionTestUtils;
27+
28+
import com.cloud.agent.AgentManager;
29+
import com.cloud.vm.VmwareCbtMigrationDiskVO;
30+
import com.cloud.vm.VmwareCbtMigrationVO;
31+
import com.cloud.vm.dao.VmwareCbtMigrationCycleDao;
32+
import com.cloud.vm.dao.VmwareCbtMigrationDao;
33+
import com.cloud.vm.dao.VmwareCbtMigrationDiskDao;
2134

2235
public class VmwareCbtMigrationDeletePolicyTest {
2336

@@ -49,6 +62,112 @@ public void testActiveMigrationsCannotBeDeleted() {
4962
Assert.assertFalse(manager.canDeleteMigrationState(VmwareCbtMigration.State.ReadyForImport));
5063
}
5164

65+
@Test
66+
public void testCancelAndDeleteCleanupProtectRecordedImportedVmInEveryState() {
67+
VmwareCbtMigrationManagerImpl cleanupManager = new VmwareCbtMigrationManagerImpl();
68+
AgentManager agentManager = Mockito.mock(AgentManager.class);
69+
VmwareCbtMigrationDiskDao diskDao = Mockito.mock(VmwareCbtMigrationDiskDao.class);
70+
PrimaryDataStoreDao poolDao = Mockito.mock(PrimaryDataStoreDao.class);
71+
ReflectionTestUtils.setField(cleanupManager, "agentManager", agentManager);
72+
ReflectionTestUtils.setField(cleanupManager, "vmwareCbtMigrationDiskDao", diskDao);
73+
ReflectionTestUtils.setField(cleanupManager, "primaryDataStoreDao", poolDao);
74+
75+
for (VmwareCbtMigration.State state : VmwareCbtMigration.State.values()) {
76+
VmwareCbtMigrationVO migration = Mockito.mock(VmwareCbtMigrationVO.class);
77+
Mockito.when(migration.getState()).thenReturn(state);
78+
Mockito.when(migration.getVmId()).thenReturn(73L);
79+
Mockito.when(migration.getConvertHostId()).thenReturn(null);
80+
81+
ReflectionTestUtils.invokeMethod(cleanupManager, "sendCleanupCommandIfPossible", migration);
82+
ReflectionTestUtils.invokeMethod(cleanupManager, "sendCleanupCommandForDelete", migration);
83+
}
84+
85+
Mockito.verifyNoInteractions(agentManager, diskDao, poolDao);
86+
}
87+
88+
@Test
89+
public void testRecordedImportedVmDoesNotPreventMigrationRecordDeletionOrSourceCleanup() {
90+
for (VmwareCbtMigration.State state : new VmwareCbtMigration.State[] {
91+
VmwareCbtMigration.State.Cancelled, VmwareCbtMigration.State.Failed, VmwareCbtMigration.State.Completed }) {
92+
VmwareCbtMigrationManagerImpl deleteManager = Mockito.spy(new VmwareCbtMigrationManagerImpl());
93+
VmwareCbtMigrationVO migration = Mockito.mock(VmwareCbtMigrationVO.class);
94+
Mockito.when(migration.getId()).thenReturn(41L);
95+
Mockito.when(migration.getState()).thenReturn(state);
96+
Mockito.when(migration.getVmId()).thenReturn(73L);
97+
Mockito.when(migration.getConvertHostId()).thenReturn(null);
98+
VmwareCbtMigrationDao migrationDao = Mockito.mock(VmwareCbtMigrationDao.class);
99+
Mockito.when(migrationDao.findById(41L)).thenReturn(migration);
100+
Mockito.when(migrationDao.remove(41L)).thenReturn(true);
101+
VmwareCbtMigrationCycleDao cycleDao = Mockito.mock(VmwareCbtMigrationCycleDao.class);
102+
Mockito.when(cycleDao.listByMigrationId(41L)).thenReturn(Collections.emptyList());
103+
VmwareCbtMigrationDiskVO disk = Mockito.mock(VmwareCbtMigrationDiskVO.class);
104+
Mockito.when(disk.getId()).thenReturn(17L);
105+
VmwareCbtMigrationDiskDao diskDao = Mockito.mock(VmwareCbtMigrationDiskDao.class);
106+
Mockito.when(diskDao.listByMigrationId(41L)).thenReturn(Collections.singletonList(disk));
107+
AgentManager agentManager = Mockito.mock(AgentManager.class);
108+
ReflectionTestUtils.setField(deleteManager, "vmwareCbtMigrationDao", migrationDao);
109+
ReflectionTestUtils.setField(deleteManager, "vmwareCbtMigrationCycleDao", cycleDao);
110+
ReflectionTestUtils.setField(deleteManager, "vmwareCbtMigrationDiskDao", diskDao);
111+
ReflectionTestUtils.setField(deleteManager, "agentManager", agentManager);
112+
Mockito.doNothing().when(deleteManager).removeLingeringSourceSnapshots(migration);
113+
DeleteVmwareCbtMigrationCmd cmd = Mockito.mock(DeleteVmwareCbtMigrationCmd.class);
114+
Mockito.when(cmd.getId()).thenReturn(41L);
115+
Mockito.when(cmd.getCleanup()).thenReturn(true);
116+
117+
Assert.assertTrue(deleteManager.deleteVmwareCbtMigration(cmd));
118+
119+
Mockito.verifyNoInteractions(agentManager);
120+
Mockito.verify(diskDao, Mockito.times(1)).listByMigrationId(41L);
121+
Mockito.verify(diskDao).remove(17L);
122+
Mockito.verify(migrationDao).remove(41L);
123+
Mockito.verify(deleteManager, Mockito.times(state == VmwareCbtMigration.State.Completed ? 0 : 1))
124+
.removeLingeringSourceSnapshots(migration);
125+
}
126+
}
127+
128+
@Test
129+
public void testMigrationWithoutImportedVmStillExaminesCleanupTargets() {
130+
VmwareCbtMigrationManagerImpl cleanupManager = new VmwareCbtMigrationManagerImpl();
131+
VmwareCbtMigrationVO migration = Mockito.mock(VmwareCbtMigrationVO.class);
132+
Mockito.when(migration.getId()).thenReturn(41L);
133+
Mockito.when(migration.getVmId()).thenReturn(null);
134+
VmwareCbtMigrationDao migrationDao = Mockito.mock(VmwareCbtMigrationDao.class);
135+
Mockito.when(migrationDao.findById(41L)).thenReturn(migration);
136+
VmwareCbtMigrationDiskDao diskDao = Mockito.mock(VmwareCbtMigrationDiskDao.class);
137+
Mockito.when(diskDao.listByMigrationId(41L)).thenReturn(Collections.emptyList());
138+
ReflectionTestUtils.setField(cleanupManager, "vmwareCbtMigrationDiskDao", diskDao);
139+
ReflectionTestUtils.setField(cleanupManager, "vmwareCbtMigrationDao", migrationDao);
140+
ReflectionTestUtils.setField(cleanupManager, "primaryDataStoreDao", Mockito.mock(PrimaryDataStoreDao.class));
141+
142+
ReflectionTestUtils.invokeMethod(cleanupManager, "sendCleanupCommandIfPossible", migration);
143+
144+
Mockito.verify(diskDao).listByMigrationId(41L);
145+
}
146+
147+
@Test
148+
public void testCleanupProtectsImportedVmRecordedAfterCallerLoadedMigration() {
149+
VmwareCbtMigrationManagerImpl cleanupManager = new VmwareCbtMigrationManagerImpl();
150+
VmwareCbtMigrationVO staleMigration = Mockito.mock(VmwareCbtMigrationVO.class);
151+
Mockito.when(staleMigration.getId()).thenReturn(41L);
152+
Mockito.when(staleMigration.getVmId()).thenReturn(null);
153+
Mockito.when(staleMigration.getConvertHostId()).thenReturn(null);
154+
VmwareCbtMigrationVO storedMigration = Mockito.mock(VmwareCbtMigrationVO.class);
155+
Mockito.when(storedMigration.getVmId()).thenReturn(73L);
156+
VmwareCbtMigrationDao migrationDao = Mockito.mock(VmwareCbtMigrationDao.class);
157+
Mockito.when(migrationDao.findById(41L)).thenReturn(storedMigration);
158+
VmwareCbtMigrationDiskDao diskDao = Mockito.mock(VmwareCbtMigrationDiskDao.class);
159+
AgentManager agentManager = Mockito.mock(AgentManager.class);
160+
ReflectionTestUtils.setField(cleanupManager, "vmwareCbtMigrationDao", migrationDao);
161+
ReflectionTestUtils.setField(cleanupManager, "vmwareCbtMigrationDiskDao", diskDao);
162+
ReflectionTestUtils.setField(cleanupManager, "agentManager", agentManager);
163+
164+
ReflectionTestUtils.invokeMethod(cleanupManager, "sendCleanupCommandIfPossible", staleMigration);
165+
ReflectionTestUtils.invokeMethod(cleanupManager, "sendCleanupCommandForDelete", staleMigration);
166+
167+
Mockito.verify(migrationDao, Mockito.times(2)).findById(41L);
168+
Mockito.verifyNoInteractions(diskDao, agentManager);
169+
}
170+
52171
@Test
53172
public void testCurrentStepDurationFormattingMatchesImportTaskStyle() {
54173
Assert.assertEquals("0 secs", manager.formatDuration(0));

0 commit comments

Comments
 (0)