Arik Hadas has uploaded a new change for review. Change subject: core: move the validation of active iso domain on run vm ......................................................................
core: move the validation of active iso domain on run vm The validation which checks that there is an active ISO domain when the VM is configured with 'iso path' was located in RunVmCommand, while it should be located in RunVmValidator. Besides moving that validation to the RunVmValidator class, a minor improvement is introduced: instead of searching the id of the active ISO domain each time we need it (and we're planning to use it more soon), it is now cached for the lifetime of the RunVm command. Change-Id: Ic1bc81686be01088aa2dde5c61e4b83c96643f73 Signed-off-by: Arik Hadas <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.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/validator/RunVmValidator.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/RunVmValidatorTest.java 4 files changed, 48 insertions(+), 56 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/06/21206/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java index 206c9a6..7ae6401 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java @@ -95,6 +95,8 @@ * from the active snapshot was restored so the memory should not be used */ private boolean memoryFromSnapshotIrrelevant; + private Guid cachedActiveIsoDomainId; + public static final String ISO_PREFIX = "iso://"; protected RunVmCommand(Guid commandId) { @@ -690,7 +692,7 @@ boolean attachCd = false; String selectedToolsVersion = ""; String selectedToolsClusterVersion = ""; - Guid isoDomainId = getIsoDomainListSyncronizer().findActiveISODomain(getVm().getStoragePoolId()); + Guid isoDomainId = getActiveIsoDomainId(); if (osRepository.isWindows(getVm().getVmOsId()) && null != isoDomainId) { // get cluster version of the vm tools @@ -808,13 +810,6 @@ getVm().setVmPayload(getParameters().getVmPayload()); } - // if there is a CD path as part of the VM definition and there is no active ISO domain, - // we don't run the VM - if (!vm.isAutoStartup() && !StringUtils.isEmpty(getVm().getIsoPath()) - && getIsoDomainListSyncronizer().findActiveISODomain(getVm().getStoragePoolId()) == null) { - return failCanDoAction(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO); - } - return true; } @@ -829,7 +824,17 @@ } protected RunVmValidator getRunVmValidator() { - return new RunVmValidator(getVm(), getParameters(), isInternalExecution()); + return new RunVmValidator(getVm(), getParameters(), + isInternalExecution(), getActiveIsoDomainId()); + } + + protected Guid getActiveIsoDomainId() { + if (cachedActiveIsoDomainId == null) { + cachedActiveIsoDomainId = getIsoDomainListSyncronizer() + .findActiveISODomain(getVm().getStoragePoolId()); + } + + return cachedActiveIsoDomainId; } @Override 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 fe36cb7..12651f3 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 @@ -225,13 +225,18 @@ RunVmParams runVmParams = new RunVmParams(vmId); runVmParams.setUseVnc(osRepository.isLinux(vm.getVmOsId()) || vm.getVmType() == VmType.Server); - return new RunVmValidator(vm, runVmParams, false).canRunVm( - messages, - fetchStoragePool(vm.getStoragePoolId()), - Collections.<Guid>emptyList(), - null, - null, - DbFacade.getInstance().getVdsGroupDao().get(vm.getVdsGroupId())); + return new RunVmValidator(vm, runVmParams, false, findActiveISODomain(vm.getStoragePoolId())) + .canRunVm( + messages, + fetchStoragePool(vm.getStoragePoolId()), + Collections.<Guid> emptyList(), + null, + null, + DbFacade.getInstance().getVdsGroupDao().get(vm.getVdsGroupId())); + } + + private static Guid findActiveISODomain(Guid storagePoolId) { + return IsoDomainListSyncronizer.getInstance().findActiveISODomain(storagePoolId); } private static StoragePool fetchStoragePool(Guid storagePoolId) { diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java index 6471816..569e48b 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java @@ -11,7 +11,6 @@ import org.apache.commons.lang.StringUtils; import org.ovirt.engine.core.bll.Backend; import org.ovirt.engine.core.bll.ImagesHandler; -import org.ovirt.engine.core.bll.IsoDomainListSyncronizer; import org.ovirt.engine.core.bll.ValidationResult; import org.ovirt.engine.core.bll.interfaces.BackendInternal; import org.ovirt.engine.core.bll.scheduling.SchedulingManager; @@ -60,6 +59,7 @@ private VM vm; private RunVmParams runVmParam; private boolean isInternalExecution; + private Guid activeIsoDomainId; private List<Disk> cachedVmDisks; private List<DiskImage> cachedVmImageDisks; @@ -67,10 +67,12 @@ private List<Network> cachedClusterNetworks; private Set<String> cachedClusterNetworksNames; - public RunVmValidator(VM vm, RunVmParams rumVmParam, boolean isInternalExecution) { + public RunVmValidator(VM vm, RunVmParams rumVmParam, boolean isInternalExecution, + Guid activeIsoDomainId) { this.vm = vm; this.runVmParam = rumVmParam; this.isInternalExecution = isInternalExecution; + this.activeIsoDomainId = activeIsoDomainId; } /** @@ -103,12 +105,12 @@ return validateVmProperties(vm, messages) && - validate(validateBootSequence(vm, runVmParam.getBootSequence(), getVmDisks()), messages) && + validate(validateBootSequence(vm, runVmParam.getBootSequence(), getVmDisks(), activeIsoDomainId), messages) && validate(new VmValidator(vm).vmNotLocked(), messages) && validate(getSnapshotValidator().vmNotDuringSnapshot(vm.getId()), messages) && validate(validateVmStatusUsingMatrix(vm), messages) && validate(validateStoragePoolUp(vm, storagePool, getVmImageDisks()), messages) && - validate(validateIsoPath(vm, runVmParam.getDiskPath(), runVmParam.getFloppyPath()), messages) && + validate(validateIsoPath(vm, runVmParam.getDiskPath(), runVmParam.getFloppyPath(), activeIsoDomainId), messages) && validate(vmDuringInitialization(vm), messages) && validate(validateVdsStatus(vm), messages) && validate(validateStatelessVm(vm, getVmDisks(), runVmParam.getRunAsStateless()), messages) && @@ -162,10 +164,9 @@ return true; } - protected ValidationResult validateBootSequence(VM vm, BootSequence bootSequence, List<Disk> vmDisks) { + protected ValidationResult validateBootSequence(VM vm, BootSequence bootSequence, List<Disk> vmDisks, Guid activeIsoDomainId) { BootSequence boot_sequence = (bootSequence != null) ? bootSequence : vm.getDefaultBootSequence(); - 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 if (boot_sequence == BootSequence.C && vmDisks.isEmpty()) { @@ -174,8 +175,7 @@ // If CD appears as first and there is no ISO in storage // pool/ISO inactive - you cannot run this VM - if (boot_sequence == BootSequence.CD - && getIsoDomainListSyncronizer().findActiveISODomain(storagePoolId) == null) { + if (boot_sequence == BootSequence.CD && activeIsoDomainId == null) { return new ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO); } @@ -240,21 +240,24 @@ new DiskImagesValidator(vmDisks).diskImagesNotLocked() : ValidationResult.VALID; } - protected ValidationResult validateIsoPath(VM vm, String diskPath, String floppyPath) { - if (vm.isAutoStartup() || (StringUtils.isEmpty(diskPath) && StringUtils.isEmpty(floppyPath))) { + protected ValidationResult validateIsoPath(VM vm, String diskPath, String floppyPath, Guid activeIsoDomainId) { + if (vm.isAutoStartup()) { return ValidationResult.VALID; } - Guid storageDomainId = getIsoDomainListSyncronizer().findActiveISODomain(vm.getStoragePoolId()); - if (storageDomainId == null) { + if (StringUtils.isEmpty(vm.getIsoPath()) && StringUtils.isEmpty(diskPath) && StringUtils.isEmpty(floppyPath)) { + return ValidationResult.VALID; + } + + if (activeIsoDomainId == null) { return new ValidationResult(VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO); } - if (!StringUtils.isEmpty(diskPath) && !isRepoImageExists(diskPath, storageDomainId, ImageFileType.ISO)) { + if (!StringUtils.isEmpty(diskPath) && !isRepoImageExists(diskPath, activeIsoDomainId, ImageFileType.ISO)) { return new ValidationResult(VdcBllMessages.ERROR_CANNOT_FIND_ISO_IMAGE_PATH); } - if (!StringUtils.isEmpty(floppyPath) && !isRepoImageExists(floppyPath, storageDomainId, ImageFileType.Floppy)) { + if (!StringUtils.isEmpty(floppyPath) && !isRepoImageExists(floppyPath, activeIsoDomainId, ImageFileType.Floppy)) { return new ValidationResult(VdcBllMessages.ERROR_CANNOT_FIND_FLOPPY_IMAGE_PATH); } @@ -420,10 +423,6 @@ protected StorageDomainDAO getStorageDomainDAO() { return DbFacade.getInstance().getStorageDomainDao(); - } - - protected IsoDomainListSyncronizer getIsoDomainListSyncronizer() { - return IsoDomainListSyncronizer.getInstance(); } protected VmPropertiesUtils getVmPropertiesUtils() { diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/RunVmValidatorTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/RunVmValidatorTest.java index 87b4c5a..f44f6e1 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/RunVmValidatorTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/RunVmValidatorTest.java @@ -21,7 +21,6 @@ import org.junit.runner.RunWith; import org.mockito.Spy; import org.mockito.runners.MockitoJUnitRunner; -import org.ovirt.engine.core.bll.IsoDomainListSyncronizer; import org.ovirt.engine.core.bll.ValidationResult; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; import org.ovirt.engine.core.common.businessentities.BootSequence; @@ -55,7 +54,6 @@ @Before public void setup() { mockVmPropertiesUtils(); - mockIsoDomainListSyncronizer(); } @Test @@ -100,7 +98,7 @@ @Test public void testVmFailNoDisks() { - validateResult(runVmValidator.validateBootSequence(new VM(), null, new ArrayList<Disk>()), + validateResult(runVmValidator.validateBootSequence(new VM(), null, new ArrayList<Disk>(), null), false, VdcBllMessages.VM_CANNOT_RUN_FROM_DISK_WITHOUT_DISK); } @@ -109,23 +107,21 @@ public void testVmWithDisks() { List<Disk> disks = new ArrayList<Disk>(); disks.add(new DiskImage()); - validateResult(runVmValidator.validateBootSequence(new VM(), null, disks), + validateResult(runVmValidator.validateBootSequence(new VM(), null, disks, null), true, null); } @Test public void testNoIsoDomain() { - validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.CD, new ArrayList<Disk>()), + validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.CD, new ArrayList<Disk>(), null), false, VdcBllMessages.VM_CANNOT_RUN_FROM_CD_WITHOUT_ACTIVE_STORAGE_DOMAIN_ISO); } @Test public void testNoDiskBootFromIsoDomain() { - IsoDomainListSyncronizer mock = mockIsoDomainListSyncronizer(); - doReturn(Guid.newGuid()).when(mock).findActiveISODomain(any(Guid.class)); - validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.CD, new ArrayList<Disk>()), + validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.CD, new ArrayList<Disk>(), Guid.newGuid()), true, null); } @@ -135,7 +131,7 @@ VmNicDao dao = mock(VmNicDao.class); doReturn(new ArrayList<VmNic>()).when(dao).getAllForVm(any(Guid.class)); doReturn(dao).when(runVmValidator).getVmNicDao(); - validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.N, new ArrayList<Disk>()), + validateResult(runVmValidator.validateBootSequence(new VM(), BootSequence.N, new ArrayList<Disk>(), null), false, VdcBllMessages.VM_CANNOT_RUN_FROM_NETWORK_WITHOUT_NETWORK); } @@ -287,22 +283,9 @@ return utils; } - private IsoDomainListSyncronizer mockIsoDomainListSyncronizer() { - IsoDomainListSyncronizer isoDomainListSyncronizer = mock(MockIsoDomainListSyncronizer.class); - doReturn(isoDomainListSyncronizer).when(runVmValidator).getIsoDomainListSyncronizer(); - return isoDomainListSyncronizer; - } - private static void validateResult(ValidationResult validationResult, boolean isValid, VdcBllMessages message) { assertEquals(isValid, validationResult.isValid()); assertEquals(message, validationResult.getMessage()); - } - - class MockIsoDomainListSyncronizer extends IsoDomainListSyncronizer { - @Override - protected void init() { - // empty impl - } } } -- To view, visit http://gerrit.ovirt.org/21206 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic1bc81686be01088aa2dde5c61e4b83c96643f73 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
