Skip to content

Commit 4928d9b

Browse files
harikrishna-patnalaharikrishna-patnala
committed
code optimizations
1 parent a3d1c94 commit 4928d9b

File tree

5 files changed

+71
-57
lines changed

5 files changed

+71
-57
lines changed

‎engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/StorageOrchestrationService.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,5 +33,5 @@ public interface StorageOrchestrationService {
3333

3434
Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore);
3535

36-
Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo source, DataStore sourceStore, DataStore destStore);
36+
Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo source, DataStore destStore);
3737
}

‎engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@
3838

3939
import com.cloud.dc.DataCenterVO;
4040
import com.cloud.dc.dao.DataCenterDao;
41+
import com.cloud.exception.ResourceAllocationException;
42+
import com.cloud.exception.StorageUnavailableException;
4143
import com.cloud.storage.VMTemplateVO;
4244
import com.cloud.storage.dao.VMTemplateDao;
4345
import com.cloud.template.TemplateManager;
@@ -322,11 +324,11 @@ public Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInf
322324
}
323325

324326
@Override
325-
public Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore sourceStore, DataStore destStore) {
327+
public Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore destStore) {
326328
Long dstZoneId = destStore.getScope().getScopeId();
327329
DataCenterVO dstZone = dcDao.findById(dstZoneId);
328330
long userId = CallContext.current().getCallingUserId();
329-
return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, sourceStore, dstZone));
331+
return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, dstZone));
330332
}
331333

