Re: [PATCH master 09/21] cmdlib: Add checks for attach/detach

[Constantinos Venetsanopoulos]

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 <apyr...@grnet.gr
>> <mailto:apyr...@grnet.gr>>
>>> 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.
>
> 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