Makes sense. I'm just worried that the change will expose bugs that might hardcode the default values in the dict and there would be no way to reset them to default. I guess we should address this checking in qa.
G On 10 Dec 2013 13:31, "Constantinos Venetsanopoulos" <[email protected]> wrote: > On 12/10/2013 02:26 PM, Guido Trotter wrote: > >> On Tue, Dec 10, 2013 at 1:03 PM, 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. >>> >>> Additionally, for other disk templates we allow modifying only >>> name and mode. If any other parameter is passed >>> _VerifyDiskModification() will raise an exception. >>> >>> Why not allowing it also for others? >> > > Which ones exactly? I don't think I'm following here. > > Do you mean the "vgname"? If yes, I think this belongs > to a different patch that will introduce such functionality. > > Thanks, > Constantinos > > Signed-off-by: Dimitris Aragiorgis <[email protected]> >>> --- >>> lib/cmdlib/instance.py | 23 +++++++++++++++-------- >>> 1 file changed, 15 insertions(+), 8 deletions(-) >>> >>> diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py >>> index be30315..0a0d29b 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): >>> >> Can't you leave this as static and pass the instance in? >> >> """Verifies a disk modification. >>> >>> """ >>> @@ -2308,10 +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) >>> - if len(params) > 2: >>> - raise errors.OpPrereqError("Disk modification doesn't support" >>> - " additional arbitrary parameters", >>> - 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] >>> >> this belongs to constants, not here. >> >> + 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 >>> @@ -3182,8 +3184,7 @@ class LUInstanceSetParams(LogicalUnit): >>> ("disk/%d" % idx, "add:size=%s,mode=%s" % (disk.size, >>> disk.mode)), >>> ]) >>> >>> - @staticmethod >>> - def _ModifyDisk(idx, disk, params, _): >>> + def _ModifyDisk(self, idx, disk, params, _): >>> """Modifies a disk. >>> >>> """ >>> @@ -3196,6 +3197,12 @@ class LUInstanceSetParams(LogicalUnit): >>> disk.name = params.get(constants.IDISK_NAME) >>> changes.append(("disk.name/%d" % idx, disk.name)) >>> >>> + for key, value in params.iteritems(): >>> + if (key not in [constants.IDISK_MODE, constants.IDISK_NAME] and >>> + self.instance.disk_template == constants.DT_EXT): >>> + disk.params[key] = value >>> + changes.append(("disk.params:%s/%d" % (key, idx), value)) >>> + >>> return changes >>> >>> def _RemoveDisk(self, idx, root, _): >>> >> Thanks, >> >> Guido >> >> >
