Re: [libvirt] [PATCH v5 2/5] vz: add migration backbone code

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 >