Re: [libvirt] [PATCH] Support a new peer-to-peer migration mode & public API

On Tue, Sep 22, 2009 at 02:43:20PM +0200, Daniel Veillard wrote: > [ Sending again due to mail output problems ] > > On Thu, Sep 17, 2009 at 06:25:01PM +0100, Daniel P. Berrange wrote: > > Introduces several new public API options for migration > > > > - VIR_MIGRATE_PEER2PEER: With this flag the client only > > invokes the virDomainMigratePerform method, expecting > > the source host driver to do whatever is required to > > complete the entire migration process. > > - VIR_MIGRATE_TUNNELLED: With this flag the actual data > > for migration will be tunnelled over the libvirtd RPC > > channel. This requires that VIR_MIGRATE_PEER2PEER is > > also set. > > Hum, I would rather add that when entering libvirt.c entry point > set explicitely VIR_MIGRATE_PEER2PEER if VIR_MIGRATE_TUNNELLED is asked > for, that's an internal impl. detail. I didn't do that, because I wanted to leave open the option to supporting TUNNELLED mode, without PEER2PEER mode. I don't really know what that would look like, but I think it is better future proofing to not presume one implies the other.

> > - virDomainMigrateToURI: This is variant of the existing > > virDomainMigrate method which can be used when the > > VIR_MIGRATE_PEER2PEER flag is set. The benefit of this > > method is that no virConnectPtr object is required for > > the destination host, only a destination URI. > > Sounds good if we have proper error handling. I'm also wondering > how authentification and access control can be handled in that mode, > the normal method of access somehow garantee that the user credentials > can be checked in some ways, but if it's direct daemon to daemon > talking how can this be performed ? The client application authenticates with the source libvirtd in the normal manner. The source libvirtd will authenticate with the destination libvirtd directly. The client application will not be authenticating with the destination libvirtd itself. While this may sound like a security issue, I don't think it really is. It is more of an authorization problem - eg the admin needs to declare somewhere which libvirtd instances are allowed to migrate guests between then. This authorization problem is really outside the scope of the client application or our public app API

> > > > +int virDomainMigrateToURI (virDomainPtr domain, const char *duri, > > + unsigned long flags, const char *dname, > > + unsigned long bandwidth); > > + > > s/duri/dest/ or target, the fact it's a connection URI should be made > obvious in the function comment. Yep, the function comment later only clearly describes what URIs are acceptable. > [...] > > + } else { > > + if (flags & VIR_MIGRATE_TUNNELLED) { > > + virLibConnError(domain->conn, VIR_ERR_OPERATION_INVALID, > > + _("cannot perform tunnelled migration without using peer2peer flag")); > > + goto error; > > + } > > I don't really agree, we should just or the flag and hide this from > the API user. I really did want to leave open the option of relaxing this restriction in the future, and thus didn't want to automatically set the PEER2PEER flag - once its set we can never unset it in the future. > > + > > + > > /* > > * Not for public use. This function is part of the internal > > * implementation of migration in the remote case. > > diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h > > index 5913798..5f1a7fe 100644 > > --- a/src/libvirt_internal.h > > +++ b/src/libvirt_internal.h > > @@ -17,6 +17,7 @@ > > * License along with this library; if not, write to the Free Software > > * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > > * > > + * NB This file is (secret) ABI sensitive. Append only > > Huh ??? The methods here are encoded in the wire protocol ABI, but not included in the public libvirt.h header. Hence they are ABI sensitive, but secret & thus append only like the rest of our API > > diff --git a/src/xen_unified.c b/src/xen_unified.c > > index dfa9ca5..954b187 100644 > > --- a/src/xen_unified.c > > +++ b/src/xen_unified.c > > @@ -455,8 +455,11 @@ static int > > xenUnifiedSupportsFeature (virConnectPtr conn ATTRIBUTE_UNUSED, int feature) > > { > > switch (feature) { > > - case VIR_DRV_FEATURE_MIGRATION_V1: return 1; > > - default: return 0; > > + case VIR_DRV_FEATURE_MIGRATION_V1: > > + case VIR_DRV_FEATURE_MIGRATION_P2P: > > + return 1; > > + default: > > + return 0; > > } > > } > > > > diff --git a/src/xend_internal.c b/src/xend_internal.c > > index 7f55116..da5c039 100644 > > --- a/src/xend_internal.c > > +++ b/src/xend_internal.c > > @@ -4400,6 +4400,12 @@ xenDaemonDomainMigratePerform (virDomainPtr domain, > > strcpy (live, "1"); > > flags &= ~VIR_MIGRATE_LIVE; > > } > > + /* Trivially support this in Xen, since XenD on dest is always > > + * ready to accept incoming migration */ > > + if ((flags & VIR_MIGRATE_PEER2PEER)) { > > + flags &= ~VIR_MIGRATE_PEER2PEER; > > + } > > + /* XXX we could easily do tunnelled migration too if we want to */ > > if (flags != 0) { > > virXendError (conn, VIR_ERR_NO_SUPPORT, > > "%s", _("xenDaemonDomainMigrate: unsupported flag")); > > Sounds and looks good to me, with the small caveat of the required > VIR_MIGRATE_PEER2PEER flag and making sure it would work later with > QEmu/KVM before commiting this. > > I must admit peer-2-peer left me a bit puzzled at the beginning > it wasn't obvious what kind of transfer it actually meant. Only de > the documentation clarified it but I have no better suggestion :-) > "direct" maybe ... I called it peer-2-peer, because the 2 libvirtd instances are talking to each other p2p, rather than having all communcation go via the client application.