On Fri, Jan 11, 2013 at 17:52:14 +0100, Michal Privoznik wrote:
> This migration cookie is meant for two purposes. The first is to be sent
> in begin phase from source to destination to let it know we support new
> implementation of VIR_MIGRATE_NON_SHARED_{DISK,INC} so destination can
> start NBD server. Then, the second purpose is, destination can let us
> know, on which port is NBD server running.
> ---
> src/qemu/qemu_migration.c | 145 +++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 124 insertions(+), 21 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index e235677..6fa4ad6 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
...
> @@ -121,6 +127,17 @@ struct _qemuMigrationCookieNetwork {
> qemuMigrationCookieNetDataPtr net;
> };
>
> +typedef struct _qemuMigrationCookieNBD qemuMigrationCookieNBD;
> +typedef qemuMigrationCookieNBD *qemuMigrationCookieNBDPtr;
> +struct _qemuMigrationCookieNBD {
> + int port; /* on which port does NBD server listen for incoming data.
> + Zero value has special meaning - it is there just to let
> + destination know we (the source) do support NBD.
> + Negative one is meant to be sent when translating from
> + perform to finish phase to let destination know it's
> + safe to stop NBD server.*/
The negative special value is redundant. Destination already knows it is
safe to stop NBD server when it gets into the finish phase.
> +};
> +
> typedef struct _qemuMigrationCookie qemuMigrationCookie;
> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
> struct _qemuMigrationCookie {
...
> @@ -882,6 +934,25 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
> (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt))))
> goto error;
>
> + if (flags & QEMU_MIGRATION_COOKIE_NBD &&
> + virXPathBoolean("count(./nbd) > 0", ctxt)) {
It would be enough to just check for "boolean(./nbd)" XPath expression
but this would work too.
> + char *port;
> +
> + if (VIR_ALLOC(mig->nbd) < 0) {
> + virReportOOMError();
> + goto error;
> + }
> +
> + port = virXPathString("string(./nbd/@port)", ctxt);
> + if (port &&
> + virStrToLong_i(port, NULL, 10, &mig->nbd->port) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Malformed nbd port '%s'"),
> + port);
> + goto error;
> + }
> + }
> +
> return 0;
>
> error:
...
> @@ -1651,7 +1738,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> origname = NULL;
>
> if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> - QEMU_MIGRATION_COOKIE_LOCKSTATE)))
> + QEMU_MIGRATION_COOKIE_LOCKSTATE |
> + QEMU_MIGRATION_COOKIE_NBD)))
> goto cleanup;
>
> if (qemuMigrationJobStart(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
> @@ -1711,8 +1799,12 @@ done:
> else
> cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS;
>
> - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen,
> - cookieFlags) < 0) {
> + /* dummy place holder for real work */
> + nbdPort = 0;
> + cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
We should guard QEMU_MIGRATION_COOKIE_NBD with the check for
non-tunnelled migration in the same way we did before to make sure that
we deny it if a newer libvirtd which already supports nbd for tunnelled
migrations talks to an older one. It's possible it would be denied by
source because destination would not provide some required data but we
don't know this yet and better safe than sorry.
> +
> + if (qemuMigrationBakeCookie(mig, driver, vm, nbdPort,
> + cookieout, cookieoutlen, cookieFlags) < 0) {
> /* We could tear down the whole guest here, but
> * cookie data is (so far) non-critical, so that
> * seems a little harsh. We'll just warn for now.
...
> @@ -2224,9 +2327,12 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> return -1;
> }
>
> - if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> - QEMU_MIGRATION_COOKIE_GRAPHICS)))
> + if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein,
> + cookieinlen, cookieFlags))) {
> + cookieFlags &= ~QEMU_MIGRATION_COOKIE_GRAPHICS;
> goto cleanup;
> + }
> + cookieFlags &= ~QEMU_MIGRATION_COOKIE_GRAPHICS;
Hmm, this looks pretty strange. Why don't you initialize cookieFlags to
zero and pass cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS to
qemuMigrationEatCookie?
Moreover, I think we should check if source wanted to use NBD but
destination did not send NBD cookie back and log this situation.
>
> if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0)
> VIR_WARN("unable to provide data for graphics client relocation");
> @@ -2258,11 +2364,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
> goto cleanup;
> }
>
> - if (flags & VIR_MIGRATE_NON_SHARED_DISK)
> - migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_DISK;
> -
> - if (flags & VIR_MIGRATE_NON_SHARED_INC)
> - migrate_flags |= QEMU_MONITOR_MIGRATE_NON_SHARED_INC;
>
> /* connect to the destination qemu if needed */
> if (spec->destType == MIGRATION_DEST_CONNECT_HOST &&
> @@ -2370,10 +2471,11 @@ cleanup:
> VIR_FORCE_CLOSE(fd);
> }
>
> + cookieFlags |= (QEMU_MIGRATION_COOKIE_PERSISTENT |
> + QEMU_MIGRATION_COOKIE_NETWORK);
> if (ret == 0 &&
> - qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen,
> - QEMU_MIGRATION_COOKIE_PERSISTENT |
> - QEMU_MIGRATION_COOKIE_NETWORK) < 0) {
> + qemuMigrationBakeCookie(mig, driver, vm, -1, cookieout,
> + cookieoutlen, cookieFlags) < 0) {
> VIR_WARN("Unable to encode migration cookie");
> }
>
This change should not be necessary, we don't need to send any NBD
related data do the destination host once migration is finished.
...
> @@ -3327,12 +3429,12 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
>
> qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup);
>
> - cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK;
> + cookieFlags = QEMU_MIGRATION_COOKIE_NETWORK | QEMU_MIGRATION_COOKIE_NBD;
> if (flags & VIR_MIGRATE_PERSIST_DEST)
> - cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
> + cookieFlags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
>
> if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein,
> - cookieinlen, cookie_flags)))
> + cookieinlen, cookieFlags)))
> goto endjob;
>
> /* Did the migration go as planned? If yes, return the domain
No need to parse NBD cookie here.
> @@ -3479,7 +3581,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> VIR_DOMAIN_EVENT_STOPPED_FAILED);
> }
>
> - if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen, 0) < 0)
> + if (qemuMigrationBakeCookie(mig, driver, vm, 0,
> + cookieout, cookieoutlen, 0) < 0)
> VIR_WARN("Unable to encode migration cookie");
>
> endjob:
Generally, the code looks good once you remove the redundant NBD data
from migration cookie passed from Perform to Finish.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list