Re: [libvirt] [PATCH 03/10] qemu: Migration job on source daemon

On 07/18/2011 06:27 PM, Jiri Denemark wrote: Nice that the flag is automatic, but: on the converse direction, if the flag is explicitly requested of an 0.9.4 client but you are using an 0.9.3 source, then we should fail the migration rather than let the migration proceed unsafely (for precedence, we reject migrations when PEER2PEER flag is set but the driver lacks the feature). Also, we should document this flag in the comments of the public migrate functions. When the client is managing migration, then you only need support for change protection on the source side; if the destination side lacks it, we don't care. You only checked for the capability on the source side, so for wider compatibility, we should validate the flag, then mask it out before issuing any callbacks to the remote driver; thus an 0.9.4 source and 0.9.3 destination would still benefit from the protection (as it is only the source that needs it) when driven by an 0.9.4 client. [If an 0.9.3 client drives the migration, even with an 0.9.4 source, you don't get the protection, and if the migration isn't using protocol v3, you don't need the protection.] So for once, flag manipulation in libvirt.c is the right thing to do here. Meanwhile, when the source is managing migration (via the peer2peer or direct callbacks), then the flag has to be preserved from client to source, since libvirt.c does not automatically add the flag. In this case, older sources will reject the flag as unknown. In version 3 migration, we should pass the flag to all three source calls in the protocol (you left off confirm), for consistency. I'd like someone to double-check my diff before I push anything, but I think that what I have in the diff below captures the above analysis. Nice way to refactor things (it took me a few reads, not to mention the confusion in the fact that migrationPerform is used to drive both the entire p2p process as well as just a phase within the v2 and v3 process). qemuMigrationPerformJob either does just phase 2 unprotected, or the entire peer-2-peer sequence. But peer2peer is protected by default (since the entire job on the source side was triggered by a single API call). Since we know the source is protected (whether or not the flag is set), and since we don't want to fail when talking to an older destination that doesn't understand the flag, then peer2peer migration should always clear the protection flag (basically, doPeer2PeerMigrate3 is a reimplementation of virDomainMigrateVersion3, and since the former learned how to autoset the bit when needed, so should the latter - it's just that the latter doesn't need to set the bit). I would offer ACK to your patch plus my delta, but it's risky enough that I'd like a second review. Here's my proposed delta: diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 4b98815..b1bda31 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -678,7 +678,7 @@ typedef enum { /* (same base image shared between source and destination) */ VIR_MIGRATE_CHANGE_PROTECTION = (1 << 8), /* protect for changing domain configuration through the * whole migration process; this will be used automatically - * if both parties support it */ + * when supported */ } virDomainMigrateFlags; diff --git i/src/libvirt.c w/src/libvirt.c index 9dc526c..dbef4a5 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -4276,7 +4276,7 @@ confirm: cookieoutlen = 0; ret = domain->conn->driver->domainMigrateConfirm3 (domain, cookiein, cookieinlen, - flags, cancelled); + flags | protection, cancelled); /* If Confirm3 returns -1, there's nothing more we can * do, but fortunately worst case is that there is a * domain left in 'paused' state on source. @@ -4295,7 +4295,7 @@ confirm: /* - * In normal migration, the libvirt client co-ordinates communcation + * In normal migration, the libvirt client co-ordinates communication * between the 2 libvirtd instances on source & dest hosts. * * In this peer-2-peer migration alternative, the libvirt client @@ -4380,7 +4380,7 @@ virDomainMigratePeer2Peer (virDomainPtr domain, /* - * In normal migration, the libvirt client co-ordinates communcation + * In normal migration, the libvirt client co-ordinates communication * between the 2 libvirtd instances on source & dest hosts. * * Some hypervisors support an alternative, direct migration where @@ -4467,6 +4467,9 @@ virDomainMigrateDirect (virDomainPtr domain, * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the * domain on the source host. * VIR_MIGRATE_PAUSED Leave the domain suspended on the remote side. + * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration + * changes during the migration process (set + * automatically when supported). * * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. * Applications using the VIR_MIGRATE_PEER2PEER flag will probably @@ -4574,6 +4577,19 @@ virDomainMigrate (virDomainPtr domain, goto error; } } else { + /* Change protection requires support only on source side, and + * is only needed in v3 migration, which automatically re-adds + * the flag for just the source side. We mask it out for + * non-peer2peer to allow migration from newer source to an + * older destination that rejects the flag. */ + if (flags & VIR_MIGRATE_CHANGE_PROTECTION && + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) { + virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("cannot enforce change protection")); + goto error; + } + flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; if (flags & VIR_MIGRATE_TUNNELLED) { virLibConnError(VIR_ERR_OPERATION_INVALID, _("cannot perform tunnelled migration without using peer2peer flag")); @@ -4642,6 +4658,9 @@ error: * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the * domain on the source host. * VIR_MIGRATE_PAUSED Leave the domain suspended on the remote side. + * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration + * changes during the migration process (set + * automatically when supported). * * VIR_MIGRATE_TUNNELLED requires that VIR_MIGRATE_PEER2PEER be set. * Applications using the VIR_MIGRATE_PEER2PEER flag will probably @@ -4756,6 +4775,19 @@ virDomainMigrate2(virDomainPtr domain, goto error; } } else { + /* Change protection requires support only on source side, and + * is only needed in v3 migration, which automatically re-adds + * the flag for just the source side. We mask it out for + * non-peer2peer to allow migration from newer source to an + * older destination that rejects the flag. */ + if (flags & VIR_MIGRATE_CHANGE_PROTECTION && + !VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) { + virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("cannot enforce change protection")); + goto error; + } + flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; if (flags & VIR_MIGRATE_TUNNELLED) { virLibConnError(VIR_ERR_OPERATION_INVALID, _("cannot perform tunnelled migration without using peer2peer flag")); @@ -4831,6 +4863,9 @@ error: * on the destination host. * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the * domain on the source host. + * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration + * changes during the migration process (set + * automatically when supported). * * The operation of this API hinges on the VIR_MIGRATE_PEER2PEER flag. * If the VIR_MIGRATE_PEER2PEER flag is NOT set, the duri parameter @@ -4952,6 +4987,9 @@ error: * on the destination host. * VIR_MIGRATE_UNDEFINE_SOURCE If the migration is successful, undefine the * domain on the source host. + * VIR_MIGRATE_CHANGE_PROTECTION Protect against domain configuration + * changes during the migration process (set + * automatically when supported). * * The operation of this API hinges on the VIR_MIGRATE_PEER2PEER flag. * diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c index dca2b16..1121749 100644 --- i/src/qemu/qemu_migration.c +++ w/src/qemu/qemu_migration.c @@ -1994,6 +1994,11 @@ static int doPeer2PeerMigrate3(struct qemud_driver *driver, NULLSTR(dconnuri), NULLSTR(uri), flags, NULLSTR(dname), resource); + /* Unlike the virDomainMigrateVersion3 counterpart, we don't need + * to worry about auto-setting the VIR_MIGRATE_CHANGE_PROTECTION + * bit here, because we are already running inside the context of + * a single job. */ + dom_xml = qemuMigrationBegin(driver, vm, xmlin, &cookieout, &cookieoutlen); if (!dom_xml) @@ -2187,7 +2192,7 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, p2p = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_P2P); /* v3proto reflects whether the caller used Perform3, but with - * p2p migrate, regardless of whether Perform3 or Perform3 + * p2p migrate, regardless of whether Perform2 or Perform3 * were used, we decide protocol based on what target supports */ *v3proto = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, @@ -2207,6 +2212,13 @@ static int doPeer2PeerMigrate(struct qemud_driver *driver, goto cleanup; } + /* Change protection is only required on the source side (us), and + * only for v3 migration when begin and perform are separate jobs. + * But peer-2-peer is already a single job, and we still want to + * talk to older destinations that would reject the flag. + * Therefore it is safe to clear the bit here. */ + flags &= ~VIR_MIGRATE_CHANGE_PROTECTION; + if (*v3proto) ret = doPeer2PeerMigrate3(driver, sconn, dconn, vm, xmlin, dconnuri, uri, flags, dname, resource); -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org