Hello Adam Litke, Fred Rolland,
I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/47419
to review the following change.
Change subject: hsm: Add initialSize argument to createVolume ......................................................................
hsm: Add initialSize argument to createVolume
Add the possibility to specify the volume initial size, for block disk with thin provisioning.
This argument is needed when importing a VM from an external provider along with its disks. If one of the VM existing disk is a block disk with thin provisioning, the new volume needs a starting size as the original disk size.
If the initial size is not specified, the default initial volume size for thin provisioning is the configured volume_utilization_chunk_mb (1GiB by default)
Change-Id: Iaf5f142541bf0b311a77a0544a1c4ffb689a9fde Bug-Url: https://bugzilla.redhat.com/1221603 Signed-off-by: Fred Rolland frolland@redhat.com Reviewed-on: https://gerrit.ovirt.org/46417 Reviewed-by: Nir Soffer nsoffer@redhat.com Tested-by: Nir Soffer nsoffer@redhat.com Reviewed-by: Adam Litke alitke@redhat.com Continuous-Integration: Jenkins CI --- M client/vdsClient.py M tests/Makefile.am A tests/blockVolumeTests.py M tests/miscTests.py M vdsm/API.py M vdsm/rpc/bindingxmlrpc.py M vdsm/rpc/vdsmapi-schema.json M vdsm/storage/blockVolume.py M vdsm/storage/fileVolume.py M vdsm/storage/hsm.py M vdsm/storage/misc.py M vdsm/storage/sd.py M vdsm/storage/sp.py M vdsm/storage/storageConstants.py M vdsm/storage/volume.py 15 files changed, 195 insertions(+), 37 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/19/47419/1
diff --git a/client/vdsClient.py b/client/vdsClient.py index b9a977e..fe263f2 100755 --- a/client/vdsClient.py +++ b/client/vdsClient.py @@ -1078,10 +1078,15 @@ srcVolUUID = args[10] else: srcVolUUID = BLANK_UUID - image = self.s.createVolume(sdUUID, spUUID, imgUUID, size, - volFormat, preallocate, - diskType, newVol, descr, - srcImgUUID, srcVolUUID) + params = [sdUUID, spUUID, imgUUID, size, volFormat, preallocate, + diskType, newVol, descr, srcImgUUID, srcVolUUID] + if len(args) > 11: + initialSizeGIB = int(args[11]) + initialSize = str(initialSizeGIB * constants.GIB) + params.append(initialSize) + + image = self.s.createVolume(*params) + if image['status']['code']: return image['status']['code'], image['status']['message'] return 0, image['uuid'] @@ -2520,7 +2525,7 @@ 'createVolume': (serv.createVolume, ('<sdUUID> <spUUID> <imgUUID> <size> <volFormat> ' '<preallocate> <diskType> <newVolUUID> <descr> ' - '<srcImgUUID> <srcVolUUID>', + '<srcImgUUID> <srcVolUUID> <initialSize>', 'Creates new volume or snapshot' )), 'extendVolumeSize': (serv.extendVolumeSize, ( diff --git a/tests/Makefile.am b/tests/Makefile.am index 8ae61d9..9e52a08 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -33,6 +33,7 @@ test_modules = \ alignmentScanTests.py \ blocksdTests.py \ + blockVolumeTests.py \ bridgeTests.py \ cPopenTests.py \ capsTests.py \ diff --git a/tests/blockVolumeTests.py b/tests/blockVolumeTests.py new file mode 100644 index 0000000..0ef3093 --- /dev/null +++ b/tests/blockVolumeTests.py @@ -0,0 +1,54 @@ +# +# Copyright 2015 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# +# Refer to the README and COPYING files for full details of the license +# + + +from vdsm.config import config +from storage.blockVolume import BlockVolume +from storage import storage_exception as se +from storage import volume +from testlib import permutations, expandPermutations +from testlib import VdsmTestCase as TestCaseBase + + +@expandPermutations +class BlockVolumeSizeTests(TestCaseBase): + + @permutations([ + # (preallocate, capacity, initial_size), result + [(volume.PREALLOCATED_VOL, 2048, None), 1], + [(volume.PREALLOCATED_VOL, 2049, None), 2], + [(volume.PREALLOCATED_VOL, 2097152, None), 1024], + [(volume.SPARSE_VOL, 9999, None), + config.getint("irs", "volume_utilization_chunk_mb")], + [(volume.SPARSE_VOL, 8388608, 1860), 1], + [(volume.SPARSE_VOL, 8388608, 1870), 2], + ]) + def test_block_volume_size(self, args, result): + size = BlockVolume._calculate_volume_alloc_size(*args) + self.assertEqual(size, result) + + @permutations([ + # preallocate + [volume.PREALLOCATED_VOL], + [volume.SPARSE_VOL], + ]) + def test_fail_invalid_block_volume_size(self, preallocate): + with self.assertRaises(se.InvalidParameterException): + BlockVolume._calculate_volume_alloc_size(preallocate, 2048, 2049) diff --git a/tests/miscTests.py b/tests/miscTests.py index 61aeb92..37f0ea0 100644 --- a/tests/miscTests.py +++ b/tests/miscTests.py @@ -576,6 +576,32 @@ self.assertRaises(expectedException, misc.validateInt, "2-1", "a")
+@expandPermutations +class ValidateSize(TestCaseBase): + + @permutations([ + # size, result + ("512", 1), + ("513", 2), + (u"1073741824", 2097152), + ]) + def test_valid_size(self, size, result): + self.assertEqual(misc.validateSize(size, "size"), result) + + @permutations([ + # size + [2097152], # 1GiB in sectors + [1000.14], + ["one"], + ["nan"], + ["3.14"], + ["-1"], + ]) + def test_invalid_size(self, size): + self.assertRaises(misc.se.InvalidParameterException, + misc.validateSize, size, "size") + + class ValidateUuid(TestCaseBase):
def testValidInput(self): diff --git a/vdsm/API.py b/vdsm/API.py index 7582852..117b0ee 100644 --- a/vdsm/API.py +++ b/vdsm/API.py @@ -825,11 +825,12 @@ volFormat, preallocate, postZero, force)
def create(self, size, volFormat, preallocate, diskType, desc, - srcImgUUID, srcVolUUID): + srcImgUUID, srcVolUUID, initialSize=None): return self._irs.createVolume(self._sdUUID, self._spUUID, self._imgUUID, size, volFormat, preallocate, diskType, self._UUID, desc, - srcImgUUID, srcVolUUID) + srcImgUUID, srcVolUUID, + initialSize=initialSize)
def delete(self, postZero, force): return self._irs.deleteVolume(self._sdUUID, self._spUUID, diff --git a/vdsm/rpc/bindingxmlrpc.py b/vdsm/rpc/bindingxmlrpc.py index 4c69b32..c521cd8 100644 --- a/vdsm/rpc/bindingxmlrpc.py +++ b/vdsm/rpc/bindingxmlrpc.py @@ -918,10 +918,10 @@ def volumeCreate(self, sdUUID, spUUID, imgUUID, size, volFormat, preallocate, diskType, volUUID, desc, srcImgUUID=API.Image.BLANK_UUID, - srcVolUUID=API.Volume.BLANK_UUID): + srcVolUUID=API.Volume.BLANK_UUID, initialSize=None): volume = API.Volume(volUUID, spUUID, sdUUID, imgUUID) return volume.create(size, volFormat, preallocate, diskType, desc, - srcImgUUID, srcVolUUID) + srcImgUUID, srcVolUUID, initialSize=initialSize)
def volumeExtendSize(self, spUUID, sdUUID, imgUUID, volUUID, newSize): volume = API.Volume(volUUID, spUUID, sdUUID, imgUUID) diff --git a/vdsm/rpc/vdsmapi-schema.json b/vdsm/rpc/vdsmapi-schema.json index 1ff0caa..ce0d7da 100644 --- a/vdsm/rpc/vdsmapi-schema.json +++ b/vdsm/rpc/vdsmapi-schema.json @@ -8282,6 +8282,11 @@ # # @srcVolUUID: If specified, create a snapshot from this Volume # +# @initialSize: #optional if specified, initial size of volume in bytes +# (as string). Allowed only when creating thin provisioned +# volume on block storage. +# (new in version 4.17.8) +# # Returns: # A task UUID # @@ -8292,7 +8297,8 @@ 'storagedomainID': 'UUID', 'imageID': 'UUID', 'size': 'str', 'volFormat': 'VolumeFormat', 'preallocate': 'VolumeAllocation', 'diskType': 'DiskType', - 'desc': 'str', 'srcImgUUID': 'UUID', 'srcVolUUID': 'UUID'}, + 'desc': 'str', 'srcImgUUID': 'UUID', 'srcVolUUID': 'UUID', + '*initialSize': 'str'}, 'returns': 'UUID'}
## diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py index 55ac3c3..4c8016c 100644 --- a/vdsm/storage/blockVolume.py +++ b/vdsm/storage/blockVolume.py @@ -59,6 +59,8 @@
SECTORS_TO_MB = 2048
+QCOW_OVERHEAD_FACTOR = 1.1 + # Reserved leases for special purposes: # - 0 SPM (Backward comapatibility with V0 and V2) # - 1 SDM (SANLock V3) @@ -118,18 +120,16 @@
@classmethod def _create(cls, dom, imgUUID, volUUID, size, volFormat, preallocate, - volParent, srcImgUUID, srcVolUUID, volPath): + volParent, srcImgUUID, srcVolUUID, volPath, initialSize=None): """ Class specific implementation of volumeCreate. All the exceptions are properly handled and logged in volume.create() """
- if preallocate == volume.SPARSE_VOL: - volSize = "%s" % config.get("irs", "volume_utilization_chunk_mb") - else: - volSize = "%s" % ((size + SECTORS_TO_MB - 1) / SECTORS_TO_MB) + lvSize = cls._calculate_volume_alloc_size(preallocate, + size, initialSize)
- lvm.createLV(dom.sdUUID, volUUID, volSize, activate=True, + lvm.createLV(dom.sdUUID, volUUID, "%s" % lvSize, activate=True, initialTag=TAG_VOL_UNINIT)
utils.rmFile(volPath) @@ -163,6 +163,39 @@
return (dom.sdUUID, slot)
+ @classmethod + def _calculate_volume_alloc_size(cls, preallocate, capacity, initial_size): + """ Calculate the allocation size in mb of the volume + 'preallocate' - Sparse or Preallocated + 'capacity' - the volume size in sectors + 'initial_size' - optional, if provided the initial allocated + size in sectors for sparse volumes + """ + if initial_size and initial_size > capacity: + log.error("The volume size %s is smaller " + "than the requested initial size %s", + capacity, initial_size) + raise se.InvalidParameterException("initial size", + initial_size) + + if initial_size and preallocate == volume.PREALLOCATED_VOL: + log.error("Initial size is not supported for preallocated volumes") + raise se.InvalidParameterException("initial size", + initial_size) + + if preallocate == volume.SPARSE_VOL: + if initial_size: + initial_size = int(initial_size * QCOW_OVERHEAD_FACTOR) + alloc_size = ((initial_size + SECTORS_TO_MB - 1) + / SECTORS_TO_MB) + else: + alloc_size = config.getint("irs", + "volume_utilization_chunk_mb") + else: + alloc_size = (capacity + SECTORS_TO_MB - 1) / SECTORS_TO_MB + + return alloc_size + def delete(self, postZero, force): """ Delete volume 'postZero' - zeroing file before deletion diff --git a/vdsm/storage/fileVolume.py b/vdsm/storage/fileVolume.py index 1259472..797e4a2 100644 --- a/vdsm/storage/fileVolume.py +++ b/vdsm/storage/fileVolume.py @@ -99,11 +99,17 @@
@classmethod def _create(cls, dom, imgUUID, volUUID, size, volFormat, preallocate, - volParent, srcImgUUID, srcVolUUID, volPath): + volParent, srcImgUUID, srcVolUUID, volPath, + initialSize=None): """ Class specific implementation of volumeCreate. All the exceptions are properly handled and logged in volume.create() """ + if initialSize: + cls.log.error("initialSize is not supported for file-based " + "volumes") + raise se.InvalidParameterException("initial size", + initialSize)
sizeBytes = size * BLOCK_SIZE truncSize = sizeBytes if volFormat == volume.RAW_FORMAT else 0 diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py index ac0f434..17b7a5a 100644 --- a/vdsm/storage/hsm.py +++ b/vdsm/storage/hsm.py @@ -64,6 +64,7 @@ from threadLocal import vars from vdsm import constants from storageConstants import STORAGE +from storageConstants import SECTOR_SIZE import resourceManager as rm from resourceFactories import IMAGE_NAMESPACE import devicemapper @@ -89,8 +90,6 @@
rmanager = rm.ResourceManager.getInstance()
-# FIXME: moved from spm.py but this should be somewhere else -SECTOR_SIZE = 512
STORAGE_CONNECTION_DIR = os.path.join(constants.P_VDSM_LIB, "connections/")
@@ -1431,7 +1430,8 @@ def createVolume(self, sdUUID, spUUID, imgUUID, size, volFormat, preallocate, diskType, volUUID, desc, srcImgUUID=volume.BLANK_UUID, - srcVolUUID=volume.BLANK_UUID): + srcVolUUID=volume.BLANK_UUID, + initialSize=None): """ Create a new volume Function Type: SPM @@ -1440,22 +1440,19 @@ """ argsStr = ("sdUUID=%s, spUUID=%s, imgUUID=%s, size=%s, volFormat=%s, " "preallocate=%s, diskType=%s, volUUID=%s, desc=%s, " - "srcImgUUID=%s, srcVolUUID=%s" % + "srcImgUUID=%s, srcVolUUID=%s, initialSize=%s" % (sdUUID, spUUID, imgUUID, size, volFormat, preallocate, - diskType, volUUID, desc, - srcImgUUID, srcVolUUID)) + diskType, volUUID, desc, srcImgUUID, srcVolUUID, + initialSize)) vars.task.setDefaultException(se.VolumeCreationError(argsStr)) # Validates that the pool is connected. WHY? pool = self.getPool(spUUID) dom = sdCache.produce(sdUUID=sdUUID) misc.validateUUID(imgUUID, 'imgUUID') misc.validateUUID(volUUID, 'volUUID') - if not isinstance(size, basestring): - self.log.error("Number of sectors as int is not supported, use " - "size in bytes as string") - raise se.InvalidParameterException("size", size) - size = misc.validateN(size, "size") - size = (size + SECTOR_SIZE - 1) / SECTOR_SIZE + size = misc.validateSize(size, "size") + if initialSize: + initialSize = misc.validateSize(initialSize, "initialSize")
if srcImgUUID: misc.validateUUID(srcImgUUID, 'srcImgUUID') @@ -1468,7 +1465,7 @@ vars.task.getSharedLock(STORAGE, sdUUID) self._spmSchedule(spUUID, "createVolume", pool.createVolume, sdUUID, imgUUID, size, volFormat, preallocate, diskType, - volUUID, desc, srcImgUUID, srcVolUUID) + volUUID, desc, srcImgUUID, srcVolUUID, initialSize)
@public def deleteVolume(self, sdUUID, spUUID, imgUUID, volumes, postZero=False, diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index b14fd26..d35c7a6 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -49,6 +49,7 @@
from vdsm import constants from vdsm import utils +from storageConstants import SECTOR_SIZE import storage_exception as se import logUtils
@@ -449,6 +450,22 @@ return n
+def validateSize(size, name): + """ + Validate number of bytes as string and convert to number of sectors, + rounding up to next sectors. + + Raises InvalidParameterException if value is not a string or if it could + not be converted to integer. + """ + if not isinstance(size, basestring): + log.error("Number of sectors as int is not supported, use size in " + "bytes as string") + raise se.InvalidParameterException("size", size) + size = validateN(size, name) + return (size + SECTOR_SIZE - 1) / SECTOR_SIZE + + def rotateFiles(directory, prefixName, gen, cp=False, persist=False): log.debug("dir: %s, prefixName: %s, versions: %s" % (directory, prefixName, gen)) diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py index 5cdb70b..4b48180 100644 --- a/vdsm/storage/sd.py +++ b/vdsm/storage/sd.py @@ -478,13 +478,14 @@ raise se.IncorrectType(preallocate)
def createVolume(self, imgUUID, size, volFormat, preallocate, diskType, - volUUID, desc, srcImgUUID, srcVolUUID): + volUUID, desc, srcImgUUID, srcVolUUID, initialSize=None): """ Create a new volume """ return self.getVolumeClass().create( self._getRepoPath(), self.sdUUID, imgUUID, size, volFormat, - preallocate, diskType, volUUID, desc, srcImgUUID, srcVolUUID) + preallocate, diskType, volUUID, desc, srcImgUUID, srcVolUUID, + initialSize=initialSize)
def getMDPath(self): return self._manifest.getMDPath() diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py index 247d322..b8fd8f3 100644 --- a/vdsm/storage/sp.py +++ b/vdsm/storage/sp.py @@ -1804,7 +1804,8 @@ def createVolume(self, sdUUID, imgUUID, size, volFormat, preallocate, diskType, volUUID=None, desc="", srcImgUUID=volume.BLANK_UUID, - srcVolUUID=volume.BLANK_UUID): + srcVolUUID=volume.BLANK_UUID, + initialSize=None): """ Creates a new volume.
@@ -1836,6 +1837,9 @@ :param srcVolUUID: The UUID of the volume that will be the base of the new volume. :type srcVolUUID: UUID + :param initialSize: The initial size of the volume in case of thin + provisioning. + :type initialSize: int
:returns: a dict with the UUID of the new volume. :rtype: dict @@ -1864,7 +1868,8 @@ newVolUUID = sdCache.produce(sdUUID).createVolume( imgUUID=imgUUID, size=size, volFormat=volFormat, preallocate=preallocate, diskType=diskType, volUUID=volUUID, - desc=desc, srcImgUUID=srcImgUUID, srcVolUUID=srcVolUUID) + desc=desc, srcImgUUID=srcImgUUID, srcVolUUID=srcVolUUID, + initialSize=initialSize) return dict(uuid=newVolUUID)
def deleteVolume(self, sdUUID, imgUUID, volumes, postZero, force): diff --git a/vdsm/storage/storageConstants.py b/vdsm/storage/storageConstants.py index 5acca65..f247c03 100644 --- a/vdsm/storage/storageConstants.py +++ b/vdsm/storage/storageConstants.py @@ -19,3 +19,4 @@ #
STORAGE = "Storage" +SECTOR_SIZE = 512 diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py index 73619c2..ef41d15 100644 --- a/vdsm/storage/volume.py +++ b/vdsm/storage/volume.py @@ -395,7 +395,8 @@
@classmethod def create(cls, repoPath, sdUUID, imgUUID, size, volFormat, preallocate, - diskType, volUUID, desc, srcImgUUID, srcVolUUID): + diskType, volUUID, desc, srcImgUUID, srcVolUUID, + initialSize=None): """ Create a new volume with given size or snapshot 'size' - in sectors @@ -404,6 +405,8 @@ 'diskType' - enum (API.Image.DiskTypes) 'srcImgUUID' - source image UUID 'srcVolUUID' - source volume UUID + 'initialSize' - initial volume size in sectors, + in case of thin provisioning """ dom = sdCache.produce(sdUUID) dom.validateCreateVolumeParams(volFormat, srcVolUUID, @@ -469,8 +472,10 @@ try: metaId = cls._create(dom, imgUUID, volUUID, size, volFormat, preallocate, volParent, srcImgUUID, - srcVolUUID, volPath) - except (se.VolumeAlreadyExists, se.CannotCreateLogicalVolume) as e: + srcVolUUID, volPath, + initialSize=initialSize) + except (se.VolumeAlreadyExists, se.CannotCreateLogicalVolume, + se.VolumeCreationError, se.InvalidParameterException) as e: cls.log.error("Failed to create volume %s: %s", volPath, e) vars.task.popRecovery() raise