DaanHoogland commented on code in PR #6408:
URL: https://github.com/apache/cloudstack/pull/6408#discussion_r879532158
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -546,15 +554,18 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume,
Snapshot snapshot, Use
// create volume on primary from snapshot
AsyncCallFuture<VolumeApiResult> future =
volService.createVolumeFromSnapshot(vol, store, snapInfo);
+ String snapshotToString =
getReflectOnlySelectedFields(snapInfo.getSnapshotVO());
+
try {
VolumeApiResult result = future.get();
if (result.isFailed()) {
- s_logger.debug("Failed to create volume from snapshot:" +
result.getResult());
- throw new CloudRuntimeException("Failed to create volume from
snapshot:" + result.getResult());
+ String logMsg = String.format("Failed to create volume from
snapshot [%s] due to [%s].", snapshotToString, result.getResult());
+ s_logger.error(logMsg);
Review Comment:
:+1:
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/datastore/PrimaryDataStoreImpl.java:
##########
@@ -91,6 +91,9 @@ public class PrimaryDataStoreImpl implements PrimaryDataStore
{
private VolumeDao volumeDao;
private Map<String, String> _details;
+ private String uuid;
Review Comment:
why the changes in this file? they seem to only add maintenance risk.
##########
engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java:
##########
@@ -171,5 +171,5 @@ DiskProfile importVolume(Type type, String name,
DiskOffering offering, Long siz
/**
* Unmanage VM volumes
*/
- void unmanageVolumes(long vmId);
+ void unmanageVolumes(VirtualMachine vm);
Review Comment:
same here. Why should we pass around the object?
##########
engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/VolumeOrchestrator.java:
##########
@@ -484,13 +489,14 @@ public VolumeInfo createVolumeFromSnapshot(Volume volume,
Snapshot snapshot, Use
}
if (pool == null) {
+ String logMsg = String.format("Could not find a storage pool
in the pod/cluster of the provided VM [%s] to create the volume [%s] in.", vm,
volumeToString);
+
//pool could not be found in the VM's pod/cluster.
if (s_logger.isDebugEnabled()) {
- s_logger.debug("Could not find any storage pool to create
Volume in the pod/cluster of the provided VM " + vm.getUuid());
+ s_logger.error(logMsg);
Review Comment:
remove the if (isDebugEnabled()). doesn´t make sense anymore.
##########
engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/VolumeOrchestrationService.java:
##########
@@ -112,7 +112,7 @@ DiskProfile allocateRawVolume(Type type, String name,
DiskOffering offering, Lon
void release(VirtualMachineProfile profile);
- void release(long vmId, long hostId);
+ void release(VirtualMachine vm, Host host);
Review Comment:
I am not to sure about passing objects from service to service here. Is this
change 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]
