On Wed, May 11, 2022 at 15:04:53 +0200, Peter Krempa wrote:
On Tue, May 10, 2022 at 17:20:38 +0200, Jiri Denemark wrote:
> Jobs that are supposed to remain active even when libvirt daemon
> restarts were reported as started at the time the daemon was restarted.
> This is not very helpful, we should restore the original timestamp.
>
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
> src/qemu/qemu_domainjob.c | 20 +++++++++++++------
> src/qemu/qemu_domainjob.h | 1 +
> src/qemu/qemu_process.c | 4 +++-
> .../migration-in-params-in.xml | 2 +-
> .../migration-out-nbd-bitmaps-in.xml | 2 +-
> .../migration-out-nbd-out.xml | 2 +-
> .../migration-out-nbd-tls-out.xml | 2 +-
> .../migration-out-params-in.xml | 2 +-
> 8 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c
> index 1f82457bd4..8e8d229afe 100644
> --- a/src/qemu/qemu_domainjob.c
> +++ b/src/qemu/qemu_domainjob.c
[...]
> @@ -261,18 +263,15 @@ qemuDomainObjRestoreAsyncJob(virDomainObj *vm,
> {
> qemuDomainObjPrivate *priv = vm->privateData;
> qemuDomainJobObj *job = &priv->job;
> - unsigned long long now;
>
> VIR_DEBUG("Restoring %s async job for domain %s",
> virDomainAsyncJobTypeToString(asyncJob), vm->def->name);
>
> - ignore_value(virTimeMillisNow(&now));
> -
> priv->job.jobsQueued++;
> job->asyncJob = asyncJob;
> job->phase = phase;
> job->asyncOwnerAPI = g_strdup(virThreadJobGet());
> - job->asyncStarted = now;
> + job->asyncStarted = started;
>
> qemuDomainObjSetAsyncJobMask(vm, allowedJobs);
>
> @@ -280,7 +279,7 @@ qemuDomainObjRestoreAsyncJob(virDomainObj *vm,
> qemuDomainJobSetStatsType(priv->job.current, statsType);
> job->current->operation = operation;
> job->current->status = status;
> - job->current->started = now;
> + job->current->started = started;
> }
You don't seem to have any fallback when reading back older status XML
where ...
>
>
> @@ -1250,8 +1249,10 @@ qemuDomainObjPrivateXMLFormatJob(virBuffer *buf,
> priv->job.phase));
> }
>
> - if (priv->job.asyncJob != VIR_ASYNC_JOB_NONE)
> + if (priv->job.asyncJob != VIR_ASYNC_JOB_NONE) {
> virBufferAsprintf(&attrBuf, " flags='0x%lx'",
priv->job.apiFlags);
> + virBufferAsprintf(&attrBuf, " asyncStarted='%llu'",
priv->job.asyncStarted);
... the start timestamp is not printed, so ...
> + }
>
> if (priv->job.cb &&
> priv->job.cb->formatJob(&childBuf, &priv->job, vm) <
0)
> @@ -1307,6 +1308,13 @@ qemuDomainObjPrivateXMLParseJob(virDomainObj *vm,
> }
> VIR_FREE(tmp);
> }
> +
> + if (virXPathULongLong("string(@asyncStarted)", ctxt,
> + &priv->job.asyncStarted) == -2) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Invalid async job start"));
... this will keep it initialized to 0 ...
> + return -1;
> + }
> }
>
> if (virXPathULongHex("string(@flags)", ctxt,
&priv->job.apiFlags) == -2) {
[...]
> diff --git a/tests/qemustatusxml2xmldata/migration-in-params-in.xml
b/tests/qemustatusxml2xmldata/migration-in-params-in.xml
> index f4bc5753c4..9e9c2deac6 100644
> --- a/tests/qemustatusxml2xmldata/migration-in-params-in.xml
> +++ b/tests/qemustatusxml2xmldata/migration-in-params-in.xml
> @@ -238,7 +238,7 @@
> <flag name='dump-completed'/>
> <flag name='hda-output'/>
> </qemuCaps>
> - <job type='none' async='migration in' phase='prepare'
flags='0x900'>
... so existing backup jobs will seem to be started at the beginning of
epoch:
> + <job type='none' async='migration in' phase='prepare'
flags='0x900' asyncStarted='0'>
> <migParams>
> <param name='compress-level' value='1'/>
> <param name='compress-threads' value='8'/>
Probably not a big problem but we IMO should initialize it to current
time. That will break the test though unless you mock the time getting
function.
We can keep this fallback in qemuDomainObjRestoreAsyncJob instead of
putting it in the parser to avoid test issues.
Jirka