On Tue, Dec 10, 2013 at 12:05 PM, Dimitris Aragiorgis <[email protected]> wrote: > * 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?
With this interdiff, it seems OK to me. Could you please resend the patch as a whole, so it's easier to apply? Thanks, Michele -- 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
