Change in vdsm[master]: qemuimg: add support for convert progress

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: qemuimg: add support for convert progress
......................................................................

qemuimg: add support for convert progress

Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
---
M lib/vdsm/qemuimg.py
M tests/qemuimgTests.py
M vdsm/storage/image.py
3 files changed, 172 insertions(+), 58 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/10/33910/1

diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py
index 39cd394..9ca9c58 100644
--- a/lib/vdsm/qemuimg.py
+++ b/lib/vdsm/qemuimg.py
@@ -160,9 +160,9 @@
     return check
 
 
-def convert(srcImage, dstImage, stop, srcFormat=None, dstFormat=None,
+def convert(srcImage, dstImage, srcFormat=None, dstFormat=None,
             backing=None, backingFormat=None):
-    cmd = [_qemuimg.cmd, "convert", "-t", "none"]
+    cmd = [_qemuimg.cmd, "convert", "-p", "-t",
"none"]
     options = []
     cwdPath = None
 
@@ -190,14 +190,66 @@
 
     cmd.append(dstImage)
 
-    (rc, out, err) = utils.watchCmd(
-        cmd, cwd=cwdPath, stop=stop, nice=utils.NICENESS.HIGH,
-        ioclass=utils.IOCLASS.IDLE)
+    return QemuImgProcess(cmd, cwd=cwdPath)
 
-    if rc != 0:
-        raise QImgError(rc, out, err)
 
-    return (rc, out, err)
+class QemuImgProcess(object):
+    REGEXPR = re.compile(r'\(([\d.]+)/100%\)')
+
+    def __init__(self, cmd, cwd=None):
+        self.progress = 0.0
+
+        self._stdout = bytearray()
+        self._stderr = bytearray()
+
+        cmd = utils.ionice_cmd(cmd, utils.IOCLASS.IDLE)
+        cmd = utils.nice_cmd(cmd, utils.NICENESS.HIGH)
+
+        self.execution = utils.CommandStream(
+            cmd, self._recvstdout, self._recvstderr, cwd=cwd,
+            deathSignal=signal.SIGKILL)
+
+    def _recvstderr(self, buffer):
+        self._stderr += buffer
+
+    def _recvstdout(self, buffer):
+        last_progress = None
+        self._stdout += buffer
+
+        while True:
+            try:
+                idx = self._stdout.index('\r')
+            except ValueError:
+                break
+
+            last_progress = self._stdout[:idx]
+            del self._stdout[:idx + 1]
+
+        if last_progress:
+            m = self.REGEXPR.match(last_progress.strip())
+            if m is None:
+                raise ValueError(
+                    'Unable to parse: "%s"' % last_progress)
+            self.progress = float(m.group(1))
+
+    @property
+    def stderr(self):
+        return str(self._stderr)
+
+    def wait(self, timeout=None):
+        returncode = self.execution.wait(timeout=timeout)
+
+        if (self.execution.returncode is not None
+                and self.execution.returncode != 0):
+            raise QImgError(returncode, "", self.stderr)
+
+        return returncode
+
+    def terminate(self):
+        self.execution.terminate()
+
+    def kill(self):
+        self.execution.kill()
 
 
 def resize(image, newSize, format=None):
diff --git a/tests/qemuimgTests.py b/tests/qemuimgTests.py
index abc0750..9e61de0 100644
--- a/tests/qemuimgTests.py
+++ b/tests/qemuimgTests.py
@@ -163,12 +163,11 @@
 
     def test_no_format(self):
         def convert(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'src', 'dst']
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'src', 'dst']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
-        with FakeCmd(utils, 'watchCmd', convert):
-            qemuimg.convert('src', 'dst', True)
+        with FakeCmd(qemuimg, 'QemuImgProcess', convert):
+            qemuimg.convert('src', 'dst')
 
     def test_qcow2_compat_unsupported(self):
         def qcow2_compat_unsupported(cmd, **kw):
@@ -176,14 +175,13 @@
             return 0, 'Supported options:\nsize ...\n', ''
 
         def convert(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'src', '-O',
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'src', '-O',
                         'qcow2', 'dst']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', qcow2_compat_unsupported):
-            with FakeCmd(utils, 'watchCmd', convert):
-                qemuimg.convert('src', 'dst', True,
dstFormat='qcow2')
+            with FakeCmd(qemuimg, 'QemuImgProcess', convert):
+                qemuimg.convert('src', 'dst', dstFormat='qcow2')
 
     def qcow2_compat_supported(self, cmd, **kw):
         self.check_supports_qcow2_compat(cmd, **kw)
