Allon Mureinik has uploaded a new change for review. Change subject: core: UpdateVmDisk: unify other disk retrieval ......................................................................
core: UpdateVmDisk: unify other disk retrieval Several validations in the canDoAction() rely on getting the VM's other disks from the database, which is unfortunately done in the said several places. This patch unifies them all to a lazy getter, getOtherVmDisks() for better performance and readability. Besides the code change, it also introduces a couple of new unit tests to ensure that no functionality was damaged. Relates-To: https://bugzilla.redhat.com/854964 Change-Id: I31bcb767bf86d6f16116cfa06a710bb0488b087f 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, 52 insertions(+), 9 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/87/12887/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 ec06a04..b047262 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 @@ -2,6 +2,7 @@ import java.util.ArrayList; import java.util.HashMap; +import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -40,6 +41,7 @@ private List<PermissionSubject> listPermissionSubjects; protected final Disk oldDisk; protected final Disk newDisk; + private List<Disk> otherVmDisks; private Map<String, Pair<String, String>> sharedLockMap; private Map<String, Pair<String, String>> exclusiveLockMap; @@ -109,13 +111,7 @@ List<VmNetworkInterface> allVmInterfaces = DbFacade.getInstance() .getVmNetworkInterfaceDao().getAllForVm(getVmId()); - List<Disk> allVmDisks = getDiskDao().getAllForVm(getVmId()); - allVmDisks.removeAll(LinqUtils.filter(allVmDisks, new Predicate<Disk>() { - @Override - public boolean eval(Disk o) { - return o.getId().equals(oldDisk.getId()); - } - })); + List<Disk> allVmDisks = new LinkedList<Disk>(getOtherVmDisks()); allVmDisks.add(newDisk); if (!checkPciAndIdeLimit(getVm().getNumOfMonitors(), allVmInterfaces, @@ -127,9 +123,9 @@ // Validate update boot disk. if (newDisk.isBoot()) { - VmHandler.updateDisksFromDb(getVm()); + VmHandler.updateDisksForVm(getVm(), getOtherVmDisks()); for (Disk disk : getVm().getDiskMap().values()) { - if (disk.isBoot() && !disk.getId().equals(oldDisk.getId())) { + if (disk.isBoot()) { addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_DISK_BOOT_IN_USE); getReturnValue().getCanDoActionMessages().add( String.format("$DiskName %1$s", disk.getDiskAlias())); @@ -143,6 +139,19 @@ return validateShareableDisk(); } + protected List<Disk> getOtherVmDisks() { + if (otherVmDisks == null) { + otherVmDisks = getDiskDao().getAllForVm(getVmId()); + otherVmDisks.removeAll(LinqUtils.filter(otherVmDisks, new Predicate<Disk>() { + @Override + public boolean eval(Disk o) { + return o.getId().equals(oldDisk.getId()); + } + })); + } + return otherVmDisks; + } + /** * Validate whether a disk can be shareable. Disk can be shareable if it is not based on qcow FS, * which means it should not be based on a template image with thin provisioning, 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 4330a45..6a28c1e 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 @@ -1,5 +1,6 @@ package org.ovirt.engine.core.bll; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; @@ -8,7 +9,9 @@ import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; +import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -18,6 +21,7 @@ import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.ovirt.engine.core.common.action.UpdateVmDiskParameters; +import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.VDS; import org.ovirt.engine.core.common.businessentities.VM; @@ -125,6 +129,35 @@ assertTrue(oldDisk.getVmSnapshotId() == null); } + @Test + public void canDoActionMakeDiskBootableSuccess() { + canDoActionMakeDiskBootable(false); + } + + @Test + public void canDoActionMakeDiskBootableFail() { + canDoActionMakeDiskBootable(true); + } + + private void canDoActionMakeDiskBootable(boolean boot) { + UpdateVmDiskParameters parameters = createParameters(); + Disk newDisk = parameters.getDiskInfo(); + newDisk.setBoot(true); + + DiskImage otherDisk = new DiskImage(); + otherDisk.setId(Guid.NewGuid()); + otherDisk.setActive(true); + otherDisk.setBoot(boot); + when(diskDao.getAllForVm(vmId)).thenReturn(new LinkedList<Disk>(Collections.singleton(otherDisk))); + when(diskDao.get(diskImageGuid)).thenReturn(createDiskImage()); + initializeCommand(parameters); + + mockVmStatusDown(); + + // The command should only succeed if there is no other bootable disk + assertEquals(!boot, command.canDoAction()); + } + private void initializeCommand() { initializeCommand(createParameters()); } @@ -217,6 +250,7 @@ */ protected UpdateVmDiskParameters createParameters() { DiskImage diskInfo = new DiskImage(); + diskInfo.setId(diskImageGuid); return new UpdateVmDiskParameters(vmId, diskImageGuid, diskInfo); } -- To view, visit http://gerrit.ovirt.org/12887 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I31bcb767bf86d6f16116cfa06a710bb0488b087f 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
