Change in vdsm[master]: storage: Add inplace virt-sparsify support

smelamud at redhat.com smelamud at redhat.com
Wed May 11 12:45:03 UTC 2016


Hello Shmuel Melamud,

I'd like you to do a code review.  Please visit

    https://gerrit.ovirt.org/57347

to review the following change.

Change subject: storage: Add inplace virt-sparsify support
......................................................................

storage: Add inplace virt-sparsify support

Added support of sparsify operation (removing unused space from a disk
image) that is performed in-place (without copying content of an input
disk to an output disk) using virt-sparsify tool.

Change-Id: I6ac2bb1fbd2acbe0fc47694d17313c6ccd01a227
Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=734120
Signed-off-by: Shmuel Melamud <smelamud at redhat.com>
---
M client/vdsClient.py
M lib/api/vdsmapi-schema.json
M lib/vdsm/rpc/Bridge.py
M lib/vdsm/rpc/bindingxmlrpc.py
M lib/vdsm/storage/exception.py
M lib/vdsm/virtsparsify.py
M tests/crossImportsTests.py.in
M tests/sparsifyTests.py
M vdsm/API.py
M vdsm/storage/hsm.py
M vdsm/storage/image.py
M vdsm/storage/sp.py
12 files changed, 86 insertions(+), 201 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/47/57347/1

diff --git a/client/vdsClient.py b/client/vdsClient.py
index 6ea0283..e3d2778 100755
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -1370,14 +1370,11 @@
         return 0, image['uuid']
 
     def sparsifyImage(self, args):
-        (spUUID, tmpSdUUID, tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID,
-         dstVolUUID) = args
-        status = self.s.sparsifyImage(spUUID, tmpSdUUID, tmpImgUUID,
-                                      tmpVolUUID, dstSdUUID, dstImgUUID,
-                                      dstVolUUID)
+        (imgUUID, spUUID, sdUUID, volUUID) = args
+        status = self.s.sparsifyImage(imgUUID, spUUID, sdUUID, volUUID)
         if status['status']['code']:
             return status['status']['code'], status['status']['message']
-        return 0, status['uuid']
+        return 0
 
     def cloneImageStructure(self, args):
         spUUID, sdUUID, imgUUID, dstSdUUID = args
diff --git a/lib/api/vdsmapi-schema.json b/lib/api/vdsmapi-schema.json
index 1da8cc0..7c7e750 100644
--- a/lib/api/vdsmapi-schema.json
+++ b/lib/api/vdsmapi-schema.json
@@ -4847,29 +4847,22 @@
 ##
 # @Image.sparsify:
 #
-# Create a copy of a sparse volume to a destination volume within the same
-# Storage Pool with free space removed from the source volume.
+# Perform an in-place sparse (e.g. not creating a new image) on given volume,
+# removing unused space from the volume.
 #
-# @imageID:          The UUID of the Image
+# @imageID:          The UUID of the Volume
 #
-# @storagepoolID:    The UUID of the Storage Pool associated with the Image
+# @storagepoolID:    The UUID of the Storage Pool associated with the Volume
 #
-# @storagedomainID:  The UUID of the Storage Domain associated with the Image
+# @storagedomainID:  The UUID of the Storage Domain associated with the Volume
 #
-# @tmpVolUUID:       The UUID of the snapshot volume
+# @volumeUUID:       The UUID of the Image that contains the Volume
 #
-# @dstSdUUID:        The UUID of the destination storage domain
-#
-# @dstImgUUID:       The UUID of the destination image
-#
-# @dstVolUUID:       The UUID of the destination volume
-#
-# Since: 4.17.0
+# Since: 4.18.0
 ##
 {'command': {'class': 'Image', 'name': 'sparsify'},
  'data': {'imageID': 'UUID', 'storagepoolID': 'UUID',
-          'storagedomainID': 'UUID', 'tmpVolUUID': 'UUID', 'dstSdUUID': 'UUID',
-          'dstImgUUID': 'UUID', 'dstVolUUID': 'UUID'}}
+          'storagedomainID': 'UUID', 'volumeUUID': 'UUID'}}
 
 ##
 # @Image.cloneStructure:
