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

Re: [libvirt] [PATCHv4 1/3] qemu_migration: Add hooks to transport network data during migration



On 10/23/2012 08:07 AM, Michal Privoznik wrote:
> On 22.10.2012 23:30, Laine Stump wrote:
>> From: Kyle Mestery <kmestery cisco com>
>>
>> Add the ability for the Qemu V3 migration protocol to
>> include transporting network configuration. A generic
>> framework is proposed with this patch to allow for the
>> transfer of opaque data.
>>
>> Signed-off-by: Kyle Mestery <kmestery cisco com>
>> ---
>>  src/qemu/qemu_migration.c | 238 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 236 insertions(+), 2 deletions(-)
> ACK but see my comments below.
>
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 487182e..67276f0 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1,3 +1,4 @@
>> +
>>  /*
>>   * qemu_migration.c: QEMU migration handling
>>   *
>> @@ -70,6 +71,7 @@ enum qemuMigrationCookieFlags {
>>      QEMU_MIGRATION_COOKIE_FLAG_GRAPHICS,
>>      QEMU_MIGRATION_COOKIE_FLAG_LOCKSTATE,
>>      QEMU_MIGRATION_COOKIE_FLAG_PERSISTENT,
>> +    QEMU_MIGRATION_COOKIE_FLAG_NETWORK,
>>  
>>      QEMU_MIGRATION_COOKIE_FLAG_LAST
>>  };
>> @@ -77,12 +79,13 @@ enum qemuMigrationCookieFlags {
>>  VIR_ENUM_DECL(qemuMigrationCookieFlag);
>>  VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>>                QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> -              "graphics", "lockstate", "persistent");
>> +              "graphics", "lockstate", "persistent", "network");
>>  
>>  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_NETWORK = (1 << QEMU_MIGRATION_COOKIE_FLAG_NETWORK),
>>  };
>>  
>>  typedef struct _qemuMigrationCookieGraphics qemuMigrationCookieGraphics;
>> @@ -95,6 +98,27 @@ struct _qemuMigrationCookieGraphics {
>>      char *tlsSubject;
>>  };
>>  
>> +typedef struct _qemuMigrationCookieNetdata qemuMigrationCookieNetdata;
>> +typedef qemuMigrationCookieNetdata *qemuMigrationCookieNetdataPtr;
>> +struct _qemuMigrationCookieNetdata {
>> +    int vporttype; /* enum virNetDevVPortProfile */
>> +
>> +    /*
>> +     * Array of pointers to saved data. Each VIF will have it's own
>> +     * data to transfer.
>> +     */
>> +    char *portdata;
>> +};
>> +
>> +typedef struct _qemuMigrationCookieNetwork qemuMigrationCookieNetwork;
>> +typedef qemuMigrationCookieNetwork *qemuMigrationCookieNetworkPtr;
>> +struct _qemuMigrationCookieNetwork {
>> +    /* How many virtual NICs are we saving data for? */
>> +    int nnets;
>> +
>> +    qemuMigrationCookieNetdataPtr net;
>> +};
>> +
>>  typedef struct _qemuMigrationCookie qemuMigrationCookie;
>>  typedef qemuMigrationCookie *qemuMigrationCookiePtr;
>>  struct _qemuMigrationCookie {
>> @@ -120,6 +144,9 @@ struct _qemuMigrationCookie {
>>  
>>      /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
>>      virDomainDefPtr persistent;
>> +
>> +    /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */
>> +    qemuMigrationCookieNetworkPtr network;
>>  };
>>  
>>  static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>> @@ -132,6 +159,23 @@ static void qemuMigrationCookieGraphicsFree(qemuMigrationCookieGraphicsPtr grap)
>>  }
>>  
>>  
>> +static void
>> +qemuMigrationCookieNetworkFree(qemuMigrationCookieNetworkPtr network)
>> +{
>> +    int i;
>> +
>> +    if (!network)
>> +        return;
>> +
>> +    if (network->net) {
>> +        for (i = 0; i < network->nnets; i++)
>> +            VIR_FREE(network->net[i].portdata);
>> +    }
> You could have spared one level of nesting if you'd just only ... [1]

Well, what would have saved another level of nesting would be if nnets
was part of qemuMigrationCookie instead of qemuMigrationCookieNetwork. I
had already removed one extra layer of nesting when I updated Kyle's
patches, and thought of making this change as well, but kind of liked
the consistency of all the "sub-cookies" being a single pointer that
could be checked for NULL:

  struct _qemuMigrationCookieNetdata {
      int vporttype; /* enum virNetDevVPortProfile */
      char *portdata;
  };

  struct _qemuMigrationCookieNetwork {
      int nnets;
      qemuMigrationCookieNetdataPtr net;
  };

  struct _qemuMigrationCookie {
   ... 
      /* If (flags & QEMU_MIGRATION_COOKIE_GRAPHICS) */
      qemuMigrationCookieGraphicsPtr graphics;

      /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
      virDomainDefPtr persistent;

      /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */
      qemuMigrationCookieNetworkPtr network;
  };

On the other hand, I noticed at the last minute that there is a
LOCKSTATE cookie that *doesn't* follow this pattern:

    /* If (flags & QEMU_MIGRATION_COOKIE_LOCKSTATE) */
    char *lockState;
    char *lockDriver;

so I'd be just as happy with this:

  struct _qemuMigrationCookie {
   ...
     /* If (flags & QEMU_MIGRATION_COOKIE_GRAPHICS) */
     qemuMigrationCookieGraphicsPtr graphics;
     /* If (flags & QEMU_MIGRATION_COOKIE_PERSISTENT) */
     virDomainDefPtr persistent;
     /* If (flags & QEMU_MIGRATION_COOKIE_NETWORK) */
     size_t nnets;
     qemuMigrationCookieNetdataPtr net;
  };

(i.e. eliminating the qemuMigrationCookieNet object completely)


>> +    VIR_FREE(network->net);
>> +    VIR_FREE(network);
>> +}
>> +
>> +
>>  static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>>  {
>>      if (!mig)
>> @@ -140,6 +184,10 @@ static void qemuMigrationCookieFree(qemuMigrationCookiePtr mig)
>>      if (mig->flags & QEMU_MIGRATION_COOKIE_GRAPHICS)
>>          qemuMigrationCookieGraphicsFree(mig->graphics);
>>  
>> +    if (mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) {
>> +        qemuMigrationCookieNetworkFree(mig->network);
>> +    }
> These curly braces aren't required. But it is not against coding style
> neither.
>
>> +
>>      VIR_FREE(mig->localHostname);
>>      VIR_FREE(mig->remoteHostname);
>>      VIR_FREE(mig->name);
>> @@ -256,6 +304,49 @@ error:
>>  }
>>  
>>  
>> +static qemuMigrationCookieNetworkPtr
>> +qemuMigrationCookieNetworkAlloc(struct qemud_driver *driver ATTRIBUTE_UNUSED,
>> +                                virDomainDefPtr def)
>> +{
>> +    qemuMigrationCookieNetworkPtr mig;
>> +    int i;
>> +
>> +    if (VIR_ALLOC(mig) < 0)
>> +        goto no_memory;
>> +
>> +    mig->nnets = def->nnets;
> [1]: ... set this after the VIR_ALLOC_N below.

Not sure how that by itself solves the problem.


>> +
>> +    if (VIR_ALLOC_N(mig->net, def->nnets) <0)
>> +        goto no_memory;
>> +
>> +    for (i = 0; i < def->nnets; i++) {
>> +        virDomainNetDefPtr netptr;
>> +        virNetDevVPortProfilePtr vport;
>> +
>> +        netptr = def->nets[i];
>> +        vport = virDomainNetGetActualVirtPortProfile(netptr);
>> +
>> +        if (vport) {
>> +            mig->net[i].vporttype = vport->virtPortType;
>> +
>> +            switch (vport->virtPortType) {
>> +            case VIR_NETDEV_VPORT_PROFILE_NONE:
>> +            case VIR_NETDEV_VPORT_PROFILE_8021QBG:
>> +            case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>> +            case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
>> +            default:
>> +                break;
>> +            }
>> +        }
>> +    }
>> +    return mig;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +    qemuMigrationCookieNetworkFree(mig);
>> +    return NULL;
>> +}
>> +
>>  static qemuMigrationCookiePtr
>>  qemuMigrationCookieNew(virDomainObjPtr dom)
>>  {
>> @@ -370,6 +461,27 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookiePtr mig,
>>  }
>>  
>>  
>> +static int
>> +qemuMigrationCookieAddNetwork(qemuMigrationCookiePtr mig,
>> +                              struct qemud_driver *driver,
>> +                              virDomainObjPtr dom)
>> +{
>> +    if (mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("Network migration data already present"));
>> +        return -1;
>> +    }
>> +
>> +    if (dom->def->nnets > 0) {
>> +        mig->network = qemuMigrationCookieNetworkAlloc(driver, dom->def);
>> +        if (!mig->network)
>> +            return -1;
>> +        mig->flags |= QEMU_MIGRATION_COOKIE_NETWORK;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  
>>  static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf,
>>                                                   qemuMigrationCookieGraphicsPtr grap)
>> @@ -389,6 +501,32 @@ static void qemuMigrationCookieGraphicsXMLFormat(virBufferPtr buf,
>>  }
>>  
>>  
>> +static void
>> +qemuMigrationCookieNetworkXMLFormat(virBufferPtr buf,
>> +                                    qemuMigrationCookieNetworkPtr optr)
>> +{
>> +    int i;
>> +
>> +    virBufferAsprintf(buf, "  <network>\n");
>> +    for (i = 0; i < optr->nnets; i++) {
>> +        /* If optr->net.vporttype[i] is not set, there is nothing to transfer */
> I believe '[i]' wants to be moved to 'net' to match the condition below.
>
>> +        if (optr->net[i].vporttype != VIR_NETDEV_VPORT_PROFILE_NONE) {
>> +            virBufferAsprintf(buf, "    <interface index='%d' vporttype='%s'",
>> +                              i, virNetDevVPortTypeToString(optr->net[i].vporttype));
>> +            if (optr->net[i].portdata) {
>> +                virBufferAddLit(buf, ">\n");
>> +                virBufferEscapeString(buf, "      <portdata>%s</portdata>\n",
>> +                                      optr->net[i].portdata);
>> +                virBufferAddLit(buf, "    </interface>\n");
>> +            } else {
>> +                virBufferAddLit(buf, "/>\n");
>> +            }
>> +        }
>> +    }
>> +    virBufferAddLit(buf, "  </network>\n");
>> +}
>> +
>> +
> Maybe if all 'net[i].vporttype' are VIR_NETDEV_VPORT_PROFILE_NONE so
> there is no <interface/> added, we don't need to add empty <network/>
> neither. But that just cosmetics and I can live with this version as-is.


