Copilot commented on code in PR #11298:
URL: https://github.com/apache/cloudstack/pull/11298#discussion_r2232753228
##########
plugins/storage/volume/primera/src/main/java/org/apache/cloudstack/storage/datastore/adapter/primera/PrimeraAdapter.java:
##########
@@ -288,39 +288,94 @@ public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject req
@Override
public ProviderVolume copy(ProviderAdapterContext context,
ProviderAdapterDataObject sourceVolumeInfo,
- ProviderAdapterDataObject targetVolumeInfo) {
+ ProviderAdapterDataObject targetVolumeInfo, Long newSize) {
+ // Log the start of the copy operation with source volume details
+ logger.debug("PrimeraAdapter: Starting volume copy operation - source
volume: '{}', target volume: '{}', requested new size: {} bytes ({} MiB)",
+ sourceVolumeInfo.getExternalName(),
targetVolumeInfo.getName(), newSize, newSize / PrimeraAdapter.BYTES_IN_MiB);
+
+ // Flag to determine copy method: online copy (direct clone) vs
offline copy (with resize)
+ boolean onlineCopy = true;
PrimeraVolumeCopyRequest request = new PrimeraVolumeCopyRequest();
PrimeraVolumeCopyRequestParameters parms = new
PrimeraVolumeCopyRequestParameters();
assert sourceVolumeInfo.getExternalName() != null: "External provider
name not provided on copy request to Primera volume provider";
- // if we have no external name, treat it as a new volume
+ // Generate external name for target volume if not already set
if (targetVolumeInfo.getExternalName() == null) {
targetVolumeInfo.setExternalName(ProviderVolumeNamer.generateObjectName(context,
targetVolumeInfo));
+ logger.debug("PrimeraAdapter: Generated external name '{}' for
target volume", targetVolumeInfo.getExternalName());
}
ProviderVolume sourceVolume = this.getVolume(context,
sourceVolumeInfo);
if (sourceVolume == null) {
throw new RuntimeException("Source volume " +
sourceVolumeInfo.getExternalUuid() + " with provider name " +
sourceVolumeInfo.getExternalName() + " not found on storage provider");
}
+ // Determine copy method based on size difference
+ // Online copy: Direct clone without size change (faster, immediate)
+ // Offline copy: Copy with potential resize (slower, requires task
completion wait)
+ Long sourceSize = sourceVolume.getAllocatedSizeInBytes();
+ if (newSize == null || sourceSize == null ||
!newSize.equals(sourceSize)) {
+ logger.debug("PrimeraAdapter: Volume size change detected (source:
{} bytes, target: {} bytes) - using offline copy method",
+ sourceSize, newSize);
+ onlineCopy = false;
+ } else {
+ logger.debug("PrimeraAdapter: No size change required (both {}
bytes) - using online copy method for faster cloning", newSize);
+ }
+
+ // Check if target volume already exists on the storage provider
ProviderVolume targetVolume = this.getVolume(context,
targetVolumeInfo);
if (targetVolume == null) {
- this.create(context, targetVolumeInfo, null,
sourceVolume.getAllocatedSizeInBytes());
+ if (!onlineCopy) {
+ // For offline copy, pre-create the target volume with the
desired size
+ logger.debug("PrimeraAdapter: Offline copy mode - pre-creating
target volume '{}' with size {} bytes",
+ targetVolumeInfo.getName(),
sourceVolume.getAllocatedSizeInBytes());
+ this.create(context, targetVolumeInfo, null,
sourceVolume.getAllocatedSizeInBytes());
Review Comment:
For offline copy mode, the target volume is being created with the source
volume size instead of the newSize parameter. This contradicts the logic that
offline copy is used when sizes differ, and the target should be created with
the desired newSize.
```suggestion
targetVolumeInfo.getName(), newSize);
this.create(context, targetVolumeInfo, null, newSize);
```
##########
plugins/storage/volume/primera/src/main/java/org/apache/cloudstack/storage/datastore/adapter/primera/PrimeraAdapter.java:
##########
@@ -288,39 +288,94 @@ public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject req
@Override
public ProviderVolume copy(ProviderAdapterContext context,
ProviderAdapterDataObject sourceVolumeInfo,
- ProviderAdapterDataObject targetVolumeInfo) {
+ ProviderAdapterDataObject targetVolumeInfo, Long newSize) {
+ // Log the start of the copy operation with source volume details
+ logger.debug("PrimeraAdapter: Starting volume copy operation - source
volume: '{}', target volume: '{}', requested new size: {} bytes ({} MiB)",
+ sourceVolumeInfo.getExternalName(),
targetVolumeInfo.getName(), newSize, newSize / PrimeraAdapter.BYTES_IN_MiB);
+
+ // Flag to determine copy method: online copy (direct clone) vs
offline copy (with resize)
+ boolean onlineCopy = true;
PrimeraVolumeCopyRequest request = new PrimeraVolumeCopyRequest();
PrimeraVolumeCopyRequestParameters parms = new
PrimeraVolumeCopyRequestParameters();
assert sourceVolumeInfo.getExternalName() != null: "External provider
name not provided on copy request to Primera volume provider";
- // if we have no external name, treat it as a new volume
+ // Generate external name for target volume if not already set
if (targetVolumeInfo.getExternalName() == null) {
targetVolumeInfo.setExternalName(ProviderVolumeNamer.generateObjectName(context,
targetVolumeInfo));
+ logger.debug("PrimeraAdapter: Generated external name '{}' for
target volume", targetVolumeInfo.getExternalName());
}
ProviderVolume sourceVolume = this.getVolume(context,
sourceVolumeInfo);
if (sourceVolume == null) {
throw new RuntimeException("Source volume " +
sourceVolumeInfo.getExternalUuid() + " with provider name " +
sourceVolumeInfo.getExternalName() + " not found on storage provider");
}
+ // Determine copy method based on size difference
+ // Online copy: Direct clone without size change (faster, immediate)
+ // Offline copy: Copy with potential resize (slower, requires task
completion wait)
+ Long sourceSize = sourceVolume.getAllocatedSizeInBytes();
+ if (newSize == null || sourceSize == null ||
!newSize.equals(sourceSize)) {
+ logger.debug("PrimeraAdapter: Volume size change detected (source:
{} bytes, target: {} bytes) - using offline copy method",
+ sourceSize, newSize);
+ onlineCopy = false;
+ } else {
+ logger.debug("PrimeraAdapter: No size change required (both {}
bytes) - using online copy method for faster cloning", newSize);
+ }
+
+ // Check if target volume already exists on the storage provider
ProviderVolume targetVolume = this.getVolume(context,
targetVolumeInfo);
if (targetVolume == null) {
- this.create(context, targetVolumeInfo, null,
sourceVolume.getAllocatedSizeInBytes());
+ if (!onlineCopy) {
+ // For offline copy, pre-create the target volume with the
desired size
+ logger.debug("PrimeraAdapter: Offline copy mode - pre-creating
target volume '{}' with size {} bytes",
+ targetVolumeInfo.getName(),
sourceVolume.getAllocatedSizeInBytes());
+ this.create(context, targetVolumeInfo, null,
sourceVolume.getAllocatedSizeInBytes());
+ } else {
+ // For online copy, the target volume will be created
automatically during the clone operation
+ logger.debug("PrimeraAdapter: Online copy mode - target volume
'{}' will be created automatically during clone operation",
+ targetVolumeInfo.getName());
+ }
+ } else {
+ logger.warn("PrimeraAdapter: Target volume '{}' already exists on
storage provider - proceeding with copy operation",
+ targetVolumeInfo.getExternalName());
}
parms.setDestVolume(targetVolumeInfo.getExternalName());
- parms.setOnline(false);
- parms.setPriority(1);
+ if (onlineCopy) {
+ // Online copy configuration: immediate clone with deduplication
and compression
+ parms.setOnline(true);
+ parms.setDestCPG(cpg);
+ parms.setTpvv(false);
+ parms.setReduce(true);
+ logger.debug("PrimeraAdapter: Configuring online copy -
destination CPG: '{}', deduplication enabled, thin provisioning disabled", cpg);
+ } else {
+ // Offline copy configuration: background task with high priority
+ parms.setOnline(false);
+ parms.setPriority(1); // Set high priority for faster completion
+ logger.debug("PrimeraAdapter: Configuring offline copy with high
priority for target volume '{}'", targetVolumeInfo.getName());
+ }
+
+ // Set request parameters and initiate the copy operation
request.setParameters(parms);
PrimeraTaskReference taskref = POST("/volumes/" +
sourceVolumeInfo.getExternalName(), request, new
TypeReference<PrimeraTaskReference>() {});
if (taskref == null) {
+ logger.error("PrimeraAdapter: Failed to initiate copy operation -
no task reference returned from storage provider");
throw new RuntimeException("Unable to retrieve task used to copy
to newly created volume");
}
- waitForTaskToComplete(taskref.getTaskid(), "copy volume " +
sourceVolumeInfo.getExternalName() + " to " +
- targetVolumeInfo.getExternalName(), taskWaitTimeoutMs);
+ // Handle task completion based on copy method
+ if (!onlineCopy) {
+ // Offline copy requires waiting for task completion
+ logger.debug("PrimeraAdapter: Offline copy initiated - waiting for
task completion (TaskID: {})", taskref.getTaskid());
+ waitForTaskToComplete(taskref.getTaskid(), "copy volume " +
sourceVolumeInfo.getExternalName() + " to " +
+ targetVolumeInfo.getExternalName(), taskWaitTimeoutMs);
+ logger.debug("PrimeraAdapter: Offline copy operation completed
successfully");
+ } else {
Review Comment:
[nitpick] The waitForTaskToComplete call is only executed for offline copy,
but it should be within the offline copy conditional block for better code
organization and readability.
```suggestion
if (onlineCopy) {
```
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -367,7 +367,8 @@ public ProviderSnapshot getSnapshot(ProviderAdapterContext
context, ProviderAdap
@Override
public ProviderVolume copy(ProviderAdapterContext context,
ProviderAdapterDataObject sourceDataObject,
- ProviderAdapterDataObject destDataObject) {
+ ProviderAdapterDataObject destDataObject, Long newSize) {
+ // Add new parameter as newSize to match method declaration but not
used anywhere
Review Comment:
[nitpick] The comment indicates the newSize parameter is not implemented
yet. Consider adding a TODO comment or proper documentation about the planned
implementation to make the intention clearer for future development.
```suggestion
// TODO: The newSize parameter is currently unused. It may be
implemented in the future
// to allow resizing the destination volume during the copy
operation.
```
--
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: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