diff --git a/lib/vdsm/rpc/Bridge.py b/lib/vdsm/rpc/Bridge.py
index 6b106dd..636e06a 100644
--- a/lib/vdsm/rpc/Bridge.py
+++ b/lib/vdsm/rpc/Bridge.py
@@ -427,6 +427,7 @@
     'Image_mergeSnapshots': {'ret': 'uuid'},
     'Image_move': {'ret': 'uuid'},
     'Image_reconcileVolumeChain': {'ret': 'volumes'},
+    'Image_sparsify': {},
     'Image_syncData': {'ret': 'uuid'},
     'Image_upload': {'ret': 'uuid'},
     'Image_prepare': {'ret': Image_prepare_Ret},
diff --git a/lib/vdsm/rpc/bindingxmlrpc.py b/lib/vdsm/rpc/bindingxmlrpc.py
index 9945dc7..b9b6a8b 100644
--- a/lib/vdsm/rpc/bindingxmlrpc.py
+++ b/lib/vdsm/rpc/bindingxmlrpc.py
@@ -746,10 +746,9 @@
         image = API.Image(imgUUID, spUUID, srcDomUUID)
         return image.move(dstDomUUID, op, postZero, force)
 
-    def imageSparsify(self, spUUID, sdUUID, imgUUID, tmpVolUUID, dstSdUUID,
-                      dstImgUUID, dstVolUUID):
+    def imageSparsify(self, imgUUID, spUUID, sdUUID, volUUID):
         image = API.Image(imgUUID, spUUID, sdUUID)
-        return image.sparsify(tmpVolUUID, dstSdUUID, dstImgUUID, dstVolUUID)
+        return image.sparsify(volUUID)
 
     def imageCloneStructure(self, spUUID, sdUUID, imgUUID, dstSdUUID):
         image = API.Image(imgUUID, spUUID, sdUUID)
diff --git a/lib/vdsm/storage/exception.py b/lib/vdsm/storage/exception.py
index 23e2628..3924b06 100644
--- a/lib/vdsm/storage/exception.py
+++ b/lib/vdsm/storage/exception.py
@@ -336,11 +336,6 @@
     message = "Volume type is not sparse"
 
 
-class CannotSparsifyVolume(StorageException):
-    code = 234
-    message = "Cannot sparsify volume"
-
-
 #################################################
 #  Images Exceptions
 #################################################
diff --git a/lib/vdsm/virtsparsify.py b/lib/vdsm/virtsparsify.py
index 44ed778..a8b08ce 100644
--- a/lib/vdsm/virtsparsify.py
+++ b/lib/vdsm/virtsparsify.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2014 Red Hat, Inc.
+# Copyright 2016 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
@@ -19,39 +19,78 @@
 #
 
 from __future__ import absolute_import
+from contextlib import contextmanager
+import logging
 import signal
+from uuid import uuid4
 
-from . import cmdutils
 from . import commands
+from . import concurrent
+from . import jobs
 from . import utils
+from . import response
 
 # Fedora, EL6
 _VIRTSPARSIFY = utils.CommandPath("virt-sparsify",
                                   "/usr/bin/virt-sparsify",)
 
 
-def sparsify(src_vol, tmp_vol, dst_vol, src_format=None, dst_format=None):
+def sparsify(imgUUID, sdUUID, spUUID, volUUID, irs):
     """
-    Sparsify the 'src_vol' volume (src_format) to 'dst_vol' volume (dst_format)
-    using libguestfs virt-sparsify
-
-    src_vol: path of base volume
-    tmp_vol: path of temporary volume created with src_vol as backing volume
-    dst_vol: path of destination volume
-    src_format: format of base volume ('raw' or `qcow2')
-    src_format: format of destination volume ('raw' or `qcow2')
+    Sparsify the volume in place
+    (without copying from an input disk to an output disk)
     """
-    cmd = [_VIRTSPARSIFY.cmd, '--tmp', 'prebuilt:' + tmp_vol]
+    job = SparsifyJob(imgUUID, sdUUID, spUUID, volUUID, irs)
+    job.start()
+    jobs.add(job)
 
-    if src_format:
-        cmd.extend(("--format", src_format))
 
-    if dst_format:
-        cmd.extend(("--convert", dst_format))
+def delete_sparsify_job(job_id):
+    return jobs.delete(job_id)
 
