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

[Nikolay Shirokovskiy]

On 17.03.2016 16:19, Jiri Denemark wrote:
> 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
> 

I've just sent new version with a bit radical restructure as described above.
How do you evaluate it?

Nikolay.