[libvirt] [PATCH v2] migration: add option to set target ndb server port

[Jiri Denemark]

On Thu, Mar 17, 2016 at 15:58:50 +0300, Nikolay Shirokovskiy wrote:
> 
> 
> On 16.03.2016 18:59, Jiri Denemark wrote:
> > 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 at virtuozzo.com>
> >> ---
> >>
> >>  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(-)
> 
> ...
> 
> >> @@ -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.
> > 
> 
> Thank you for reviewing. 
> 
> I'm about to resend this patch (know series) and have 
> only argue about this place. I can't just set port at the beginning. In this
> case if nbdPort is specified nbd server will not be started. I hesitating
> on how to change this place with less nesting (which looks like the problem)
> and finally decided to split this cycle. Basically we want to delay starting
> ndb server until we found we have some disks to migrate. Let's check 
> this condition separately. In this case we could move logic of starting
> nbd server out of the cycle. After that port acquring and nbd starting logic could
> be written sequentially without using of extra flags of extra nesting.

Yeah, I didn't meant port should be automatically allocated before we
enter the loop. Just something like

    port = nbdPort;
    ...
    for (i = 0; i < vm->def->ndisks; i++) {
        ...
        if (!port &&
            (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0 ||
             qemuMonitorNBDServerStart(priv->mon, listenAddr, port) < 0))
            goto exit_monitor;
        ...
    }

But you'd also need to remember whether you started the nbd server to
make sure priv->nbdPort is not set if nbdPort was set but no server was
needed.

So after all your solution might probably be better, just reformat it a
bit:

    if (nbdPort)
        port = nbdPort;
    else if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
        goto exit_monitor;

Jirka