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

Re: [libvirt] [RFC PATCH v2 2/3] qemu: RDMA migration support using 'rdma' URI



On Mon, Jan 13, 2014 at 14:28:11 +0800, mrhines linux vnet ibm com wrote:
> From: "Michael R. Hines" <mrhines us ibm com>
> 
> The switch from x-rdma => rdma has not yet happened,
> but at least we can review the patch until it goes
> through on qemu-devel.

The paragraph above would better fit after "---" below so that it
disappears once this patch gets applied as the statement won't be valid
anymore at that time.

> USAGE: $ virsh migrate --live --migrateuri x-rdma:hostname domain qemu+ssh://hostname/system

s/x-rdma/rdma/ and I believe we should use rdma://hostname as the URI

> Full documenation of the feature: http://wiki.qemu.org/Features/RDMALiveMigration
> 
> This patch includes mainly making all the locations in
> libvirt where the 'tcp' string was hard-coded to be
> more flexible to use more than one protocol.


> 
> Signed-off-by: Michael R. Hines <mrhines us ibm com>
> ---
>  src/qemu/qemu_capabilities.c |  13 +++++
>  src/qemu/qemu_capabilities.h |   1 +
>  src/qemu/qemu_command.c      |   8 ++++
>  src/qemu/qemu_migration.c    | 110 ++++++++++++++++++++++++++++++++++---------
>  src/qemu/qemu_monitor.c      |   3 +-
>  src/qemu/qemu_monitor.h      |   1 +
>  src/util/viruri.c            |   7 ++-
>  7 files changed, 119 insertions(+), 24 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 548b988..d82b48c 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -243,6 +243,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>                "virtio-mmio",
>                "ich9-intel-hda",
>                "kvm-pit-lost-tick-policy",
> +
> +              "migrate-qemu-rdma", /* 160 */

s/migrate-qemu-rdma/migrate-rdma/

"qemu" string in pretty redundant here given that it is a qemu
capability.

>      );
>  
>  struct _virQEMUCaps {
> @@ -906,6 +908,9 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache)
>      virCapabilitiesAddHostMigrateTransport(caps,
>                                             "tcp");
>  
> +    virCapabilitiesAddHostMigrateTransport(caps,
> +                                           "rdma");
> +
>      /* QEMU can support pretty much every arch that exists,
>       * so just probe for them all - we gracefully fail
>       * if a qemu-system-$ARCH binary can't be found
> @@ -1110,6 +1115,7 @@ virQEMUCapsComputeCmdFlags(const char *help,
>       *  -incoming unix   (qemu >= 0.12.0)
>       *  -incoming fd     (qemu >= 0.12.0)
>       *  -incoming stdio  (all earlier kvm)
> +     *  -incoming rdma   (qemu >= 2.0.0)
>       *
>       * NB, there was a pre-kvm-79 'tcp' support, but it
>       * was broken, because it blocked the monitor console
> @@ -1130,6 +1136,9 @@ virQEMUCapsComputeCmdFlags(const char *help,
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_KVM_STDIO);
>      }
>  
> +    if (version >= 2000000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA);
> +
>      if (version >= 10000)
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_0_10);
>  

This is not needed, we won't be parsing -help for any QEMU that supports
RDMA.

> @@ -2561,6 +2570,10 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps,
>      if (qemuCaps->version >= 1006000)
>          virQEMUCapsSet(qemuCaps, QEMU_CAPS_DEVICE_VIDEO_PRIMARY);
>  
> +    if (qemuCaps->version >= 2000000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA);
> +
> +

And here we need a better check for rdma migration. What if someone
compiles QEMU without RDMA support?

>      if (virQEMUCapsProbeQMPCommands(qemuCaps, mon) < 0)
>          goto cleanup;
>      if (virQEMUCapsProbeQMPEvents(qemuCaps, mon) < 0)
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 02d47c6..3e78961 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -198,6 +198,7 @@ enum virQEMUCapsFlags {
>      QEMU_CAPS_DEVICE_VIRTIO_MMIO = 157, /* -device virtio-mmio */
>      QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */
>      QEMU_CAPS_KVM_PIT_TICK_POLICY = 159, /* kvm-pit.lost_tick_policy */
> +    QEMU_CAPS_MIGRATE_QEMU_RDMA  = 160, /* have qemu rdma migration */

