Re: [PATCH v2] libxl: add p2p migration

[Jim Fehlig <jfehlig@suse.com>]

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@oracle.com>
> ---
> 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?

> +
> +    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?

Regards,
Jim

> +}
> +
> +/* 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,