Re: [libvirt] [PATCH 0/7] add virDomainGetGuestInfo()
On Wed, 2019-08-07 at 12:39 +0200, Tomáš Golembiovský wrote:
On Thu, Aug 01, 2019 at 08:37:03AM -0500, Jonathon Jongsma wrote:
> This series adds several bits of guest information provided by a
> new API
> function virDomainGetGuestInfo(). There is an implementation for
> qemu using the
> guest agent. In particular, it adds information about logged-in
> users, guest
> OS, and timezone.
>
> I had previously submitted a patch series with a dedicated API
> function for
> querying guest users that returned an array of structures
> describing the users.
> It was suggested that I convert his to a more futureproof design
> using typed
> parameters and also combine it with other bits of guest information
> similar to
> virDomainListGetStats(). In fact, there were several bugs that
> requested this
> information be added to the 'bulk stats' API, but Peter Krempa
> suggested adding
> a new API for all data queried from the guest agent (see
> https://bugzilla.redhat.com/show_bug.cgi?id=1705514). This is that
> API
> proposal.
The reason we suggested 'bulk stats' approach is so that we can
retrieve
information for all VMs in single call. This is not just about
laziness
on side of management app, it is much easier for libvirt. We can
either
fetch the info for all VMs one-by-one (which can take too long), or
resort to massive threading. On the other hand it seems that libvirt
with its async jobs can handle this in single thread. I am not
libvirt
expert so please correct me if I am making wrong assumptions here.
Also,
libvirt has pretty good knowledge whether the guest agent is running
or
not. From application side we cannot get this information reliably
and
we need to resort to trial-error approach.
It's not entirely clear to me what you are suggesting here. Are you
saying that this API is generally OK, but that you wish it would query
all domains at once? Or are you suggesting some additional changes?
> It follows the stats API quite closely, and tries to return data
in
> similar ways (for example, the "users.N.*" field name scheme was
> inspired by
> various stats fields).
>
> I plan to follow this series up with a patch that adds fsinfo to
> the returned
> information, but wanted to get comments on this approach now.
Apart from the above I have two other concerns.
With how the API call is designed you cannot pick which commands to
run
(you always get them all). With the number of included commands
growing
in the future the time to complete the call will grow as well and
could
just take too long. Also, if you run the call periodically you always
don't want to run all the commands. For example, you don't need to
check
the os-info as often as list of logged in users.
If I understand what you're saying, I think it's incorrect. The API
that I proposed takes a 'types' parameter (similar to the 'stats'
parameter in the bulk stats API) which lets you specify which guest
information types you would like to query. So if you want, you can
query only a subset of the guest info categories.
You cannot set the timeout on the guest agent commands. Instead you
block on them. As described in bug [1], rogue guest can potentially
block your call indefinitely. If you plan to address this problem in
a
more general manner later that's probably fine too.
Yes, I think this issue is probably better addressed separately.