This patch series proposes a new way to build a "libvirt QEMU shim"
which supports more use cases than the previous approach from last year,
as well as being vastly simpler to implement.

Specifically

 - We can use this to embed the QEMU driver in unit tests allowing
   the full range of odriver functionality to be exercised during
   *unit* testing without interfering with the host OS libvirtd.

 - Apps like libguestfs can embed the QEMU driver to allow them to
   spawn VMs that are immediate children of their app, thus inheriting
   process context, as well as hiding these "service VM" from the
   main libvirtd.

   This would avoid a common irritation where libguestfs VMs suddenly
   appear & disappear in virt-manager's VM list.

 - Apps like kubevirt could use this to eliminate the need to use the
   RPC layer for libvirt. It can directly embed the QEMU driver giving
   it more direct control over how it is run & again inheriting process
   state. This is useful for kubevirt's  one VM per container model
   where Kubernetes itself provides the aggregated view, thus making
   libvirtd's aggregated view redundant

 - The previous shim was very much one shim == one QEMU process. This
   new embed approach supports that of course, but there's also the
   option to run multiple VMs if desired.

 - Using a virtual root directory for QEMU driver state meams we do
   not need to solve the hard refactoring problem of getting the main
   libvirtd to detect the VMs launched via the shim. They can safely
   live independantly.

 - Thanks to the driver refactoring work it is still possible for the
   embedded drivers to talk to main libvirtd to use the secondary
   drivers for storage/network/etc if needed. Ideally we would allow
   those secondary drivers to be embedded too but that's not implemented
   here. Most important would be the secrets driver. Other drivers are
   much less important.

Daniel P. Berrangé (5):
  access: report an error if no access manager is present
  libvirt: pass a root directory path into drivers
  qemu: honour the root parameter during driver initialization
  libvirt: support an "embed" URI path selector for QEMU driver
  qemu: introduce a new style libvirt_qemu_shim program

 src/access/viraccessmanager.c           |  5 ++
 src/driver-state.h                      |  1 +
 src/interface/interface_backend_netcf.c |  1 +
 src/interface/interface_backend_udev.c  |  1 +
 src/libvirt.c                           | 41 ++++++++++++++-
 src/libvirt_internal.h                  |  1 +
 src/libxl/libxl_driver.c                |  1 +
 src/lxc/lxc_driver.c                    |  2 +
 src/network/bridge_driver.c             |  1 +
 src/node_device/node_device_hal.c       |  1 +
 src/node_device/node_device_udev.c      |  1 +
 src/nwfilter/nwfilter_driver.c          |  1 +
 src/qemu/Makefile.inc.am                |  7 +++
 src/qemu/qemu_conf.c                    | 41 +++++++++------
 src/qemu/qemu_conf.h                    |  3 +-
 src/qemu/qemu_driver.c                  |  9 ++--
 src/qemu/qemu_shim.c                    | 70 +++++++++++++++++++++++++
 src/remote/remote_daemon.c              |  1 +
 src/remote/remote_driver.c              |  8 +++
 src/secret/secret_driver.c              |  1 +
 src/storage/storage_driver.c            |  1 +
 src/vz/vz_driver.c                      |  1 +
 22 files changed, 179 insertions(+), 20 deletions(-)
 create mode 100644 src/qemu/qemu_shim.c

