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(a)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()
--
To view, visit
https://gerrit.ovirt.org/65148
To unsubscribe, visit
https://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e427c909a8dffc3ba2a5c838c7d1e7ce7cce55d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke(a)redhat.com>