shwstppr commented on code in PR #7889:
URL: https://github.com/apache/cloudstack/pull/7889#discussion_r1372986600
##########
server/src/main/java/com/cloud/api/ApiDBUtils.java:
##########
@@ -1278,7 +1278,8 @@ public static HypervisorType
getHypervisorTypeFromFormat(long dcId, ImageFormat
if(pool.getPoolType() == StoragePoolType.RBD ||
pool.getPoolType() == StoragePoolType.PowerFlex ||
pool.getPoolType() == StoragePoolType.CLVM ||
- pool.getPoolType() == StoragePoolType.Linstor) {
+ pool.getPoolType() == StoragePoolType.Linstor ||
+ pool.getPoolType() == StoragePoolType.FiberChannel) {
Review Comment:
Can be,
```
if(List.of(StoragePoolType.RBD, StoragePoolType.PowerFlex,
StoragePoolType.CLVM,
StoragePoolType.Linstor,
StoragePoolType.FiberChannel).contains(pool.getPoolType())) {
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1238,7 +1238,8 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd) throws
ResourceAllocationExcep
if (storagePoolId != null) {
StoragePoolVO storagePoolVO =
_storagePoolDao.findById(storagePoolId);
- if (storagePoolVO.isManaged() &&
!storagePoolVO.getPoolType().equals(Storage.StoragePoolType.PowerFlex)) {
+ if (storagePoolVO.isManaged() &&
!(storagePoolVO.getPoolType().equals(Storage.StoragePoolType.PowerFlex) ||
Review Comment:
```suggestion
if (storagePoolVO.isManaged() &&
!List.of(Storage.StoragePoolType.PowerFlex,
Storage.StoragePoolType.FiberChannel).contains(storagePoolVO.getPoolType())) {
```
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -512,20 +515,30 @@ else if (volumePath == null) {
}
}
+ private void
handleVolumeMigrationFromManagedStorageToManagedStorage(VolumeInfo
srcVolumeInfo, VolumeInfo destVolumeInfo,
+
AsyncCompletionCallback<CopyCommandResult> callback) {
+ if (!HypervisorType.KVM.equals(srcVolumeInfo.getHypervisorType())) {
+ String errMsg = "Currently migrating volumes between managed
storage providers is only supported on KVM hypervisor";
Review Comment:
```suggestion
String errMsg = String.format("Currently migrating volumes
between managed storage providers is not supported on %s hypervisor",
srcVolumeInfo.getHypervisorType().toString());
```
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -826,24 +840,73 @@ private void handleFailedVolumeMigration(VolumeInfo
srcVolumeInfo, VolumeInfo de
_volumeDao.update(srcVolumeInfo.getId(), volumeVO);
}
- private void handleVolumeMigrationForKVM(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo) {
+ private void handleVolumeMigrationForKVM(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo, AsyncCompletionCallback<CopyCommandResult> callback)
{
VirtualMachine vm = srcVolumeInfo.getAttachedVM();
- if (vm != null && vm.getState() != VirtualMachine.State.Stopped) {
- throw new CloudRuntimeException("Currently, if a volume to migrate
from non-managed storage to managed storage on KVM is attached to " +
- "a VM, the VM must be in the Stopped state.");
- }
+ checkAvailableForMigration(vm);
-
destVolumeInfo.getDataStore().getDriver().createAsync(destVolumeInfo.getDataStore(),
destVolumeInfo, null);
+ String errMsg = null;
+ try {
+
destVolumeInfo.getDataStore().getDriver().createAsync(destVolumeInfo.getDataStore(),
destVolumeInfo, null);
+ VolumeVO volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ updatePathFromScsiName(volumeVO);
+ destVolumeInfo =
_volumeDataFactory.getVolume(destVolumeInfo.getId(),
destVolumeInfo.getDataStore());
+ HostVO hostVO =
getHostOnWhichToExecuteMigrationCommand(srcVolumeInfo, destVolumeInfo);
- VolumeVO volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ // migrate the volume via the hypervisor
+ String path = migrateVolumeForKVM(srcVolumeInfo, destVolumeInfo,
hostVO, "Unable to migrate the volume from non-managed storage to managed
storage");
- volumeVO.setPath(volumeVO.get_iScsiName());
+ updateVolumePath(destVolumeInfo.getId(), path);
+ volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ // only set this if it was not set. default to QCOW2 for KVM
+ if (volumeVO.getFormat() == null) {
+ volumeVO.setFormat(ImageFormat.QCOW2);
+ _volumeDao.update(volumeVO.getId(), volumeVO);
+ }
+ } catch (Exception ex) {
+ errMsg = "Primary storage migration failued due to an unexpected
error: " +
+ ex.getMessage();
+ if (ex instanceof CloudRuntimeException) {
+ throw ex;
+ } else {
+ throw new CloudRuntimeException(errMsg, ex);
+ }
+ } finally {
+ CopyCmdAnswer copyCmdAnswer;
+ if (errMsg != null) {
+ copyCmdAnswer = new CopyCmdAnswer(errMsg);
+ }
+ else {
+ destVolumeInfo =
_volumeDataFactory.getVolume(destVolumeInfo.getId(),
destVolumeInfo.getDataStore());
+ DataTO dataTO = destVolumeInfo.getTO();
+ copyCmdAnswer = new CopyCmdAnswer(dataTO);
+ }
- _volumeDao.update(volumeVO.getId(), volumeVO);
+ CopyCommandResult result = new CopyCommandResult(null,
copyCmdAnswer);
+ result.setResult(errMsg);
+ callback.complete(result);
+ }
+ }
- destVolumeInfo = _volumeDataFactory.getVolume(destVolumeInfo.getId(),
destVolumeInfo.getDataStore());
+ private void checkAvailableForMigration(VirtualMachine vm) {
Review Comment:
Similar code exists at line 541, should this method be used there?
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -294,7 +294,8 @@ public Answer copyTemplateToPrimaryStorage(final
CopyCommand cmd) {
newTemplate.setSize(primaryVol.getSize());
if (primaryPool.getType() == StoragePoolType.RBD ||
primaryPool.getType() == StoragePoolType.PowerFlex ||
- primaryPool.getType() == StoragePoolType.Linstor) {
+ primaryPool.getType() == StoragePoolType.Linstor ||
+ primaryPool.getType() == StoragePoolType.FiberChannel) {
Review Comment:
Can be checked using List
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java:
##########
@@ -466,13 +466,17 @@ protected Answer migrateVolumeToPool(DataObject srcData,
DataObject destData) {
s_logger.error(errMsg);
answer = new Answer(command, false, errMsg);
} else {
+ s_logger.info("Sending MIGRATE_COPY request to node " + ep);
Review Comment:
Do we need all logs here with INFO level, they seem like they should be
TRACE or DEBUG
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -826,24 +840,73 @@ private void handleFailedVolumeMigration(VolumeInfo
srcVolumeInfo, VolumeInfo de
_volumeDao.update(srcVolumeInfo.getId(), volumeVO);
}
- private void handleVolumeMigrationForKVM(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo) {
+ private void handleVolumeMigrationForKVM(VolumeInfo srcVolumeInfo,
VolumeInfo destVolumeInfo, AsyncCompletionCallback<CopyCommandResult> callback)
{
VirtualMachine vm = srcVolumeInfo.getAttachedVM();
- if (vm != null && vm.getState() != VirtualMachine.State.Stopped) {
- throw new CloudRuntimeException("Currently, if a volume to migrate
from non-managed storage to managed storage on KVM is attached to " +
- "a VM, the VM must be in the Stopped state.");
- }
+ checkAvailableForMigration(vm);
-
destVolumeInfo.getDataStore().getDriver().createAsync(destVolumeInfo.getDataStore(),
destVolumeInfo, null);
+ String errMsg = null;
+ try {
+
destVolumeInfo.getDataStore().getDriver().createAsync(destVolumeInfo.getDataStore(),
destVolumeInfo, null);
+ VolumeVO volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ updatePathFromScsiName(volumeVO);
+ destVolumeInfo =
_volumeDataFactory.getVolume(destVolumeInfo.getId(),
destVolumeInfo.getDataStore());
+ HostVO hostVO =
getHostOnWhichToExecuteMigrationCommand(srcVolumeInfo, destVolumeInfo);
- VolumeVO volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ // migrate the volume via the hypervisor
+ String path = migrateVolumeForKVM(srcVolumeInfo, destVolumeInfo,
hostVO, "Unable to migrate the volume from non-managed storage to managed
storage");
- volumeVO.setPath(volumeVO.get_iScsiName());
+ updateVolumePath(destVolumeInfo.getId(), path);
+ volumeVO = _volumeDao.findById(destVolumeInfo.getId());
+ // only set this if it was not set. default to QCOW2 for KVM
+ if (volumeVO.getFormat() == null) {
+ volumeVO.setFormat(ImageFormat.QCOW2);
+ _volumeDao.update(volumeVO.getId(), volumeVO);
+ }
+ } catch (Exception ex) {
+ errMsg = "Primary storage migration failued due to an unexpected
error: " +
Review Comment:
```suggestion
errMsg = "Primary storage migration failed due to an unexpected
error: " +
```
##########
server/src/main/java/com/cloud/server/StatsCollector.java:
##########
@@ -1620,7 +1620,11 @@ protected void runInContext() {
for (StoragePoolVO pool : pools) {
List<VolumeVO> volumes =
_volsDao.findByPoolId(pool.getId(), null);
for (VolumeVO volume : volumes) {
- if (volume.getFormat() != ImageFormat.QCOW2 &&
volume.getFormat() != ImageFormat.VHD && volume.getFormat() != ImageFormat.OVA
&& (volume.getFormat() != ImageFormat.RAW || pool.getPoolType() !=
Storage.StoragePoolType.PowerFlex)) {
+ if (volume.getFormat() != ImageFormat.QCOW2 &&
+ volume.getFormat() != ImageFormat.VHD &&
+ volume.getFormat() != ImageFormat.OVA &&
+ pool.getPoolType() !=
Storage.StoragePoolType.PowerFlex &&
+ pool.getPoolType() !=
Storage.StoragePoolType.FiberChannel) {
Review Comment:
Can be simplified,
```
if (!List.of(ImageFormat.QCOW2, ImageFormat.VHD,
ImageFormat.OVA).contains(volume.getFormat()) &&
!List.of(Storage.StoragePoolType.PowerFlex,
Storage.StoragePoolType.FiberChannel).contains(pool.getPoolType())) {
```
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -400,15 +403,15 @@ private void handleCopyAsyncForVolumes(VolumeInfo
srcVolumeInfo, VolumeInfo dest
} else if (!isVolumeOnManagedStorage(destVolumeInfo)) {
handleVolumeMigrationFromManagedStorageToNonManagedStorage(srcVolumeInfo,
destVolumeInfo, callback);
} else {
- String errMsg = "The source volume to migrate and the
destination volume are both on managed storage. " +
- "Migration in this case is not yet supported.";
-
- handleError(errMsg, callback);
+
handleVolumeMigrationFromManagedStorageToManagedStorage(srcVolumeInfo,
destVolumeInfo, callback);
}
} else if (!isVolumeOnManagedStorage(destVolumeInfo)) {
- String errMsg = "The 'StorageSystemDataMotionStrategy' does
not support this migration use case.";
-
- handleError(errMsg, callback);
+ if
(!HypervisorType.KVM.equals(srcVolumeInfo.getHypervisorType())) {
+ String errMsg = "Currently migrating volumes between
managed storage providers is only supported on KVM hypervisor";
Review Comment:
```suggestion
String errMsg = String.format("Currently migrating
volumes between managed storage providers is not supported on %s hypervisor",
srcVolumeInfo.getHypervisorType().toString());
```
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -512,20 +515,30 @@ else if (volumePath == null) {
}
}
+ private void
handleVolumeMigrationFromManagedStorageToManagedStorage(VolumeInfo
srcVolumeInfo, VolumeInfo destVolumeInfo,
+
AsyncCompletionCallback<CopyCommandResult> callback) {
+ if (!HypervisorType.KVM.equals(srcVolumeInfo.getHypervisorType())) {
+ String errMsg = "Currently migrating volumes between managed
storage providers is only supported on KVM hypervisor";
+ handleError(errMsg, callback);
+ } else {
+ handleVolumeMigrationForKVM(srcVolumeInfo, destVolumeInfo,
callback);
+ }
+ }
+
private void
handleVolumeMigrationFromManagedStorageToNonManagedStorage(VolumeInfo
srcVolumeInfo, VolumeInfo destVolumeInfo,
AsyncCompletionCallback<CopyCommandResult> callback) {
String errMsg = null;
try {
- if (!ImageFormat.QCOW2.equals(srcVolumeInfo.getFormat())) {
+ if (!HypervisorType.KVM.equals(srcVolumeInfo.getHypervisorType()))
{
throw new CloudRuntimeException("Currently, only the KVM
hypervisor type is supported for the migration of a volume " +
"from managed storage to non-managed storage.");
}
HypervisorType hypervisorType = HypervisorType.KVM;
VirtualMachine vm = srcVolumeInfo.getAttachedVM();
- if (vm != null && vm.getState() != VirtualMachine.State.Stopped) {
+ if (vm != null && (vm.getState() != VirtualMachine.State.Stopped
&& vm.getState() != VirtualMachine.State.Migrating)) {
Review Comment:
Do we need this check? Shouldn't this be blocked at higher level itself?
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -579,9 +592,10 @@ private void
handleVolumeMigrationFromManagedStorageToNonManagedStorage(VolumeIn
private void verifyFormatWithPoolType(ImageFormat imageFormat,
StoragePoolType poolType) {
if (imageFormat != ImageFormat.VHD && imageFormat != ImageFormat.OVA
&& imageFormat != ImageFormat.QCOW2 &&
- !(imageFormat == ImageFormat.RAW && StoragePoolType.PowerFlex
== poolType)) {
+ !(imageFormat == ImageFormat.RAW && (StoragePoolType.PowerFlex
== poolType ||
+ StoragePoolType.FiberChannel == poolType))) {
throw new CloudRuntimeException("Only the following image types
are currently supported: " +
- ImageFormat.VHD.toString() + ", " +
ImageFormat.OVA.toString() + ", " + ImageFormat.QCOW2.toString() + ", and " +
ImageFormat.RAW.toString() + "(for PowerFlex)");
+ ImageFormat.VHD.toString() + ", " +
ImageFormat.OVA.toString() + ", " + ImageFormat.QCOW2.toString() + ", and " +
ImageFormat.RAW.toString() + "(for PowerFlex and FiberChannel)");
Review Comment:
Can be formatted with String.format
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1701,13 +1797,32 @@ private void createVolumeFromSnapshot(SnapshotInfo
snapshotInfo) {
* invocation of createVolumeFromSnapshot(SnapshotInfo).
*/
private void deleteVolumeFromSnapshot(SnapshotInfo snapshotInfo) {
- SnapshotDetailsVO snapshotDetails =
handleSnapshotDetails(snapshotInfo.getId(), "delete");
+ VolumeVO volumeVO = null;
+ // cleanup any temporary volume previously created for copy from a
snapshot
+ if
("true".equalsIgnoreCase(snapshotInfo.getDataStore().getDriver().getCapabilities().get("CAN_CREATE_TEMP_VOLUME_FROM_SNAPSHOT")))
{
+ SnapshotDetailsVO tempUuid = null;
+ tempUuid = _snapshotDetailsDao.findDetail(snapshotInfo.getId(),
"TemporaryVolumeCopyUUID");
+ if (tempUuid == null || tempUuid.getValue() == null) {
+ return;
+ }
- try {
-
snapshotInfo.getDataStore().getDriver().createAsync(snapshotInfo.getDataStore(),
snapshotInfo, null);
+ volumeVO = _volumeDao.findByUuid(tempUuid.getValue());
+ if (volumeVO != null) {
+ _volumeDao.remove(volumeVO.getId());
+ }
+ _snapshotDetailsDao.remove(tempUuid.getId());
+ _snapshotDetailsDao.removeDetail(snapshotInfo.getId(),
"TemporaryVolumeCopyUUID");
}
- finally {
- _snapshotDetailsDao.remove(snapshotDetails.getId());
+ // fallback to original behavior of sending magic command to a create
function to delete :)
Review Comment:
Same as above, return can be added instead of else for simpler read
##########
api/src/main/java/org/apache/cloudstack/api/command/admin/storage/UpdateStoragePoolCmd.java:
##########
@@ -61,6 +63,20 @@ public class UpdateStoragePoolCmd extends BaseCmd {
" enable it back.")
private Boolean enabled;
+ @Parameter(name = ApiConstants.DETAILS,
+ type = CommandType.MAP,
Review Comment:
minor - indentation seems a bit off
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1685,13 +1777,17 @@ private CopyCmdAnswer copyImageToVolume(DataObject
srcDataObject, VolumeInfo des
* resign the SR and the VDI that should be inside of the snapshot before
copying the VHD file to secondary storage.
*/
private void createVolumeFromSnapshot(SnapshotInfo snapshotInfo) {
- SnapshotDetailsVO snapshotDetails =
handleSnapshotDetails(snapshotInfo.getId(), "create");
-
- try {
-
snapshotInfo.getDataStore().getDriver().createAsync(snapshotInfo.getDataStore(),
snapshotInfo, null);
- }
- finally {
- _snapshotDetailsDao.remove(snapshotDetails.getId());
+ if
("true".equalsIgnoreCase(snapshotInfo.getDataStore().getDriver().getCapabilities().get("CAN_CREATE_TEMP_VOLUME_FROM_SNAPSHOT")))
{
+ prepTempVolumeForCopyFromSnapshot(snapshotInfo);
+ // fallback to original behavior of sending magic ddata to the create
function
Review Comment:
can add return here without adding else statement
##########
plugins/acl/project-role-based/src/main/java/org/apache/cloudstack/acl/ProjectRoleBasedApiAccessChecker.java:
##########
@@ -119,7 +119,7 @@ public boolean checkAccess(User user, String
apiCommandName) throws PermissionDe
Account userAccount = accountService.getAccount(user.getAccountId());
if (accountService.isRootAdmin(userAccount.getId()) ||
accountService.isDomainAdmin(userAccount.getAccountId())) {
- LOGGER.info(String.format("Account [%s] is Root Admin or Domain
Admin, all APIs are allowed.", userAccount.getAccountName()));
+ LOGGER.trace(String.format("Account [%s] is Root Admin or Domain
Admin, all APIs are allowed.", userAccount.getAccountName()));
Review Comment:
Is this needed?
--
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]
