Change in vdsm[master]: storage: Initial size for thin provisioning disk

frolland at redhat.com frolland at redhat.com
Sun Oct 11 11:28:17 UTC 2015


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 <frolland at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Fred Rolland <frolland at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Greg Padgett <gpadgett at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list