In short, may be factor out common p2p code that now resides in libvirt-domain.c and reuse it for vz? Looks like vz driver is not a good place for such common and kinda complicated code. On 11.09.2015 14:06, Nikolay Shirokovskiy wrote:
On 11.09.2015 12:28, Daniel P. Berrange wrote:
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.
Well I'm not against implementing the full set of protocol functions. This will be done anyway as a part of supporting direct managed migrating scheme as you recommend. So it is not a point of dispute.
The point of dispute is what p2p migration should look like. Well I understand the argument of ease of fixes/workaronds. But if there is even a disire in future share p2p code why not to do it from the start?
Look vzDomainMigratePerform3P2PParams is identical to virDomainMigrateVersion3Full except for that last one supports plain version 3 protocol. Can we reuse virDomainMigrateVersion3Full? Thus at least for vz p2p and direct managed will be identical from procedural POV, the only difference is who executes the migration procedure - client or daemon. So the last statement will be not only a declaration but implemented in code.
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.
Looks like you misunderstood me. Here i just wanted you to review https://www.redhat.com/archives/libvir-list/2015-September/msg00397.html patch. and estimate it as an alternative to helpers methods you suggested in https://www.redhat.com/archives/libvir-list/2015-September/msg00372.html.
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