[PATCH v3 1/3] libxl: add p2p migration

[PATCH v3 1/3] libxl: add p2p migration

[Joao Martins]

Introduce support for VIR_MIGRATE_PEER2PEER in libvirt migration.
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 take
of doing all phases of migration 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 yet and
we always use V3 with extensible params, thus it also makes the
implementation simpler.

It is worth noting that the receiver didn't have any changes,
and since 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 v2:
 - Remove Connect Close callback

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 | 203 ++++++++++++++++++++++++++++++++++++++++++++
 src/libxl/libxl_migration.h |  11 +++
 3 files changed, 224 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d4e9c2a7..d34f843 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -4999,6 +4999,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;
@@ -5324,9 +5325,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..28445fc 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,208 @@ 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);
+
+    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),
+};
+
+/* 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;
+
+    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);
+    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,
-- 
2.1.4

[PATCH v3 2/3] libxl: move begin phase job handling

[Joao Martins]

. From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
This is a preparatory patch to be able to begin a job in the
perform phase.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
 src/libxl/libxl_migration.c | 16 +++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index d34f843..28220b2 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
 {
     const char *xmlin = NULL;
     virDomainObjPtr vm = NULL;
+    libxlDriverPrivatePtr driver;
+    char *ret;
 
 #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
     virReportUnsupportedError();
@@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
         return NULL;
     }
 
-    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
+    driver = domain->conn->privateData;
+    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
+        virObjectUnlock(vm);
+        return NULL;
+    }
+
+    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
+
+    if (!libxlDomainObjEndJob(driver, vm))
+        vm = NULL;
+
+    if (vm)
+        virObjectUnlock(vm);
+
+    return ret;
 }
 
 static int
diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 28445fc..e3c7914 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -235,17 +235,14 @@ libxlDomainMigrationBegin(virConnectPtr conn,
     virDomainDefPtr def;
     char *xml = NULL;
 
-    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
-        goto cleanup;
-
     if (xmlin) {
         if (!(tmpdef = virDomainDefParseString(xmlin, cfg->caps,
                                                driver->xmlopt,
                                                VIR_DOMAIN_DEF_PARSE_INACTIVE)))
-            goto endjob;
+            goto cleanup;
 
         if (!libxlDomainDefCheckABIStability(driver, vm->def, tmpdef))
-            goto endjob;
+            goto cleanup;
 
         def = tmpdef;
     } else {
@@ -253,18 +250,11 @@ libxlDomainMigrationBegin(virConnectPtr conn,
     }
 
     if (!libxlDomainMigrationIsAllowed(def))
-        goto endjob;
+        goto cleanup;
 
     xml = virDomainDefFormat(def, VIR_DOMAIN_DEF_FORMAT_SECURE);
 
- endjob:
-    if (!libxlDomainObjEndJob(driver, vm))
-        vm = NULL;
-
  cleanup:
-    if (vm)
-        virObjectUnlock(vm);
-
     virDomainDefFree(tmpdef);
     virObjectUnref(cfg);
     return xml;
-- 
2.1.4

[PATCH v3 3/3] libxl: begin job on perform phase

[Joao Martins]

Add a job LIBXL_JOB_MODIFY in the perform phase to prevent
changes to the domain from happening during migration.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 src/libxl/libxl_driver.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 28220b2..73ed448 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5341,18 +5341,25 @@ libxlDomainMigratePerform3Params(virDomainPtr dom,
     if (virDomainMigratePerform3ParamsEnsureACL(dom->conn, vm->def) < 0)
         goto cleanup;
 
+    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+        goto cleanup;
+
     if (flags & VIR_MIGRATE_PEER2PEER) {
         if (libxlDomainMigrationPerformP2P(driver, vm, dom->conn, dom_xml,
                                            dconnuri, uri, dname, flags) < 0)
-            goto cleanup;
+            goto endjob;
     } else {
         if (libxlDomainMigrationPerform(driver, vm, dom_xml, dconnuri,
                                         uri, dname, flags) < 0)
-            goto cleanup;
+            goto endjob;
     }
 
     ret = 0;
 
+ endjob:
+    if (!libxlDomainObjEndJob(driver, vm))
+        vm = NULL;
+
  cleanup:
     if (vm)
         virObjectUnlock(vm);
-- 
2.1.4

Re: [PATCH v3 2/3] libxl: move begin phase job handling

[Jim Fehlig]

On 01/20/2016 12:00 PM, Joao Martins wrote:
> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
> This is a preparatory patch to be able to begin a job in the
> perform phase.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>  src/libxl/libxl_migration.c | 16 +++-------------
>  2 files changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d34f843..28220b2 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>  {
>      const char *xmlin = NULL;
>      virDomainObjPtr vm = NULL;
> +    libxlDriverPrivatePtr driver;
> +    char *ret;
>  
>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>      virReportUnsupportedError();
> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>          return NULL;
>      }
>  
> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
> +    driver = domain->conn->privateData;
> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
> +        virObjectUnlock(vm);
> +        return NULL;
> +    }
> +
> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
> +
> +    if (!libxlDomainObjEndJob(driver, vm))
> +        vm = NULL;

IIRC the qemu driver handles this a bit differently, and probably more
correctly. On the sender, a job is started during the begin phase and ended in
the confirm phase. On the receiver, a job is started in the prepare phase and
ended in the finish phase. This prevents modifying the virDomainObj between
phases, e.g. the begin and perform phases on the sender. Is it possible to
change the job handling similarly in the libxl migration phases?

Regards,
Jim

Re: [PATCH v3 2/3] libxl: move begin phase job handling

[Joao Martins]

On 02/02/2016 01:23 AM, Jim Fehlig wrote:
> On 01/20/2016 12:00 PM, Joao Martins wrote:
>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>> This is a preparatory patch to be able to begin a job in the
>> perform phase.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>  src/libxl/libxl_migration.c | 16 +++-------------
>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index d34f843..28220b2 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>  {
>>      const char *xmlin = NULL;
>>      virDomainObjPtr vm = NULL;
>> +    libxlDriverPrivatePtr driver;
>> +    char *ret;
>>  
>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>      virReportUnsupportedError();
>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>          return NULL;
>>      }
>>  
>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>> +    driver = domain->conn->privateData;
>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>> +        virObjectUnlock(vm);
>> +        return NULL;
>> +    }
>> +
>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>> +
>> +    if (!libxlDomainObjEndJob(driver, vm))
>> +        vm = NULL;
> 
> IIRC the qemu driver handles this a bit differently, and probably more
> correctly. On the sender, a job is started during the begin phase and ended in
> the confirm phase. On the receiver, a job is started in the prepare phase and
> ended in the finish phase. This prevents modifying the virDomainObj between
> phases, e.g. the begin and perform phases on the sender. Is it possible to
> change the job handling similarly in the libxl migration phases?
> 
Yeah, IIUC I believe this is the behavior depicted by
VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
support it. So since we're going this way, we should adversite it too in
libxlConnectSupportsFeature() ?

Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
patches meant solely for improving job handling in migration in general. What do
you think in making it a separate patch from this series? (with the changes
suggested above)

Joao

> Regards,
> Jim
> 

Re: [PATCH v3 2/3] libxl: move begin phase job handling

[Jim Fehlig]

Joao Martins wrote:
> 
> On 02/02/2016 01:23 AM, Jim Fehlig wrote:
>> On 01/20/2016 12:00 PM, Joao Martins wrote:
>>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>>> This is a preparatory patch to be able to begin a job in the
>>> perform phase.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>>  src/libxl/libxl_migration.c | 16 +++-------------
>>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index d34f843..28220b2 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>  {
>>>      const char *xmlin = NULL;
>>>      virDomainObjPtr vm = NULL;
>>> +    libxlDriverPrivatePtr driver;
>>> +    char *ret;
>>>  
>>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>>      virReportUnsupportedError();
>>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>          return NULL;
>>>      }
>>>  
>>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>> +    driver = domain->conn->privateData;
>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>>> +        virObjectUnlock(vm);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>> +
>>> +    if (!libxlDomainObjEndJob(driver, vm))
>>> +        vm = NULL;
>> IIRC the qemu driver handles this a bit differently, and probably more
>> correctly. On the sender, a job is started during the begin phase and ended in
>> the confirm phase. On the receiver, a job is started in the prepare phase and
>> ended in the finish phase. This prevents modifying the virDomainObj between
>> phases, e.g. the begin and perform phases on the sender. Is it possible to
>> change the job handling similarly in the libxl migration phases?
>>
> Yeah, IIUC I believe this is the behavior depicted by
> VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
> support it. So since we're going this way, we should adversite it too in
> libxlConnectSupportsFeature() ?

Yep, sounds good.

> Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
> patches meant solely for improving job handling in migration in general. What do
> you think in making it a separate patch from this series? (with the changes
> suggested above)

Agreed. I'll take another look at the P2P patch and push it (after fixing the
nits) if there are no further comments. Implementing the CHANGE_PROTECTION
behavior and improving job handling should be done separately.

Regards,
Jim

Re: [PATCH v3 2/3] libxl: move begin phase job handling

[Joao Martins]

On 02/02/2016 03:39 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>>
>> On 02/02/2016 01:23 AM, Jim Fehlig wrote:
>>> On 01/20/2016 12:00 PM, Joao Martins wrote:
>>>> . From libxlMigrationBegin to libxlDomainMigrateBegin3Params().
>>>> This is a preparatory patch to be able to begin a job in the
>>>> perform phase.
>>>>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>>> ---
>>>>  src/libxl/libxl_driver.c    | 18 +++++++++++++++++-
>>>>  src/libxl/libxl_migration.c | 16 +++-------------
>>>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>>> index d34f843..28220b2 100644
>>>> --- a/src/libxl/libxl_driver.c
>>>> +++ b/src/libxl/libxl_driver.c
>>>> @@ -5187,6 +5187,8 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>>  {
>>>>      const char *xmlin = NULL;
>>>>      virDomainObjPtr vm = NULL;
>>>> +    libxlDriverPrivatePtr driver;
>>>> +    char *ret;
>>>>  
>>>>  #ifdef LIBXL_HAVE_NO_SUSPEND_RESUME
>>>>      virReportUnsupportedError();
>>>> @@ -5223,7 +5225,21 @@ libxlDomainMigrateBegin3Params(virDomainPtr domain,
>>>>          return NULL;
>>>>      }
>>>>  
>>>> -    return libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>>> +    driver = domain->conn->privateData;
>>>> +    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) {
>>>> +        virObjectUnlock(vm);
>>>> +        return NULL;
>>>> +    }
>>>> +
>>>> +    ret = libxlDomainMigrationBegin(domain->conn, vm, xmlin);
>>>> +
>>>> +    if (!libxlDomainObjEndJob(driver, vm))
>>>> +        vm = NULL;
>>> IIRC the qemu driver handles this a bit differently, and probably more
>>> correctly. On the sender, a job is started during the begin phase and ended in
>>> the confirm phase. On the receiver, a job is started in the prepare phase and
>>> ended in the finish phase. This prevents modifying the virDomainObj between
>>> phases, e.g. the begin and perform phases on the sender. Is it possible to
>>> change the job handling similarly in the libxl migration phases?
>>>
>> Yeah, IIUC I believe this is the behavior depicted by
>> VIR_MIGRATE_CHANGE_PROTECTION flag in which is set by default if the driver
>> support it. So since we're going this way, we should adversite it too in
>> libxlConnectSupportsFeature() ?
> 
> Yep, sounds good.
> 
>> Btw, the P2P patch keeps the original behavior wrt to jobs, and these two
>> patches meant solely for improving job handling in migration in general. What do
>> you think in making it a separate patch from this series? (with the changes
>> suggested above)
> 
> Agreed. I'll take another look at the P2P patch and push it (after fixing the
> nits) if there are no further comments. Implementing the CHANGE_PROTECTION
> behavior and improving job handling should be done separately.
> 
Cool, Thanks!

> Regards,
> Jim
> 

Re: [PATCH v3 1/3] libxl: add p2p migration

[Jim Fehlig]

On 01/20/2016 12:00 PM, Joao Martins wrote:
> Introduce support for VIR_MIGRATE_PEER2PEER in libvirt migration.
> 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
s/let//
> migration logic with virConnectPtr. In this function we take
> of doing all phases of migration in the destination similar to

"... take care of doing..." ?
> 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 yet and

One of those "yet" can be dropped. Even as an native English speaker, I'm not
sure which one is the most correct to drop :-).

> we always use V3 with extensible params, thus it also makes the
> implementation simpler.
>
> It is worth noting that the receiver didn't have any changes,
> and since 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 v2:
>  - Remove Connect Close callback
>
> 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 | 203 ++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_migration.h |  11 +++
>  3 files changed, 224 insertions(+), 3 deletions(-)

Looks good other than the commit message nits. I do have a question about the
job handling changes in 2 and 3 though.

Regards,
Jim

Re: [PATCH v3 1/3] libxl: add p2p migration

[Jim Fehlig]

Joao Martins wrote:
> Introduce support for VIR_MIGRATE_PEER2PEER in libvirt migration.
> 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 take
> of doing all phases of migration 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 yet and
> we always use V3 with extensible params, thus it also makes the
> implementation simpler.
> 
> It is worth noting that the receiver didn't have any changes,
> and since 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 v2:
>  - Remove Connect Close callback
> 
> 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 | 203 ++++++++++++++++++++++++++++++++++++++++++++
>  src/libxl/libxl_migration.h |  11 +++
>  3 files changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index d4e9c2a7..d34f843 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -4999,6 +4999,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;
> @@ -5324,9 +5325,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..28445fc 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,208 @@ 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);
> +
> +    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),
> +};
> +
> +/* 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)

I knew there was something I forgot to mention yesterday when reviewing this
patch...

Since you set keepalive on the destination connection, should we also make the
interval and count configurable like Jirka did for the qemu P2P migration with
commit 1e626437? Also, if the connection dies, how are we informed about that?
Would that be done via the connect close callback that was removed in this
version? It is not clear to me how errors on the dconn connection are handled.

Regards,
Jim


> +        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);
> +    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,

Re: [PATCH v3 1/3] libxl: add p2p migration

[Joao Martins]

On 02/02/2016 11:41 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> Introduce support for VIR_MIGRATE_PEER2PEER in libvirt migration.
>> 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 take
>> of doing all phases of migration 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 yet and
>> we always use V3 with extensible params, thus it also makes the
>> implementation simpler.
>>
>> It is worth noting that the receiver didn't have any changes,
>> and since 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 v2:
>>  - Remove Connect Close callback
>>
>> 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 | 203 ++++++++++++++++++++++++++++++++++++++++++++
>>  src/libxl/libxl_migration.h |  11 +++
>>  3 files changed, 224 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index d4e9c2a7..d34f843 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -4999,6 +4999,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;
>> @@ -5324,9 +5325,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..28445fc 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,208 @@ 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);
>> +
>> +    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),
>> +};
>> +
>> +/* 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)
> 
> I knew there was something I forgot to mention yesterday when reviewing this
> patch...
> 
> Since you set keepalive on the destination connection, should we also make the
> interval and count configurable like Jirka did for the qemu P2P migration with
> commit 1e626437?

Good idea, will do it for v4.

> Also, if the connection dies, how are we informed about that?
> Would that be done via the connect close callback that was removed in this
> version? It is not clear to me how errors on the dconn connection are handled.
> 
In the close callback, we're just notified that the connection got closed, but I
am not sure if there is much we can do about it in the handler.
We could set an error on the callback, and then check dconn callers on
libxlDoMigrateP2P() before any RPC calls?

Regards,
Joao

> Regards,
> Jim
> 
> 
>> +        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);
>> +    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,
> 

Re: [PATCH v3 1/3] libxl: add p2p migration

[Jim Fehlig]

On 02/03/2016 12:36 PM, Joao Martins wrote:
>
> On 02/02/2016 11:41 PM, Jim Fehlig wrote:
>> Also, if the connection dies, how are we informed about that?
>> Would that be done via the connect close callback that was removed in this
>> version? It is not clear to me how errors on the dconn connection are handled.
>>
> In the close callback, we're just notified that the connection got closed, but I
> am not sure if there is much we can do about it in the handler.
> We could set an error on the callback, and then check dconn callers on
> libxlDoMigrateP2P() before any RPC calls?

Ah, nevermind. If the connection got closed, the dconn calls will fail and
errors will be handled at the call sites.

Regards,
Jim