Re: [libvirt PATCH 00/16] Add support for persistent mediated devices
On Fri, 2020-07-17 at 10:08 +0200, Erik Skultety wrote:
On Thu, Jul 16, 2020 at 05:21:30PM -0500, Jonathon Jongsma wrote:
> This patch series follows the previously-merged series which added
> support for
> transient mediated devices. This series expands mdev support to
> include
> persistent device definitions. Again, it relies on mdevctl as the
> backend.
>
> It follows the common libvirt pattern of APIs by adding the
> following new APIs
> for node devices:
> - virNodeDeviceDefineXML() - defines a persistent device
> - virNodeDeviceUndefine() - undefines a persistent device
> - virNodeDeviceCreate() - starts a previously-defined device
>
> It also adds virsh commands mapping to these new APIs: nodedev-
> define,
> nodedev-undefine, and nodedev-start.
>
> The method of staying up-to-date with devices defined by mdevctl is
> currently a
> little bit crude due to the fact that mdevctl does not emit any
> events when new
> devices are added or removed. As a workaround, we create a file
> monitor for the
> mdevctl config directory and re-query mdevctl when we detect
> changes within
> that directory. In the future, mdevctl may introduce a more elegant
> solution.
In general, I don't think it is desirable for libvirt to monitor the
configuration directory for changes made outside of libvirt and
expect libvirt
to keep up with the new configuration and still be able manage the
device.
I'd say that we only manage devices that we created and have control
over.
I'm not sure that it's entirely accurate to say that we have "control
over" these devices. If we're using mdevctl as our configuration
backend (which I think is the right decision), then any device that is
defined in mdevctl can also be changed or removed by a user executing
mdevctl externally. I'm not sure there's really anything we can do to
prevent that unless we decide to abandon mdevctl as a backend.
Everything else is outside libvirt's scope to handle (the same
goes
for
persistent device removals).
If any externally-created mediated devices are active (not just
defined, but actually started) libvirt will already see them because
they will be visible via udev. So trying to limit the list of mdevs to
only those created by libvirt might quickly become a bit complicated, I
think.
(Note also that the previously-merged first patch series already allows
libvirt to e.g. stop devices that are started externally by mdevctl. If
we really wanted to maintain a strict separation between mdevs created
by libvirt and those created externally, it may require some more
substantial changes.)
If a device cannot be created, because mdevctl already has a
colliding device
that was created outside of libvirt, we should document that the user
is
expected to remove the device configuration and re-create it with
libvirt -
let's say mdevctl adds support for an alternative config dir, how
does libvirt
know it should monitor that one instead /etc/mdev.d?
Yes, relying on external programs always comes with some risk of
changes. But I envision this as a stopgap measure until mdevctl
introduces a more elegant event notification feature. I think we can
get that feature implemented in mdevctl (and supported in libvirt)
before mdevctl changes its configuration directory location.
All summed up, until there's some event signalling in mdevctl,
we
should only
support devices we previously defined.
So this sounds like you would be ok with supporting externally-defined
devices if there was a more elegant solution to signalling? Earlier it
sounded like you objected to that as out-of-scope.
Another thing to note: the directory monitoring is not only for
monitoring new external changes. Currently the directory monitor also
serves as the mechanism by which we pick up new devices that libvirt
itself defines. If we decide to remove the directory monitoring, we'd
need a different approach for this. That would not be difficult of
course, but it's something to note.
Jonathon