[libvirt] [PATCH 4/6] Added domainMigrateStartPostCopy in qemu driver

[Cristian KLEIN]

On 2014-09-24 15:06, Jiri Denemark wrote:
> On Tue, Sep 23, 2014 at 16:09:59 +0200, Cristian Klein wrote:
>> Signed-off-by: Cristian Klein <cristian.klein at cs.umu.se>
>> ---
>>   src/qemu/qemu_driver.c       | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>   src/qemu/qemu_monitor.c      | 19 +++++++++++++++++++
>>   src/qemu/qemu_monitor.h      |  2 ++
>>   src/qemu/qemu_monitor_json.c | 18 ++++++++++++++++++
>>   src/qemu/qemu_monitor_json.h |  1 +
>>   src/qemu/qemu_monitor_text.c | 11 +++++++++++
>>   src/qemu/qemu_monitor_text.h |  2 ++
>
> No need to touch qemu_monitor_text.[ch] since we will only use QMP with
> any QEMU that supports post-copy migration.
>
>>   7 files changed, 97 insertions(+)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index e73d4f9..02c9a3b 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -11861,6 +11861,49 @@ qemuDomainGetJobStats(virDomainPtr dom,
>>   }
>>
>>
>> +static int qemuMigrateStartPostCopy(virDomainPtr dom)
>> +{
>> +    virQEMUDriverPtr driver = dom->conn->privateData;
>> +    virDomainObjPtr vm;
>> +    int ret = -1;
>> +    qemuDomainObjPrivatePtr priv;
>> +
>> +    if (!(vm = qemuDomObjFromDomain(dom)))
>> +        goto cleanup;
>> +
>> +    if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MIGRATION_OP) < 0)
>> +        goto cleanup;
>> +
>> +    if (!virDomainObjIsActive(vm)) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID,
>> +                       "%s", _("domain is not running"));
>> +        goto endjob;
>> +    }
>> +
>> +    priv = vm->privateData;
>> +
>> +    if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) {
>> +        virReportError(VIR_ERR_OPERATION_INVALID, "%s",
>> +                       _("post-copy can only be started "
>> +		         "while migration is in progress"));
>
> We forbid the use of tabs for indentation. Please, run make syntax-check
> (in addition to make check) before sending patches to catch this type of
> issues.
>
>> +        goto cleanup;
>> +    }
>> +
>> +    VIR_DEBUG("Starting post-copy");
>> +    qemuDomainObjEnterMonitor(driver, vm);
>> +    ret = qemuMonitorMigrateStartPostCopy(priv->mon);
>> +    qemuDomainObjExitMonitor(driver, vm);
>> +
>> + endjob:
>> +    if (!qemuDomainObjEndJob(driver, vm))
>> +        vm = NULL;
>> +
>> + cleanup:
>> +    if (vm)
>> +        virObjectUnlock(vm);
>> +    return ret;
>> +}
>> +
>>   static int qemuDomainAbortJob(virDomainPtr dom)
>>   {
>>       virQEMUDriverPtr driver = dom->conn->privateData;
>> @@ -18259,6 +18302,7 @@ static virDriver qemuDriver = {
>>       .nodeGetFreePages = qemuNodeGetFreePages, /* 1.2.6 */
>>       .connectGetDomainCapabilities = qemuConnectGetDomainCapabilities, /* 1.2.7 */
>>       .connectGetAllDomainStats = qemuConnectGetAllDomainStats, /* 1.2.8 */
>> +    .domainMigrateStartPostCopy = qemuMigrateStartPostCopy, /* 1.2.9 */
>
> This will need to be updated.
>
>>   };
>>
>>
>> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
>> index 14688bf..0b230cc 100644
>> --- a/src/qemu/qemu_monitor.c
>> +++ b/src/qemu/qemu_monitor.c
>> @@ -2384,6 +2384,25 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
>>       return ret;
>>   }
>>
>> +int qemuMonitorMigrateStartPostCopy(qemuMonitorPtr mon)
>> +{
>> +    int ret;
>> +    VIR_DEBUG("mon=%p", mon);
>> +
>> +    if (!mon) {
>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>> +                       _("monitor must not be NULL"));
>> +        return -1;
>> +    }
>> +
>> +    if (mon->json)
>> +        ret = qemuMonitorJSONMigrateStartPostCopy(mon);
>> +    else
>> +        ret = qemuMonitorTextMigrateStartPostCopy(mon);
>
> Just return an error if !mon->json as other recent functions in
> qemu_monitor.c do.
>
>> +    return ret;
>> +}
>> +
>> +
>>   int qemuMonitorMigrateCancel(qemuMonitorPtr mon)
>>   {
>>       int ret;
>> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
>> index 587f779..97d980d 100644
>> --- a/src/qemu/qemu_monitor.h
>> +++ b/src/qemu/qemu_monitor.h
>> @@ -539,6 +539,8 @@ int qemuMonitorMigrateToUnix(qemuMonitorPtr mon,
>>                                unsigned int flags,
>>                                const char *unixfile);
>>
>> +int qemuMonitorMigrateStartPostCopy(qemuMonitorPtr mon);
>> +
>>   int qemuMonitorMigrateCancel(qemuMonitorPtr mon);
>>
>>   int qemuMonitorGetDumpGuestMemoryCapability(qemuMonitorPtr mon,
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index e98962b..14e7f84 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -2798,6 +2798,24 @@ int qemuMonitorJSONMigrate(qemuMonitorPtr mon,
>>       return ret;
>>   }
>>
>> +int qemuMonitorJSONMigrateStartPostCopy(qemuMonitorPtr mon)
>> +{
>> +    int ret;
>> +    virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("migrate-start-postcopy", NULL);
>> +    virJSONValuePtr reply = NULL;
>> +    if (!cmd)
>> +        return -1;
>
> I think the following would be better to avoid the long line:
>
>      virJSONValuePtr cmd;
>
>      cmd = qemuMonitorJSONMakeCommand("migrate-start-postcopy", NULL);
>      if (!cmd)
>      ...
>
>> +
>> +    ret = qemuMonitorJSONCommand(mon, cmd, &reply);
>> +
>> +    if (ret == 0)
>> +        ret = qemuMonitorJSONCheckError(cmd, reply);
>> +
>> +    virJSONValueFree(cmd);
>> +    virJSONValueFree(reply);
>> +    return ret;
>> +}
>> +
>>   int qemuMonitorJSONMigrateCancel(qemuMonitorPtr mon)
>>   {
>>       int ret;
>> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
>> index d366179..54beb7f 100644
>> --- a/src/qemu/qemu_monitor_json.h
>> +++ b/src/qemu/qemu_monitor_json.h
>> @@ -151,6 +151,7 @@ int qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitorPtr mon,
>>                                              bool *spice_migrated);
>>
>>
>> +int qemuMonitorJSONMigrateStartPostCopy(qemuMonitorPtr mon);
>>   int qemuMonitorJSONMigrateCancel(qemuMonitorPtr mon);
>>
>>   int qemuMonitorJSONGetDumpGuestMemoryCapability(qemuMonitorPtr mon,
> ...

Already fixed for v2.

> This mostly looks good in isolation but I think this is not going to
> work. When post-copy is started, QEMU on the destination host will be
> resumed (I'm not sure if that happens automatically or we have to do
> it), which basically means we need to jump out of the Perform state and
> call Finish and once it returns, we should keep waiting for the
> post-copy migration to finish in Confirm state and kill the domain at
> the end. It's certainly possible the steps we need to do are a bit
> different since I'm not familiar with all the details about post-copy
> migration, but I believe we need to do something. And just running a
> single QEMU command is not enough to start post-copy in libvirt.

I'm not sure to follow. I tested the patch and it worked well: A VM that 
was "unmigratable" with pre-copy was successfully migrated through 
post-copy. Through the migration protocol, once we start post-copy on 
the source qemu, the following will happen:

- source qemu suspends VM and transfer CPU state;
- destination qemu resumes the VM.

Could you tell me why you think it's necessary to jump out of Perform 
state? What is libvirt doing when calling Finish that the destination VM 
requires to function properly?

-- 
Cristian Klein, PhD
Post-doc @ Umeå Universitet
http://www8.cs.umu.se/~cklein