[libvirt] [PATCH v3 8/9] qemu: migration: selective block device migration

[John Ferlan]

On 05/26/2015 09:01 AM, Michal Privoznik wrote:
> From: Pavel Boldin <pboldin at mirantis.com>
> 
> Implement a `migrate_disks' parameters for the QEMU driver. This multi-
> value parameter can be used to explicitly specify what block devices
> are to be migrated using the NBD server. Tunnelled migration using NBD
> is to be done.
> 
> Signed-off-by: Pavel Boldin <pboldin at mirantis.com>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  include/libvirt/libvirt-domain.h |   9 ++
>  src/qemu/qemu_driver.c           |  72 +++++++++---
>  src/qemu/qemu_migration.c        | 245 ++++++++++++++++++++++++++++-----------
>  src/qemu/qemu_migration.h        |  24 ++--
>  4 files changed, 258 insertions(+), 92 deletions(-)
> 

Didn't see much major - just a NIT or two... Also noted sometimes the
order of arguments is nmigrate_disks, migrate_disks and others it's
reversed (qemuMigrationBegin and virTypedParamsPickStrings). It's not a
big deal to me personally, but there are those that do ;-)

...


> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 279b43f..5a8057e 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1579,12 +1579,34 @@ qemuMigrationPrecreateDisk(virConnectPtr conn,
>      return ret;
>  }
>  
> +static bool
> +qemuMigrateDisk(virDomainDiskDef const *disk,
> +                size_t nmigrate_disks, const char **migrate_disks)

NIT: 3rd arg on own line

> +{
> +    size_t i;
> +    /* Check if the disk alias is in the list */
> +    if (nmigrate_disks) {
> +        for (i = 0; i < nmigrate_disks; i++) {
> +            if (STREQ(disk->dst, migrate_disks[i]))
> +                return true;
> +        }
> +        return false;
> +    }
> +
> +    /* Default is to migrate only non-shared non-readonly disks
> +     * with source */
> +    return !disk->src->shared && !disk->src->readonly &&
> +           virDomainDiskGetSource(disk);
> +}
> +
>  

...

>  
> @@ -2710,6 +2734,8 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>                          const char *dname,
>                          char **cookieout,
>                          int *cookieoutlen,
> +                        size_t nmigrate_disks,
> +                        const char **migrate_disks,
>                          unsigned long flags)
>  {
>      char *rv = NULL;
> @@ -2721,9 +2747,11 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>      bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
>  
>      VIR_DEBUG("driver=%p, vm=%p, xmlin=%s, dname=%s,"
> -              " cookieout=%p, cookieoutlen=%p, flags=%lx",
> +              " cookieout=%p, cookieoutlen=%p,"
> +              " nmigrate_disks=%zu, migrate_disks=%p, flags=%lx",
>                driver, vm, NULLSTR(xmlin), NULLSTR(dname),
> -              cookieout, cookieoutlen, flags);
> +              cookieout, cookieoutlen, nmigrate_disks,
> +              migrate_disks, flags);
>  
>      if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
>          goto cleanup;
> @@ -2738,17 +2766,54 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver,
>      if (!qemuMigrationIsAllowed(driver, vm, NULL, true, abort_on_error))
>          goto cleanup;
>  
> -    if (!(flags & VIR_MIGRATE_UNSAFE) && !qemuMigrationIsSafe(vm->def))
> +    if (!(flags & VIR_MIGRATE_UNSAFE) &&
> +        !qemuMigrationIsSafe(vm->def, nmigrate_disks, migrate_disks))
>          goto cleanup;
>  
> -    if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) &&
> -        virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DRIVE_MIRROR)) {
> -        /* TODO support NBD for TUNNELLED migration */
> -        if (flags & VIR_MIGRATE_TUNNELLED) {
> -            VIR_WARN("NBD in tunnelled migration is currently not supported");
> -        } else {
> -            cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
> -            priv->nbdPort = 0;
> +    if (flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC)) {
> +        bool has_drive_mirror =  virQEMUCapsGet(priv->qemuCaps,
> +                                                QEMU_CAPS_DRIVE_MIRROR);
> +
> +        if (nmigrate_disks) {
> +            if (has_drive_mirror) {
> +                size_t i, j;
> +
> +                /* Check user requested only known disk targets. */
> +                for (i = 0; i < nmigrate_disks; i++) {
> +                    for (j = 0; j < vm->def->ndisks; j++) {
> +                        if (STREQ(vm->def->disks[j]->dst, migrate_disks[i]))
> +                            break;
> +                    }
> +
> +                    if (j == vm->def->ndisks) {
> +                        virReportError(VIR_ERR_INVALID_ARG,
> +                                       _("disk target %s not found"),
> +                                       migrate_disks[i]);
> +                        goto cleanup;
> +                    }
> +                }
> +
> +                if (flags & VIR_MIGRATE_TUNNELLED) {
> +                    virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                                   _("Selecting disks to migrate is not "
> +                                     "implemented for tunnelled migration"));
> +                    goto cleanup;
> +                }

NIT: this check could be done prior to loops...

> +            } else {
> +                virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                               _("qemu does not support drive-mirror command"));


NIT: s/qemu/this qemu binary

> +                goto cleanup;

A level of indention could be removed by

if (nmigrate_disks) {
    size_t i, j;

    if (!has_drive_mirror) {
        virReportError()
        goto cleanup;
    }

    if (flags & VIR_MIGRATE_TUNNELLED) {
        virReportError()
        goto cleanup;
    }

    for (...) {}
}


Rest seemed OK to me, too.

John