Allon Mureinik has posted comments on this change.
Change subject: core: Add storage space validation to removeSnapshot
......................................................................
Patch Set 3: (17 inline comments)
Are you sure this calculation is OK?
off the top of my head, I'd guess you only need as much space as the two
volumes you're merging.
Also, see inline.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java
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
s/includes,/include/ (remove the comma)
s/disk/disk's/
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.
s/summarize/sum/
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);
Use entrySey() instead of keySet() and get(key)
Line 209: long sizeRequested = 0;
Line 210: for (DiskImage diskImage : diskImages) {
Line 211: sizeRequested += diskImage.getActualSize();
Line 212: }
....................................................
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;
save the promotion, use a long literal - 0L.
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);
can't this be defined as a List<DiskImage> ?
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);
No.
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);
can't this be defined as a List<DiskImage> ?
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);
can't this be defined as a List<DiskImage> ?
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 183: }
Line 184:
Line 185: @Test
Line 186: public void
testNotEnoughForMultipleDiskAndDomainsSecondDomainFails() {
Line 187: ArrayList<DiskImage> imagesDisks =
mockMultipleSourceImagesForDomain(4, STORAGE_DOMAIN_ID, 4);
can't this be defined as a List<DiskImage> ?
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);
can't this be defined as a List<DiskImage> ?
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>();
can't this be defined as a List<DiskImage> ?
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);
consider using constants,
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>();
can't this be defined as a List<DiskImage> ?
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) {
should be private static.
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);
....................................................
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.
Gramer: s/image/image's/
Question: Is the temp volume the size of the IMAGE? Don't you mean the size of
the VOLUMN?
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.
s/if/whether/
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
