[libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
From f56b290eab36bbb7a9ac53778a55638d473504d1 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 11 Oct 2013 11:27:13 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we use virPortAllocator here to solve the problem. Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.h | 6 +++--- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_migration.c | 28 +++++++++++++++++++++------- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2e2acfb..3277ba4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -51,6 +51,9 @@ # define QEMU_WEBSOCKET_PORT_MIN 5700 # define QEMU_WEBSOCKET_PORT_MAX 65535 +# define QEMU_MIGRATION_PORT_MIN 49152 +# define QEMU_MIGRATION_PORT_MAX 49215 + typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks; typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr; struct _qemuBuildCommandLineCallbacks { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index da29a2a..3176085 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -221,6 +221,9 @@ struct _virQEMUDriver { /* Immutable pointer, self-locking APIs */ virPortAllocatorPtr webSocketPorts; + /* Immutable pointer, self-locking APIs */ + virPortAllocatorPtr migrationPorts; + /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo; @@ -242,9 +245,6 @@ struct _qemuDomainCmdlineDef { char **env_value; }; -/* Port numbers used for KVM migration. */ -# define QEMUD_MIGRATION_FIRST_PORT 49152 -# define QEMUD_MIGRATION_NUM_PORTS 64 void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21f116c..16c55a6 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -160,6 +160,7 @@ struct _qemuDomainObjPrivate { unsigned long migMaxBandwidth; char *origname; int nbdPort; /* Port used for migration with NBD */ + int migrationPort; virChrdevsPtr devs; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cfdbb9a..c08a73c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -687,6 +687,11 @@ qemuStateInitialize(bool privileged, cfg->webSocketPortMax)) == NULL) goto error; + if ((qemu_driver->migrationPorts = + virPortAllocatorNew(QEMU_MIGRATION_PORT_MIN, + QEMU_MIGRATION_PORT_MAX)) == NULL) + goto error; + if (qemuSecurityInit(qemu_driver) < 0) goto error; @@ -993,6 +998,7 @@ qemuStateCleanup(void) { virObjectUnref(qemu_driver->domains); virObjectUnref(qemu_driver->remotePorts); virObjectUnref(qemu_driver->webSocketPorts); + virObjectUnref(qemu_driver->migrationPorts); virObjectUnref(qemu_driver->xmlopt); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..93ae237 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2147,6 +2147,9 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainJobTypeToString(priv->job.active), qemuDomainAsyncJobTypeToString(priv->job.asyncJob)); + virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + priv->migrationPort = 0; + if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) return; qemuDomainObjDiscardAsyncJob(driver, vm); @@ -2297,6 +2300,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, *def = NULL; priv = vm->privateData; + priv->migrationPort = port; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; @@ -2415,6 +2419,11 @@ cleanup: VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); + if (ret < 0) { + virPortAllocatorRelease(driver->migrationPorts, port); + if (priv) + priv->migrationPort = 0; + } if (vm) { if (ret >= 0 || vm->persistent) virObjectUnlock(vm); @@ -2493,7 +2502,6 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *origname, unsigned long flags) { - static int port = 0; int this_port; char *hostname = NULL; const char *p; @@ -2521,8 +2529,9 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; - if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; + if (virPortAllocatorAcquire(driver->migrationPorts, + (unsigned short *)&this_port) < 0) + goto cleanup; /* Get hostname */ if ((hostname = virGetHostname()) == NULL) @@ -2578,9 +2587,9 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (uri->port == 0) { /* Generate a port */ - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; - if (port == QEMUD_MIGRATION_NUM_PORTS) - port = 0; + if (virPortAllocatorAcquire(driver->migrationPorts, + (unsigned short *)&this_port) < 0) + goto cleanup; /* Caller frees */ if (virAsprintf(uri_out, "%s:%d", uri_in, this_port) < 0) @@ -2600,8 +2609,11 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, cleanup: virURIFree(uri); VIR_FREE(hostname); - if (ret != 0) + if (ret != 0) { VIR_FREE(*uri_out); + virPortAllocatorRelease(driver->migrationPorts, + (unsigned short)this_port); + } return ret; } @@ -4370,6 +4382,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver, } qemuMigrationStopNBDServer(driver, vm, mig); + virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + priv->migrationPort = 0; if (flags & VIR_MIGRATE_PERSIST_DEST) { virDomainDefPtr vmdef; -- 1.7.3.1.msysgit.0 Best Regards, -WangYufei
On Tue, Oct 15, 2013 at 04:24:14AM +0000, Wangyufei (A) wrote:
From f56b290eab36bbb7a9ac53778a55638d473504d1 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 11 Oct 2013 11:27:13 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we use virPortAllocator here to solve the problem.
In future please line wrap your commit messages at ~70 characters.
Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.h | 6 +++--- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_migration.c | 28 +++++++++++++++++++++------- 5 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 2e2acfb..3277ba4 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -51,6 +51,9 @@ # define QEMU_WEBSOCKET_PORT_MIN 5700 # define QEMU_WEBSOCKET_PORT_MAX 65535
+# define QEMU_MIGRATION_PORT_MIN 49152 +# define QEMU_MIGRATION_PORT_MAX 49215 + typedef struct _qemuBuildCommandLineCallbacks qemuBuildCommandLineCallbacks; typedef qemuBuildCommandLineCallbacks *qemuBuildCommandLineCallbacksPtr; struct _qemuBuildCommandLineCallbacks { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index da29a2a..3176085 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -221,6 +221,9 @@ struct _virQEMUDriver { /* Immutable pointer, self-locking APIs */ virPortAllocatorPtr webSocketPorts;
+ /* Immutable pointer, self-locking APIs */ + virPortAllocatorPtr migrationPorts; + /* Immutable pointer, lockless APIs*/ virSysinfoDefPtr hostsysinfo;
@@ -242,9 +245,6 @@ struct _qemuDomainCmdlineDef { char **env_value; };
-/* Port numbers used for KVM migration. */ -# define QEMUD_MIGRATION_FIRST_PORT 49152 -# define QEMUD_MIGRATION_NUM_PORTS 64
void qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 21f116c..16c55a6 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -160,6 +160,7 @@ struct _qemuDomainObjPrivate { unsigned long migMaxBandwidth; char *origname; int nbdPort; /* Port used for migration with NBD */ + int migrationPort;
virChrdevsPtr devs;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cfdbb9a..c08a73c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -687,6 +687,11 @@ qemuStateInitialize(bool privileged, cfg->webSocketPortMax)) == NULL) goto error;
+ if ((qemu_driver->migrationPorts = + virPortAllocatorNew(QEMU_MIGRATION_PORT_MIN, + QEMU_MIGRATION_PORT_MAX)) == NULL) + goto error; + if (qemuSecurityInit(qemu_driver) < 0) goto error;
@@ -993,6 +998,7 @@ qemuStateCleanup(void) { virObjectUnref(qemu_driver->domains); virObjectUnref(qemu_driver->remotePorts); virObjectUnref(qemu_driver->webSocketPorts); + virObjectUnref(qemu_driver->migrationPorts);
virObjectUnref(qemu_driver->xmlopt);
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..93ae237 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2147,6 +2147,9 @@ qemuMigrationPrepareCleanup(virQEMUDriverPtr driver, qemuDomainJobTypeToString(priv->job.active), qemuDomainAsyncJobTypeToString(priv->job.asyncJob));
+ virPortAllocatorRelease(driver->migrationPorts, priv->migrationPort); + priv->migrationPort = 0; + if (!qemuMigrationJobIsActive(vm, QEMU_ASYNC_JOB_MIGRATION_IN)) return; qemuDomainObjDiscardAsyncJob(driver, vm); @@ -2297,6 +2300,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
*def = NULL; priv = vm->privateData; + priv->migrationPort = port; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup;
@@ -2415,6 +2419,11 @@ cleanup: VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); + if (ret < 0) { + virPortAllocatorRelease(driver->migrationPorts, port); + if (priv) + priv->migrationPort = 0; + } if (vm) { if (ret >= 0 || vm->persistent) virObjectUnlock(vm); @@ -2493,7 +2502,6 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *origname, unsigned long flags) { - static int port = 0; int this_port; char *hostname = NULL; const char *p; @@ -2521,8 +2529,9 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - this_port = QEMUD_MIGRATION_FIRST_PORT + port++; - if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0; + if (virPortAllocatorAcquire(driver->migrationPorts, + (unsigned short *)&this_port) < 0) + goto cleanup;
Just declare 'this_port' to be an 'unsigned short' instead of casting it everywhere. ACK if you fix that. As a followup, it might be desirable to make the migration port range be configurable via /etc/libvirt/qemu.conf, in the same way that we have the VNC/SPICE port range configurable. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Oct 15, 2013 at 11:59:17 +0100, Daniel Berrange wrote:
> On Tue, Oct 15, 2013 at 04:24:14AM +0000, Wangyufei (A) wrote:
> > >From f56b290eab36bbb7a9ac53778a55638d473504d1 Mon Sep 17 00:00:00 2001
> > From: WangYufei <james.wangyufei@huawei.com>
> > Date: Fri, 11 Oct 2013 11:27:13 +0800
> > Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
...
> > @@ -2521,8 +2529,9 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
> > * to be a correct hostname which refers to the target machine).
> > */
> > if (uri_in == NULL) {
> > - this_port = QEMUD_MIGRATION_FIRST_PORT + port++;
> > - if (port == QEMUD_MIGRATION_NUM_PORTS) port = 0;
> > + if (virPortAllocatorAcquire(driver->migrationPorts,
> > + (unsigned short *)&this_port) < 0)
> > + goto cleanup;
>
> Just declare 'this_port' to be an 'unsigned short' instead of casting
> it everywhere.
>
>
> ACK if you fix that.
Oops, this patch has a few issues that are more serious than this. I'll
be sending an amended patch shortly.
> As a followup, it might be desirable to make the migration port range
> be configurable via /etc/libvirt/qemu.conf, in the same way that we
> have the VNC/SPICE port range configurable.
Yeah, I was planning on making such patch once this one gets pushed.
Jirka
On Tue, Oct 15, 2013 at 04:24:14 +0000, Wangyufei (A) wrote:
From f56b290eab36bbb7a9ac53778a55638d473504d1 Mon Sep 17 00:00:00 2001 From: WangYufei <james.wangyufei@huawei.com> Date: Fri, 11 Oct 2013 11:27:13 +0800 Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we use virPortAllocator here to solve the problem.
Signed-off-by: WangYufei <james.wangyufei@huawei.com> --- src/qemu/qemu_command.h | 3 +++ src/qemu/qemu_conf.h | 6 +++--- src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 6 ++++++ src/qemu/qemu_migration.c | 28 +++++++++++++++++++++------- 5 files changed, 34 insertions(+), 10 deletions(-)
...
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 3a1aab7..93ae237 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c ... @@ -2297,6 +2300,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
*def = NULL; priv = vm->privateData; + priv->migrationPort = port; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup;
@@ -2415,6 +2419,11 @@ cleanup: VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); + if (ret < 0) { + virPortAllocatorRelease(driver->migrationPorts, port); + if (priv) + priv->migrationPort = 0; + }
This would possibly release a port that was not acquired from port allocator but was supplied via migration URI.
if (vm) { if (ret >= 0 || vm->persistent) virObjectUnlock(vm);
...
@@ -2600,8 +2609,11 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, cleanup: virURIFree(uri); VIR_FREE(hostname); - if (ret != 0) + if (ret != 0) { VIR_FREE(*uri_out); + virPortAllocatorRelease(driver->migrationPorts, + (unsigned short)this_port); + }
And this may result in the port being released twice (once in qemuMigrationPrepareAny and once here).
return ret; }
In other words, I think we need the following changes to be squashed in (I'll send the complete patch with the suggested changes squashed). Jirka diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index f8315da..0f17d0e 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -2159,7 +2159,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virDomainDefPtr *def, const char *origname, virStreamPtr st, - unsigned int port, + unsigned short port, + bool autoPort, const char *listenAddress, unsigned long flags) { @@ -2328,7 +2329,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, *def = NULL; priv = vm->privateData; - priv->migrationPort = port; if (VIR_STRDUP(priv->origname, origname) < 0) goto cleanup; @@ -2440,6 +2440,8 @@ done: goto cleanup; } + if (autoPort) + priv->migrationPort = port; ret = 0; cleanup: @@ -2447,11 +2449,6 @@ cleanup: VIR_FREE(xmlout); VIR_FORCE_CLOSE(dataFD[0]); VIR_FORCE_CLOSE(dataFD[1]); - if (ret < 0) { - virPortAllocatorRelease(driver->migrationPorts, port); - if (priv) - priv->migrationPort = 0; - } if (vm) { if (ret >= 0 || vm->persistent) virObjectUnlock(vm); @@ -2531,7 +2528,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *listenAddress, unsigned long flags) { - int this_port; + unsigned short port = 0; + bool autoPort = true; char *hostname = NULL; const char *p; char *uri_str = NULL; @@ -2558,8 +2556,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { - if (virPortAllocatorAcquire(driver->migrationPorts, - (unsigned short *)&this_port) < 0) + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) goto cleanup; /* Get hostname */ @@ -2616,8 +2613,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, if (uri->port == 0) { /* Generate a port */ - if (virPortAllocatorAcquire(driver->migrationPorts, - (unsigned short *)&this_port) < 0) + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) goto cleanup; /* Caller frees */ @@ -2625,7 +2621,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, goto cleanup; } else { - this_port = uri->port; + port = uri->port; + autoPort = false; } } @@ -2634,14 +2631,14 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - NULL, this_port, listenAddress, flags); + NULL, port, autoPort, listenAddress, flags); cleanup: virURIFree(uri); VIR_FREE(hostname); if (ret != 0) { VIR_FREE(*uri_out); - virPortAllocatorRelease(driver->migrationPorts, - (unsigned short)this_port); + if (autoPort) + virPortAllocatorRelease(driver->migrationPorts, port); } return ret; }
participants (3)
-
Daniel P. Berrange -
Jiri Denemark -
Wangyufei (A)