On Thu, Sep 27, 2012 at 05:51:05PM +0300, Constantinos Venetsanopoulos wrote: > On 09/27/2012 04:26 PM, Iustin Pop wrote: > >On Wed, Sep 26, 2012 at 05:38:20PM +0300, Constantinos Venetsanopoulos wrote: > >>The option is added to allow us the passing of arbitrary ext-params > >>during disk addition (gnt-instance modify --disk add: ...), same as > >>we do during instance add. > >> > >>The option is needed because during gnt-instance modify parameters' > >>validation, we do not know the type of the disk template > >>(whereas in gnt-instance add we find it from the -t option). > >> > >>The option is called 'arbit' and not 'ext' params because it may be > >>useful in other contexts too, in the future. > >> > >>The corresponding prereq checks for the existence of the 'provider' > >>parameter in case of an 'ext' template are also taken care off in > >>this commit. It serves as a general introduction of the 'ext' > >>template in gnt-instance modify. > >> > >>Signed-off-by: Constantinos Venetsanopoulos <[email protected]> > >> > >>Conflicts: > >> > >> lib/cmdlib.py > >Conflicts? :) > > Oooops :) > I've been rebasing a hundred times (always resolving conflicts) the last > two weeks to catch up with you guys so this must have slipped. > Could you please remove it when applying?
Sure, no problem. > >This is not a review of the patch, but just reading the commit message, > >I find "--allow-arbit-params" too hard to read. What about > >"--flexible-params"? > > I think "allow" should be in there. Maybe it becomes more clear when > you see the code. For "arbit" I'm not sure either. In any case, we can > change that to "--flexible-params" during the review if you think that > fits best at the end. Hmm, I read the code again and I am still undecided. If I understood the patch correctly, the problem it tries to solve is that during the _opcode_ validation, which is done outside of the LU context, we don't have access to the disk template, hence no way to check the parameters to the opcode. And you fix this by adding a parameter which directs the code to use the extended disk template specific code. If this is correct, then I'm not sure it's the best solution. Since we can't do actual validation at the opcode layer, then maybe we shouldn't; what about delaying validation until we are in an LU context, and we have access to the instance? Yes, the job will fail later, but that would seem cleaner than "propagating" the disk template into the opcode layer. Thoughts? thanks, iustin
