[Libvirt-cim] [PATCH 0 of 3] (#3) [RFC] Add external migration checks

[Heidi Eckhart]

As Jay already said - the code is of that high quality that we are used 
to get from you :). So I have no concerns about the implementation of 
your approach. But I have concerns in regard to the approach how this 
feature is added to the providers.

First: its not a (let me say) common CIM way to add a property array to 
a class and let the client use this array as de-facto command line 
interface. What you want to do is giving the client a possibility to 
enhance the providers build in functionality - without rewriting the 
provider. And this functionality should be configurable by the client. 
The idea is valid and the implementation is straightforward.
But what the client gets is a very powerful access point to - not only 
the resource managed by this class, but also to - all other resources on 
the system. So please don't blame me for bringing up security issues, 
but you do not have under control what kind of scripts will be put into 
this directory. As you officially need to point clients to this 
directory to make clear how to use the CheckParamaters property, its 
well known. But deep in my heard I trust the system admins, that they do 
not copy some strange scripts in there ;). So I know that this argument 
can be very fast invalidated. On the other hand, all scripts ... also of 
different authors, because you can have more than one mgmt. app on one 
system ... have to make sure, that they can handle input parameters, 
that might follow absolutely no logic. But ok, this is the 
responsibility of the client to harden its scripts in that way. So these 
issues are bound to the implementation approach and something I could 
live with. What does not mean, that I like this time-bomb.

As I said before, the way how this feature is added to the schema is 
definitely not CIM conform. Its a property misused as command line 
interface. That's not what a property is - yet by definition of the word 
itself ;). But I don't only want to vote against this patch set without 
having a proposal. Normally you start evaluating the necessary 
functionality and enhance the existing model. This could mean adding 
methods or properties. But we can not foresee what the client wants to 
do by its scripts. On the other hand I can imagine that the amount of 
checks is limited. First by the intention of CheckParameters - which is 
de-facto a replacement for the methods CheckIsVSMigratable...(). And 
second by the hypervisor itself. So I suggest to go the CIM way for 
implementing this functionality. What you want is enhancing 
CheckIsVSMigratable...() and also the executing methods 
MigrateVSTo...(), because these methods have to do the check again, as 
between the client's CheckIsVSMigratable...() result and the 
MigrateVSTo...() request is enough time, that the result of the check 
can became invalid in the meantime. I looked a bit through the Migration 
Profile and found a class that I suggest to use for this functionality. 
That's the description of this class:
"CIM_VirtualSystemMigrationSettingData defines the parameters to control 
a virtual system migration implementation, as managed by a 
CIM_VirtualSystemMigrationService. It is expected that a migration 
implementation will subclass this class to add implementation-specific 
migration options."
An instance of this class is transported to all CheckIsVSMigratable...() 
and MigrateVSTo...() via the MigrationSettingData parameter. So we stay 
absolutely conform to the Migration Profile. The Xen and KVM subclasses 
can check properties, that are well know to cause migration problems. 
E.g. if the migration depends on the architecture, this class can get a 
property of maybe TargetArchitecture, which needs to be checked by the 
provider. Your approach of letting the client parameterize scripts 
through this class might look like this:
[Description("An array containing the scripts to be executed as part of 
the check, if the migration is possible.")]
string ScriptsToExecute[];
[Description("An array containing the parameters given as input to the 
scripts defined by ScriptsToExecute. Each entry in this array is related 
to the same array entry in ScriptToExecute. A list of parameters for one 
script is divided by space characters.")]
string ScriptsParameterList[];

What do you - and all the others in this list interested in this feature 
- think about this ?

Thanks ... Heidi

-- 
Regards

Heidi Eckhart
Software Engineer
IBM Linux Technology Center - Open Hypervisor