4:28 a.m.
On Fri, Sep 11, 2015 at 12:05:54PM +0300, Nikolay Shirokovskiy wrote:
> As mentioned in the previous review, I really want to see VZ
> provide the full set of drive callbacks required by the V3
> migration protocol, not just those you happen to need to have
> today. ie add Begin, Finish & Confirm. This in turn makes it
> quite easy to support non-P2P mode which is desirable because
> in P2P mode there is no practical way todo interactive auth
> for the destination connection, but in non-P2P mode the client
> app opens the destination connection, so can do auth if needed.
Ok, I just wanted this to be a matter of a different patchset.
>
> I believe that the following changes to this patch should do
> all that is required. NB, I have only compile tested this,
> not actually run it, since I don't have an active VZ install,
> only the SDK install.
For me the patch you suggested is overkill in part of p2p.
vzDomainMigratePerform3P2PParams follows virDomainMigrateVersion3Full too
close, while vz p2p migration is much simplier that qemu or some abstract case.
As you can see from the fact that we have many versions of the
migration protocol, what appears obviously sufficient when first
implmented, has frequently turned out to be insufficient. So while
you only need 2 calls today, I would not bet on that always being
sufficient in the future. Implementing all phases today provides
better future proofing, should we need to make use of them in the
future, as it allows the potential to implement fixes / workarounds
on the dest, without relying on also lock-step upgrading the host
to add the callers you didn't make to start with.
It would also be desirable in the future, to provide the means to
share the code impl for P2P migration across all the hypervisor
drivers. This would again imply supporting all stages of the v3
migration protocol. If we don't do this now we will create problems
in the future if we wish to change to shared code, because any
shared impl would be calling functions that were not implemented
on older versions of VZ.
1. Confirm phase is noop and this even won't change in future as vz migration
is practically unmanaged direct one. So why call it in p2p at all?
2. Finish phase function is to return migrated domain only(in vz case). Thus it
have meaning only for direct managed case due its API definition.
3. Calling begin phase in p2p is excessive too. We don't use generated dxml, so
this phase is only a stub for direct managed case.
Even if we support changing domain xml in future it is more practical
to use specific functions and not general API which cases a lot
of preparation work and has rather uncomfortable way of passing
parameters.
I'd better just add a small patch to existent vzDomainMigratePerform3Params which
branches on how to get miguri and session_uuid.
As to supporting all API versions. I resent a second version of the patch
that should cover the case of toURI API so that implementing only
version3 params protocol functions would be enough. But the patch
does not cover the case of migration{N} API.
This patch I have provided covers the normal virDomainMigrate API, in
addition to the virDomainMigrateToURI method. As I mentioned, supporting
virDomainMigrate is desirable, because it allows the calling application
to perform arbitrary authentication with the destination libvirtd, which
is not possible when using P2P migration. Supporting the non-P2P migration
mode requires that we provide all the callbacks defined for version 3,
and once we do this, it is sensible to support all the callbacks in the
P2P case too, to ensure we have identical functional behaviour in both
cases.
So in summary, I want to see the full V3 migration protocol implemented
in VZ before this is acceptable for merge. This patch I supplied would
be sufficient to achieve this.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|