[libvirt] [PATCH 4/5] Introcude VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT

[John Ferlan]

$SUBJ: "Introduce" and "NO_WAIT"


On 06/07/2018 07:59 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1552092
> 
> If there's a long running job it might cause us to wait 30
> seconds before we give up acquiring job. This may cause trouble

s/job/the job/

s/may cause trouble/is problematic/

> to interactive applications that fetch stats repeatedly every few
> seconds.
> 
> Solution is to introduce

The solution is...

> VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT flag which tries to
> acquire job but does not wait if acquiring failed.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/libvirt-domain.c             | 10 ++++++++++
>  src/qemu/qemu_driver.c           | 15 ++++++++++++---
>  3 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index da773b76cb..1a1d34620d 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -2055,6 +2055,7 @@ typedef enum {
>      VIR_CONNECT_GET_ALL_DOMAINS_STATS_SHUTOFF = VIR_CONNECT_LIST_DOMAINS_SHUTOFF,
>      VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER = VIR_CONNECT_LIST_DOMAINS_OTHER,
>  
> +    VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT = 1 << 29, /* ignore stalled domains */

"stalled"?  How about "don't wait on other jobs"

>      VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING = 1 << 30, /* include backing chain for block stats */
>      VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS = 1U << 31, /* enforce requested stats */
>  } virConnectGetAllDomainStatsFlags;
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index d44b553c74..906b097adb 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11502,6 +11502,11 @@ virConnectGetDomainCapabilities(virConnectPtr conn,
>   * fields for offline domains if the statistics are meaningful only for a
>   * running domain.
>   *
> + * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT in
> + * @flags means when libvirt is unable to fetch stats for any of
> + * the domains (for whatever reason) such domain is silently
> + * ignored.
> + *

So we're adding this for both capabilities and...

>   * Similarly to virConnectListAllDomains, @flags can contain various flags to
>   * filter the list of domains to provide stats for.
>   *
> @@ -11586,6 +11591,11 @@ virConnectGetAllDomainStats(virConnectPtr conn,
>   * fields for offline domains if the statistics are meaningful only for a
>   * running domain.
>   *
> + * Passing VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT in
> + * @flags means when libvirt is unable to fetch stats for any of
> + * the domains (for whatever reason) such domain is silently
> + * ignored.
> + *

...stats in the API documentation, but...

BTW: the domain isn't silently ignored, instead only a subset of
statistics is returned for the domain.  That subset being statistics
that don't involve querying the underlying hypervisor.

>   * Note that any of the domain list filtering flags in @flags may be rejected
>   * by this function.
>   *
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 38ea865ce3..28338de05f 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20446,6 +20446,7 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,

...only adding the check in the DomainStats?

>      virCheckFlags(VIR_CONNECT_LIST_DOMAINS_FILTERS_ACTIVE |
>                    VIR_CONNECT_LIST_DOMAINS_FILTERS_PERSISTENT |
>                    VIR_CONNECT_LIST_DOMAINS_FILTERS_STATE |
> +                  VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT |
>                    VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING |
>                    VIR_CONNECT_GET_ALL_DOMAINS_STATS_ENFORCE_STATS, -1);
>  
> @@ -20480,9 +20481,17 @@ qemuConnectGetAllDomainStats(virConnectPtr conn,
>  
>          virObjectLock(vm);
>  
> -        if (HAVE_JOB(privflags) &&
> -            qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) == 0)
> -            domflags |= QEMU_DOMAIN_STATS_HAVE_JOB;

Existing, but if BeginJob fails for some "other" reason, then one
wonders how much of the next steps actually happen.  Furthermore, we
don't clear the LastError.

> +        if (HAVE_JOB(privflags)) {
> +            int rv;
> +
> +            if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT)
> +                rv = qemuDomainObjBeginJobInstant(driver, vm, QEMU_JOB_QUERY);

Let's assume rv = -1 for a moment and it's the last domain that caused
the failure - does that mean the caller then re...

> +            else
> +                rv = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY);
> +
> +            if (rv == 0)
> +                domflags |= QEMU_DOMAIN_STATS_HAVE_JOB;

to add to my comment above, if rv < 0, then should we clear the last
error since this code doesn't seem to care...


John

> +        }
>          /* else: without a job it's still possible to gather some data */
>  
>          if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
>