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

Re: [libvirt] [PATCH 1/1 V2] Migrate per-port data for Open vSwitch ports during Qemu Live Migration



On Sep 6, 2012, at 10:58 AM, Laine Stump wrote:
> On 09/04/2012 04:35 PM, Kyle Mestery wrote:
>> Add the ability to migrate per-port data on Open vSwitch
>> ports during qemu live migration. A controller can use this
>> to store data relating to each port, and have it migrated
>> with the virtual machine and populated on the destination
>> host.
> 
> Good job in figuring this out!
> 
>> 
>> Signed-off-by: Kyle Mestery <kmestery cisco com>
>> Cc: Laine Stump <laine laine org>
>> ---
>> src/qemu/qemu_migration.c | 246 +++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 244 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 1b21ef6..8c1a8f1 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -70,6 +70,7 @@ enum qemuMigrationCookieFlags {
>>     QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>>     QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>>     QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
>> +    QEMU_MIGRATION_COOKIE_FLAG_OVS_PORT_DATA,
>> 
>>     QEMU_MIGRATION_COOKIE_FLAG_LAST
>> };
>> @@ -77,12 +78,13 @@ enum qemuMigrationCookieFlags {
>> VIR_ENUM_DECL(qemuMigrationCookieFlag);
>> VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>>               QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> -              "graphics", "lockstate", "persistent");
>> +              "graphics", "lockstate", "persistent", "ovsportdata");
>> 
>> enum qemuMigrationCookieFeatures {
>>     QEMU_MIGRATION_COOKIE_GRAPHICS  = (1 << QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS),
>>     QEMU_MIGRATION_COOKIE_LOCKSTATE = (1 << QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE),
>>     QEMU_MIGRATION_COOKIE_PERSISTENT = (1 << QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT),
>> +    QEMU_MIGRATION_COOKIE_OVS_PORT_DATA = (1 << QEMU_MIGRATION_COOKIE_FLAG_OVS_PORT_DATA),
>> };
>> 
>> typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
>> @@ -95,6 +97,19 @@ struct _qemuMigrationCookieGraphics {
>>     char *tlsSubject;
>> };
>> 
>> +typedef struct _qemuMigrationCookieOvsPortData qemuMigrationCookieOvsPortData;
>> +typedef qemuMigrationCookieOvsPortData *qemuMigrationCookieOvsPortDataPtr;
>> +struct _qemuMigrationCookieOvsPortData {
>> +    /* How many virtual NICs are we saving data for? */
>> +    int nnets;
>> +
>> +    /*
>> +     * Array of pointers to saved data. Each VIF will have it's own
>> +     * data to transfer.
>> +     */
>> +    char **portdata;
>> +};
>> +
>> typedef struct _qemuMigrationCookie qemuMigrationCookie;
>> typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>> struct _qemuMigrationCookie {
>> @@ -120,6 +135,9 @@ struct _qemuMigrationCookie {
>> 
>>     /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
>>     virDomainDefPtr persistent;
>> +
>> +    /* If (flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) */
>> +    qemuMigrationCookieOvsPortDataPtr ovsportdata;
>> };
>> 
>> static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> @@ -132,6 +150,24 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> }
>> 
>> 
>> +static void qemuMigrationCookieOvsPortDataFree(qemuMigrationCookieOvsPortDataPtr
>> +                                               ovsportdata)
>> +{
>> +    int i;
>> +
>> +    if (!ovsportdata)
>> +        return;
>> +
>> +    for (i = 0; i < ovsportdata->nnets; i++) {
>> +        VIR_FREE(ovsportdata->portdata[i]);
>> +    }
>> +
>> +    VIR_FREE(ovsportdata->portdata);
>> +
>> +    VIR_FREE(ovsportdata);
>> +}
>> +
>> +
>> static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>> {
>>     if (!mig)
>> @@ -140,6 +176,10 @@ static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>>     if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS)
>>         qemuMigrationCookieGraphicsFree(mig->graphics);
>> 
>> +    if (mig->flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) {
>> +        qemuMigrationCookieOvsPortDataFree(mig->ovsportdata);
>> +    }
>> +
>>     VIR_FREE(mig->localHostname);
>>     VIR_FREE(mig->remoteHostname);
>>     VIR_FREE(mig->name);
>> @@ -256,6 +296,60 @@ error:
>> }
>> 
>> 
>> +static qemuMigrationCookieOvsPortDataPtr
>> +qemuMigrationCookieOvsPortDataAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED,
>> +                                    virDomainDefPtr def)
>> +{
>> +    qemuMigrationCookieOvsPortDataPtr mig;
>> +    int i;
>> +    virCommandPtr cmd = NULL;
>> +    virDomainNetDefPtr netptr;
>> +
>> +    if (VIR_ALLOC(mig) < 0)
>> +        goto no_memory;
>> +
>> +    mig->nnets = def->nnets;
>> +
>> +    if (VIR_ALLOC_N(mig->portdata, def->nnets) < 0)
>> +        goto no_memory;
>> +
>> +    for (i = 0; i < def->nnets; i++) {
>> +        virNetDevVPortProfilePtr vport = virDomainNetGetActualVirtPortProfile(def->nets[i]);
>> +        netptr = def->nets[i];
>> +
>> +        if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) {
>> +            if (VIR_ALLOC(mig->portdata[i]) < 0)
>> +                goto no_memory;
>> +
>> +            cmd = virCommandNewArgList(OVSVSCTL, "get", "Interface",
>> +                                       netptr->ifname, "external_ids:PortData",
>> +                                       NULL);
>> +
>> +            virCommandSetOutputBuffer(cmd, &mig->portdata[i]);
>> +
>> +            /* Run the command */
>> +            if (virCommandRun(cmd, NULL) < 0) {
>> +                virReportSystemError(VIR_ERR_INTERNAL_ERROR,
>> +                                     _("Unable to run command to get OVS port data for "
>> +                                     "interface %s"), netptr->ifname);
>> +                goto error;
>> +            }
>> +
>> +            /* Wipeout the newline */
>> +            mig->portdata[i][strlen(mig->portdata[i]) - 1] = '\0';
> 
> Both this run of ovs-vsctl and the one that re-populates the data at the
> remote end should be done by functions in virnetdevopenvswitch.c
> 
Hi Laine:

I'm in the process of reworking this patch along the lines you and Daniel have
provided input towards. I defined some helper functions in virnetdevopenvswitch.c,
but when calling them from qemu_migration.c, the build is failing during linking.
I suspect I need to add whatever gets built out of src/util to the qemu stuff, but before
going down that path, wanted to run this by the list. Here is the error I see:

make[3]: Leaving directory `/production/git/local/libvirt/libvirt/python'
Making all in tests
make[3]: Entering directory `/production/git/local/libvirt/libvirt/python/tests'
make[3]: Nothing to be done for `all'.
make[3]: Leaving directory `/production/git/local/libvirt/libvirt/python/tests'
make[2]: Leaving directory `/production/git/local/libvirt/libvirt/python'
Making all in tests
make[2]: Entering directory `/production/git/local/libvirt/libvirt/tests'
  CCLD   qemuxml2argvtest
../src/.libs/libvirt_driver_qemu_impl.a(libvirt_driver_qemu_impl_la-qemu_migration.o): In function `qemuMigrationCookieNetworkAlloc':
/production/git/local/libvirt/libvirt/src/qemu/qemu_migration.c:343: undefined reference to `virNetDevOpenvswitchGetMigrateData'
../src/.libs/libvirt_driver_qemu_impl.a(libvirt_driver_qemu_impl_la-qemu_migration.o): In function `qemuDomainMigrateOPDRelocate':
/production/git/local/libvirt/libvirt/src/qemu/qemu_migration.c:1302: undefined reference to `virNetDevOpenvswitchSetMigrateData'
collect2: error: ld returned 1 exit status
make[2]: *** [qemuxml2argvtest] Error 1
make[2]: Leaving directory `/production/git/local/libvirt/libvirt/tests'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/production/git/local/libvirt/libvirt'
make: *** [all] Error 2
[kmestery fedora-build libvirt]$ 

Any pointers appreciated! Once I get this addressed, I think the new patches are
ready for submission.

Thanks!
Kyle

>> +        }
>> +    }
>> +
>> +    return mig;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +error:
>> +    qemuMigrationCookieOvsPortDataFree(mig);
>> +    return NULL;
>> +}
>> +
>> +
>> static qemuMigrationCookiePtr
>> qemuMigrationCookieNew(virDomainObjPtr dom)
>> {
>> @@ -370,6 +464,27 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookiePtr mig,
>> }
>> 
>> 
>> +static int
>> +qemuMigrationCookieAddOvsPortData(qemuMigrationCookiePtr mig,
>> +                                  struct qemud_driver *driver,
>> +                                  virDomainObjPtr dom)
>> +{
>> +    if (mig->flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Migration ovs port data data already present"));
>> +        return -1;
>> +    }
>> +
>> +    if (dom->def->nnets >= 1) {
>> +        if (!(mig->ovsportdata = qemuMigrationCookieOvsPortDataAlloc(
>> +                                        driver, dom->def)))
>> +            return -1;
>> +        mig->flags |= QEMU_MIGRATION_COOKIE_OVS_PORT_DATA;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> 
>> static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf,
>>                                                  qemuMigrationCookieGraphicsPtr grap)
>> @@ -389,6 +504,25 @@ static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf,
>> }
>> 
>> 
>> +static void qemuMigrationCookieOvsPortDataXMLFormat(virBufferPtr buf,
>> +                                                    qemuMigrationCookieOvsPortDataPtr optr)
>> +{
>> +    int i;
>> +
>> +    virBufferAsprintf(buf, "  <ovsportdata nnets='%d'>\n", optr->nnets);
> 
> A minor point - is there a reason for including nnets here, when it can
> be implied from the number of <vif> elements and/or their "num" attributes?
> 
> But beyond that, I'm thinking we might want a more generic toplevel
> element name (similar to the graphics one being called <graphics> even
> though it could be used for vnc or spice), so that in the future it
> could be passed to / retrieved from a separate network driver/daemon
> without regard (within the migration code at least) to the specific type
> of the interface (or at least allow for the migration code to get
> multiple types without needing to clutter the namespace).
> 
> In the end, what we're really doing here is sending a last-instant
> update to the state of the domain's network interface. Thinking of it in
> that way, it would be really nice if it mimicked the <network> element
> of the domain XML as much as possible, to make explanations easier and
> make future additions more automatic, but I'm not sure how far we want
> to go in that direction, since the <network> element of the domain is
> unnecessarily complex for this specific job (e.g. it has both the
> configuration (which could be type='network') *and* the actual network
> connection that's used (which could have a different type if the config
> type is 'network'). Another difference specific to this case is that, in
> order to detect an interface that uses Open vSwitch, you need to look
> both for type='bridge' and for <virtualport type='openvswitch'>.
> 
> So, to take this to the very end of that road of reasoning, how bad of
> an idea would it be to make the "cookie" for each interface be the full
> live XML of that interface, then just include the portdata inside
> <virtualport> for every interface that has <virtualport
> type='openvswitch'>?
> 
> The advantage of doing it this way is that this would automatically
> handle any other need for last-minute interface state change info we
> might encounter in the future (as long as 1) the desired information is
> included in virDomainNetDef and handled by the standard
> parser/formatter, and 2) the cookie eating code did something with that
> data), and we could just call the parse/format functions in src/conf
> rather than needing our own. (A side effect is that "virsh dumpxml
> $myguest" would display the portdata for every Open vSwitch interface.
> Not having seen the length of the data, I don't know if that's a plus or
> a minus :-)
> 
> (Note that this would mean you would need to call
> virDomainNetGetActualVirtPortProfile() on the receiving end)
> 
> (really, I can see a potential need for this for other types of devices
> that need up-to-the-last-instant state info sent to the destination. But
> of course the more far-reaching you try to be, the bigger the chance
> you'll get it wrong :-P)
> 
> 
>> +    if (optr->nnets > 0)
>> +        virBufferAsprintf(buf, "    <vifs>\n");
> 
> Stepping back from my larger change idea above, does having the extra
> <vifs> level in the hierarchy make something easier (either now or in a
> hypothetical future with more data sent in this cookie)? Or could we
> just have the <vif> elements right below the toplevel?
> 
> Oh, and just to shake things up - we need to account for the possibility
> where the source host is using Open vSwitch, and the destination isn't :-)
> 
>> +    for (i = 0; i < optr->nnets; i++) {
>> +        virBufferAsprintf(buf, "      <vif num='%d' portdata='%s'/>\n",
>> +                          i, optr->portdata[i]);
>> +    }
>> +    if (optr->nnets > 0)
>> +        virBufferAsprintf(buf, "    </vifs>\n");
>> +
>> +    virBufferAddLit(buf, "  </ovsportdata>\n");
>> +}
>> +
>> +
>> static int
>> qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
>>                              virBufferPtr buf,
>> @@ -439,6 +573,10 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
>>         virBufferAdjustIndent(buf, -2);
>>     }
>> 
>> +    if ((mig->flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) &&
>> +        mig->ovsportdata)
>> +        qemuMigrationCookieOvsPortDataXMLFormat(buf, mig->ovsportdata);
>> +
>>     virBufferAddLit(buf, "</qemu-migration>\n");
>>     return 0;
>> }
>> @@ -516,6 +654,57 @@ error:
>> }
>> 
>> 
>> +static qemuMigrationCookieOvsPortDataPtr
>> +qemuMigrationCookieOvsPortDataXMLParse(xmlXPathContextPtr ctxt)
>> +{
>> +    qemuMigrationCookieOvsPortDataPtr optr;
>> +    int i;
>> +    int n;
>> +    xmlNodePtr *vifs = NULL;
>> +
>> +    if (VIR_ALLOC(optr) < 0)
>> +        goto no_memory;
>> +
>> +    if (virXPathInt("string(./ovsportdata/@nnets)", ctxt, &optr->nnets) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s", _("missing nnets attribute in migration data"));
>> +        goto error;
>> +    }
>> +
>> +    if (VIR_ALLOC_N(optr->portdata, optr->nnets) < 0)
>> +        goto no_memory;
>> +
>> +    if ((n = virXPathNodeSet("./ovsportdata/vifs/vif", ctxt, &vifs)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s", _("missing vif information"));
>> +        goto error;
>> +    }
>> +
>> +    for (i = 0; i < n; i++) {
>> +        if (VIR_ALLOC(optr->portdata[i]) < 0)
>> +            goto no_memory;
>> +
>> +        if (!(optr->portdata[i] = virXMLPropString(vifs[i], "portdata"))) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           "%s", _("missing portdata attribute in migration data"));
>> +            goto error;
>> +        }
>> +    }
>> +
>> +    VIR_FREE(vifs);
>> +
>> +    return optr;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +error:
>> +    if (vifs)
>> +        VIR_FREE(vifs);
>> +    qemuMigrationCookieOvsPortDataFree(optr);
>> +    return NULL;
>> +}
>> +
>> +
>> static int
>> qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>>                             struct qemud_driver *driver,
>> @@ -662,6 +851,11 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>>         VIR_FREE(nodes);
>>     }
>> 
>> +    if ((flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) &&
>> +        virXPathBoolean("count(./ovsportdata) > 0", ctxt) &&
>> +        (!(mig->ovsportdata = qemuMigrationCookieOvsPortDataXMLParse(ctxt))))
>> +        goto error;
>> +
>>     return 0;
>> 
>> error:
>> @@ -721,6 +915,10 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig,
>>         qemuMigrationCookieAddPersistent(mig, dom) < 0)
>>         return -1;
>> 
>> +    if (flags & QEMU_MIGRATION_COOKIE_OVS_PORT_DATA &&
>> +        qemuMigrationCookieAddOvsPortData(mig, driver, dom) < 0)
>> +        return -1;
>> +
>>     if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig)))
>>         return -1;
>> 
>> @@ -1050,6 +1248,44 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver,
>> }
>> 
>> 
>> +static int
>> +qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED,
>> +                             virDomainObjPtr vm,
>> +                             qemuMigrationCookiePtr cookie)
>> +{
>> +    virCommandPtr cmd = NULL;
>> +    virDomainNetDefPtr netptr;
>> +    int ret = 0;
>> +    int i;
>> +    virBufferPtr buf;
>> +
>> +    if (VIR_ALLOC(buf) < 0) {
>> +        ret = -1;
>> +        goto out;
>> +    }
>> +
>> +    for (i = 0; i < cookie->ovsportdata->nnets; i++) {
>> +        netptr = vm->def->nets[i];
>> +
>> +        virBufferAsprintf(buf, "external_ids:PortData=%s",
>> +                          cookie->ovsportdata->portdata[i]);
>> +
>> +        cmd = virCommandNewArgList(OVSVSCTL, "set", "Interface",
>> +                                   netptr->ifname, virBufferCurrentContent(buf),
>> +                                   NULL);
>> +        /* Run the command */
>> +        if ((ret = virCommandRun(cmd, NULL)) < 0) {
>> +            virReportSystemError(VIR_ERR_INTERNAL_ERROR,
>> +                                 _("Unable to run command to set OVS port data for "
>> +                                 "interface %s"), vm->def->nets[i]->ifname);
>> +        }
>> +    }
>> +
>> +out:
>> +    return ret;
>> +}
>> +
>> +
>> /* This is called for outgoing non-p2p migrations when a connection to the
>>  * client which initiated the migration was closed but we were waiting for it
>>  * to follow up with the next phase, that is, in between
>> @@ -1994,7 +2230,8 @@ cleanup:
>> 
>>     if (ret == 0 &&
>>         qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen,
>> -                                QEMU_MIGRATION_COOKIE_PERSISTENT ) < 0)
>> +                                QEMU_MIGRATION_COOKIE_PERSISTENT |
>> +                                QEMU_MIGRATION_COOKIE_OVS_PORT_DATA) < 0)
>>         VIR_WARN("Unable to encode migration cookie");
>> 
>>     qemuMigrationCookieFree(mig);
>> @@ -2929,6 +3166,7 @@ qemuMigrationFinish(struct qemud_driver *driver,
>> 
>>     qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup);
>> 
>> +    cookie_flags = QEMU_MIGRATION_COOKIE_OVS_PORT_DATA;
>>     if (flags & VIR_MIGRATE_PERSIST_DEST)
>>         cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
>> 
>> @@ -2956,6 +3194,10 @@ qemuMigrationFinish(struct qemud_driver *driver,
>>             goto endjob;
>>         }
>> 
>> +        if (mig->ovsportdata)
>> +            if (qemuDomainMigrateOPDRelocate(driver, vm, mig) < 0)
>> +                VIR_WARN("unable to provide data for OVS port data relocation");
>> +
>>         if (flags & VIR_MIGRATE_PERSIST_DEST) {
>>             virDomainDefPtr vmdef;
>>             if (vm->persistent)
> 
> In general ACK to the idea, but I think we should try to setup a more
> general framework so that other types of network connections can plug in
> their own data without needing to start over from scratch (and also to
> isolate things like calling "ovs-vsctl" to virnetdevopenvswitch.c).
> 
> I would like to hear others' opinions on whether or not they think it's
> reasonable to include the entire <network> live XML in the cookie, or if
> they think that's going beyond the bounds of the intended purpose of the
> migration cookie. I don't know if we should go all the way in that
> direction, but possibly we should at least make it a *subset* of the
> full domain <network> xml (as is done with the <graphics> cookie).
> 




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