On Tue, Aug 25, 2020 at 07:47:12 +0200, Martin Kletzander wrote:
Adds new typed param for migration and uses this as a UNIX socket path that should be used for the NBD part of migration. And also adds virsh support.
Partially resolves: https://bugzilla.redhat.com/1638889
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- docs/manpages/virsh.rst | 8 +++ include/libvirt/libvirt-domain.h | 12 ++++ src/qemu/qemu_domain.h | 1 + src/qemu/qemu_driver.c | 33 ++++++++-- src/qemu/qemu_migration.c | 110 ++++++++++++++++++++++--------- src/qemu/qemu_migration.h | 3 + src/qemu/qemu_migration_cookie.c | 22 +++++-- src/qemu/qemu_migration_cookie.h | 1 + tools/virsh-domain.c | 12 ++++ 9 files changed, 160 insertions(+), 42 deletions(-) ... diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index adba79aded54..71a644ca6b95 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -166,6 +166,7 @@ struct _qemuDomainObjPrivate { unsigned long migMaxBandwidth; char *origname; int nbdPort; /* Port used for migration with NBD */ + char *nbdSocketPath; /* Port used for migration with NBD */
Copy&pasta error :-) s/Port/Socket/
unsigned short migrationPort; int preMigrationState;
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b887185d012d..3f4690f8fb72 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -379,6 +379,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, size_t nmigrate_disks, const char **migrate_disks, int nbdPort, + const char *nbdSocketPath, const char *tls_alias) { int ret = -1; @@ -386,7 +387,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, size_t i; virStorageNetHostDef server = { .name = (char *)listenAddr, /* cast away const */ - .transport = VIR_STORAGE_NET_HOST_TRANS_TCP, + .transport = VIR_STORAGE_NET_HOST_TRANS_UNIX,
I'd expect transport to remain VIR_STORAGE_NET_HOST_TRANS_TCP unless nbdSocketPath is specified. In other words, the following hunk should be sufficient.
.port = nbdPort, }; bool server_started = false; @@ -397,6 +398,13 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, return -1; }
+ /* Prefer nbdSocketPath as there is no way to indicate we do not want to + * listen on a port */ + if (nbdSocketPath) { + server.socket = (char *)nbdSocketPath; + server.transport = VIR_STORAGE_NET_HOST_TRANS_UNIX; + } + for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; g_autofree char *diskAlias = NULL; @@ -425,7 +433,7 @@ qemuMigrationDstStartNBDServer(virQEMUDriverPtr driver, devicename = diskAlias; }
- if (!server_started) { + if (!server_started && !nbdSocketPath) {
I'd probably change this to if (!server_started && server.transport == VIR_STORAGE_NET_HOST_TRANS_TCP) { to make it more obvious we only care about port allocation for TCP.
if (server.port) { if (virPortAllocatorSetUsed(server.port) < 0) goto cleanup;
...
@@ -885,13 +903,17 @@ qemuMigrationSrcNBDStorageCopyDriveMirror(virQEMUDriverPtr driver, const char *diskAlias, const char *host, int port, + const char *socket, unsigned long long mirror_speed, bool mirror_shallow) { g_autofree char *nbd_dest = NULL; int mon_ret;
- if (strchr(host, ':')) { + if (socket) { + nbd_dest = g_strdup_printf("nbd+unix:///%s?socket=%s", + diskAlias, socket);
Is the number of slashes correct here? The "nbd+unix://" is clear, but then we pass "/diskAlias" rather than just "diskAlias". Oh I see, since "//" is present, the path has to either be empty or start with "/" to form a valid URI. Everything seems to be correct then, although a bit confusing since diskAlias is not actually a path. But I guess that's what QEMU expects.
+ } else if (strchr(host, ':')) { nbd_dest = g_strdup_printf("nbd:[%s]:%d:exportname=%s", host, port, diskAlias); } else { ... @@ -2329,6 +2357,8 @@ qemuMigrationDstPrepare(virDomainObjPtr vm,
if (tunnel) { migrateFrom = g_strdup("stdio"); + } else if (g_strcmp0(protocol, "unix") == 0) { + migrateFrom = g_strdup_printf("%s:%s", protocol, listenAddress); } else { bool encloseAddress = false; bool hostIPv6Capable = false;
Looks like this hunk would better fit in 8/9 (qemu: Allow migration over UNIX socket). ... With the small issues addressed and if all I said is correct Reviewed-by: Jiri Denemark <jdenemar@redhat.com>