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@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(-)
>>
>> ...
>>
>>>> @@ -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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list