On Tue, Nov 18, 2014 at 2:08 PM, Aaron Karper <[email protected]> wrote: > 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
Sounds good! LGTM, 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