332334
protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy, int skipped) {
@@ -678,17 +680,12 @@ public TemplateApiResult call() {
678680
private class CrossZoneCopyTemplateTask implements Callable<TemplateApiResult> {
679681
private final long userId;
680682
private final TemplateInfo sourceTmpl;
681-
private final DataStore sourceStore;
682683
private final DataCenterVO dstZone;
683684
private final String logid;
684685

685-
CrossZoneCopyTemplateTask(long userId,
686-
TemplateInfo sourceTmpl,
687-
DataStore sourceStore,
688-
DataCenterVO dstZone) {
686+
CrossZoneCopyTemplateTask(long userId, TemplateInfo sourceTmpl, DataCenterVO dstZone) {
689687
this.userId = userId;
690688
this.sourceTmpl = sourceTmpl;
691-
this.sourceStore = sourceStore;
692689
this.dstZone = dstZone;
693690
this.logid = ThreadContext.get(LOGCONTEXTID);
694691
}
@@ -699,16 +696,17 @@ public TemplateApiResult call() {
699696
TemplateApiResult result;
700697
VMTemplateVO template = templateDao.findById(sourceTmpl.getId());
701698
try {
699+
DataStore sourceStore = sourceTmpl.getDataStore();
702700
boolean success = templateManager.copy(userId, template, sourceStore, dstZone);
703701

704702
result = new TemplateApiResult(sourceTmpl);
705703
if (!success) {
706704
result.setResult("Cross-zone template copy failed");
707705
}
708-
} catch (Exception e) {
706+
} catch (StorageUnavailableException | ResourceAllocationException e) {
709707
logger.error("Exception while copying template [{}] from zone [{}] to zone [{}]",
710708
template,
711-
sourceStore.getScope().getScopeId(),
709+
sourceTmpl.getDataStore().getScope().getScopeId(),
712710
dstZone.getId(),
713711
e);
714712
result = new TemplateApiResult(sourceTmpl);

‎engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java‎

Lines changed: 35 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -615,13 +615,11 @@ protected void tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore
615615
}
616616

617617
protected boolean tryCopyingTemplateToImageStore(VMTemplateVO tmplt, DataStore destStore) {
618-
Long destZoneId = destStore.getScope().getScopeId();
619-
620-
List<DataStore> storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId);
621-
if (searchAndCopyWithinZone(tmplt, destStore, storesInSameZone)) {
618+
if (searchAndCopyWithinZone(tmplt, destStore)) {
622619
return true;
623620
}
624621

622+
Long destZoneId = destStore.getScope().getScopeId();
625623
logger.debug("Template [{}] not found in any image store of zone [{}]. Checking other zones.",
626624
tmplt.getUniqueName(), destZoneId);
627625

@@ -643,45 +641,54 @@ private boolean searchAndCopyAcrossZones(VMTemplateVO tmplt, DataStore destStore
643641
continue;
644642
}
645643

646-
DataStore sourceStore = findImageStorageHavingTemplate(tmplt, storesInOtherZone);
647-
if (sourceStore == null) {
648-
logger.debug("Template [{}] not found in any image store of zone [{}].",
644+
TemplateObject sourceTmpl = findUsableTemplate(tmplt, storesInOtherZone);
645+
if (sourceTmpl == null) {
646+
logger.debug("Template [{}] not found with a valid install path in any image store of zone [{}].",
649647
tmplt.getUniqueName(), otherZoneId);
650648
continue;
651649
}
652650

653-
TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore);
654-
if (sourceTmpl.getInstallPath() == null) {
655-
logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.",
656-
tmplt.getUniqueName(), sourceStore.getName());
657-
continue;
658-
}
659-
660651
logger.info("Template [{}] found in zone [{}]. Initiating cross-zone copy to zone [{}].",
661652
tmplt.getUniqueName(), otherZoneId, destZoneId);
662653

663-
return copyTemplateAcrossZones(sourceStore, destStore, tmplt);
654+
return copyTemplateAcrossZones(destStore, sourceTmpl);
664655
}
665656

666-
logger.debug("Template [{}] was not found in any zone. Cannot perform zone-to-zone copy.",
667-
tmplt.getUniqueName());
657+
logger.debug("Template [{}] was not found in any zone. Cannot perform zone-to-zone copy.", tmplt.getUniqueName());
668658
return false;
669659
}
670660

671-
private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore, List<DataStore> stores) {
672-
for (DataStore sourceStore : stores) {
661+
protected TemplateObject findUsableTemplate(VMTemplateVO tmplt, List<DataStore> imageStores) {
662+
for (DataStore store : imageStores) {
663+
TemplateObject tmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), store);
664+
if (tmpl == null) {
665+
continue;
666+
}
667+
668+
if (tmpl.getInstallPath() == null) {
669+
logger.debug("Template [{}] found in image store [{}] but install path is null. Skipping.",
670+
tmplt.getUniqueName(), store.getName());
671+
continue;
672+
}
673+
return tmpl;
674+
}
675+
return null;
676+
}
677+
678+
private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore) {
679+
Long destZoneId = destStore.getScope().getScopeId();
680+
List<DataStore> storesInSameZone = _storeMgr.getImageStoresByZoneIds(destZoneId);
681+
for (DataStore sourceStore : storesInSameZone) {
673682
Map<String, TemplateProp> existingTemplatesInSourceStore = listTemplate(sourceStore);
674683
if (existingTemplatesInSourceStore == null ||
675684
!existingTemplatesInSourceStore.containsKey(tmplt.getUniqueName())) {
676-
logger.debug("Template [{}] does not exist on image store [{}]; searching another.",
677-
tmplt.getUniqueName(), sourceStore.getName());
685+
logger.debug("Template [{}] does not exist on image store [{}]; searching another.", tmplt.getUniqueName(), sourceStore.getName());
678686
continue;
679687
}
680688

681689
TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore);
682690
if (sourceTmpl.getInstallPath() == null) {
683-
logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.",
684-
tmplt.getUniqueName(), sourceStore.getName());
691+
logger.warn("Cannot copy template [{}] from image store [{}]; install path is null.", tmplt.getUniqueName(), sourceStore.getName());
685692
continue;
686693
}
687694

@@ -691,36 +698,22 @@ private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore,
691698
return false;
692699
}
693700

694-
private DataStore findImageStorageHavingTemplate(VMTemplateVO tmplt, List<DataStore> stores) {
695-
for (DataStore store : stores) {
696-
Map<String, TemplateProp> templates = listTemplate(store);
697-
if (templates != null && templates.containsKey(tmplt.getUniqueName())) {
698-
return store;
699-
}
700-
}
701-
return null;
702-
}
703-
704-
private boolean copyTemplateAcrossZones(DataStore sourceStore,
705-
DataStore destStore,
706-
VMTemplateVO tmplt) {
701+
private boolean copyTemplateAcrossZones(DataStore destStore, TemplateObject sourceTmpl) {
707702
Long dstZoneId = destStore.getScope().getScopeId();
708703
DataCenterVO dstZone = _dcDao.findById(dstZoneId);
709704

710705
if (dstZone == null) {
711-
logger.warn("Destination zone [{}] not found for template [{}].",
712-
dstZoneId, tmplt.getUniqueName());
706+
logger.warn("Destination zone [{}] not found for template [{}].", dstZoneId, sourceTmpl.getUniqueName());
713707
return false;
714708
}
715709

716-
TemplateObject sourceTmpl = (TemplateObject) _templateFactory.getTemplate(tmplt.getId(), sourceStore);
717710
try {
718-
storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, sourceStore, destStore);
711+
storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, destStore);
719712
return true;
720713
} catch (Exception e) {
721714
logger.error("Failed to copy template [{}] from zone [{}] to zone [{}].",
722-
tmplt.getUniqueName(),
723-
sourceStore.getScope().getScopeId(),
715+
sourceTmpl.getUniqueName(),
716+
sourceTmpl.getDataStore().getScope().getScopeId(),
724717
dstZoneId,
725718
e);
726719
return false;

‎engine/storage/image/src/test/java/org/apache/cloudstack/storage/image/TemplateServiceImplTest.java‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,6 @@ public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAno
213213

214214
Map<String, TemplateProp> templatesInOtherZone = new HashMap<>();
215215
templatesInOtherZone.put(tmpltMock.getUniqueName(), tmpltPropMock);
216-
Mockito.doReturn(templatesInOtherZone).when(templateService).listTemplate(otherZoneStoreMock);
217216

218217
TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class);
219218
Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock);
@@ -241,7 +240,6 @@ public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenDestinationZoneIsM
241240

