Maor Lipchuk has posted comments on this change.
Change subject: core: Add storage space validation to removeSnapshot
......................................................................
Patch Set 3: (21 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java
Line 188: }
Line 189:
Line 190: if (!isEnoughSpaceToMergeSnapshots()) {
Line 191: return false;
Line 192: }
Do not agree, it could be easily confuse developers which will want to
introduce new validation for Vm's with no images.
Line 193: }
Line 194:
Line 195: return true;
Line 196: }
Line 196: }
Line 197:
Line 198: /**
Line 199: * Validates if each storage domain has enough free space to
perform removeSnapshot. <BR/>
Line 200: * The remove snapshot logic in VDSM includes, creating a new
temporary volume which might be large as the disk
done
Line 201: * actual size. <BR/>
Line 202: * Hence, as part of the validation, we summarize all the disks
virtual sizes, for each storage domain.
Line 203: *
Line 204: * @return True if there is enough space in all relevant storage
domains. False otherwise.
Line 198: /**
Line 199: * Validates if each storage domain has enough free space to
perform removeSnapshot. <BR/>
Line 200: * The remove snapshot logic in VDSM includes, creating a new
temporary volume which might be large as the disk
Line 201: * actual size. <BR/>
Line 202: * Hence, as part of the validation, we summarize all the disks
virtual sizes, for each storage domain.
done
Line 203: *
Line 204: * @return True if there is enough space in all relevant storage
domains. False otherwise.
Line 205: */
Line 206: protected boolean isEnoughSpaceToMergeSnapshots() {
Line 204: * @return True if there is enough space in all relevant storage
domains. False otherwise.
Line 205: */
Line 206: protected boolean isEnoughSpaceToMergeSnapshots() {
Line 207: for (final Guid storageDomainId :
getStorageToDiskMap().keySet()) {
Line 208: List<DiskImage> diskImages =
getStorageToDiskMap().get(storageDomainId);
done
Line 209: long sizeRequested = 0;
Line 210: for (DiskImage diskImage : diskImages) {
Line 211: sizeRequested += diskImage.getActualSize();
Line 212: }
Line 212: }
Line 213: storage_domains storageDomain =
Line 214:
getStorageDomainDAO().getForStoragePool(storageDomainId, getStoragePoolId());
Line 215: if (!validate(new
StorageDomainValidator(storageDomain).isDomainHasSpaceForRequest(sizeRequested,
false))) {
Line 216: return false;
Do not agree, It's in the loop, return will create a regression
Line 217: }
Line 218: }
Line 219: return true;
Line 220: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java
Line 59: return isDomainHasSpaceForRequest(requestedSize, true);
Line 60: }
Line 61:
Line 62: public ValidationResult isDomainHasSpaceForRequest(final long
requestedSize, final boolean useThresHold) {
Line 63: long size = useThresHold ? getLowDiskSpaceThreshold() : 0;
done
Line 64: if (storageDomain.getavailable_disk_size() != null &&
Line 65: storageDomain.getavailable_disk_size() - requestedSize
< size) {
Line 66: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_DISK_SPACE_LOW_ON_TARGET_STORAGE_DOMAIN,
Line 67: storageName());
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandTest.java
Line 142: }
Line 143:
Line 144: @Test
Line 145: public void testEnoughSpaceToMergeSnapshotsWithMultipleDisk() {
Line 146: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
I think it might, will give it a shot.
Line 147: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 148: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID);
Line 149: assertTrue("Validation should succeed. Free space minus
threshold should be bigger then summarize all disks size",
Line 150: cmd.isEnoughSpaceToMergeSnapshots());
Line 151: }
Line 152:
Line 153: @Test
Line 154: public void testNotEnoughSpaceToMergeSnapshotsWithMultipleDisk() {
Line 155: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
same as above :-)
Line 156: System.out.println(imagesDisks);
Line 157: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 158: mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID);
Line 159: assertFalse("Validation should fail. Free space minus
threshold should be smaller then summarize all disks size",
Line 152:
Line 153: @Test
Line 154: public void testNotEnoughSpaceToMergeSnapshotsWithMultipleDisk() {
Line 155: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
Line 156: System.out.println(imagesDisks);
oops, how did that get here :-)
Line 157: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 158: mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID);
Line 159: assertFalse("Validation should fail. Free space minus
threshold should be smaller then summarize all disks size",
Line 160: cmd.isEnoughSpaceToMergeSnapshots());
Line 161: }
Line 162:
Line 163: @Test
Line 164: public void
testEnoughSpaceToMergeSnapshotsWithMultipleDiskAndDomains() {
Line 165: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
same as above
Line 166: imagesDisks.addAll(mockMultipleSourceImagesForDomain(4,
STORAGE_DOMAIN_ID2, 4));
Line 167: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 168: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID);
Line 169: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2);
Line 172: }
Line 173:
Line 174: @Test
Line 175: public void
testNotEnoughForMultipleDiskAndDomainsFirstDomainFails() {
Line 176: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
same as above.
Line 177: imagesDisks.addAll(mockMultipleSourceImagesForDomain(4,
STORAGE_DOMAIN_ID2, 4));
Line 178: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 179: mockStorageDomainDAOGetForStoragePool(15, STORAGE_DOMAIN_ID);
Line 180: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID2);
Line 182: cmd.isEnoughSpaceToMergeSnapshots());
Line 183: }
Line 184:
Line 185: @Test
Line 186: public void
testNotEnoughForMultipleDiskAndDomainsSecondDomainFails() {
Yes, different storage sizes, see also test names
Line 187: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
Line 188: imagesDisks.addAll(mockMultipleSourceImagesForDomain(4,
STORAGE_DOMAIN_ID2, 4));
Line 189: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 190: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID);
Line 183: }
Line 184:
Line 185: @Test
Line 186: public void
testNotEnoughForMultipleDiskAndDomainsSecondDomainFails() {
Line 187: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
same as above.
Line 188: imagesDisks.addAll(mockMultipleSourceImagesForDomain(4,
STORAGE_DOMAIN_ID2, 4));
Line 189: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 190: mockStorageDomainDAOGetForStoragePool(22, STORAGE_DOMAIN_ID);
Line 191: mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2);
Line 194: }
Line 195:
Line 196: @Test
Line 197: public void
testNotEnoughForMultipleDiskAndDomainsAllDomainsFail() {
Line 198: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
same as above.
Line 199: imagesDisks.addAll(mockMultipleSourceImagesForDomain(4,
STORAGE_DOMAIN_ID2, 4));
Line 200: doReturn(imagesDisks).when(cmd).getSourceImages();
Line 201: mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID);
Line 202: mockStorageDomainDAOGetForStoragePool(10, STORAGE_DOMAIN_ID2);
Line 231: private Guid mockSourceImage() {
Line 232: Guid imageId = Guid.NewGuid();
Line 233: DiskImage image = new DiskImage();
Line 234: image.setImageId(imageId);
Line 235: ArrayList<Guid> list = new ArrayList<Guid>();
No, since the API in the image BE does not allow it.
it be supported with GWT.
Line 236: list.add(STORAGE_DOMAIN_ID);
Line 237: image.setstorage_ids(list);
Line 238: image.setActualSize(4);
Line 239: image.setsize(40);
Line 235: ArrayList<Guid> list = new ArrayList<Guid>();
Line 236: list.add(STORAGE_DOMAIN_ID);
Line 237: image.setstorage_ids(list);
Line 238: image.setActualSize(4);
Line 239: image.setsize(40);
done for actual size (size is nice to have but doesn't used much)
Line 240:
doReturn(Collections.singletonList(image)).when(cmd).getSourceImages();
Line 241: return imageId;
Line 242: }
Line 243:
Line 242: }
Line 243:
Line 244: /** Mocks a call to {@link
RemoveSnapshotCommand#getSourceImages()} and returns list of images */
Line 245: private ArrayList<DiskImage>
mockMultipleSourceImagesForDomain(int numberOfDisks, Guid storageDomainId, int
actualDiskSize) {
Line 246: ArrayList<DiskImage> listDisks = new ArrayList<DiskImage>();
No, since the API in the image BE does not allow it.
it be supported with GWT.
Line 247: for (int index=0; index < numberOfDisks; index++) {
Line 248: Guid imageId = Guid.NewGuid();
Line 249: DiskImage image = new DiskImage();
Line 250: image.setImageId(imageId);
Line 256: }
Line 257: return listDisks;
Line 258: }
Line 259:
Line 260: protected storage_domains createStorageDomain(int availableSpace,
Guid storageDomainId) {
done.
Line 261: storage_domains sd = new storage_domains();
Line 262: sd.setstorage_domain_type(StorageDomainType.Master);
Line 263: sd.setstatus(StorageDomainStatus.Active);
Line 264: sd.setavailable_disk_size(availableSpace);
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/StorageDomainValidatorTest.java
Line 79:
Line 80: @Test
Line 81: public void testDomainWithEnoughSpaceForRequestWithoutThreshold() {
Line 82: validator = new StorageDomainValidator(mockStorageDomain(12,
748, StorageType.NFS));
Line 83: assertTrue("Domain should have enough space for request.",
validator.isDomainHasSpaceForRequest(12, false)
No they are not, this will check 12GB and the other one will check 13GB
Line 84: .isValid());
Line 85: }
Line 86:
Line 87: @Test
....................................................
Commit Message
Line 9: Today there is not validation in RemoveSnapshotCommand to ensure there
is enough
Line 10: space on the domain to merge the disk snapshots.
Line 11: This space is needed on remove snapshot process since VDSM creates a
new
Line 12: temporary volume, as part of the merge process, which might be large as
Line 13: the image actual size.
done
Line 14:
Line 15: The proposed fix introduces a new validation in the command which
Line 16: validates if each storage domain has enough free space to perform
removeSnapshot.
Line 17:
Line 12: temporary volume, as part of the merge process, which might be large as
Line 13: the image actual size.
Line 14:
Line 15: The proposed fix introduces a new validation in the command which
Line 16: validates if each storage domain has enough free space to perform
removeSnapshot.
done
Line 17:
Line 18: Change-Id: Ifbebc5af8e7489bccfa5d8e79695fe21c5371ec9
Line 19: Bug-Url: https://bugzilla.redhat.com/864892
--
To view, visit http://gerrit.ovirt.org/11849
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifbebc5af8e7489bccfa5d8e79695fe21c5371ec9
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
