[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 RESEND 02/12] Introduce NBD migration cookie



On Mon, Feb 18, 2013 at 15:38:35 +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 the NBD server is running.
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 36e55d2..82c3f97 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
...
> @@ -121,6 +127,14 @@ 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. */

I think you can drop this note about 0 being special. The support for
NBD can detected by an empty <nbd> element in the cookie (i.e.,
QEMU_MIGRATION_COOKIE_NBD is set in mig->flags and mig->ndb is not
NULL). And it seems the parsing code does what I suggest and does not
need port='0' attribute in <nbd/> once 9/12 is applied.

> +};
> +
>  typedef struct _qemuMigrationCookie qemuMigrationCookie;
>  typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>  struct _qemuMigrationCookie {
...
> @@ -837,6 +883,12 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>              goto error;
>          }
>  
> +        /* nbd is optional */
> +        if (val == QEMU_MIGRATION_COOKIE_FLAG_NBD) {
> +            VIR_FREE(str);
> +            continue;
> +        }
> +

This does not make sense at all, just remove this hunk. Features marked
as mandatory are always mandatory and NBD is no exception.

>          if ((flags & (1 << val)) == 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unsupported migration cookie feature %s"),
> @@ -889,6 +941,27 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>          (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt))))
>          goto error;
>  
> +    if (flags & QEMU_MIGRATION_COOKIE_NBD &&
> +        virXPathBoolean("boolean(./nbd)", ctxt)) {
> +        char *port;
> +
> +        port = virXPathString("string(./nbd/@port)", ctxt);
> +        if (port) {
> +            if (VIR_ALLOC(mig->nbd) < 0) {
> +                virReportOOMError();
> +                goto error;
> +            }
> +            if (virStrToLong_i(port, NULL, 10, &mig->nbd->port) < 0) {
> +                VIR_FREE(port);
> +                virReportError(VIR_ERR_INTERNAL_ERROR,
> +                               _("Malformed nbd port '%s'"),
> +                               port);
> +                goto error;
> +            }
> +            VIR_FREE(port);
> +        }

We even have virXPathInt that does the conversion for you, btw. But
anyway, sender would send <nbd/> if port == 0 and the receiver would
just ignore it since there is no port attribute present. Something's
strange here. And you actually fix that in 9/12.

> +    }
> +
>      virObjectUnref(caps);
>      return 0;
>  
...
> @@ -1666,7 +1755,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)
> @@ -1726,8 +1816,19 @@ done:
>      else
>          cookieFlags = QEMU_MIGRATION_COOKIE_GRAPHICS;
>  
> -    if (qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen,
> -                                cookieFlags) < 0) {
> +    if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) &&
> +        virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) {

I think we should only do this if the source actually asked for NBD,
that is mig->nbd is not NULL.

> +        /* TODO support NBD for TUNNELLED migration */
> +        if (flags & VIR_MIGRATE_TUNNELLED)
> +            VIR_DEBUG("NBD in tunnelled migration is currently not supported");
> +        else {
> +            cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
> +            priv->nbdPort = 0;
> +        }
> +    }
> +
> +    if (qemuMigrationBakeCookie(mig, driver, vm, 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.
...
> @@ -2241,8 +2353,9 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>          return -1;
>      }
>  
> -    if (!(mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> -                                       QEMU_MIGRATION_COOKIE_GRAPHICS)))
> +    mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
> +                                 cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS);
> +    if (!mig)
>          goto cleanup;
>  

I think we should emit a log message when we support NBD but it was not
present in migration cookie. And perhaps clear the flag from
cookieFlags. Now I see you did that in 7/12.

>      if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig) < 0)
...

Jirka


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]