I was a bit bothered by that extra object in there anyway - I'm going to
respin to remove it - the array will be directly off the main cookie object.


>>  static int
>>  qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
>>                               virBufferPtr buf,
>> @@ -439,6 +577,9 @@ qemuMigrationCookieXMLFormat(struct qemud_driver *driver,
>>          virBufferAdjustIndent(buf, -2);
>>      }
>>  
>> +    if ((mig->flags & QEMU_MIGRATION_COOKIE_NETWORK) && mig->network)
>> +        qemuMigrationCookieNetworkXMLFormat(buf, mig->network);
>> +
>>      virBufferAddLit(buf, "</qemu-migration>\n");
>>      return 0;
>>  }
>> @@ -516,6 +657,58 @@ error:
>>  }
>>  
>>  
>> +static qemuMigrationCookieNetworkPtr
>> +qemuMigrationCookieNetworkXMLParse(xmlXPathContextPtr ctxt)
>> +{
>> +    qemuMigrationCookieNetworkPtr optr;
>> +    int i;
>> +    int n;
>> +    xmlNodePtr *interfaces = NULL;
>> +    char *vporttype;
>> +    xmlNodePtr save_ctxt = ctxt->node;
>> +
>> +    if (VIR_ALLOC(optr) < 0)
>> +        goto no_memory;
>> +
>> +    if ((n = virXPathNodeSet("./network/interface", ctxt, &interfaces)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       "%s", _("missing interface information"));
>> +        goto error;
>> +    }
>> +
>> +    optr->nnets = n;
>> +    if (VIR_ALLOC_N(optr->net, optr->nnets) <0)
>> +        goto no_memory;
>> +
>> +    for (i = 0; i < n; i++) {
>> +        /* portdata is optional, and may not exist */
>> +        ctxt->node = interfaces[i];
>> +        optr->net[i].portdata = virXPathString("string(./portdata[1])", ctxt);
>> +
>> +        if (!(vporttype = virXMLPropString(interfaces[i], "vporttype"))) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           "%s", _("missing vporttype attribute in migration data"));
>> +            goto error;
>> +        }
>> +        optr->net[i].vporttype = virNetDevVPortTypeFromString(vporttype);
>> +    }
>> +
>> +    VIR_FREE(interfaces);
>> +
>> +cleanup:
>> +    ctxt->node = save_ctxt;
>> +    return optr;
>> +
>> +no_memory:
>> +    virReportOOMError();
>> +error:
>> +    VIR_FREE(interfaces);
>> +    qemuMigrationCookieNetworkFree(optr);
>> +    optr = NULL;
>> +    goto cleanup;
>> +}
>> +
>> +
>>  static int
>>  qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>>                              struct qemud_driver *driver,
>> @@ -663,6 +856,11 @@ qemuMigrationCookieXMLParse(qemuMigrationCookiePtr mig,
>>          VIR_FREE(nodes);
>>      }
>>  
>> +    if ((flags & QEMU_MIGRATION_COOKIE_NETWORK) &&
>> +        virXPathBoolean("count(./network) > 0", ctxt) &&
>> +        (!(mig->network = qemuMigrationCookieNetworkXMLParse(ctxt))))
>> +        goto error;
>> +
>>      return 0;
>>  
>>  error:
>> @@ -722,6 +920,10 @@ qemuMigrationBakeCookie(qemuMigrationCookiePtr mig,
>>          qemuMigrationCookieAddPersistent(mig, dom) < 0)
>>          return -1;
>>  
>> +    if (flags & QEMU_MIGRATION_COOKIE_NETWORK &&
>> +        qemuMigrationCookieAddNetwork(mig, driver, dom) < 0)
>> +        return -1;
>> +
>>      if (!(*cookieout = qemuMigrationCookieXMLFormatStr(driver, mig)))
>>          return -1;
>>  
>> @@ -1084,6 +1286,32 @@ qemuDomainMigrateGraphicsRelocate(struct qemud_driver *driver,
>>  }
>>  
>>  
>> +static int
>> +qemuDomainMigrateOPDRelocate(struct qemud_driver *driver ATTRIBUTE_UNUSED,
>> +                             virDomainObjPtr vm,
>> +                             qemuMigrationCookiePtr cookie)
>> +{
>> +    virDomainNetDefPtr netptr ATTRIBUTE_UNUSED;
>> +    int ret = 0;
>> +    int i;
>> +
>> +    for (i = 0; i < cookie->network->nnets; i++) {
>> +        netptr = vm->def->nets[i];
>> +
>> +        switch (cookie->network->net[i].vporttype) {
>> +        case VIR_NETDEV_VPORT_PROFILE_NONE:
>> +        case VIR_NETDEV_VPORT_PROFILE_8021QBG:
>> +        case VIR_NETDEV_VPORT_PROFILE_8021QBH:
>> +        case VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH:
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +
>> +    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
>> @@ -2029,7 +2257,8 @@ cleanup:
>>  
>>      if (ret == 0 &&
>>          qemuMigrationBakeCookie(mig, driver, vm, cookieout, cookieoutlen,
>> -                                QEMU_MIGRATION_COOKIE_PERSISTENT ) < 0)
>> +                                QEMU_MIGRATION_COOKIE_PERSISTENT |
>> +                                QEMU_MIGRATION_COOKIE_NETWORK) < 0)
>>          VIR_WARN("Unable to encode migration cookie");
>>  
>>      qemuMigrationCookieFree(mig);
>> @@ -2963,6 +3192,7 @@ qemuMigrationFinish(struct qemud_driver *driver,
>>  
>>      qemuDomainCleanupRemove(vm, qemuMigrationPrepareCleanup);
>>  
>> +    cookie_flags = QEMU_MIGRATION_COOKIE_NETWORK;
>>      if (flags & VIR_MIGRATE_PERSIST_DEST)
>>          cookie_flags |= QEMU_MIGRATION_COOKIE_PERSISTENT;
>>  
>> @@ -2990,6 +3220,10 @@ qemuMigrationFinish(struct qemud_driver *driver,
>>              goto endjob;
>>          }
>>  
>> +        if (mig->network)
>> +            if (qemuDomainMigrateOPDRelocate(driver, vm, mig) < 0)
>> +                VIR_WARN("unable to provide network data for relocation");
>> +
>>          if (flags & VIR_MIGRATE_PERSIST_DEST) {
>>              virDomainDefPtr vmdef;
>>              if (vm->persistent)
>>
>


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