[libvirt] [PATCH v1 02/11] Introduce NBD migration cookie

[Michal Privoznik]

On 27.11.2012 23:18, Eric Blake wrote:
> ----- Original Message -----
>> This migration cookie is meant for two purposes. The first is to be
>> sent
>> in begin phase from source to destination to let it know we support
>> new
>> implementation of VIR_MIGRATE_NON_SHARED_{DISK,INC} so destination
>> can
>> start NBD server. Then, the second purpose is, destination can let us
>> know, on which port is NBD server running.
>> ---
>>  src/qemu/qemu_migration.c |  108
>>  ++++++++++++++++++++++++++++++++++++++++-----
>>  1 files changed, 97 insertions(+), 11 deletions(-)
> 
> Before looking at the code, I have a question:
> 
> Should we be adding a new VIR_DRV_FEATURE_... bit, and setting that
> driver feature bit in qemu_driver.c:qemuSupportsFeature(), and only
> send the cookie on the source side if the destination side knows how
> to handle it?
> 
> My concern (and it might be an invalid concern) is that if we send
> the nbd cookie but the destination is too old to handle it, then
> will the destination reject the cookie?

The destination would not parse that part (without any error of course)
and doesn't reply to the destination neither. So in Perform phase, when
the source gets a migration cookie from dst without any NBD data it is
obvious dst is running an older libvirt without NBD support and hence
source will fall back to previous implementation.

However, we can't switch to VIR_DRV_FEATURE approach as you suggest,
because usage of new implementation is not just matter of qemu driver
itself, but depends on current qemu instance that is to be migrated
(it's pointless to ask cached capabilities as running qemu can has a
different ones). It would be nice if we could do qemuSupportsFeature()
but I am afraid we can't.

> 
> On the other hand, this is more than just a handshake determined by
> the compile-time versions of libvirtd; after all, not only does
> libvirtd on both sides of the migration need to understand the
> cookie, but also qemu on the destination side has to support the
> NBD server, and qemu on the source side has to support drive-mirror.
> If any one of those pieces are not present, then we can still
> migrate storage, by using the older flags of the 'migrate' command
> itself, rather than outright failure.
> 
> It would help if you document the fallback cases, something like:
> 
> old libvirt src -> any destination:
>   no cookie sent, old-style storage migration
> new libvirt src, but src qemu too old:
>   same as old libvirt src
> new libvirt/qemu src -> old libvirt destination:
>   src->dst cookie sent? may depend on VIR_DRV_FEATURE
>   but no dst->src cookie reply, so use old-style migration
> new libvirt src -> new libvirt old qemu destination:
>   VIR_DRV_FEATURE would be set (libvirt understands it),
>   but because qemu does not support it, the dst must not
>   return the cookie reply, so the source can fall back to
>   old-style migration
> new libvirt src -> new libvirt/qemu dest:
>   cookie exchange succeeds, use new style migration

Okay, I'll add it into comment somewhere in code.

> 
> Now, on to the code:
> 
>> @@ -79,13 +80,14 @@ enum qemuMigrationCookieFlags {
>>  VIR_ENUM_DECL(qemuMigrationCookieFlag);
>>  VIR_ENUM_IMPL(qemuMigrationCookieFlag,
>>                QEMU_MIGRATION_COOKIE_FLAG_LAST,
>> -              "graphics", "lockstate", "persistent", "network");
>> +              "graphics", "lockstate", "persistent", "network",
>> "nbd");
> 
> [About the only good thing of this webmail interface botching
> long lines is that it is easy to spot lines that are worth wrapping]
> 
>> +static int
>> +qemuMigrationCookieAddNBD(qemuMigrationCookiePtr mig,
>> +                          int nbdPort)
>> +{
>> +    /* It is not a bug if there already is a NBD data */
>> +    if (!mig->nbd &&
>> +        VIR_ALLOC(mig->nbd) < 0) {
>> +        virReportOOMError();
>> +        return -1;
>> +    }
>> +
>> +    mig->nbd->port = nbdPort;
> 
> If there is already NBD data (I'm assuming that it gets
> created once per disk that needs migration?), does this
> override of the former mig->nbd->port cause us to forget
> an important piece of information?

Not really. It will overwrite info about the port NBD server is
listening to. However, this will in place when we need it,
the proper value is passed. Otherwise, in all other places 0 is being
passed - which is what we want, since in that phases the port info is
not relevant at all. So I think it is okay as is.

> 
> Overall, the cookie manipulation seems reasonable, but I'm still
> worried whether new libvirt talking to older libvirt will cause
> confusion if the <nbd> element appears in the cookie, and also
> whether new libvirt gracefully handles the absence of a cookie
> from older libvirt.

It does play nice. I've tested it.

Michal