Copilot commented on code in PR #14243:
URL: https://github.com/apache/cloudstack/pull/14243#discussion_r4325771484
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/StorageStrategy.java:
##########
@@ -495,6 +498,27 @@ public String getNetworkInterface() {
*/
abstract public CloudStackVolume createCloudStackVolume(CloudStackVolume
cloudstackVolume);
+ /**
+ * Creates the protocol-specific backend object that caches a template on
this pool.
+ *
+ * <p>iSCSI creates an empty LUN ({@code
/vol/<flexVol>/cs_tmpl_<id>}) sized to
+ * {@code sizeInBytes}. NFS is a no-op on the array: the KVM agent later
writes the qcow2
+ * into the mounted FlexVolume.</p>
+ *
+ * <p>Returns a {@link CloudStackVolume} so the driver can map it to
{@code CreateCmdResult}
+ * and update {@code template_spool_ref}. SAN populates {@code lun}; NAS
returns an empty
+ * volume (no LUN / file yet).</p>
+ *
+ * @param storagePool CloudStack primary storage pool (one FlexVolume)
+ * @param templateInfo template being cached
+ * @param details pool details (SVM, protocol, etc.)
+ * @param sizeInBytes virtual size for the cache object (required for
SAN; ignored for NAS)
+ * @return created cache identity, or an empty {@link CloudStackVolume}
when nothing is
+ * pre-created on the array
+ */
+ abstract public CloudStackVolume createTemplateCache(StoragePoolVO
storagePool, TemplateInfo templateInfo,
+ Map<String, String> details, long sizeInBytes);
Review Comment:
The Javadoc states “NAS returns an empty volume” / “return … an empty
CloudStackVolume”, but `UnifiedNASStrategy.createTemplateCache(...)` currently
returns `null`. Please align the contract and implementations (either document
that `null` is returned for NAS/no-op, or change NAS to return an empty
`CloudStackVolume`) so callers don’t have to guess.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedSANStrategy.java:
##########
@@ -87,12 +91,97 @@ public CloudStackVolume
createCloudStackVolume(CloudStackVolume cloudstackVolume
logger.error("FeignException occurred while creating LUN: {},
Status: {}, Exception: {}",
cloudstackVolume.getLun().getName(), e.status(),
e.getMessage());
throw new CloudRuntimeException("Failed to create Lun: " +
e.getMessage());
+ } catch (CloudRuntimeException e) {
+ throw e;
} catch (Exception e) {
logger.error("Exception occurred while creating LUN: {},
Exception: {}", cloudstackVolume.getLun().getName(), e.getMessage());
throw new CloudRuntimeException("Failed to create Lun: " +
e.getMessage());
}
}
+ /**
+ * Creates an empty LUN that caches a template on this pool's FlexVolume.
+ *
+ * <p>Sized to the template virtual disk size (what KVM writes after
{@code qemu-img convert}
+ * to RAW). On create failure, best-effort deletes any leftover LUN before
rethrowing.</p>
+ */
+ @Override
+ public CloudStackVolume createTemplateCache(StoragePoolVO storagePool,
TemplateInfo templateInfo,
+ Map<String, String> details,
long sizeInBytes) {
+ if (sizeInBytes <= 0) {
+ throw new CloudRuntimeException("Unknown virtual size for template
[" + templateInfo.getId()
+ + "]; cannot size the template LUN on pool [" +
storagePool.getId() + "]");
+ }
+
+ CloudStackVolume request = buildTemplateLunRequest(storagePool,
details, templateInfo.getId(), sizeInBytes);
+ String lunName = request.getLun().getName();
+ CloudStackVolume created = null;
+ try {
+ created = createCloudStackVolume(request);
+ Lun lun = created.getLun();
+ logger.info("createTemplateCache: Created template cache LUN [{}]
(uuid [{}], {} bytes) on pool [{}] for template [{}]",
+ lun.getName(), lun.getUuid(), sizeInBytes,
storagePool.getId(), templateInfo.getId());
+ return created;
+ } catch (Exception e) {
+
bestEffortDeleteTemplateCacheLun(details.get(OntapStorageConstants.SVM_NAME),
lunName,
+ created != null && created.getLun() != null ?
created.getLun().getUuid() : null);
+ if (e instanceof CloudRuntimeException) {
+ throw (CloudRuntimeException) e;
+ }
+ throw new CloudRuntimeException("Failed to create template cache
LUN for template [" + templateInfo.getId()
+ + "]: " + e.getMessage(), e);
+ }
+ }
+
+ private CloudStackVolume buildTemplateLunRequest(StoragePoolVO
storagePool, Map<String, String> details,
+ long templateId, long
sizeInBytes) {
+ Svm svm = new Svm();
+ svm.setName(details.get(OntapStorageConstants.SVM_NAME));
+
+ Lun lunRequest = new Lun();
+ lunRequest.setSvm(svm);
+ lunRequest.setName(OntapStorageUtils.getLunName(storagePool.getName(),
+ OntapStorageConstants.TEMPLATE_LUN_PREFIX + templateId));
+ lunRequest.setOsType(Lun.OsTypeEnum.valueOf(
+
OntapStorageUtils.getOSTypeFromHypervisor(storagePool.getHypervisor().name())));
+ LunSpace lunSpace = new LunSpace();
+ lunSpace.setSize(sizeInBytes);
+ lunRequest.setSpace(lunSpace);
+
+ CloudStackVolume request = new CloudStackVolume();
+ request.setLun(lunRequest);
+ return request;
+ }
+
+ private void bestEffortDeleteTemplateCacheLun(String svmName, String
lunName, String lunUuid) {
+ try {
+ String uuid = lunUuid;
+ if (uuid == null || uuid.isEmpty()) {
+ Map<String, String> lookup = Map.of(
+ OntapStorageConstants.NAME, lunName,
+ OntapStorageConstants.SVM_DOT_NAME, svmName);
+ CloudStackVolume existing = getCloudStackVolume(lookup);
+ if (existing == null || existing.getLun() == null ||
existing.getLun().getUuid() == null) {
Review Comment:
`Map.of(...)` throws a `NullPointerException` if `svmName` or `lunName` is
null, which can prevent the intended best-effort cleanup path from even
attempting resolution/deletion. Consider guarding for blank/null
`svmName`/`lunName` before building the map and/or using a mutable `HashMap` so
the cleanup routine can fail gracefully while still logging a clear reason.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/service/UnifiedNASStrategy.java:
##########
@@ -112,9 +131,106 @@ 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 void copyCloudStackVolume(CloudStackVolume cloudstackVolume) {
+ 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");
+ }
+ if (cloudstackVolume.getVolumeInfo() == null) {
+ throw new CloudRuntimeException("Failed to clone file, volume info
is missing from 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");
Review Comment:
`Long.parseLong(cloudstackVolume.getDatastoreId())` can throw
`NumberFormatException`, which currently bypasses the method’s error
handling/logging and will surface as an unwrapped runtime exception. Suggest
validating `datastoreId` (and throwing a `CloudRuntimeException` with datastore
id value included) or moving the parse inside the try/catch so failures are
reported consistently.
##########
plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/driver/OntapPrimaryDatastoreDriver.java:
##########
@@ -203,8 +217,224 @@ 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);
+
+ long templateSize = templatePoolRef.getTemplateSize();
+ if (templateSize <= 0) {
+ throw new CloudRuntimeException("Template cache size is missing or
invalid (" + templateSize
+ + ") for template [" + templateId + "] on pool [" +
storagePool.getId()
+ + "]; cannot clone volume [" + volumeInfo.getId() + "]");
+ }
+
+ 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 > templateSize) {
+ logger.info("cloneCloudStackVolumeFromTemplate: Growing clone of
template [{}] from {} to {} bytes for volume [{}]",
+ templateId, templateSize, 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());
+ }
Review Comment:
`Long.valueOf(detail.getValue())` may throw `NumberFormatException` if the
persisted value is malformed, causing volume creation to fail with a low-signal
exception. Wrapping this in a `CloudRuntimeException` that includes `volumeId`
and the invalid stored value would make failures diagnosable and consistent
with other driver error handling.
--
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]
