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()
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
Patch Set 1:
* 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.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
Patch Set 2:
* 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.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
Patch Set 2: Code-Review-1
(3 comments)
I like this direction, but we can do this with minimal changes.
https://gerrit.ovirt.org/#/c/65148/2/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py:
Line 191: class QemuImgOperation(object): Line 192: REGEXPR = re.compile(r'\s*(([\d.]+)/100%)\s*') Line 193: Line 194: def __init__(self, cmd, cwd=None): Line 195: self.base_cmd = cmd I don't think we need to keep the original command, we can wrap and prepare it for running here, but start it in start(). Line 196: self.cwd = cwd Line 197: self._aborted = False Line 198: self._progress = 0.0 Line 199:
Line 256: Line 257: if not self._stream.closed: Line 258: return Line 259: Line 260: self._proc.wait() This reminds these patches, maybe you like to review them: - github: https://github.com/oVirt/vdsm/pull/8 - here: https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:pr... Line 261: Line 262: if self._aborted: Line 263: raise exception.ActionStopped() Line 264:
https://gerrit.ovirt.org/#/c/65148/2/vdsm/storage/image.py File vdsm/storage/image.py:
Line 1111: newVol.getVolumePath(), Line 1112: srcFormat=sc.fmt2str(srcVolParams['volFormat']), Line 1113: dstFormat=sc.fmt2str(volParams['volFormat']), Line 1114: qcow2Compat=sdDom.qcow2_compat()) Line 1115: operation.start() Maybe start the operation in _wait_for_qemuimg_operation?
We can also rename it to run_qemuimg_opeation Line 1116: with utils.stopwatch("Copy volume %s" % srcVol.volUUID): Line 1117: self._wait_for_qemuimg_operation(operation) Line 1118: except qemuimg.QImgError: Line 1119: self.log.exception('conversion failure for volume %s',
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
Patch Set 3:
* 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.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Require explicit start of QemuImgOperation ......................................................................
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.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Explicit run semantics for QemuImgOperation ......................................................................
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.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: qemuimg: Explicit run semantics for QemuImgOperation ......................................................................
Patch Set 5: Code-Review-1
(10 comments)
https://gerrit.ovirt.org/#/c/65148/5/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py:
Line 276 Line 277 Line 278 Line 279 Line 280 This will break now if called before the command was run.
Can be fixed by:
return self._command and self._command.poll() is not None
Line 278 Line 279 Line 280 Line 281 Line 282 We need to make this private, other code should not call this.
Line 300 Line 301 Line 302 Line 303 Line 304 Lets document that this is the only method that may be called from another thread.
Line 301 Line 302 Line 303 Line 304 Line 305 This will raise AttributeError if called before the command was run (e.g. abort called in the same time job is run.
Line 234: with_nice=utils.NICENESS.HIGH, Line 235: with_ioclass=utils.IOCLASS.IDLE) Line 236: self.cwd = cwd Line 237: self._command = None Line 238: self._stream = None When you create the operation, self._command is None... Line 239: Line 240: def _recvstderr(self, buffer): Line 241: self._stderr += buffer Line 242:
Line 303: def _start(self): Line 304: # Needed for tests. Should not be used directly. Line 305: _log.debug(cmdutils.command_log_line(self.cmd, cwd=self.cwd)) Line 306: self._command = CPopen(self.cmd, cwd=self.cwd, Line 307: deathSignal=signal.SIGKILL) Before we start the process we must make sure that we were no aborted, and we must make sure that we are not aborted after we check but before we start the command and assign to self._command.
This is very similar to what we do in copy_data.Job.
I guess we need:
def _start(self): with self._lock: if self._aborted: raise ActionStopped self._command = ... self._stream = ...
Now we switch from pending to running state without races.
And we must take the same lock in abort:
def abort(self): with self._lock: self._aborted = True if self.command and self.command.poll() is None: self._command.terminate()
terminate is async, invoke kill(2), and we are waiting in run(). Line 308: self._stream = utils.CommandStream( Line 309: self._command, self._recvstdout, self._recvstderr) Line 310: Line 311: def abort(self):
https://gerrit.ovirt.org/#/c/65148/5/tests/qemuimg_test.py File tests/qemuimg_test.py:
Line 313 Line 314 Line 315 Line 316 Line 317 Why not use run() for this test? then we can avoid calling private methods.
Line 318: self.assertRaises(qemuimg.QImgError, p.poll) Line 319: Line 320: def test_progress_simple(self): Line 321: p = qemuimg.QemuImgOperation(['true']) Line 322: p._start() This test is bad, we should realy test that output received from a process is handled properly instead using private methods. This test make it harder to change the code instead of helping.
But this is not related to your patch so lets improve this later. Line 323: Line 324: for progress in self._progress_iterator(): Line 325: p._recvstdout(self.PROGRESS_FORMAT % progress) Line 326: self.assertEquals(p.progress, progress)
Line 437: top = os.path.join(tmpdir, "top.img") Line 438: make_image(top, size, qemuimg.FORMAT.QCOW2, 1, "1.1", base) Line 439: Line 440: op = qemuimg.commit(top, topFormat=qemuimg.FORMAT.QCOW2) Line 441: op.run() Much nicer. Line 442: self.assertEquals(100, op.progress) Line 443: Line 444: Line 445: def make_image(path, size, format, index, qcow2_compat, backing=None):
https://gerrit.ovirt.org/#/c/65148/5/vdsm/storage/image.py File vdsm/storage/image.py:
Line 130 Line 131 Line 132 Line 133 Line 134 I think we should rename this to _run_qemuimg_operation. Can be done later if you want.
gerrit-hooks has posted comments on this change.
Change subject: qemuimg: Explicit run semantics for QemuImgOperation ......................................................................
Patch Set 6:
* Update Tracker::IGNORE, no bug url/s found * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::WARN, no public bug url found * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
vdsm-patches@lists.fedorahosted.org