[v2,12/17] 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 <jsnow@redhat.com>

---

Yes, I regret that this class has become quite a dumping ground for
complexity around the exit path. It's in need of a refactor to help
separate out the exception handling and cleanup mechanisms from the
VM-related stuff, but it's not a priority to do that just yet -- but
it's on the list.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 python/qemu/machine/machine.py | 48 +++++++++++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 6 deletions(-)

On Wed, Sep 22, 2021 at 08:49:33PM -0400, John Snow wrote:
> 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 <jsnow@redhat.com>
> 
> ---
> 
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

This second S-o-b won't matter.

Reviewed-by: Eric Blake <eblake@redhat.com>

On Thu, Oct 7, 2021 at 11:08 AM Eric Blake <eblake@redhat.com> wrote:

> On Wed, Sep 22, 2021 at 08:49:33PM -0400, John Snow wrote:
> > 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 <jsnow@redhat.com>
> >
> > ---
> >
> > Yes, I regret that this class has become quite a dumping ground for
> > complexity around the exit path. It's in need of a refactor to help
> > separate out the exception handling and cleanup mechanisms from the
> > VM-related stuff, but it's not a priority to do that just yet -- but
> > it's on the list.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> This second S-o-b won't matter.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>

Sorry, it's a bug with git-publish I've just not invested time into fixing.
It happens when I use a '---' to add an additional note for reviewers.
git-publish likes to add the second line because it doesn't "see" the first
one anymore. It's harmless, ultimately, but ... it sure does make me look
like I have no idea what I'm doing ;)

--js

On Wed, Sep 22, 2021 at 8:50 PM John Snow <jsnow@redhat.com> wrote:

> 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 <jsnow@redhat.com>
>
> ---
>
> Yes, I regret that this class has become quite a dumping ground for
> complexity around the exit path. It's in need of a refactor to help
> separate out the exception handling and cleanup mechanisms from the
> VM-related stuff, but it's not a priority to do that just yet -- but
> it's on the list.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  python/qemu/machine/machine.py | 48 +++++++++++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/python/qemu/machine/machine.py
> b/python/qemu/machine/machine.py
> index 0bd40bc2f76..c33a78a2d9f 100644
> --- a/python/qemu/machine/machine.py
> +++ b/python/qemu/machine/machine.py
> @@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
>          # 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 @@ def _launch(self) -> None:
>                                         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._quit_issued:
>

I strongly suspect that this line needs to actually be "if not
(self._user_killed or self._quit_issued):" to also handle the force-kill
cases.

I wrote a different sync compatibility layer in a yet-to-be-published
branch and noticed this code still allows EOFError through. Why it did not
seem to come up in *this* series I still don't know -- I think the timing
of when exactly the coroutines get scheduled can be finnicky depending on
what else is running. So, sometimes, we manage to close the loop before we
get EOF and sometimes we don't.

I am mulling over adding a "tolerate_eof: bool = False" argument to
disconnect() to allow the caller to handle this stuff with a little less
boilerplate. Anyone have strong feelings on that?

(Ultimately, because there's no canonical "goodbye" in-band message, the
QMP layer itself can never really know if EOF was expected or not -- that's
really up to whomever is managing the connection, but it does add a layer
of complexity around the exit path that I am starting to find is a
nuisance. Not to mention that there are likely many possible cases in which
we might expect the remote to disappear on us that don't involve using QMP
at all -- kill is one, using the guest agent to coerce the guest into
shutdown is another. Networking and migration are other theoretical(?)
avenues for intended disconnects.)


> +                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 @@ def _soft_shutdown(self, timeout: Optional[int]) ->
> None:
>          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)
> --
> 2.31.1
>
>

On 07.10.21 18:52, John Snow wrote:
>
>
> On Wed, Sep 22, 2021 at 8:50 PM John Snow <jsnow@redhat.com> wrote:
>
>     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 <jsnow@redhat.com>
>
>     ---
>
>     Yes, I regret that this class has become quite a dumping ground for
>     complexity around the exit path. It's in need of a refactor to help
>     separate out the exception handling and cleanup mechanisms from the
>     VM-related stuff, but it's not a priority to do that just yet -- but
>     it's on the list.
>
>     Signed-off-by: John Snow <jsnow@redhat.com>
>     ---
>      python/qemu/machine/machine.py | 48
>     +++++++++++++++++++++++++++++-----
>      1 file changed, 42 insertions(+), 6 deletions(-)
>
>     diff --git a/python/qemu/machine/machine.py
>     b/python/qemu/machine/machine.py
>     index 0bd40bc2f76..c33a78a2d9f 100644
>     --- a/python/qemu/machine/machine.py
>     +++ b/python/qemu/machine/machine.py
>     @@ -342,9 +342,15 @@ def _post_shutdown(self) -> None:
>              # 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 @@ def _launch(self) -> None:
>                                             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._quit_issued:
>
>
> I strongly suspect that this line needs to actually be "if not 
> (self._user_killed or self._quit_issued):" to also handle the 
> force-kill cases.

Sounds right.

(Other than that, the patch looks good to me.)

> I wrote a different sync compatibility layer in a yet-to-be-published 
> branch and noticed this code still allows EOFError through. Why it did 
> not seem to come up in *this* series I still don't know -- I think the 
> timing of when exactly the coroutines get scheduled can be finnicky 
> depending on what else is running. So, sometimes, we manage to close 
> the loop before we get EOF and sometimes we don't.
>
> I am mulling over adding a "tolerate_eof: bool = False" argument to 
> disconnect() to allow the caller to handle this stuff with a little 
> less boilerplate. Anyone have strong feelings on that?

FWIW I don’t.

> (Ultimately, because there's no canonical "goodbye" in-band message, 
> the QMP layer itself can never really know if EOF was expected or not 
> -- that's really up to whomever is managing the connection, but it 
> does add a layer of complexity around the exit path that I am starting 
> to find is a nuisance. Not to mention that there are likely many 
> possible cases in which we might expect the remote to disappear on us 
> that don't involve using QMP at all -- kill is one, using the guest 
> agent to coerce the guest into shutdown is another. Networking and 
> migration are other theoretical(?) avenues for intended disconnects.)

For the iotests we don’t have a guest, so I suppose this doesn’t concern 
me, does it? :)

Hanna