Re: [libvirt] [PATCH] migration: support all toURI and proto combos
On Tue, Sep 08, 2015 at 13:43:09 +0300, Nikolay Shirokovskiy wrote:
Current implementation of 'toURI' migration interfaces does
not support all
combinations of interface versions and protocol versions. For example 'toURI2'
with p2p flag will not migrate if driver supports only v3params proto.
In general, this patch is pretty large and hard to review, I suggest
splitting it into a series of several shorter patches. They all need to
compile and work, but it shouldn't be too hard in this case.
This is not convinient as drivers that starts to support migration
have to
manually support older versions of protocol. I guess this should be done in
one place, namely here.
Well, the thing is all the code you're changing runs on a *client* and
then the appropriate API calls are sent as RPC to a daemon. So just
checking what APIs are implemented by the *client's* remote driver to
choose what API will be called on the daemon is wrong. The client can be
new enough to support all migration APIs while the daemon may be pretty
old supporting only a few of them. Thus, for backward compatibility, the
client has to either check what API is supported by the daemon (via
VIR_DRV_SUPPORTS_FEATURE(..., VIR_DRV_FEATURE_MIGRATION_...)) or it has
to be conservative and call the oldest API which supports the parameters
passed by the application/user.
Moreover, various versions of virDomainMigrateToURI are not really about
the migration protocol to be used. They differ only in the set of
parameters, the actual migration protocol will be decided by the daemon
itself after making a connection to the destination daemon. That said,
it should be fairly easy to implement all the entry points in a driver
while still allowing only protocol version 3.
Another issue is that there are a lot of code duplication in
implementation of
toURI interfaces and it is not obvious from code how are they related.
This implementation uses extensible parameters as intermediate parameters
representation. This is possible as interfaces are done backward compatible in
terms of parameters and later versions supports all parameters of former
versions.
Another change that is convinient to add here is reusing the code for p2p and
direct migration. The only major difference between them is how URIs are passed
in places that supports both connection and migration uri. Now this difference
is plain in code. See comments in code for details.
Sounds like this could be separated.
Minor p2p vs direct difference is that direct not support proto with
extensible
parameters. This behaviour is saved.
Unobvious check of migration dconnuri in case of p2p migration is actually
protection from migration to localhost (from cover letter to patch 59d13aae
that introduced it). So reformat this check to make it goal obvious.
Another very good candidate for a separate patch.
==Behaviour changes
Previous implementation of direct migration demands that proto v1 is
implemented even if proto v3 is used. As this is somewhat against the purpuse
of this patch this behaviour is dropped.
Overall, if you see a way to refactor and improve the code so that it's
more readable and maintainable, just go ahead, but be careful about this
kind of changes in behavior.
Jirka