| Message ID | 20200710050649.32434-6-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/machine.py: refactor shutdown | expand |
On 7/10/20 7:06 AM, 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(-) > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
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. > 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). - Cleber.
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. >
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. > >> 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). > The docstring gets hit with a polish beam later in the series, but still neglects that it might do nothing. I squashed in a change to patch #10 and pushed to my gitlab to add this clarification to the docstring. https://gitlab.com/jsnow/qemu/-/tree/python-package-shutdown https://gitlab.com/jsnow/qemu/-/commit/b3f14deb730fda9bb2a28a9366a8096d3617f4f4
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() + 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 + self._early_cleanup() if self.is_running():
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(-)