Allon Mureinik has uploaded a new change for review.
Change subject: core: VM ImageLockd status check in ImagesHandler
......................................................................
core: VM ImageLockd status check in ImagesHandler
Since we now have statuses per disk, checking a VM's status for
ImageLocked seems unrelated to ImagesHandler.
In fact, this has become a more "logical" locked status, not directly
associated to the VM's images.
The relevant code was removed from ImagesHandler and moved to
VmValidator, where it belongs.
Note that most places that required this validation already validated
first that the VM was in fact down, so adding an additional status check
there was pointless.
In the places where the additional check was warranted, care was taken
to make the VM's status check /AFTER/ the disks', so that the most
informative error message (containing the aliases of the locked disks)
could be produced.
To seal the deal, the API of the relevant methods was changed to use VM
ID instead of a VM, to show it is no longer needed.
Change-Id: Iacfcb81df64b778b2093c7c84a8582cfd59be29c
Signed-off-by: Allon Mureinik <amure...@redhat.com>
---
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
M
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java
9 files changed, 57 insertions(+), 21 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/90/11190/1
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
index a0b2d3a..07d2a29 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
@@ -17,6 +17,7 @@
import org.ovirt.engine.core.bll.utils.VmDeviceUtils;
import org.ovirt.engine.core.bll.utils.WipeAfterDeleteUtils;
import org.ovirt.engine.core.bll.validator.StorageDomainValidator;
+import org.ovirt.engine.core.bll.validator.VmValidator;
import org.ovirt.engine.core.common.AuditLogType;
import org.ovirt.engine.core.common.VdcObjectType;
import org.ovirt.engine.core.common.action.AddDiskParameters;
@@ -142,7 +143,8 @@
returnValue = isStoragePoolMatching(vm) &&
performImagesChecks(vm) &&
validate(getSnapshotValidator().vmNotDuringSnapshot(vm.getId())) &&
-
validate(getSnapshotValidator().vmNotInPreview(vm.getId()));
+
validate(getSnapshotValidator().vmNotInPreview(vm.getId())) &&
+ validate(new VmValidator(vm).vmNotLocked());
}
return returnValue;
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
index 63c4d87..8fa8e1c 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java
@@ -7,8 +7,9 @@
import java.util.Map;
import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator;
-import org.ovirt.engine.core.bll.utils.VmDeviceUtils;
import org.ovirt.engine.core.bll.utils.PermissionSubject;
+import org.ovirt.engine.core.bll.utils.VmDeviceUtils;
+import org.ovirt.engine.core.bll.validator.VmValidator;
import org.ovirt.engine.core.common.VdcObjectType;
import org.ovirt.engine.core.common.action.AddVmFromSnapshotParameters;
import org.ovirt.engine.core.common.action.VdcActionType;
@@ -16,9 +17,9 @@
import org.ovirt.engine.core.common.businessentities.ImageStatus;
import org.ovirt.engine.core.common.businessentities.Snapshot;
import org.ovirt.engine.core.common.businessentities.Snapshot.SnapshotStatus;
-import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
import org.ovirt.engine.core.common.businessentities.VM;
import org.ovirt.engine.core.common.businessentities.VMStatus;
+import
org.ovirt.engine.core.common.businessentities.network.VmNetworkInterface;
import org.ovirt.engine.core.common.locks.LockingGroup;
import
org.ovirt.engine.core.common.queries.GetVmConfigurationBySnapshotQueryParams;
import org.ovirt.engine.core.common.queries.VdcQueryReturnValue;
@@ -274,7 +275,11 @@
return false;
}
- if (!ImagesHandler.checkImagesLocked(getSourceVmFromDb(),
getReturnValue().getCanDoActionMessages())) {
+ if (!ImagesHandler.checkImagesLocked(getSourceVmFromDb().getId(),
getReturnValue().getCanDoActionMessages())) {
+ return false;
+ }
+
+ if (!validate(new VmValidator(getSourceVmFromDb()).vmNotLocked())) {
return false;
}
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
index db8b1ce..7cbc0fd 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java
@@ -303,7 +303,8 @@
true,
true,
true,
- disksList);
+ disksList)
+ && validate(vmValidator.vmNotLocked());
}
return result;
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
index 2822deb..e0b76fe 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
@@ -470,9 +470,9 @@
ListUtils.nullSafeAdd(messages,
VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND.toString());
}
- List<DiskImage> images = getImages(vm, diskImageList);
+ List<DiskImage> images = getImages(vm.getId(), diskImageList);
if (returnValue && checkImagesLocked) {
- returnValue = checkImagesLocked(vm, messages, images);
+ returnValue = checkImagesLocked(messages, images);
}
if (returnValue && checkIsValid) {
@@ -495,11 +495,11 @@
return returnValue;
}
- public static boolean checkImagesLocked(VM vm, List<String> messages) {
- return checkImagesLocked(vm, messages, getImages(vm, null));
+ public static boolean checkImagesLocked(Guid vmId, List<String> messages) {
+ return checkImagesLocked(messages, getImages(vmId, null));
}
- private static boolean checkImagesLocked(VM vm, List<String> messages,
List<DiskImage> images) {
+ private static boolean checkImagesLocked(List<String> messages,
List<DiskImage> images) {
boolean returnValue = true;
List<String> lockedDisksAliases = new ArrayList<String>();
for (DiskImage diskImage : images) {
@@ -515,16 +515,12 @@
String.format("$%1$s %2$s", "diskAliases",
StringUtils.join(lockedDisksAliases, ", ")));
}
- if (returnValue && vm.getStatus() == VMStatus.ImageLocked) {
- ListUtils.nullSafeAdd(messages,
VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED.toString());
- returnValue = false;
- }
return returnValue;
}
- private static List<DiskImage> getImages(VM vm, Collection<? extends Disk>
diskImageList) {
+ private static List<DiskImage> getImages(Guid vmId, Collection<? extends
Disk> diskImageList) {
if (diskImageList == null) {
- return
filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(vm.getId()),
true, false);
+ return
filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(vmId), true,
false);
}
return filterImageDisks(diskImageList, true, false);
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
index 47f2df0..3373207 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java
@@ -159,10 +159,17 @@
return false;
}
- // we cannot force remove if there is running task
- if (getParameters().getForce() && getVm().getStatus() ==
VMStatus.ImageLocked
- &&
AsyncTaskManager.getInstance().HasTasksByStoragePoolId(getVm().getStoragePoolId()))
{
- return
failCanDoAction(VdcBllMessages.VM_CANNOT_REMOVE_HAS_RUNNING_TASKS);
+ // Handle VM status with ImageLocked
+ if (getVm().getStatus() == VMStatus.ImageLocked) {
+ // without force remove, we can't remove the VM
+ if (!getParameters().getForce()) {
+ return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED);
+ }
+
+ // If it is force, we cannot remove if there are task
+ if
(AsyncTaskManager.getInstance().HasTasksByStoragePoolId(getVm().getStoragePoolId()))
{
+ return
failCanDoAction(VdcBllMessages.VM_CANNOT_REMOVE_HAS_RUNNING_TASKS);
+ }
}
return true;
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
index 1f1682c..2bf69d0 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmPoolCommandBase.java
@@ -5,6 +5,7 @@
import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator;
import org.ovirt.engine.core.bll.utils.PermissionSubject;
+import org.ovirt.engine.core.bll.validator.VmValidator;
import org.ovirt.engine.core.common.VdcObjectType;
import org.ovirt.engine.core.common.action.RunVmParams;
import org.ovirt.engine.core.common.action.VmPoolParametersBase;
@@ -213,6 +214,14 @@
!Guid.Empty.equals(storageDomainId),
true,
disks);
+
+ if (returnValue) {
+ ValidationResult vmNotLockResult = new
VmValidator(vm).vmNotLocked();
+ if (!vmNotLockResult.isValid()) {
+ messages.add(vmNotLockResult.getMessage().name());
+ returnValue = false;
+ }
+ }
}
if (!returnValue) {
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
index 2047fa0..32a7e22 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmRunHandler.java
@@ -11,6 +11,7 @@
import org.ovirt.engine.core.bll.command.utils.StorageDomainSpaceChecker;
import org.ovirt.engine.core.bll.interfaces.BackendInternal;
import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator;
+import org.ovirt.engine.core.bll.validator.VmValidator;
import org.ovirt.engine.core.common.VdcActionUtils;
import org.ovirt.engine.core.common.action.RunVmParams;
import org.ovirt.engine.core.common.action.VdcActionType;
@@ -109,6 +110,13 @@
if (retValue && !performImageChecksForRunningVm(vm,
message, runParams, vmDisks)) {
retValue = false;
}
+
+ ValidationResult vmNotLockedResult = new
VmValidator(vm).vmNotLocked();
+ if (!vmNotLockedResult.isValid()) {
+ message.add(vmNotLockedResult.getMessage().name());
+ retValue = false;
+ }
+
// Check if iso and floppy path exists
if (retValue && !vm.isAutoStartup()
&&
!validateIsoPath(findActiveISODomain(vm.getStoragePoolId()),
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
index 76bb66b..8b9bdc3 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java
@@ -218,7 +218,7 @@
}
protected boolean checkImagesStatus() {
- return ImagesHandler.checkImagesLocked(getVm(),
getReturnValue().getCanDoActionMessages());
+ return ImagesHandler.checkImagesLocked(getVmId(),
getReturnValue().getCanDoActionMessages());
}
private boolean isLiveMigrationEnabled() {
diff --git
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java
index 72c1781..8d0e149 100644
---
a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java
+++
b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/VmValidator.java
@@ -33,6 +33,14 @@
return ValidationResult.VALID;
}
+ public ValidationResult vmNotLocked() {
+ if (vm.getStatus() == VMStatus.ImageLocked) {
+ return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED);
+ }
+
+ return ValidationResult.VALID;
+ }
+
public ValidationResult vmNotRunningStateless() {
if (DbFacade.getInstance().getSnapshotDao().exists(vm.getId(),
SnapshotType.STATELESS)) {
VdcBllMessages message = vm.isRunning() ?
VdcBllMessages.ACTION_TYPE_FAILED_VM_RUNNING_STATELESS :
--
To view, visit http://gerrit.ovirt.org/11190
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iacfcb81df64b778b2093c7c84a8582cfd59be29c
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
