Maor Lipchuk has uploaded a new change for review. Change subject: core: Re-ordering validations at create snapshot command. ......................................................................
core: Re-ordering validations at create snapshot command. Some validations in the create snapshot command were only executed if the VM had disks, that caused the command to skip validations for diskless VM. The fix was to move the relevant validations to be executed regardless to the VM disks. Also added a test for create snapshot from VM. Bug-Url: https://bugzilla.redhat.com/903245 Change-Id: I58bfc73383ce5ca06bf670a8f63b5563da9caa02 Signed-off-by: Maor Lipchuk <[email protected]> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java A backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java 2 files changed, 290 insertions(+), 19 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/64/14964/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 affbf5d..ec0cdf1 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 @@ -64,7 +64,7 @@ super(parameters); parameters.setEntityId(getVmId()); setSnapshotName(parameters.getDescription()); - setStoragePoolId(getVm().getStoragePoolId()); + setStoragePoolId(getVm() != null ? getVm().getStoragePoolId() : null); } @Override @@ -80,7 +80,7 @@ * Filter all allowed snapshot disks. * @return list of disks to be snapshot. */ - private List<DiskImage> getDisksList() { + protected List<DiskImage> getDisksList() { if (selectedActiveDisks == null) { selectedActiveDisks = ImagesHandler.filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(getVmId()), true, @@ -306,33 +306,55 @@ @Override protected boolean canDoAction() { - VmValidator vmValidator = new VmValidator(getVm()); + // Initialize VM validator. + VmValidator vmValidator = createVmValidator(); + SnapshotsValidator snapshotValidator = createSnapshotValidator(); + StoragePoolValidator spValidator = createStoragePoolValidator(); + boolean result = validateVM(vmValidator); + result = validate(spValidator.isUp()) + && validate(snapshotValidator.vmNotDuringSnapshot(getVmId())) + && validate(snapshotValidator.vmNotInPreview(getVmId())) + && validate(vmValidator.vmNotDuringMigration()) + && validate(vmValidator.vmNotRunningStateless()) + && validate(vmValidator.vmNotIlegal()) + && validate(vmValidator.vmNotLocked()); + + // Validate VM with disks. List<DiskImage> disksList = getDisksList(); if (result && disksList.size() > 0) { - StoragePoolValidator spValidator = new StoragePoolValidator(getStoragePool()); - SnapshotsValidator snapshotValidator = new SnapshotsValidator(); - MultipleStorageDomainsValidator sdValidator = - new MultipleStorageDomainsValidator(getVm().getStoragePoolId(), - ImagesHandler.getAllStorageIdsForImageIds(disksList)); - DiskImagesValidator diskImagesValidator = new DiskImagesValidator(disksList); - result = validate(spValidator.isUp()) - && validate(snapshotValidator.vmNotDuringSnapshot(getVmId())) - && validate(snapshotValidator.vmNotInPreview(getVmId())) - && validate(vmValidator.vmNotDuringMigration()) - && validate(vmValidator.vmNotRunningStateless()) - && validate(vmValidator.vmNotIlegal()) - && validate(diskImagesValidator.diskImagesNotLocked()) + MultipleStorageDomainsValidator sdValidator = createMultipleStorageDomainsValidator(disksList); + DiskImagesValidator diskImagesValidator = createDiskImageValidator(disksList); + result = validate(diskImagesValidator.diskImagesNotLocked()) && validate(diskImagesValidator.diskImagesNotIllegal()) - && validate(vmValidator.vmNotLocked()) && validate(sdValidator.allDomainsExistAndActive()) && validate(sdValidator.allDomainsWithinThresholds()); } - return result; } - private boolean validateVM(VmValidator vmValidator) { + protected StoragePoolValidator createStoragePoolValidator() { + return new StoragePoolValidator(getStoragePool()); + } + + protected SnapshotsValidator createSnapshotValidator() { + return new SnapshotsValidator(); + } + + protected DiskImagesValidator createDiskImageValidator(List<DiskImage> disksList) { + return new DiskImagesValidator(disksList); + } + + protected MultipleStorageDomainsValidator createMultipleStorageDomainsValidator(List<DiskImage> disksList) { + return new MultipleStorageDomainsValidator(getVm().getStoragePoolId(), + ImagesHandler.getAllStorageIdsForImageIds(disksList)); + } + + protected VmValidator createVmValidator() { + return new VmValidator(getVm()); + } + + protected boolean validateVM(VmValidator vmValidator) { return canDoSnapshot(getVm()) && validate(vmValidator.vmNotSavingRestoring()); } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java new file mode 100644 index 0000000..abc4b76 --- /dev/null +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommandTest.java @@ -0,0 +1,249 @@ +package org.ovirt.engine.core.bll; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.spy; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; +import org.ovirt.engine.core.bll.storage.StoragePoolValidator; +import org.ovirt.engine.core.bll.validator.DiskImagesValidator; +import org.ovirt.engine.core.bll.validator.MultipleStorageDomainsValidator; +import org.ovirt.engine.core.bll.validator.VmValidator; +import org.ovirt.engine.core.common.action.CreateAllSnapshotsFromVmParameters; +import org.ovirt.engine.core.common.businessentities.DiskImage; +import org.ovirt.engine.core.compat.Guid; +import org.ovirt.engine.core.dal.VdcBllMessages; +import org.ovirt.engine.core.dao.SnapshotDao; +import org.ovirt.engine.core.dao.VmDAO; + +/** A test case for the {@link CreateAllSnapshotsFromVmCommand} class. */ +@RunWith(MockitoJUnitRunner.class) +public class CreateAllSnapshotsFromVmCommandTest { + private CreateAllSnapshotsFromVmCommand<CreateAllSnapshotsFromVmParameters> cmd; + + @Mock + private SnapshotDao snapshotDao; + + @Mock + private VmDAO vmDao; + + @Mock + private VmValidator vmValidator; + + @Mock + private SnapshotsValidator snapshotValidator; + + @Mock + private DiskImagesValidator diskImagesValidator; + + @Mock + private MultipleStorageDomainsValidator multipleStorageDomainsValidator; + + @Mock + private StoragePoolValidator storagePoolValidator; + + @SuppressWarnings("unchecked") + @Before + public void setUp() { + CreateAllSnapshotsFromVmParameters params = new CreateAllSnapshotsFromVmParameters(Guid.NewGuid(), ""); + cmd = spy(new CreateAllSnapshotsFromVmCommand<CreateAllSnapshotsFromVmParameters>(params)); + doReturn(vmValidator).when(cmd).createVmValidator(); + doReturn(snapshotValidator).when(cmd).createSnapshotValidator(); + doReturn(storagePoolValidator).when(cmd).createStoragePoolValidator(); + doReturn(diskImagesValidator).when(cmd).createDiskImageValidator(any(List.class)); + doReturn(multipleStorageDomainsValidator).when(cmd).createMultipleStorageDomainsValidator(any(List.class)); + } + + @Test + public void testPositiveCanDoActionWithNoDisks() { + setUpGeneralValidations(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertTrue(cmd.canDoAction()); + assertTrue(cmd.getReturnValue().getCanDoActionMessages().isEmpty()); + } + + @Test + public void testStoragePoolIsNotUp() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND)).when(storagePoolValidator) + .isUp(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_IMAGE_REPOSITORY_NOT_FOUND.name())); + } + + @Test + public void testVmDuringSnaoshot() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_DURING_SNAPSHOT)).when(snapshotValidator) + .vmNotDuringSnapshot(any(Guid.class)); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_DURING_SNAPSHOT.name())); + } + + @Test + public void testVmInPreview() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_IN_PREVIEW)).when(snapshotValidator) + .vmNotInPreview(any(Guid.class)); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_VM_IN_PREVIEW.name())); + } + + @Test + public void testVmDuringMigration() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_MIGRATION_IN_PROGRESS)).when(vmValidator) + .vmNotDuringMigration(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_MIGRATION_IN_PROGRESS.name())); + } + + @Test + public void testVmRunningStateless() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_RUNNING_STATELESS)).when(vmValidator) + .vmNotRunningStateless(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_VM_RUNNING_STATELESS.name())); + } + + @Test + public void testVmIllegal() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_IMAGE_IS_ILLEGAL)).when(vmValidator) + .vmNotIlegal(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_VM_IMAGE_IS_ILLEGAL.name())); + } + + @Test + public void testVmLocked() { + setUpGeneralValidations(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED)).when(vmValidator) + .vmNotLocked(); + doReturn(getEmptyDiskList()).when(cmd).getDisksList(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_VM_IS_LOCKED.name())); + } + + @Test + public void testPositiveCanDoActionWithDisks() { + setUpGeneralValidations(); + setUpDiskValidations(); + doReturn(getFullDiskList()).when(cmd).getDisksList(); + assertTrue(cmd.canDoAction()); + assertTrue(cmd.getReturnValue().getCanDoActionMessages().isEmpty()); + } + + @Test + public void testImagesLocked() { + setUpGeneralValidations(); + setUpDiskValidations(); + doReturn(getFullDiskList()).when(cmd).getDisksList(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED)).when(diskImagesValidator) + .diskImagesNotLocked(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED.name())); + } + + @Test + public void testImagesIllegal() { + setUpGeneralValidations(); + setUpDiskValidations(); + doReturn(getFullDiskList()).when(cmd).getDisksList(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_ILLEGAL)).when(diskImagesValidator) + .diskImagesNotIllegal(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISKS_ILLEGAL.name())); + } + + @Test + public void testAllDomainsExistAndActive() { + setUpGeneralValidations(); + setUpDiskValidations(); + doReturn(getFullDiskList()).when(cmd).getDisksList(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST)).when(multipleStorageDomainsValidator) + .allDomainsExistAndActive(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST.name())); + } + + @Test + public void testAllDomainsWithinTheshold() { + setUpGeneralValidations(); + setUpDiskValidations(); + doReturn(getFullDiskList()).when(cmd).getDisksList(); + doReturn(new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN)).when(multipleStorageDomainsValidator) + .allDomainsExistAndActive(); + assertFalse(cmd.canDoAction()); + assertTrue(cmd.getReturnValue() + .getCanDoActionMessages() + .contains(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN.name())); + } + + private void setUpDiskValidations() { + doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotLocked(); + doReturn(ValidationResult.VALID).when(diskImagesValidator).diskImagesNotIllegal(); + doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsExistAndActive(); + doReturn(ValidationResult.VALID).when(multipleStorageDomainsValidator).allDomainsWithinThresholds(); + } + + private void setUpGeneralValidations() { + doReturn(Boolean.TRUE).when(cmd).validateVM(vmValidator); + doReturn(ValidationResult.VALID).when(storagePoolValidator).isUp(); + doReturn(ValidationResult.VALID).when(vmValidator).vmNotDuringMigration(); + doReturn(ValidationResult.VALID).when(vmValidator).vmNotRunningStateless(); + doReturn(ValidationResult.VALID).when(vmValidator).vmNotIlegal(); + doReturn(ValidationResult.VALID).when(vmValidator).vmNotLocked(); + doReturn(ValidationResult.VALID).when(snapshotValidator).vmNotDuringSnapshot(any(Guid.class)); + doReturn(ValidationResult.VALID).when(snapshotValidator).vmNotInPreview(any(Guid.class)); + } + + private static List<DiskImage> getEmptyDiskList() { + List<DiskImage> diskList = new ArrayList<>(); + return diskList; + } + + private static List<DiskImage> getFullDiskList() { + List<DiskImage> diskList = new ArrayList<>(); + DiskImage newDiskImage = new DiskImage(); + diskList.add(newDiskImage); + return diskList; + } +} -- To view, visit http://gerrit.ovirt.org/14964 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I58bfc73383ce5ca06bf670a8f63b5563da9caa02 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