-    cmd.extend((src_vol, dst_vol))
 
-    rc, out, err = commands.execCmd(cmd, deathSignal=signal.SIGKILL)
+def abort_sparsify_job(job_id):
+    return jobs.abort(job_id)
 
-    if rc != 0:
-        raise cmdutils.Error(cmd, rc, out, err)
+
+ at contextmanager
+def volume(imgUUID, sdUUID, spUUID, volUUID, irs):
+    res = irs.prepareImage(sdUUID, spUUID, imgUUID, volUUID)
+    if response.is_error(res):
+        raise Exception('Cannot find volume path for %r' % volUUID)
+    try:
+        yield res['path']
+    finally:
+        try:
+            irs.teardownImage(sdUUID, spUUID, imgUUID)
+        except Exception:
+            logging.exception('Error tearing down image: %r', imgUUID)
+            raise
+
+
+class SparsifyJob(jobs.Job):
+    def __init__(self, imgUUID, sdUUID, spUUID, volUUID, irs):
+        self._imgUUID = imgUUID
+        self._sdUUID = sdUUID
+        self._spUUID = spUUID
+        self._volUUID = volUUID
+        self._irs = irs
+        self._id = uuid4()
+        self._thread = None
+
+    def start(self):
+        self._thread = concurrent.thread(self._run)
+        self._thread.start()
+
+    def _run(self):
+        with volume(self._imgUUID, self._sdUUID, self._spUUID,
+                    self._volUUID, self._irs) as path:
+            cmd = [_VIRTSPARSIFY.cmd, '--machine-readable', '--in-place', path]
+            logging.info('>>> cmd: %r', cmd)
+            rc, out, err = commands.execCmd(cmd,
+                                            deathSignal=signal.SIGTERM,
+                                            env={'LIBGUESTFS_BACKEND':
+                                                 'direct'})
+            logging.info('>>> rc: %r', rc)
+            logging.info('>>> out: %r', out)
+            logging.info('>>> err: %r', err)
diff --git a/tests/crossImportsTests.py.in b/tests/crossImportsTests.py.in
index 225f81a..87555e7 100644
--- a/tests/crossImportsTests.py.in
+++ b/tests/crossImportsTests.py.in
@@ -51,7 +51,6 @@
             # some random samples. Some names must include 'p', 'y', 'c'
             Mod("response.py", "response"),
             Mod("cmdutils.py", "cmdutils"),
-            Mod("virtsparsify.py", "virtsparsify"),
             Mod("pthread.py", "pthread"),
             Mod("constants.py", "constants"),
             Mod("compat.py", "compat"),
diff --git a/tests/sparsifyTests.py b/tests/sparsifyTests.py
index f99b0c8..81bf477 100644
--- a/tests/sparsifyTests.py
+++ b/tests/sparsifyTests.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2015 Red Hat, Inc.
+# Copyright 2016 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
@@ -18,26 +18,8 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
-from vdsm import cmdutils
-from vdsm import virtsparsify
-
-from monkeypatch import MonkeyPatch
-from testlib import VdsmTestCase as TestCaseBase
-
-
-FAKE_VOLUME = '/we/dont/care/about/this/path'
-
-
-class FakeCommand(object):
-    # as per manpage, false ignores any argument
-    cmd = '/bin/false'
-
-
-class VirtSparsifyTests(TestCaseBase):
-
-    @MonkeyPatch(virtsparsify, '_VIRTSPARSIFY', FakeCommand())
-    def test_raise_error_on_failure(self):
-
-        self.assertRaises(cmdutils.Error,
-                          virtsparsify.sparsify,
-                          FAKE_VOLUME, FAKE_VOLUME, FAKE_VOLUME)
+# from vdsm import cmdutils
+# from vdsm import virtsparsify
+#
+# from monkeypatch import MonkeyPatch
+# from testlib import VdsmTestCase as TestCaseBase
diff --git a/vdsm/API.py b/vdsm/API.py
index e7aac09..019db0c 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -908,10 +908,9 @@
                                    self._UUID, vmUUID, operation, postZero,
                                    force)
 
