Allon Mureinik has uploaded a new change for review. Change subject: core: UpdateVmCommand: Shared disk boot status ......................................................................
core: UpdateVmCommand: Shared disk boot status Check the boot status of a disk against all the VMs it's attached to, not only the one it's context is edited in. Change-Id: I6a9d2e8800b9bf693fc55d8a627e0725277985ac Bug-Url: https://bugzilla.redhat.com/854964 Signed-off-by: Allon Mureinik <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java 2 files changed, 83 insertions(+), 36 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/89/12889/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 5de04f5..850bb99 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 @@ -41,7 +41,7 @@ private List<PermissionSubject> listPermissionSubjects; protected final Disk oldDisk; protected final Disk newDisk; - private List<Disk> otherVmDisks; + private Map<Guid, List<Disk>> otherVmDisks = new HashMap<Guid, List<Disk>>(); private Map<String, Pair<String, String>> sharedLockMap; private Map<String, Pair<String, String>> exclusiveLockMap; @@ -78,9 +78,15 @@ return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_NOT_DOWN); } } + + if (!checkCanPerformRegularUpdate(vmsDiskPluggedTo)) { + return false; + } } - return checkCanPerformRegularUpdate(); + // Set disk alias name in the disk retrieved from the parameters. + ImagesHandler.setDiskAlias(newDisk, getVm()); + return validateShareableDisk(); } @@ -106,49 +112,52 @@ addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM_DISK); } - private boolean checkCanPerformRegularUpdate() { - if (oldDisk.getDiskInterface() != newDisk.getDiskInterface()) { - List<VmNetworkInterface> allVmInterfaces = getVmNetworkInterfaceDao().getAllForVm(getVmId()); + private boolean checkCanPerformRegularUpdate(List<VM> vmsDiskPluggedTo) { + for (VM vm : vmsDiskPluggedTo) { + Guid vmId = vm.getId(); + if (oldDisk.getDiskInterface() != newDisk.getDiskInterface()) { + List<VmNetworkInterface> allVmInterfaces = getVmNetworkInterfaceDao().getAllForVm(vmId); - List<Disk> allVmDisks = new LinkedList<Disk>(getOtherVmDisks()); - allVmDisks.add(newDisk); - if (!checkPciAndIdeLimit(getVm().getNumOfMonitors(), - allVmInterfaces, - allVmDisks, - getReturnValue().getCanDoActionMessages())) { - return false; - } - } - - // Validate update boot disk. - if (newDisk.isBoot()) { - VmHandler.updateDisksForVm(getVm(), getOtherVmDisks()); - for (Disk disk : getVm().getDiskMap().values()) { - if (disk.isBoot()) { - addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE); - getReturnValue().getCanDoActionMessages().add( - String.format("$DiskName %1$s", disk.getDiskAlias())); + List<Disk> allVmDisks = new LinkedList<Disk>(getOtherVmDisks(vmId)); + allVmDisks.add(newDisk); + if (!checkPciAndIdeLimit(vm.getNumOfMonitors(), + allVmInterfaces, + allVmDisks, + getReturnValue().getCanDoActionMessages())) { return false; + } + } + + // Validate update boot disk. + if (newDisk.isBoot()) { + VmHandler.updateDisksForVm(vm, getOtherVmDisks(vmId)); + for (Disk disk : vm.getDiskMap().values()) { + if (disk.isBoot()) { + addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE); + getReturnValue().getCanDoActionMessages().add( + String.format("$DiskName %1$s", disk.getDiskAlias())); + return false; + } } } } - // Set disk alias name in the disk retrieved from the parameters. - ImagesHandler.setDiskAlias(newDisk, getVm()); - return validateShareableDisk(); + return true; } - protected List<Disk> getOtherVmDisks() { - if (otherVmDisks == null) { - otherVmDisks = getDiskDao().getAllForVm(getVmId()); - otherVmDisks.removeAll(LinqUtils.filter(otherVmDisks, new Predicate<Disk>() { + protected List<Disk> getOtherVmDisks(Guid vmId) { + List<Disk> disks = otherVmDisks.get(vmId); + if (disks == null) { + disks = getDiskDao().getAllForVm(vmId); + disks.removeAll(LinqUtils.filter(disks, new Predicate<Disk>() { @Override public boolean eval(Disk o) { return o.getId().equals(oldDisk.getId()); } })); + otherVmDisks.put(vmId, disks); } - return otherVmDisks; + return disks; } /** diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java index 6a28c1e..ae58735 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java @@ -9,6 +9,7 @@ import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.LinkedList; @@ -158,6 +159,39 @@ assertEquals(!boot, command.canDoAction()); } + @Test + public void canDoActionMakeDiskBootableOnOtherVmSuccess() { + canDoActionMakeDiskBootableOnOtherVm(false); + } + + @Test + public void canDoActionMakeDiskBootableOnOtherVmFail() { + canDoActionMakeDiskBootableOnOtherVm(true); + } + + private void canDoActionMakeDiskBootableOnOtherVm(boolean boot) { + UpdateVmDiskParameters parameters = createParameters(); + Disk newDisk = parameters.getDiskInfo(); + newDisk.setBoot(true); + + Guid otherVmId = Guid.NewGuid(); + VM otherVm = new VM(); + otherVm.setId(otherVmId); + + DiskImage otherDisk = new DiskImage(); + otherDisk.setId(Guid.NewGuid()); + otherDisk.setActive(true); + otherDisk.setBoot(boot); + when(diskDao.getAllForVm(otherVmId)).thenReturn(new LinkedList<Disk>(Collections.singleton(otherDisk))); + when(diskDao.get(diskImageGuid)).thenReturn(createDiskImage()); + initializeCommand(parameters); + + mockVmStatusDown(otherVm); + + // The command should only succeed if there is no other bootable disk + assertEquals(!boot, command.canDoAction()); + } + private void initializeCommand() { initializeCommand(createParameters()); } @@ -182,7 +216,7 @@ private void mockNullVm() { doReturn(vmDAO).when(command).getVmDAO(); - mockGetForDisk(null); + mockGetForDisk((VM) null); mockGetVmsListForDisk(null); when(vmDAO.get(command.getParameters().getVmId())).thenReturn(null); } @@ -190,13 +224,15 @@ /** * Mock a VM in status Up */ - protected VM mockVmStatusDown() { + protected VM mockVmStatusDown(VM... otherPluggedVMs) { VM vm = new VM(); vm.setStatus(VMStatus.Down); vm.setGuestOs("rhel6"); vm.setId(vmId); doReturn(vmDAO).when(command).getVmDAO(); - mockGetForDisk(vm); + List<VM> vms = new LinkedList<VM>(Arrays.asList(otherPluggedVMs)); + vms.add(vm); + mockGetForDisk(vms); mockGetVmsListForDisk(vm); storage_pool storagePool = mockStoragePool(Version.v3_1); vm.setStoragePoolId(storagePool.getId()); @@ -205,8 +241,10 @@ } private void mockGetForDisk(VM vm) { - List<VM> vms = new ArrayList<VM>(); - vms.add(vm); + mockGetForDisk(Collections.singletonList(vm)); + } + + private void mockGetForDisk(List<VM> vms) { Map<Boolean, List<VM>> vmsMap = new HashMap<Boolean, List<VM>>(); vmsMap.put(Boolean.TRUE, vms); when(vmDAO.getForDisk(diskImageGuid)).thenReturn(vmsMap); -- To view, visit http://gerrit.ovirt.org/12889 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6a9d2e8800b9bf693fc55d8a627e0725277985ac Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Allon Mureinik <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
