On Thu, Nov 06, 2014 at 12:30:43AM +0200, Alex Pyrgiotis wrote:
> Add checks for:
> * number of arguments,
> * instance's compatibility with disk template/nodes
>
> and update existing ones.
>
> Signed-off-by: Alex Pyrgiotis <[email protected]>
>
> diff --git a/lib/cmdlib/instance_set_params.py b/lib/cmdlib/instance_set_
params.py
> index 37ece5b..a4cc576 100644
> --- a/lib/cmdlib/instance_set_params.py
> +++ b/lib/cmdlib/instance_set_params.py
> @@ -89,6 +89,30 @@ class LUInstanceSetParams(LogicalUnit):
> HTYPE = constants.HTYPE_INSTANCE
> REQ_BGL = False
>
> + def GenericGetDiskInfo(self, **params):
Avoid taking all params and prefer `params.get('uuid', None)` with checks
for the values provided here. The linter warns for a reason.
> + """Find a disk object using the provided params.
> +
> + Accept arguments as keywords and use the
GetDiskInfo/GetDiskInfoByName
> + config functions to retrieve the disk info based on these arguments.
> +
> + In case of an error, raise the appropriate exceptions.
> + """
> + name = constants.IDISK_NAME
> + if "uuid" in params:
> + disk = self.cfg.GetDiskInfo(params["uuid"])
> + if disk is None:
> + raise errors.OpPrereqError("No disk was found with this UUID:
%s" %
> + params["uuid"], errors.ECODE_INVAL)
> + elif name in params:
> + disk = self.cfg.GetDiskInfoByName(params[name])
> + if disk is None:
> + raise errors.OpPrereqError("No disk was found with this %s: %s" %
> + (name, params[name]),
errors.ECODE_INVAL)
> + else:
> + raise errors.ProgrammerError("No disk UUID or name was given")
> +
> + return disk
> +
> @staticmethod
> def _UpgradeDiskNicMods(kind, mods, verify_fn):
> assert ht.TList(mods)
> @@ -99,14 +123,15 @@ class LUInstanceSetParams(LogicalUnit):
>
> addremove = 0
> for op, params in mods:
> - if op in (constants.DDM_ADD, constants.DDM_REMOVE):
> + if op in (constants.DDM_ADD, constants.DDM_ATTACH,
> + constants.DDM_REMOVE, constants.DDM_DETACH):
> result.append((op, -1, params))
> addremove += 1
>
> if addremove > 1:
> - raise errors.OpPrereqError("Only one %s add or remove
operation is"
> - " supported at a time" % kind,
> - errors.ECODE_INVAL)
> + raise errors.OpPrereqError("Only one %s
add/attach/remove/detach "
> + "operation is supported at a
time" %
> + kind, errors.ECODE_INVAL)
> else:
> result.append((constants.DDM_MODIFY, op, params))
>
> @@ -120,21 +145,33 @@ class LUInstanceSetParams(LogicalUnit):
> def _CheckMods(kind, mods, key_types, item_fn):
> """Ensures requested disk/NIC modifications are valid.
>
> + Note that the 'attach' action needs a way to refer to the UUID of
the disk,
> + since the disk name is not unique cluster-wide. However, the UUID of
the
> + disk is not settable but rather generated by Ganeti automatically,
> + therefore it cannot be passed as an IDISK parameter. For this
reason, this
> + function will override the checks to accept uuid parameters solely
for the
> + attach action.
> """
> + # Create a key_types copy with the 'uuid' as a valid key type.
> + key_types_attach = key_types.copy()
> + key_types_attach['uuid'] = 'string'
> +
Why not add the UUID as a 'Maybe String' to IDISK_PARAMS_TYPES?
> for (op, _, params) in mods:
> assert ht.TDict(params)
>
> # If 'key_types' is an empty dict, we assume we have an
> # 'ext' template and thus do not ForceDictType
> if key_types:
> - utils.ForceDictType(params, key_types)
> + utils.ForceDictType(params, (key_types if op !=
constants.DDM_ATTACH
> + else key_types_attach))
>
> - if op == constants.DDM_REMOVE:
> + if op in (constants.DDM_REMOVE, constants.DDM_DETACH):
> if params:
> raise errors.OpPrereqError("No settings should be passed when"
> - " removing a %s" % kind,
> + " removing or detaching a %s" %
kind,
> errors.ECODE_INVAL)
> - elif op in (constants.DDM_ADD, constants.DDM_MODIFY):
> + elif op in (constants.DDM_ADD, constants.DDM_ATTACH,
> + constants.DDM_MODIFY):
> item_fn(op, params)
> else:
> raise errors.ProgrammerError("Unhandled operation '%s'" % op)
> @@ -160,6 +197,8 @@ class LUInstanceSetParams(LogicalUnit):
> if name is not None and name.lower() == constants.VALUE_NONE:
> params[constants.IDISK_NAME] = None
>
> + # This check is necessary both when adding and attaching disks
> + if op in (constants.DDM_ADD, constants.DDM_ATTACH):
> CheckSpindlesExclusiveStorage(params, excl_stor, True)
>
> # Check disk access param (only for specific disks)
> @@ -174,6 +213,16 @@ class LUInstanceSetParams(LogicalUnit):
> (self.instance.hypervisor,
access_type),
> errors.ECODE_STATE)
>
> + if op == constants.DDM_ATTACH:
> + if len(params) != 1 or ('uuid' not in params and
> + constants.IDISK_NAME not in params):
> + raise errors.OpPrereqError("Only one argument is permitted in %s
op,"
> + " either %s or uuid" %
(constants.DDM_ATTACH,
> +
constants.IDISK_NAME,
> + ),
> + errors.ECODE_INVAL)
> + self._CheckAttachDisk(params)
> +
> elif op == constants.DDM_MODIFY:
> if constants.IDISK_SIZE in params:
> raise errors.OpPrereqError("Disk size change not possible, use"
> @@ -297,6 +346,26 @@ class LUInstanceSetParams(LogicalUnit):
> (self.op.pnode_uuid, self.op.pnode) = \
> ExpandNodeUuidAndName(self.cfg, self.op.pnode_uuid,
self.op.pnode)
>
> + def _CheckAttachDisk(self, params):
> + """Check if disk can be attached to an instance.
> +
> + Check if the disk and instance have the same template. Also, check
if the
> + disk nodes are visible from the instance.
> + """
> + disk = self.GenericGetDiskInfo(**params) # pylint: disable=W0142
> + if disk.dev_type != self.instance.disk_template:
> + raise errors.OpPrereqError("Instance has '%s' template while disk
has"
> + " '%s' template" %
> + (self.instance.disk_template,
disk.dev_type),
> + errors.ECODE_INVAL)
> +
> + instance_nodes = self.cfg.GetInstanceNodes(self.instance.uuid)
> + if not set(disk.nodes).issubset(set(instance_nodes)):
> + raise errors.OpPrereqError("Disk nodes are %s while the instance's
nodes"
> + " are %s" %
> + (disk.nodes, instance_nodes),
> + errors.ECODE_INVAL)
> +
> def ExpandNames(self):
> self._ExpandAndLockInstance()
> self.needed_locks[locking.LEVEL_NODEGROUP] = []
> @@ -673,12 +742,12 @@ class LUInstanceSetParams(LogicalUnit):
> if self.instance.disk_template in constants.DT_EXT:
> for mod in self.diskmod:
> ext_provider = mod[2].get(constants.IDISK_PROVIDER, None)
> - if mod[0] == constants.DDM_ADD:
> + 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 add" %
> + " '%s' missing, during disk %s" %
> (constants.DT_EXT,
> - constants.IDISK_PROVIDER),
> + constants.IDISK_PROVIDER,
mod[0]),
> errors.ECODE_NOENT)
> elif mod[0] == constants.DDM_MODIFY:
> if ext_provider:
> @@ -698,10 +767,10 @@ class LUInstanceSetParams(LogicalUnit):
>
> if not self.op.wait_for_sync and not self.instance.disks_active:
> for mod in self.diskmod:
> - if mod[0] == constants.DDM_ADD:
> - raise errors.OpPrereqError("Can't add a disk to an instance
with"
> + if mod[0] in (constants.DDM_ADD, constants.DDM_ATTACH):
> + raise errors.OpPrereqError("Can't %s a disk to an instance
with"
> " deactivated disks and"
> - " --no-wait-for-sync given.",
> + " --no-wait-for-sync given" %
mod[0],
> errors.ECODE_INVAL)
>
> if self.op.disks and not self.instance.disks:
> --
> 1.7.10.4
>
--
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
