[libvirt] [PATCH] migration: remove direct migration dependency on version1 of driver
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com> Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth); - if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; } -- 1.7.1
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? Moreover, can you please send patches rebased to current HEAD? Michal
On 28.08.2015 08:54, Michal Privoznik wrote:
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? I see you mean actual driver could be behind remote one and checking for perform3 always gives true so we need to check for feature instead?
Moreover, can you please send patches rebased to current HEAD? Sorry, most time i rebase.
Michal
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
On 28.08.2015 08:54, Michal Privoznik wrote:
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? I see you mean actual driver could be behind remote one and checking for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present. But after your patch, the function implementation would be needed always, even if the feature is not present. Michal
On 28.08.2015 19:04, Michal Privoznik wrote:
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
On 28.08.2015 08:54, Michal Privoznik wrote:
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? I see you mean actual driver could be behind remote one and checking for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present. But after your patch, the function implementation would be needed always, even if the feature is not present. Why? You can implement version1 or version3 or both for this check to pass.
Michal
On 31.08.2015 10:42, Nikolay Shirokovskiy wrote:
On 28.08.2015 19:04, Michal Privoznik wrote:
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
On 28.08.2015 08:54, Michal Privoznik wrote:
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? I see you mean actual driver could be behind remote one and checking for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present. But after your patch, the function implementation would be needed always, even if the feature is not present.
Why? You can implement version1 or version3 or both for this check to pass.
Unfortunately no. I mean, the current code does not have any fallback. Whenever the feature is detected, domainMigratePerform3 is used and that's it. Therefore I think we are aiming on something among the following lines: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cbf08fc..218efe8 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3425,16 +3425,16 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth); - if (!domain->conn->driver->domainMigratePerform) { - virReportUnsupportedError(); - return -1; - } - /* Perform the migration. The driver isn't supposed to return * until the migration is complete. */ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V3)) { + if (!domain->conn->driver->domainMigratePerform3) { + virReportUnsupportedError(); + return -1; + } + VIR_DEBUG("Using migration protocol 3"); /* dconn URI not relevant in direct migration, since no * target libvirtd is involved */ @@ -3450,6 +3450,11 @@ virDomainMigrateDirect(virDomainPtr domain, dname, bandwidth); } else { + if (!domain->conn->driver->domainMigratePerform) { + virReportUnsupportedError(); + return -1; + } + VIR_DEBUG("Using migration protocol 2"); if (xmlin) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", The other approach could be: diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cbf08fc..9d4827a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3420,12 +3420,18 @@ virDomainMigrateDirect(virDomainPtr domain, const char *uri, unsigned long bandwidth) { + bool v3migration; + VIR_DOMAIN_DEBUG(domain, "xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu", NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth); - if (!domain->conn->driver->domainMigratePerform) { + v3migration = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + + if ((!v3migration && !domain->conn->driver->domainMigratePerform) || + (v3migration && !domain->conn->driver->domainMigratePerform3)) { virReportUnsupportedError(); return -1; } @@ -3433,8 +3439,7 @@ virDomainMigrateDirect(virDomainPtr domain, /* Perform the migration. The driver isn't supposed to return * until the migration is complete. */ - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3)) { + if (v3migration) { VIR_DEBUG("Using migration protocol 3"); /* dconn URI not relevant in direct migration, since no * target libvirtd is involved */ Michal
On 31.08.2015 12:05, Michal Privoznik wrote:
On 31.08.2015 10:42, Nikolay Shirokovskiy wrote:
On 28.08.2015 19:04, Michal Privoznik wrote:
On 28.08.2015 11:29, Nikolay Shirokovskiy wrote:
On 28.08.2015 08:54, Michal Privoznik wrote:
On 27.08.2015 12:23, Nikolay Shirokovskiy wrote:
From: Nikolay Shirokovskiy <Nikolay Shirokovskiy nshirokovskiy@virtuozzo.com>
Direct migration should work if *perform3 is present but *perform is not. This is situation when driver migration is implemented after new version of driver function is introduced. We should not be forced to support old version too as its parameter space is subspace of newer one.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/libvirt-domain.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 6ab50ba..c89775b 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3427,7 +3427,8 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(dconnuri), NULLSTR(miguri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + if (!domain->conn->driver->domainMigratePerform && + !domain->conn->driver->domainMigratePerform3) { virReportUnsupportedError(); return -1; }
Hm.. domainMigratePerform3 will be used iff connection driver has VIR_DRV_FEATURE_MIGRATION_V3 feature. But this check will require that regardless. What if we check the presence of implementation with respect to that? I see you mean actual driver could be behind remote one and checking for perform3 always gives true so we need to check for feature instead?
Sort of. domainMigratePerform3 is used only if the feature is present. But after your patch, the function implementation would be needed always, even if the feature is not present.
Why? You can implement version1 or version3 or both for this check to pass.
Unfortunately no. I mean, the current code does not have any fallback. Whenever the feature is detected, domainMigratePerform3 is used and that's it. Therefore I think we are aiming on something among the following lines:
I see. There are 2 issues in the code. 1. perform must must be implemented even if perform3 will be used. 2. perform3 called without check I address only 1st case, you addess both. Among suggested implementations I would prefer first. So I guess I should say ACK.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cbf08fc..218efe8 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3425,16 +3425,16 @@ virDomainMigrateDirect(virDomainPtr domain, NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { - virReportUnsupportedError(); - return -1; - } - /* Perform the migration. The driver isn't supposed to return * until the migration is complete. */ if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, VIR_DRV_FEATURE_MIGRATION_V3)) { + if (!domain->conn->driver->domainMigratePerform3) { + virReportUnsupportedError(); + return -1; + } + VIR_DEBUG("Using migration protocol 3"); /* dconn URI not relevant in direct migration, since no * target libvirtd is involved */ @@ -3450,6 +3450,11 @@ virDomainMigrateDirect(virDomainPtr domain, dname, bandwidth); } else { + if (!domain->conn->driver->domainMigratePerform) { + virReportUnsupportedError(); + return -1; + } + VIR_DEBUG("Using migration protocol 2"); if (xmlin) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
The other approach could be:
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index cbf08fc..9d4827a 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -3420,12 +3420,18 @@ virDomainMigrateDirect(virDomainPtr domain, const char *uri, unsigned long bandwidth) { + bool v3migration; + VIR_DOMAIN_DEBUG(domain, "xmlin=%s, flags=%lx, dname=%s, uri=%s, bandwidth=%lu", NULLSTR(xmlin), flags, NULLSTR(dname), NULLSTR(uri), bandwidth);
- if (!domain->conn->driver->domainMigratePerform) { + v3migration = VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATION_V3); + + if ((!v3migration && !domain->conn->driver->domainMigratePerform) || + (v3migration && !domain->conn->driver->domainMigratePerform3)) { virReportUnsupportedError(); return -1; } @@ -3433,8 +3439,7 @@ virDomainMigrateDirect(virDomainPtr domain, /* Perform the migration. The driver isn't supposed to return * until the migration is complete. */ - if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, - VIR_DRV_FEATURE_MIGRATION_V3)) { + if (v3migration) { VIR_DEBUG("Using migration protocol 3"); /* dconn URI not relevant in direct migration, since no * target libvirtd is involved */
Michal
participants (3)
-
Michal Privoznik -
Nikolay Shirokovskiy -
Nikolay Shirokovskiy