<div dir="ltr">Hi Jirka,<div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 25, 2018 at 8:43 PM, Jiri Denemark <span dir="ltr"><<a href="mailto:jdenemar@redhat.com" target="_blank">jdenemar@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Jan 25, 2018 at 19:51:23 +0530, Prerna Saxena wrote:<br>
> In case of non-p2p migration, in case libvirt client gets disconnected from source libvirt<br>
> after PERFORM phase is over, the daemon just resets the current migration job.<br>
> However, the VM could be left paused on both source and destination in such case. In case<br>
> the client reconnects and queries migration status, the job has been blanked out from source libvirt,<br>
> and this reconnected client has no clear way of figuring out if an unclean migration had previously<br>
> been aborted.<br>
<br>
</span>The virDomainGetState API should return VIR_DOMAIN_PAUSED with<br>
VIR_DOMAIN_PAUSED_MIGRATION reason. Is this not enough?<br>
<span class=""><br></span></blockquote><div><br></div><div>I understand that a client application should poll source libvirtd for status of migration job completion using virDomainGetJobStats().</div><div>However, as you explained above, cleanup callbacks clear the job info so a client should additionally be polling for virDomainGetState() too.</div><div>Would it not be cleaner to have a single API reflect the source of truth?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> This patch calls out a "potentially" incomplete migration as a failed<br>
> job, so that a client may<br>
<br>
</span>As you say it's potentially incomplete, so marking it as failed is not<br>
completely correct. It's a split brain when the source cannot<br>
distinguish whether the migration was successful or not.<br></blockquote><div><br></div><div>Agree, it might have run to completion too, as we observed in some cases. Do you think marking the job status as "UNKNOWN" is better articulation of the current state?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c<br>
> index e8e0313..7c60d17 100644<br>
> --- a/src/qemu/qemu_domain.c<br>
> +++ b/src/qemu/qemu_domain.c<br>
> @@ -4564,6 +4564,22 @@ qemuDomainObjDiscardAsyncJob(<wbr>virQEMUDriverPtr driver, virDomainObjPtr obj)<br>
>      qemuDomainObjSaveJob(driver, obj);<br>
>  }<br>
><br>
> +<br>
> +void<br>
> +qemuDomainObjFailAsyncJob(<wbr>virQEMUDriverPtr driver, virDomainObjPtr obj)<br>
> +{<br>
> +    qemuDomainObjPrivatePtr priv = obj->privateData;<br>
> +    VIR_FREE(priv->job.completed);<br>
> +    if (VIR_ALLOC(priv->job.<wbr>completed) == 0) {<br>
> +        priv->job.current->type = VIR_DOMAIN_JOB_FAILED;<br>
> +        priv->job.completed = priv->job.current;<br>
<br>
</span>This will just leak the memory allocated for priv->job.completed by<br>
overwriting the pointer to the one from priv->job.current, ...<br>
<span class=""><br>
> +    } else {<br>
> +        VIR_WARN("Unable to allocate job.completed for VM %s", obj->def->name);<br>
> +    }<br>
> +    qemuDomainObjResetAsyncJob(<wbr>priv);<br>
<br>
</span>which will point to a freed memory after this call.<br></blockquote><div><br></div><div>Agree, I will fix this.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +    qemuDomainObjEndJob(driver, obj);<br>
<br>
And while this is almost certainly (I didn't really check though) not<br>
something you should call from a close callback, you don't save the<br>
changes to the status XML so once libvirtd restarts, it will think the<br>
domain is still being migrated.<br>
</blockquote></div><br></div><div class="gmail_extra">I will add the same to status XML. </div><div class="gmail_extra">I am suggesting that strengthening the job data would be additionally useful. If the daemon has not restarted, job information can still get us fairly accurate status of migration. Pls let me know if you think this is not useful, I will be happy to learn the rationale.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards,</div><div class="gmail_extra">Prerna</div></div>