Re: [PATCH v5 05/12] python/machine.py: Prohibit multiple shutdown() calls

[John Snow]

On 7/13/20 10:48 PM, Cleber Rosa wrote:
> On Fri, Jul 10, 2020 at 01:06:42AM -0400, John Snow wrote:
>> If the VM is not launched, don't try to shut it down. As a change,
>> _post_shutdown now unconditionally also calls _early_cleanup in order to
>> offer comprehensive object cleanup in failure cases.
>>
>> As a courtesy, treat it as a NOP instead of rejecting it as an
>> error. This is slightly nicer for acceptance tests where vm.shutdown()
>> is issued unconditionally in tearDown callbacks.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  python/qemu/machine.py | 14 +++++++++++++-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
>> index cac466fbe6..e66a7d66dd 100644
>> --- a/python/qemu/machine.py
>> +++ b/python/qemu/machine.py
>> @@ -283,6 +283,13 @@ def _post_launch(self):
>>              self._qmp.accept()
>>  
>>      def _post_shutdown(self):
>> +        """
>> +        Called to cleanup the VM instance after the process has exited.
>> +        May also be called after a failed launch.
>> +        """
>> +        # Comprehensive reset for the failed launch case:
>> +        self._early_cleanup()
>> +
> 
> I'm not following why this is needed here.  AFAICT, the closing of the
> console socket file was introduced when the QEMU process was alive,
> and doing I/O on the serial device attached to the console file (and
> was only necessary because of that).  In those scenarios, a race
> between the time of sending the QMP command to quit, and getting a
> response from QMP could occur.
> 
> But here, IIUC, there's no expectations for the QEMU process to be alive.
> To the best of my understanding and testing, this line did not contribute
> to the robustness of the shutdown behavior.
> 
> Finally, given that currently, only the closing of the console socket
> is done within _early_cleanup(), and that is conditional, this also does
> no harm AFAICT.  If more early cleanups operations were needed, then I
> would feel less bothered about calling it here.
> 

Sure, I can tie it up to be a little less abstract, but I'll go over
some of my reasons below to see if any of them resonate for you.

What I wanted was three things:

(1) A single call that comprehensively cleaned up the VM object back to
a known state, no matter what state it was in.

This is the answer to your first paragraph: I wanted a function to
always thoroughly reset an object back to base state. In the current
code, we won't have a console socket object here yet.

I felt this was good as it illustrates to other contributors what the
comprehensive steps to reset the object are.

(It might be considered bad in that it does perform potentially
unnecessary cleanup at times; but as a matter of taste I prefer this to
conditional inconsistency.)


(2) A guarantee that no matter how a VM object was terminated (either
via context handler, shutdown(), or wait()) that the exact same cleanup
steps would be performed for consistency

This is why wait() now points to shutdown(), and why _post_shutdown is
called outside of either of the shutdown handlers. No matter what,
_post_shutdown is getting called and it is going to clean everything.


(3) Avoiding special-casing certain cleanup actions open-coded in the
shutdown handlers.

It's #3 that wound up with me creating the "_early_cleanup" hook. I
created it because I had the idea to create "Console" and "QMP Socket"
as inheritable mixins to help keep the core class simpler.

In these cases, the mixins need a hook for "early cleanup."

No, I didn't finish that work and it's not ready. We could remove it for
now, but that's why this is here and why it's named like it is.

Long term, I think Machine Mixins are the only viable way to add more
features. The upstream version of the code now has more than 10
arguments and 20+ fields. It's getting unwieldy.


(....all that said, we can still do it later. If you prefer, for now, I
can at least rename the function "_close_socket" to make it less abstract.

(I think it should still be called in _post_shutdown, though.))

>>          if self._qmp:
>>              self._qmp.close()
>>              self._qmp = None
>> @@ -328,7 +335,7 @@ def launch(self):
>>              self._launch()
>>              self._launched = True
>>          except:
>> -            self.shutdown()
>> +            self._post_shutdown()
>>  
>>              LOG.debug('Error launching VM')
>>              if self._qemu_full_args:
>> @@ -357,6 +364,8 @@ def _launch(self):
>>      def _early_cleanup(self) -> None:
>>          """
>>          Perform any cleanup that needs to happen before the VM exits.
>> +
>> +        Called additionally by _post_shutdown for comprehensive cleanup.
>>          """
>>          # If we keep the console socket open, we may deadlock waiting
>>          # for QEMU to exit, while QEMU is waiting for the socket to
>> @@ -377,6 +386,9 @@ def shutdown(self, has_quit=False, hard=False):
>>          """
>>          Terminate the VM and clean up
>>          """
>> +        if not self._launched:
>> +            return
>> +
> 
> Because of the additional logic, it'd be a good opportunity to make
> the docstring more accurate.  This method may now *not* do *any* of if
> termination and cleaning (while previously it would attempt those
> anyhow).
> 

Kinda-sorta. The old version used to have "if self.running", which could
race.

I'll amend the docstring(s) as needed to make it clear that it's a NOP
if the machine has been cleaned up already.

> - Cleber.
>