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@xxxxxxxxxx>
> > ---
> > 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
