Re: [libvirt] [PATCH 3/9] Introduce two method migration APIs

2011/5/25 Daniel P. Berrange <berrange(a)redhat.com&gt;: > On Wed, May 25, 2011 at 09:45:50AM +0200, Matthias Bolte wrote: >> 2011/5/24 Daniel P. Berrange <berrange(a)redhat.com&gt;: >> > There are two pieces of information which are desirable for >> > migration, which cannot be supplied by applications >> > >> >  - The explicit QEMU migration URI, while using Peer2Peer >> >   migration >> >  - An override for the target VM XML >> > >> > This introduces two new public APIs to support these extra >> > parameters. There is no need for extra wire protocool changes, >> > since this is supported by the v3 migration enhancements >> > >> > * include/libvirt/libvirt.h.in, >> >  src/libvirt.c, src/libvirt_public.syms: Add virDomainMigrate2 >> >  and virDomainMigrateToURI2 >> > --- >> >  include/libvirt/libvirt.h.in |   12 ++ >> >  src/libvirt.c                |  321 +++++++++++++++++++++++++++++++++++++++++- >> >  src/libvirt_public.syms      |    2 + >> >  3 files changed, 332 insertions(+), 3 deletions(-) >> > >> >> > diff --git a/src/libvirt.c b/src/libvirt.c >> > index 2b2c1fd..4bdcd43 100644 >> > --- a/src/libvirt.c >> > +++ b/src/libvirt.c >> >> > + >> > +/** >> > + * virDomainMigrate2: >> > + * @domain: a domain object >> > + * @dconn: destination host (a connection object) >> > + * @flags: flags >> > + * @dxml: optional XML config for launching guest on target >> >> The other optional parameters have their optional in (). So this >> optional should be in () too, or the others should have their () >> removed. >> >> > + * @dname: (optional) rename domain to this at destination >> > + * @uri: (optional) dest hostname/URI as seen from the source host >> > + * @bandwidth: (optional) specify migration bandwidth limit in Mbps >> > + * >> > + * Migrate the domain object from its current host to the destination >> > + * host given by dconn (a connection to the destination host). >> > + * >> >> > +/** >> > + * virDomainMigrateToURI2: >> > + * @domain: a domain object >> > + * @dconnuri: optional URI for target libvirtd if @flags includes VIR_MIGRATE_PEER2PEER >> > + * @miguri: optional URI for invoking the migration, not if @flags includs VIR_MIGRATE_TUNNELLED >> > + * @dxml: optional XML config for launching guest on target >> >> The same comment from above about the word optional applies here too. >> >> > + * @flags: flags >> > + * @dname: (optional) rename domain to this at destination >> > + * @bandwidth: (optional) specify migration bandwidth limit in Mbps >> >> ACK to be patch as it, but do we really want to add a new public API >> to 0.9.2 for a feature that no driver implements yet and might not >> implement it before 0.9.2? As I understand it the URI separation >> already works but the XML override isn't implemented yet. Or do you >> already have the XML override implemented but did post the patch yet? > > I don't have XML override implemented yet. QEMU returns an error > if you attempt to use it. I figured that since I had made the > migration wire protocol support it, we might as well have the > public API too. > > I can work up some QEMU code without much trouble too. The only > missing bit is the ability to compare two virDomainDefPtr > instances and validate that they have the same guest ABI (ie > same list of device models + device addresses). > > Daniel My concern here is that we add a public API and don't implemented it in a driver for 0.9.2 and later on when we implement it we might figure out that we should have made the public API for it different, but we already released it and can't change it anymore. To be fair this concern is quite weak and I don't see how this could actually happen in this particular case, but I'd like to be on the safe side with something that we can't change easily later on anymore.

So, I would appreciate it if we get this actually implemented for 0.9.2, but if not I don't consider this as a showstopper for this patch.