Copilot commented on code in PR #14243:
URL: https://github.com/apache/cloudstack/pull/14243#discussion_r4219654798
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java:
##########
@@ -203,8 +218,217 @@ public void createAsync(DataStore dataStore, DataObject
dataObject, AsyncComplet
*/
private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool,
VolumeInfo volumeObject, Map<String, String> details) {
StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
- CloudStackVolume cloudStackVolumeRequest =
OntapStorageUtils.createCloudStackVolumeRequestByProtocol(storagePool, details,
volumeObject);
- return storageStrategy.createCloudStackVolume(cloudStackVolumeRequest);
+ return
storageStrategy.createCloudStackVolume(createVolumeRequest(storagePool,
details, volumeObject));
+ }
+
+ /**
+ * Creates the backend object that caches a template on this pool's
FlexVolume.
+ *
+ * <p>Protocol-specific work is delegated to {@link
StorageStrategy#createTemplateCache}.
+ * This method maps the result to {@link CreateCmdResult} and records SAN
identity on
+ * {@code template_spool_ref} ({@code local_download_path} = LUN uuid).</p>
+ */
+ private CreateCmdResult createTemplateOnPrimary(StoragePoolVO storagePool,
TemplateInfo templateInfo, Map<String, String> details) {
+ StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
+ long sizeInBytes =
getDataObjectSizeIncludingHypervisorSnapshotReserve(templateInfo, storagePool);
+ CloudStackVolume created = null;
+ try {
+ created = storageStrategy.createTemplateCache(storagePool,
templateInfo, details, sizeInBytes);
+ String path = recordTemplateCacheOnSpoolRef(storagePool,
templateInfo, created, sizeInBytes);
+ return new CreateCmdResult(path, new Answer(null, true, null));
+ } catch (Exception e) {
+ // Compensating delete for SAN: strategy cleans create-time LUN
failures; this covers
+ // post-create failures (e.g. template_spool_ref update) after a
LUN was returned.
+ if (created != null && created.getLun() != null) {
+ bestEffortDeleteTemplateCacheLun(storageStrategy,
details.get(OntapStorageConstants.SVM_NAME),
+ created.getLun().getName(),
created.getLun().getUuid());
+ }
+ if (e instanceof CloudRuntimeException) {
+ throw (CloudRuntimeException) e;
+ }
+ throw new CloudRuntimeException("Failed to create template cache
for template [" + templateInfo.getId()
+ + "]: " + e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Persists SAN cache identity on {@code template_spool_ref} when a LUN
was created.
+ * NFS returns the template uuid as the create path; {@code install_path}
is filled later.
+ */
+ private String recordTemplateCacheOnSpoolRef(StoragePoolVO storagePool,
TemplateInfo templateInfo,
+ CloudStackVolume created,
long sizeInBytes) {
+ if (created == null || created.getLun() == null) {
+ return templateInfo.getUuid();
+ }
+ Lun lun = created.getLun();
+ VMTemplateStoragePoolVO templatePoolRef =
findTemplatePoolRef(storagePool.getId(), templateInfo.getId());
+ templatePoolRef.setLocalDownloadPath(lun.getUuid());
+ templatePoolRef.setTemplateSize(sizeInBytes);
+ vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
+ return lun.getName();
+ }
+
+ /**
+ * Clones the cached template into a new volume on the same FlexVolume.
+ *
+ * <p>Invoked when {@code StorageSystemDataMotionStrategy} has recorded a
+ * {@code cloneOfTemplate} detail on the volume.</p>
+ */
+ private CloudStackVolume cloneCloudStackVolumeFromTemplate(StoragePoolVO
storagePool, VolumeInfo volumeInfo,
+ Map<String,
String> details, long templateId) {
+ VMTemplateStoragePoolVO templatePoolRef =
findTemplatePoolRef(storagePool.getId(), templateId);
+ StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
+ boolean iscsi = isIscsi(details);
+
+ CloudStackVolume request = iscsi
+ ? createCloneLunRequest(storagePool, details, volumeInfo,
templatePoolRef, templateId)
+ : createCloneFileRequest(storagePool, volumeInfo,
templatePoolRef, templateId);
+
+ CloudStackVolume cloned =
storageStrategy.cloneCloudStackVolume(request);
+ // SAN cloneCloudStackVolume validates the Feign response (LUN name +
uuid) before returning
+ if (cloned == null) {
+ throw new CloudRuntimeException("ONTAP returned nothing when
cloning template [" + templateId
+ + "] for volume [" + volumeInfo.getId() + "]");
+ }
+
+ logger.info("cloneCloudStackVolumeFromTemplate: Cloned template [{}]
for volume [{}] on pool [{}]",
+ templateId, volumeInfo.getId(), storagePool.getId());
+
+ long requestedSize =
getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool);
+ if (requestedSize > templatePoolRef.getTemplateSize()) {
+ logger.info("cloneCloudStackVolumeFromTemplate: Growing clone of
template [{}] from {} to {} bytes for volume [{}]",
+ templateId, templatePoolRef.getTemplateSize(),
requestedSize, volumeInfo.getId());
Review Comment:
`templatePoolRef.getTemplateSize()` can be null (or 0) depending on how the
spool ref row was populated, which can cause a NullPointerException and/or
incorrect growth decisions. Guard against null/invalid template sizes (e.g.,
treat as unknown and fail fast with a clear error, or default to
`requestedSize` only if that matches the intended semantics).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -694,4 +732,22 @@ public Boolean jobPollForSuccess(String jobUUID, int
maxRetries, int sleepTimeIn
}
return true;
}
+
+ /**
+ * Polls an ONTAP async job when the API response includes a job reference.
+ *
+ * <p>When no job is returned (synchronous completion), the operation is
treated as
+ * successful after HTTP 2xx.</p>
+ *
+ * @param response ONTAP job response (may be null or without a job)
+ * @param operationName label for logging and error messages
+ */
+ public void pollJobIfPresent(JobResponse response, String operationName) {
+ if (response == null || response.getJob() == null ||
response.getJob().getUuid() == null) {
+ logger.debug("pollJobIfPresent: No async job returned for
operation [{}], continuing without polling",
+ operationName);
+ return;
+ }
+ jobPollForSuccess(response.getJob().getUuid(), 60, 2000);
Review Comment:
`pollJobIfPresent` ignores the boolean return value of `jobPollForSuccess`,
so async job failures can be silently treated as success. Capture the return
value and throw a `CloudRuntimeException` (including `operationName` and job
UUID) when polling reports failure.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java:
##########
@@ -203,8 +218,217 @@ public void createAsync(DataStore dataStore, DataObject
dataObject, AsyncComplet
*/
private CloudStackVolume createCloudStackVolume(StoragePoolVO storagePool,
VolumeInfo volumeObject, Map<String, String> details) {
StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
- CloudStackVolume cloudStackVolumeRequest =
OntapStorageUtils.createCloudStackVolumeRequestByProtocol(storagePool, details,
volumeObject);
- return storageStrategy.createCloudStackVolume(cloudStackVolumeRequest);
+ return
storageStrategy.createCloudStackVolume(createVolumeRequest(storagePool,
details, volumeObject));
+ }
+
+ /**
+ * Creates the backend object that caches a template on this pool's
FlexVolume.
+ *
+ * <p>Protocol-specific work is delegated to {@link
StorageStrategy#createTemplateCache}.
+ * This method maps the result to {@link CreateCmdResult} and records SAN
identity on
+ * {@code template_spool_ref} ({@code local_download_path} = LUN uuid).</p>
+ */
+ private CreateCmdResult createTemplateOnPrimary(StoragePoolVO storagePool,
TemplateInfo templateInfo, Map<String, String> details) {
+ StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
+ long sizeInBytes =
getDataObjectSizeIncludingHypervisorSnapshotReserve(templateInfo, storagePool);
+ CloudStackVolume created = null;
+ try {
+ created = storageStrategy.createTemplateCache(storagePool,
templateInfo, details, sizeInBytes);
+ String path = recordTemplateCacheOnSpoolRef(storagePool,
templateInfo, created, sizeInBytes);
+ return new CreateCmdResult(path, new Answer(null, true, null));
+ } catch (Exception e) {
+ // Compensating delete for SAN: strategy cleans create-time LUN
failures; this covers
+ // post-create failures (e.g. template_spool_ref update) after a
LUN was returned.
+ if (created != null && created.getLun() != null) {
+ bestEffortDeleteTemplateCacheLun(storageStrategy,
details.get(OntapStorageConstants.SVM_NAME),
+ created.getLun().getName(),
created.getLun().getUuid());
+ }
+ if (e instanceof CloudRuntimeException) {
+ throw (CloudRuntimeException) e;
+ }
+ throw new CloudRuntimeException("Failed to create template cache
for template [" + templateInfo.getId()
+ + "]: " + e.getMessage(), e);
+ }
+ }
+
+ /**
+ * Persists SAN cache identity on {@code template_spool_ref} when a LUN
was created.
+ * NFS returns the template uuid as the create path; {@code install_path}
is filled later.
+ */
+ private String recordTemplateCacheOnSpoolRef(StoragePoolVO storagePool,
TemplateInfo templateInfo,
+ CloudStackVolume created,
long sizeInBytes) {
+ if (created == null || created.getLun() == null) {
+ return templateInfo.getUuid();
+ }
+ Lun lun = created.getLun();
+ VMTemplateStoragePoolVO templatePoolRef =
findTemplatePoolRef(storagePool.getId(), templateInfo.getId());
+ templatePoolRef.setLocalDownloadPath(lun.getUuid());
+ templatePoolRef.setTemplateSize(sizeInBytes);
+ vmTemplatePoolDao.update(templatePoolRef.getId(), templatePoolRef);
+ return lun.getName();
+ }
+
+ /**
+ * Clones the cached template into a new volume on the same FlexVolume.
+ *
+ * <p>Invoked when {@code StorageSystemDataMotionStrategy} has recorded a
+ * {@code cloneOfTemplate} detail on the volume.</p>
+ */
+ private CloudStackVolume cloneCloudStackVolumeFromTemplate(StoragePoolVO
storagePool, VolumeInfo volumeInfo,
+ Map<String,
String> details, long templateId) {
+ VMTemplateStoragePoolVO templatePoolRef =
findTemplatePoolRef(storagePool.getId(), templateId);
+ StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
+ boolean iscsi = isIscsi(details);
+
+ CloudStackVolume request = iscsi
+ ? createCloneLunRequest(storagePool, details, volumeInfo,
templatePoolRef, templateId)
+ : createCloneFileRequest(storagePool, volumeInfo,
templatePoolRef, templateId);
+
+ CloudStackVolume cloned =
storageStrategy.cloneCloudStackVolume(request);
+ // SAN cloneCloudStackVolume validates the Feign response (LUN name +
uuid) before returning
+ if (cloned == null) {
+ throw new CloudRuntimeException("ONTAP returned nothing when
cloning template [" + templateId
+ + "] for volume [" + volumeInfo.getId() + "]");
+ }
+
+ logger.info("cloneCloudStackVolumeFromTemplate: Cloned template [{}]
for volume [{}] on pool [{}]",
+ templateId, volumeInfo.getId(), storagePool.getId());
+
+ long requestedSize =
getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeInfo, storagePool);
+ if (requestedSize > templatePoolRef.getTemplateSize()) {
+ logger.info("cloneCloudStackVolumeFromTemplate: Growing clone of
template [{}] from {} to {} bytes for volume [{}]",
+ templateId, templatePoolRef.getTemplateSize(),
requestedSize, volumeInfo.getId());
+ storageStrategy.resizeCloudStackVolume(cloned, requestedSize);
+ }
+
+ return cloned;
+ }
+
+ /**
+ * Returns the CloudStack template id the volume should be cloned from, or
null for a blank volume.
+ *
+ * <p>{@code StorageSystemDataMotionStrategy} persists this detail
immediately before calling
+ * {@code createAsync} and removes it right after, so it is only visible
during creation.</p>
+ */
+ private Long getTemplateIdForCloning(long volumeId) {
+ VolumeDetailVO detail = volumeDetailsDao.findDetail(volumeId,
OntapStorageConstants.CLONE_OF_TEMPLATE);
+ if (detail == null || detail.getValue() == null ||
detail.getValue().isEmpty()) {
+ return null;
+ }
+ return Long.valueOf(detail.getValue());
+ }
+
+ private VMTemplateStoragePoolVO findTemplatePoolRef(long poolId, long
templateId) {
+ VMTemplateStoragePoolVO templatePoolRef =
vmTemplatePoolDao.findByPoolTemplate(poolId, templateId, null);
+ if (templatePoolRef == null) {
+ throw new CloudRuntimeException("No template_spool_ref row for
template [" + templateId + "] on pool [" + poolId + "]");
+ }
+ return templatePoolRef;
+ }
+
+ /**
+ * Deletes the LUN caching a template on this pool, invoked by template
eviction
+ * ({@code TemplateManagerImpl.evictTemplateFromStoragePool}).
+ *
+ * <p>Volumes previously cloned from this LUN are unaffected: an ONTAP
sis-clone shares blocks
+ * with its source through reference counting rather than depending on it,
so the source can be
+ * removed while its clones stay online.</p>
+ *
+ * <p>{@code deleteCloudStackVolume} already unmaps as it deletes ({@code
allow_delete_while_mapped})
+ * and treats a missing LUN as success.</p>
+ */
+ private void deleteTemplateOnPrimary(DataStore store, TemplateInfo
templateInfo) {
+ StoragePoolVO storagePool = storagePoolDao.findById(store.getId());
+ if (storagePool == null) {
+ throw new CloudRuntimeException("Storage Pool not found for id: "
+ store.getId());
+ }
+
+ Map<String, String> details =
storagePoolDetailsDao.listDetailsKeyPairs(store.getId());
+ VMTemplateStoragePoolVO templatePoolRef =
vmTemplatePoolDao.findByPoolTemplate(storagePool.getId(), templateInfo.getId(),
null);
+ if (templatePoolRef == null) {
+ logger.warn("deleteTemplateOnPrimary: No template_spool_ref for
template [{}] on pool [{}]; nothing to delete",
+ templateInfo.getId(), storagePool.getId());
+ return;
+ }
+
+ StorageStrategy storageStrategy =
OntapStorageUtils.getStrategyByStoragePoolDetails(details);
+ if (isIscsi(details)) {
+ deleteIscsiTemplateCache(storagePool, templateInfo,
templatePoolRef, storageStrategy, details);
+ } else {
+ deleteNfsTemplateCache(details, templateInfo, templatePoolRef,
storageStrategy);
+ }
+ }
+
+ private void deleteIscsiTemplateCache(StoragePoolVO storagePool,
TemplateInfo templateInfo,
+ VMTemplateStoragePoolVO
templatePoolRef, StorageStrategy storageStrategy,
+ Map<String, String> details) {
+ String lunName = getTemplateLunName(storagePool, templateInfo.getId());
+ String lunUuid = templatePoolRef.getLocalDownloadPath();
+ deleteTemplateCacheLun(storageStrategy,
details.get(OntapStorageConstants.SVM_NAME), lunName, lunUuid);
+ logger.info("deleteIscsiTemplateCache: Deleted template cache LUN [{}]
for template [{}] on pool [{}]",
+ lunName, templateInfo.getId(), storagePool.getId());
+ }
+
+ /**
+ * Deletes a template-cache LUN. When {@code lunUuid} is missing, resolves
the LUN by its
+ * deterministic name so eviction and create-time rollback still work.
+ */
+ private void deleteTemplateCacheLun(StorageStrategy storageStrategy,
String svmName, String lunName, String lunUuid) {
+ String uuid = lunUuid;
+ if (uuid == null || uuid.isEmpty()) {
+ CloudStackVolume existing =
getCloudStackVolumeByName(storageStrategy, svmName, lunName);
+ if (existing == null || existing.getLun() == null ||
existing.getLun().getUuid() == null) {
+ logger.warn("deleteTemplateCacheLun: LUN [{}] not found on SVM
[{}]; nothing to delete", lunName, svmName);
+ return;
+ }
+ uuid = existing.getLun().getUuid();
+ }
+
+ Lun lun = new Lun();
+ lun.setUuid(uuid);
+ lun.setName(lunName);
+
+ CloudStackVolume deleteRequest = new CloudStackVolume();
+ deleteRequest.setLun(lun);
+ storageStrategy.deleteCloudStackVolume(deleteRequest);
+ }
+
+ /**
+ * Best-effort cleanup of a template-cache LUN after a failed create.
Never masks the original
+ * create failure.
+ */
+ private void bestEffortDeleteTemplateCacheLun(StorageStrategy
storageStrategy, String svmName, String lunName,
+ String lunUuid) {
+ try {
+ deleteTemplateCacheLun(storageStrategy, svmName, lunName, lunUuid);
+ logger.info("bestEffortDeleteTemplateCacheLun: Removed leftover
template cache LUN [{}] on SVM [{}]",
+ lunName, svmName);
+ } catch (Exception cleanupEx) {
+ logger.warn("bestEffortDeleteTemplateCacheLun: Failed to remove
leftover template cache LUN [{}] on SVM [{}]: {}",
+ lunName, svmName, cleanupEx.getMessage());
+ }
+ }
+
+ private void deleteNfsTemplateCache(Map<String, String> details,
TemplateInfo templateInfo,
+ VMTemplateStoragePoolVO
templatePoolRef, StorageStrategy storageStrategy) {
+ String filePath = templatePoolRef.getInstallPath();
+ if (filePath == null || filePath.isEmpty()) {
+ logger.warn("deleteNfsTemplateCache: No install_path recorded for
template [{}]; nothing to delete",
+ templateInfo.getId());
+ return;
+ }
+ String flexVolUuid = details.get(OntapStorageConstants.VOLUME_UUID);
+ if (flexVolUuid == null || flexVolUuid.isEmpty()) {
+ // Misconfigured pool detail — fail eviction rather than calling
ONTAP with a null
+ // volume UUID (which would hit /api/storage/volumes/null and
still look like success
+ // upstream if we swallowed the error).
+ throw new CloudRuntimeException("FlexVolume UUID (volumeUUID) is
missing from storage pool details; "
+ + "cannot delete NFS template cache file [" + filePath +
"] for template ["
+ + templateInfo.getId() + "]");
+ }
+ ((UnifiedNASStrategy) storageStrategy).deleteFileByPath(flexVolUuid,
filePath);
Review Comment:
This hard cast to `UnifiedNASStrategy` can trigger a `ClassCastException` if
`OntapStorageUtils.getStrategyByStoragePoolDetails(...)` ever returns a
different NAS strategy implementation. Prefer moving `deleteFileByPath` onto
the `StorageStrategy` interface (or at least guard with `instanceof` and throw
a targeted `CloudRuntimeException` explaining the unexpected strategy type).
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -112,9 +131,103 @@ public void deleteCloudStackVolume(CloudStackVolume
cloudstackVolume) {
}
}
+ /**
+ * Clones a file inside the FlexVolume using ONTAP's file clone API.
+ *
+ * <p>The source is taken from {@code file.path} and the destination from
+ * {@code destinationPath}, both relative to the root of the FlexVolume
backing the pool.</p>
+ */
+ @Override
+ public CloudStackVolume cloneCloudStackVolume(CloudStackVolume
cloudstackVolume) {
+ if (cloudstackVolume == null || cloudstackVolume.getFile() == null
+ || cloudstackVolume.getFile().getPath() == null ||
cloudstackVolume.getDestinationPath() == null) {
+ logger.error("cloneCloudStackVolume: File clone failed. Invalid
request: {}", cloudstackVolume);
+ throw new CloudRuntimeException("Failed to clone file, invalid
request");
+ }
+ if (cloudstackVolume.getDatastoreId() == null) {
+ throw new CloudRuntimeException("Failed to clone file, no
datastore id in the request");
+ }
+
+ Map<String, String> details =
storagePoolDetailsDao.listDetailsKeyPairs(Long.parseLong(cloudstackVolume.getDatastoreId()));
+ String flexVolUuid = details.get(OntapStorageConstants.VOLUME_UUID);
+ String flexVolName = details.get(OntapStorageConstants.VOLUME_NAME);
+ if (flexVolUuid == null || flexVolUuid.isEmpty()) {
+ throw new CloudRuntimeException("Failed to clone file, FlexVolume
uuid is missing from pool details");
+ }
+ String sourcePath = cloudstackVolume.getFile().getPath();
+ String destinationPath = cloudstackVolume.getDestinationPath();
+
+ logger.info("cloneCloudStackVolume: Cloning file [{}] to [{}] in
FlexVol [{}]", sourcePath, destinationPath, flexVolName);
+ try {
+ FileCloneRequest request = new FileCloneRequest(flexVolUuid,
flexVolName, sourcePath, destinationPath);
+ JobResponse jobResponse =
nasFeignClient.cloneFile(getAuthHeader(), request);
+ pollJobIfPresent(jobResponse, "clone file [" + sourcePath + "] to
[" + destinationPath + "]");
+
+ updateCloudStackVolumeMetadata(cloudstackVolume.getDatastoreId(),
cloudstackVolume.getVolumeInfo());
Review Comment:
`updateCloudStackVolumeMetadata(...)` is called without verifying
`cloudstackVolume.getVolumeInfo()` is present. If this method is invoked with a
request that omits `volumeInfo`, it can throw unexpectedly after a successful
clone. Either validate `volumeInfo` upfront (and fail early with a clear
message) or make metadata update conditional when `volumeInfo` is null.
--
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]
