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
to interactive applications that fetch stats repeatedly every few
seconds.
Solution is to introduce
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@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 */
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.
+ *
* 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.
+ *
* 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,
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;
+ if (HAVE_JOB(privflags)) {
+ int rv;
+
+ if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT)
+ rv = qemuDomainObjBeginJobInstant(driver, vm, QEMU_JOB_QUERY);
+ else
+ rv = qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY);
+
+ if (rv == 0)
+ domflags |= QEMU_DOMAIN_STATS_HAVE_JOB;
+ }
/* else: without a job it's still possible to gather some data */
if (flags & VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING)
--
2.16.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
$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@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)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/13/2018 05:34 PM, John Ferlan wrote:
>
> $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@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"
Well, my hidden idea was also that we can "misuse" this flag to not wait
on other places too. For instance, if we find out (somehow) that a
domain is in D state, we would consider it stale even without any job
running on it. Okay, we have no way of detecting if qemu is in D state
right now, but you get my point. If we don't lock this flag down to just
domain jobs (that not all drivers have btw), we can use it more widely.
>
>> 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...
Not really, this is jut git generating misleading diff (for human). In
fact this flag is added to virConnectGetAllDomainStats() and
virDomainListGetStats().
>
>> * 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.
OKay.
>
>> * 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?
Sure. Because what virDomainListGetStats does is it calls
driver->connectGetAllDomainStats() just like
virConnectGetAllDomainStats() does. So at the driver side there's only
one function serving two public APIs.
>
>> 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.
Not sure what do you mean. Any StatsWorker callback (from
qemuDomainGetStats()) that needs to talk to monitor checks if grabbing
job succeeded or not. If it didn't the callback returns immediately. You
can see this live - just put sleep(60) right at the beginning of
qemuAgentGetTime(), and then from one console run 'virsh domtime $dom'
and from another one 'virsh domstats'.
> 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...
unfinished sen... :-)
>
>> + 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...
Probably yeah. But that can be done outside of this patch set.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[...]
>>> 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"
>
> Well, my hidden idea was also that we can "misuse" this flag to not wait
> on other places too. For instance, if we find out (somehow) that a
> domain is in D state, we would consider it stale even without any job
> running on it. Okay, we have no way of detecting if qemu is in D state
> right now, but you get my point. If we don't lock this flag down to just
> domain jobs (that not all drivers have btw), we can use it more widely.
>
Ahhh, I see what you're driving at - some flag name which allows us to
reuse the name for other conditions which have caused delays in
collection due to some underlying condition...
a/k/a: {AVOID|IGNORE}_BLOCKING_CONDITIONS...
>>
>>> 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 */
Would the new flag be mutually exclusive to ENFORCE? I want some list
of stats, but don't wait to get the answer.
>>> } 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...
>
> Not really, this is jut git generating misleading diff (for human). In
> fact this flag is added to virConnectGetAllDomainStats() and
> virDomainListGetStats().
>
>>
>>> * 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.
>
> OKay.
>
>>
>>> * 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?
>
> Sure. Because what virDomainListGetStats does is it calls
> driver->connectGetAllDomainStats() just like
> virConnectGetAllDomainStats() does. So at the driver side there's only
> one function serving two public APIs.
>
The ... are all related - you've answered the concern above. I was too
lazy to look at the actual placement - just took the git diff output and
assumed Capabilities.
>>
>>> 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.
>
> Not sure what do you mean. Any StatsWorker callback (from
> qemuDomainGetStats()) that needs to talk to monitor checks if grabbing
> job succeeded or not. If it didn't the callback returns immediately. You
> can see this live - just put sleep(60) right at the beginning of
> qemuAgentGetTime(), and then from one console run 'virsh domtime $dom'
> and from another one 'virsh domstats'.
>
>> 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...
>
> unfinished sen... :-)
>
Darn those interruptions - I think in the end this was all related to
the clearing of the error message. I know I was concerned that for this
avoid wait condition that we wouldn't message. For this case, it's
handled by ignoring it, but perhaps some other future consumer would
need to know it has to message that the call failed. It would then need
to check if the last message was NULL, then generate a message.
Of course then I got into the - hey wait, we don't clear the last error
in the event that we're ignoring it and naturally neglected to go back
to my first train of thought to complete my "tence" (;-)).
John
>>
>>> + 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...
>
> Probably yeah. But that can be done outside of this patch set.
>
> Michal
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/14/2018 04:10 PM, John Ferlan wrote:
>
> [...]
>
>>>> 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"
>>
>> Well, my hidden idea was also that we can "misuse" this flag to not wait
>> on other places too. For instance, if we find out (somehow) that a
>> domain is in D state, we would consider it stale even without any job
>> running on it. Okay, we have no way of detecting if qemu is in D state
>> right now, but you get my point. If we don't lock this flag down to just
>> domain jobs (that not all drivers have btw), we can use it more widely.
>>
>
> Ahhh, I see what you're driving at - some flag name which allows us to
> reuse the name for other conditions which have caused delays in
> collection due to some underlying condition...
>
> a/k/a: {AVOID|IGNORE}_BLOCKING_CONDITIONS...
>
>>>
>>>> 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 */
>
> Would the new flag be mutually exclusive to ENFORCE? I want some list
> of stats, but don't wait to get the answer.
I thought about this and I don't think so. So current behaviour is: with
enforce you still might miss some stats if acquiring job fails. ENFORCE
merely tells libvirt to check if requested stats are available (=driver
knows how to fetch requested stats).
Therefore I don't think these two flags are mutually exclusive.
>
>>>> } 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...
>>
>> Not really, this is jut git generating misleading diff (for human). In
>> fact this flag is added to virConnectGetAllDomainStats() and
>> virDomainListGetStats().
>>
>>>
>>>> * 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.
>>
>> OKay.
>>
>>>
>>>> * 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?
>>
>> Sure. Because what virDomainListGetStats does is it calls
>> driver->connectGetAllDomainStats() just like
>> virConnectGetAllDomainStats() does. So at the driver side there's only
>> one function serving two public APIs.
>>
>
> The ... are all related - you've answered the concern above. I was too
> lazy to look at the actual placement - just took the git diff output and
> assumed Capabilities.
>
>>>
>>>> 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.
>>
>> Not sure what do you mean. Any StatsWorker callback (from
>> qemuDomainGetStats()) that needs to talk to monitor checks if grabbing
>> job succeeded or not. If it didn't the callback returns immediately. You
>> can see this live - just put sleep(60) right at the beginning of
>> qemuAgentGetTime(), and then from one console run 'virsh domtime $dom'
>> and from another one 'virsh domstats'.
>>
>>> 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...
>>
>> unfinished sen... :-)
>>
>
> Darn those interruptions - I think in the end this was all related to
> the clearing of the error message. I know I was concerned that for this
> avoid wait condition that we wouldn't message. For this case, it's
> handled by ignoring it, but perhaps some other future consumer would
> need to know it has to message that the call failed. It would then need
> to check if the last message was NULL, then generate a message.
Yup. But again, this is pre-existing and deserves a separate patch. I
can send it and (to motivate others to merge these) - I will do that
once these are in as a follow up ;-)
>
> Of course then I got into the - hey wait, we don't clear the last error
> in the event that we're ignoring it and naturally neglected to go back
> to my first train of thought to complete my "tence" (;-)).
:-D
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Jun 14, 2018 at 11:07:43AM +0200, Michal Privoznik wrote:
> On 06/13/2018 05:34 PM, John Ferlan wrote:
> >
> > $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@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"
>
> Well, my hidden idea was also that we can "misuse" this flag to not wait
> on other places too. For instance, if we find out (somehow) that a
> domain is in D state, we would consider it stale even without any job
> running on it. Okay, we have no way of detecting if qemu is in D state
> right now, but you get my point. If we don't lock this flag down to just
> domain jobs (that not all drivers have btw), we can use it more widely.
I would suggest we call it "NOWAIT" with explanation that we will only
report statistics that can be obtained immediately without any blocking,
whatever may be the cause.
On a tangent, I think this problem really calls for a significantly
different design approach, medium term.
The bulk stats query APIs were a good step forward on what we had
before where users must call many libvirt APIs, but it is still
not very scalable. With huge numbers of guests, we're still having
to serialize stats query calls into 1000's of QEMU processes.
I think we must work with QEMU to define a better interface, taking
advantage of fact we're colocated on the same host. ie we tell QEMU
we we want stats exported in memory page <blah>, and QEMU will keep
that updated at all times.
When libvirt needs the info it can then just read it straight out of
the shared memory page, no blocking on any jobs, no QMP serialization,
etc.
For that matter, we can do a similar thing in libvirt API too. We can
export a shared memory region for applications to use, which we keep
updated on some regular interval that app requests. They can then always
access updated stats without calling libvirt APIs at all.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 06/14/2018 05:35 PM, Daniel P. Berrangé wrote:
> On Thu, Jun 14, 2018 at 11:07:43AM +0200, Michal Privoznik wrote:
>> On 06/13/2018 05:34 PM, John Ferlan wrote:
>>>
>>> $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@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"
>>
>> Well, my hidden idea was also that we can "misuse" this flag to not wait
>> on other places too. For instance, if we find out (somehow) that a
>> domain is in D state, we would consider it stale even without any job
>> running on it. Okay, we have no way of detecting if qemu is in D state
>> right now, but you get my point. If we don't lock this flag down to just
>> domain jobs (that not all drivers have btw), we can use it more widely.
>
> I would suggest we call it "NOWAIT" with explanation that we will only
> report statistics that can be obtained immediately without any blocking,
> whatever may be the cause.
Okay, works for me. I'll post v2 shortly.
>
>
> On a tangent, I think this problem really calls for a significantly
> different design approach, medium term.
>
> The bulk stats query APIs were a good step forward on what we had
> before where users must call many libvirt APIs, but it is still
> not very scalable. With huge numbers of guests, we're still having
> to serialize stats query calls into 1000's of QEMU processes.
>
> I think we must work with QEMU to define a better interface, taking
> advantage of fact we're colocated on the same host. ie we tell QEMU
> we we want stats exported in memory page <blah>, and QEMU will keep
> that updated at all times.
>
> When libvirt needs the info it can then just read it straight out of
> the shared memory page, no blocking on any jobs, no QMP serialization,
> etc.
>
> For that matter, we can do a similar thing in libvirt API too. We can
> export a shared memory region for applications to use, which we keep
> updated on some regular interval that app requests. They can then always
> access updated stats without calling libvirt APIs at all.
This is clever idea. But qemu is not the only source of stats we gather.
We also fetch some data from CGroups, /proc, ovs bridge, etc. So libvirt
would need to add its own stats for client to see. This means there will
be a function that updates the shared mem every so often (as client
tells us via some new API?). The same goes for qemu impl. Now imagine
two clients wanting two different refresh rates with GCD 1 :-)
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 15, 2018 at 09:19:44AM +0200, Michal Privoznik wrote:
> On 06/14/2018 05:35 PM, Daniel P. Berrangé wrote:
> > On Thu, Jun 14, 2018 at 11:07:43AM +0200, Michal Privoznik wrote:
> >> On 06/13/2018 05:34 PM, John Ferlan wrote:
> >>>
> >>> $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@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"
> >>
> >> Well, my hidden idea was also that we can "misuse" this flag to not wait
> >> on other places too. For instance, if we find out (somehow) that a
> >> domain is in D state, we would consider it stale even without any job
> >> running on it. Okay, we have no way of detecting if qemu is in D state
> >> right now, but you get my point. If we don't lock this flag down to just
> >> domain jobs (that not all drivers have btw), we can use it more widely.
> >
> > I would suggest we call it "NOWAIT" with explanation that we will only
> > report statistics that can be obtained immediately without any blocking,
> > whatever may be the cause.
>
> Okay, works for me. I'll post v2 shortly.
>
> >
> >
> > On a tangent, I think this problem really calls for a significantly
> > different design approach, medium term.
> >
> > The bulk stats query APIs were a good step forward on what we had
> > before where users must call many libvirt APIs, but it is still
> > not very scalable. With huge numbers of guests, we're still having
> > to serialize stats query calls into 1000's of QEMU processes.
> >
> > I think we must work with QEMU to define a better interface, taking
> > advantage of fact we're colocated on the same host. ie we tell QEMU
> > we we want stats exported in memory page <blah>, and QEMU will keep
> > that updated at all times.
> >
> > When libvirt needs the info it can then just read it straight out of
> > the shared memory page, no blocking on any jobs, no QMP serialization,
> > etc.
> >
> > For that matter, we can do a similar thing in libvirt API too. We can
> > export a shared memory region for applications to use, which we keep
> > updated on some regular interval that app requests. They can then always
> > access updated stats without calling libvirt APIs at all.
>
> This is clever idea. But qemu is not the only source of stats we gather.
> We also fetch some data from CGroups, /proc, ovs bridge, etc. So libvirt
> would need to add its own stats for client to see. This means there will
> be a function that updates the shared mem every so often (as client
> tells us via some new API?). The same goes for qemu impl. Now imagine
> two clients wanting two different refresh rates with GCD 1 :-)
Yes, the shared mem areas used by qemu would have to be separate from
the one exposed by libvirt. Each QEMU would expose a separate area to
libvirt, and libvirt would aggregate stats from every QEMU, and also
the other places like cgroups, and then put them into its own single
shared mem area.
I would imagine we could be restrictive with refresh rate, eg require
5 seconds, or a multiple of 5 seconds, so we can easily reconcile
multiple client rates.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.