-    def sparsify(self, tmpVolUUID, dstSdUUID, dstImgUUID, dstVolUUID):
-        return self._irs.sparsifyImage(self._spUUID, self._sdUUID, self._UUID,
-                                       tmpVolUUID, dstSdUUID, dstImgUUID,
-                                       dstVolUUID)
+    def sparsify(self, volumeUUID):
+        return self._irs.sparsifyImage(self._UUID, self._spUUID, self._sdUUID,
+                                       volumeUUID, self._irs)
 
     def cloneStructure(self, dstSdUUID):
         return self._irs.cloneImageStructure(self._spUUID, self._sdUUID,
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index bd33b25..13e0305 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -78,6 +78,7 @@
 import devicemapper
 import dispatcher
 import storageServer
+import virtsparsify
 
 
 import sdm.api.create_volume
@@ -1586,23 +1587,14 @@
             misc.parseBool(force))
 
     @public
-    def sparsifyImage(self, spUUID, tmpSdUUID, tmpImgUUID, tmpVolUUID,
-                      dstSdUUID, dstImgUUID, dstVolUUID):
+    def sparsifyImage(self, imgUUID, spUUID, sdUUID, volUUID, irs):
         """
         Reduce sparse image size by converting free space on image to free
         space on storage domain using virt-sparsify.
         """
 
-        pool = self.getPool(spUUID)
-
-        sdUUIDs = sorted(set((tmpSdUUID, dstSdUUID)))
-
-        for dom in sdUUIDs:
-            vars.task.getSharedLock(STORAGE, dom)
-
-        self._spmSchedule(spUUID, "sparsifyImage", pool.sparsifyImage,
-                          tmpSdUUID, tmpImgUUID, tmpVolUUID, dstSdUUID,
-                          dstImgUUID, dstVolUUID)
+        virtsparsify.sparsify(imgUUID, spUUID, sdUUID, volUUID, irs)
+        # TODO use self instead of irs?
 
     @public
     def cloneImageStructure(self, spUUID, sdUUID, imgUUID, dstSdUUID):
diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py
index 381b2a1..4a52428 100644
--- a/vdsm/storage/image.py
+++ b/vdsm/storage/image.py
@@ -26,7 +26,6 @@
 
 import volume
 from vdsm import qemuimg
-from vdsm import virtsparsify
 from vdsm.config import config
 from vdsm.storage import exception as se
 from vdsm.storage import fileUtils
@@ -582,74 +581,6 @@
         self.log.info("%s task on image %s was successfully finished",
                       OP_TYPES[op], imgUUID)
         return True
-
-    def _getSparsifyVolume(self, sdUUID, imgUUID, volUUID):
-        # FIXME:
-        # sdCache.produce.produceVolume gives volumes that return getVolumePath
-        # with a colon (:) for NFS servers. So, we're using volClass().
-        # https://bugzilla.redhat.com/1128942
-        # If, and when the bug gets solved, use
-        # sdCache.produce(...).produceVolume(...) to create the volumes.
-        volClass = sdCache.produce(sdUUID).getVolumeClass()
-        return volClass(self.repoPath, sdUUID, imgUUID, volUUID)
-
-    def sparsify(self, tmpSdUUID, tmpImgUUID, tmpVolUUID, dstSdUUID,
-                 dstImgUUID, dstVolUUID):
-        """
-        Reduce sparse image size by converting free space on image to free
-        space on storage domain using virt-sparsify.
-        """
-        self.log.info("tmpSdUUID=%s, tmpImgUUID=%s, tmpVolUUID=%s, "
-                      "dstSdUUID=%s, dstImgUUID=%s, dstVolUUID=%s", tmpSdUUID,
-                      tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID,
-                      dstVolUUID)
-
-        tmpVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID, tmpVolUUID)
-        dstVolume = self._getSparsifyVolume(dstSdUUID, dstImgUUID, dstVolUUID)
-
-        if not dstVolume.isSparse():
-            raise se.VolumeNotSparse()
-
-        srcVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID,
-                                            tmpVolume.getParent())
-
-        tmpVolume.prepare()
-        try:
-            dstVolume.prepare()
-            try:
-                # By definition "sparsification" is implemented writing a file
-                # with zeroes as large as the entire file-system. So at least
-                # tmpVolume needs to be as large as the virtual disk size for
-                # the worst case.
-                # TODO: Some extra space may be needed for QCOW2 headers
-                tmpVolume.extend(tmpVolume.getSize())
-                # For the dstVolume we may think of an optimization where the
-                # extension is as large as the source (and at the end we
-                # shrinkToOptimalSize).
-                # TODO: Extend the dstVolume only as much as the actual size of
-                # srcVolume
-                # TODO: Some extra space may be needed for QCOW2 headers
-                dstVolume.extend(tmpVolume.getSize())
-
-                srcFormat = volume.fmt2str(srcVolume.getFormat())
-                dstFormat = volume.fmt2str(dstVolume.getFormat())
-
-                virtsparsify.sparsify(srcVolume.getVolumePath(),
-                                      tmpVolume.getVolumePath(),
-                                      dstVolume.getVolumePath(),
-                                      src_format=srcFormat,
-                                      dst_format=dstFormat)
-            except Exception:
-                self.log.exception('Unexpected error sparsifying %s',
-                                   tmpVolUUID)
-                raise se.CannotSparsifyVolume(tmpVolUUID)
-            finally:
-                dstVolume.teardown(sdUUID=dstSdUUID, volUUID=dstVolUUID)
-        finally:
-            tmpVolume.teardown(sdUUID=tmpSdUUID, volUUID=tmpVolUUID)
-
-        tmpVolume.shrinkToOptimalSize()
-        dstVolume.shrinkToOptimalSize()
 
     def cloneStructure(self, sdUUID, imgUUID, dstSdUUID):
         self._createTargetImage(sdCache.produce(dstSdUUID), sdUUID, imgUUID)
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index af838de..da4e320 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -1616,48 +1616,6 @@
             image.Image(self.poolPath).move(srcDomUUID, dstDomUUID, imgUUID,
                                             vmUUID, op, postZero, force)
 