242241
Map<String, TemplateProp> templates = new HashMap<>();
243242
templates.put(tmpltMock.getUniqueName(), tmpltPropMock);
244-
Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock);
245243

246244
TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class);
247245
Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock);
@@ -267,7 +265,6 @@ public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIs
267265

268266
Map<String, TemplateProp> templates = new HashMap<>();
269267
templates.put(tmpltMock.getUniqueName(), tmpltPropMock);
270-
Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock);
271268

272269
TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class);
273270
Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock);
@@ -294,4 +291,31 @@ public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenTemplateNotFoundIn
294291

295292
Assert.assertFalse(result);
296293
}
294+
295+
@Test
296+
public void testFindUsableTemplateReturnsTemplateWithNonNullInstallPath() {
297+
VMTemplateVO template = Mockito.mock(VMTemplateVO.class);
298+
Mockito.when(template.getId()).thenReturn(10L);
299+
Mockito.when(template.getUniqueName()).thenReturn("test-template");
300+
301+
DataStore storeWithNullPath = Mockito.mock(DataStore.class);
302+
Mockito.when(storeWithNullPath.getName()).thenReturn("store-null");
303+
304+
DataStore storeWithValidPath = Mockito.mock(DataStore.class);
305+
TemplateObject tmplWithNullPath = Mockito.mock(TemplateObject.class);
306+
Mockito.when(tmplWithNullPath.getInstallPath()).thenReturn(null);
307+
308+
TemplateObject tmplWithValidPath = Mockito.mock(TemplateObject.class);
309+
Mockito.when(tmplWithValidPath.getInstallPath()).thenReturn("/mnt/secondary/template.qcow2");
310+
311+
Mockito.doReturn(tmplWithNullPath).when(templateDataFactoryMock).getTemplate(10L, storeWithNullPath);
312+
Mockito.doReturn(tmplWithValidPath).when(templateDataFactoryMock).getTemplate(10L, storeWithValidPath);
313+
314+
List<DataStore> imageStores = List.of(storeWithNullPath, storeWithValidPath);
315+
316+
TemplateObject result = templateService.findUsableTemplate(template, imageStores);
317+
318+
Assert.assertNotNull(result);
319+
Assert.assertEquals(tmplWithValidPath, result);
320+
}
297321
}

‎server/src/main/java/com/cloud/storage/ImageStoreDetailsUtil.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,6 @@ public String getNfsVersionByUuid(String storeUuid){
7979
}
8080

8181
public boolean isCopyTemplatesFromOtherStoragesEnabled(Long storeId, Long zoneId) {
82-
8382
final Map<String, String> storeDetails = imageStoreDetailsDao.getDetails(storeId);
8483
final String keyWithoutDots = StorageManager.COPY_TEMPLATES_FROM_OTHER_SECONDARY_STORAGES.key()
8584
.replace(".", "");

0 commit comments

Comments
 (0)