Skip to content

Commit 8e65a56

Browse files
rajiv-jain-netapprajiv-jain-netapp
committed
CSTACKEX-259: review comments resolve
1 parent c4f3b78 commit 8e65a56

4 files changed

Lines changed: 336 additions & 44 deletions

File tree

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java‎

Lines changed: 95 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
171171
// Create the backend storage object: a clone of the cached template when the
172172
// orchestrator asked for one, otherwise a blank LUN (iSCSI) or qcow2 file (NFS).
173173
Long cloneOfTemplateId = getTemplateIdForCloning(volInfo.getId());
174-
CloudStackVolume created = cloneOfTemplateId != null
174+
CloudStackVolume clonedCloudStackVolume = cloneOfTemplateId != null
175175
? cloneCloudStackVolumeFromTemplate(storagePool, volInfo, details, cloneOfTemplateId)
176176
: createCloudStackVolume(storagePool, volInfo, details);
177177

@@ -181,17 +181,14 @@ public void createAsync(DataStore dataStore, DataObject dataObject, AsyncComplet
181181
logger.info("createAsync: Volume format set to [{}] for hypervisor [{}] and protocol [{}]", volumeVO.getFormat(), storagePool.getHypervisor(), details.get(OntapStorageConstants.PROTOCOL));
182182

183183
if (ProtocolType.ISCSI.name().equalsIgnoreCase(details.get(OntapStorageConstants.PROTOCOL))) {
184-
String lunName = created != null && created.getLun() != null ? created.getLun().getName() : null;
185-
if (lunName == null) {
186-
throw new CloudRuntimeException("Missing LUN name for volume " + volInfo.getId());
187-
}
184+
// createCloudStackVolume validates the Feign response (LUN name + uuid) before returning
185+
Lun createdLun = clonedCloudStackVolume.getLun();
186+
String lunName = createdLun.getName();
188187

189188
// Persist LUN details for future operations (delete, grant/revoke access)
190-
volumeDetailsDao.addDetail(volInfo.getId(), OntapStorageConstants.LUN_DOT_UUID, created.getLun().getUuid(), false);
189+
volumeDetailsDao.addDetail(volInfo.getId(), OntapStorageConstants.LUN_DOT_UUID, createdLun.getUuid(), false);
191190
volumeDetailsDao.addDetail(volInfo.getId(), OntapStorageConstants.LUN_DOT_NAME, lunName, false);
192-
if (created.getLun().getUuid() != null) {
193-
volumeVO.setFolder(created.getLun().getUuid());
194-
}
191+
volumeVO.setFolder(createdLun.getUuid());
195192

196193
logger.info("createAsync: Created LUN [{}] for volume [{}]. LUN mapping will occur during grantAccess() to per-host igroup.",
197194
lunName, volumeVO.getId());
@@ -264,22 +261,31 @@ private CreateCmdResult createTemplateOnPrimary(StoragePoolVO storagePool, Templ
264261
}
265262

266263
StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details);
267-
CloudStackVolume created = storageStrategy.createCloudStackVolume(
268-
createTemplateLunRequest(storagePool, details, templateInfo.getId(), sizeInBytes));
269-
270-
if (created == null || created.getLun() == null || created.getLun().getName() == null) {
271-
throw new CloudRuntimeException("ONTAP returned no LUN for the cache of template [" + templateInfo.getId() + "]");
264+
CloudStackVolume createdCloudStackVolume = null;
265+
try {
266+
createdCloudStackVolume = storageStrategy.createCloudStackVolume(
267+
createTemplateLunRequest(storagePool, details, templateInfo.getId(), sizeInBytes));
268+
// createCloudStackVolume validates the Feign response (LUN name + uuid) before returning
269+
Lun lun = createdCloudStackVolume.getLun();
270+
templatePoolRef.setLocalDownloadPath(lun.getUuid());
271+
templatePoolRef.setTemplateSize(sizeInBytes);
272+
vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
273+
274+
logger.info("createTemplateOnPrimary: Created template cache LUN [{}] (uuid [{}], {} bytes) on pool [{}] for template [{}]",
275+
lun.getName(), lun.getUuid(), sizeInBytes, storagePool.getId(), templateInfo.getId());
276+
277+
return new CreateCmdResult(lun.getName(), new Answer(null, true, null));
278+
} catch (Exception e) {
279+
// Compensating delete: orchestrator only cleans DB metadata on create failure.
280+
bestEffortDeleteTemplateCacheLun(storageStrategy, details.get(OntapStorageConstants.SVM_NAME),
281+
getTemplateLunName(storagePool, templateInfo.getId()),
282+
createdCloudStackVolume != null && createdCloudStackVolume.getLun() != null ? createdCloudStackVolume.getLun().getUuid() : null);
283+
if (e instanceof CloudRuntimeException) {
284+
throw (CloudRuntimeException) e;
285+
}
286+
throw new CloudRuntimeException("Failed to create template cache LUN for template [" + templateInfo.getId()
287+
+ "]: " + e.getMessage(), e);
272288
}
273-
274-
Lun lun = created.getLun();
275-
templatePoolRef.setLocalDownloadPath(lun.getUuid());
276-
templatePoolRef.setTemplateSize(sizeInBytes);
277-
vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
278-
279-
logger.info("createTemplateOnPrimary: Created template cache LUN [{}] (uuid [{}], {} bytes) on pool [{}] for template [{}]",
280-
lun.getName(), lun.getUuid(), sizeInBytes, storagePool.getId(), templateInfo.getId());
281-
282-
return new CreateCmdResult(lun.getName(), new Answer(null, true, null));
283289
}
284290

285291
/**
@@ -299,7 +305,8 @@ private CloudStackVolume cloneCloudStackVolumeFromTemplate(StoragePoolVO storage
299305
: createCloneFileRequest(storagePool, volumeInfo, templatePoolRef, templateId);
300306

301307
CloudStackVolume cloned = storageStrategy.cloneCloudStackVolume(request);
302-
if (cloned == null || (iscsi && (cloned.getLun() == null || cloned.getLun().getName() == null))) {
308+
// SAN cloneCloudStackVolume validates the Feign response (LUN name + uuid) before returning
309+
if (cloned == null) {
303310
throw new CloudRuntimeException("ONTAP returned nothing when cloning template [" + templateId
304311
+ "] for volume [" + volumeInfo.getId() + "]");
305312
}
@@ -366,31 +373,60 @@ private void deleteTemplateOnPrimary(DataStore store, TemplateInfo templateInfo)
366373

367374
StorageStrategy storageStrategy = OntapStorageUtils.getStrategyByStoragePoolDetails(details);
368375
if (isIscsi(details)) {
369-
deleteIscsiTemplateCache(storagePool, templateInfo, templatePoolRef, storageStrategy);
376+
deleteIscsiTemplateCache(storagePool, templateInfo, templatePoolRef, storageStrategy, details);
370377
} else {
371378
deleteNfsTemplateCache(details, templateInfo, templatePoolRef, storageStrategy);
372379
}
373380
}
374381

375382
private void deleteIscsiTemplateCache(StoragePoolVO storagePool, TemplateInfo templateInfo,
376-
VMTemplateStoragePoolVO templatePoolRef, StorageStrategy storageStrategy) {
383+
VMTemplateStoragePoolVO templatePoolRef, StorageStrategy storageStrategy,
384+
Map<String, String> details) {
385+
String lunName = getTemplateLunName(storagePool, templateInfo.getId());
377386
String lunUuid = templatePoolRef.getLocalDownloadPath();
378-
if (lunUuid == null || lunUuid.isEmpty()) {
379-
logger.warn("deleteTemplateOnPrimary: No cached LUN recorded for template [{}] on pool [{}]; nothing to delete",
380-
templateInfo.getId(), storagePool.getId());
381-
return;
387+
deleteTemplateCacheLun(storageStrategy, details.get(OntapStorageConstants.SVM_NAME), lunName, lunUuid);
388+
logger.info("deleteTemplateOnPrimary: Deleted template cache LUN [{}] for template [{}] on pool [{}]",
389+
lunName, templateInfo.getId(), storagePool.getId());
390+
}
391+
392+
/**
393+
* Deletes a template-cache LUN. When {@code lunUuid} is missing, resolves the LUN by its
394+
* deterministic name so eviction and create-time rollback still work.
395+
*/
396+
private void deleteTemplateCacheLun(StorageStrategy storageStrategy, String svmName, String lunName, String lunUuid) {
397+
String uuid = lunUuid;
398+
if (uuid == null || uuid.isEmpty()) {
399+
CloudStackVolume existing = getCloudStackVolumeByName(storageStrategy, svmName, lunName);
400+
if (existing == null || existing.getLun() == null || existing.getLun().getUuid() == null) {
401+
logger.warn("deleteTemplateCacheLun: LUN [{}] not found on SVM [{}]; nothing to delete", lunName, svmName);
402+
return;
403+
}
404+
uuid = existing.getLun().getUuid();
382405
}
383406

384407
Lun lun = new Lun();
385-
lun.setUuid(lunUuid);
386-
lun.setName(getTemplateLunName(storagePool, templateInfo.getId()));
408+
lun.setUuid(uuid);
409+
lun.setName(lunName);
387410

388411
CloudStackVolume deleteRequest = new CloudStackVolume();
389412
deleteRequest.setLun(lun);
390413
storageStrategy.deleteCloudStackVolume(deleteRequest);
414+
}
391415

392-
logger.info("deleteTemplateOnPrimary: Deleted template cache LUN [{}] for template [{}] on pool [{}]",
393-
lun.getName(), templateInfo.getId(), storagePool.getId());
416+
/**
417+
* Best-effort cleanup of a template-cache LUN after a failed create. Never masks the original
418+
* create failure.
419+
*/
420+
private void bestEffortDeleteTemplateCacheLun(StorageStrategy storageStrategy, String svmName, String lunName,
421+
String lunUuid) {
422+
try {
423+
deleteTemplateCacheLun(storageStrategy, svmName, lunName, lunUuid);
424+
logger.info("bestEffortDeleteTemplateCacheLun: Removed leftover template cache LUN [{}] on SVM [{}]",
425+
lunName, svmName);
426+
} catch (Exception cleanupEx) {
427+
logger.warn("bestEffortDeleteTemplateCacheLun: Failed to remove leftover template cache LUN [{}] on SVM [{}]: {}",
428+
lunName, svmName, cleanupEx.getMessage());
429+
}
394430
}
395431

396432
private void deleteNfsTemplateCache(Map<String, String> details, TemplateInfo templateInfo,
@@ -683,15 +719,30 @@ private void grantAccessTemplate(TemplateInfo templateInfo, Host host, DataStore
683719

684720
ensureAccessGroupForHost(sanStrategy, host, storagePool, svmName, accessGroupName);
685721

686-
String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
687-
String iscsiPath = buildIscsiPath(storagePool, lunNumber);
688-
689-
templatePoolRef.setInstallPath(iscsiPath);
690-
vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
691-
refreshManagedStoreTarget(dataStore, iscsiPath);
692-
693-
logger.info("grantAccessTemplate: Mapped template cache LUN [{}] to igroup [{}] as [{}] for template [{}]",
694-
lunName, accessGroupName, iscsiPath, templateInfo.getId());
722+
boolean lunMapped = false;
723+
try {
724+
String lunNumber = sanStrategy.ensureLunMapped(svmName, lunName, accessGroupName);
725+
lunMapped = true;
726+
String iscsiPath = buildIscsiPath(storagePool, lunNumber);
727+
728+
templatePoolRef.setInstallPath(iscsiPath);
729+
vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
730+
refreshManagedStoreTarget(dataStore, iscsiPath);
731+
732+
logger.info("grantAccessTemplate: Mapped template cache LUN [{}] to igroup [{}] as [{}] for template [{}]",
733+
lunName, accessGroupName, iscsiPath, templateInfo.getId());
734+
} catch (RuntimeException e) {
735+
// Undo only this call's side effect (map). Do not delete the cache LUN — create already succeeded.
736+
if (lunMapped) {
737+
try {
738+
unmapLunFromHost(sanStrategy, svmName, lunName, accessGroupName, host);
739+
} catch (Exception rollbackEx) {
740+
logger.warn("grantAccessTemplate: Failed to unmap template cache LUN [{}] from igroup [{}] after grant failure: {}",
741+
lunName, accessGroupName, rollbackEx.getMessage());
742+
}
743+
}
744+
throw e;
745+
}
695746
}
696747

697748
/**

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume
7979
throw new CloudRuntimeException("Failed to create Lun: " + cloudstackVolume.getLun().getName());
8080
}
8181
Lun lun = createdLun.getRecords().get(0);
82+
validateCreatedLun(lun, cloudstackVolume.getLun().getName(), "createCloudStackVolume");
8283
logger.debug("createCloudStackVolume: LUN created successfully. Lun: {}", lun);
8384

8485
CloudStackVolume createdCloudStackVolume = new CloudStackVolume();
@@ -88,6 +89,8 @@ public CloudStackVolume createCloudStackVolume(CloudStackVolume cloudstackVolume
8889
logger.error("FeignException occurred while creating LUN: {}, Status: {}, Exception: {}",
8990
cloudstackVolume.getLun().getName(), e.status(), e.getMessage());
9091
throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage());
92+
} catch (CloudRuntimeException e) {
93+
throw e;
9194
} catch (Exception e) {
9295
logger.error("Exception occurred while creating LUN: {}, Exception: {}", cloudstackVolume.getLun().getName(), e.getMessage());
9396
throw new CloudRuntimeException("Failed to create Lun: " + e.getMessage());
@@ -154,6 +157,7 @@ public CloudStackVolume cloneCloudStackVolume(CloudStackVolume cloudstackVolume)
154157
throw new CloudRuntimeException("Failed to clone Lun: " + lunRequest.getName());
155158
}
156159
Lun lun = clonedLun.getRecords().get(0);
160+
validateCreatedLun(lun, lunRequest.getName(), "cloneCloudStackVolume");
157161
logger.debug("cloneCloudStackVolume: LUN cloned successfully. Lun: {}", lun);
158162

159163
CloudStackVolume clonedCloudStackVolume = new CloudStackVolume();
@@ -163,12 +167,26 @@ public CloudStackVolume cloneCloudStackVolume(CloudStackVolume cloudstackVolume)
163167
logger.error("FeignException occurred while cloning LUN: {}, Status: {}, Exception: {}",
164168
lunRequest.getName(), e.status(), e.getMessage());
165169
throw new CloudRuntimeException("Failed to clone Lun: " + e.getMessage());
170+
} catch (CloudRuntimeException e) {
171+
throw e;
166172
} catch (Exception e) {
167173
logger.error("Exception occurred while cloning LUN: {}, Exception: {}", lunRequest.getName(), e.getMessage());
168174
throw new CloudRuntimeException("Failed to clone Lun: " + e.getMessage());
169175
}
170176
}
171177

178+
179+
/**
180+
* Ensures ONTAP returned a usable LUN identity from create/clone. Callers in the datastore
181+
* driver rely on non-null name and uuid, so reject incomplete records at the Feign boundary.
182+
*/
183+
private void validateCreatedLun(Lun lun, String requestName, String operation) {
184+
if (lun == null || lun.getName() == null || lun.getUuid() == null) {
185+
logger.error("{}: ONTAP returned incomplete LUN for {}", operation, requestName);
186+
throw new CloudRuntimeException("ONTAP returned incomplete LUN for: " + requestName);
187+
}
188+
}
189+
172190
/**
173191
* Grows an existing LUN to {@code sizeInBytes}.
174192
*

0 commit comments

Comments
 (0)