On Fri, Nov 21, 2014 at 1:21 PM, 'Aaron Karper' via ganeti-devel
<[email protected]> wrote:
> Since the current check only checks all disks for the disabled template,
> it didn't catch diskless instances that were being disabled. The new
> logic handles that separately.
>
> Signed-off-by: Aaron Karper <[email protected]>
> ---
> lib/cmdlib/cluster/__init__.py | 8 ++++++++
> test/py/cmdlib/cluster_unittest.py | 8 ++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/lib/cmdlib/cluster/__init__.py b/lib/cmdlib/cluster/__init__.py
> index a92149c..95a0e00 100644
> --- a/lib/cmdlib/cluster/__init__.py
> +++ b/lib/cmdlib/cluster/__init__.py
> @@ -1165,6 +1165,14 @@ class LUClusterSetParams(LogicalUnit):
> "Cannot disable disk template '%s', because there is at least
> one"
> " disk using it:\n * %s" % (disk_template, "\n *
> ".join(disk_desc)),
> errors.ECODE_STATE)
> + if constants.DT_DISKLESS in disabled_disk_templates:
> + instances = self.cfg.GetAllInstancesInfo()
> + for inst in instances.values():
> + if not inst.disks:
> + raise errors.OpPrereqError(
> + "Cannot disable disk template 'diskless', because there is at"
> + " least one instance using it:\n * %s" % inst.name,
> + errors.ECODE_STATE)
>
> @staticmethod
> def _CheckInstanceCommunicationNetwork(network, warning_fn):
> diff --git a/test/py/cmdlib/cluster_unittest.py
> b/test/py/cmdlib/cluster_unittest.py
> index 803c373..82c25f5 100644
> --- a/test/py/cmdlib/cluster_unittest.py
> +++ b/test/py/cmdlib/cluster_unittest.py
> @@ -884,6 +884,14 @@ class TestLUClusterSetParams(CmdlibTestCase):
> ipolicy={constants.IPOLICY_DTS: enabled_disk_templates})
> self.ExecOpCodeExpectOpPrereqError(op, "Cannot disable disk template")
>
> + def testDisableDiskTemplateWithExistingInstanceDiskless(self):
> + enabled_disk_templates = [constants.DT_PLAIN]
> + self.cfg.AddNewInstance(disks=[])
Please, invert this line and the previous one: at first sight, it
seems like you are adding a diskless instance in a system where the
PLAIN disk template is active, but actually that set of disk templates
does not become active until the following OpClusterSetParam.
Defining it after adding the instance makes this much more obvious and readable.
> + op = opcodes.OpClusterSetParams(
> + enabled_disk_templates=enabled_disk_templates,
> + ipolicy={constants.IPOLICY_DTS: enabled_disk_templates})
> + self.ExecOpCodeExpectOpPrereqError(op, "Cannot disable disk template")
> +
> def testVgNameNoLvmDiskTemplateEnabled(self):
> vg_name = "test_vg"
> self.cfg.SetEnabledDiskTemplates([constants.DT_DISKLESS])
> --
> 2.1.0.rc2.206.gedb03e5
>
Rest LGTM, no need to resend.
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
