Tal Nisan has uploaded a new change for review. Change subject: core: UpdateVmDiskCommand canDoAction cleanup ......................................................................
core: UpdateVmDiskCommand canDoAction cleanup Change-Id: Id74d24f991a1c00b92dec72125148a6a9fa75d5f Signed-off-by: Tal Nisan <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java 1 file changed, 71 insertions(+), 72 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/86/10386/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java index 4d3c209..77317e1 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java @@ -6,12 +6,12 @@ import java.util.List; import java.util.Map; -import org.ovirt.engine.core.bll.quota.QuotaStorageDependent; -import org.ovirt.engine.core.bll.quota.QuotaStorageConsumptionParameter; import org.ovirt.engine.core.bll.quota.QuotaConsumptionParameter; +import org.ovirt.engine.core.bll.quota.QuotaStorageConsumptionParameter; +import org.ovirt.engine.core.bll.quota.QuotaStorageDependent; +import org.ovirt.engine.core.bll.utils.PermissionSubject; import org.ovirt.engine.core.bll.utils.VmDeviceUtils; import org.ovirt.engine.core.common.AuditLogType; -import org.ovirt.engine.core.bll.utils.PermissionSubject; import org.ovirt.engine.core.common.VdcObjectType; import org.ovirt.engine.core.common.action.UpdateVmDiskParameters; import org.ovirt.engine.core.common.businessentities.ActionGroup; @@ -39,14 +39,15 @@ private static final long serialVersionUID = 5915267156998835363L; private List<PermissionSubject> listPermissionSubjects; - private final Disk _oldDisk; - private boolean shouldUpdateQuotaForDisk; + private final Disk oldDisk; + private final Disk newDisk; private Map<String, String> sharedLockMap; private Map<String, String> exclusiveLockMap; public UpdateVmDiskCommand(T parameters) { super(parameters); - _oldDisk = getDiskDao().get(getParameters().getDiskId()); + oldDisk = getDiskDao().get(getParameters().getDiskId()); + newDisk = getParameters().getDiskInfo(); } @Override @@ -56,44 +57,44 @@ @Override protected boolean canDoAction() { - boolean retValue = isVmExist(); - if (retValue) { - if (!isDiskExist(_oldDisk)) { - return false; - } + if (!isVmExist()) { + return false; + } + if (!isDiskExist(oldDisk)) { + return false; + } - List<VM> listVms = getVmDAO().getForDisk(_oldDisk.getId()).get(Boolean.TRUE); - buidSharedLockMap(listVms); - buidExclusiveLockMap(listVms); + List<VM> vmsDiskPluggedTo = getVmDAO().getForDisk(oldDisk.getId()).get(Boolean.TRUE); + + if (vmsDiskPluggedTo != null && !vmsDiskPluggedTo.isEmpty()) { + buildSharedLockMap(vmsDiskPluggedTo); + buildExclusiveLockMap(vmsDiskPluggedTo); acquireLockInternal(); // Check if all VMs are in status down. - if (listVms != null && !listVms.isEmpty()) { - for (VM vm : listVms) { - if (vm.getStatus() != VMStatus.Down) { - addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN); - return false; - } + for (VM vm : vmsDiskPluggedTo) { + if (vm.getStatus() != VMStatus.Down) { + addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN); + return false; } } - retValue = checkCanPerformRegularUpdate(); } - return retValue; + + return checkCanPerformRegularUpdate(); } - private void buidSharedLockMap(List<VM> listVms) { - if (listVms != null && !listVms.isEmpty()) { + + private void buildSharedLockMap(List<VM> vmsDiskPluggedTo) { sharedLockMap = new HashMap<String, String>(); - for (VM vm : listVms) { + for (VM vm : vmsDiskPluggedTo) { sharedLockMap.put(vm.getId().toString(), LockingGroup.VM.name()); } - } } - private void buidExclusiveLockMap(List<VM> listVms) { - if (getParameters().getDiskInfo().isBoot() && listVms != null && !listVms.isEmpty()) { + private void buildExclusiveLockMap(List<VM> vmsDiskPluggedTo) { + if (newDisk.isBoot()) { exclusiveLockMap = new HashMap<String, String>(); - for (VM vm : listVms) { + for (VM vm : vmsDiskPluggedTo) { exclusiveLockMap.put(vm.getId().toString(), LockingGroup.VM_DISK_BOOT.name()); } } @@ -106,8 +107,7 @@ } private boolean checkCanPerformRegularUpdate() { - boolean retValue = true; - if (_oldDisk.getDiskInterface() != getParameters().getDiskInfo().getDiskInterface()) { + if (oldDisk.getDiskInterface() != newDisk.getDiskInterface()) { List<VmNetworkInterface> allVmInterfaces = DbFacade.getInstance() .getVmNetworkInterfaceDao().getAllForVm(getVmId()); @@ -116,35 +116,34 @@ @Override public boolean eval(Disk o) { return o.getId().equals( - _oldDisk.getId()); + oldDisk.getId()); } })); - allVmDisks.add(getParameters().getDiskInfo()); + allVmDisks.add(newDisk); if (!checkPciAndIdeLimit(getVm().getNumOfMonitors(), allVmInterfaces, allVmDisks, getReturnValue().getCanDoActionMessages())) { - retValue = false; + return false; } } // Validate update boot disk. - if (retValue && getParameters().getDiskInfo().isBoot()) { + if (newDisk.isBoot()) { VmHandler.updateDisksFromDb(getVm()); for (Disk disk : getVm().getDiskMap().values()) { - if (disk.isBoot() && !disk.getId().equals(_oldDisk.getId())) { - retValue = false; + if (disk.isBoot() && !disk.getId().equals(oldDisk.getId())) { addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE); getReturnValue().getCanDoActionMessages().add( String.format("$DiskName %1$s", disk.getDiskAlias())); - break; + return false; } } } // Set disk alias name in the disk retrieved from the parameters. - ImagesHandler.setDiskAlias(getParameters().getDiskInfo(), getVm()); - return retValue && validateShareableDisk(); + ImagesHandler.setDiskAlias(newDisk, getVm()); + return validateShareableDisk(); } /** @@ -154,11 +153,11 @@ * @return Indication whether the disk can be shared or not. */ protected boolean validateShareableDisk() { - if (DiskStorageType.LUN == _oldDisk.getDiskStorageType()) { + if (DiskStorageType.LUN == oldDisk.getDiskStorageType()) { return true; } - boolean isDiskUpdatedToShareable = getParameters().getDiskInfo().isShareable(); - boolean isDiskShareable = _oldDisk.isShareable(); + boolean isDiskUpdatedToShareable = newDisk.isShareable(); + boolean isOldDiskSharable = oldDisk.isShareable(); // Check if VM is not during snapshot. if (getSnapshotDao().exists(getVmId(), SnapshotStatus.IN_PREVIEW)) { @@ -166,39 +165,39 @@ return false; } - if (!isDiskShareable && isDiskUpdatedToShareable) { + if (!isOldDiskSharable && isDiskUpdatedToShareable) { List<DiskImage> diskImageList = - getDiskImageDao().getAllSnapshotsForImageGroup(_oldDisk.getId()); + getDiskImageDao().getAllSnapshotsForImageGroup(oldDisk.getId()); // If disk image list is more then one then we assume that it has a snapshot, since one image is the active // disk and all the other images are the snapshots. - if ((diskImageList.size() > 1) || !Guid.Empty.equals(((DiskImage)_oldDisk).getit_guid())) { + if ((diskImageList.size() > 1) || !Guid.Empty.equals(((DiskImage) oldDisk).getit_guid())) { addCanDoActionMessage(VdcBllMessages.SHAREABLE_DISK_IS_NOT_SUPPORTED_FOR_DISK); return false; } - if (!isVersionSupportedForShareable(_oldDisk, getStoragePoolDAO().get(getVm().getStoragePoolId()) + if (!isVersionSupportedForShareable(oldDisk, getStoragePoolDAO().get(getVm().getStoragePoolId()) .getcompatibility_version() .getValue())) { addCanDoActionMessage(VdcBllMessages.ACTION_NOT_SUPPORTED_FOR_CLUSTER_POOL_LEVEL); return false; } - DiskImage diskImage = (DiskImage) getParameters().getDiskInfo(); + DiskImage diskImage = (DiskImage) newDisk; if (!isVolumeFormatSupportedForShareable(diskImage.getvolume_format())) { addCanDoActionMessage(VdcBllMessages.SHAREABLE_DISK_IS_NOT_SUPPORTED_BY_VOLUME_FORMAT); return false; } - // If user want to update the disk to be shareable then update the vm snapshot id to be null. - ((DiskImage) _oldDisk).setvm_snapshot_id(null); - } else if (isDiskShareable && !isDiskUpdatedToShareable) { - if (getVmDAO().getVmsListForDisk(_oldDisk.getId()).size() > 1) { + // If user want to update the disk to be sharable then update the vm snapshot id to be null. + ((DiskImage) oldDisk).setvm_snapshot_id(null); + } else if (isOldDiskSharable && !isDiskUpdatedToShareable) { + if (getVmDAO().getVmsListForDisk(oldDisk.getId()).size() > 1) { addCanDoActionMessage(VdcBllMessages.DISK_IS_ALREADY_SHARED_BETWEEN_VMS); return false; } // If disk is not floating, then update its vm snapshot id to the active VM snapshot. - ((DiskImage) _oldDisk).setvm_snapshot_id(DbFacade.getInstance() + ((DiskImage) oldDisk).setvm_snapshot_id(DbFacade.getInstance() .getSnapshotDao() .getId(getVmId(), SnapshotType.ACTIVE) .getValue()); @@ -212,7 +211,7 @@ if (listPermissionSubjects == null) { listPermissionSubjects = new ArrayList<PermissionSubject>(); - Guid diskId = _oldDisk == null ? null : _oldDisk.getId(); + Guid diskId = oldDisk == null ? null : oldDisk.getId(); listPermissionSubjects.add(new PermissionSubject(diskId, VdcObjectType.Disk, ActionGroup.EDIT_DISK_PROPERTIES)); @@ -225,16 +224,16 @@ @Override public Object runInTransaction() { clearAddressOnInterfaceChange(); - _oldDisk.setBoot(getParameters().getDiskInfo().isBoot()); - _oldDisk.setDiskInterface(getParameters().getDiskInfo().getDiskInterface()); - _oldDisk.setPropagateErrors(getParameters().getDiskInfo().getPropagateErrors()); - _oldDisk.setWipeAfterDelete(getParameters().getDiskInfo().isWipeAfterDelete()); - _oldDisk.setDiskAlias(getParameters().getDiskInfo().getDiskAlias()); - _oldDisk.setDiskDescription(getParameters().getDiskInfo().getDiskDescription()); - _oldDisk.setShareable(getParameters().getDiskInfo().isShareable()); - DbFacade.getInstance().getBaseDiskDao().update(_oldDisk); - if (_oldDisk.getDiskStorageType() == DiskStorageType.IMAGE) { - DiskImage diskImage = (DiskImage) _oldDisk; + oldDisk.setBoot(newDisk.isBoot()); + oldDisk.setDiskInterface(newDisk.getDiskInterface()); + oldDisk.setPropagateErrors(newDisk.getPropagateErrors()); + oldDisk.setWipeAfterDelete(newDisk.isWipeAfterDelete()); + oldDisk.setDiskAlias(newDisk.getDiskAlias()); + oldDisk.setDiskDescription(newDisk.getDiskDescription()); + oldDisk.setShareable(newDisk.isShareable()); + DbFacade.getInstance().getBaseDiskDao().update(oldDisk); + if (oldDisk.getDiskStorageType() == DiskStorageType.IMAGE) { + DiskImage diskImage = (DiskImage) oldDisk; diskImage.setQuotaId(getQuotaId()); getImageDao().update(diskImage.getImage()); } @@ -250,8 +249,8 @@ private void clearAddressOnInterfaceChange() { // clear the disk address if the type has changed - if (_oldDisk.getDiskInterface() != getParameters().getDiskInfo().getDiskInterface()) { - getVmDeviceDao().clearDeviceAddress(getVmDeviceDao().get(new VmDeviceId(_oldDisk.getId(), getVmId())) + if (oldDisk.getDiskInterface() != newDisk.getDiskInterface()) { + getVmDeviceDao().clearDeviceAddress(getVmDeviceDao().get(new VmDeviceId(oldDisk.getId(), getVmId())) .getDeviceId()); } } @@ -265,7 +264,7 @@ @Override public String getDiskAlias() { - return _oldDisk.getDiskAlias(); + return oldDisk.getDiskAlias(); } @Override @@ -288,12 +287,12 @@ } private boolean isQuotaValidationNeeded() { - return DiskStorageType.IMAGE == _oldDisk.getDiskStorageType(); + return DiskStorageType.IMAGE == oldDisk.getDiskStorageType(); } private Guid getQuotaId() { - if (getParameters().getDiskInfo() != null && isQuotaValidationNeeded()) { - return ((DiskImage) getParameters().getDiskInfo()).getQuotaId(); + if (newDisk != null && isQuotaValidationNeeded()) { + return ((DiskImage) newDisk).getQuotaId(); } return null; } @@ -303,8 +302,8 @@ List<QuotaConsumptionParameter> list = new ArrayList<QuotaConsumptionParameter>(); if (isQuotaValidationNeeded()) { - DiskImage oldDiskImage = (DiskImage) _oldDisk; - DiskImage newDiskImage = (DiskImage) getParameters().getDiskInfo(); + DiskImage oldDiskImage = (DiskImage) oldDisk; + DiskImage newDiskImage = (DiskImage) newDisk; if (oldDiskImage.getQuotaId() == null || !oldDiskImage.getQuotaId().equals(newDiskImage.getQuotaId())) { if (oldDiskImage.getQuotaId() != null && !Guid.Empty.equals(oldDiskImage.getQuotaId())) { list.add(new QuotaStorageConsumptionParameter( -- To view, visit http://gerrit.ovirt.org/10386 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Id74d24f991a1c00b92dec72125148a6a9fa75d5f Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tal Nisan <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
