On Wed, Jul 7, 2021 at 11:20 PM John Snow <js...@redhat.com> wrote:
>
>
> On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh...@gmail.com>
> wrote:
>
>> Added a draft of AQMP TUI.
>>
>> Implements the follwing basic features:
>> 1) Command transmission/reception.
>> 2) Shows events asynchronously.
>> 3) Shows server status in the bottom status bar.
>>
>> Also added necessary pylint, mypy configurations
>>
>> Signed-off-by: G S Niteesh Babu <niteesh...@gmail.com>
>> ---
>> python/qemu/aqmp/aqmp_tui.py | 246 +++++++++++++++++++++++++++++++++++
>> python/setup.cfg | 16 ++-
>> 2 files changed, 261 insertions(+), 1 deletion(-)
>> create mode 100644 python/qemu/aqmp/aqmp_tui.py
>>
>> diff --git a/python/qemu/aqmp/aqmp_tui.py b/python/qemu/aqmp/aqmp_tui.py
>> new file mode 100644
>> index 0000000000..8e9e8ac8ff
>> --- /dev/null
>> +++ b/python/qemu/aqmp/aqmp_tui.py
>> @@ -0,0 +1,246 @@
>> +# Copyright (c) 2021
>> +#
>> +# Authors:
>> +# Niteesh Babu G S <niteesh...@gmail.com>
>> +#
>> +# This work is licensed under the terms of the GNU GPL, version 2 or
>> +# later. See the COPYING file in the top-level directory.
>> +
>> +import argparse
>> +import asyncio
>> +import logging
>> +import signal
>> +
>> +import urwid
>> +import urwid_readline
>> +
>> +from .protocol import ConnectError
>> +from .qmp_protocol import QMP, ExecInterruptedError, ExecuteError
>> +from .util import create_task, pretty_traceback
>> +
>> +
>> +UPDATE_MSG = 'UPDATE_MSG'
>>
> +
>> +
>> +class StatusBar(urwid.Text):
>> + """
>> + A simple Text widget that currently only shows connection status.
>> + """
>> + def __init__(self, text=''):
>> + super().__init__(text, align='right')
>> +
>> +
>> +class Editor(urwid_readline.ReadlineEdit):
>> + """
>> + Support urwid_readline features along with
>> + history support which lacks in urwid_readline
>> + """
>> + def __init__(self, master):
>> + super().__init__(caption='> ', multiline=True)
>> + self.master = master
>> + self.history = []
>> + self.last_index = -1
>> + self.show_history = False
>> +
>> + def keypress(self, size, key):
>> + # TODO: Add some logic for down key and clean up logic if
>> possible.
>> + # Returning None means the key has been handled by this widget
>> + # which otherwise is propogated to the parent widget to be
>> + # handled
>> + msg = self.get_edit_text()
>> + if key == 'up' and not msg:
>> + # Show the history when 'up arrow' is pressed with no input
>> text.
>> + # NOTE: The show_history logic is necessary because in
>> 'multiline'
>> + # mode (which we use) 'up arrow' is used to move between
>> lines.
>> + self.show_history = True
>> + last_msg = self.history[self.last_index] if self.history
>> else ''
>> + self.set_edit_text(last_msg)
>> + self.edit_pos = len(last_msg)
>> + self.last_index += 1
>> + elif key == 'up' and self.show_history:
>> + if self.last_index < len(self.history):
>> + self.set_edit_text(self.history[self.last_index])
>> + self.edit_pos = len(self.history[self.last_index])
>> + self.last_index += 1
>> + elif key == 'meta enter':
>> + # When using multiline, enter inserts a new line into the
>> editor
>> + # send the input to the server on alt + enter
>> + self.master.cb_send_to_server(msg)
>> + self.history.insert(0, msg)
>> + self.set_edit_text('')
>> + self.last_index = 0
>> + self.show_history = False
>> + else:
>> + self.show_history = False
>> + self.last_index = 0
>> + return super().keypress(size, key)
>> + return None
>> +
>> +
>> +class EditorWidget(urwid.Filler):
>> + """
>> + Wraps CustomEdit
>> + """
>> + def __init__(self, master):
>> + super().__init__(Editor(master), valign='top')
>> +
>> +
>> +class HistoryBox(urwid.ListBox):
>> + """
>> + Shows all the QMP message transmitted/received
>> + """
>> + def __init__(self, master):
>> + self.master = master
>> + self.history = urwid.SimpleFocusListWalker([])
>> + super().__init__(self.history)
>> +
>> + def add_to_history(self, history):
>> + self.history.append(urwid.Text(history))
>> + if self.history:
>> + self.history.set_focus(len(self.history) - 1)
>> +
>> +
>> +class HistoryWindow(urwid.Frame):
>> + """
>> + Composes the HistoryBox and EditorWidget
>> + """
>> + def __init__(self, master):
>> + self.master = master
>> + self.editor = EditorWidget(master)
>> + self.editor_widget = urwid.LineBox(self.editor)
>> + self.history = HistoryBox(master)
>> + self.body = urwid.Pile([('weight', 80, self.history),
>> + ('weight', 10, self.editor_widget)])
>> + super().__init__(self.body)
>> + urwid.connect_signal(self.master, UPDATE_MSG,
>> self.cb_add_to_history)
>> +
>> + def cb_add_to_history(self, msg):
>> + self.history.add_to_history(msg)
>> +
>> +
>> +class Window(urwid.Frame):
>> + """
>> + This is going to be the main window that is going to compose other
>> + windows. In this stage it is unnecesssary but will be necessary in
>> + future when we will have multiple windows and want to the switch
>> between
>> + them and display overlays
>> + """
>> + def __init__(self, master):
>> + self.master = master
>> + footer = StatusBar()
>> + body = HistoryWindow(master)
>> + super().__init__(body, footer=footer)
>> +
>> +
>> +class App(QMP):
>> + def __init__(self, address):
>> + urwid.register_signal(self.__class__, UPDATE_MSG)
>>
>
> Do we really need a custom signal? It looks like Urwid has some "default"
> ones... are they not sufficient? I suppose the idea here is that the
> 'UPDATE_MSG' signal means that we've updated the history list, so we need
> to re-render.
>
> If not, you may use type(self) here which looks just a little cleaner.
>
>
>> + self.window = Window(self)
>> + self.address = address
>> + self.aloop = asyncio.get_event_loop()
>>
>
> I would recommend delaying calling get_event_loop() until run(), just so
> that all of the loop management stuff is handled in one place. That way,
> the loop isn't "fixed" until we call run().
>
>
>> + self.loop = None
>> + super().__init__()
>> +
>> + # Gracefully handle SIGTERM and SIGINT signals
>> + cancel_signals = [signal.SIGTERM, signal.SIGINT]
>> + for sig in cancel_signals:
>> + self.aloop.add_signal_handler(sig, self.kill_app)
>>
>
> If you agree with the above comment, this needs to move into run() as well.
>
>
>> +
>> + def _cb_outbound(self, msg):
>> + urwid.emit_signal(self, UPDATE_MSG, "<-- " + str(msg))
>> + return msg
>> +
>> + def _cb_inbound(self, msg):
>> + urwid.emit_signal(self, UPDATE_MSG, "--> " + str(msg))
>> + return msg
>> +
>> + async def wait_for_events(self):
>> + async for event in self.events:
>> + self.handle_event(event)
>> +
>> + async def _send_to_server(self, msg):
>> + # TODO: Handle more validation errors (eg: ValueError)
>> + try:
>> + response = await self._raw(bytes(msg, 'utf-8'))
>> + logging.info('Response: %s %s', response, type(response))
>>
>
> You could log the responses in the inbound hook instead.
> I'd also use self.logger.debug instead of logging.info(...) so that you
> re-use the same logger instance.
>
>
>> + except ExecuteError:
>> + logging.info('Error response from server for msg: %s', msg)
>>
>
> self.logger.info() here.
>
>
>> + except ExecInterruptedError:
>> + logging.info('Error server disconnected before reply')
>>
>
> And same here.
>
>
>> + # FIXME: Handle this better
>>
>
> What ideas do you have for handling this better? What's wrong with it
> right now?
>
>
>> + # Show the disconnected message in the history window
>> + urwid.emit_signal(self, UPDATE_MSG,
>> + '{"error": "Server disconnected before
>> reply"}')
>> + self.window.footer.set_text("Server disconnected")
>> + except Exception as err:
>> + logging.info('Exception from _send_to_server: %s', str(err))
>>
>
> use self.logger.error here, since it's an unhandled error.
>
>
>> + raise err
>> +
>> + def cb_send_to_server(self, msg):
>> + create_task(self._send_to_server(msg))
>> +
>>
>
> I wish we didn't have to create tasks for this, but I suppose bridging
> asyncio and Urwid is just simply not very pretty. One thing to keep in mind
> is that when you create a task without a handle like this (i.e. you aren't
> saving the 'task' value anywhere), if that task exits with an Exception, it
> will cause Python to emit that "Unhandled Exception" warning that you see
> ... but only once the program otherwise ends. What will end up happening in
> practice is that the task will die without showing you the Exception.
>
> You might want to find a way to make Python crash a little more
> aggressively when an unhandled exception happens in a task, or otherwise
> make sure that ERROR level logging messages are visible directly in the TUI
> history pane, so that we can see te errors when they happen.
>
>
An update on this:
I was re-reading the asyncio docs last night and I was reminded that
asyncio has an exception handler mechanism that you can set:
https://docs.python.org/3/library/asyncio-eventloop.html#error-handling-api
However, I don't actually know right now if this handler applies to *tasks*
or if it applies only to *coroutines*. What I do know is that in the
*default event loop* in asyncio, if you have a task that raises an
Exception, that Exception does not by default get printed out anywhere or
otherwise cause any kind of obvious failure. It is normally completely
invisible until you *await on the task*.
Here's an example of what I mean:
```
async def failing_task():
raise Exception("Where does this exception go?")
async def some_function():
# This will work, it won't raise an Exception
task = create_task(failing_task())
# As of right now, failing_task won't even have run yet. We still
retain control.
# Yielding here allows the scheduled task to run and immediately fail.
await asyncio.sleep(0)
# When we reach this point, the task has definitely failed already.
# However, we won't have seen any error printed anywhere yet ...
await task # <-- Any exceptions raised by the task happen HERE
````
That's what happens when you *save a reference to the task being created*,
but what happens when you don't save a reference to it?
```
async def failing_task():
raise Exception("Where does this exception go?")
async def some_function():
create_task(failing_task()) # This still works just fine.
await asyncio.sleep(0) # This will give the task a chance to run. It
will fail.
# When we reach this point, the task has definitely failed already.
# However, we won't have seen any error printed anywhere yet ...
# Worse, we don't have a handle to the task to find what its error was!
# Where did its exception go?
# When this program ends, you will get Python warnings complaining that
there were task results that were never retrieved.
```
This is why I am concerned about the usage of create_task() without saving
a reference to the created task, because it makes Exception management
trickier. However, I am willing to bet (but do not KNOW), that Urwid has
its own exception handlers it's already using ... I would confirm that
Exceptions raised in "orphaned" tasks have reliable behavior that you can
count on; it's not obvious to me at a glance that it works like we expect.
If Urwid treats Exceptions from orphaned tasks by stopping the application,
that's acceptable to me -- it means that anything we forget to handle
ourselves will get logged and crash the app, which is actually quite an
improvement over "the error just gets silently swallowed."
Make sense?
> + def unhandled_input(self, key):
>> + if key == 'esc':
>> + self.kill_app()
>> +
>> + def kill_app(self):
>> + # TODO: Work on the disconnect logic
>> + create_task(self._kill_app())
>> +
>>
>
> Yes, the next thing I'd like to see here is reconnection logic -- I made a
> little prototype in some code I gave you, but it probably needs to be
> touched up. I recall that my version would attempt to reconnect infinitely
> whenever the app was disconnected, regardless of what happened to cause the
> disconnection. What we likely want is only to reconnect on certain kinds of
> errors -- ConnectionResetError is likely a good candidate, but other kinds
> of problems are likely ones we want to STAY disconnected when encountering.
>
> We also probably want some logic like num_retries, and retry_delay.
>
>
>> + async def _kill_app(self):
>> + # It is ok to call disconnect even in disconnect state
>> + await self.disconnect()
>>
>
> Be aware that this raises Exceptions if the connection terminated
> ungracefully, i.e. the server hung up before we were expecting it. You
> might want to handle it (and do something related to connection retry
> management) first -- there are at least a few erorrs here that wouldn't be
> too strange to run into.
>
> I worry that when you hit 'esc' instead of ctrl^C, you'll see different
> behavior here -- because ctrl+C creates a task, if this raises an exception
> here, I think that we won't exit -- we'll get another unhandled exception
> that won't show up until the app exits. I'm not confident in this, but I
> think you should confirm that exiting both ways works exactly like you
> think it does.
>
>
>> + logging.info('disconnect finished, Exiting app')
>>
>
> self.logger.debug
>
>
>> + raise urwid.ExitMainLoop()
>> +
>> + def handle_event(self, event):
>> + if event['event'] == 'SHUTDOWN':
>> + self.window.footer.set_text('Server shutdown')
>> +
>>
>
> A bit spartan as an event handler, but it serves its purpose as a
> demonstration for the proof of concept.
>
> It'd be nice to have the footer show a [VM: {state}] status where the
> state maps 1:1 with qapi/run-state.json's @RunState enumeration. I made a
> quick hack that you saw, but it wasn't strictly correct.
>
>
>> + async def connect_server(self):
>> + try:
>> + await self.connect(self.address)
>> + self.window.footer.set_text("Connected to {:s}".format(
>> + f"{self.address[0]}:{self.address[1]}"
>> + if isinstance(self.address, tuple)
>> + else self.address
>> + ))
>> + except ConnectError as err:
>> + logging.debug('Cannot connect to server %s', str(err))
>> + self.window.footer.set_text('Server shutdown')
>>
>
> Like in other places, I wonder what happens if we have an unhandled
> exception here, because this is running in a task.
>
>
>> +
>> + def run(self):
>> + self.aloop.set_debug(True)
>>
>
> Add a debug argument to run() and default it to False, and add a --debug
> flag to the argparser that turns this on conditionally instead.
>
>
>> + event_loop = urwid.AsyncioEventLoop(loop=self.aloop)
>> + self.loop = urwid.MainLoop(self.window,
>> + unhandled_input=self.unhandled_input,
>> + handle_mouse=True,
>> + event_loop=event_loop)
>> +
>> + create_task(self.wait_for_events(), self.aloop)
>> + create_task(self.connect_server(), self.aloop)
>> + try:
>> + self.loop.run()
>> + except Exception as err:
>> + logging.error('%s\n%s\n', str(err), pretty_traceback())
>> + raise err
>> +
>> +
>> +def main():
>> + parser = argparse.ArgumentParser(description='AQMP TUI')
>> + parser.add_argument('-a', '--address', metavar='IP:PORT',
>> required=True,
>> + help='Address of the QMP server', dest='address')
>> + parser.add_argument('--log', help='Address of the QMP server',
>> + dest='log_file')
>> + args = parser.parse_args()
>> +
>> + logging.basicConfig(filename=args.log_file, level=logging.DEBUG)
>> +
>> + address = args.address.split(':')
>> + address[1] = int(address[1])
>> +
>> + App(tuple(address)).run()
>>
>
> I would take the address as a positional argument instead of with the
> '--address' flag to mimic how qmp-shell works.
>
>>
>> +
>> +
>> +if __name__ == '__main__':
>> + main() # type: ignore
>> diff --git a/python/setup.cfg b/python/setup.cfg
>> index c62803bffc..c6d38451eb 100644
>> --- a/python/setup.cfg
>> +++ b/python/setup.cfg
>> @@ -81,8 +81,22 @@ namespace_packages = True
>> # fusepy has no type stubs:
>> allow_subclassing_any = True
>>
>> +[mypy-qemu.aqmp.aqmp_tui]
>> +disallow_untyped_defs = False
>> +disallow_incomplete_defs = False
>> +check_untyped_defs = False
>>
>
> Just keep in mind that we'll need to remove these particular ignores. The
> rest can stay.
>
>
>> +# urwid and urwid_readline have no type stubs:
>> +allow_subclassing_any = True
>> +
>> +# The following missing import directives are because these libraries do
>> not
>> +# provide type stubs. Allow them on an as-needed basis for mypy.
>> [mypy-fuse]
>> -# fusepy has no type stubs:
>> +ignore_missing_imports = True
>> +
>> +[mypy-urwid]
>> +ignore_missing_imports = True
>> +
>> +[mypy-urwid_readline]
>> ignore_missing_imports = True
>>
>> [pylint.messages control]
>> --
>> 2.17.1
>>
>>
> Looking good so far, let's focus on managing the connection state and
> making sure that Exceptions raised from task contexts are handled properly.
> I still need to look more deeply into the classes below App, but I wanted
> to give you your overdue feedback. Thank you for your patience!
>
> --js
>
