* Michele Tartara <[email protected]> [2013-12-10 11:00:05 +0100]: > On Tue, Dec 10, 2013 at 10:56 AM, Guido Trotter <[email protected]> wrote: > > On Tue, Dec 10, 2013 at 10:48 AM, Michele Tartara <[email protected]> > > wrote: > >> On Tue, Dec 10, 2013 at 10:14 AM, Dimitris Aragiorgis <[email protected]> > >> wrote: > >>> Disks of ext template are allowed to have arbitrary parameters > >>> stored in Disk object's params slot. Those parameters can be > >>> passed during creation of a new disk, either in LUInstanceCreate() > >>> or in LUInsanceSetParams(). Still those parameters can not be > >>> changed afterwards. With this patch we override this limitation. > >>> > >>> Signed-off-by: Dimitris Aragiorgis <[email protected]> > >>> --- > >>> lib/cmdlib/instance.py | 13 +++++++------ > >>> 1 file changed, 7 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > >>> index be30315..e01dd13 100644 > >>> --- a/lib/cmdlib/instance.py > >>> +++ b/lib/cmdlib/instance.py > >>> @@ -2308,10 +2308,6 @@ class LUInstanceSetParams(LogicalUnit): > >>> if constants.IDISK_SIZE in params: > >>> raise errors.OpPrereqError("Disk size change not possible, use" > >>> " grow-disk", errors.ECODE_INVAL) > >>> - if len(params) > 2: > >>> - raise errors.OpPrereqError("Disk modification doesn't support" > >>> - " additional arbitrary parameters", > >>> - errors.ECODE_INVAL) > >> > >> The OpPrereqError exception used to be raised for every template. Now, > >> this patch wants to prevent it to be raised for the ext template > >> (which is fine) but it actually removes it completely from all the > >> template. > >> Shouldn't it be something like (pseudocode): > >> > >> if len(params) > 2 and template != EXT: > >> error "Disk modification supports additional arbitrary parameters > >> only for the EXT template" > >> > > > > Also the params dict in upgradeconfig is allowed to stay for all > > templates, not just for ext. > > Shouldn't we allow here to modify *all* parameters? > > > > Thanks, > > > > Guido > > > > > Fine for me, but in that case I also suggest renaming the patch without a > reference to the "ext" template. > > Thanks, > Michele > >
I got lost you here. IMHO the len(params) > 2 check is useless.
In case of template other than ext we pass IDISK_PARAMS_TYPES dict
to check parameters validity. This is needed during new disk
creation. Upon disk modification and in _ModifyDisk() we only change
MODE and NAME. So the most correct check inside VerifyDiskModification
would be with something like the following interdiff (with a bit
more verbose error reporting..):
diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py
index 33461d9..d894b97 100644
--- a/lib/cmdlib/instance.py
+++ b/lib/cmdlib/instance.py
@@ -2277,8 +2277,7 @@ class LUInstanceSetParams(LogicalUnit):
else:
raise errors.ProgrammerError("Unhandled operation '%s'" % op)
- @staticmethod
- def _VerifyDiskModification(op, params):
+ def _VerifyDiskModification(self, op, params):
"""Verifies a disk modification.
"""
@@ -2308,6 +2307,13 @@ class LUInstanceSetParams(LogicalUnit):
if constants.IDISK_SIZE in params:
raise errors.OpPrereqError("Disk size change not possible, use"
" grow-disk", errors.ECODE_INVAL)
+
+ # Disk modification supports changing only the disk name and mode.
+ # Changing arbitrary parameters is allowed only for ext disk template",
+ if self.instance.disk_template != constants.DT_EXT:
+ allowed_mods = [constants.IDISK_MODE, constants.IDISK_NAME]
+ utils.ForceDictType(params, allowed_mods)
+
name = params.get(constants.IDISK_NAME, None)
if name is not None and name.lower() == constants.VALUE_NONE:
params[constants.IDISK_NAME] = None
What do you think?
> --
> Google Germany GmbH
> Dienerstr. 12
> 80331 München
>
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
> Geschäftsführer: Graham Law, Christine Elizabeth Flores
signature.asc
Description: Digital signature