s/_QEMU// as it is redundant.

>  
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 763417f..0d23d8b 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -9448,6 +9448,14 @@ qemuBuildCommandLine(virConnectPtr conn,
>                  goto error;
>              }
>              virCommandAddArg(cmd, migrateFrom);
> +        } else if (STRPREFIX(migrateFrom, "rdma")) {
> +            if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_RDMA)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               "%s", _("RDMA migration is not supported with "
> +                                       "this QEMU binary"));
> +                goto error;
> +            }
> +            virCommandAddArg(cmd, migrateFrom);
>          } else if (STREQ(migrateFrom, "stdio")) {
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD)) {
>                  virCommandAddArgFormat(cmd, "fd:%d", migrateFd);
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index ef6f1c5..1e0f538 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -46,6 +46,7 @@
>  #include "virerror.h"
>  #include "viralloc.h"
>  #include "virfile.h"
> +#include "virprocess.h"
>  #include "datatypes.h"
>  #include "fdstream.h"
>  #include "viruuid.h"
> @@ -2163,6 +2164,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>                          virDomainDefPtr *def,
>                          const char *origname,
>                          virStreamPtr st,
> +                        const char *protocol,
>                          unsigned short port,
>                          bool autoPort,
>                          const char *listenAddress,
> @@ -2275,6 +2277,16 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>              freeaddrinfo(info);
>              hostIPv6Capable = true;
>          }
> +
> +        /*
> +         * RDMA (iWarp) until linux 3.11 is broken, need
> +         * better host librdmacm IPv6 support detection.
> +         * Disallow by default for now if RDMA.
> +         */
> +        if (hostIPv6Capable && strstr(protocol, "rdma")) {
> +            hostIPv6Capable = false;
> +        }
> +

Is this still needed? 3.11 will already be quite old when QEMU with RDMA
support is released...