-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 5/17/19 2:24 PM, Daniel P. Berrangé wrote:
> This patch series proposes a new way to build a "libvirt QEMU shim"
> which supports more use cases than the previous approach from last year,
> as well as being vastly simpler to implement.
> 
> Specifically
> 
>   - We can use this to embed the QEMU driver in unit tests allowing
>     the full range of odriver functionality to be exercised during
>     *unit* testing without interfering with the host OS libvirtd.
> 
>   - Apps like libguestfs can embed the QEMU driver to allow them to
>     spawn VMs that are immediate children of their app, thus inheriting
>     process context, as well as hiding these "service VM" from the
>     main libvirtd.
> 
>     This would avoid a common irritation where libguestfs VMs suddenly
>     appear & disappear in virt-manager's VM list.
> 
>   - Apps like kubevirt could use this to eliminate the need to use the
>     RPC layer for libvirt. It can directly embed the QEMU driver giving
>     it more direct control over how it is run & again inheriting process
>     state. This is useful for kubevirt's  one VM per container model
>     where Kubernetes itself provides the aggregated view, thus making
>     libvirtd's aggregated view redundant
> 
>   - The previous shim was very much one shim == one QEMU process. This
>     new embed approach supports that of course, but there's also the
>     option to run multiple VMs if desired.
> 
>   - Using a virtual root directory for QEMU driver state meams we do
>     not need to solve the hard refactoring problem of getting the main
>     libvirtd to detect the VMs launched via the shim. They can safely
>     live independantly.
> 
>   - Thanks to the driver refactoring work it is still possible for the
>     embedded drivers to talk to main libvirtd to use the secondary
>     drivers for storage/network/etc if needed. Ideally we would allow
>     those secondary drivers to be embedded too but that's not implemented
>     here. Most important would be the secrets driver. Other drivers are
>     much less important.
> 
> Daniel P. Berrangé (5):
>    access: report an error if no access manager is present
>    libvirt: pass a root directory path into drivers
>    qemu: honour the root parameter during driver initialization
>    libvirt: support an "embed" URI path selector for QEMU driver
>    qemu: introduce a new style libvirt_qemu_shim program
> 
>   src/access/viraccessmanager.c           |  5 ++
>   src/driver-state.h                      |  1 +
>   src/interface/interface_backend_netcf.c |  1 +
>   src/interface/interface_backend_udev.c  |  1 +
>   src/libvirt.c                           | 41 ++++++++++++++-
>   src/libvirt_internal.h                  |  1 +
>   src/libxl/libxl_driver.c                |  1 +
>   src/lxc/lxc_driver.c                    |  2 +
>   src/network/bridge_driver.c             |  1 +
>   src/node_device/node_device_hal.c       |  1 +
>   src/node_device/node_device_udev.c      |  1 +
>   src/nwfilter/nwfilter_driver.c          |  1 +
>   src/qemu/Makefile.inc.am                |  7 +++
>   src/qemu/qemu_conf.c                    | 41 +++++++++------
>   src/qemu/qemu_conf.h                    |  3 +-
>   src/qemu/qemu_driver.c                  |  9 ++--
>   src/qemu/qemu_shim.c                    | 70 +++++++++++++++++++++++++
>   src/remote/remote_daemon.c              |  1 +
>   src/remote/remote_driver.c              |  8 +++
>   src/secret/secret_driver.c              |  1 +
>   src/storage/storage_driver.c            |  1 +
>   src/vz/vz_driver.c                      |  1 +
>   22 files changed, 179 insertions(+), 20 deletions(-)
>   create mode 100644 src/qemu/qemu_shim.c
> 

Codewise, we'll need a more clean approach, but since this is just a PoC 
lets not care about that for now.

I like this, I wonder if there are some hidden traps, but if we make 
this a preview only for some time then we should be able to catch them 
soon enough.

IIUC this is orthogonal to splitting libvirtd into multiple daemons, 
i.e. containers that are target of this feature already have way of 
setting up networking so they will not rely on our network driver. 
However, we might need to give users a helping hand in identifying what 
domain XMLs will call other drivers (daemons). I mean, I've just tried 
to start a domain I have with this shim you added in 5/5 and found that 
it tries to conenct to storage driver and network driver (yes, I have a 
disk type='pool' and interface type='network').

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 17, 2019 at 13:24:52 +0100, Daniel Berrange wrote:
> This patch series proposes a new way to build a "libvirt QEMU shim"
> which supports more use cases than the previous approach from last year,
> as well as being vastly simpler to implement.

Few thoughts:

I like this approach in general. We were bound to lose the full host
view of VMs started by libvirt shim sooner or later anyways as people
would want to stuff the libvirtd into a container.

Few things we should consider prior to making this a feature:

1) We should introduce a hard limit that the 'root' directory must not
be created by a library version greater than we are running right when
connecting. This is to prevent downgrades and thus all the problems with
them right upfront.

2) The library needs to make sure that the event loop is registered in
this case and ideally also make sure that it has run so that we avoid
problems with stuck instances.

3) The capability caching is not leveraged when creating a new instance
of the 'root'.

4) Internally we'll need to relax few checks in the migration region as
igrating to a different "root" will certainly make sense now.

5) It would be cool to have a way to virConnectDomainEventRegisterAny
prior to actually opening the connection. This would allow e.g. get the
events for block jobs which terminated while the app was away for some
reason so that it does not have to poll.

6) We need to write a doc setting the expectations how to use this in
the APP. (Clarify threading and that users can't avoid domain jobs etc.
[1])

7) We should get of rid of the 'autostart' feature for 'embed' type
connections.

8) Some kind of label depending on 'root' should be used in logs as
things may log into the same place and that would be a pain to debug.

> Specifically
> 
>  - We can use this to embed the QEMU driver in unit tests allowing
>    the full range of odriver functionality to be exercised during
>    *unit* testing without interfering with the host OS libvirtd.

This implies ... [2]

[...]

