Skip to content

Commit d2b28db

Browse files
harikrishna-patnalaharikrishna-patnala
committed
Combine template copy and download into a single asynchronous operation
1 parent 8f4f20d commit d2b28db

File tree

6 files changed

+108
-62
lines changed

6 files changed

+108
-62
lines changed

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,5 @@ public interface StorageOrchestrationService {
3131

3232
MigrationResponse migrateResources(Long srcImgStoreId, Long destImgStoreId, List<Long> templateIdList, List<Long> snapshotIdList);
3333

34-
Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInfo source, DataStore destStore);
35-
36-
Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo source, DataStore destStore);
34+
Future<TemplateApiResult> orchestrateTemplateCopyFromSecondaryStores(long templateId, DataStore destStore);
3735
}

‎engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/TemplateService.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,4 +80,6 @@ public TemplateInfo getTemplate() {
8080
List<DatadiskTO> getTemplateDatadisksOnImageStore(TemplateInfo templateInfo, String configurationId);
8181

8282
AsyncCallFuture<TemplateApiResult> copyTemplateToImageStore(DataObject source, DataStore destStore);
83-
}
83+
84+
void handleTemplateCopyFromSecondaryStores(long templateId, DataStore destStore);
85+
}

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

Lines changed: 15 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@
4444
import com.cloud.storage.dao.VMTemplateDao;
4545
import com.cloud.template.TemplateManager;
4646
import org.apache.cloudstack.api.response.MigrationResponse;
47-
import org.apache.cloudstack.context.CallContext;
4847
import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
4948
import org.apache.cloudstack.engine.subsystem.api.storage.DataObject;
5049
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
@@ -53,6 +52,7 @@
5352
import org.apache.cloudstack.engine.subsystem.api.storage.SecondaryStorageService.DataObjectResult;
5453
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotDataFactory;
5554
import org.apache.cloudstack.engine.subsystem.api.storage.SnapshotInfo;
55+
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateDataFactory;
5656
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateInfo;
5757
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService;
5858
import org.apache.cloudstack.engine.subsystem.api.storage.TemplateService.TemplateApiResult;
@@ -116,6 +116,8 @@ public class StorageOrchestrator extends ManagerBase implements StorageOrchestra
116116
@Inject
117117
VMTemplateDao templateDao;
118118
@Inject
119+
TemplateDataFactory templateDataFactory;
120+
@Inject
119121
DataCenterDao dcDao;
120122

121123

@@ -324,11 +326,9 @@ public Future<TemplateApiResult> orchestrateTemplateCopyToImageStore(TemplateInf
324326
}
325327