>          if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache,
>                                                      (*def)->emulator)))
>              goto cleanup;
> @@ -2318,9 +2330,9 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>           * -incoming <IPv4 addr>:port or -incoming <hostname>:port
>           */
>          if ((encloseAddress &&
> -             virAsprintf(&migrateFrom, "tcp:[%s]:%d", listenAddress, port) < 0) ||
> +             virAsprintf(&migrateFrom, "%s:[%s]:%d", protocol, listenAddress, port) < 0) ||
>              (!encloseAddress &&
> -             virAsprintf(&migrateFrom, "tcp:%s:%d", listenAddress, port) < 0))
> +             virAsprintf(&migrateFrom, "%s:%s:%d", protocol, listenAddress, port) < 0))
>              goto cleanup;
>      }
>  
> @@ -2507,7 +2519,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
>  
>      ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
>                                    cookieout, cookieoutlen, def, origname,
> -                                  st, 0, false, NULL, flags);
> +                                  st, "tcp", 0, false, NULL, flags);
>      return ret;
>  }
>  
> @@ -2529,6 +2541,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>      unsigned short port = 0;
>      bool autoPort = true;
>      char *hostname = NULL;
> +    const char *protocol = NULL;
> +    char *well_formed_protocol = NULL;
>      const char *p;
>      char *uri_str = NULL;
>      int ret = -1;
> @@ -2577,21 +2591,37 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>          if (virAsprintf(uri_out, "tcp:%s:%d", hostname, port) < 0)
>              goto cleanup;
>      } else {
> -        /* Check the URI starts with "tcp:".  We will escape the
> +        char * protocol_save = NULL;
> +        char * uri_save = NULL;
> +
> +        /* Check the URI starts with a valid prefix.  We will escape the
>           * URI when passing it to the qemu monitor, so bad
>           * characters in hostname part don't matter.
>           */
> -        if (!(p = STRSKIP(uri_in, "tcp:"))) {
> -            virReportError(VIR_ERR_INVALID_ARG, "%s",
> -                           _("only tcp URIs are supported for KVM/QEMU"
> -                             " migrations"));
> +
> +        if (VIR_STRDUP(uri_save, uri_in) <= 0) {
>              goto cleanup;
>          }
>  
> -        /* Convert uri_in to well-formed URI with // after tcp: */
> -        if (!(STRPREFIX(uri_in, "tcp://"))) {
> +        protocol = strtok_r(uri_save, ":", &protocol_save);
> +        VIR_FREE(uri_save);
> +        if (protocol) {
> +            if (virAsprintf(&well_formed_protocol, "%s://", protocol) < 0)
> +                goto cleanup;
> +        }
> +
> +        /* Make sure it's a valid protocol */
> +        if (!(p = STRSKIP(uri_in, "tcp:")) &&
> +            !(p = STRSKIP(uri_in, "rdma:"))) {
> +            virReportError(VIR_ERR_INVALID_ARG, _("URI %s (%s) not supported"
> +                            " for KVM/QEMU migrations"), protocol, uri_in);
> +            goto cleanup;
> +        }
> +
> +        /* Convert uri_in to well-formed URI with // after colon */
> +        if (!(STRPREFIX(uri_in, well_formed_protocol))) {
>              well_formed_uri = false;
> -            if (virAsprintf(&uri_str, "tcp://%s", p) < 0)
> +            if (virAsprintf(&uri_str, "%s://%s", protocol, p) < 0)
>                  goto cleanup;
>          }

As I already said several month ago, "tcp:hostname" was a mistake and we
should not be redoing it with rdma. Thus I think we should just check
for "tcp:" and turn it into "tcp://" and then parse the result as a
proper URI. RDMA should always use rdma:// to avoid this backward
compatibility hacks.

>  
> @@ -2637,10 +2667,13 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>  
>      ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
>                                    cookieout, cookieoutlen, def, origname,
> -                                  NULL, port, autoPort, listenAddress, flags);
> +                                  NULL, protocol ? protocol : "tcp",
> +                                  port, autoPort, listenAddress, flags);
>  cleanup:
>      virURIFree(uri);
>      VIR_FREE(hostname);
> +    VIR_FREE(protocol);
> +    VIR_FREE(well_formed_protocol);
>      if (ret != 0) {
>          VIR_FREE(*uri_out);
>          if (autoPort)
> @@ -2844,6 +2877,7 @@ struct _qemuMigrationSpec {
>      enum qemuMigrationDestinationType destType;
>      union {
>          struct {
> +            const char *proto;
>              const char *name;
>              int port;
>          } host;
> @@ -3205,6 +3239,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
>      switch (spec->destType) {
>      case MIGRATION_DEST_HOST:
>          ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
> +                                       spec->dest.host.proto,
>                                         spec->dest.host.name,
>                                         spec->dest.host.port);
>          break;
> @@ -3335,7 +3370,7 @@ cancel:
>      goto cleanup;
>  }
>  
> -/* Perform migration using QEMU's native TCP migrate support,
> +/* Perform migration using QEMU's native migrate support,
>   * not encrypted obviously
>   */
>  static int doNativeMigrate(virQEMUDriverPtr driver,
> @@ -3353,7 +3388,11 @@ static int doNativeMigrate(virQEMUDriverPtr driver,
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      virURIPtr uribits = NULL;
>      int ret = -1;
> +    char *tmp = NULL;
>      qemuMigrationSpec spec;
> +    char *well_formed_proto = NULL;
> +    char * protocol_save = NULL;
> +    char * uri_save = NULL;
>  
>      VIR_DEBUG("driver=%p, vm=%p, uri=%s, cookiein=%s, cookieinlen=%d, "
>                "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, "
> @@ -3362,20 +3401,44 @@ static int doNativeMigrate(virQEMUDriverPtr driver,
>                cookieout, cookieoutlen, flags, resource,
>                NULLSTR(graphicsuri));
>  
> -    if (STRPREFIX(uri, "tcp:") && !STRPREFIX(uri, "tcp://")) {
> -        char *tmp;
> -        /* HACK: source host generates bogus URIs, so fix them up */
> -        if (virAsprintf(&tmp, "tcp://%s", uri + strlen("tcp:")) < 0)
> +    ret = VIR_STRDUP(uri_save, uri);
> +    if (ret <= 0) {
> +        return -1;
> +    }
> +
> +    spec.dest.host.proto = strtok_r(uri_save, ":", &protocol_save);
> +    VIR_FREE(uri_save);
> +
> +    /* HACK: source host generates bogus URIs, so fix them up */
> +    if (spec.dest.host.proto) {
> +        ret = virAsprintf(&well_formed_proto, "%s://",
> +                            spec.dest.host.proto);
> +        if (ret < 0)
> +            return ret;
> +    }
> +
> +    /* HACK: source host generates bogus URIs, so fix them up */
> +
> +    if (!STRPREFIX(uri, well_formed_proto)) {
> +        if (virAsprintf(&tmp, "%s://%s", spec.dest.host.proto,
> +                        uri + strlen(spec.dest.host.proto) + 1) < 0)
>              return -1;
> -        uribits = virURIParse(tmp);
> -        VIR_FREE(tmp);
>      } else {
>          uribits = virURIParse(uri);
>      }

And the same applies here. We should keep the hack only for "tcp:" URIs
and pass all other URIs just straight to virURIParse.

> -    if (!uribits)
> -        return -1;
>  
> -    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD))
> +    if (tmp) {
> +        uribits = virURIParse(tmp);
> +        VIR_FREE(tmp);
> +    }
> +
> +    if (!uribits) {
> +        ret = -1;
> +        goto err;
> +    }
> +
> +    if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATE_QEMU_FD) &&
> +            !STREQ(spec.dest.host.proto, "rdma"))

Indentation is 4 spaces off.

>          spec.destType = MIGRATION_DEST_CONNECT_HOST;
>      else
>          spec.destType = MIGRATION_DEST_HOST;
> @@ -3392,6 +3455,9 @@ static int doNativeMigrate(virQEMUDriverPtr driver,
>  
>      virURIFree(uribits);
>  
> +err:
> +    VIR_FREE(well_formed_proto);
> +
>      return ret;
>  }
>  
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 1514715..5a450e2 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2164,6 +2164,7 @@ int qemuMonitorMigrateToFd(qemuMonitorPtr mon,
>  
>  int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
>                               unsigned int flags,
> +                             const char *proto,
>                               const char *hostname,
>                               int port)
>  {
> @@ -2179,7 +2180,7 @@ int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
>      }
>  
>  
> -    if (virAsprintf(&uri, "tcp:%s:%d", hostname, port) < 0)
> +    if (virAsprintf(&uri, "%s:%s:%d", proto, hostname, port) < 0)
>          return -1;
>  
>      if (mon->json)
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 27f9cb4..16b0b77 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -476,6 +476,7 @@ int qemuMonitorMigrateToFd(qemuMonitorPtr mon,
>  
>  int qemuMonitorMigrateToHost(qemuMonitorPtr mon,
>                               unsigned int flags,
> +                             const char *proto,
>                               const char *hostname,
>                               int port);
>  
> diff --git a/src/util/viruri.c b/src/util/viruri.c
> index 35efad8..662029a 100644
> --- a/src/util/viruri.c
> +++ b/src/util/viruri.c
> @@ -187,7 +187,12 @@ virURIParse(const char *uri)
>          goto error;
>  
>      /* First check: does it even make sense to jump inside */
> -    if (ret->server != NULL &&
> +
> +    /*
> +     * IPv6 rdma over iwarp is broken in linux. Waiting for a
> +     * fix on the kernel side...
> +     */
> +    if (ret->server != NULL && !STREQ(ret->scheme, "rdma") &&
>          ret->server[0] == '[') {
>          size_t length = strlen(ret->server);

This change is definitely not OK. virURIParse is not a place such hacks.

Jirka


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