[libvirt] [PATCH 2/3] qemu: RDMA migration support using 'x-rdma' URI
Michael R. Hines
mrhines at linux.vnet.ibm.com
Fri Jul 26 18:48:42 UTC 2013
On 07/26/2013 02:27 PM, Jiri Denemark wrote:
> On Fri, Jul 26, 2013 at 13:47:44 -0400, mrhines at linux.vnet.ibm.com wrote:
>> From: "Michael R. Hines" <mrhines at us.ibm.com>
>>
>> QEMU has in tree now for version 1.6 support for RDMA Live migration.
>> 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.
>>
>> While the RDMA protocol has been extensively tested (from multiple
>> companies as well as virt-test), the protocol 'x-rdma' will later be
>> renamed to 'rdma' after the community has allowed the feature more cooking.
> This does not prevent us from calling the protocol "rdma" right away and
> possibly translating it to "x-rdma". However, I don't think we actually
> want to commit patches for rdma migration before QEMU changes the name
> to "rdma".
Acknowledged.
>> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
>> index 5dc3c9e..94d17c6 100644
>> --- a/src/qemu/qemu_capabilities.c
>> +++ b/src/qemu/qemu_capabilities.c
>> @@ -234,6 +234,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST,
>>
>> "vnc-share-policy", /* 150 */
>> "device-del-event",
>> +
>> + "x-rdma", /* 152 */
>> );
>>
>> struct _virQEMUCaps {
>> @@ -1101,6 +1103,7 @@ virQEMUCapsComputeCmdFlags(const char *help,
>> * -incoming unix (qemu >= 0.12.0)
>> * -incoming fd (qemu >= 0.12.0)
>> * -incoming stdio (all earlier kvm)
>> + * -incoming x-rdma (qemu >= 1.6.0)
>> *
>> * NB, there was a pre-kvm-79 'tcp' support, but it
>> * was broken, because it blocked the monitor console
>> @@ -2437,6 +2440,10 @@ virQEMUCapsInitArchQMPBasic(virQEMUCapsPtr qemuCaps,
>> char *archstr = NULL;
>> int ret = -1;
>>
>> + if (qemuCaps->version >= MIN_X_RDMA_VERSION) {
>> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MIGRATE_QEMU_X_RDMA);
>> + }
>> +
>> if (!(archstr = qemuMonitorGetTargetArch(mon)))
>> return -1;
>>
> This is wrong. First, you're adding this into a totally wrong place and
> second, we need a better detection which is not based on qemu version.
How would we detect without using the QEMU version?
This feature doesn't have any new command-line arguments
(except for -incoming ....)
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 19001b9..de20d23 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -2169,7 +2169,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>> virDomainDefPtr *def,
>> virStreamPtr st,
>> unsigned int port,
>> - unsigned long flags)
>> + unsigned long flags,
>> + const char *protocol)
>> {
>> virDomainObjPtr vm = NULL;
>> virDomainEventPtr event = NULL;
>> @@ -2280,7 +2281,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>> * and there is at least one IPv6 address configured
>> */
>> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_IPV6_MIGRATION) &&
>> - getaddrinfo("::", NULL, &hints, &info) == 0) {
>> + getaddrinfo("::", NULL, &hints, &info) == 0 &&
>> + !strstr(protocol, "rdma")) {
>> freeaddrinfo(info);
>> listenAddr = "[::]";
>> } else {
> Is there any reason why RDMA migration does not work over IPv6?
Laziness on my part - It was never implemented because QEMU's parsing of
the "[::]" brackets is hard-coded for TCP/IP.
I'll submit a patch to break this out on qemu-devel.
>> @@ -2291,7 +2293,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>> /* QEMU will be started with -incoming [::]:port
>> * or -incoming 0.0.0.0:port
>> */
>> - if (virAsprintf(&migrateFrom, "tcp:%s:%d", listenAddr, port) < 0)
>> + if (virAsprintf(&migrateFrom, "%s:%s:%d", protocol,
>> + listenAddr, port) < 0)
>> goto cleanup;
>> }
>>
>> @@ -2482,7 +2485,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
>>
>> ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
>> cookieout, cookieoutlen, def,
>> - st, 0, flags);
>> + st, 0, flags, "tcp");
>> return ret;
>> }
>>
>> @@ -2502,6 +2505,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>> static int port = 0;
>> int this_port;
>> char *hostname = NULL;
>> + const char *protocol = NULL;
>> + char *well_formed_protocol = NULL;
>> const char *p;
>> char *uri_str = NULL;
>> int ret = -1;
>> @@ -2550,20 +2555,29 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>> if (virAsprintf(uri_out, "tcp:%s:%d", hostname, this_port) < 0)
>> goto cleanup;
>> } else {
>> - /* Check the URI starts with "tcp:". We will escape the
>> + /* 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"));
>> +
>> + protocol = strtok(strdup(uri_in), ":");
>> + 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, "x-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 tcp: */
>> - if (!(STRPREFIX(uri_in, "tcp://"))) {
>> - if (virAsprintf(&uri_str, "tcp://%s", p) < 0)
>> +
>> + /* Convert uri_in to well-formed URI with // after colon */
>> + if (!(STRPREFIX(uri_in, well_formed_protocol))) {
>> + if (virAsprintf(&uri_str, "%s://%s", protocol, p) < 0)
>> goto cleanup;
>> }
> Just because we did the mistake with tcp protocol we don't have to
> repeat it with rdma. Just change the rdma protocol to always be
> well-formed, i.e., rdma://host
Having a conditional check only for 'rdma' is what I was trying to avoid.
Wouldn't it be better to have both options available?
Having both choices is kind of nice - and it's unlikely that users will
stop breaking
the habit for a while.
>>
>> @@ -2602,10 +2616,20 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>>
>> ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
>> cookieout, cookieoutlen, def,
>> - NULL, this_port, flags);
>> + NULL, this_port, flags,
>> + protocol ? protocol : "tcp");
>> cleanup:
>> virURIFree(uri);
>> VIR_FREE(hostname);
>> +
>> + if (protocol) {
>> + VIR_FREE(protocol);
>> + }
>> +
>> + if (well_formed_protocol) {
>> + VIR_FREE(well_formed_protocol);
>> + }
>> +
> You're not supposed to check if a variable you're about to free is
> non-NULL. Running make syntax-check would have warned you.
Understood =)
More information about the libvir-list
mailing list