Re: [PATCH 2/2] python/qemu/machine: accept QMP connection asynchronously

[John Snow]

On Thu, Jun 30, 2022 at 4:23 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 07:54:08PM -0400, John Snow wrote:
> > On Tue, Jun 28, 2022 at 10:17 AM Daniel P. Berrangé <berrange@redhat.com> 
> > wrote:
> > >
> > > On Tue, Jun 28, 2022 at 05:49:39PM +0400, marcandre.lureau@redhat.com 
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > >
> > > > QMP accept is currently synchronous. If qemu dies before the connection
> > > > is established, it will wait there. Instead turn the code to do
> > > > concurrently accept() and wait(). Returns when the first task is
> > > > completed to determine whether a connection was established.
> > >
> > > If the spawned QEMU process was given -daemonize, won't this code
> > > mistakenly think the subprocess has quit ?
> >
> > Do we use daemonize with this code anywhere? Is it important that we
> > are able to?
>
> Well what's the intended breadth of use cases this code wants to
> target ?

Well, I dunno. I'm going to say that machine.py is something that will
probably stay in-tree as an ad-hoc testing utility. I have some
thoughts about growing it into something "just a little bit more than
that", but I think we can cross that bridge when we get there. I will
probably do like what I did for QMP: write a replacement, test it with
our existing test infrastructure, then do a drop-in switcheroo.

>
> If you don't daemonize QEMU, then QEMU will (likely) get killed off
> when the parent python process terminates. That can be ok for adhoc
> testing scenarios where the QEMU process is transient and throwaway,
> or for using QEMU as an embedded technology (think libguestfs). If
> you want your QEMU to run full OS workloads, then generally you won't
> want it to die when the mgmt app is restarted (eg for software upgrade),
> whereupon you want to daemonize.
>

Yeah, I think that's a case that I wasn't invested in managing.
Certainly it's an expansion of scope.

> > Many of the shutdown routines I wrote expect to work directly with a
> > launched process ... at least, that expectation exists in my head. I
> > suppose a lot of this code may actually just coincidentally work with
> > -daemonize and I wouldn't have noticed. I certainly haven't been
> > testing it explicitly. I definitely make no accommodations for it, so
> > I would expect some stale processes in various cases at a minimum.
>
> Looking at the code it probably works by accident - the shutdown()
> methods kinda assumes we're talking to a direct child, but it'll
> happen to work right now as it'll simply cleanup the defunct
> intermediate child, while QEMU stays running.
>

That's my conclusion, yes.

> > If we want to expand to accommodate this feature, can we do that
> > later? Machine needs a bit of a remodel anyway. (I want to write an
> > 'idiomatic' asyncio version to match the QMP lib. I have some
> > questions to work out WRT which portions of this appliance can be
> > upstreamed and which need to remain only in our testing tree. We can
> > talk about those pieces later, just throwing it out there that it's on
> > my list.)
>
> The machine class is probably the part that looks least ready to be
> published as an API on pypi. Its code really shows its origins as an
> adhoc testing framework, rather than a general purpose API for running
> and managing QEMU from python.

I agree.

It might still be useful in some form, eventually, to use as supported
example code of showing how you can launch QEMU and interact with it
via QMP. In the current form, though, it's definitely just ad-hoc nuts
and bolts for the test suite. I am fine with keeping it like that for
purposes of review and maintenance, etc.

--js