On Mon, May 20, 2013 at 5:11 PM, Bernardo Dal Seno <[email protected]>wrote:
> Masterd checks that specifications for new disks don't include spindles > when exclusive storage is disabled. > > Signed-off-by: Bernardo Dal Seno <[email protected]> > --- > lib/cmdlib/instance.py | 27 +++++++++++++++++++++++---- > lib/cmdlib/instance_storage.py | 26 ++++++++++++++++++++++---- > 2 files changed, 45 insertions(+), 8 deletions(-) > > diff --git a/lib/cmdlib/instance.py b/lib/cmdlib/instance.py > index 6498ce2..392ae49 100644 > --- a/lib/cmdlib/instance.py > +++ b/lib/cmdlib/instance.py > @@ -1030,6 +1030,13 @@ class LUInstanceCreate(LogicalUnit): > raise errors.OpPrereqError("Disk template %s not supported with" > " exclusive storage" % > self.op.disk_template, > errors.ECODE_STATE) > In case of DRBD, only one of the two nodes has to have exclusive storage enabled to skip the "else" branch. The "then" branch would clearly produce an error then, but in the spirit of defensive programming a "compat.all(map(has_es, nodes))" instead of "else" would reveal the indent a bit better and be more future proof. > + else: > + for disk in self.disks: > + if constants.IDISK_SPINDLES in disk: > + raise errors.OpPrereqError("Spindles in instance disks cannot > be" > + " specified when exclusive storage > is not" > + " active", > + errors.ECODE_INVAL) > > nodenames = [pnode.name] + self.secondaries > > @@ -2209,7 +2216,7 @@ class LUInstanceSetParams(LogicalUnit): > raise errors.ProgrammerError("Unhandled operation '%s'" % op) > > @staticmethod > - def _VerifyDiskModification(op, params): > + def _VerifyDiskModification(op, params, excl_stor): > """Verifies a disk modification. > > """ > @@ -2235,6 +2242,14 @@ class LUInstanceSetParams(LogicalUnit): > if name is not None and name.lower() == constants.VALUE_NONE: > params[constants.IDISK_NAME] = None > > + spindles = params.get(constants.IDISK_SPINDLES, None) > + if spindles is not None: > + if not excl_stor: > + raise errors.OpPrereqError("Spindles in instance disks cannot > be" > + " specified when exclusive storage > is not" > + " active", > + errors.ECODE_INVAL) > + > Isn't that basically the same check as above, but written in different terms? Couldn't we factor out a helper function? > elif op == constants.DDM_MODIFY: > if constants.IDISK_SIZE in params: > raise errors.OpPrereqError("Disk size change not possible, use" > @@ -2611,14 +2626,18 @@ class LUInstanceSetParams(LogicalUnit): > instance = self.instance > self.diskparams = self.cfg.GetInstanceDiskParams(instance) > > + excl_stor = compat.any( > + rpc.GetExclusiveStorageForNodeNames(self.cfg, instance.all_nodes) > + ) > + > Why "any" and not "all"? Is it enough that one node has exclusive storage enabled? I know, that there can only be one node if exclusive storage is enabled, but why assume that every reader always knows? > # Check disk modifications. This is done here and not in > CheckArguments > # (as with NICs), because we need to know the instance's disk template > + ver_fn = lambda op, par: self._VerifyDiskModification(op, par, > excl_stor) > if instance.disk_template == constants.DT_EXT: > - self._CheckMods("disk", self.op.disks, {}, > - self._VerifyDiskModification) > + self._CheckMods("disk", self.op.disks, {}, ver_fn) > else: > self._CheckMods("disk", self.op.disks, constants.IDISK_PARAMS_TYPES, > - self._VerifyDiskModification) > + ver_fn) > > self.diskmod = _PrepareContainerMods(self.op.disks, None) > > diff --git a/lib/cmdlib/instance_storage.py > b/lib/cmdlib/instance_storage.py > index 76fc529..6669404 100644 > --- a/lib/cmdlib/instance_storage.py > +++ b/lib/cmdlib/instance_storage.py > @@ -343,10 +343,13 @@ def ComputeDisks(op, default_vg): > constants.IDISK_NAME: name, > } > > - if constants.IDISK_METAVG in disk: > - new_disk[constants.IDISK_METAVG] = disk[constants.IDISK_METAVG] > - if constants.IDISK_ADOPT in disk: > - new_disk[constants.IDISK_ADOPT] = disk[constants.IDISK_ADOPT] > + for key in [ > + constants.IDISK_METAVG, > + constants.IDISK_ADOPT, > + constants.IDISK_SPINDLES, > + ]: > + if key in disk: > + new_disk[key] = disk[key] > > # For extstorage, demand the `provider' option and add any > # additional parameters (ext-params) to the dict > @@ -758,6 +761,21 @@ class LUInstanceRecreateDisks(LogicalUnit): > > assert not self.glm.is_owned(locking.LEVEL_NODE_ALLOC) > > + if self.op.nodes: > + nodes = self.op.nodes > + else: > + nodes = instance.all_nodes > + excl_stor = compat.any( > + rpc.GetExclusiveStorageForNodeNames(self.cfg, nodes) > + ) > Why "any"? > + if not excl_stor: > + for (_, new_params) in self.disks: > + if constants.IDISK_SPINDLES in new_params: > + raise errors.OpPrereqError("Spindles in instance disks cannot > be" > + " specified when exclusive storage > is not" > + " active", > + errors.ECODE_INVAL) > + > Looks again like the above two checks. > def Exec(self, feedback_fn): > """Recreate the disks. > > -- > 1.8.2.1 > > Thanks, Thomas. -- Thomas Thrainer | Software Engineer | [email protected] | Google Germany GmbH Dienerstr. 12 80331 München Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Graham Law, Katherine Stephens
