Freddy Rolland has posted comments on this change. Change subject: storage: Initial size for thin provisioning disk ......................................................................
Patch Set 6: (15 comments) https://gerrit.ovirt.org/#/c/46417/6//COMMIT_MSG Commit Message: Line 14: is a block disk with thin provisioning, the new volume needs Line 15: a starting size as the original disk size. Line 16: Line 17: If the initial size is not specified, the default initial Line 18: volume size for thin provisioning is 1G. > is the configured volume_utilization_chunk_mb (1GiB by default). Done Line 19: Line 20: Change-Id: Iaf5f142541bf0b311a77a0544a1c4ffb689a9fde Line 21: Bug-Url: https://bugzilla.redhat.com/1221603 https://gerrit.ovirt.org/#/c/46417/6/tests/blockVolumeTests.py File tests/blockVolumeTests.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: import storage.volume as volume > Use: Done Line 22: import storage.storage_exception as se Line 23: Line 24: from storage.blockVolume import BlockVolume Line 25: from testlib import permutations, expandPermutations Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: Line 21: import storage.volume as volume Line 22: import storage.storage_exception as se > from storage import storage_exception as se Done Line 23: Line 24: from storage.blockVolume import BlockVolume Line 25: from testlib import permutations, expandPermutations Line 26: from testlib import VdsmTestCase as TestCaseBase Line 20: Line 21: import storage.volume as volume Line 22: import storage.storage_exception as se Line 23: Line 24: from storage.blockVolume import BlockVolume > resort after changing the import above this. Done Line 25: from testlib import permutations, expandPermutations Line 26: from testlib import VdsmTestCase as TestCaseBase Line 27: from vdsm.config import config Line 28: Line 23: Line 24: from storage.blockVolume import BlockVolume Line 25: from testlib import permutations, expandPermutations Line 26: from testlib import VdsmTestCase as TestCaseBase Line 27: from vdsm.config import config > Should be before the storage imports. The order is: stdlib, vdsm lib, modul Done Line 28: Line 29: Line 30: @expandPermutations Line 31: class BlockVolumeSizeTests(TestCaseBase): Line 45: initial_size, result): Line 46: size = BlockVolume._calculate_volume_alloc_size(preallocate, Line 47: capacity, Line 48: initial_size) Line 49: self.assertEqual(size, result) > Did you see my comment about using *args in previous version? Done Line 50: Line 51: @permutations([ Line 52: # preallocate, capacity, initial_size, Line 53: (volume.PREALLOCATED_VOL, 2048, 5000), # initial bigger than size Line 56: def test_fail_invalide_block_volume_size(self, preallocate, Line 57: capacity, initial_size): Line 58: with self.assertRaises(se.InvalidParameterException): Line 59: BlockVolume._calculate_volume_alloc_size(preallocate, capacity, Line 60: initial_size) > Since you change only hte volume preallocated type, you don't need to add t Done https://gerrit.ovirt.org/#/c/46417/6/tests/miscTests.py File tests/miscTests.py: Line 583: # size, name, result Line 584: (1000, "size", 1000), Line 585: ("512", "size", 1), Line 586: ("513", "size", 2), Line 587: (None, "size", None), > "size" is repeated for no reason. mention it once in the body of the test Done Line 588: ]) Line 589: def test_validate_size(self, size, name, result): Line 590: self.assertEqual(misc.validateSize(size, name), result) Line 591: Line 585: ("512", "size", 1), Line 586: ("513", "size", 2), Line 587: (None, "size", None), Line 588: ]) Line 589: def test_validate_size(self, size, name, result): > I would call this test_valid_size Done Line 590: self.assertEqual(misc.validateSize(size, name), result) Line 591: Line 592: @permutations([ Line 593: # size, name Line 593: # size, name Line 594: (1000.14, "size"), Line 595: ((1, 2), "size"), Line 596: ]) Line 597: def test_fail_validate_size(self, size, name): > I would call this test_invalid_size Done Line 598: self.assertRaises(misc.se.InvalidParameterException, Line 599: misc.validateSize, size, name) Line 600: Line 601: https://gerrit.ovirt.org/#/c/46417/6/vdsm/storage/blockVolume.py File vdsm/storage/blockVolume.py: Line 125: Class specific implementation of volumeCreate. All the exceptions are Line 126: properly handled and logged in volume.create() Line 127: """ Line 128: Line 129: volSize = cls._calculate_volume_alloc_size(preallocate, > Lets call this lvSize Done Line 130: size, initialSize) Line 131: Line 132: lvm.createLV(dom.sdUUID, volUUID, "%s" % volSize, activate=True, Line 133: initialTag=TAG_VOL_UNINIT) Line 165: Line 166: @classmethod Line 167: def _calculate_volume_alloc_size(cls, preallocate, capacity, initial_size): Line 168: """ Calculate the allocation size in mb of the volume Line 169: 'preallocate' - Spare or Preallocated > Sparse Done Line 170: 'capacity' - the volume size in sectors Line 171: 'initial_size' - optional, if provided the initial allocated Line 172: size in sectors for sparse volumes Line 173: """ Line 180: Line 181: if preallocate == volume.SPARSE_VOL: Line 182: if initial_size: Line 183: initial_size = int(initial_size * QCOW_OVERHEAD_FACTOR) Line 184: vol_size_mb = ((initial_size + SECTORS_TO_MB - 1) > Lets call this alloc_size to make this more clear. Done Line 185: / SECTORS_TO_MB) Line 186: else: Line 187: vol_size_mb = config.getint("irs", Line 188: "volume_utilization_chunk_mb") https://gerrit.ovirt.org/#/c/46417/6/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 1453: # TODO: For backwards compatibility, we need to support accepting Line 1454: # number of sectors as int type Updated interface is accepting string Line 1455: # type in bytes (ugly, get rid of this when possible) Line 1456: size = misc.validateSize(size, "size") Line 1457: initialSize = misc.validateSize(initialSize, "initialSize") > This allows now None size, which will fail in the lower levels. I think we Done Line 1458: Line 1459: if srcImgUUID: Line 1460: misc.validateUUID(srcImgUUID, 'srcImgUUID') Line 1461: if srcVolUUID: https://gerrit.ovirt.org/#/c/46417/6/vdsm/storage/misc.py File vdsm/storage/misc.py: Line 456: number of sectors as int type and bytes as string type. Line 457: In case of bytes, we need to convert to sectors. Line 458: """ Line 459: if size is None: Line 460: return None > Don't support None, see my comment in hsm.py. Done Line 461: elif isinstance(size, basestring): Line 462: size = validateN(size, name) Line 463: return (size + SECTOR_SIZE - 1) / SECTOR_SIZE Line 464: elif isinstance(size, int): -- To view, visit https://gerrit.ovirt.org/46417 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaf5f142541bf0b311a77a0544a1c4ffb689a9fde Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Fred Rolland <froll...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches