Adam Litke has uploaded a new change for review.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
qemuimg: Require explicit start of QemuImgOperation
Currently the underlying qemuimg command is started by QemuImgOperation.__init__. In general this is a bad practice because it eliminates control and can make testing more difficult. Add a start() method to this class and require that it be called to actually launch the underlying command.
This will be used by a subsequent patch which creates the operation under one lock and starts it under a different lock.
Change-Id: I8e427c909a8dffc3ba2a5c838c7d1e7ce7cce55d Signed-off-by: Adam Litke alitke@redhat.com --- M lib/vdsm/qemuimg.py M tests/qemuimg_test.py M tests/storage_sdm_copy_data_test.py M vdsm/storage/image.py M vdsm/storage/sdm/api/copy_data.py 5 files changed, 30 insertions(+), 13 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/48/65148/1
diff --git a/lib/vdsm/qemuimg.py b/lib/vdsm/qemuimg.py index 8eef8fa..486882b 100644 --- a/lib/vdsm/qemuimg.py +++ b/lib/vdsm/qemuimg.py @@ -192,18 +192,24 @@ REGEXPR = re.compile(r'\s*(([\d.]+)/100%)\s*')
def __init__(self, cmd, cwd=None): + self.base_cmd = cmd + self.cwd = cwd self._aborted = False self._progress = 0.0
self._stdout = bytearray() self._stderr = bytearray() + self._proc = None + self._stream = None
- cmd = cmdutils.wrap_command(cmd, with_nice=utils.NICENESS.HIGH, + def start(self): + cmd = cmdutils.wrap_command(self.base_cmd, + with_nice=utils.NICENESS.HIGH, with_ioclass=utils.IOCLASS.IDLE) - _log.debug(cmdutils.command_log_line(cmd, cwd=cwd)) - self._command = CPopen(cmd, cwd=cwd, deathSignal=signal.SIGKILL) - self._stream = utils.CommandStream( - self._command, self._recvstdout, self._recvstderr) + _log.debug(cmdutils.command_log_line(cmd, cwd=self.cwd)) + self._proc = CPopen(cmd, cwd=self.cwd, deathSignal=signal.SIGKILL) + self._stream = utils.CommandStream(self._proc, + self._recvstdout, self._recvstderr)
def _recvstderr(self, buffer): self._stderr += buffer @@ -243,7 +249,7 @@
@property def finished(self): - return self._command.poll() is not None + return self._proc.poll() is not None
def poll(self, timeout=None): self._stream.receive(timeout=timeout) @@ -251,14 +257,14 @@ if not self._stream.closed: return
- self._command.wait() + self._proc.wait()
if self._aborted: raise exception.ActionStopped()
- cmdutils.retcode_log_line(self._command.returncode, self.error) - if self._command.returncode != 0: - raise QImgError(self._command.returncode, "", self.error) + cmdutils.retcode_log_line(self._proc.returncode, self.error) + if self._proc.returncode != 0: + raise QImgError(self._proc.returncode, "", self.error)
def wait_for_completion(self): timeout = config.getint("irs", "progress_interval") @@ -267,9 +273,9 @@ _log.debug('qemu-img operation progress: %s%%', self.progress)
def abort(self): - if self._command.poll() is None: + if self._proc.poll() is None: self._aborted = True - self._command.terminate() + self._proc.terminate()
def resize(image, newSize, format=None): diff --git a/tests/qemuimg_test.py b/tests/qemuimg_test.py index 0642932..5afda1d 100644 --- a/tests/qemuimg_test.py +++ b/tests/qemuimg_test.py @@ -313,10 +313,12 @@
def test_failure(self): p = qemuimg.QemuImgOperation(['false']) + p.start() self.assertRaises(qemuimg.QImgError, p.poll)
def test_progress_simple(self): p = qemuimg.QemuImgOperation(['true']) + p.start()
for progress in self._progress_iterator(): p._recvstdout(self.PROGRESS_FORMAT % progress) @@ -332,6 +334,7 @@ ]) def test_partial(self, output_list, progress_list): p = qemuimg.QemuImgOperation(['true']) + p.start()
for output, progress in zip(output_list, progress_list): p._recvstdout(output) @@ -341,6 +344,7 @@
def test_progress_batch(self): p = qemuimg.QemuImgOperation(['true']) + p.start()
p._recvstdout( (self.PROGRESS_FORMAT % 10.00) + @@ -354,6 +358,7 @@
def test_unexpected_output(self): p = qemuimg.QemuImgOperation(['true']) + p.start()
self.assertRaises(ValueError, p._recvstdout, 'Hello World\r')
diff --git a/tests/storage_sdm_copy_data_test.py b/tests/storage_sdm_copy_data_test.py index d60936a..948d170 100644 --- a/tests/storage_sdm_copy_data_test.py +++ b/tests/storage_sdm_copy_data_test.py @@ -330,11 +330,13 @@ self.error = error self.abort_event = threading.Event()
+ def start(self): + self.ready_event.set() + def abort(self): self.abort_event.set()
def wait_for_completion(self): - self.ready_event.set() if self.error: raise self.error() if self.wait_for_abort: diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py index e98ca9b..812a03a 100644 --- a/vdsm/storage/image.py +++ b/vdsm/storage/image.py @@ -478,6 +478,7 @@ dstQcow2Compat=destDom.qcow2_compat(), backing=backing, backingFormat=backingFormat) + operation.start() with utils.stopwatch("Copy volume %s" % srcVol.volUUID): self._wait_for_qemuimg_operation(operation) except ActionStopped: @@ -865,6 +866,7 @@ srcFormat=sc.fmt2str(volParams['volFormat']), dstFormat=sc.fmt2str(dstVolFormat), dstQcow2Compat=destDom.qcow2_compat()) + operation.start() with utils.stopwatch("Copy volume %s" % srcVol.volUUID): self._wait_for_qemuimg_operation(operation) except ActionStopped: @@ -1110,6 +1112,7 @@ srcFormat=sc.fmt2str(srcVolParams['volFormat']), dstFormat=sc.fmt2str(volParams['volFormat']), qcow2Compat=sdDom.qcow2_compat()) + operation.start() with utils.stopwatch("Copy volume %s" % srcVol.volUUID): self._wait_for_qemuimg_operation(operation) except qemuimg.QImgError: diff --git a/vdsm/storage/sdm/api/copy_data.py b/vdsm/storage/sdm/api/copy_data.py index 3bfff4e..3dc6fff 100644 --- a/vdsm/storage/sdm/api/copy_data.py +++ b/vdsm/storage/sdm/api/copy_data.py @@ -82,6 +82,7 @@ dstFormat=dst_format, backing=self._dest.backing_path, backingFormat=self._dest.backing_qemu_format) + self._operation.start() self._operation.wait_for_completion()