On Tue, Nov 18, 2014 at 12:11:44PM +0100, Michele Tartara wrote: > On Mon, Nov 17, 2014 at 11:29 AM, 'Aaron Karper' via ganeti-devel > <[email protected]> wrote: > > So far the error only showed that there are disks of a certain template > > and that that template can therefore not be disabled. The new error also > > shows on what instance (if any) it is mounted. > > > > Signed-off-by: Aaron Karper <[email protected]> > > --- > > lib/cmdlib/cluster/__init__.py | 15 +++++++++++++-- > > lib/config.py | 30 ++++++++++++++++++++++++++++++ > > test/py/cmdlib/cluster_unittest.py | 2 +- > > 3 files changed, 44 insertions(+), 3 deletions(-) > > > > diff --git a/lib/cmdlib/cluster/__init__.py b/lib/cmdlib/cluster/__init__.py > > index 28ddb96..6244e17 100644 > > --- a/lib/cmdlib/cluster/__init__.py > > +++ b/lib/cmdlib/cluster/__init__.py > > @@ -1150,10 +1150,21 @@ class LUClusterSetParams(LogicalUnit): > > > > """ > > for disk_template in disabled_disk_templates: > > - if self.cfg.HasAnyDiskOfType(disk_template): > > Why implementing all of this here, instead of changing the > HasAnyDiskOfType function?
how about giving all disks of a certain type? commit 84ad8393ee37810fdec5d9010131d5d1ba67c404 Author: Aaron Karper <[email protected]> Date: Tue Nov 18 14:03:50 2014 +0100 Replace HasAnyDiskOfType by the useful DisksOfType diff --git a/lib/cmdlib/cluster/__init__.py b/lib/cmdlib/cluster/__init__.py index 6244e17..a92149c 100644 --- a/lib/cmdlib/cluster/__init__.py +++ b/lib/cmdlib/cluster/__init__.py @@ -971,7 +971,7 @@ class LUClusterSetParams(LogicalUnit): errors.ECODE_INVAL) if self.op.vg_name is not None and not self.op.vg_name: - if self.cfg.HasAnyDiskOfType(constants.DT_PLAIN): + if self.cfg.DisksOfType(constants.DT_PLAIN): raise errors.OpPrereqError("Cannot disable lvm storage while lvm-based" " instances exist", errors.ECODE_INVAL) @@ -1122,7 +1122,7 @@ class LUClusterSetParams(LogicalUnit): if drbd_enabled: raise errors.OpPrereqError("Cannot disable drbd helper while" " DRBD is enabled.", errors.ECODE_STATE) - if self.cfg.HasAnyDiskOfType(constants.DT_DRBD8): + if self.cfg.DisksOfType(constants.DT_DRBD8): raise errors.OpPrereqError("Cannot disable drbd helper while" " drbd-based instances exist", errors.ECODE_INVAL) @@ -1150,12 +1150,11 @@ class LUClusterSetParams(LogicalUnit): """ for disk_template in disabled_disk_templates: - disks_with_type = [d for d in self.cfg.GetAllDiskInfo().values() - if d.dev_type == disk_template] + disks_with_type = self.cfg.DisksOfType(disk_template) if disks_with_type: disk_desc = [] for disk in disks_with_type: - instance_uuid = self.cfg.GetInstanceForDisk(d.uuid) + instance_uuid = self.cfg.GetInstanceForDisk(disk.uuid) instance = self.cfg.GetInstanceInfo(instance_uuid) if instance: instance_desc = "on " + instance.name diff --git a/lib/config.py b/lib/config.py index cfddeab..4d34810 100644 --- a/lib/config.py +++ b/lib/config.py @@ -3205,11 +3205,11 @@ class ConfigWriter(object): return self._ConfigData().cluster @_ConfigSync(shared=1) - def HasAnyDiskOfType(self, dev_type): + def DisksOfType(self, dev_type): """Check if in there is at disk of the given type in the configuration. """ - return self._ConfigData().HasAnyDiskOfType(dev_type) + return self._ConfigData().DisksOfType(dev_type) @_ConfigSync(shared=1) def GetDetachedConfig(self): diff --git a/lib/objects.py b/lib/objects.py index ba72ed4..a6610a0 100644 > > > + disks_with_type = [d for d in self.cfg.GetAllDiskInfo().values() > > + if d.dev_type == disk_template] > > + if disks_with_type: > > + disk_desc = [] > > + for disk in disks_with_type: > > + instance_uuid = self.cfg.GetInstanceForDisk(d.uuid) This line is also fixed in the interdiff > > + instance = self.cfg.GetInstanceInfo(instance_uuid) > > + if instance: > > + instance_desc = "on " + instance.name > > + else: > > + instance_desc = "detached" > > + disk_desc.append("%s (%s)" % (disk, instance_desc)) > > raise errors.OpPrereqError( > > "Cannot disable disk template '%s', because there is at least > > one" > > - " instance using it." % disk_template, > > + " disk using it:\n * %s" % (disk_template, "\n * > > ".join(disk_desc)), > > errors.ECODE_STATE) > > > > @staticmethod > > diff --git a/lib/config.py b/lib/config.py > > index 816e947..cfddeab 100644 > > --- a/lib/config.py > > +++ b/lib/config.py > > @@ -3557,6 +3557,36 @@ class ConfigWriter(object): > > if not self._offline: > > self._wconfd.FlushConfig() > > > > + @_ConfigSync(shared=1) > > + def GetAllDiskInfo(self): > > + """Get the configuration of all disks. > > + > > + @rtype: dict > > + @return: dict of (disk, disk_info), where disk_info is what > > + would GetDiskInfo return for disk > > + """ > > + return self._UnlockedGetAllDiskInfo() > > + > > + def _UnlockedGetAllDiskInfo(self): > > + return dict((disk_uuid, self._UnlockedGetDiskInfo(disk_uuid)) > > + for disk_uuid in self._UnlockedGetDiskList()) > > + > > + def _UnlockedGetDiskList(self): > > + return self._ConfigData().disks.keys() > > + > > + @_ConfigSync(shared=1) > > + def GetInstanceForDisk(self, disk_uuid): > > + """Returns the instance the disk is currently attached to. > > + > > + @type disk_uuid: string > > + @param disk_uuid: the identifier of the disk in question. > > + > > + @rtype: string > > + @return: uuid of instance the disk is attached to. > > + """ > > + for inst_uuid, inst_info in > > self._UnlockedGetAllInstancesInfo().items(): > > + if disk_uuid in inst_info.disks: > > + return inst_uuid > > > > class DetachedConfig(ConfigWriter): > > def __init__(self, config_data): > > diff --git a/test/py/cmdlib/cluster_unittest.py > > b/test/py/cmdlib/cluster_unittest.py > > index fca96ad..803c373 100644 > > --- a/test/py/cmdlib/cluster_unittest.py > > +++ b/test/py/cmdlib/cluster_unittest.py > > @@ -866,7 +866,7 @@ class TestLUClusterSetParams(CmdlibTestCase): > > op = opcodes.OpClusterSetParams( > > enabled_disk_templates=new_disk_templates, > > ipolicy={constants.IPOLICY_DTS: new_disk_templates}) > > - self.ExecOpCodeExpectOpPrereqError(op, "least one instance using it") > > + self.ExecOpCodeExpectOpPrereqError(op, "least one disk using it") > > > > def testEnabledDiskTemplatesWithoutVgName(self): > > enabled_disk_templates = [constants.DT_PLAIN] > > -- > > 2.1.0.rc2.206.gedb03e5 > > > > Thanks, > Michele > > -- > 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
