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.

> > Tomas > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1705426 > > >