Allon Mureinik has uploaded a new change for review. Change subject: core: Remove VmRunHandler.getPluggedDisks(VM) ......................................................................
core: Remove VmRunHandler.getPluggedDisks(VM) Removed VmRunHandler.getPluggedDisks(VM) and replaced it with a direct DAO call DiskDao.getAllForVm(vmId, true). This approach has better performance (since all the filtering is done in the database, and there is no roundtrip via Java) and leaves the code easier to understand (there's just less of it). Change-Id: I55052dad87a552df200039e111fd0dd6658f6ae8 Signed-off-by: Allon Mureinik <[email protected]> --- 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/VmRunHandler.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RunVmCommandTest.java 3 files changed, 10 insertions(+), 64 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/55/13355/1 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 76aa476..03e3d69 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 @@ -222,7 +222,7 @@ @Override public Void runInTransaction() { - List<Disk> pluggedDisks = VmRunHandler.getInstance().getPluggedDisks(getVm()); + List<Disk> pluggedDisks = getDiskDao().getAllForVm(getVm().getId(), true); runVdsCommand(VDSCommandType.Snapshot, new SnapshotVDSCommandParameters(getVm().getRunOnVds().getValue(), getVm().getId(), 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 273ad02..c666dc1 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 @@ -27,14 +27,12 @@ import org.ovirt.engine.core.common.businessentities.VDSStatus; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; -import org.ovirt.engine.core.common.businessentities.VmDevice; import org.ovirt.engine.core.common.businessentities.storage_pool; import org.ovirt.engine.core.common.config.Config; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.queries.GetAllIsoImagesListParameters; import org.ovirt.engine.core.common.queries.VdcQueryReturnValue; import org.ovirt.engine.core.common.queries.VdcQueryType; -import org.ovirt.engine.core.common.utils.VmDeviceType; import org.ovirt.engine.core.common.vdscommands.IsVmDuringInitiatingVDSCommandParameters; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.compat.Guid; @@ -43,7 +41,6 @@ import org.ovirt.engine.core.dao.DiskDao; import org.ovirt.engine.core.dao.StorageDomainDAO; import org.ovirt.engine.core.dao.StoragePoolDAO; -import org.ovirt.engine.core.dao.VmDeviceDAO; import org.ovirt.engine.core.utils.log.Log; import org.ovirt.engine.core.utils.log.LogFactory; import org.ovirt.engine.core.utils.vmproperties.VmPropertiesUtils; @@ -85,7 +82,7 @@ Guid storagePoolId = vm.getStoragePoolId(); // Block from running a VM with no HDD when its first boot device is // HD and no other boot devices are configured - List<Disk> vmDisks = getPluggedDisks(vm); + List<Disk> vmDisks = getDiskDao().getAllForVm(vm.getId(), true); if (boot_sequence == BootSequence.C && vmDisks.size() == 0) { String messageStr = !vmDisks.isEmpty() ? VdcBllMessages.VM_CANNOT_RUN_FROM_DISK_WITHOUT_PLUGGED_DISK.toString() : @@ -243,7 +240,7 @@ */ public Map<StorageDomain, Integer> mapStorageDomainsToNumOfDisks(VM vm) { Map<StorageDomain, Integer> map = new HashMap<StorageDomain, Integer>(); - for (Disk disk : getPluggedDisks(vm)) { + for (Disk disk : getDiskDao().getAllForVm(vm.getId(), true)) { if (disk.isAllowSnapshot()) { for (StorageDomain domain : getStorageDomainDAO().getAllStorageDomainsByImageId(((DiskImage) disk).getImageId())) { map.put(domain, map.containsKey(domain) ? Integer.valueOf(map.get(domain) + 1) : Integer.valueOf(1)); @@ -264,31 +261,6 @@ !vm.isAutoStartup() || !runParams.getIsInternal(), !vm.isAutoStartup() || !runParams.getIsInternal(), vmDisks); - } - - /** - * The following method should return only plugged images which are attached to VM, - * only those images are relevant for the RunVmCommand - * @param vm - * @return - */ - protected List<Disk> getPluggedDisks(VM vm) { - List<Disk> diskImages = getDiskDao().getAllForVm(vm.getId()); - List<VmDevice> diskVmDevices = getVmDeviceDAO().getVmDeviceByVmIdTypeAndDevice(vm.getId(), - VmDeviceType.DISK.getName(), - VmDeviceType.DISK.getName()); - List<Disk> result = new ArrayList<Disk>(); - for (Disk diskImage : diskImages) { - for (VmDevice diskVmDevice : diskVmDevices) { - if (diskImage.getId().equals(diskVmDevice.getDeviceId())) { - if (diskVmDevice.getIsPlugged()) { - result.add(diskImage); - } - break; - } - } - } - return result; } /** @@ -384,10 +356,6 @@ protected DiskDao getDiskDao() { return DbFacade.getInstance().getDiskDao(); - } - - protected VmDeviceDAO getVmDeviceDAO() { - return DbFacade.getInstance().getVmDeviceDao(); } protected StorageDomainDAO getStorageDomainDAO() { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RunVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RunVmCommandTest.java index 802998c..ed373cc 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RunVmCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RunVmCommandTest.java @@ -36,17 +36,14 @@ import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.IVdsAsyncCommand; +import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.StoragePoolStatus; import org.ovirt.engine.core.common.businessentities.VM; import org.ovirt.engine.core.common.businessentities.VMStatus; -import org.ovirt.engine.core.common.businessentities.VmDevice; -import org.ovirt.engine.core.common.businessentities.VmDeviceId; import org.ovirt.engine.core.common.businessentities.VmStatic; -import org.ovirt.engine.core.common.businessentities.StorageDomain; import org.ovirt.engine.core.common.businessentities.storage_pool; import org.ovirt.engine.core.common.config.ConfigValues; import org.ovirt.engine.core.common.interfaces.VDSBrokerFrontend; -import org.ovirt.engine.core.common.utils.VmDeviceType; import org.ovirt.engine.core.common.vdscommands.VDSCommandType; import org.ovirt.engine.core.common.vdscommands.VDSParametersBase; import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; @@ -59,7 +56,6 @@ import org.ovirt.engine.core.dao.StorageDomainDAO; import org.ovirt.engine.core.dao.StoragePoolDAO; import org.ovirt.engine.core.dao.VmDAO; -import org.ovirt.engine.core.dao.VmDeviceDAO; import org.ovirt.engine.core.utils.MockConfigRule; import org.ovirt.engine.core.utils.vmproperties.VmPropertiesUtils; @@ -130,13 +126,6 @@ diskImage.setId(Guid.NewGuid()); diskImage.setStorageIds(new ArrayList<Guid>(Arrays.asList(new Guid()))); return diskImage; - } - - private static VmDevice createDiskVmDevice(final DiskImage diskImage) { - final VmDevice vmDevice = new VmDevice(); - vmDevice.setIsPlugged(true); - vmDevice.setId(new VmDeviceId(diskImage.getId(), Guid.NewGuid())); - return vmDevice; } /** @@ -341,7 +330,7 @@ @Test public void canRunVmFailNodisk() { - initDAOMocks(Collections.<Disk> emptyList(), Collections.<VmDevice> emptyList()); + initDAOMocks(Collections.<Disk> emptyList()); final VM vm = new VM(); doReturn(new VdsSelector(vm, new Guid(), true, new VdsFreeMemoryChecker(command))).when(command) @@ -356,8 +345,7 @@ final ArrayList<Disk> disks = new ArrayList<Disk>(); final DiskImage diskImage = createImage(); disks.add(diskImage); - final VmDevice vmDevice = createDiskVmDevice(diskImage); - initDAOMocks(disks, Collections.singletonList(vmDevice)); + initDAOMocks(disks); final VM vm = new VM(); vm.setStatus(VMStatus.Up); vm.setStoragePoolId(Guid.NewGuid()); @@ -373,8 +361,7 @@ final ArrayList<Disk> disks = new ArrayList<Disk>(); final DiskImage diskImage = createImage(); disks.add(diskImage); - final VmDevice vmDevice = createDiskVmDevice(diskImage); - initDAOMocks(disks, Collections.singletonList(vmDevice)); + initDAOMocks(disks); final VM vm = new VM(); SnapshotsValidator snapshotsValidator = mock(SnapshotsValidator.class); when(snapshotsValidator.vmNotDuringSnapshot(vm.getId())) @@ -394,7 +381,6 @@ final ArrayList<Disk> disks = new ArrayList<Disk>(); final DiskImage diskImage = createImage(); disks.add(diskImage); - final VmDevice vmDevice = createDiskVmDevice(diskImage); final VdsSelector vdsSelector = mock(VdsSelector.class); when(vdsSelector.canFindVdsToRunOn(anyListOf(String.class), anyBoolean())).thenReturn(true); @@ -403,7 +389,7 @@ VDSReturnValue vdsReturnValue = new VDSReturnValue(); vdsReturnValue.setReturnValue(false); when(vdsBrokerFrontend.RunVdsCommand(eq(VDSCommandType.IsVmDuringInitiating), any(VDSParametersBase.class))).thenReturn(vdsReturnValue); - initDAOMocks(disks, Collections.singletonList(vmDevice)); + initDAOMocks(disks); final VM vm = new VM(); // set stateless and HA @@ -464,12 +450,11 @@ /** * @param disks - * @param vmDevices * @param guid */ - protected void initDAOMocks(final List<Disk> disks, final List<VmDevice> vmDevices) { + protected void initDAOMocks(final List<Disk> disks) { final DiskDao diskDao = mock(DiskDao.class); - when(diskDao.getAllForVm(Guid.Empty)).thenReturn(disks); + when(diskDao.getAllForVm(Guid.Empty, true)).thenReturn(disks); doReturn(diskDao).when(command).getDiskDao(); doReturn(diskDao).when(vmRunHandler).getDiskDao(); @@ -478,13 +463,6 @@ .thenReturn(new ArrayList<StorageDomain>()); doReturn(storageDomainDAO).when(command).getStorageDomainDAO(); doReturn(storageDomainDAO).when(vmRunHandler).getStorageDomainDAO(); - - final VmDeviceDAO vmDeviceDao = mock(VmDeviceDAO.class); - when(vmDeviceDao.getVmDeviceByVmIdTypeAndDevice(Guid.Empty, - VmDeviceType.DISK.getName(), - VmDeviceType.DISK.getName())).thenReturn(vmDevices); - doReturn(vmDeviceDao).when(command).getVmDeviceDao(); - doReturn(vmDeviceDao).when(vmRunHandler).getVmDeviceDAO(); } private SnapshotsValidator mockSuccessfulSnapshotValidator() { -- To view, visit http://gerrit.ovirt.org/13355 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I55052dad87a552df200039e111fd0dd6658f6ae8 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
