On Tue, Sep 23, 2014 at 16:09:56 +0200, Cristian Klein wrote:

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- src/qemu/qemu_migration.c | 1 + src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 3 ++- src/qemu/qemu_monitor_text.c | 3 ++- 5 files changed, 7 insertions(+), 3 deletions(-)

...

diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 46d2782..a3c8aa5 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -1458,7 +1458,8 @@ int qemuMonitorTextGetMigrationStatus(qemuMonitorPtr mon, goto cleanup; }

- if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE) { + if (status->status == QEMU_MONITOR_MIGRATION_STATUS_ACTIVE || + status->status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY_ACTIVE) { tmp = end + 1;

I don't think we need to change text monitor code since we won't be talking with HMP to any version of QEMU that supports post-copy migration. Otherwise the patch is just fine and we just need to wait for post-copy to land in QEMU. Jirka

On Tue, Sep 23, 2014 at 16:09:57 +0200, Cristian Klein wrote:

Added a new `libvirt` migration flag `VIR_MIGRATE_POSTCOPY` and the necessary code to pass this flag to qemu as migration capability.

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_migration.h | 3 ++- 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6371b7b..bdc33c6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1225,6 +1225,7 @@ typedef enum { VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */ VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */ VIR_MIGRATE_RDMA_PIN_ALL = (1 << 14), /* RDMA memory pinning */ + VIR_MIGRATE_POSTCOPY = (1 << 15), /* enable (but don't start) post-copy */ } virDomainMigrateFlags;

diff --git a/src/libvirt.c b/src/libvirt.c index 1a285ca..33aeafa 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -5265,6 +5265,7 @@ virDomainMigrateDirect(virDomainPtr domain, * automatically when supported). * VIR_MIGRATE_UNSAFE Force migration even if it is considered unsafe. * VIR_MIGRATE_OFFLINE Migrate offline + * VIR_MIGRATE_POSTCOPY Enable (but don't start) post-copy * * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. * Applications using the VIR_MIGRATE_PEER2PEER flag will probably @@ -5291,6 +5292,12 @@ virDomainMigrateDirect(virDomainPtr domain, * can use either VIR_MIGRATE_NON_SHARED_DISK or * VIR_MIGRATE_NON_SHARED_INC as they are mutually exclusive. * + * If you want to enable post-copy migration you must set the + * VIR_MIGRATE_POSTCOPY flag. Once migration is active, you may + * start post-copy by calling virDomainMigrateStartPostCopy. + * When to start post-copy is entirely left to the user, libvirt + * only implements the necessary mechanism. + * * In either case it is typically only necessary to specify a * URI if the destination host has multiple interfaces and a * specific interface is required to transmit migration data. diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a5bd825..bd1f2d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1799,6 +1799,45 @@ qemuMigrationSetOffline(virQEMUDriverPtr driver,

static int +qemuMigrationSetPostCopy(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob job)

s/ // in the two lines above to fix indentation.

+{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int ret; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, job) < 0) + return -1; + + ret = qemuMonitorGetMigrationCapability( + priv->mon, + QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY);

QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY is not defined anywhere in this patchset. It seems you forgot to update qemu_monitor.[ch].

+ + if (ret < 0) { + goto cleanup; + } else if (ret == 0) { + if (job == QEMU_ASYNC_JOB_MIGRATION_IN) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Post-copy migration is not supported by " + "target QEMU binary")); + } else { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("Post-copy migration is not supported by " + "source QEMU binary")); + } + ret = -1; + goto cleanup; + } + + ret = qemuMonitorSetMigrationCapability( + priv->mon, + QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY); + + cleanup: + qemuDomainObjExitMonitor(driver, vm); + return ret; +} +static int qemuMigrationSetCompression(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob job) @@ -3580,6 +3619,10 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (flags & VIR_MIGRATE_RDMA_PIN_ALL && qemuMigrationSetPinAll(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + + if (flags & VIR_MIGRATE_POSTCOPY && + qemuMigrationSetPostCopy(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup;

if (qemuDomainObjEnterMonitorAsync(driver, vm,

I'd expect similar thing would need to be done in the Prepare phase on destination... However, if destination does not need to set the capability, we at least need to check if destination QEMU supports it and report failure from Prepare if it doesn't. And the QEMU_ASYNC_JOB_MIGRATION_IN branch in qemuMigrationSetPostCopy would be unreachable in that case.

diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index e7a90c3..349c9c4 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -41,7 +41,8 @@ VIR_MIGRATE_COMPRESSED | \ VIR_MIGRATE_ABORT_ON_ERROR | \ VIR_MIGRATE_AUTO_CONVERGE | \ - VIR_MIGRATE_RDMA_PIN_ALL) + VIR_MIGRATE_RDMA_PIN_ALL | \ + VIR_MIGRATE_POSTCOPY)

/* All supported migration parameters and their types. */ # define QEMU_MIGRATION_PARAMETERS \

Jirka

On Thu, Sep 25, 2014 at 11:42:16 +0200, Cristian KLEIN wrote:

On 2014-09-24 14:32, Jiri Denemark wrote:

...

On Tue, Sep 23, 2014 at 16:09:57 +0200, Cristian Klein wrote:

...

Added a new `libvirt` migration flag `VIR_MIGRATE_POSTCOPY` and the necessary code to pass this flag to qemu as migration capability.

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- include/libvirt/libvirt.h.in | 1 + src/libvirt.c | 7 +++++++ src/qemu/qemu_migration.c | 43 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_migration.h | 3 ++- 4 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6371b7b..bdc33c6 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1225,6 +1225,7 @@ typedef enum { VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */ VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */ VIR_MIGRATE_RDMA_PIN_ALL = (1 << 14), /* RDMA memory pinning */ + VIR_MIGRATE_POSTCOPY = (1 << 15), /* enable (but don't start) post-copy */ } virDomainMigrateFlags;

In my first review, I forgot to mention that I think another flag requesting post-copy to be started right away (instead of waiting for virDomainMigrateStartPostCopy to be called) could be useful too. ...

...

...

@@ -3580,6 +3619,10 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (flags & VIR_MIGRATE_RDMA_PIN_ALL && qemuMigrationSetPinAll(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + + if (flags & VIR_MIGRATE_POSTCOPY && + qemuMigrationSetPostCopy(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup;

if (qemuDomainObjEnterMonitorAsync(driver, vm,

I'd expect similar thing would need to be done in the Prepare phase on destination... However, if destination does not need to set the capability, we at least need to check if destination QEMU supports it and report failure from Prepare if it doesn't. And the QEMU_ASYNC_JOB_MIGRATION_IN branch in qemuMigrationSetPostCopy would be unreachable in that case.

It's up to the source qemu (through the migration protocol) to activate post-copy on the destination qemu. So there are no other steps to be done on the source.

I have mixed feelings about having libvirt check the necessary capability on the destination. Personally, I would prefer qemu to return an error like "post-copy unavailable" when the "migrate" command is issued, as there might be more factors that influence the availability of post-copy. For example, it's not sufficient for the destination qemu to support post-copy, but the destination kernel also needs userfault support.

Yeah, it won't catch all cases, but will at least fail early when someone tries to enable post-copy migration while migrating to a host with older QEMU that does not support it. Moreover, if QEMU is smart enough to check if post-copy migration can actually be used when the migration capability is set, we could catch even cases when userfault is not supported on the destination. Jirka

On Wed, Sep 24, 2014 at 14:45:02 +0200, Jiri Denemark wrote:

On Tue, Sep 23, 2014 at 16:09:58 +0200, Cristian Klein wrote:

...

The user first start migration using the `VIR_MIGRATE_POSTCOPY` flag, then calls `virDomainMigrateStartPostCopy` asynchronously to switch from pre-copy to post-copy.

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- include/libvirt/libvirt.h.in | 2 ++ src/driver.h | 4 ++++ src/libvirt.c | 37 +++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++++ 4 files changed, 48 insertions(+)

diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index bdc33c6..eabedfa 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1346,6 +1346,8 @@ int virDomainMigrateToURI3(virDomainPtr domain, unsigned int nparams, unsigned int flags);

+int virDomainMigrateStartPostCopy (virDomainPtr domain); + int virDomainMigrateSetMaxDowntime (virDomainPtr domain, unsigned long long downtime, unsigned int flags); diff --git a/src/driver.h b/src/driver.h index bb748c4..6866ccd 100644 --- a/src/driver.h +++ b/src/driver.h @@ -1212,6 +1212,9 @@ typedef int virDomainStatsRecordPtr **retStats, unsigned int flags);

+typedef int +(*virDrvDomainMigrateStartPostCopy)(virDomainPtr domain); + typedef struct _virDriver virDriver; typedef virDriver *virDriverPtr;

@@ -1435,6 +1438,7 @@ struct _virDriver { virDrvNodeGetFreePages nodeGetFreePages; virDrvConnectGetDomainCapabilities connectGetDomainCapabilities; virDrvConnectGetAllDomainStats connectGetAllDomainStats; + virDrvDomainMigrateStartPostCopy domainMigrateStartPostCopy; };

diff --git a/src/libvirt.c b/src/libvirt.c index 33aeafa..e685da2 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17803,6 +17803,43 @@ virDomainMigrateSetCompressionCache(virDomainPtr domain,

/** + * virDomainMigrateStartPostCopy: + * @domain: a domain object + * + * Starts post-copy migration. This function has to be called while + * migration (initially pre-copy) is in progress. The migration operation + * must be called with the VIR_MIGRATE_POSTCOPY flag. + * + * Returns 0 in case of success, -1 otherwise. + */ +int +virDomainMigrateStartPostCopy(virDomainPtr domain)

Oops, I forgot to mention we should add unsigned int flags parameter for this API just in case we need it in the future. Jirka

On Tue, Sep 23, 2014 at 16:09:59 +0200, Cristian Klein wrote:

Signed-off-by: Cristian Klein <cristian.klein@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, ...

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

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

On Tue, Sep 23, 2014 at 16:10:00 +0200, Cristian Klein wrote:

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 12 +++++++++++- 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 75a3a7b..9a6a974 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8152,6 +8152,7 @@ static virDriver remote_driver = { .nodeGetFreePages = remoteNodeGetFreePages, /* 1.2.6 */ .connectGetDomainCapabilities = remoteConnectGetDomainCapabilities, /* 1.2.7 */ .connectGetAllDomainStats = remoteConnectGetAllDomainStats, /* 1.2.8 */ + .domainMigrateStartPostCopy = remoteDomainMigrateStartPostCopy, /* 1.2.9 */ };

static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index a4ca0c3..c443636 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3090,6 +3090,10 @@ struct remote_connect_get_all_domain_stats_args { struct remote_connect_get_all_domain_stats_ret { remote_domain_stats_record retStats<REMOTE_DOMAIN_LIST_MAX>; }; + +struct remote_domain_migrate_start_post_copy_args { + remote_nonnull_domain dom; +}; /*----- Protocol. -----*/

/* Define the program number, protocol version and procedure numbers here. */ @@ -5472,5 +5476,11 @@ enum remote_procedure { * @generate: both * @acl: domain:block_write */ - REMOTE_PROC_DOMAIN_BLOCK_COPY = 345 + REMOTE_PROC_DOMAIN_BLOCK_COPY = 345, + + /** + * @generate: both + * @acl: domain:migrate + */ + REMOTE_PROC_DOMAIN_MIGRATE_START_POST_COPY = 346 };

This is pretty straightforward and you could have just squashed this patch into "Added new public API virDomainMigrateStartPostCopy". Jirka

On Tue, Sep 23, 2014 at 16:10:01 +0200, Cristian Klein wrote:

Added a new parameter to the `virsh migrate` command to switch to post-copy migration after a given timeout.

Signed-off-by: Cristian Klein <cristian.klein@cs.umu.se> --- tools/virsh-domain.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-- tools/virsh.pod | 5 ++++ 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index a6ced5f..1bba710 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9250,6 +9250,10 @@ static const vshCmdOptDef opts_migrate[] = { .type = VSH_OT_INT, .help = N_("force guest to suspend if live migration exceeds timeout (in seconds)") }, + {.name = "postcopy-after", + .type = VSH_OT_INT, + .help = N_("switch to post-copy migration if live migration exceeds timeout (in seconds)") + },

I think we want both postcopy and postcopy-after options. And we should also add a dedicated migrate-startpostcopy command.

{.name = "xml", .type = VSH_OT_STRING, .help = N_("filename containing updated XML for the target") @@ -9331,6 +9335,8 @@ doMigrate(void *opaque) VIR_FREE(xml); }

+ if (vshCommandOptBool(cmd, "postcopy-after")) /* actually an int */ + flags |= VIR_MIGRATE_POSTCOPY; if (vshCommandOptBool(cmd, "live")) flags |= VIR_MIGRATE_LIVE; if (vshCommandOptBool(cmd, "p2p")) @@ -9422,6 +9428,50 @@ vshMigrationTimeout(vshControl *ctl, virDomainSuspend(dom); }

+static void +vshMigrationPostCopyAfter(vshControl *ctl, + virDomainPtr dom, + void *opaque ATTRIBUTE_UNUSED) +{ + vshDebug(ctl, VSH_ERR_DEBUG, "starting post-copy\n"); + int rv = virDomainMigrateStartPostCopy(dom); + if (rv < 0) { + vshError(ctl, "%s", _("start post-copy command failed")); + } else { + vshDebug(ctl, VSH_ERR_INFO, "switched to post-copy\n"); + } +} + +/* Parse the --postcopy-after parameter in seconds, but store its + * value in milliseconds. Return -1 on error, 0 if + * no timeout was requested, and 1 if timeout was set. + * Copy-paste-adapted from vshCommandOptTimeoutToMs. + */ +static int +vshCommandOptPostCopyAfterToMs(vshControl *ctl, const vshCmd *cmd, int *postCopyAfter) +{ + int rv = vshCommandOptInt(cmd, "postcopy-after", postCopyAfter); + + if (rv < 0 || (rv > 0 && *postCopyAfter < 0)) { + vshError(ctl, "%s", _("invalid postcopy-after parameter")); + return -1; + } + if (rv > 0) { + /* Ensure that we can multiply by 1000 without overflowing. */ + if (*postCopyAfter > INT_MAX / 1000) { + vshError(ctl, "%s", _("post-copy after parameter is too big")); + return -1; + } + *postCopyAfter *= 1000; + /* 0 is a special value inside virsh, which means no timeout, so + * use 1ms instead for "start post-copy immediately" + */ + if (*postCopyAfter == 0) + *postCopyAfter = 1; + } + return rv; +} + static bool cmdMigrate(vshControl *ctl, const vshCmd *cmd) { @@ -9431,6 +9481,7 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd) bool verbose = false; bool functionReturn = false; int timeout = 0; + int postCopyAfter = 0; bool live_flag = false; vshCtrlData data = { .dconn = NULL };

@@ -9450,6 +9501,18 @@ cmdMigrate(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

+ if (vshCommandOptPostCopyAfterToMs(ctl, cmd, &postCopyAfter) < 0) { + goto cleanup; + } else if (postCopyAfter > 0 && !live_flag) { + vshError(ctl, "%s", + _("migrate: Unexpected postcopy-after for offline migration")); + goto cleanup; + } else if (postCopyAfter > 0 && timeout > 0) { + vshError(ctl, "%s", + _("migrate: --postcopy-after is incompatible with --timeout")); + goto cleanup; + } +

Is post-copy only allowed for live migrations? I know it doesn't make much sense otherwise but I'm just checking... Anyway, this check belongs to qemu driver rather than virsh so that any API user gets the error message. Jirka