Vered Volansky has uploaded a new change for review. Change subject: core: MoveOrCopyDiskCommand space verification ......................................................................
core: MoveOrCopyDiskCommand space verification This is a part in a series of patches intended to fix storage space allocation validation throughout the system (see bz). Added hasSpaceForClonedDisk(s) in StorageDomainValidator. Applied use in MoveOrCopyDiskCommand (former use is buggy). Change-Id: I951aeb531cb7ff7aaf3f1d50fc55100b6a806059 Bug-url: https://bugzilla.redhat.com/960934 Signed-off-by: Vered Volansky <vvola...@redhat.com> --- M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java M backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java M backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java 6 files changed, 133 insertions(+), 36 deletions(-) git pull ssh://gerrit.ovirt.org:29418/ovirt-engine refs/changes/47/22447/1 diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java index 4da4fe8..ffedd6d 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java @@ -147,27 +147,9 @@ * @return */ protected boolean validateSpaceRequirements() { - if (!isStorageDomainSpaceWithinThresholds()) { - return false; - } - - getImage().getSnapshots().addAll(getAllImageSnapshots()); - if (!isDomainHasStorageSpaceForRequest()) { - return false; - } - return true; - } - - private boolean isDomainHasStorageSpaceForRequest() { - return validate(new StorageDomainValidator(getStorageDomain()).isDomainHasSpaceForRequest(Math.round(getImage().getActualDiskWithSnapshotsSize()))); - } - - protected boolean isStorageDomainSpaceWithinThresholds() { - return validate(new StorageDomainValidator(getStorageDomain()).isDomainWithinThresholds()); - } - - protected List<DiskImage> getAllImageSnapshots() { - return ImagesHandler.getAllImageSnapshots(getImage().getImageId(), getImage().getImageTemplateId()); + StorageDomainValidator storageDomainValidator = createStorageDomainValidator(); + return validate(storageDomainValidator.isDomainWithinThresholds()) && + validate(storageDomainValidator.hasSpaceForClonedDisk(getImage())); } protected boolean checkIfNeedToBeOverride() { @@ -206,7 +188,7 @@ * @return */ protected boolean checkCanBeMoveInVm() { - return validate(new DiskValidator(getImage()).isDiskPluggedToVmsThatAreNotDown(false, getVmsWithVmDeviceInfoForDiskId())); + return validate(createDiskValidator().isDiskPluggedToVmsThatAreNotDown(false, getVmsWithVmDeviceInfoForDiskId())); } /** @@ -335,7 +317,6 @@ /** * The following method will determine if a provided vm/template exists - * @param retValue * @return */ private boolean canFindVmOrTemplate() { @@ -440,4 +421,11 @@ return jobProperties; } + protected StorageDomainValidator createStorageDomainValidator() { + return new StorageDomainValidator(getStorageDomain()); + } + + protected DiskValidator createDiskValidator() { + return new DiskValidator(getImage()); + } } diff --git a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java index 4d92435..c37071d 100644 --- a/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java +++ b/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java @@ -3,8 +3,10 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import org.ovirt.engine.core.bll.ImagesHandler; import org.ovirt.engine.core.bll.ValidationResult; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.StorageDomain; @@ -102,6 +104,34 @@ return validateRequiredSpace(availableSize, totalSizeForDisks); } + public ValidationResult hasSpaceForClonedDisks(Collection<DiskImage> diskImages) { + double availableSize = storageDomain.getAvailableDiskSizeInBytes(); + double totalSizeForDisks = 0.0; + + for (DiskImage diskImage : diskImages) { + double diskCapacity = diskImage.getSize(); + double sizeForDisk = diskCapacity; + + if ((storageDomain.getStorageType().isFileDomain() && diskImage.getVolumeType() == VolumeType.Sparse) || + storageDomain.getStorageType().isBlockDomain() && diskImage.getVolumeFormat() == VolumeFormat.COW) { + addImageSnapshots(diskImage); + double usedSapce = diskImage.getActualDiskWithSnapshotsSizeInBytes(); + sizeForDisk = Math.min(diskCapacity, usedSapce); + } + + if (diskImage.getVolumeFormat() == VolumeFormat.COW) { + sizeForDisk = Math.ceil(QCOW_OVERHEAD_FACTOR * sizeForDisk); + } + totalSizeForDisks += sizeForDisk; + } + + return validateRequiredSpace(availableSize, totalSizeForDisks); + } + + public ValidationResult hasSpaceForClonedDisk(DiskImage diskImage) { + return hasSpaceForClonedDisks(Collections.singleton(diskImage)); + } + public ValidationResult hasSpaceForNewDisk(DiskImage diskImage) { return hasSpaceForNewDisks(Collections.singleton(diskImage)); } @@ -144,4 +174,9 @@ } return map; } + + private void addImageSnapshots(DiskImage diskImage) { + List<DiskImage> snapshots = ImagesHandler.getAllImageSnapshots(diskImage.getImageId(), diskImage.getImageTemplateId()); + diskImage.getSnapshots().addAll(snapshots); + } } diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java index de3ad44..a7cef3e 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/AddDiskToVmCommandTest.java @@ -22,6 +22,7 @@ import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; import org.ovirt.engine.core.bll.snapshots.SnapshotsValidator; +import org.ovirt.engine.core.bll.validator.DiskImagesValidator; import org.ovirt.engine.core.bll.validator.DiskValidator; import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.action.AddDiskParameters; diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java index 7d0c5b4..9720733 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommandTest.java @@ -4,6 +4,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.any; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig; @@ -17,7 +18,10 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.runners.MockitoJUnitRunner; +import org.ovirt.engine.core.bll.validator.DiskValidator; +import org.ovirt.engine.core.bll.validator.StorageDomainValidator; import org.ovirt.engine.core.common.action.MoveOrCopyImageGroupParameters; +import org.ovirt.engine.core.common.businessentities.Disk; import org.ovirt.engine.core.common.businessentities.DiskImage; import org.ovirt.engine.core.common.businessentities.ImageOperation; import org.ovirt.engine.core.common.businessentities.ImageStatus; @@ -99,7 +103,7 @@ } @Test - public void canDoActionCanNotFindTemplet() throws Exception { + public void canDoActionCanNotFindTemplate() throws Exception { initializeCommand(ImageOperation.Copy); initTemplateDiskImage(); doReturn(null).when(command).getTemplateForImage(); @@ -115,7 +119,7 @@ destStorageId = srcStorageId; initializeCommand(ImageOperation.Move); initVmDiskImage(); - initVm(); + initVmForFail(); initSrcStorageDomain(); assertFalse(command.canDoAction()); assertTrue(command.getReturnValue() @@ -127,7 +131,7 @@ public void canDoActionVmIsNotDown() throws Exception { initializeCommand(ImageOperation.Move); initVmDiskImage(); - initVm(); + initVmForFail(); initSrcStorageDomain(); initDestStorageDomain(); doReturn(vmDeviceDao).when(command).getVmDeviceDAO(); @@ -141,7 +145,7 @@ public void canDoActionDiskIsLocked() throws Exception { initializeCommand(ImageOperation.Move); initVmDiskImage(); - initVm(); + initVmForFail(); command.getImage().setImageStatus(ImageStatus.LOCKED); doReturn(vmDeviceDao).when(command).getVmDeviceDAO(); assertFalse(command.canDoAction()); @@ -162,7 +166,40 @@ VdcBllMessages.VM_TEMPLATE_IMAGE_IS_LOCKED.toString())); } - protected void initVm() { + @Test + public void canDoActionNotEnoughSpace() throws Exception { + initializeCommand(ImageOperation.Move); + initVmForSpace(); + initVmDiskImage(); + initSrcStorageDomain(); + initDestStorageDomain(); + doReturn(mockStorageDomainValidatorWithoutSpace()).when(command).createStorageDomainValidator(); + CanDoActionTestUtils.runAndAssertCanDoActionFailure(command, VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN); + } + + @Test + public void canDoActionEnoughSpace() throws Exception { + initializeCommand(ImageOperation.Move); + initVmForSpace(); + initVmDiskImage(); + initSrcStorageDomain(); + initDestStorageDomain(); + //when(diskImageDao.getSnapshotById(any(Guid.class))).thenReturn(null); + doReturn(mockStorageDomainValidatorWithSpace()).when(command).createStorageDomainValidator(); + CanDoActionTestUtils.runAndAssertCanDoActionSuccess(command); + } + + protected void initVmForSpace() { + VM vm = new VM(); + vm.setStatus(VMStatus.Down); + doReturn(vmDao).when(command).getVmDAO(); + when(vmDao.get(any(Guid.class))).thenReturn(vm); + VmDevice device = new VmDevice(); + List<Pair<VM, VmDevice>> vmList = Collections.singletonList(new Pair<>(vm, device)); + when(vmDao.getVmsWithPlugInfo(any(Guid.class))).thenReturn(vmList); + } + + protected void initVmForFail() { VM vm = new VM(); vm.setStatus(VMStatus.Down); doReturn(vmDao).when(command).getVmDAO(); @@ -186,6 +223,26 @@ when(vmDao.getVmsWithPlugInfo(any(Guid.class))).thenReturn(vmList); } + private static StorageDomainValidator mockStorageDomainValidatorWithoutSpace() { + StorageDomainValidator storageDomainValidator = mockStorageDomainValidator(); + when(storageDomainValidator.hasSpaceForClonedDisk(any(DiskImage.class))).thenReturn( + new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN)); + return storageDomainValidator; + } + + private static StorageDomainValidator mockStorageDomainValidatorWithSpace() { + StorageDomainValidator storageDomainValidator = mockStorageDomainValidator(); + when(storageDomainValidator.hasSpaceForClonedDisk(any(DiskImage.class))).thenReturn(ValidationResult.VALID); + return storageDomainValidator; + } + + private static StorageDomainValidator mockStorageDomainValidator() { + StorageDomainValidator storageDomainValidator = mock(StorageDomainValidator.class); + when(storageDomainValidator.isDomainExistAndActive()).thenReturn(ValidationResult.VALID); + when(storageDomainValidator.isDomainWithinThresholds()).thenReturn(ValidationResult.VALID); + return storageDomainValidator; + } + private void initSrcStorageDomain() { StorageDomain stDomain = new StorageDomain(); stDomain.setStatus(StorageDomainStatus.Active); @@ -207,12 +264,10 @@ destStorageId, operation))); - // Spy away the storage domain checker methods - doReturn(true).when(command).isStorageDomainSpaceWithinThresholds(); - - // Spy away the image handler methods + // Spy away the image handler method doReturn(true).when(command).checkImageConfiguration(); - doReturn(Collections.emptyList()).when(command).getAllImageSnapshots(); + + doReturn(mockStorageDomainValidatorWithSpace()).when(command).createStorageDomainValidator(); doReturn(false).when(command).acquireLock(); } @@ -229,6 +284,12 @@ when(diskImageDao.get(any(Guid.class))).thenReturn(diskImage); } + private DiskValidator spyDiskValidator(Disk disk) { + DiskValidator diskValidator = spy(new DiskValidator(disk)); + doReturn(diskValidator).when(command).createDiskValidator(); + return diskValidator; + } + /** * The following class is created in order to allow to use a mock diskImageDao in constructor */ diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java index 9d8ab62..00a6833 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorFreeSpaceTest.java @@ -22,13 +22,17 @@ public class StorageDomainValidatorFreeSpaceTest { private DiskImage disk; private StorageDomain sd; + + private boolean isValidForCloned; private boolean isValidForNew; public StorageDomainValidatorFreeSpaceTest(DiskImage disk, - StorageDomain sd, - boolean isValidForNew) { + StorageDomain sd, + boolean isValidForCloned, + boolean isValidForNew) { this.disk = disk; this.sd = sd; + this.isValidForCloned = isValidForCloned; this.isValidForNew = isValidForNew; } @@ -55,6 +59,7 @@ sd.setAvailableDiskSize(107); // GB params.add(new Object[] { disk, sd, + volumeFormat == VolumeFormat.RAW && volumeType == VolumeType.Sparse, volumeFormat == VolumeFormat.COW || volumeType == VolumeType.Sparse }); } } @@ -64,6 +69,13 @@ return params; } + public void testValidateClonedDisk() { + StorageDomainValidator sdValidator = new StorageDomainValidator(sd); + assertEquals(disk.getVolumeFormat() + ", " + disk.getVolumeType() + ", " + sd.getStorageType(), + isValidForCloned, + sdValidator.hasSpaceForClonedDisk(disk).isValid()); + } + @Test public void testValidateNewDisk() { StorageDomainValidator sdValidator = new StorageDomainValidator(sd); diff --git a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java index 70a94bd..e210aaf 100644 --- a/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java +++ b/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java @@ -16,7 +16,7 @@ /** * A test case for the {@link StorageDomainValidator} class. - * The hasSpaceForNewDisk() method is covered separately in + * The hasSpaceForClonedDisk() and hasSpaceForNewDisk() methods are covered separately in * {@link StorageDomainValidatorFreeSpaceTest}. */ public class StorageDomainValidatorTest { -- To view, visit http://gerrit.ovirt.org/22447 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I951aeb531cb7ff7aaf3f1d50fc55100b6a806059 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vered Volansky <vvola...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
