Re: [PATCH 3/6] python/aqmp-tui: Add AQMP TUI draft

["Niteesh G. S." <niteesh.gs@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 16650 bytes --]

On Thu, Jul 8, 2021 at 8:50 AM John Snow <jsnow@redhat.com> wrote:

>
>
> On Fri, Jul 2, 2021 at 5:26 PM G S Niteesh Babu <niteesh.gs@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.gs@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.gs@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.
>
AFAIK urwid has default signals only for inbuilt widgets like Button, Edit,
and a few more. For eg the button class has a 'click' signal which we can
connect to and is emitted on a button click. I had again gone through the
document to check if there are any default one that we can use here. But I
couldn't find any.
And yes we use UPDATE_MSG to notify the history list widget of a new
message.

If not, you may use type(self) here which looks just a little cleaner.
>
Fixed.

>
>
>> +        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().
>
Fixed.

>
>
>> +        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.
>
Fixed.

>
>
>> +
>> +    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.
>
Fixed.

>
>
>> +        except ExecuteError:
>> +            logging.info('Error response from server for msg: %s', msg)
>>
>
> self.logger.info() here.
>
Fixed

>
>
>> +        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?
>
We can initiate a reconnect here and maybe add this request to a pending
list and
prompt the user for a reissue automatically after reconnecting.

>
>
>> +            # 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.
>
Fixed.

> +            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.
>
Though discussed in IRC. Putting it here so that others can also see.
We are OK to create tasks with one-shot actions because the urwid loop
takes care for handling the exceptions and crashing the App.
This relieves us from the pain of manually handling the task exceptions.

>
>
>> +    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.
>
As you mentioned our primary goal is to have a proper base. I'll work on
this feature once we are done reviewing this base.

>
>
>> +    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.
>
Fixed.

>
>
>> +        logging.info('disconnect finished, Exiting app')
>>
>
> self.logger.debug
>
Fixed.

>
>
>> +        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.
>
Sure will add in future revisions.

>
>
>> +    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.
>
Fixed.

>
>
+
>> +    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.
>
Fixed.

> +        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.
>
Fixed

>
>>
>> +
>> +
>> +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.
>
Yup!

>
>
>> +# 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!
>
Thanks for your feedback :)

>
> --js
>

[-- Attachment #2: Type: text/html, Size: 28414 bytes --]