-    def sparsifyImage(self, tmpSdUUID, tmpImgUUID, tmpVolUUID, dstSdUUID,
-                      dstImgUUID, dstVolUUID):
-        """
-        Reduce sparse image size by converting free space on image to free
-        space on host using virt-sparsify.
-
-        :param tmpSdUUID: The UUID of the storage domain where the temporary
-                            snapshot of source volume exists.
-        :type tmpSdUUID: UUUID
-        :param tmpImgUUID: The UUID of the temporary snapshot image.
-        :type tmpImgUUID: UUID
-        :param tmpVolUUID: The UUID of the temporary snapshot volume that needs
-                            to be sparsified.
-        :type tmpVolUUID: UUID
-        :param dstSdUUID: The UUID of the storage domain where the destination
-                            image exists.
-        :type dstSdUUID: UUUID
-        :param dstImgUUID: The UUID of the destination image to which the
-                            destination volume belongs.
-        :type dstImgUUID: UUID
-        :param dstVolUUID: The UUID of the destination volume for the
-                            sparsified volume.
-        :type dstVolUUID: UUID
-        """
-        srcNamespace = sd.getNamespace(tmpSdUUID, IMAGE_NAMESPACE)
-        dstNamespace = sd.getNamespace(dstSdUUID, IMAGE_NAMESPACE)
-
-        # virt-sparsify writes to temporary volume when using --tmp:prebuilt,
-        # so we acquire exclusive lock for the temporary image.
-        # Destination image is where the sparsified volume gets written to, so
-        # we acquire exclusive lock for the destination image too.
-        # Since source volume is only a parent of temporary volume, we don't
-        # need to acquire any lock for it.
-        with nested(
-            rmanager.acquireResource(srcNamespace, tmpImgUUID,
-                                     rm.LockType.exclusive),
-            rmanager.acquireResource(dstNamespace, dstImgUUID,
-                                     rm.LockType.exclusive)):
-            image.Image(self.poolPath).sparsify(
-                tmpSdUUID, tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID,
-                dstVolUUID)
-
     def cloneImageStructure(self, sdUUID, imgUUID, dstSdUUID):
         """
         Clone an image structure from a source domain to a destination domain


-- 
To view, visit https://gerrit.ovirt.org/57347
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ac2bb1fbd2acbe0fc47694d17313c6ccd01a227
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shmuel Leib Melamud <smelamud at redhat.com>
Gerrit-Reviewer: Shmuel Melamud <smelamud at redhat.com>


More information about the vdsm-patches mailing list