326328
@Override
327-
public Future<TemplateApiResult> orchestrateTemplateCopyAcrossZones(TemplateInfo templateInfo, DataStore destStore) {
329+
public Future<TemplateApiResult> orchestrateTemplateCopyFromSecondaryStores(long srcTemplateId, DataStore destStore) {
328330
Long dstZoneId = destStore.getScope().getScopeId();
329-
DataCenterVO dstZone = dcDao.findById(dstZoneId);
330-
long userId = CallContext.current().getCallingUserId();
331-
return submit(dstZoneId, new CrossZoneCopyTemplateTask(userId, templateInfo, dstZone));
331+
return submit(dstZoneId, new CopyTemplateFromSecondaryStorageTask(srcTemplateId, destStore));
332332
}
333333

334334
protected Pair<String, Boolean> migrateCompleted(Long destDatastoreId, DataStore srcDatastore, List<DataObject> files, MigrationPolicy migrationPolicy, int skipped) {
@@ -677,42 +677,28 @@ public TemplateApiResult call() {
677677
}
678678
}
679679

680-
private class CrossZoneCopyTemplateTask implements Callable<TemplateApiResult> {
681-
private final long userId;
682-
private final TemplateInfo sourceTmpl;
683-
private final DataCenterVO dstZone;
680+
private class CopyTemplateFromSecondaryStorageTask implements Callable<TemplateApiResult> {
681+
private final long srcTemplateId;
682+
private final DataStore destStore;
684683
private final String logid;
685684

686-
CrossZoneCopyTemplateTask(long userId, TemplateInfo sourceTmpl, DataCenterVO dstZone) {
687-
this.userId = userId;
688-
this.sourceTmpl = sourceTmpl;
689-
this.dstZone = dstZone;
685+
CopyTemplateFromSecondaryStorageTask(long srcTemplateId, DataStore destStore) {
686+
this.srcTemplateId = srcTemplateId;
687+
this.destStore = destStore;
690688
this.logid = ThreadContext.get(LOGCONTEXTID);
691689
}
692690

693691
@Override
694692
public TemplateApiResult call() {
695693
ThreadContext.put(LOGCONTEXTID, logid);
696694
TemplateApiResult result;
697-
VMTemplateVO template = templateDao.findById(sourceTmpl.getId());
695+
long destZoneId = destStore.getScope().getScopeId();
696+
TemplateInfo sourceTmpl = templateDataFactory.getTemplate(srcTemplateId, DataStoreRole.Image);
698697
try {
699-
DataStore sourceStore = sourceTmpl.getDataStore();
700-
boolean success = templateManager.copy(userId, template, sourceStore, dstZone);
701-
698+
templateService.handleTemplateCopyFromSecondaryStores(srcTemplateId, destStore);
702699
result = new TemplateApiResult(sourceTmpl);
703-
if (!success) {
704-
result.setResult("Cross-zone template copy failed");
705-
}
706-
} catch (StorageUnavailableException | ResourceAllocationException e) {
707-
logger.error("Exception while copying template [{}] from zone [{}] to zone [{}]",
708-
template,
709-
sourceTmpl.getDataStore().getScope().getScopeId(),
710-
dstZone.getId(),
711-
e);
712-
result = new TemplateApiResult(sourceTmpl);
713-
result.setResult(e.getMessage());
714700
} finally {
715-
tryCleaningUpExecutor(dstZone.getId());
701+
tryCleaningUpExecutor(destZoneId);
716702
ThreadContext.clearAll();
717703
}
718704

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

Lines changed: 49 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@
3131

3232
import javax.inject.Inject;
3333

34+
import com.cloud.exception.StorageUnavailableException;
35+
import org.apache.cloudstack.context.CallContext;
3436
import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
3537
import org.apache.cloudstack.engine.subsystem.api.storage.CopyCommandResult;
3638
import org.apache.cloudstack.engine.subsystem.api.storage.CreateCmdResult;
@@ -70,6 +72,7 @@
7072
import org.apache.commons.lang3.StringUtils;
7173
import org.apache.logging.log4j.Logger;
7274
import org.apache.logging.log4j.LogManager;
75+
import org.apache.logging.log4j.ThreadContext;
7376
import org.springframework.stereotype.Component;
7477

7578
import com.cloud.agent.api.Answer;
@@ -548,10 +551,7 @@ public void handleTemplateSync(DataStore store) {
548551
}
549552

550553
if (availHypers.contains(tmplt.getHypervisorType())) {
551-
boolean copied = imageStoreDetailsUtil.isCopyTemplatesFromOtherStoragesEnabled(storeId, zoneId) && tryCopyingTemplateToImageStore(tmplt, store);
552-
if (!copied) {
553-
tryDownloadingTemplateToImageStore(tmplt, store);
554-
}
554+
storageOrchestrator.orchestrateTemplateCopyFromSecondaryStores(tmplt.getId(), store);
555555
} else {
556556
logger.info("Skip downloading template {} since current data center does not have hypervisor {}", tmplt, tmplt.getHypervisorType());
557557
}
@@ -598,6 +598,16 @@ public void handleTemplateSync(DataStore store) {
598598

599599
}
600600

601+
@Override
602+
public void handleTemplateCopyFromSecondaryStores(long templateId, DataStore destStore) {
603+
VMTemplateVO template = _templateDao.findById(templateId);
604+
long zoneId = destStore.getScope().getScopeId();
605+
boolean copied = imageStoreDetailsUtil.isCopyTemplatesFromOtherStoragesEnabled(destStore.getId(), zoneId) && tryCopyingTemplateToImageStore(template, destStore);
606+
if (!copied) {
607+
tryDownloadingTemplateToImageStore(template, destStore);
608+
}
609+
}
610+
601611
protected void tryDownloadingTemplateToImageStore(VMTemplateVO tmplt, DataStore destStore) {
602612
if (tmplt.getUrl() == null) {
603613
logger.info("Not downloading template [{}] to image store [{}], as it has no URL.", tmplt.getUniqueName(),
@@ -686,8 +696,17 @@ private boolean searchAndCopyWithinZone(VMTemplateVO tmplt, DataStore destStore)
686696
return false;
687697
}
688698

689-
storageOrchestrator.orchestrateTemplateCopyToImageStore(sourceTmpl, destStore);
690-
return true;
699+
TemplateApiResult result;
700+
AsyncCallFuture<TemplateApiResult> future = copyTemplateToImageStore(sourceTmpl, destStore);
701+
try {
702+
result = future.get();
703+
} catch (ExecutionException | InterruptedException e) {
704+
logger.warn("Exception while copying template [{}] from image store [{}] to image store [{}]: {}",
705+
sourceTmpl.getUniqueName(), sourceTmpl.getDataStore().getName(), destStore.getName(), e.toString());
706+
result = new TemplateApiResult(sourceTmpl);
707+
result.setResult(e.getMessage());
708+
}
709+
return result.isSuccess();
691710
}
692711

693712
private boolean copyTemplateAcrossZones(DataStore destStore, TemplateObject sourceTmpl) {
@@ -699,9 +718,29 @@ private boolean copyTemplateAcrossZones(DataStore destStore, TemplateObject sour
699718
return false;
700719
}
701720

721+
TemplateApiResult result;
702722
try {
703-
storageOrchestrator.orchestrateTemplateCopyAcrossZones(sourceTmpl, destStore);
704-
return true;
723+
VMTemplateVO template = _templateDao.findById(sourceTmpl.getId());
724+
try {
725+
DataStore sourceStore = sourceTmpl.getDataStore();
726+
long userId = CallContext.current().getCallingUserId();
727+
boolean success = _tmpltMgr.copy(userId, template, sourceStore, dstZone);
728+
729+
result = new TemplateApiResult(sourceTmpl);
730+
if (!success) {
731+
result.setResult("Cross-zone template copy failed");
732+
}
733+
} catch (StorageUnavailableException | ResourceAllocationException e) {
734+
logger.error("Exception while copying template [{}] from zone [{}] to zone [{}]",
735+
template,
736+
sourceTmpl.getDataStore().getScope().getScopeId(),
737+
dstZone.getId(),
738+
e);
739+
result = new TemplateApiResult(sourceTmpl);
740+
result.setResult(e.getMessage());
741+
} finally {
742+
ThreadContext.clearAll();
743+
}
705744
} catch (Exception e) {
706745
logger.error("Failed to copy template [{}] from zone [{}] to zone [{}].",
707746
sourceTmpl.getUniqueName(),
@@ -710,6 +749,8 @@ private boolean copyTemplateAcrossZones(DataStore destStore, TemplateObject sour
710749
e);
711750
return false;
712751
}
752+
753+
return result.isSuccess();
713754
}
714755

715756
@Override

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

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,18 @@
2020

2121
import com.cloud.dc.DataCenterVO;
2222
import com.cloud.dc.dao.DataCenterDao;
23+
import com.cloud.exception.ResourceAllocationException;
24+
import com.cloud.exception.StorageUnavailableException;
25+
import com.cloud.storage.dao.VMTemplateDao;
2326
import com.cloud.storage.template.TemplateProp;
2427
import com.cloud.template.TemplateManager;
28+
import com.cloud.user.Account;
29+
import com.cloud.user.User;
30+
import org.apache.cloudstack.context.CallContext;
2531
import org.apache.cloudstack.engine.orchestration.service.StorageOrchestrationService;
2632
import org.apache.cloudstack.engine.subsystem.api.storage.DataStore;
2733
import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
2834
import org.apache.cloudstack.engine.subsystem.api.storage.Scope;
29-
import org.apache.cloudstack.framework.async.AsyncCallFuture;
3035
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreDao;
3136
import org.apache.cloudstack.storage.datastore.db.TemplateDataStoreVO;
3237
import org.apache.cloudstack.storage.image.store.TemplateObject;
@@ -48,6 +53,8 @@
4853
import java.util.List;
4954
import java.util.Map;
5055

56+
import static org.mockito.Mockito.mock;
57+
5158
@RunWith(MockitoJUnitRunner.class)
5259
public class TemplateServiceImplTest {
5360

@@ -88,6 +95,9 @@ public class TemplateServiceImplTest {
8895
@Mock
8996
DataCenterDao _dcDao;
9097

98+
@Mock
99+
VMTemplateDao templateDao;
100+
91101
@Mock
92102
TemplateManager _tmpltMgr;
93103

@@ -103,7 +113,6 @@ public void setUp() {
103113
Mockito.doReturn(List.of(sourceStoreMock, destStoreMock)).when(dataStoreManagerMock).getImageStoresByZoneIds(zoneId);
104114
Mockito.doReturn(templatesInSourceStore).when(templateService).listTemplate(sourceStoreMock);
105115
Mockito.doReturn(null).when(templateService).listTemplate(destStoreMock);
106-
Mockito.doReturn("install-path").when(templateInfoMock).getInstallPath();
107116
Mockito.doReturn(templateInfoMock).when(templateDataFactoryMock).getTemplate(2L, sourceStoreMock);
108117
}
109118

@@ -168,7 +177,7 @@ public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenTemplateDoesNotExi
168177
boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
169178

170179
Assert.assertFalse(result);
171-
Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
180+
Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyFromSecondaryStores(Mockito.anyLong(), Mockito.any());
172181
}
173182

174183
@Test
@@ -184,22 +193,11 @@ public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenInstallPathIsNull(
184193
boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
185194

186195
Assert.assertFalse(result);
187-
Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
196+
Mockito.verify(storageOrchestrator, Mockito.never()).orchestrateTemplateCopyFromSecondaryStores(Mockito.anyLong(), Mockito.any());
188197
}
189198

190199
@Test
191-
public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherStorageAndTaskWasScheduled() {
192-
templatesInSourceStore.put(tmpltMock.getUniqueName(), tmpltPropMock);
193-
Mockito.doReturn(new AsyncCallFuture<>()).when(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
194-
195-
boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
196-
197-
Assert.assertTrue(result);
198-
Mockito.verify(storageOrchestrator).orchestrateTemplateCopyToImageStore(Mockito.any(), Mockito.any());
199-
}
200-
201-
@Test
202-
public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone() {
200+
public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAnotherZone() throws StorageUnavailableException, ResourceAllocationException {
203201
Scope scopeMock = Mockito.mock(Scope.class);
204202
Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
205203
Mockito.doReturn(1L).when(scopeMock).getScopeId();
@@ -222,6 +220,7 @@ public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenTemplateExistsInAno
222220

223221
DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class);
224222
Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L);
223+
Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any());
225224

226225
boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
227226

@@ -256,7 +255,7 @@ public void tryCopyingTemplateToImageStoreTestReturnsFalseWhenDestinationZoneIsM
256255
}
257256

258257
@Test
259-
public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIsScheduled() {
258+
public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIsScheduled() throws StorageUnavailableException, ResourceAllocationException {
260259
Scope scopeMock = Mockito.mock(Scope.class);
261260
Mockito.doReturn(scopeMock).when(destStoreMock).getScope();
262261
Mockito.doReturn(1L).when(scopeMock).getScopeId();
@@ -270,15 +269,27 @@ public void tryCopyingTemplateToImageStoreTestReturnsTrueWhenCrossZoneCopyTaskIs
270269

271270
Map<String, TemplateProp> templates = new HashMap<>();
272271
templates.put("unique-name", tmpltPropMock);
273-
274272
Mockito.doReturn(templates).when(templateService).listTemplate(otherZoneStoreMock);
275273

276274
TemplateObject sourceTmplMock = Mockito.mock(TemplateObject.class);
277275
Mockito.doReturn(sourceTmplMock).when(templateDataFactoryMock).getTemplate(100L, otherZoneStoreMock);
278276
Mockito.doReturn("/mnt/secondary/template.qcow2").when(sourceTmplMock).getInstallPath();
277+
Mockito.doReturn(100L).when(sourceTmplMock).getId();
278+
279+
DataStore sourceStoreMock = Mockito.mock(DataStore.class);
280+
Scope sourceScopeMock = Mockito.mock(Scope.class);
281+
Mockito.doReturn(sourceStoreMock).when(sourceTmplMock).getDataStore();
279282

280283
DataCenterVO dstZoneMock = Mockito.mock(DataCenterVO.class);
281284
Mockito.doReturn(dstZoneMock).when(_dcDao).findById(1L);
285+
VMTemplateVO templateVoMock = Mockito.mock(VMTemplateVO.class);
286+
Mockito.doReturn(templateVoMock).when(templateDao).findById(100L);
287+
288+
Mockito.doReturn(true).when(_tmpltMgr).copy(Mockito.anyLong(), Mockito.any(), Mockito.any(), Mockito.any());
289+
290+
Account account = mock(Account.class);
291+
User user = mock(User.class);
292+
CallContext callContext = mock(CallContext.class);
282293

283294
boolean result = templateService.tryCopyingTemplateToImageStore(tmpltMock, destStoreMock);
284295

‎server/src/main/java/com/cloud/template/TemplateManagerImpl.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -838,6 +838,9 @@ public boolean copy(long userId, VMTemplateVO template, DataStore srcSecStore, D
838838
// Copy will just find one eligible image store for the destination zone
839839
// and copy template there, not propagate to all image stores
840840
// for that zone
841+
842+
boolean copied = false;
843+
841844
for (DataStore dstSecStore : dstSecStores) {
842845
TemplateDataStoreVO dstTmpltStore = _tmplStoreDao.findByStoreTemplate(dstSecStore.getId(), tmpltId);
843846
if (dstTmpltStore != null && dstTmpltStore.getDownloadState() == Status.DOWNLOADED) {
@@ -852,9 +855,12 @@ public boolean copy(long userId, VMTemplateVO template, DataStore srcSecStore, D
852855
TemplateApiResult result = future.get();
853856
if (result.isFailed()) {
854857
logger.debug("copy template failed for image store {}: {}", dstSecStore, result.getResult());
858+
_tmplStoreDao.removeByTemplateStore(tmpltId, dstSecStore.getId());
855859
continue; // try next image store
856860
}
857861

862+
copied = true;
863+
858864
_tmpltDao.addTemplateToZone(template, dstZoneId);
859865

860866
if (account.getId() != Account.ACCOUNT_ID_SYSTEM) {
@@ -882,12 +888,14 @@ public boolean copy(long userId, VMTemplateVO template, DataStore srcSecStore, D
882888
}
883889
}
884890
}
891+
892+
return true;
893+
885894
} catch (Exception ex) {
886-
logger.debug("failed to copy template to image store:{} ,will try next one", dstSecStore);
895+
logger.debug("failed to copy template to image store:{}, will try next one", dstSecStore, ex);
887896
}
888897
}
889-
return true;
890-
898+
return copied;
891899
}
892900

893901
@Override

0 commit comments

Comments
 (0)