From 461044ceb4fe58eb919dedfeb62f619f79d4d552 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 19 Oct 2021 10:49:17 -0400 Subject: python: Add iotest linters to test suite Run mypy and pylint on the iotests files directly from the Python CI test infrastructure. This ensures that any accidental breakages to the qemu.[qmp|aqmp|machine|utils] packages will be caught by that test suite. It also ensures that these linters are run with well-known versions and test against a wide variety of python versions, which helps to find accidental cross-version python compatibility issues. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Message-id: 20211019144918.3159078-15-jsnow@redhat.com Signed-off-by: John Snow --- python/tests/iotests-mypy.sh | 4 ++++ python/tests/iotests-pylint.sh | 4 ++++ 2 files changed, 8 insertions(+) create mode 100755 python/tests/iotests-mypy.sh create mode 100755 python/tests/iotests-pylint.sh (limited to 'python') diff --git a/python/tests/iotests-mypy.sh b/python/tests/iotests-mypy.sh new file mode 100755 index 0000000000..ee76470819 --- /dev/null +++ b/python/tests/iotests-mypy.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --mypy diff --git a/python/tests/iotests-pylint.sh b/python/tests/iotests-pylint.sh new file mode 100755 index 0000000000..4cae03424b --- /dev/null +++ b/python/tests/iotests-pylint.sh @@ -0,0 +1,4 @@ +#!/bin/sh -e + +cd ../tests/qemu-iotests/ +python3 -m linters --pylint -- cgit 1.4.1 From b9420e4f4b899c96221fa5a1f7f025214e756c50 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:05 -0400 Subject: python/machine: remove has_quit argument If we spy on the QMP commands instead, we don't need callers to remember to pass it. Seems like a fair trade-off. The one slightly weird bit is overloading this instance variable for wait(), where we use it to mean "don't issue the qmp 'quit' command". This means that wait() will "fail" if the QEMU process does not terminate of its own accord. In most cases, we probably did already actually issue quit -- some iotests do this -- but in some others, we may be waiting for QEMU to terminate for some other reason, such as a test wherein we tell the guest (directly) to shut down. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-2-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 34 +++++++++++++++++++--------------- tests/qemu-iotests/040 | 7 +------ tests/qemu-iotests/218 | 2 +- tests/qemu-iotests/255 | 2 +- 4 files changed, 22 insertions(+), 23 deletions(-) (limited to 'python') diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 056d340e35..0bd40bc2f7 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -170,6 +170,7 @@ class QEMUMachine: self._console_socket: Optional[socket.socket] = None self._remove_files: List[str] = [] self._user_killed = False + self._quit_issued = False def __enter__(self: _T) -> _T: return self @@ -368,6 +369,7 @@ class QEMUMachine: command = '' LOG.warning(msg, -int(exitcode), command) + self._quit_issued = False self._user_killed = False self._launched = False @@ -443,15 +445,13 @@ class QEMUMachine: self._subp.kill() self._subp.wait(timeout=60) - def _soft_shutdown(self, timeout: Optional[int], - has_quit: bool = False) -> None: + def _soft_shutdown(self, timeout: Optional[int]) -> None: """ Perform early cleanup, attempt to gracefully shut down the VM, and wait for it to terminate. :param timeout: Timeout in seconds for graceful shutdown. A value of None is an infinite wait. - :param has_quit: When True, don't attempt to issue 'quit' QMP command :raise ConnectionReset: On QMP communication errors :raise subprocess.TimeoutExpired: When timeout is exceeded waiting for @@ -460,21 +460,19 @@ class QEMUMachine: self._early_cleanup() if self._qmp_connection: - if not has_quit: + if not self._quit_issued: # Might raise ConnectionReset - self._qmp.cmd('quit') + self.qmp('quit') # May raise subprocess.TimeoutExpired self._subp.wait(timeout=timeout) - def _do_shutdown(self, timeout: Optional[int], - has_quit: bool = False) -> None: + def _do_shutdown(self, timeout: Optional[int]) -> None: """ Attempt to shutdown the VM gracefully; fallback to a hard shutdown. :param timeout: Timeout in seconds for graceful shutdown. A value of None is an infinite wait. - :param has_quit: When True, don't attempt to issue 'quit' QMP command :raise AbnormalShutdown: When the VM could not be shut down gracefully. The inner exception will likely be ConnectionReset or @@ -482,13 +480,13 @@ class QEMUMachine: may result in its own exceptions, likely subprocess.TimeoutExpired. """ try: - self._soft_shutdown(timeout, has_quit) + self._soft_shutdown(timeout) except Exception as exc: self._hard_shutdown() raise AbnormalShutdown("Could not perform graceful shutdown") \ from exc - def shutdown(self, has_quit: bool = False, + def shutdown(self, hard: bool = False, timeout: Optional[int] = 30) -> None: """ @@ -498,7 +496,6 @@ class QEMUMachine: If the VM has not yet been launched, or shutdown(), wait(), or kill() have already been called, this method does nothing. - :param has_quit: When true, do not attempt to issue 'quit' QMP command. :param hard: When true, do not attempt graceful shutdown, and suppress the SIGKILL warning log message. :param timeout: Optional timeout in seconds for graceful shutdown. @@ -512,7 +509,7 @@ class QEMUMachine: self._user_killed = True self._hard_shutdown() else: - self._do_shutdown(timeout, has_quit) + self._do_shutdown(timeout) finally: self._post_shutdown() @@ -529,7 +526,8 @@ class QEMUMachine: :param timeout: Optional timeout in seconds. Default 30 seconds. A value of `None` is an infinite wait. """ - self.shutdown(has_quit=True, timeout=timeout) + self._quit_issued = True + self.shutdown(timeout=timeout) def set_qmp_monitor(self, enabled: bool = True) -> None: """ @@ -574,7 +572,10 @@ class QEMUMachine: conv_keys = True qmp_args = self._qmp_args(conv_keys, args) - return self._qmp.cmd(cmd, args=qmp_args) + ret = self._qmp.cmd(cmd, args=qmp_args) + if cmd == 'quit' and 'error' not in ret and 'return' in ret: + self._quit_issued = True + return ret def command(self, cmd: str, conv_keys: bool = True, @@ -585,7 +586,10 @@ class QEMUMachine: On failure raise an exception. """ qmp_args = self._qmp_args(conv_keys, args) - return self._qmp.command(cmd, **qmp_args) + ret = self._qmp.command(cmd, **qmp_args) + if cmd == 'quit': + self._quit_issued = True + return ret def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]: """ diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040 index f3677de9df..6af5ab9e76 100755 --- a/tests/qemu-iotests/040 +++ b/tests/qemu-iotests/040 @@ -92,10 +92,9 @@ class TestSingleDrive(ImageCommitTestCase): self.vm.add_device('virtio-scsi') self.vm.add_device("scsi-hd,id=scsi0,drive=drive0") self.vm.launch() - self.has_quit = False def tearDown(self): - self.vm.shutdown(has_quit=self.has_quit) + self.vm.shutdown() os.remove(test_img) os.remove(mid_img) os.remove(backing_img) @@ -127,8 +126,6 @@ class TestSingleDrive(ImageCommitTestCase): result = self.vm.qmp('quit') self.assert_qmp(result, 'return', {}) - self.has_quit = True - # Same as above, but this time we add the filter after starting the job @iotests.skip_if_unsupported(['throttle']) def test_commit_plus_filter_and_quit(self): @@ -147,8 +144,6 @@ class TestSingleDrive(ImageCommitTestCase): result = self.vm.qmp('quit') self.assert_qmp(result, 'return', {}) - self.has_quit = True - def test_device_not_found(self): result = self.vm.qmp('block-commit', device='nonexistent', top='%s' % mid_img) self.assert_qmp(result, 'error/class', 'DeviceNotFound') diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218 index 325d8244fb..4922b4d3b6 100755 --- a/tests/qemu-iotests/218 +++ b/tests/qemu-iotests/218 @@ -187,4 +187,4 @@ with iotests.VM() as vm, \ log(vm.qmp('quit')) with iotests.Timeout(5, 'Timeout waiting for VM to quit'): - vm.shutdown(has_quit=True) + vm.shutdown() diff --git a/tests/qemu-iotests/255 b/tests/qemu-iotests/255 index c43aa9c67a..3d6d0e80cb 100755 --- a/tests/qemu-iotests/255 +++ b/tests/qemu-iotests/255 @@ -123,4 +123,4 @@ with iotests.FilePath('src.qcow2') as src_path, \ vm.qmp_log('block-job-cancel', device='job0') vm.qmp_log('quit') - vm.shutdown(has_quit=True) + vm.shutdown() -- cgit 1.4.1 From 49a608b8c22db72d47e7676ff9c3b87d1d408736 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:06 -0400 Subject: python/machine: Handle QMP errors on close more meticulously To use the AQMP backend, Machine just needs to be a little more diligent about what happens when closing a QMP connection. The operation is no longer a freebie in the async world; it may return errors encountered in the async bottom half on incoming message receipt, etc. (AQMP's disconnect, ultimately, serves as the quiescence point where all async contexts are gathered together, and any final errors reported at that point.) Because async QMP continues to check for messages asynchronously, it's almost certainly likely that the loop will have exited due to EOF after issuing the last 'quit' command. That error will ultimately be bubbled up when attempting to close the QMP connection. The manager class here then is free to discard it -- if it was expected. Signed-off-by: John Snow Reviewed-by: Hanna Reitz Reviewed-by: Kevin Wolf Message-id: 20211026175612.4127598-3-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 48 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-) (limited to 'python') diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index 0bd40bc2f7..a0cf69786b 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -342,9 +342,15 @@ class QEMUMachine: # Comprehensive reset for the failed launch case: self._early_cleanup() - if self._qmp_connection: - self._qmp.close() - self._qmp_connection = None + try: + self._close_qmp_connection() + except Exception as err: # pylint: disable=broad-except + LOG.warning( + "Exception closing QMP connection: %s", + str(err) if str(err) else type(err).__name__ + ) + finally: + assert self._qmp_connection is None self._close_qemu_log_file() @@ -420,6 +426,31 @@ class QEMUMachine: close_fds=False) self._post_launch() + def _close_qmp_connection(self) -> None: + """ + Close the underlying QMP connection, if any. + + Dutifully report errors that occurred while closing, but assume + that any error encountered indicates an abnormal termination + process and not a failure to close. + """ + if self._qmp_connection is None: + return + + try: + self._qmp.close() + except EOFError: + # EOF can occur as an Exception here when using the Async + # QMP backend. It indicates that the server closed the + # stream. If we successfully issued 'quit' at any point, + # then this was expected. If the remote went away without + # our permission, it's worth reporting that as an abnormal + # shutdown case. + if not (self._user_killed or self._quit_issued): + raise + finally: + self._qmp_connection = None + def _early_cleanup(self) -> None: """ Perform any cleanup that needs to happen before the VM exits. @@ -460,9 +491,14 @@ class QEMUMachine: self._early_cleanup() if self._qmp_connection: - if not self._quit_issued: - # Might raise ConnectionReset - self.qmp('quit') + try: + if not self._quit_issued: + # May raise ExecInterruptedError or StateError if the + # connection dies or has *already* died. + self.qmp('quit') + finally: + # Regardless, we want to quiesce the connection. + self._close_qmp_connection() # May raise subprocess.TimeoutExpired self._subp.wait(timeout=timeout) -- cgit 1.4.1 From 0f71c9a93693de54ca80a47c3996d64cd65f72b7 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:07 -0400 Subject: python/aqmp: Remove scary message The scary message interferes with the iotests output. Coincidentally, if iotests works by removing this, then it's good evidence that we don't really need to scare people away from using it. Signed-off-by: John Snow Acked-by: Hanna Reitz Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-4-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/__init__.py | 12 ------------ 1 file changed, 12 deletions(-) (limited to 'python') diff --git a/python/qemu/aqmp/__init__.py b/python/qemu/aqmp/__init__.py index d1b0e4dc3d..880d5b6fa7 100644 --- a/python/qemu/aqmp/__init__.py +++ b/python/qemu/aqmp/__init__.py @@ -22,7 +22,6 @@ managing QMP events. # the COPYING file in the top-level directory. import logging -import warnings from .error import AQMPError from .events import EventListener @@ -31,17 +30,6 @@ from .protocol import ConnectError, Runstate, StateError from .qmp_client import ExecInterruptedError, ExecuteError, QMPClient -_WMSG = """ - -The Asynchronous QMP library is currently in development and its API -should be considered highly fluid and subject to change. It should -not be used by any other scripts checked into the QEMU tree. - -Proceed with caution! -""" - -warnings.warn(_WMSG, FutureWarning) - # Suppress logging unless an application engages it. logging.getLogger('qemu.aqmp').addHandler(logging.NullHandler()) -- cgit 1.4.1 From f122be6093f5fc61fe5784e342781fab07f5defc Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:11 -0400 Subject: python/aqmp: Create sync QMP wrapper for iotests This is a wrapper around the async QMPClient that mimics the old, synchronous QEMUMonitorProtocol class. It is designed to be interchangeable with the old implementation. It does not, however, attempt to mimic Exception compatibility. Signed-off-by: John Snow Acked-by: Hanna Reitz Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-8-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/aqmp/legacy.py | 138 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 138 insertions(+) create mode 100644 python/qemu/aqmp/legacy.py (limited to 'python') diff --git a/python/qemu/aqmp/legacy.py b/python/qemu/aqmp/legacy.py new file mode 100644 index 0000000000..9e7b9fb80b --- /dev/null +++ b/python/qemu/aqmp/legacy.py @@ -0,0 +1,138 @@ +""" +Sync QMP Wrapper + +This class pretends to be qemu.qmp.QEMUMonitorProtocol. +""" + +import asyncio +from typing import ( + Awaitable, + List, + Optional, + TypeVar, + Union, +) + +import qemu.qmp +from qemu.qmp import QMPMessage, QMPReturnValue, SocketAddrT + +from .qmp_client import QMPClient + + +# pylint: disable=missing-docstring + + +class QEMUMonitorProtocol(qemu.qmp.QEMUMonitorProtocol): + def __init__(self, address: SocketAddrT, + server: bool = False, + nickname: Optional[str] = None): + + # pylint: disable=super-init-not-called + self._aqmp = QMPClient(nickname) + self._aloop = asyncio.get_event_loop() + self._address = address + self._timeout: Optional[float] = None + + _T = TypeVar('_T') + + def _sync( + self, future: Awaitable[_T], timeout: Optional[float] = None + ) -> _T: + return self._aloop.run_until_complete( + asyncio.wait_for(future, timeout=timeout) + ) + + def _get_greeting(self) -> Optional[QMPMessage]: + if self._aqmp.greeting is not None: + # pylint: disable=protected-access + return self._aqmp.greeting._asdict() + return None + + # __enter__ and __exit__ need no changes + # parse_address needs no changes + + def connect(self, negotiate: bool = True) -> Optional[QMPMessage]: + self._aqmp.await_greeting = negotiate + self._aqmp.negotiate = negotiate + + self._sync( + self._aqmp.connect(self._address) + ) + return self._get_greeting() + + def accept(self, timeout: Optional[float] = 15.0) -> QMPMessage: + self._aqmp.await_greeting = True + self._aqmp.negotiate = True + + self._sync( + self._aqmp.accept(self._address), + timeout + ) + + ret = self._get_greeting() + assert ret is not None + return ret + + def cmd_obj(self, qmp_cmd: QMPMessage) -> QMPMessage: + return dict( + self._sync( + # pylint: disable=protected-access + + # _raw() isn't a public API, because turning off + # automatic ID assignment is discouraged. For + # compatibility with iotests *only*, do it anyway. + self._aqmp._raw(qmp_cmd, assign_id=False), + self._timeout + ) + ) + + # Default impl of cmd() delegates to cmd_obj + + def command(self, cmd: str, **kwds: object) -> QMPReturnValue: + return self._sync( + self._aqmp.execute(cmd, kwds), + self._timeout + ) + + def pull_event(self, + wait: Union[bool, float] = False) -> Optional[QMPMessage]: + if not wait: + # wait is False/0: "do not wait, do not except." + if self._aqmp.events.empty(): + return None + + # If wait is 'True', wait forever. If wait is False/0, the events + # queue must not be empty; but it still needs some real amount + # of time to complete. + timeout = None + if wait and isinstance(wait, float): + timeout = wait + + return dict( + self._sync( + self._aqmp.events.get(), + timeout + ) + ) + + def get_events(self, wait: Union[bool, float] = False) -> List[QMPMessage]: + events = [dict(x) for x in self._aqmp.events.clear()] + if events: + return events + + event = self.pull_event(wait) + return [event] if event is not None else [] + + def clear_events(self) -> None: + self._aqmp.events.clear() + + def close(self) -> None: + self._sync( + self._aqmp.disconnect() + ) + + def settimeout(self, timeout: Optional[float]) -> None: + self._timeout = timeout + + def send_fd_scm(self, fd: int) -> None: + self._aqmp.send_fd_scm(fd) -- cgit 1.4.1 From 76cd358671e6b8e7c435ec65b1c44200254514a9 Mon Sep 17 00:00:00 2001 From: John Snow Date: Tue, 26 Oct 2021 13:56:12 -0400 Subject: python, iotests: replace qmp with aqmp Swap out the synchronous QEMUMonitorProtocol from qemu.qmp with the sync wrapper from qemu.aqmp instead. Add an escape hatch in the form of the environment variable QEMU_PYTHON_LEGACY_QMP which allows you to cajole QEMUMachine into using the old implementation, proving that both implementations work concurrently. Signed-off-by: John Snow Reviewed-by: Kevin Wolf Reviewed-by: Hanna Reitz Message-id: 20211026175612.4127598-9-jsnow@redhat.com Signed-off-by: John Snow --- python/qemu/machine/machine.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'python') diff --git a/python/qemu/machine/machine.py b/python/qemu/machine/machine.py index a0cf69786b..a487c39745 100644 --- a/python/qemu/machine/machine.py +++ b/python/qemu/machine/machine.py @@ -41,7 +41,6 @@ from typing import ( ) from qemu.qmp import ( # pylint: disable=import-error - QEMUMonitorProtocol, QMPMessage, QMPReturnValue, SocketAddrT, @@ -50,6 +49,12 @@ from qemu.qmp import ( # pylint: disable=import-error from . import console_socket +if os.environ.get('QEMU_PYTHON_LEGACY_QMP'): + from qemu.qmp import QEMUMonitorProtocol +else: + from qemu.aqmp.legacy import QEMUMonitorProtocol + + LOG = logging.getLogger(__name__) -- cgit 1.4.1