[libvirt] [PATCH v1 02/11] Introduce NBD migration cookie

[Eric Blake]

----- Original Message -----
> 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 |  108
>  ++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 97 insertions(+), 11 deletions(-)

Before looking at the code, I have a question:

Should we be adding a new VIR_DRV_FEATURE_... bit, and setting that
driver feature bit in qemu_driver.c:qemuSupportsFeature(), and only
send the cookie on the source side if the destination side knows how
to handle it?

My concern (and it might be an invalid concern) is that if we send
the nbd cookie but the destination is too old to handle it, then
will the destination reject the cookie?

On the other hand, this is more than just a handshake determined by
the compile-time versions of libvirtd; after all, not only does
libvirtd on both sides of the migration need to understand the
cookie, but also qemu on the destination side has to support the
NBD server, and qemu on the source side has to support drive-mirror.
If any one of those pieces are not present, then we can still
migrate storage, by using the older flags of the 'migrate' command
itself, rather than outright failure.

It would help if you document the fallback cases, something like:

old libvirt src -> any destination:
  no cookie sent, old-style storage migration
new libvirt src, but src qemu too old:
  same as old libvirt src
new libvirt/qemu src -> old libvirt destination:
  src->dst cookie sent? may depend on VIR_DRV_FEATURE
  but no dst->src cookie reply, so use old-style migration
new libvirt src -> new libvirt old qemu destination:
  VIR_DRV_FEATURE would be set (libvirt understands it),
  but because qemu does not support it, the dst must not
  return the cookie reply, so the source can fall back to
  old-style migration
new libvirt src -> new libvirt/qemu dest:
  cookie exchange succeeds, use new style migration

Now, on to the code:

> @@ -79,13 +80,14 @@ enum qemuMigrationCookieFlags {
>  VIR_ENUM_DECL(qemuMigrationCookieFlag);
>  VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>                QEMU_MIGRATION_COOKIE_FLAG_LAST,
> -              "graphics", "lockstate", "persistent", "network");
> +              "graphics", "lockstate", "persistent", "network",
> "nbd");

[About the only good thing of this webmail interface botching
long lines is that it is easy to spot lines that are worth wrapping]

> +static int
> +qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
> +                          int nbdPort)
> +{
> +    /* It is not a bug if there already is a NBD data */
> +    if (!mig->nbd &&
> +        VIR_ALLOC(mig->nbd) < 0) {
> +        virReportOOMError();
> +        return -1;
> +    }
> +
> +    mig->nbd->port = nbdPort;

If there is already NBD data (I'm assuming that it gets
created once per disk that needs migration?), does this
override of the former mig->nbd->port cause us to forget
an important piece of information?

Overall, the cookie manipulation seems reasonable, but I'm still
worried whether new libvirt talking to older libvirt will cause
confusion if the <nbd> element appears in the cookie, and also
whether new libvirt gracefully handles the absence of a cookie
from older libvirt.