Copilot commented on code in PR #13084:
URL: https://github.com/apache/cloudstack/pull/13084#discussion_r3161303674
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -2009,6 +2009,34 @@ protected void setVolumeMigrationOptions(VolumeInfo
srcVolumeInfo, VolumeInfo de
destVolumeInfo.setMigrationOptions(migrationOptions);
}
+ /**
+ * KVM/libvirt selects linked-clone or full-clone storage migration for
the whole VM migration request.
+ * If any disk is backed by a direct-download template, force the request
to full clone so libvirt does
+ * not use incremental shared-backing semantics for a disk whose backing
chain is not guaranteed on the destination.
+ */
+ protected boolean shouldForceFullCloneMigration(Map<VolumeInfo, DataStore>
volumeDataStoreMap, Host destHost) {
+ for (Map.Entry<VolumeInfo, DataStore> entry :
volumeDataStoreMap.entrySet()) {
+ VolumeInfo srcVolumeInfo = entry.getKey();
+ DataStore destDataStore = entry.getValue();
+ StoragePoolVO sourceStoragePool =
_storagePoolDao.findById(srcVolumeInfo.getPoolId());
+ StoragePoolVO destStoragePool =
_storagePoolDao.findById(destDataStore.getId());
+
Review Comment:
`shouldForceFullCloneMigration` performs two `_storagePoolDao.findById(...)`
lookups per volume, and `copyAsync` immediately repeats those same lookups in
the main loop. This doubles DAO calls for every live migration request.
Consider computing the “force full clone” decision inside the existing
`copyAsync` loop (before `decideMigrationType...`) or refactoring to reuse the
already-fetched `sourceStoragePool`/`destStoragePool` objects to avoid
redundant database access.
##########
engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategyTest.java:
##########
@@ -210,6 +280,223 @@ public void configureMigrateDiskInfoWithBackingTest() {
Assert.assertEquals("backingPath",
migrateDiskInfo.getBackingStoreText());
}
+ @Test
+ public void
createLinkedCloneMigrationOptionsUsesSourceBackingFileWhenDestinationReferencePathDiffers()
{
+ VolumeInfo srcVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VolumeInfo destVolumeInfo = Mockito.mock(VolumeInfo.class);
+ StoragePoolVO srcPool = Mockito.mock(StoragePoolVO.class);
+ DataStore srcDataStore = Mockito.mock(DataStore.class);
+ Scope scope = Mockito.mock(Scope.class);
+ VMTemplateStoragePoolVO ref =
Mockito.mock(VMTemplateStoragePoolVO.class);
+
+ Mockito.when(srcPool.getUuid()).thenReturn("src-pool");
+
Mockito.when(srcPool.getPoolType()).thenReturn(StoragePoolType.NetworkFilesystem);
+ Mockito.when(srcPool.getClusterId()).thenReturn(1L);
+ Mockito.when(srcVolumeInfo.getTemplateId()).thenReturn(13L);
+ Mockito.when(srcVolumeInfo.getDataStore()).thenReturn(srcDataStore);
+ Mockito.when(srcDataStore.getScope()).thenReturn(scope);
+ Mockito.when(scope.getScopeType()).thenReturn(ScopeType.CLUSTER);
+ Mockito.when(destVolumeInfo.getPoolId()).thenReturn(4L);
+ Mockito.when(templatePoolDao.findByPoolTemplate(4L, 13L,
null)).thenReturn(ref);
+ Mockito.when(ref.getInstallPath()).thenReturn("target-backing.qcow2");
+
+ MigrationOptions options =
strategy.createLinkedCloneMigrationOptions(srcVolumeInfo, destVolumeInfo,
"source-backing.qcow2", srcPool);
+
+ Assert.assertTrue(options.isCopySrcTemplate());
+ Assert.assertEquals("source-backing.qcow2",
options.getSrcBackingFilePath());
+ }
+
+ @Test
+ public void
updateCopiedTemplateReferenceUpdatesExistingDestinationReference() {
+ VolumeInfo srcVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VolumeInfo destVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VMTemplateStoragePoolVO srcRef =
Mockito.mock(VMTemplateStoragePoolVO.class);
+ VMTemplateStoragePoolVO destRef =
Mockito.mock(VMTemplateStoragePoolVO.class);
+
+ Mockito.when(srcVolumeInfo.getPoolId()).thenReturn(5L);
+ Mockito.when(srcVolumeInfo.getTemplateId()).thenReturn(13L);
+ Mockito.when(destVolumeInfo.getPoolId()).thenReturn(4L);
+ Mockito.when(srcRef.getTemplateId()).thenReturn(13L);
+ Mockito.when(srcRef.getTemplateSize()).thenReturn(1851129856L);
+
Mockito.when(srcRef.getLocalDownloadPath()).thenReturn("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+
Mockito.when(srcRef.getInstallPath()).thenReturn("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+ Mockito.when(destRef.getId()).thenReturn(19L);
+ Mockito.when(templatePoolDao.findByPoolTemplate(5L, 13L,
null)).thenReturn(srcRef);
+ Mockito.when(templatePoolDao.findByPoolTemplate(4L, 13L,
null)).thenReturn(destRef);
+
+ strategy.updateCopiedTemplateReference(srcVolumeInfo, destVolumeInfo);
+
+ Mockito.verify(destRef).setDownloadPercent(100);
+
Mockito.verify(destRef).setDownloadState(VMTemplateStorageResourceAssoc.Status.DOWNLOADED);
+
Mockito.verify(destRef).setState(ObjectInDataStoreStateMachine.State.Ready);
+ Mockito.verify(destRef).setTemplateSize(1851129856L);
+
Mockito.verify(destRef).setLocalDownloadPath("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+
Mockito.verify(destRef).setInstallPath("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+ Mockito.verify(templatePoolDao).update(19L, destRef);
+ Mockito.verify(templatePoolDao,
Mockito.never()).persist(Mockito.any(VMTemplateStoragePoolVO.class));
+ }
+
+ @Test
+ public void
updateCopiedTemplateReferencePersistsDestinationReferenceWhenMissing() {
+ VolumeInfo srcVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VolumeInfo destVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VMTemplateStoragePoolVO srcRef =
Mockito.mock(VMTemplateStoragePoolVO.class);
+ ArgumentCaptor<VMTemplateStoragePoolVO> captor =
ArgumentCaptor.forClass(VMTemplateStoragePoolVO.class);
+
+ Mockito.when(srcVolumeInfo.getPoolId()).thenReturn(5L);
+ Mockito.when(srcVolumeInfo.getTemplateId()).thenReturn(13L);
+ Mockito.when(destVolumeInfo.getPoolId()).thenReturn(4L);
+ Mockito.when(srcRef.getTemplateId()).thenReturn(13L);
+ Mockito.when(srcRef.getTemplateSize()).thenReturn(1851129856L);
+
Mockito.when(srcRef.getLocalDownloadPath()).thenReturn("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+
Mockito.when(srcRef.getInstallPath()).thenReturn("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7");
+ Mockito.when(templatePoolDao.findByPoolTemplate(5L, 13L,
null)).thenReturn(srcRef);
+ Mockito.when(templatePoolDao.findByPoolTemplate(4L, 13L,
null)).thenReturn(null);
+
+ strategy.updateCopiedTemplateReference(srcVolumeInfo, destVolumeInfo);
+
+ Mockito.verify(templatePoolDao).persist(captor.capture());
+ VMTemplateStoragePoolVO persistedRef = captor.getValue();
+ Assert.assertEquals(4L, persistedRef.getPoolId());
+ Assert.assertEquals(13L, persistedRef.getTemplateId());
+ Assert.assertEquals(100, persistedRef.getDownloadPercent());
+ Assert.assertEquals(VMTemplateStorageResourceAssoc.Status.DOWNLOADED,
persistedRef.getDownloadState());
+ Assert.assertEquals(ObjectInDataStoreStateMachine.State.Ready,
persistedRef.getState());
+ Assert.assertEquals(1851129856L, persistedRef.getTemplateSize());
+ Assert.assertEquals("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7",
persistedRef.getLocalDownloadPath());
+ Assert.assertEquals("d06b4640-d7d3-45d7-92c1-8cd3c8eb1eb7",
persistedRef.getInstallPath());
+ Mockito.verify(templatePoolDao,
Mockito.never()).update(Mockito.anyLong(),
Mockito.any(VMTemplateStoragePoolVO.class));
+ }
+
+ @Test
+ public void
updateCopiedTemplateReferenceThrowsWhenSourceReferenceMissing() {
+ VolumeInfo srcVolumeInfo = Mockito.mock(VolumeInfo.class);
+ VolumeInfo destVolumeInfo = Mockito.mock(VolumeInfo.class);
+
+ Mockito.when(srcVolumeInfo.getPoolId()).thenReturn(5L);
+ Mockito.when(srcVolumeInfo.getTemplateId()).thenReturn(13L);
+ Mockito.when(templatePoolDao.findByPoolTemplate(5L, 13L,
null)).thenReturn(null);
+
+ try {
+ strategy.updateCopiedTemplateReference(srcVolumeInfo,
destVolumeInfo);
+ Assert.fail("Expected CloudRuntimeException to be thrown");
+ } catch (CloudRuntimeException e) {
+ Assert.assertTrue(e.getMessage().contains("source template
reference was not found"));
+ Assert.assertTrue(e.getMessage().contains("pool [5]"));
+ Assert.assertTrue(e.getMessage().contains("template [13]"));
+ }
+ }
+
+ @Test
+ public void
shouldForceFullCloneMigrationReturnsTrueForMixedDirectDownloadVolumes() {
+ VolumeInfo directDownloadVolume = Mockito.mock(VolumeInfo.class);
+ VolumeInfo regularVolume = Mockito.mock(VolumeInfo.class);
+ DataStore destPrimary = Mockito.mock(DataStore.class);
+ DataStore otherDestPrimary = Mockito.mock(DataStore.class);
+ Host destHost = Mockito.mock(Host.class);
+ Map<VolumeInfo, DataStore> volumeMap = new HashMap<>();
+
+ configurePoolLookup(directDownloadVolume, destPrimary, 1L, 2L,
StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem);
+ configurePoolLookup(regularVolume, otherDestPrimary, 3L, 4L,
StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem);
+ Mockito.when(directDownloadVolume.isDirectDownload()).thenReturn(true);
+ Mockito.when(regularVolume.isDirectDownload()).thenReturn(false);
+
+ volumeMap.put(directDownloadVolume, destPrimary);
+ volumeMap.put(regularVolume, otherDestPrimary);
+
+ Assert.assertTrue(strategy.shouldForceFullCloneMigration(volumeMap,
destHost));
+ }
+
+ @Test
+ public void
shouldForceFullCloneMigrationIgnoresSkippedDirectDownloadVolumes() {
+ VolumeInfo skippedDirectDownloadVolume =
Mockito.mock(VolumeInfo.class);
+ VolumeInfo regularVolume = Mockito.mock(VolumeInfo.class);
+ DataStore samePowerFlexStore = Mockito.mock(DataStore.class);
+ DataStore destPrimary = Mockito.mock(DataStore.class);
+ Host destHost = Mockito.mock(Host.class);
+ Map<VolumeInfo, DataStore> volumeMap = new HashMap<>();
+
+ configurePoolLookup(skippedDirectDownloadVolume, samePowerFlexStore,
1L, 1L, StoragePoolType.PowerFlex, StoragePoolType.PowerFlex);
+ configurePoolLookup(regularVolume, destPrimary, 3L, 4L,
StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem);
Review Comment:
In `shouldForceFullCloneMigrationIgnoresSkippedDirectDownloadVolumes`,
`skippedDirectDownloadVolume` is never stubbed to return `true` from
`isDirectDownload()`, so the assertion would still pass even if the skip logic
were removed. To actually verify the “ignored direct-download volume” behavior,
set
`Mockito.when(skippedDirectDownloadVolume.isDirectDownload()).thenReturn(true)`
and keep the expectation `false` due to the PowerFlex same-pool skip.
```suggestion
configurePoolLookup(regularVolume, destPrimary, 3L, 4L,
StoragePoolType.NetworkFilesystem, StoragePoolType.NetworkFilesystem);
Mockito.when(skippedDirectDownloadVolume.isDirectDownload()).thenReturn(true);
```
--
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]
