On 11/19/14, 15:05 PM, Aaron Karper wrote:
> On Fri, Nov 14, 2014 at 11:37:43AM +0200, Constantinos Venetsanopoulos wrote:
>> also cc:ing the list
>>
>> On 11/14/14, 11:36 AM, Constantinos Venetsanopoulos wrote:
>>> Hello Aaron,
>>>
>>> On 11/10/14, 18:04 PM, 'Aaron Karper' via ganeti-devel 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.
>>>>
>>>>> + """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?
>>>>
>>> We had actually done that in the first place, but realized it is
>>> semantically wrong to have the UUID as an IDISK_PARAM, since
>>> IDISK_PARAMs are parameters that are settable by the user at
>>> disk level (e.g. during gnt-instance add). In the other hand the
>>> UUID is automatically generated by Ganeti, so it does not make
>>> much sense to be defined as an IDISK_PARAM, just to pass the
>>> verification checks at this single point. It would be hacky and
>>> error prone in the future. Plus, we would then need explicit
>>> checks in every other part of the code to disallow the user of
>>> setting it.
>>>
>>> So, we decided that the above is the cleaner it could get. There
>>> was also a discussion some years ago about unifying the way
>>> diskparams and IDISK_PARAMS work (and even replacing
>>> IDISK_PARAMs with the diskparams mechanism), but required
>>> a lot of cleanup work and design, and was left out at some point.
>>>
>>> Hope the above make sense.
> That makes sense. If nobody else disagrees, you can leave this as is.
Ack.
Thanks,
Constantinos
>>> Thanks,
>>> Constantinos
>>>
>>>>> 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
