[libvirt] [PATCH 5/5] qemu: implement the 'libvirt_qemu' shim for launching guests externally

[Daniel P. Berrange]

On Tue, Jan 09, 2018 at 03:13:41PM +0100, Martin Kletzander wrote:
> On Wed, Dec 20, 2017 at 04:47:50PM +0000, Daniel P. Berrange wrote:
> > This introduces a new binary 'libvirt_qemu' which can be used to launch
> > guests externally from libvirtd. eg
> > 
> >  libvirt_qemu -c qemu:///system /path/to/xml/file
> > 
> > This will launch a guest from the requested XML file and then connect to
> > qemu:///system to register it with libvirtd. At this point all normal
> > libvirtd operations should generally work.
> > 
> > NB, this impl has many flaws:
> > 
> > - We don't generate a unique 'id', so all guests will get id==1.
> >   Surprisingly this doesn't break the world.
> > 
> > - Tracking of unique name + UUIDs is *not* race free.
> > 
> > - No tracking/allocation of shared resource state (PCI devices,
> >   etc)
> > 
> 
> You will also not get the same events as you would when starting the domain
> as usual.

Hmm, yes, interesting. If someone is connected to libvirtd watching events
they'll  not get any startup events. That's something we'd need to fix.

> > Most other functionality works, but might have expected results.
> > 
> 
> If I start a domain with a network, it segfaults.  I'll see later if that's my
> fault or not.  It probably (and hopefully) is.

Not intentional if that's happening !


> > - The libvirt_qemu will exit if startup fails, however, it
> >   won't exit when QEMU later shuts down - you must listen
> >   for libvirtd events to detect that right now. We'll
> >   fix that
> > 
> > - Killing the libvirt_qemu shim will not kill the QEMU
> >   process. You must kill via the libvirt API as normal.
> >   This won't change, because we need to be able to
> >   ultimately restart the libvirt_qemu shim in order to
> >   apply software updates to running instance.
> >   We might wire up a special signal though to let
> >   you kill libvirt_qemu & take out QEMU at same time
> >   eg SIGQUIT or something like that perhaps.
> > 
> 
> IMHO we should use standard signals to do standard things.  TERM should
> gracefully shutdown the domain.  Not in PoC, but later.  That way users
> can treat the shim process as other processes.

I guess we can use SIGUSR1 to trigger restart of the shim while
leaving QEMU running.


> > +if WITH_QEMU
> > +if WITH_LIBVIRTD
> > +libexec_PROGRAMS += libvirt_qemu
> > +
> 
> Shouldn't I be able to launch this as a user without mgmt app?  It
> should go to bin_PROGRAMS, I presume.

Yes

> > +libvirt_qemu_SOURCES = \
> > +		$(QEMU_CONTROLLER_SOURCES) \
> > +		$(DATATYPES_SOURCES)
> > +libvirt_qemu_LDFLAGS = \
> > +		$(AM_LDFLAGS) \
> > +		$(PIE_LDFLAGS) \
> > +		$(NULL)
> > +libvirt_qemu_LDADD = \
> > +		libvirt.la \
> > +		libvirt-qemu.la \
> > +		libvirt_driver_qemu_impl.la
> > +if WITH_NETWORK
> > +libvirt_qemu_LDADD += libvirt_driver_network_impl.la
> > +endif WITH_NETWORK
> > +if WITH_STORAGE
> > +libvirt_qemu_LDADD += libvirt_driver_storage_impl.la
> > +endif WITH_STORAGE
> 
> > +libvirt_qemu_LDADD += ../gnulib/lib/libgnu.la $(LIBSOCKET)
> 
> This is not part of any condition, should be up there with libvirt.la
> just for clarity.

IIRC, libgnu.la needs to be the last library listed

> > +static void
> > +virQEMUControllerDriverFree(virQEMUDriverPtr driver)
> > +{
> > +    if (!driver)
> > +        return;
> > +
> > +    virObjectUnref(driver->config);
> > +    virObjectUnref(driver->hostdevMgr);
> > +    virHashFree(driver->sharedDevices);
> > +    virObjectUnref(driver->caps);
> > +    virObjectUnref(driver->qemuCapsCache);
> > +
> > +    virObjectUnref(driver->domains);
> > +    virObjectUnref(driver->remotePorts);
> > +    virObjectUnref(driver->webSocketPorts);
> > +    virObjectUnref(driver->migrationPorts);
> > +    virObjectUnref(driver->migrationErrors);
> > +
> > +    virObjectUnref(driver->xmlopt);
> > +
> > +    virSysinfoDefFree(driver->hostsysinfo);
> > +
> > +    virObjectUnref(driver->closeCallbacks);
> > +
> > +    VIR_FREE(driver->qemuImgBinary);
> > +
> > +    virObjectUnref(driver->securityManager);
> > +
> > +    ebtablesContextFree(driver->ebtables);
> > +
> > +    virLockManagerPluginUnref(driver->lockManager);
> > +
> > +    virMutexDestroy(&driver->lock);
> > +    VIR_FREE(driver);
> > +}
> > +
> > +static virQEMUDriverPtr virQEMUControllerNewDriver(bool privileged)
> 
> This is very similar to what I did, but I just exported this function
> privately si that there is less duplication of code and I added an enum
> that is used as a parameter instead of the bool @privileged.  It is a
> tristate (user, system, shim) that special-cases the shim slightly.

Yeah, there's definitely much more duplication here that I would like.
I would expect to refactor this better.

> The reason behind that is that I kind of presumed we need to be able to
> launch system QEMUs with non-root user.  Thinking about it I'm not sure
> that's the case for kubevirt, but it definitely is for others.  The way
> it is done now is that you initialize the driver based on geteuid(), but
> you connect to any uri specified by the user.  What I had in mind was
> that with this shim PoC users would be able to start their own VMs (if
> libvirtd permissions are set up the right way, even with some directory
> access changes done in the daemon) as system VMs as if they had seclabel
> set up for their user.  I think it's feasible, but I maybe overestimated
> what's possible for the PoC.



Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|