LGTM, thanks On Wed, 22 Apr 2015 at 11:20 'Hrvoje Ribicic' via ganeti-devel < [email protected]> wrote:
> The external disk template provider checks were happening in a part of > code separate from the checks of other modifications. This inadvertently > caused an error because some calculations of disk templates were > duplicated. To fix the issue and prevent further issues, this patch > folds these modifications in. > > Signed-off-by: Hrvoje Ribicic <[email protected]> > --- > lib/cmdlib/instance_set_params.py | 46 > ++++++++++----------------------------- > 1 file changed, 11 insertions(+), 35 deletions(-) > > diff --git a/lib/cmdlib/instance_set_params.py > b/lib/cmdlib/instance_set_params.py > index b54f569..e8c6409 100644 > --- a/lib/cmdlib/instance_set_params.py > +++ b/lib/cmdlib/instance_set_params.py > @@ -53,9 +53,9 @@ from ganeti.cmdlib.common import INSTANCE_DOWN, \ > GetUpdatedParams, CheckInstanceState, ExpandNodeUuidAndName, \ > IsValidDiskAccessModeCombination, AnnotateDiskParams > from ganeti.cmdlib.instance_storage import CalculateFileStorageDir, \ > - CheckNodesFreeDiskPerVG, CheckRADOSFreeSpace, > CheckSpindlesExclusiveStorage, \ > - ComputeDiskSizePerVG, ComputeDisksInfo, CreateDisks, \ > - CreateSingleBlockDev, GenerateDiskTemplate, \ > + CheckDiskExtProvider, CheckNodesFreeDiskPerVG, CheckRADOSFreeSpace, \ > + CheckSpindlesExclusiveStorage, ComputeDiskSizePerVG, ComputeDisksInfo, \ > + CreateDisks, CreateSingleBlockDev, GenerateDiskTemplate, \ > IsExclusiveStorageEnabledNodeUuid, ShutdownInstanceDisks, \ > WaitForSync, WipeOrCleanupDisks, AssembleInstanceDisks > from ganeti.cmdlib.instance_utils import BuildInstanceHookEnvByObject, \ > @@ -210,6 +210,7 @@ class LUInstanceSetParams(LogicalUnit): > # This check is necessary both when adding and attaching disks > if op in (constants.DDM_ADD, constants.DDM_ATTACH): > CheckSpindlesExclusiveStorage(params, excl_stor, True) > + CheckDiskExtProvider(params, disk_type) > > # Check disk access param (only for specific disks) > if disk_type in constants.DTS_HAVE_ACCESS: > @@ -244,10 +245,13 @@ class LUInstanceSetParams(LogicalUnit): > if not utils.AllDiskOfType(disk_info, [constants.DT_EXT]): > utils.ForceDictType(params, > constants.MODIFIABLE_IDISK_PARAMS_TYPES) > else: > - # We have to check that 'access' parameter can not be modified > - if constants.IDISK_ACCESS in params: > - raise errors.OpPrereqError("Disk 'access' parameter change is" > - " not possible", errors.ECODE_INVAL) > + # We have to check that the 'access' and 'disk_provider' > parameters > + # cannot be modified > + for param in [constants.IDISK_ACCESS, constants.IDISK_PROVIDER]: > + if param in params: > + raise errors.OpPrereqError("Disk '%s' parameter change is" > + " not possible" % param, > + errors.ECODE_INVAL) > > name = params.get(constants.IDISK_NAME, None) > if name is not None and name.lower() == constants.VALUE_NONE: > @@ -773,34 +777,6 @@ class LUInstanceSetParams(LogicalUnit): > self._CheckMods("disk", self.op.disks, {}, ver_fn) > > self.diskmod = PrepareContainerMods(self.op.disks, None) > - disk_info = self.cfg.GetInstanceDisks(self.instance.uuid) > - > - # Check the validity of the `provider' parameter > - for mod in self.diskmod: > - dev_type = mod[2].get(constants.IDISK_TYPE, > disk_info[mod[1]].dev_type) > - if dev_type == constants.DT_EXT: > - ext_provider = mod[2].get(constants.IDISK_PROVIDER, None) > - if mod[0] in (constants.DDM_ADD, constants.DDM_ATTACH): > - if ext_provider is None: > - raise errors.OpPrereqError("Instance template is '%s' and > parameter" > - " '%s' missing, during disk %s" % > - (constants.DT_EXT, > - constants.IDISK_PROVIDER, mod[0]), > - errors.ECODE_NOENT) > - elif mod[0] == constants.DDM_MODIFY: > - if ext_provider: > - raise errors.OpPrereqError("Parameter '%s' is invalid during > disk" > - " modification" % > - constants.IDISK_PROVIDER, > - errors.ECODE_INVAL) > - else: > - ext_provider = mod[2].get(constants.IDISK_PROVIDER, None) > - if ext_provider is not None: > - raise errors.OpPrereqError("Parameter '%s' is only valid for" > - " instances of type '%s'" % > - (constants.IDISK_PROVIDER, > - constants.DT_EXT), > - errors.ECODE_INVAL) > > if not self.op.wait_for_sync and not self.instance.disks_active: > for mod in self.diskmod: > -- > 2.2.0.rc0.207.ga3a616c > >