@@ -191,14 +189,13 @@
 
     def test_qcow2_compat_supported(self):
         def convert(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'src', '-O',
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'src', '-O',
                         'qcow2', '-o', 'compat=0.10',
'dst']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):
-            with FakeCmd(utils, 'watchCmd', convert):
-                qemuimg.convert('src', 'dst', True,
dstFormat='qcow2')
+            with FakeCmd(qemuimg, 'QemuImgProcess', convert):
+                qemuimg.convert('src', 'dst', dstFormat='qcow2')
 
     def check_supports_qcow2_compat(self, cmd, **kw):
         expected = [QEMU_IMG, 'convert', '-O', 'qcow2',
'-o', '?', '/dev/null',
@@ -207,49 +204,95 @@
 
     def test_qcow2_no_backing_file(self):
         def qcow2_no_backing_file(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'source', '-O',
-                        'qcow2', '-o', 'compat=0.10',
'target']
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'source',
+                        '-O', 'qcow2', '-o',
'compat=0.10', 'target']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):
-            with FakeCmd(utils, 'watchCmd', qcow2_no_backing_file):
-                qemuimg.convert('source', 'target', None,
dstFormat='qcow2')
+            with FakeCmd(qemuimg, 'QemuImgProcess', qcow2_no_backing_file):
+                qemuimg.convert('source', 'target',
dstFormat='qcow2')
 
     def test_qcow2_backing_file(self):
         def qcow2_backing_file(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'source', '-O',
-                        'qcow2', '-o',
'compat=0.10,backing_file=backing',
-                        'target']
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'source',
+                        '-O', 'qcow2',
+                        '-o', 'compat=0.10,backing_file=backing',
'target']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):
-            with FakeCmd(utils, 'watchCmd', qcow2_backing_file):
-                qemuimg.convert('source', 'target', None,
dstFormat='qcow2',
+            with FakeCmd(qemuimg, 'QemuImgProcess', qcow2_backing_file):
+                qemuimg.convert('source', 'target',
dstFormat='qcow2',
                                 backing='backing')
 
     def test_qcow2_backing_format(self):
         def qcow2_backing_format(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'source', '-O',
-                        'qcow2', '-o', 'compat=0.10',
'target']
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'source',
+                        '-O', 'qcow2', '-o',
'compat=0.10', 'target']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):
-            with FakeCmd(utils, 'watchCmd', qcow2_backing_format):
-                qemuimg.convert('source', 'target', None,
dstFormat='qcow2',
+            with FakeCmd(qemuimg, 'QemuImgProcess', qcow2_backing_format):
+                qemuimg.convert('source', 'target',
dstFormat='qcow2',
                                 backingFormat='qcow2')
 
     def test_qcow2_backing_file_and_format(self):
         def qcow2_backing_format(cmd, **kw):
-            expected = [QEMU_IMG, 'convert', '-t', 'none',
'source', '-O',
-                        'qcow2', '-o',
'compat=0.10,backing_file=backing,'
+            expected = [QEMU_IMG, 'convert', '-p', '-t',
'none', 'source',
+                        '-O', 'qcow2',
+                        '-o', 'compat=0.10,backing_file=backing,'
                         'backing_fmt=qcow2', 'target']
             self.assertEqual(cmd, expected)
-            return 0, '', ''
 
         with FakeCmd(utils, 'execCmd', self.qcow2_compat_supported):
-            with FakeCmd(utils, 'watchCmd', qcow2_backing_format):
-                qemuimg.convert('source', 'target', None,
dstFormat='qcow2',
+            with FakeCmd(qemuimg, 'QemuImgProcess', qcow2_backing_format):
+                qemuimg.convert('source', 'target',
dstFormat='qcow2',
                                 backing='backing',
backingFormat='qcow2')
+
+
+class QemuImgProcessTests(TestCaseBase):
+    PROGRESS_FORMAT = "    (%.2f/100%%)\r"
+
+    @staticmethod
+    def _progress_iterator():
+        return map(lambda x: x / 100.0, xrange(0, 10000, 1))
+
+    def test_progress_simple(self):
+        p = qemuimg.QemuImgProcess([])
+
+        for progress in self._progress_iterator():
+            p._recvstdout(self.PROGRESS_FORMAT % progress)
+            self.assertEquals(p.progress, progress)
+
+        self.assertEquals(p.wait(), 0)
+
+    def test_progress_incomplete(self):
+        p = qemuimg.QemuImgProcess([])
+
+        for progress in self._progress_iterator():
+            stdout = self.PROGRESS_FORMAT % progress
+            p._recvstdout(stdout[:12])
+            p._recvstdout(stdout[12:])
+            self.assertEquals(p.progress, progress)
+
+        self.assertEquals(p.wait(), 0)
+
+    def test_progress_batch(self):
+        p = qemuimg.QemuImgProcess([])
+
+        p._recvstdout(
+            (self.PROGRESS_FORMAT % 10.00) +
+            (self.PROGRESS_FORMAT % 25.00) +
+            (self.PROGRESS_FORMAT % 33.33))
+
+        self.assertEquals(p.progress, 33.33)
+        self.assertEquals(p.wait(), 0)
+
+    def test_unexpected_output(self):
+        p = qemuimg.QemuImgProcess([])
+
+        self.assertRaises(ValueError, p._recvstdout, "Hello World\r")
+
+        p._recvstdout("Hello ")
+        self.assertRaises(ValueError, p._recvstdout, "World\r")
+
+        self.assertEquals(p.wait(), 0)
diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py
index 41e3f30..cdcd82a 100644
--- a/vdsm/storage/image.py
+++ b/vdsm/storage/image.py
@@ -93,6 +93,7 @@
     """
     log = logging.getLogger('Storage.Image')
     _fakeTemplateLock = threading.Lock()
+    _QEMU_LOGGING_INTERVAL = 60.0
 
     @classmethod
     def createImageRollback(cls, taskObj, imageDir):
@@ -109,6 +110,24 @@
 
     def __init__(self, repoPath):
         self.repoPath = repoPath
+
+    def qemuImgConvert(self, *args, **kwargs):
+        self.log.debug('starting qemu-img operation')
+        command = qemuimg.convert(*args, **kwargs)
+
+        def abortImgConversion():
+            self.log.info('aborting ongoing qemu-img operation')
+            command.terminate()
+
+        retcode = None
+
+        with vars.task.abort_callback(abortImgConversion):
+            while retcode is None:
+                retcode = command.wait(self._QEMU_LOGGING_INTERVAL)
+                self.log.debug('qemu-img operation progress: %s%%',
+                               command.progress)
+
+        self.log.debug('qemu-img operation has completed: %s', retcode)
 
     def create(self, sdUUID, imgUUID):
         """Create placeholder for image's volumes
@@ -444,13 +463,12 @@
                         backingFormat = None
 
                     self.log.debug("start qemu convert")
-                    qemuimg.convert(srcVol.getVolumePath(),
-                                    dstVol.getVolumePath(),
-                                    vars.task.aborting,
-                                    srcFormat=srcFormat,
-                                    dstFormat=dstFormat,
-                                    backing=backing,
-                                    backingFormat=backingFormat)
+                    self.qemuImgConvert(srcVol.getVolumePath(),
+                                        dstVol.getVolumePath(),
+                                        srcFormat=srcFormat,
+                                        dstFormat=dstFormat,
+                                        backing=backing,
+                                        backingFormat=backingFormat)
                 except ActionStopped:
                     raise
                 except se.StorageException:
@@ -830,10 +848,11 @@
                 dstVol.prepare(rw=True, setrw=True)
 
                 try:
-                    qemuimg.convert(volParams['path'], dstPath,
-                                    vars.task.aborting,
-                                    volume.fmt2str(volParams['volFormat']),
-                                    volume.fmt2str(dstVolFormat))
+                    self.qemuImgConvert(
+                        volParams['path'],
+                        dstPath,
+                        srcFormat=volume.fmt2str(volParams['volFormat']),
+                        dstFormat=volume.fmt2str(dstVolFormat))
                 except ActionStopped:
                     raise
                 except qemuimg.QImgError:
@@ -1045,7 +1064,7 @@
         # volume and rebase successor's children (if exists) on top of it.
         # Step 1: Create an empty volume named sucessor_MERGE similar to
         #         ancestor volume.
-        # Step 2: qemuimg.convert successor -> sucessor_MERGE
+        # Step 2: qemuImgConvert successor -> sucessor_MERGE
         # Step 3: Rename successor to _remove_me__successor
         # Step 4: Rename successor_MERGE to successor
         # Step 5: Unsafely rebase successor's children on top of temporary
@@ -1071,11 +1090,11 @@
                 # Step 2: Convert successor to new volume
                 #   qemu-img convert -f qcow2 successor -O raw newUUID
                 try:
-                    qemuimg.convert(srcVolParams['path'],
-                                    newVol.getVolumePath(),
-                                    vars.task.aborting,
-                                    volume.fmt2str(srcVolParams['volFormat']),
-                                    volume.fmt2str(volParams['volFormat']))
+                    self.qemuImgConvert(
+                        srcVolParams['path'],
+                        newVol.getVolumePath(),
+                        srcFormat=volume.fmt2str(srcVolParams['volFormat']),
+                        dstFormat=volume.fmt2str(volParams['volFormat']))
                 except qemuimg.QImgError:
                     self.log.exception('conversion failure for volume %s',
                                        srcVol.volUUID)


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 1:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/12828/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/11880/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/12671/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 2:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13387/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12597/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created_staging/163/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13549/ : FAILURE

-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

Francesco Romani has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 2:

(5 comments)

more initial comments

http://gerrit.ovirt.org/#/c/33910/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 196: class QemuImgProcess(object):
Line 197:     REGEXPR = re.compile(r'\(([\d.]+)/100%\)')
Line 198: 
Line 199:     def __init__(self, cmd, cwd=None):
Line 200:         self.progress = 0.0
maybe it is just me, but I'd like it more as read-only property built on a private
field.
Line 201: 
Line 202:         self._stdout = bytearray()
Line 203:         self._stderr = bytearray()
Line 204: 


Line 204: 
Line 205:         cmd = utils.ionice_cmd(cmd, utils.IOCLASS.IDLE)
Line 206:         cmd = utils.nice_cmd(cmd, utils.NICENESS.HIGH)
Line 207: 
Line 208:         self.execution = utils.CommandStream(
couldn't this be made _private?
Line 209:             cmd, self._recvstdout, self._recvstderr, cwd=cwd,
Line 210:             deathSignal=signal.SIGKILL)
Line 211: 
Line 212:     def _recvstderr(self, buffer):


Line 219:         while True:
Line 220:             try:
Line 221:                 idx = self._stdout.index('\r')
Line 222:             except ValueError:
Line 223:                 break
it seems to me that if there are multiple progress report to parse, everyone but the last
one will be overwritten
and silently discarded.

If the above is right, why don't just do rindex (or rfind) once instead of looping?
Line 224: 
Line 225:             last_progress = self._stdout[:idx]
Line 226:             del self._stdout[:idx + 1]
Line 227: 


Line 222:             except ValueError:
Line 223:                 break
Line 224: 
Line 225:             last_progress = self._stdout[:idx]
Line 226:             del self._stdout[:idx + 1]
we discussed about a possible deque usage, although I find it clearer, I can happily live
without.
Line 227: 
Line 228:         if last_progress:
Line 229:             m = self.REGEXPR.match(last_progress.strip())
Line 230:             if m is None:


http://gerrit.ovirt.org/#/c/33910/2/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 110: 
Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def qemuImgConvert(self, *args, **kwargs):
No big deal, but could this be made a function, instead of a method? it seems to use very
little of the class context.
Line 115:         self.log.debug('starting qemu-img operation')
Line 116:         command = qemuimg.convert(*args, **kwargs)
Line 117: 
Line 118:         def abortImgConversion():


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.ovirt.org/#/c/33910/2/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 204: 
Line 205:         cmd = utils.ionice_cmd(cmd, utils.IOCLASS.IDLE)
Line 206:         cmd = utils.nice_cmd(cmd, utils.NICENESS.HIGH)
Line 207: 
Line 208:         self.execution = utils.CommandStream(
>  couldn't this be made _private? 
Done
Line 209:             cmd, self._recvstdout, self._recvstderr, cwd=cwd,
Line 210:             deathSignal=signal.SIGKILL)
Line 211: 
Line 212:     def _recvstderr(self, buffer):


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 3:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13567/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14524/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14356/ : SUCCESS

-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/33910/3/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 122:         retcode = None
Line 123: 
Line 124:         with vars.task.abort_callback(abortImgConversion):
Line 125:             while retcode is None:
Line 126:                 retcode = command.wait(self._QEMU_LOGGING_INTERVAL)
I wonder: is there a textbook resolution to this race between waitpid() and kill()? Are we
ever to rely on the slowness of pid recycling?
Line 127:                 self.log.debug('qemu-img operation progress: %s%%',
Line 128:                                command.progress)
Line 129: 
Line 130:         self.log.debug('qemu-img operation has completed: %s', retcode)


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.ovirt.org/#/c/33910/3/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 122:         retcode = None
Line 123: 
Line 124:         with vars.task.abort_callback(abortImgConversion):
Line 125:             while retcode is None:
Line 126:                 retcode = command.wait(self._QEMU_LOGGING_INTERVAL)
>  I wonder: is there a textbook resolution to this race between
> waitpid() and 
We wait for the process - as soon as it existed, we can mark the
process as exited, so the task abort callback has no effect.

I don't see practical race - maybe I'm missing something?
Line 127:                 self.log.debug('qemu-img operation progress: %s%%',
Line 128:                                command.progress)
Line 129: 
Line 130:         self.log.debug('qemu-img operation has completed: %s', retcode)


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/33910/3/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 257:             raise QImgError(returncode, "", self.stderr)
Line 258: 
Line 259:         return self._command.returncode
Line 260: 
Line 261:     def terminate(self):
If I understand correctly Dan concern about a race - we can check here the process status
before terminating:

    if self._command.poll() is None:
        self._command.terminate()
Line 262:         self._command.terminate()
Line 263: 
Line 264:     def kill(self):
Line 265:         self._command.kill()


Line 260: 
Line 261:     def terminate(self):
Line 262:         self._command.terminate()
Line 263: 
Line 264:     def kill(self):
Same as terminate.
Line 265:         self._command.kill()
Line 266: 
Line 267: 
Line 268: def resize(image, newSize, format=None):


-- 
To view, visit http://gerrit.ovirt.org/33910
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 4:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16806/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16978/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 5:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16902/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 5:

Build Started (2/2) ->
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17074/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 5:

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16902/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17074/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18264/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

Build Started (2/2)

0 -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1494/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6: Code-Review-1 Verified-1

Build Failed 

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/18264/ : UNSTABLE

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/1494/ : 0

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

(2 comments)

Nice, but I think we should simplify and clarify the code receiving data.

Looked only at the new QimgProgress code, will look at the rest later today.

https://gerrit.ovirt.org/#/c/33910/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 224:         self._term_lock = threading.Lock()
Line 225: 
Line 226:         _log.debug(utils.commandLogFormat(cmd, cwd=cwd))
Line 227:         self._command = CPopen(cmd, cwd=cwd, deathSignal=signal.SIGKILL)
Line 228:         self._execution = utils.CommandStream(
_execution is a strange term here - this is a CommandStream - this is a good example that
CommandStream is not a good name.

How about:

    self._receiver = utils.CommandRceiver(...)

I think this is what this class does - allow you to register recv_stdout and recv_stderr
and call wait until there is nothing to receive.
Line 229:             self._command, self._recvstdout, self._recvstderr)
Line 230: 
Line 231:     def _recvstderr(self, buffer):
Line 232:         self._stderr += buffer


Line 241:             except ValueError:
Line 242:                 break
Line 243: 
Line 244:             last_progress = self._stdout[:idx]
Line 245:             del self._stdout[:idx + 1]
I think it will be simpler and more clear:

    end = buffer.rindex('\r')
    if end < 0:
        self._stdout += buffer
        return  # Needs more data
 
    received = self._stdout + buffer[:end]
    self._stdout = buffer[end + 1:]

    # Skip older progress chunks
    last_progress = received.rsplit('\r', 1)[-1]

    handle last_progress...
Line 246: 
Line 247:         if last_progress:
Line 248:             m = self.REGEXPR.match(last_progress.strip())
Line 249:             if m is None:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

(1 comment)

https://gerrit.ovirt.org/#/c/33910/6//COMMIT_MSG
Commit Message:

Line 13:  command = qemuimg.convert(srcImage, dstImage)
Line 14: 
Line 15:  while not command.finished:
Line 16:      command.wait(10)
Line 17:      log.debug('command progress: %s%%', command.progress)
This is kind of generic interface for async commands with progress - we should look at
this later.
Line 18: 
Line 19: Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 6:

(9 comments)

Reviewed everything, suggested some cleanups.

https://gerrit.ovirt.org/#/c/33910/6/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 174: 
Line 175: 
Line 176: def convert(srcImage, dstImage, srcFormat=None, dstFormat=None,
Line 177:             backing=None, backingFormat=None):
Line 178:     cmd = [_qemuimg.cmd, "convert", "-p", "-t",
"none"]
Is -p available on all platforms (fedora >= 21, rhel >= 7.1, centos >= 7.1,
debian?)?
Line 179:     options = []
Line 180:     cwdPath = None
Line 181: 
Line 182:     if _supports_src_cache('convert'):


https://gerrit.ovirt.org/#/c/33910/6/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 110: 
Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def qemuImgConvert(self, *args, **kwargs):
Using *args and **kwargs is a bit too lazy here.
Line 115:         self.log.debug('starting qemu-img operation')
Line 116:         command = qemuimg.convert(*args, **kwargs)
Line 117: 
Line 118:         def abortImgConversion():


Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def qemuImgConvert(self, *args, **kwargs):
Line 115:         self.log.debug('starting qemu-img operation')
operation -> convert
Line 116:         command = qemuimg.convert(*args, **kwargs)
Line 117: 
Line 118:         def abortImgConversion():
Line 119:             self.log.info('aborting ongoing qemu-img operation')


Line 116:         command = qemuimg.convert(*args, **kwargs)
Line 117: 
Line 118:         def abortImgConversion():
Line 119:             self.log.info('aborting ongoing qemu-img operation')
Line 120:             command.terminate()
Do we need this helper? Don't we have a log in task.py?

We can use:

    with vars.task.abort_callback(command.terminate):
        ...
Line 121: 
Line 122:         with vars.task.abort_callback(abortImgConversion):
Line 123:             while not command.finished:
Line 124:                 command.wait(self._QEMU_LOGGING_INTERVAL)


Line 121: 
Line 122:         with vars.task.abort_callback(abortImgConversion):
Line 123:             while not command.finished:
Line 124:                 command.wait(self._QEMU_LOGGING_INTERVAL)
Line 125:                 self.log.debug('qemu-img operation progress: %s%%',
operation -> convert
Line 126:                                command.progress)
Line 127: 
Line 128:         self.log.debug('qemu-img operation has completed')
Line 129: 


Line 124:                 command.wait(self._QEMU_LOGGING_INTERVAL)
Line 125:                 self.log.debug('qemu-img operation progress: %s%%',
Line 126:                                command.progress)
Line 127: 
Line 128:         self.log.debug('qemu-img operation has completed')
operation -> convert
Line 129: 
Line 130:     def create(self, sdUUID, imgUUID):
Line 131:         """Create placeholder for image's volumes
Line 132:             'sdUUID' - storage domain UUID


Line 465:                                         dstVol.getVolumePath(),
Line 466:                                         srcFormat=srcFormat,
Line 467:                                         dstFormat=dstFormat,
Line 468:                                         backing=backing,
Line 469:                                         backingFormat=backingFormat)
Lets create the command here, and move the waiting loop into QemuImgProcess.wait():

    cmd = qemuimg.convert(...)
    
    with vars.task.abort_callback(cmd.terminate):
        cmd.wait(progress_interval=30)

Also move logging into qemuimg.convert so we get proper logging from any caller without
duplicating logging.

Another option - create the command here, and add _wait_for_qemuimg(cmd) method for
waiting. But I think that having a private method is usually a sign that the code wants to
be in another place.
Line 470:                 except ActionStopped:
Line 471:                     raise
Line 472:                 except se.StorageException:
Line 473:                     self.log.error("Unexpected error", exc_info=True)


Line 849:                     self.qemuImgConvert(
Line 850:                         volParams['path'],
Line 851:                         dstPath,
Line 852:                        
srcFormat=volume.fmt2str(volParams['volFormat']),
Line 853:                         dstFormat=volume.fmt2str(dstVolFormat))
Same two lines for waiting should be fine also here.
Line 854:                 except ActionStopped:
Line 855:                     raise
Line 856:                 except qemuimg.QImgError as e:
Line 857:                     self.log.exception('conversion failure for volume
%s',


Line 1091:                     self.qemuImgConvert(
Line 1092:                         srcVolParams['path'],
Line 1093:                         newVol.getVolumePath(),
Line 1094:                        
srcFormat=volume.fmt2str(srcVolParams['volFormat']),
Line 1095:                        
dstFormat=volume.fmt2str(volParams['volFormat']))
And here.
Line 1096:                 except qemuimg.QImgError:
Line 1097:                     self.log.exception('conversion failure for volume
%s',
Line 1098:                                        srcVol.volUUID)
Line 1099:                     raise se.MergeSnapshotsError(newUUID)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(4 comments)

Added some comments on the code, will review the tests later.

https://gerrit.ovirt.org/#/c/33910/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 241:             idx = self._stdout.rindex('\r')
Line 242:         except ValueError:
Line 243:             return
Line 244: 
Line 245:         last_progress = self._stdout[:idx].rsplit('\r', 1)[-1]
This line is a little bit too complex, would be nicer as two simpler steps, and maybe a
diagram showing what we look for:

    # [ (31/100%) \r (32/100%) \r (33/1 ]
Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
Line 249:         if m is None:


Line 244: 
Line 245:         last_progress = self._stdout[:idx].rsplit('\r', 1)[-1]
Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
Why not add leading and trailing whitespace into the regex? This will perform unneeded
copy of the string.
Line 249:         if m is None:
Line 250:             raise ValueError('Unable to parse: "%s"' %
last_progress)
Line 251: 
Line 252:         self._progress = float(m.group(1))


Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
Line 249:         if m is None:
Line 250:             raise ValueError('Unable to parse: "%s"' %
last_progress)
%r is better here
Line 251: 
Line 252:         self._progress = float(m.group(1))
Line 253: 
Line 254:     @property


Line 269:         if not self._stream.closed:
Line 270:             return
Line 271: 
Line 272:         with self._term_lock:
Line 273:             self._command.wait()
Taking the lock here means that terminate will wait forever if we call from another
thread. We want to allow termination while waiting for the command to exit, right?
Line 274: 
Line 275:         if self._command.returncode != 0:
Line 276:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 277: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(3 comments)

https://gerrit.ovirt.org/#/c/33910/7/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 368:     PROGRESS_FORMAT = "    (%.2f/100%%)\r"
Line 369: 
Line 370:     @staticmethod
Line 371:     def _progress_iterator():
Line 372:         return map(lambda x: x / 100.0, xrange(0, 10000, 1))
Why not use a generator?

    for value in xrange(0, 10000, 1):
        yield value / 100.0
Line 373: 
Line 374:     def test_failure(self):
Line 375:         p = qemuimg.QemuImgProcess(['false'])
Line 376:         self.assertRaises(qemuimg.QImgError, p.wait)


Line 378:     def test_progress_simple(self):
Line 379:         p = qemuimg.QemuImgProcess(['true'])
Line 380: 
Line 381:         for progress in self._progress_iterator():
Line 382:             p._recvstdout(self.PROGRESS_FORMAT % progress)
This works but it assumes too much internal knowledge. I think we would like to run a fake
qemu-img program that generate expected outputs and errors instead of this.

Another option, mock it so you can replace the process stdout with a StringIO returning
fake output.
Line 383:             self.assertEquals(p.progress, progress)
Line 384: 
Line 385:         p.wait()
Line 386:         self.assertEquals(p.finished, True)


Line 384: 
Line 385:         p.wait()
Line 386:         self.assertEquals(p.finished, True)
Line 387: 
Line 388:     def test_progress_incomplete(self):
This test would be more clear if we limit the test to only few iterations, and have
constant output and result that we can see - for example:

    @permutations([
        # qemu-img output, progress value
        (("    (1/100%)\r", "    (2/100%)\r"), (1, 2)),
        (("    (1/10", "0%)\r    (2/100%)\r"), (1, 2)),
    ])
Line 389:         p = qemuimg.QemuImgProcess(['true'])
Line 390: 
Line 391:         old_progress = 0.0
Line 392: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/33910/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 269:         if not self._stream.closed:
Line 270:             return
Line 271: 
Line 272:         with self._term_lock:
Line 273:             self._command.wait()
>  Taking the lock here means that terminate will wait forever if we
> call from 
If the stream is closed the command should exit very quickly.
Line 274: 
Line 275:         if self._command.returncode != 0:
Line 276:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 277: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/33910/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 269:         if not self._stream.closed:
Line 270:             return
Line 271: 
Line 272:         with self._term_lock:
Line 273:             self._command.wait()
>  If the stream is closed the command should exit very quickly.
We should not assume this.

Do you have any idea why we need the lock?
Line 274: 
Line 275:         if self._command.returncode != 0:
Line 276:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 277: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/33910/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 269:         if not self._stream.closed:
Line 270:             return
Line 271: 
Line 272:         with self._term_lock:
Line 273:             self._command.wait()
>  We should not assume this. 
It seems designed merely to prevent
terminating the process after it has closed stdout.  I really don't think it's
necessary.
Line 274: 
Line 275:         if self._command.returncode != 0:
Line 276:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 277: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 8:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 8: Verified+1

Tested with the following engine flows:

* Offline move disk
* Clone disk
* Create template from VM
* Offline delete snapshot where base == RAW

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 9:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 9: Code-Review-1

(14 comments)

https://gerrit.ovirt.org/#/c/33910/9/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 22: import logging
Line 23: import os
Line 24: import re
Line 25: import signal
Line 26: import threading
Not needed as we are removing the terminate lock.
Line 27: 
Line 28: from cpopen import CPopen
Line 29: 
Line 30: from . import utils


Line 210: 
Line 211:     return QemuImgProcess(cmd, cwd=cwdPath)
Line 212: 
Line 213: 
Line 214: class QemuImgProcess(object):
Lets rename to QemuImgOperation

I would like to have uniform interface for "operations", regardless of the
implementation (running a command, waiting for event, whatever).
Line 215:     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
Line 216: 
Line 217:     def __init__(self, cmd, cwd=None):
Line 218:         self._progress = 0.0


Line 220:         self._stdout = bytearray()
Line 221:         self._stderr = bytearray()
Line 222: 
Line 223:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 224:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
This line may need now cmdutils.taskset if vdsm is using cpu_affinity. Please check how
this is done in utils.py.

We probably need to create a helper in utils to wrap a command with taskset if needed,
since we want to use CPopen directly, not only in execCmd.
Line 225: 
Line 226:         self._term_lock = threading.Lock()
Line 227: 
Line 228:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))


Line 222: 
Line 223:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 224:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
Line 225: 
Line 226:         self._term_lock = threading.Lock()
Didn't we agree that we don't need this lock?
Line 227: 
Line 228:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))
Line 229:         self._command = CPopen(cmd, cwd=cwd, deathSignal=signal.SIGKILL)
Line 230:         self._stream = utils.CommandStream(


Line 263:         return self._progress
Line 264: 
Line 265:     @property
Line 266:     def stderr(self):
Line 267:         return str(self._stderr)
lets rename to "error"

Better match for the "operation" concept.
Line 268: 
Line 269:     @property
Line 270:     def finished(self):
Line 271:         return self._command.returncode is not None


Line 267:         return str(self._stderr)
Line 268: 
Line 269:     @property
Line 270:     def finished(self):
Line 271:         return self._command.returncode is not None
This should be self._command.poll() - returncode is not updated until you poll.
Line 272: 
Line 273:     def wait(self, timeout=None):
Line 274:         self._stream.receive(timeout=timeout)
Line 275: 


Line 277:             return
Line 278: 
Line 279:         with self._term_lock:
Line 280:             self._command.wait()
Line 281: 
If the command was aborted, we should raise Aborted error instead, to prevent unhelpful
and confusing traceback in the thread waiting for this.
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 284: 
Line 285:     def terminate(self):


Line 279:         with self._term_lock:
Line 280:             self._command.wait()
Line 281: 
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
log here the command termination using cmdutils.retcode_log_line - not having an end log
makes debugging.

It should be impossible to use this class without having proper logging.
Line 284: 
Line 285:     def terminate(self):
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:


Line 281: 
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 284: 
Line 285:     def terminate(self):
Lets rename this to abort(), matching new Job interface.
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:
Line 288:                 self._command.terminate()
Line 289: 


Line 284: 
Line 285:     def terminate(self):
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:
Line 288:                 self._command.terminate()
This should do:

    if self._command.poll() is None:
        self._aborted = True
        self._command.terminate()

This will wake up the waiting thread and it will exit correctly. It can check the _aborted
flag to log stuff correctly.  There is no need to log a huge traceback when the process
was aborted by the user.
Line 289: 
Line 290: 
Line 291: def resize(image, newSize, format=None):
Line 292:     cmd = [_qemuimg.cmd, "resize"]


https://gerrit.ovirt.org/#/c/33910/9/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 110: 
Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def _qemuTaskProgressWait(self, command):
Lets rename this to _wait_for_qemuimg_operation(self, op)
Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')


Line 113: 
Line 114:     def _qemuTaskProgressWait(self, command):
Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
We don't need this inline function, we can use command.abort directly.

The log belong to command.abort, so we don't have to duplicate it in all users of the
operation.
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.terminate()
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):


Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.terminate()
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):
Use 

    with vars.task.abort_callback(op.abort):
Line 122:             while not command.finished:
Line 123:                 command.wait(self._QEMU_LOGGING_INTERVAL)
Line 124:                 self.log.debug('qemu-img operation progress: %s%%',
Line 125:                                command.progress)


Line 458:                     else:
Line 459:                         backing = None
Line 460:                         backingFormat = None
Line 461: 
Line 462:                     qemucmd = qemuimg.convert(
Lets call this "operation" or "op"
Line 463:                         srcVol.getVolumePath(),
Line 464:                         dstVol.getVolumePath(),
Line 465:                         srcFormat=srcFormat,
Line 466:                         dstFormat=dstFormat,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 10:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 9:

(10 comments)

https://gerrit.ovirt.org/#/c/33910/9/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 22: import logging
Line 23: import os
Line 24: import re
Line 25: import signal
Line 26: import threading
>  Not needed as we are removing the terminate lock. 
Done
Line 27: 
Line 28: from cpopen import CPopen
Line 29: 
Line 30: from . import utils


Line 210: 
Line 211:     return QemuImgProcess(cmd, cwd=cwdPath)
Line 212: 
Line 213: 
Line 214: class QemuImgProcess(object):
>  Lets rename to QemuImgOperation 
Done
Line 215:     REGEXPR = re.compile(r'\s*\(([\d.]+)/100%\)\s*')
Line 216: 
Line 217:     def __init__(self, cmd, cwd=None):
Line 218:         self._progress = 0.0


Line 220:         self._stdout = bytearray()
Line 221:         self._stderr = bytearray()
Line 222: 
Line 223:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 224:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
>  This line may need now cmdutils.taskset if vdsm is using
> cpu_affinity. Plea 
Done
Line 225: 
Line 226:         self._term_lock = threading.Lock()
Line 227: 
Line 228:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))


Line 222: 
Line 223:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 224:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
Line 225: 
Line 226:         self._term_lock = threading.Lock()
>  Didn't we agree that we don't need this lock? 
Done
Line 227: 
Line 228:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))
Line 229:         self._command = CPopen(cmd, cwd=cwd, deathSignal=signal.SIGKILL)
Line 230:         self._stream = utils.CommandStream(


Line 263:         return self._progress
Line 264: 
Line 265:     @property
Line 266:     def stderr(self):
Line 267:         return str(self._stderr)
>  lets rename to "error" 
Done
Line 268: 
Line 269:     @property
Line 270:     def finished(self):
Line 271:         return self._command.returncode is not None


Line 267:         return str(self._stderr)
Line 268: 
Line 269:     @property
Line 270:     def finished(self):
Line 271:         return self._command.returncode is not None
>  This should be self._command.poll() - returncode is not updated until
> you p 
Done
Line 272: 
Line 273:     def wait(self, timeout=None):
Line 274:         self._stream.receive(timeout=timeout)
Line 275: 


Line 277:             return
Line 278: 
Line 279:         with self._term_lock:
Line 280:             self._command.wait()
Line 281: 
>  If the command was aborted, we should raise Aborted error instead, to
> preve 
Done
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 284: 
Line 285:     def terminate(self):


Line 279:         with self._term_lock:
Line 280:             self._command.wait()
Line 281: 
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
>  log here the command termination using cmdutils.retcode_log_line -
> not havi 
Done
Line 284: 
Line 285:     def terminate(self):
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:


Line 281: 
Line 282:         if self._command.returncode != 0:
Line 283:             raise QImgError(self._command.returncode, "",
self.stderr)
Line 284: 
Line 285:     def terminate(self):
>  Lets rename this to abort(), matching new Job interface. 
Done
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:
Line 288:                 self._command.terminate()
Line 289: 


Line 284: 
Line 285:     def terminate(self):
Line 286:         with self._term_lock:
Line 287:             if not self._stream.closed:
Line 288:                 self._command.terminate()
>  This should do: 
Done
Line 289: 
Line 290: 
Line 291: def resize(image, newSize, format=None):
Line 292:     cmd = [_qemuimg.cmd, "resize"]


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(6 comments)

https://gerrit.ovirt.org/#/c/33910/7/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 241:             idx = self._stdout.rindex('\r')
Line 242:         except ValueError:
Line 243:             return
Line 244: 
Line 245:         last_progress = self._stdout[:idx].rsplit('\r', 1)[-1]
>  This line is a little bit too complex, would be nicer as two simpler
> steps, 
Done
Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
Line 249:         if m is None:


Line 244: 
Line 245:         last_progress = self._stdout[:idx].rsplit('\r', 1)[-1]
Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
>  Why not add leading and trailing whitespace into the regex? This will
> perfo 
Done
Line 249:         if m is None:
Line 250:             raise ValueError('Unable to parse: "%s"' %
last_progress)
Line 251: 
Line 252:         self._progress = float(m.group(1))


Line 246:         del self._stdout[:idx + 1]
Line 247: 
Line 248:         m = self.REGEXPR.match(last_progress.strip())
Line 249:         if m is None:
Line 250:             raise ValueError('Unable to parse: "%s"' %
last_progress)
>  %r is better here 
Done
Line 251: 
Line 252:         self._progress = float(m.group(1))
Line 253: 
Line 254:     @property


https://gerrit.ovirt.org/#/c/33910/7/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 368:     PROGRESS_FORMAT = "    (%.2f/100%%)\r"
Line 369: 
Line 370:     @staticmethod
Line 371:     def _progress_iterator():
Line 372:         return map(lambda x: x / 100.0, xrange(0, 10000, 1))
>  Why not use a generator? 
Done
Line 373: 
Line 374:     def test_failure(self):
Line 375:         p = qemuimg.QemuImgProcess(['false'])
Line 376:         self.assertRaises(qemuimg.QImgError, p.wait)


Line 378:     def test_progress_simple(self):
Line 379:         p = qemuimg.QemuImgProcess(['true'])
Line 380: 
Line 381:         for progress in self._progress_iterator():
Line 382:             p._recvstdout(self.PROGRESS_FORMAT % progress)
>  This works but it assumes too much internal knowledge. I think we
> would lik 
I think you are making this too complicated. I really don't think
it's a problem that we use the internal callback to send fake data.
Line 383:             self.assertEquals(p.progress, progress)
Line 384: 
Line 385:         p.wait()
Line 386:         self.assertEquals(p.finished, True)


Line 384: 
Line 385:         p.wait()
Line 386:         self.assertEquals(p.finished, True)
Line 387: 
Line 388:     def test_progress_incomplete(self):
>  This test would be more clear if we limit the test to only few
> iterations,  
Done
Line 389:         p = qemuimg.QemuImgProcess(['true'])
Line 390: 
Line 391:         old_progress = 0.0
Line 392: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 7:

(1 comment)

https://gerrit.ovirt.org/#/c/33910/7/tests/qemuimgTests.py
File tests/qemuimgTests.py:

Line 378:     def test_progress_simple(self):
Line 379:         p = qemuimg.QemuImgProcess(['true'])
Line 380: 
Line 381:         for progress in self._progress_iterator():
Line 382:             p._recvstdout(self.PROGRESS_FORMAT % progress)
>  I think you are making this too complicated. I really don't think
> it's a pr 
OK
Line 383:             self.assertEquals(p.progress, progress)
Line 384: 
Line 385:         p.wait()
Line 386:         self.assertEquals(p.finished, True)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 10: Code-Review-1

(3 comments)

Also see my comments in version 9 in image.py

https://gerrit.ovirt.org/#/c/33910/10/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 219: 
Line 220:         self._stdout = bytearray()
Line 221:         self._stderr = bytearray()
Line 222: 
Line 223:         cmd = cmdutils.taskset(cmd, utils._ANY_CPU)
- taskset should be last for consistency with utils.execCmd, where we keep it last to make
sure we run sudo with taskset and not tasket with sudo (which is not allowed by our sudo
rules)
- We should use taskset only if vdsm is using the cpu_affinity configuration, same logic
as in utils.execCmd. We need to extract this logic somewhere so we do not have to repeat
it.
Line 224:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 225:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
Line 226: 
Line 227:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))


Line 277: 
Line 278:         self._command.wait()
Line 279: 
Line 280:         if self._aborted:
Line 281:             raise utils.ActionStopped()
Why not OpefrationStopped or OperationAborted?
Line 282: 
Line 283:         cmdutils.retcode_log_line(self._command.returncode, self.error)
Line 284:         if self._command.returncode != 0:
Line 285:             raise QImgError(self._command.returncode, "", self.error)


https://gerrit.ovirt.org/#/c/33910/10/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.abort()
Why not use command.abort directly?

Please see my comments in this module from previous version.
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):
Line 122:             while not command.finished:
Line 123:                 command.wait(self._QEMU_LOGGING_INTERVAL)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 10:

(3 comments)

https://gerrit.ovirt.org/#/c/33910/10/lib/vdsm/qemuimg.py
File lib/vdsm/qemuimg.py:

Line 219: 
Line 220:         self._stdout = bytearray()
Line 221:         self._stderr = bytearray()
Line 222: 
Line 223:         cmd = cmdutils.taskset(cmd, utils._ANY_CPU)
>  - taskset should be last for consistency with utils.execCmd, where we
> keep  
ok.  Will do.
Line 224:         cmd = cmdutils.ionice(cmd, utils.IOCLASS.IDLE)
Line 225:         cmd = cmdutils.nice(cmd, utils.NICENESS.HIGH)
Line 226: 
Line 227:         _log.debug(cmdutils.command_log_line(cmd, cwd=cwd))


Line 277: 
Line 278:         self._command.wait()
Line 279: 
Line 280:         if self._aborted:
Line 281:             raise utils.ActionStopped()
>  Why not OpefrationStopped or OperationAborted? 
Because we
already have a suitable exception defined.  Seems unnecessary to add a new one just so the
names match.
Line 282: 
Line 283:         cmdutils.retcode_log_line(self._command.returncode, self.error)
Line 284:         if self._command.returncode != 0:
Line 285:             raise QImgError(self._command.returncode, "", self.error)


https://gerrit.ovirt.org/#/c/33910/10/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.abort()
>  Why not use command.abort directly? 
ok.
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):
Line 122:             while not command.finished:
Line 123:                 command.wait(self._QEMU_LOGGING_INTERVAL)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 9:

(4 comments)

https://gerrit.ovirt.org/#/c/33910/9/vdsm/storage/image.py
File vdsm/storage/image.py:

Line 110: 
Line 111:     def __init__(self, repoPath):
Line 112:         self.repoPath = repoPath
Line 113: 
Line 114:     def _qemuTaskProgressWait(self, command):
>  Lets rename this to _wait_for_qemuimg_operation(self, op) 
Done
Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')


Line 113: 
Line 114:     def _qemuTaskProgressWait(self, command):
Line 115:         self.log.debug('waiting for qemu-img operation to complete')
Line 116: 
Line 117:         def abortImgConversion():
>  We don't need this inline function, we can use command.abort
> directly. 
Done
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.terminate()
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):


Line 117:         def abortImgConversion():
Line 118:             self.log.info('aborting ongoing qemu-img operation')
Line 119:             command.terminate()
Line 120: 
Line 121:         with vars.task.abort_callback(abortImgConversion):
>  Use  
Done
Line 122:             while not command.finished:
Line 123:                 command.wait(self._QEMU_LOGGING_INTERVAL)
Line 124:                 self.log.debug('qemu-img operation progress: %s%%',
Line 125:                                command.progress)


Line 458:                     else:
Line 459:                         backing = None
Line 460:                         backingFormat = None
Line 461: 
Line 462:                     qemucmd = qemuimg.convert(
>  Lets call this "operation" or "op" 
Done
Line 463:                         srcVol.getVolumePath(),
Line 464:                         dstVol.getVolumePath(),
Line 465:                         srcFormat=srcFormat,
Line 466:                         dstFormat=dstFormat,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 11: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 11: Code-Review+1

(2 comments)

Just minor commit message cleanup.

https://gerrit.ovirt.org/#/c/33910/11//COMMIT_MSG
Commit Message:

Line 11: order to parse and maintain the progress of the process.
Line 12: 
Line 13:  operation = qemuimg.convert(srcImage, dstImage)
Line 14: 
Line 15:  while not command.operation:
commmand.operation -> operation.finished or convert.finished  (since you are using
qemuimg.convert in the example)
Line 16:      operation.wait(10)
Line 17:      log.debug('command progress: %s%%', operation.progress)
Line 18: 
Line 19: Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299


Line 13:  operation = qemuimg.convert(srcImage, dstImage)
Line 14: 
Line 15:  while not command.operation:
Line 16:      operation.wait(10)
Line 17:      log.debug('command progress: %s%%', operation.progress)
command -> operation, or convert
Line 18: 
Line 19: Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: Yes

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 12:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 12: Code-Review-1

Adam, please check my comments from version 11.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 13:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 14:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 14:

Ahh, love the spurious CI failures always happening at the perfect time.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 14: Code-Review+2

Adam, is this rebased on current master? That error in the ci should be fixed now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 15:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and
is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5',
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

Adam Litke has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 15: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 15: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: qemuimg: add support for convert progress
......................................................................


qemuimg: add support for convert progress

This patch adds the support for QemuImgProcess, a class designed
to oversee the execution of a long qemu-img convert operation in
order to parse and maintain the progress of the process.

 operation = qemuimg.convert(srcImage, dstImage)

 while not operation.finished:
     operation.wait(10)
     log.debug('convert progress: %s%%', operation.progress)

Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Signed-off-by: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Reviewed-on: https://gerrit.ovirt.org/33910
Tested-by: Adam Litke <alitke(a)redhat.com&gt;
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer <nsoffer(a)redhat.com&gt;
---
M lib/vdsm/qemuimg.py
M tests/qemuimgTests.py
M vdsm/storage/image.py
3 files changed, 205 insertions(+), 57 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Adam Litke: Verified
  Jenkins CI: Passed CI tests



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org

automation(a)ovirt.org has posted comments on this change.

Change subject: qemuimg: add support for convert progress
......................................................................


Patch Set 16:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id0b53e418c62bb2e91444ba5f351c916ca417299
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Adam Litke <alitke(a)redhat.com&gt;
Gerrit-Reviewer: Ala Hino <ahino(a)redhat.com&gt;
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com&gt;
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com&gt;
Gerrit-Reviewer: Francesco Romani <fromani(a)redhat.com&gt;
Gerrit-Reviewer: Freddy Rolland <frolland(a)redhat.com&gt;
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com&gt;
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-HasComments: No