Re: [Xen-devel] [PATCH v2] libxl: add p2p migration

[Joao Martins <joao.m.martins@xxxxxxxxxx>]

On 12/02/2015 11:27 PM, Jim Fehlig wrote:
> On 11/10/2015 08:32 AM, Joao Martins wrote:
>> Introduce support for VIR_MIGRATE_PEER2PEER in libxl driver
>> for supporting migration in Openstack. Most of the changes
>> occur at the source and no modifications at the receiver.
>>
>> In P2P mode there is only the Perform phase so we must handle
>> the connection with the destination and actually perform the
>> migration. libxlDomainPerformP2P implements the connection to
>> the destination and let libxlDoMigrateP2P implements the actual
>> migration logic with virConnectPtr. In this function we do
>> the migration steps in the destination similar to
>> virDomainMigrateVersion3Full. We appropriately save the last
>> error reported in each of the phases to provide proper
>> reporting. We don't yet support VIR_MIGRATE_TUNNELED and
>> we always use V3 with extensible params, making the
>> implementation simpler.
>>
>> It is worth noting that the receiver didn't have any changes,
>> and because it's still the v3 sequence thus it is possible to
>> migrate from a P2P to non-P2P host.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>> ---
>> Changes since v1:
>>  - Move Begin step to libxlDoMigrateP2P to have all 4 steps
>>  together.
>>  - Remove if before VIR_FREE(dom_xml)
>> ---
>>  src/libxl/libxl_driver.c    |  13 ++-
>>  src/libxl/libxl_migration.c | 220 
>> ++++++++++++++++++++++++++++++++++++++++++++
>>  src/libxl/libxl_migration.h |  11 +++
>>  3 files changed, 241 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index fcdcbdb..da98265 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4713,6 +4713,7 @@ libxlConnectSupportsFeature(virConnectPtr conn, int 
>> feature)
>>      switch (feature) {
>>      case VIR_DRV_FEATURE_TYPED_PARAM_STRING:
>>      case VIR_DRV_FEATURE_MIGRATION_PARAMS:
>> +    case VIR_DRV_FEATURE_MIGRATION_P2P:
>>          return 1;
>>      default:
>>          return 0;
>> @@ -5039,9 +5040,15 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
>>      if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
>>          goto cleanup;
>>  
>> -    if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri,
>> -                                    uri, dname, flags) < 0)
>> -        goto cleanup;
>> +    if (flags & VIR_MIGRATE_PEER2PEER) {
>> +        if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
>> +                                           dconnuri, uri, dname, flags) < 0)
>> +            goto cleanup;
>> +    } else {
>> +        if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri,
>> +                                        uri, dname, flags) < 0)
>> +            goto cleanup;
>> +    }
>>  
>>      ret = 0;
>>  
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 0d23e5f..a1c7b55 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -42,6 +42,7 @@
>>  #include "libxl_conf.h"
>>  #include "libxl_migration.h"
>>  #include "locking/domain_lock.h"
>> +#include "virtypedparam.h"
>>  
>>  #define VIR_FROM_THIS VIR_FROM_LIBXL
>>  
>> @@ -456,6 +457,225 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>>      return ret;
>>  }
>>  
>> +/* This function is a simplification of virDomainMigrateVersion3Full
>> + * excluding tunnel support and restricting it to migration v3
>> + * with params since it was the first to be introduced in libxl.
>> + */
>> +static int
>> +libxlDoMigrateP2P(libxlDriverPrivatePtr driver,
>> +                  virDomainObjPtr vm,
>> +                  virConnectPtr sconn,
>> +                  const char *xmlin,
>> +                  virConnectPtr dconn,
>> +                  const char *dconnuri ATTRIBUTE_UNUSED,
>> +                  const char *dname,
>> +                  const char *uri,
>> +                  unsigned int flags)
>> +{
>> +    virDomainPtr ddomain = NULL;
>> +    virTypedParameterPtr params = NULL;
>> +    int nparams = 0;
>> +    int maxparams = 0;
>> +    char *uri_out = NULL;
>> +    char *dom_xml = NULL;
>> +    unsigned long destflags;
>> +    bool cancelled = true;
>> +    virErrorPtr orig_err = NULL;
>> +    int ret = -1;
>> +
>> +    dom_xml = libxlDomainMigrationBegin(sconn, vm, xmlin);
>> +    if (!dom_xml)
>> +        goto cleanup;
>> +
>> +    if (virTypedParamsAddString(&params, &nparams, &maxparams,
>> +                                VIR_MIGRATE_PARAM_DEST_XML, dom_xml) < 0)
>> +        goto cleanup;
>> +
>> +    if (dname &&
>> +        virTypedParamsAddString(&params, &nparams, &maxparams,
>> +                                VIR_MIGRATE_PARAM_DEST_NAME, dname) < 0)
>> +        goto cleanup;
>> +
>> +    if (uri &&
>> +        virTypedParamsAddString(&params, &nparams, &maxparams,
>> +                                VIR_MIGRATE_PARAM_URI, uri) < 0)
>> +        goto cleanup;
>> +
>> +    /* We don't require the destination to have P2P support
>> +     * as it looks to be normal migration from the receiver perpective.
>> +     */
>> +    destflags = flags & ~(VIR_MIGRATE_PEER2PEER);
>> +
>> +    VIR_DEBUG("Prepare3");
>> +    virObjectUnlock(vm);
>> +    ret = dconn->driver->domainMigratePrepare3Params
>> +        (dconn, params, nparams, NULL, 0, NULL, NULL, &uri_out, destflags);
>> +    virObjectLock(vm);
> 
> Is it necessary to unlock and lock the vm while calling prepare and finish on
> the destination libvirtd?
> 
They can be removed, but then we leave the virDomainObj locked for a "long" time
when no operations are being done on his context. Plus no API calls could be
done there in the meantime (and migration can take a rather long time). Prepare,
Finish, ConnectOpenAuth and ConnectSupportsFeature  are done on the remote
driver (RPC calls to the target libvirtd) so they have a rather high "cost".
What do you think?

>> +
>> +    if (ret == -1)
>> +        goto cleanup;
>> +
>> +    if (uri_out) {
>> +        if (virTypedParamsReplaceString(&params, &nparams,
>> +                                        VIR_MIGRATE_PARAM_URI, uri_out) < 
>> 0) {
>> +            orig_err = virSaveLastError();
>> +            goto finish;
>> +        }
>> +    } else {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("domainMigratePrepare3 did not set uri"));
>> +        goto finish;
>> +    }
>> +
>> +    VIR_DEBUG("Perform3 uri=%s", NULLSTR(uri_out));
>> +    ret = libxlDomainMigrationPerform(driver, vm, NULL, NULL,
>> +                                      uri_out, NULL, flags);
>> +
>> +    if (ret < 0)
>> +        orig_err = virSaveLastError();
>> +
>> +    cancelled = (ret < 0);
>> +
>> + finish:
>> +    VIR_DEBUG("Finish3 ret=%d", ret);
>> +    if (virTypedParamsGetString(params, nparams,
>> +                                VIR_MIGRATE_PARAM_DEST_NAME, NULL) <= 0 &&
>> +        virTypedParamsReplaceString(&params, &nparams,
>> +                                    VIR_MIGRATE_PARAM_DEST_NAME,
>> +                                    vm->def->name) < 0) {
>> +        ddomain = NULL;
>> +    } else {
>> +        virObjectUnlock(vm);
>> +        ddomain = dconn->driver->domainMigrateFinish3Params
>> +            (dconn, params, nparams, NULL, 0, NULL, NULL,
>> +             destflags, cancelled);
>> +        virObjectLock(vm);
>> +    }
>> +
>> +    cancelled = (ddomain == NULL);
>> +
>> +    /* If Finish3Params set an error, and we don't have an earlier
>> +     * one we need to preserve it in case confirm3 overwrites
>> +     */
>> +    if (!orig_err)
>> +        orig_err = virSaveLastError();
>> +
>> +    VIR_DEBUG("Confirm3 cancelled=%d vm=%p", cancelled, vm);
>> +    ret = libxlDomainMigrationConfirm(driver, vm, flags, cancelled);
>> +
>> +    if (ret < 0)
>> +        VIR_WARN("Guest %s probably left in 'paused' state on source",
>> +                 vm->def->name);
>> +
>> + cleanup:
>> +    if (ddomain) {
>> +        virObjectUnref(ddomain);
>> +        ret = 0;
>> +    } else {
>> +        ret = -1;
>> +    }
>> +
>> +    if (orig_err) {
>> +        virSetError(orig_err);
>> +        virFreeError(orig_err);
>> +    }
>> +
>> +    VIR_FREE(dom_xml);
>> +    VIR_FREE(uri_out);
>> +    virTypedParamsFree(params, nparams);
>> +    return ret;
>> +}
>> +
>> +static int virConnectCredType[] = {
>> +    VIR_CRED_AUTHNAME,
>> +    VIR_CRED_PASSPHRASE,
>> +};
>> +
>> +static virConnectAuth virConnectAuthConfig = {
>> +    .credtype = virConnectCredType,
>> +    .ncredtype = ARRAY_CARDINALITY(virConnectCredType),
>> +};
>> +
>> +static void
>> +libxlMigrationConnectionClosed(virConnectPtr conn,
>> +                               int reason,
>> +                               void *opaque)
>> +{
>> +    virDomainObjPtr vm = opaque;
>> +
>> +    VIR_DEBUG("conn=%p, reason=%d, vm=%s", conn, reason, vm->def->name);
>> +    virDomainObjBroadcast(vm);
> 
> I would expect to see a corresponding virDomainObjWait for this call to
> virDomainObjBroadcast. But with the current migration code, I don't think the
> connect close callback is needed. Is there anything to do when the connection 
> to
> destination libvirtd closes?
> 
You're right, it's my mistake. There is no one waiting for migration to complete
so this part isn't needed. I will remove the close callback on v3.

Regards,
Joao

>> +}
>> +
>> +/* On P2P mode there is only the Perform3 phase and we need to handle
>> + * the connection with the destination libvirtd and perform the migration.
>> + * Here we first tackle the first part of it, and libxlDoMigrationP2P 
>> handles
>> + * the migration process with an established virConnectPtr to the 
>> destination.
>> + */
>> +int
>> +libxlDomainMigrationPerformP2P(libxlDriverPrivatePtr driver,
>> +                               virDomainObjPtr vm,
>> +                               virConnectPtr sconn,
>> +                               const char *xmlin,
>> +                               const char *dconnuri,
>> +                               const char *uri_str ATTRIBUTE_UNUSED,
>> +                               const char *dname,
>> +                               unsigned int flags)
>> +{
>> +    int ret = -1;
>> +    int keepAliveInterval = 5;
>> +    int keepAliveCount = 5;
>> +    bool useParams;
>> +    virConnectPtr dconn = NULL;
>> +    virErrorPtr orig_err = NULL;
>> +
>> +    virObjectUnlock(vm);
>> +    dconn = virConnectOpenAuth(dconnuri, &virConnectAuthConfig, 0);
>> +    virObjectLock(vm);
>> +
>> +    if (dconn == NULL) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED,
>> +                       _("Failed to connect to remote libvirt URI %s: %s"),
>> +                       dconnuri, virGetLastErrorMessage());
>> +        return ret;
>> +    }
>> +
>> +    if (virConnectSetKeepAlive(dconn, keepAliveInterval,
>> +                               keepAliveCount) < 0)
>> +        goto cleanup;
>> +
>> +    if (virConnectRegisterCloseCallback(dconn, 
>> libxlMigrationConnectionClosed,
>> +                                        vm, NULL) < 0) {
>> +        goto cleanup;
>> +    }
>> +
>> +    virObjectUnlock(vm);
>> +    useParams = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
>> +                                         VIR_DRV_FEATURE_MIGRATION_PARAMS);
>> +    virObjectLock(vm);
>> +
>> +    if (!useParams) {
>> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> +                       _("Destination libvirt does not support migration 
>> with extensible parameters"));
>> +        goto cleanup;
>> +    }
>> +
>> +    ret = libxlDoMigrateP2P(driver, vm, sconn, xmlin, dconn, dconnuri,
>> +                            dname, uri_str, flags);
>> +
>> + cleanup:
>> +    orig_err = virSaveLastError();
>> +    virObjectUnlock(vm);
>> +    virConnectUnregisterCloseCallback(dconn, 
>> libxlMigrationConnectionClosed);
>> +    virObjectUnref(dconn);
>> +    virObjectLock(vm);
>> +    if (orig_err) {
>> +        virSetError(orig_err);
>> +        virFreeError(orig_err);
>> +    }
>> +    return ret;
>> +}
>> +
>>  int
>>  libxlDomainMigrationPerform(libxlDriverPrivatePtr driver,
>>                              virDomainObjPtr vm,
>> diff --git a/src/libxl/libxl_migration.h b/src/libxl/libxl_migration.h
>> index 20b45d8..0f83bb4 100644
>> --- a/src/libxl/libxl_migration.h
>> +++ b/src/libxl/libxl_migration.h
>> @@ -28,6 +28,7 @@
>>  
>>  # define LIBXL_MIGRATION_FLAGS                  \
>>      (VIR_MIGRATE_LIVE |                         \
>> +     VIR_MIGRATE_PEER2PEER |                    \
>>       VIR_MIGRATE_UNDEFINE_SOURCE |              \
>>       VIR_MIGRATE_PAUSED)
>>  
>> @@ -56,6 +57,16 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>>                              unsigned int flags);
>>  
>>  int
>> +libxlDomainMigrationPerformP2P(libxlDriverPrivatePtr driver,
>> +                               virDomainObjPtr vm,
>> +                               virConnectPtr sconn,
>> +                               const char *dom_xml,
>> +                               const char *dconnuri,
>> +                               const char *uri_str,
>> +                               const char *dname,
>> +                               unsigned int flags);
>> +
>> +int
>>  libxlDomainMigrationPerform(libxlDriverPrivatePtr driver,
>>                              virDomainObjPtr vm,
>>                              const char *dom_xml,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel