On Tue, Feb 16, 2016 at 10:31:50 +0300, Nikolay Shirokovskiy wrote:
> Current libvirt + qemu pair lacks secure migrations in case of
> VMs with non-shared disks. The only option to migrate securely
> natively is to use tunneled mode and some kind of secure
> destination URI. But tunelled mode does not support non-shared
> disks.
>
> The other way to make migration secure is to organize a tunnel
> by external means. This is possible in case of shared disks
> migration thru use of proper combination of destination URI,
> migration URI and VIR_MIGRATE_PARAM_LISTEN_ADDRESS migration
> param. But again this is not possible in case of non shared disks
> migration as we have no option to control target nbd server port.
> But fixing this much more simplier that supporting non-shared
> disks in tunneled mode.
>
> So this patch adds option to set target ndb port.
>
> Finally all qemu migration connections will be secured AFAIK but
> even in this case this patch could be convinient if one wants
> all migration traffic be put in a single connection.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
>
> include/libvirt/libvirt-domain.h | 10 +++++
> src/qemu/qemu_driver.c | 25 ++++++++----
> src/qemu/qemu_migration.c | 85 ++++++++++++++++++++++++++++------------
> src/qemu/qemu_migration.h | 3 ++
> tools/virsh-domain.c | 12 ++++++
> tools/virsh.pod | 5 ++-
> 6 files changed, 106 insertions(+), 34 deletions(-)
Overall, it would be nice if you could split this patch in two parts:
- public API + virsh
- qemu implementation
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 65f1618..aa380f5 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -757,6 +757,16 @@ typedef enum {
> */
> # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks"
>
> +/**
> + * VIR_MIGRATE_PARAM_NBD_PORT:
> + *
> + * virDomainMigrate* params field: port that destination nbd server should use
> + * for incoming disks migration. Type is VIR_TYPED_PARAM_INT. If set to 0 or
> + * omitted, libvirt will choose a suitable default. At the moment this is only
> + * supported by the QEMU driver.
> + */
> +# define VIR_MIGRATE_PARAM_NBD_PORT "nbd_port"
I think VIR_MIGRATE_PARAM_DISKS_PORT "disks_port" would be better as we
don't really advertise elsewhere NBD will be used for migrating disks.
Not to mention that other hypervisors may use something different or it
may even change in the future. When changing it, you'll also need to
modify the description to be slightly more general.
> +
> /* Domain migration. */
> virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn,
> unsigned long flags, const char *dname,
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index f2c7b61..4cadf34 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1709,7 +1709,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> const char *listenAddr,
> size_t nmigrate_disks,
> - const char **migrate_disks)
> + const char **migrate_disks,
> + int nbdPort)
> {
> int ret = -1;
> qemuDomainObjPrivatePtr priv = vm->privateData;
> @@ -1717,6 +1718,12 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
> char *diskAlias = NULL;
> size_t i;
>
> + if (nbdPort < 0 || nbdPort > USHRT_MAX) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("nbd port must be in range 0-65535"));
> + return -1;
> + }
> +
> for (i = 0; i < vm->def->ndisks; i++) {
> virDomainDiskDefPtr disk = vm->def->disks[i];
>
> @@ -1733,10 +1740,15 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
> QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
> goto cleanup;
>
> - if (!port &&
> - ((virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) ||
> - (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))) {
> - goto exit_monitor;
> + if (port == 0) {
> + if (nbdPort)
> + port = nbdPort;
> + else
> + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
> + goto exit_monitor;
> +
> + if (qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0)
> + goto exit_monitor;
If you initialize port = nbdPort at the beginning, you can just drop
this change.
> }
>
> if (qemuMonitorNBDServerAdd(priv->mon, diskAlias, true) < 0)
> @@ -1750,7 +1762,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
>
> cleanup:
> VIR_FREE(diskAlias);
> - if (ret < 0)
> + // second clause means port is autoselected
We don't use // comments in libvirt and the comment itself is quite
redundant I think.
> + if (ret < 0 && nbdPort == 0)
> virPortAllocatorRelease(driver->migrationPorts, port);
> return ret;
>
> @@ -3312,6 +3325,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> const char *listenAddress,
> size_t nmigrate_disks,
> const char **migrate_disks,
> + int nbdPort,
> unsigned long flags)
> {
> virDomainObjPtr vm = NULL;
> @@ -3513,7 +3527,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> flags & (VIR_MIGRATE_NON_SHARED_DISK | VIR_MIGRATE_NON_SHARED_INC) &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NBD_SERVER)) {
> if (qemuMigrationStartNBDServer(driver, vm, incoming->address,
> - nmigrate_disks, migrate_disks) < 0) {
> + nmigrate_disks, migrate_disks,
> + nbdPort) < 0) {
> goto stopjob;
> }
> cookieFlags |= QEMU_MIGRATION_COOKIE_NBD;
> @@ -3565,6 +3580,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>
> if (autoPort)
> priv->migrationPort = port;
> + // in this case port is autoselected and we don't need to manage it anymore
> + // after cookie is baked
Change the comment to use /* */
> + if (nbdPort != 0)
> + priv->nbdPort = 0;
> ret = 0;
>
> cleanup:
> @@ -3576,7 +3595,10 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> /* priv is set right after vm is added to the list of domains
> * and there is no 'goto cleanup;' in the middle of those */
> VIR_FREE(priv->origname);
> - virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
> + // release if port is auto selected which is not the case if
> + // it is given in parameters
Wrong comment style again.
> + if (nbdPort == 0)
> + virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
> priv->nbdPort = 0;
> qemuDomainRemoveInactive(driver, vm);
> }
> @@ -3633,7 +3655,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver,
>
> ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
> cookieout, cookieoutlen, def, origname,
> - st, NULL, 0, false, NULL, 0, NULL, flags);
> + st, NULL, 0, false, NULL, 0, NULL, 0, flags);
Since the new port parameter is not supported with tunnelled migration,
shouldn't we report an error when a user specifies the port?
> return ret;
> }
>
...
> @@ -4944,6 +4968,11 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
> VIR_MIGRATE_PARAM_MIGRATE_DISKS,
> migrate_disks[i]) < 0)
> goto cleanup;
> + if (nbdPort &&
> + virTypedParamsAddInt(¶ms, &nparams, &maxparams,
> + VIR_MIGRATE_PARAM_NBD_PORT,
> + nbdPort) < 0)
s/ // in the two lines above
> + goto cleanup;
> }
>
> if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED)
...
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index 2445e13..2796361 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -51,6 +51,7 @@
> VIR_MIGRATE_PARAM_BANDWIDTH, VIR_TYPED_PARAM_ULLONG, \
> VIR_MIGRATE_PARAM_GRAPHICS_URI, VIR_TYPED_PARAM_STRING, \
> VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \
> + VIR_MIGRATE_PARAM_NBD_PORT, VIR_TYPED_PARAM_INT, \
> VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \
> VIR_TYPED_PARAM_MULTIPLE, \
Just add the new parameter at the end of this list.
> NULL
...
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index bf65a60..e648a3a 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -9653,6 +9653,10 @@ static const vshCmdOptDef opts_migrate[] = {
> .type = VSH_OT_STRING,
> .help = N_("comma separated list of disks to be migrated")
> },
> + {.name = "nbd-port",
> + .type = VSH_OT_INT,
> + .help = N_("port to use by target nbd server")
> + },
All changes in virsh will need to be modified to avoid using "NBD".
> {.name = NULL}
> };
>
> @@ -9663,6 +9667,7 @@ doMigrate(void *opaque)
> virDomainPtr dom = NULL;
> const char *desturi = NULL;
> const char *opt = NULL;
> + int optInt = 0;
I think diskPort would be a nicer name of the variable.
> unsigned int flags = 0;
> virshCtrlData *data = opaque;
> vshControl *ctl = data->ctl;
> @@ -9705,6 +9710,13 @@ doMigrate(void *opaque)
> VIR_MIGRATE_PARAM_LISTEN_ADDRESS, opt) < 0)
> goto save_error;
>
> + if (vshCommandOptInt(ctl, cmd, "nbd-port", &optInt) < 0)
> + goto out;
> + if (optInt &&
> + virTypedParamsAddInt(¶ms, &nparams, &maxparams,
> + VIR_MIGRATE_PARAM_NBD_PORT, optInt) < 0)
> + goto save_error;
> +
> if (vshCommandOptStringReq(ctl, cmd, "dname", &opt) < 0)
> goto out;
> if (opt &&
> diff --git a/tools/virsh.pod b/tools/virsh.pod
> index 435c649..66418b2 100644
> --- a/tools/virsh.pod
> +++ b/tools/virsh.pod
> @@ -1532,7 +1532,7 @@ to the I<uri> namespace is displayed instead of being modified.
> [I<--compressed>] [I<--abort-on-error>] [I<--auto-converge>]
> I<domain> I<desturi> [I<migrateuri>] [I<graphicsuri>] [I<listen-address>]
> [I<dname>] [I<--timeout> B<seconds>] [I<--xml> B<file>]
> -[I<--migrate-disks> B<disk-list>]
> +[I<--migrate-disks> B<disk-list>] [I<--nbd-port> B<port>]
>
> Migrate domain to another host. Add I<--live> for live migration; <--p2p>
> for peer-2-peer migration; I<--direct> for direct migration; or I<--tunnelled>
> @@ -1659,6 +1659,9 @@ addresses are accepted as well as hostnames (the resolving is done on
> destination). Some hypervisors do not support this feature and will return an
> error if this parameter is used.
>
> +Optional I<nbd-port> sets the port that hypervisor on destination side should
> +bind to for incoming nbd traffic. Currently it is supported only by qemu.
> +
> =item B<migrate-setmaxdowntime> I<domain> I<downtime>
>
> Set maximum tolerable downtime for a domain which is being live-migrated to
Looks mostly OK to me.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list