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

Here's an example of the output of the virsh command added in this patch:

virsh # guestinfo win7
users.count         : 1
users.0.name        : test
users.0.domain      : test-PC
users.0.login_time  : 1564664122706
os.id               : mswindows
os.name             : Microsoft Windows
os.pretty-name      : Windows 7 Professional
os.version          : Microsoft Windows 77
os.version-id       :
os.machine          : x86_64
os.variant          : client
os.variant-id       : client
os.kernel-release   : 7601
os.kernel-version   : 6.1
timezone.name       : Central Daylight Time
timezone.offset     : -18000
hostname            : TEST-PC

Jonathon Jongsma (7):
  lib: add virDomainGetGuestInfo()
  remote: implement virDomainGetGuestInfo
  qemu_agent: add helper for getting guest users
  qemu_agent: add helper function for querying OS info
  qemu_agent: add helper for querying timezone info
  qemu: Implement virDomainGetGuestInfo()
  virsh: add 'guestinfo' command

 include/libvirt/libvirt-domain.h    |  13 +
 src/driver-hypervisor.h             |   8 +
 src/libvirt-domain.c                | 105 ++++++++
 src/libvirt_public.syms             |   1 +
 src/qemu/qemu_agent.c               | 190 +++++++++++++++
 src/qemu/qemu_agent.h               |   4 +
 src/qemu/qemu_driver.c              |  77 ++++++
 src/remote/remote_daemon_dispatch.c |  41 ++++
 src/remote/remote_driver.c          |  53 ++++
 src/remote/remote_protocol.x        |  21 +-
 src/remote_protocol-structs         |  12 +
 tests/qemuagenttest.c               | 366 ++++++++++++++++++++++++++++
 tools/virsh-domain.c                |  53 ++++
 13 files changed, 943 insertions(+), 1 deletion(-)

-- 
2.21.0

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

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

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.

    Tomas

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



-- 
Tomáš Golembiovský <tgolembi@redhat.com>

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

On Wed, Aug 07, 2019 at 12:39:09 +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

I don't want to crush your dreams, but for the VM bulk stats API theres
nothing fancy that libvirt does here. We just fetch a list of all VMs
and then sequentially in a loop fill in all the stats.

> 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

Actually in some cases this might bring better results in cases when one
VM is not responding.

> with its async jobs can handle this in single thread. I am not libvirt

Yes it's a single thread and a loop.

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

Well, we know whether the guest agent channel socket is connected from
guest's side, but we also expose this information in the XML file and
there's also an event which is fired always when the state of the guest
agent changes. This is possible via notifications from qemu via the
virtio channel.

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

This was clarified in a different reply.

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

Libvirt's guest-agent APIs actually have some built-in timeout so that a
rouge agent can't block forever, but this timeout is not really
configurable. A good point would be probably to have an argument for the
API to be able to use an arbitrary timeout for the calls.


In general I'm not really sure that there's benefit in having the API
return the stats for all guest OSes rather than just one. In my design
for the VM stats API I think I went a bit too far in this case and the
benefits of returning stats for all VMs compared to returning all data
for a single VM are worth that. What was worth in that implementation
was to be able to query multiple types of stats at once.

I'm willing to discuss this further though.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

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

On Wed, Aug 07, 2019 at 10:41:10AM -0500, Jonathon Jongsma wrote:
> 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?

What I am saying is that being able to query single VM is nice, but it's
just a start because we need to be able to query all the (running) VMs
at once. And having libvirt API for that would be much more benefitial
(for reasons specified) than querying VMs one-by-one in management apps.
Of course it's pretty fine to have separate (external) API calls for
single VM/list of VMs/all VMs, but how it behaves "under the hood" is
what needs to be considered (is single VM special case of all VMs or is
it the other way around?). 
 

> 
> 
> > > 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 are right, please ignore this comment.

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

OK.

    Tomas

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

-- 
Tomáš Golembiovský <tgolembi@redhat.com>

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