DaanHoogland commented on code in PR #6307:
URL: https://github.com/apache/cloudstack/pull/6307#discussion_r857525628
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2113,170 +2132,224 @@ private Volume orchestrateAttachVolumeToVM(Long vmId,
Long volumeId, Long device
public Volume attachVolumeToVM(Long vmId, Long volumeId, Long deviceId) {
Account caller = CallContext.current().getCallingAccount();
- // Check that the volume ID is valid
- VolumeInfo volumeToAttach = volFactory.getVolume(volumeId);
- // Check that the volume is a data volume
- if (volumeToAttach == null || !(volumeToAttach.getVolumeType() ==
Volume.Type.DATADISK || volumeToAttach.getVolumeType() == Volume.Type.ROOT)) {
- throw new InvalidParameterValueException("Please specify a volume
with the valid type: " + Volume.Type.ROOT.toString() + " or " +
Volume.Type.DATADISK.toString());
- }
+ VolumeInfo volumeToAttach = getAndCheckVolumeInfo(volumeId);
- // Check that the volume is not currently attached to any VM
- if (volumeToAttach.getInstanceId() != null) {
- throw new InvalidParameterValueException("Please specify a volume
that is not attached to any VM.");
- }
+ UserVmVO vm = getAndCheckUserVmVO(vmId, volumeToAttach);
- // Check that the volume is not destroyed
- if (volumeToAttach.getState() == Volume.State.Destroy) {
- throw new InvalidParameterValueException("Please specify a volume
that is not destroyed.");
- }
+ checkDeviceId(deviceId, volumeToAttach, vm);
- // Check that the virtual machine ID is valid and it's a user vm
- UserVmVO vm = _userVmDao.findById(vmId);
- if (vm == null || vm.getType() != VirtualMachine.Type.User) {
- throw new InvalidParameterValueException("Please specify a valid
User VM.");
- }
+ checkNumberOfAttachedVolumes(deviceId, vm);
- // Check that the VM is in the correct state
- if (vm.getState() != State.Running && vm.getState() != State.Stopped) {
- throw new InvalidParameterValueException("Please specify a VM that
is either running or stopped.");
- }
+ excludeLocalStorageIfNeeded(volumeToAttach);
- // Check that the VM and the volume are in the same zone
- if (vm.getDataCenterId() != volumeToAttach.getDataCenterId()) {
- throw new InvalidParameterValueException("Please specify a VM that
is in the same zone as the volume.");
+ checkForDevicesInCopies(vmId, vm);
+
+ checkRightsToAttach(caller, volumeToAttach, vm);
+
+ HypervisorType rootDiskHyperType = vm.getHypervisorType();
+ HypervisorType volumeToAttachHyperType =
_volsDao.getHypervisorType(volumeToAttach.getId());
+
+ StoragePoolVO volumeToAttachStoragePool =
_storagePoolDao.findById(volumeToAttach.getPoolId());
+ if (s_logger.isTraceEnabled() && volumeToAttachStoragePool != null) {
+ s_logger.trace(String.format("volume to attach (%s/%s) has a
primary storage assigned to begin with (%s/%s)",
+ volumeToAttach.getName(), volumeToAttach.getUuid(),
volumeToAttachStoragePool.getName(), volumeToAttachStoragePool.getUuid()));
}
- // Check that the device ID is valid
- if (deviceId != null) {
- // validate ROOT volume type
- if (deviceId.longValue() == 0) {
-
validateRootVolumeDetachAttach(_volsDao.findById(volumeToAttach.getId()), vm);
- // vm shouldn't have any volume with deviceId 0
- if (!_volsDao.findByInstanceAndDeviceId(vm.getId(),
0).isEmpty()) {
- throw new InvalidParameterValueException("Vm already has
root volume attached to it");
- }
- // volume can't be in Uploaded state
- if (volumeToAttach.getState() == Volume.State.Uploaded) {
- throw new InvalidParameterValueException("No support for
Root volume attach in state " + Volume.State.Uploaded);
- }
+ checkForMatchinHypervisorTypesIf(volumeToAttachStoragePool != null &&
!volumeToAttachStoragePool.isManaged(), rootDiskHyperType,
volumeToAttachHyperType);
+
+ AsyncJobExecutionContext asyncExecutionContext =
AsyncJobExecutionContext.getCurrentExecutionContext();
+
+ if (asyncExecutionContext != null) {
+ AsyncJob job = asyncExecutionContext.getJob();
+
+ if (s_logger.isInfoEnabled()) {
+ s_logger.info("Trying to attaching volume " + volumeId + " to
vm instance:" + vm.getId() + ", update async job-" + job.getId() + " progress
status");
}
+
+ _jobMgr.updateAsyncJobAttachment(job.getId(), "Volume", volumeId);
}
- // Check that the number of data volumes attached to VM is less than
- // that supported by hypervisor
- if (deviceId == null || deviceId.longValue() != 0) {
- List<VolumeVO> existingDataVolumes =
_volsDao.findByInstanceAndType(vmId, Volume.Type.DATADISK);
- int maxAttachableDataVolumesSupported =
getMaxDataVolumesSupported(vm);
- if (existingDataVolumes.size() >=
maxAttachableDataVolumesSupported) {
- throw new InvalidParameterValueException(
- "The specified VM already has the maximum number of
data disks (" + maxAttachableDataVolumesSupported + ") attached. Please specify
another VM.");
- }
+ if
(asyncExecutionContext.isJobDispatchedBy(VmWorkConstants.VM_WORK_JOB_DISPATCHER))
{
+ return savelyOrchestrateAttachVolume(vmId, volumeId, deviceId);
+ } else {
+ return getVolumeAttachJobResult(vmId, volumeId, deviceId);
}
+ }
- // If local storage is disabled then attaching a volume with local disk
- // offering not allowed
- DataCenterVO dataCenter =
_dcDao.findById(volumeToAttach.getDataCenterId());
- if (!dataCenter.isLocalStorageEnabled()) {
- DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volumeToAttach.getDiskOfferingId());
- if (diskOffering.isUseLocalStorage()) {
- throw new InvalidParameterValueException("Zone is not
configured to use local storage but volume's disk offering " +
diskOffering.getName() + " uses it");
+ @Nullable private Volume getVolumeAttachJobResult(Long vmId, Long
volumeId, Long deviceId) {
+ Outcome<Volume> outcome = attachVolumeToVmThroughJobQueue(vmId,
volumeId, deviceId);
+
+ Volume vol = null;
+ try {
+ outcome.get();
+ } catch (InterruptedException e) {
+ throw new RuntimeException("Operation is interrupted", e);
+ } catch (ExecutionException e) {
+ throw new RuntimeException("Execution excetion", e);
+ }
+
+ Object jobResult = _jobMgr.unmarshallResultObject(outcome.getJob());
+ if (jobResult != null) {
+ if (jobResult instanceof ConcurrentOperationException) {
+ throw (ConcurrentOperationException)jobResult;
+ } else if (jobResult instanceof InvalidParameterValueException) {
+ throw (InvalidParameterValueException)jobResult;
+ } else if (jobResult instanceof RuntimeException) {
+ throw (RuntimeException)jobResult;
+ } else if (jobResult instanceof Throwable) {
+ throw new RuntimeException("Unexpected exception",
(Throwable)jobResult);
+ } else if (jobResult instanceof Long) {
+ vol = _volsDao.findById((Long)jobResult);
}
}
+ return vol;
+ }
- // if target VM has associated VM snapshots
- List<VMSnapshotVO> vmSnapshots = _vmSnapshotDao.findByVm(vmId);
- if (vmSnapshots.size() > 0) {
- throw new InvalidParameterValueException("Unable to attach volume,
please specify a VM that does not have VM snapshots");
+ private Volume savelyOrchestrateAttachVolume(Long vmId, Long volumeId,
Long deviceId) {
+ // avoid re-entrance
+
+ VmWorkJobVO placeHolder = null;
+ placeHolder = createPlaceHolderWork(vmId);
+ try {
+ return orchestrateAttachVolumeToVM(vmId, volumeId, deviceId);
+ } finally {
+ _workJobDao.expunge(placeHolder.getId());
}
+ }
- // if target VM has backups
- if (vm.getBackupOfferingId() != null ||
vm.getBackupVolumeList().size() > 0) {
- throw new InvalidParameterValueException("Unable to attach volume,
please specify a VM that does not have any backups");
+ private void checkForMatchinHypervisorTypesIf(boolean checkNeeded,
HypervisorType rootDiskHyperType, HypervisorType volumeToAttachHyperType) {
+ // managed storage can be used for different types of hypervisors
+ // only perform this check if the volume's storage pool is not null
and not managed
+ if (checkNeeded) {
+ if (volumeToAttachHyperType != HypervisorType.None &&
rootDiskHyperType != volumeToAttachHyperType) {
+ throw new InvalidParameterValueException("Can't attach a
volume created by: " + volumeToAttachHyperType + " to a " + rootDiskHyperType +
" vm");
+ }
}
+ }
+ private void checkRightsToAttach(Account caller, VolumeInfo
volumeToAttach, UserVmVO vm) {
// permission check
_accountMgr.checkAccess(caller, null, true, volumeToAttach, vm);
- if (!(Volume.State.Allocated.equals(volumeToAttach.getState()) ||
Volume.State.Ready.equals(volumeToAttach.getState()) ||
Volume.State.Uploaded.equals(volumeToAttach.getState()))) {
- throw new InvalidParameterValueException("Volume state must be in
Allocated, Ready or in Uploaded state");
- }
-
Account owner = _accountDao.findById(volumeToAttach.getAccountId());
- if (!(volumeToAttach.getState() == Volume.State.Allocated ||
volumeToAttach.getState() == Volume.State.Ready)) {
+ if (!Arrays.asList(Volume.State.Allocated,
Volume.State.Ready).contains(volumeToAttach.getState())) {
try {
_resourceLimitMgr.checkResourceLimit(owner,
ResourceType.primary_storage, volumeToAttach.getSize());
} catch (ResourceAllocationException e) {
s_logger.error("primary storage resource limit check failed",
e);
throw new InvalidParameterValueException(e.getMessage());
}
}
+ }
- HypervisorType rootDiskHyperType = vm.getHypervisorType();
- HypervisorType volumeToAttachHyperType =
_volsDao.getHypervisorType(volumeToAttach.getId());
+ private void checkForDevicesInCopies(Long vmId, UserVmVO vm) {
+ // if target VM has associated VM snapshots
+ List<VMSnapshotVO> vmSnapshots = _vmSnapshotDao.findByVm(vmId);
+ if (vmSnapshots.size() > 0) {
+ throw new InvalidParameterValueException("Unable to attach volume,
please specify a VM that does not have VM snapshots");
+ }
- StoragePoolVO volumeToAttachStoragePool =
_storagePoolDao.findById(volumeToAttach.getPoolId());
+ // if target VM has backups
+ if (vm.getBackupOfferingId() != null ||
vm.getBackupVolumeList().size() > 0) {
+ throw new InvalidParameterValueException("Unable to attach volume,
please specify a VM that does not have any backups");
+ }
+ }
- // managed storage can be used for different types of hypervisors
- // only perform this check if the volume's storage pool is not null
and not managed
- if (volumeToAttachStoragePool != null &&
!volumeToAttachStoragePool.isManaged()) {
- if (volumeToAttachHyperType != HypervisorType.None &&
rootDiskHyperType != volumeToAttachHyperType) {
- throw new InvalidParameterValueException("Can't attach a
volume created by: " + volumeToAttachHyperType + " to a " + rootDiskHyperType +
" vm");
+ /**
+ * If local storage is disabled then attaching a volume with a local
diskoffering is not allowed
+ */
+ private void excludeLocalStorageIfNeeded(VolumeInfo volumeToAttach) {
+ DataCenterVO dataCenter =
_dcDao.findById(volumeToAttach.getDataCenterId());
+ if (!dataCenter.isLocalStorageEnabled()) {
+ DiskOfferingVO diskOffering =
_diskOfferingDao.findById(volumeToAttach.getDiskOfferingId());
+ if (diskOffering.isUseLocalStorage()) {
+ throw new InvalidParameterValueException("Zone is not
configured to use local storage but volume's disk offering " +
diskOffering.getName() + " uses it");
}
}
+ }
- AsyncJobExecutionContext asyncExecutionContext =
AsyncJobExecutionContext.getCurrentExecutionContext();
-
- if (asyncExecutionContext != null) {
- AsyncJob job = asyncExecutionContext.getJob();
+ /**
+ * Check that the number of data volumes attached to VM is less than the
number that are supported by the hypervisor
+ */
+ private void checkNumberOfAttachedVolumes(Long deviceId, UserVmVO vm) {
+ if (deviceId == null || deviceId.longValue() != 0) {
+ List<VolumeVO> existingDataVolumes =
_volsDao.findByInstanceAndType(vm.getId(), Volume.Type.DATADISK);
+ int maxAttachableDataVolumesSupported =
getMaxDataVolumesSupported(vm);
+ if (existingDataVolumes.size() >=
maxAttachableDataVolumesSupported) {
+ throw new InvalidParameterValueException(
+ "The specified VM already has the maximum number of
data disks (" + maxAttachableDataVolumesSupported + ") attached. Please specify
another VM.");
+ }
+ }
+ }
- if (s_logger.isInfoEnabled()) {
- s_logger.info("Trying to attaching volume " + volumeId + " to
vm instance:" + vm.getId() + ", update async job-" + job.getId() + " progress
status");
+ private void checkDeviceId(Long deviceId, VolumeInfo volumeToAttach,
UserVmVO vm) {
+ if (deviceId != null) {
Review Comment:
extra return statements ar a code smell I don´t want to introduce.
--
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]
