Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Reviewed-on: https://gerrit.ovirt.org/33910
Tested-by: Adam Litke
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer
---
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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, 1, 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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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, 1, 1))
Why not use a generator?
for value in xrange(0, 1, 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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Ala Hino
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Freddy Rolland
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Ala Hino Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Freddy Rolland Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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:
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
automat...@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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Federico Simoncelli
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Federico Simoncelli Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
Gerrit-Reviewer: Adam Litke
Gerrit-Reviewer: Dan Kenigsberg
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: Adam Litke Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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 Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: qemuimg: add support for convert progress
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
---
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, '
