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.
> >
> > 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
>
--
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
