[libvirt] [PATCH 0/5] Introduce VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT
Problem is, if there is a long standing job on a domain fetching statistics might hiccup which might be a problem for mgmt apps that query stats every few seconds. Orthogonal to this, I think we should break our sync QEMU_JOB_* into two separate jobs: one for qemu monitor the other for qemu guest agent. Problem is, if thread A grabs QEMU_JOB_MODIFY and talks only to guest agent it prevents thread B from grabbing QEMU_JOB_QUERY which wants to talk only to qemu monitor. But this is a different can of worms and I haven't thought it through completely just yet. Michal Privoznik (5): qemu_domain: Document qemuDomainObjBeginJob qemuDomainObjBeginJobInternal: Remove spurious @ret assignment qemu_domain: Introduce qemuDomainObjBeginJobInstant Introcude VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT virsh: Introduce --best-effort to domstats include/libvirt/libvirt-domain.h | 1 + src/libvirt-domain.c | 10 +++++++++ src/qemu/qemu_domain.c | 47 ++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 4 ++++ src/qemu/qemu_driver.c | 15 ++++++++++--- tools/virsh-domain-monitor.c | 7 ++++++ tools/virsh.pod | 16 +++++++++----- 7 files changed, 85 insertions(+), 15 deletions(-) -- 2.16.4
Provide a small comment on the function and its parameters. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c5237e4d41..97149613a2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6334,8 +6334,22 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) /* Give up waiting for mutex after 30 seconds */ #define QEMU_JOB_WAIT_TIME (1000ull * 30) -/* - * obj must be locked before calling +/** + * qemuDomainObjBeginJobInternal: + * @driver: qemu driver + * @obj: domain object + * @job: qemuDomainJob to start + * @asyncJob: qemuDomainAsyncJob to start + * + * Acquires job over domain object which must be locked before + * calling. If there's already a job running waits up to + * QEMU_JOB_WAIT_TIME after which the functions fails reporting + * an error. + * + * Returns: 0 on success, + * -2 if unable to start job because of timeout or + * maxQueuedJobs limit, + * -1 otherwise. */ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, -- 2.16.4
On 06/07/2018 07:59 AM, Michal Privoznik wrote:
Provide a small comment on the function and its parameters.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c5237e4d41..97149613a2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6334,8 +6334,22 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) /* Give up waiting for mutex after 30 seconds */ #define QEMU_JOB_WAIT_TIME (1000ull * 30)
-/* - * obj must be locked before calling +/** + * qemuDomainObjBeginJobInternal: + * @driver: qemu driver + * @obj: domain object + * @job: qemuDomainJob to start + * @asyncJob: qemuDomainAsyncJob to start + * + * Acquires job over domain object which must be locked before
s/over/for a/ ?
+ * calling. If there's already a job running waits up to + * QEMU_JOB_WAIT_TIME after which the functions fails reporting + * an error. + *
Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ * Returns: 0 on success, + * -2 if unable to start job because of timeout or + * maxQueuedJobs limit, + * -1 otherwise. */ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver,
The variable is initialized to -1 already. There's no way it can be overwritten by the time control gets to the line I'm removing. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 97149613a2..5273ab56ac 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6471,7 +6471,6 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, else blocker = priv->job.asyncOwnerAPI; - ret = -1; if (errno == ETIMEDOUT) { if (blocker) { virReportError(VIR_ERR_OPERATION_TIMEOUT, -- 2.16.4
On 06/07/2018 07:59 AM, Michal Privoznik wrote:
The variable is initialized to -1 already. There's no way it can be overwritten by the time control gets to the line I'm removing.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 1 - 1 file changed, 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The aim of this API is to allow caller do best effort. Some functions of ours can work even when acquiring job fails (e.g. qemuConnectGetAllDomainStats()). But what they can't bear is delay if they have to wait up to 30 seconds for each domain. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5273ab56ac..9657573342 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) * @obj: domain object * @job: qemuDomainJob to start * @asyncJob: qemuDomainAsyncJob to start + * @instant: don't wait trying to acquire @job * * Acquires job over domain object which must be locked before * calling. If there's already a job running waits up to * QEMU_JOB_WAIT_TIME after which the functions fails reporting - * an error. + * an error unless @instant is set. + * If @instant is true this function tries to acquire job and if + * it fails returns immediately without waiting. No error is + * reported in this case. * * Returns: 0 on success, * -2 if unable to start job because of timeout or @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, + bool instant) { qemuDomainObjPrivatePtr priv = obj->privateData; unsigned long long now; @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, } while (!nested && !qemuDomainNestedJobAllowed(priv, job)) { + if (instant) + goto cleanup; + VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0) goto error; } while (priv->job.active) { + if (instant) + goto cleanup; + VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) goto error; @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv; if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, - asyncJob) < 0) + asyncJob, false) < 0) return -1; priv = obj->privateData; @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, + false); } +int +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_ASYNC_JOB_NONE, true); +} /* * obj must be locked and have a reference before calling diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f17157b951..4b63c00dff 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob) ATTRIBUTE_RETURN_CHECK; +int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) + ATTRIBUTE_RETURN_CHECK; void qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj); -- 2.16.4
FWIW: While I'm not the best at naming things, it would seem to me that this should be named "Nowait" rather than "Instant". On 06/07/2018 07:59 AM, Michal Privoznik wrote:
The aim of this API is to allow caller do best effort. Some
s/allow caller/allow the caller/ s/do best effort/to avoid waiting for a domain job if the domain is running something else that could take a long time./
functions of ours can work even when acquiring job fails (e.g.
s/of ours// s/job/the job/
qemuConnectGetAllDomainStats()). But what they can't bear is delay if they have to wait up to 30 seconds for each domain.
s/./that is processing some other job./
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5273ab56ac..9657573342 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) * @obj: domain object * @job: qemuDomainJob to start * @asyncJob: qemuDomainAsyncJob to start + * @instant: don't wait trying to acquire @job
Prefer "nowait" (or some camelCase version of that)
* * Acquires job over domain object which must be locked before * calling. If there's already a job running waits up to * QEMU_JOB_WAIT_TIME after which the functions fails reporting - * an error. + * an error unless @instant is set. + * If @instant is true this function tries to acquire job and if + * it fails returns immediately without waiting. No error is
s/it fails returns/it fails, then it returns/
+ * reported in this case.
Hmm... no error reported - meaning if ret = -1 and use this flag, it's up to the caller to generate the error... For this series, perhaps fine, but someone else using this, perhaps not.
* * Returns: 0 on success, * -2 if unable to start job because of timeout or @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, + bool instant) { qemuDomainObjPrivatePtr priv = obj->privateData; unsigned long long now; @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, }
while (!nested && !qemuDomainNestedJobAllowed(priv, job)) { + if (instant) + goto cleanup; +
If async is not supported for this (as I believe seen in the new API), then is this path possible?
VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0) goto error; }
while (priv->job.active) { + if (instant) + goto cleanup; + VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) goto error; @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv;
if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, - asyncJob) < 0) + asyncJob, false) < 0) return -1;
priv = obj->privateData; @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, + false); }
+int +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_ASYNC_JOB_NONE, true);
^^^^^^^^^^^^^^^^^^^^ Doesn't this mean async jobs are not supported. John
+}
/* * obj must be locked and have a reference before calling diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f17157b951..4b63c00dff 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -512,6 +512,10 @@ int qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainAsyncJob asyncJob) ATTRIBUTE_RETURN_CHECK; +int qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) + ATTRIBUTE_RETURN_CHECK;
void qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj);
On 06/13/2018 05:28 PM, John Ferlan wrote:
FWIW: While I'm not the best at naming things, it would seem to me that this should be named "Nowait" rather than "Instant".
On 06/07/2018 07:59 AM, Michal Privoznik wrote:
The aim of this API is to allow caller do best effort. Some
s/allow caller/allow the caller/
s/do best effort/to avoid waiting for a domain job if the domain is running something else that could take a long time./
functions of ours can work even when acquiring job fails (e.g.
s/of ours//
s/job/the job/
qemuConnectGetAllDomainStats()). But what they can't bear is delay if they have to wait up to 30 seconds for each domain.
s/./that is processing some other job./
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_domain.c | 30 +++++++++++++++++++++++++----- src/qemu/qemu_domain.h | 4 ++++ 2 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5273ab56ac..9657573342 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -6340,11 +6340,15 @@ qemuDomainJobAllowed(qemuDomainObjPrivatePtr priv, qemuDomainJob job) * @obj: domain object * @job: qemuDomainJob to start * @asyncJob: qemuDomainAsyncJob to start + * @instant: don't wait trying to acquire @job
Prefer "nowait" (or some camelCase version of that)
Okay. Naming is hard :-P
* * Acquires job over domain object which must be locked before * calling. If there's already a job running waits up to * QEMU_JOB_WAIT_TIME after which the functions fails reporting - * an error. + * an error unless @instant is set. + * If @instant is true this function tries to acquire job and if + * it fails returns immediately without waiting. No error is
s/it fails returns/it fails, then it returns/
+ * reported in this case.
Hmm... no error reported - meaning if ret = -1 and use this flag, it's up to the caller to generate the error... For this series, perhaps fine, but someone else using this, perhaps not.
* * Returns: 0 on success, * -2 if unable to start job because of timeout or @@ -6355,7 +6359,8 @@ static int ATTRIBUTE_NONNULL(1) qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, virDomainObjPtr obj, qemuDomainJob job, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, + bool instant) { qemuDomainObjPrivatePtr priv = obj->privateData; unsigned long long now; @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, }
while (!nested && !qemuDomainNestedJobAllowed(priv, job)) { + if (instant) + goto cleanup; +
If async is not supported for this (as I believe seen in the new API), then is this path possible?
Of course. For instance, if there's an async job running and another thread is trying to start a sync, non-nested job that is not allowed for the async job. Say, thread A is doing a snapshot which sets job mask (=allowed sync jobs) to: query, destroy, abort, suspend, migration-op. Then there's a thread B which is executing qemuDomainSetUserPassword(). This threads tries to grab modify job. However, since grabbing modify is not allowed (due to job mask) thread B sits and waits until thread A releases the async job. At that point, thread B can safely grab modify job because there's no other (nor async) job running. Long story short, not only synchronous jobs serialize, also async (and any combination of those two) do.
VIR_DEBUG("Waiting for async job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.asyncCond, &obj->parent.lock, then) < 0) goto error; }
while (priv->job.active) { + if (instant) + goto cleanup; + VIR_DEBUG("Waiting for job (vm=%p name=%s)", obj, obj->def->name); if (virCondWaitUntil(&priv->job.cond, &obj->parent.lock, then) < 0) goto error; @@ -6517,7 +6528,7 @@ int qemuDomainObjBeginJob(virQEMUDriverPtr driver, qemuDomainJob job) { if (qemuDomainObjBeginJobInternal(driver, obj, job, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_NONE, false) < 0) return -1; else return 0; @@ -6532,7 +6543,7 @@ int qemuDomainObjBeginAsyncJob(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv;
if (qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC, - asyncJob) < 0) + asyncJob, false) < 0) return -1;
priv = obj->privateData; @@ -6561,9 +6572,18 @@ qemuDomainObjBeginNestedJob(virQEMUDriverPtr driver,
return qemuDomainObjBeginJobInternal(driver, obj, QEMU_JOB_ASYNC_NESTED, - QEMU_ASYNC_JOB_NONE); + QEMU_ASYNC_JOB_NONE, + false); }
+int +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_ASYNC_JOB_NONE, true);
^^^^^^^^^^^^^^^^^^^^ Doesn't this mean async jobs are not supported.
I'm not quite sure what do you mean. You mean whether grabbing a sync job while there's async job already running? This is supported. The fact that asyncJob is QEMU_ASYNC_JOB_NONE does not mean "set sync job to @job and async job to QEMU_ASYNC_JOB_NONE". In fact, qemuDomainObjBeginJobInternal() is capable of grabbing either sync or async job but not both at the same time. When grabbing a sync job, @asynJob == QEMU_ASYNC_JOB_NONE and when grabbing an async job, @job == QEMU_JOB_ASYNC. Anyway, the point of BeginJobInstant() is to be drop-in replacement for plain BeginJob(), so whatever the latter passes to BeginJobInternal() the former should mimic it. But then again, I'm not quite sure I understand what you mean. Michal
[...]
qemuDomainObjPrivatePtr priv = obj->privateData; unsigned long long now; @@ -6395,12 +6400,18 @@ qemuDomainObjBeginJobInternal(virQEMUDriverPtr driver, }
while (!nested && !qemuDomainNestedJobAllowed(priv, job)) { + if (instant) + goto cleanup; +
If async is not supported for this (as I believe seen in the new API), then is this path possible?
Of course. For instance, if there's an async job running and another thread is trying to start a sync, non-nested job that is not allowed for the async job. Say, thread A is doing a snapshot which sets job mask (=allowed sync jobs) to: query, destroy, abort, suspend, migration-op. Then there's a thread B which is executing qemuDomainSetUserPassword(). This threads tries to grab modify job. However, since grabbing modify is not allowed (due to job mask) thread B sits and waits until thread A releases the async job. At that point, thread B can safely grab modify job because there's no other (nor async) job running.
Long story short, not only synchronous jobs serialize, also async (and any combination of those two) do.
Ah... OK. [...]
+int +qemuDomainObjBeginJobInstant(virQEMUDriverPtr driver, + virDomainObjPtr obj, + qemuDomainJob job) +{ + return qemuDomainObjBeginJobInternal(driver, obj, job, + QEMU_ASYNC_JOB_NONE, true); ^^^^^^^^^^^^^^^^^^^^ Doesn't this mean async jobs are not supported.
I'm not quite sure what do you mean. You mean whether grabbing a sync job while there's async job already running? This is supported. The fact that asyncJob is QEMU_ASYNC_JOB_NONE does not mean "set sync job to @job and async job to QEMU_ASYNC_JOB_NONE". In fact, qemuDomainObjBeginJobInternal() is capable of grabbing either sync or async job but not both at the same time. When grabbing a sync job, @asynJob == QEMU_ASYNC_JOB_NONE and when grabbing an async job, @job == QEMU_JOB_ASYNC.
Anyway, the point of BeginJobInstant() is to be drop-in replacement for plain BeginJob(), so whatever the latter passes to BeginJobInternal() the former should mimic it. But then again, I'm not quite sure I understand what you mean.
I should have used the [1] or something to signify that this particular comment was related to the one above where I was asking about the need for the bool/instant check for async jobs, but I think you figured that out even though you left a small window of self doubt ;-) John
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
$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)
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
[...]
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
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
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 :|
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
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 :|
This new switch can be used to set VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT flag for stats fetching API. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 7 +++++++ tools/virsh.pod | 16 +++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8cbb3db37c..96a65a8ac7 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1992,6 +1992,10 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("add backing chain information to block stats"), }, + {.name = "best-effort", + .type = VSH_OT_BOOL, + .help = N_("ignore stalled domains"), + }, VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(N_("list of domains to get stats for"), 0), {.name = NULL} }; @@ -2087,6 +2091,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "backing")) flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING; + if (vshCommandOptBool(cmd, "best-effort")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT; + if (vshCommandOptBool(cmd, "domain")) { if (VIR_ALLOC_N(domlist, 1) < 0) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 3f3314a87e..8f3bc67405 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -968,11 +968,11 @@ that require a block device name (such as I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command. -=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>] -[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>] -[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] -[I<--list-transient>] [I<--list-running>] [I<--list-paused>] -[I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] +=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--best-effort>] +[I<--state>] [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] +[I<--block>] [I<--perf>] [[I<--list-active>] [I<--list-inactive>] +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>] +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] Get statistics for multiple or all domains. Without any argument this command prints all available statistics for all domains. @@ -1123,6 +1123,12 @@ daemon supports the selected group of stats. Flag I<--enforce> forces the command to fail if the daemon doesn't support the selected group. +When collecting stats libvirtd may wait for some time if there's +already another job running on given domain for it to finish. +This may cause unnecessary delay in delivering stats. Using +I<--best-effort> suppresses this behaviour. On the other hand +some statistics might be missing for such domain. + =item B<domiflist> I<domain> [I<--inactive>] Print a table showing the brief information of all virtual interfaces -- 2.16.4
On 06/07/2018 07:59 AM, Michal Privoznik wrote:
This new switch can be used to set VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT flag for stats fetching API.
I'll reiterate my preference for nowait
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh-domain-monitor.c | 7 +++++++ tools/virsh.pod | 16 +++++++++++----- 2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 8cbb3db37c..96a65a8ac7 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1992,6 +1992,10 @@ static const vshCmdOptDef opts_domstats[] = { .type = VSH_OT_BOOL, .help = N_("add backing chain information to block stats"), }, + {.name = "best-effort", + .type = VSH_OT_BOOL, + .help = N_("ignore stalled domains"), + }, VIRSH_COMMON_OPT_DOMAIN_OT_ARGV(N_("list of domains to get stats for"), 0), {.name = NULL} }; @@ -2087,6 +2091,9 @@ cmdDomstats(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "backing")) flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BACKING;
+ if (vshCommandOptBool(cmd, "best-effort")) + flags |= VIR_CONNECT_GET_ALL_DOMAINS_STATS_BEST_EFFORT; + if (vshCommandOptBool(cmd, "domain")) { if (VIR_ALLOC_N(domlist, 1) < 0) goto cleanup; diff --git a/tools/virsh.pod b/tools/virsh.pod index 3f3314a87e..8f3bc67405 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -968,11 +968,11 @@ that require a block device name (such as I<domblkinfo> or I<snapshot-create> for disk snapshots) will accept either target or unique source names printed by this command.
-=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--state>] -[I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] [I<--block>] -[I<--perf>] [[I<--list-active>] [I<--list-inactive>] [I<--list-persistent>] -[I<--list-transient>] [I<--list-running>] [I<--list-paused>] -[I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...] +=item B<domstats> [I<--raw>] [I<--enforce>] [I<--backing>] [I<--best-effort>] +[I<--state>] [I<--cpu-total>] [I<--balloon>] [I<--vcpu>] [I<--interface>] +[I<--block>] [I<--perf>] [[I<--list-active>] [I<--list-inactive>] +[I<--list-persistent>] [I<--list-transient>] [I<--list-running>] +[I<--list-paused>] [I<--list-shutoff>] [I<--list-other>]] | [I<domain> ...]
Get statistics for multiple or all domains. Without any argument this command prints all available statistics for all domains. @@ -1123,6 +1123,12 @@ daemon supports the selected group of stats. Flag I<--enforce> forces the command to fail if the daemon doesn't support the selected group.
+When collecting stats libvirtd may wait for some time if there's +already another job running on given domain for it to finish. +This may cause unnecessary delay in delivering stats. Using +I<--best-effort> suppresses this behaviour. On the other hand +some statistics might be missing for such domain. +
By supplying the --nowait qualifier, this tells libvirt to avoid collecting detailed statistics from domains that are actively running some other job. In this case, it is possible that the returned statistics for those domains only includes a minimal set of statistics that does not require querying the domain hypervisor. John
=item B<domiflist> I<domain> [I<--inactive>]
Print a table showing the brief information of all virtual interfaces
On Thu, Jun 07, 2018 at 01:59:11PM +0200, Michal Privoznik wrote:
Problem is, if there is a long standing job on a domain fetching statistics might hiccup which might be a problem for mgmt apps that query stats every few seconds.
Orthogonal to this, I think we should break our sync QEMU_JOB_* into two separate jobs: one for qemu monitor the other for qemu guest agent. Problem is, if thread A grabs QEMU_JOB_MODIFY and talks only to guest agent it prevents thread B from grabbing QEMU_JOB_QUERY which wants to talk only to qemu monitor. But this is a different can of worms and I haven't thought it through completely just yet.
Yeah, I think that is important todo because the guest agent is untrusted service and so can arbitrarily delay its response to libvirt. Even though we timeout the agent after a while, we shouldn't really let it delay other APIs using the monitor. 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 :|
On 06/07/2018 02:53 PM, Daniel P. Berrangé wrote:
On Thu, Jun 07, 2018 at 01:59:11PM +0200, Michal Privoznik wrote:
Problem is, if there is a long standing job on a domain fetching statistics might hiccup which might be a problem for mgmt apps that query stats every few seconds.
Orthogonal to this, I think we should break our sync QEMU_JOB_* into two separate jobs: one for qemu monitor the other for qemu guest agent. Problem is, if thread A grabs QEMU_JOB_MODIFY and talks only to guest agent it prevents thread B from grabbing QEMU_JOB_QUERY which wants to talk only to qemu monitor. But this is a different can of worms and I haven't thought it through completely just yet.
Yeah, I think that is important todo because the guest agent is untrusted service and so can arbitrarily delay its response to libvirt. Even though we timeout the agent after a while, we shouldn't really let it delay other APIs using the monitor.
Indeed. I'm working on it as we speak. Nevertheless, these patches are independent of that (in case any reviewer is waiting for those patches first). Michal
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Michal Privoznik