[libvirt PATCH 17/80] qemu: Restore async job start timestamp on reconnect

[Peter Krempa]

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