On 11/10/2014 06:04 PM, Aaron Karper wrote:
> 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] <mailto:[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.
>
ACK
>> + """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?
>
The discussion with Constantinos has covered this.
>>
>> 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
--
Alex | [email protected]