>  - Using a virtual root directory for QEMU driver state meams we do
>    not need to solve the hard refactoring problem of getting the main
>    libvirtd to detect the VMs launched via the shim. They can safely
>    live independantly.

[1] Or that removing the directory may e.g. get rid of some state which
may be needed like snapshot metadata etc.

>  - Thanks to the driver refactoring work it is still possible for the
>    embedded drivers to talk to main libvirtd to use the secondary
>    drivers for storage/network/etc if needed. Ideally we would allow
>    those secondary drivers to be embedded too but that's not implemented
>    here. Most important would be the secrets driver. Other drivers are
>    much less important.

[1] ... that this is ideally supported as well. It'll be surprising if
your tests spawn VMs which will infiltrate your 'default' network for
example. We might want to also have a way how to specify this during
connection.

> 
> Daniel P. Berrangé (5):
>   access: report an error if no access manager is present
>   libvirt: pass a root directory path into drivers
>   qemu: honour the root parameter during driver initialization
>   libvirt: support an "embed" URI path selector for QEMU driver
>   qemu: introduce a new style libvirt_qemu_shim program

Also this new program since it's advertised as 'copy this into your
code' should avoid using virAsprintf for constructing the URI.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 17, 2019 at 15:22:12 +0200, Peter Krempa wrote:
> On Fri, May 17, 2019 at 13:24:52 +0100, Daniel Berrange wrote:
> > This patch series proposes a new way to build a "libvirt QEMU shim"
> > which supports more use cases than the previous approach from last year,
> > as well as being vastly simpler to implement.
> 
> Few thoughts:

two more:

[...]

9) Users may have different ideas regarding values in qemu.conf
(such as default_tls_x509_cert_dir) thus we probably need a way to
provide that one as well separately.

10) users almost certainly will want to use a separate instance of the
secret driver as there's no other way to define secrets for disks and
sharing the instance seems wrong
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 17, 2019 at 06:45:08PM +0200, Peter Krempa wrote:
> On Fri, May 17, 2019 at 15:22:12 +0200, Peter Krempa wrote:
> > On Fri, May 17, 2019 at 13:24:52 +0100, Daniel Berrange wrote:
> > > This patch series proposes a new way to build a "libvirt QEMU shim"
> > > which supports more use cases than the previous approach from last year,
> > > as well as being vastly simpler to implement.
> > 
> > Few thoughts:
> 
> two more:
> 
> [...]
> 
> 9) Users may have different ideas regarding values in qemu.conf
> (such as default_tls_x509_cert_dir) thus we probably need a way to
> provide that one as well separately.

I believe my patch should already deal with that as I prefix all the
dirs that the QEMU driver knows about.

> 10) users almost certainly will want to use a separate instance of the
> secret driver as there's no other way to define secrets for disks and
> sharing the instance seems wrong

Yep.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, May 17, 2019 at 17:49:31 +0100, Daniel Berrange wrote:
> On Fri, May 17, 2019 at 06:45:08PM +0200, Peter Krempa wrote:
> > On Fri, May 17, 2019 at 15:22:12 +0200, Peter Krempa wrote:
> > > On Fri, May 17, 2019 at 13:24:52 +0100, Daniel Berrange wrote:
> > > > This patch series proposes a new way to build a "libvirt QEMU shim"
> > > > which supports more use cases than the previous approach from last year,
> > > > as well as being vastly simpler to implement.
> > > 
> > > Few thoughts:
> > 
> > two more:
> > 
> > [...]
> > 
> > 9) Users may have different ideas regarding values in qemu.conf
> > (such as default_tls_x509_cert_dir) thus we probably need a way to
> > provide that one as well separately.
> 
> I believe my patch should already deal with that as I prefix all the
> dirs that the QEMU driver knows about.

Hmm, I'm not sure whether just prefixing every path used internally with
the passed prefix will be a good idea.

Ideally we should keep the directory contents opaque to the user so they
don't ever try to mess with the files in the directory. This
disqualifies the use of this for passing in the qemu config file.

This also reminds me that we need some locking for the directory so that
there aren't two processes openning the same prefix and messing up the
files internally. It will even not work in some cases because the
attempt to reopen the monitor sockets would possibly fail. If not we've
got even more of a problem.

Also this would mean that a prefix of '/' would be equal to the system
instance handled by libvirtd so we need also interlocking with libvirt
if we don't change the layout.

