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