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
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list