One disadvantage of this idea is that I can see users that will
actually use this to replace libvirt's RPC with something else.
This will be caused by the fact that there can be only one process
handling each prefix as extending qemu driver to allow concurrent access
would be a nightmare.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, May 20, 2019 at 10:53:30AM +0200, Peter Krempa wrote:
> On Fri, May 17, 2019 at 17:49:31 +0100, Daniel Berrange wrote:
> > On Fri, May 17, 2019 at 06:45:08PM +0200, Peter Krempa wrote:
> > > On Fri, May 17, 2019 at 15:22:12 +0200, Peter Krempa wrote:
> > > > On Fri, May 17, 2019 at 13:24:52 +0100, Daniel Berrange wrote:
> > > > > This patch series proposes a new way to build a "libvirt QEMU shim"
> > > > > which supports more use cases than the previous approach from last year,
> > > > > as well as being vastly simpler to implement.
> > > > 
> > > > Few thoughts:
> > > 
> > > two more:
> > > 
> > > [...]
> > > 
> > > 9) Users may have different ideas regarding values in qemu.conf
> > > (such as default_tls_x509_cert_dir) thus we probably need a way to
> > > provide that one as well separately.
> > 
> > I believe my patch should already deal with that as I prefix all the
> > dirs that the QEMU driver knows about.
> 
> Hmm, I'm not sure whether just prefixing every path used internally with
> the passed prefix will be a good idea.
> 
> Ideally we should keep the directory contents opaque to the user so they
> don't ever try to mess with the files in the directory. This
> disqualifies the use of this for passing in the qemu config file.

I don't think the sub-dir is really much different from the normal
libvirt locations in that respect. We already recommend apps/admins
to not touch /etc/libvirt/qemu files, nor the /run/libvirt/qemu
files, nor the /var/lib/libvirt/qemu files. At the same time though
there are some files which are reasonable for apps/admins to look
at. UNIX domain sockets for virtio-serial devices in the local
state sub-dir. Per VM log files, though we really should create a
virStream API for accessing logs, and so on.

We don't really document this very well even today. If we improve
our docs in this area, I don't think apps need to treat the virtial
prefix dirs any different from how we tell apps to treat libvirt's
files under /

> This also reminds me that we need some locking for the directory so that
> there aren't two processes openning the same prefix and messing up the
> files internally. It will even not work in some cases because the
> attempt to reopen the monitor sockets would possibly fail. If not we've
> got even more of a problem.

QEMU should reject multiple attempts to open monitor socket, or rather
the second attempt will just never succeeed/hang, since QEMU won't
call accept() until the first connection goes away. This 1-1 model
is an intentional design limitation of chardevs in QEMU.

We should definitely look at doing locking though. Currently we implicitly
have the libvirtd daemon's own pidfile as the mutual exclusion mechanism.

We'll need to have a separate lockfile independantly of that for the
QEMU driver. Probably just $prefix/var/run/libvirt/qemu/driver.lock is
good enough

> Also this would mean that a prefix of '/' would be equal to the system
> instance handled by libvirtd so we need also interlocking with libvirt
> if we don't change the layout.

Yes.

> One disadvantage of this idea is that I can see users that will
> actually use this to replace libvirt's RPC with something else.
> This will be caused by the fact that there can be only one process
> handling each prefix as extending qemu driver to allow concurrent access
> would be a nightmare.

Yes, but in many ways we already have this situation.

libvirt-dbus is providing a new RPC mechanism to talk to libvirt. It happens
to then delegate to libvirt's own RPC, but from an app developer POV that
is invisible.

Large scale mgmt apps like OpenStack/oVirt/KubeVirt have in some sense
replaced libvirt RPC at the cross-host level - they only use libvirt
RPC within scope of a single host, so most of the time there's only a
single client of libvirtd - VDSM, Nova, or Kubevirt's virt handler.

Of course there is a critical distinction. Even if there's normally
only a single client of libvirtd, other clients are not prevented
from running if needed. So you can still run "virsh list" on a host
running openstack/oVirt and get "normal" results. KubeVirt has
changed since they run one-libvirtd instance per VM so  any
aggregated APIs like "virsh list" are largely useless, only showing
1 VM.

The original shim concept was intended to try to fix that behaviour
by allowing the shim to register its existance back with the central
libvirtd. The feedback from kubevirt devs though is that even if we
had that ability in the shim, they'd likely not use it.

If we consider just the single VM oriented APIs though, the inability
to use "virsh" commands with an embedded driver instance could be a
major pain point for debugging runtime problems, depending on the app.

For short lived VMs used for embedded purposes such as with libguestfs
I think the limitation would be tolerable.

For long lived VMs for the inability to use virsh to debug would be
pretty troubling, unless the app embedding the QEMU driver exposed
a comparable set of features for collecting debug info.

You'd also be unable to use QMP directly unless a second monitor was
added to every VM, since QEMU limits you to one active connection
per monitor chardev.

This debuggability issue is probably the biggest downside to this
embedded driver approach.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list