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

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