5:21 a.m.
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