On Tue, Aug 27, 2013 at 1:15 PM, Helga Velroyen <[email protected]> wrote:
> > > > On Mon, Aug 26, 2013 at 4:24 PM, Thomas Thrainer <[email protected]>wrote: > >> >> >> >> On Thu, Aug 22, 2013 at 10:16 AM, Helga Velroyen <[email protected]>wrote: >> >>> As in the previous patch, the option '--no-drbd-storage' >>> is deprectated, because it is subsumed by the non-inclusion >>> of 'drbd' in the list of enabled disk templates. >>> >>> Signed-off-by: Helga Velroyen <[email protected]> >>> --- >>> lib/client/gnt_cluster.py | 21 ++++---- >>> lib/cmdlib/cluster.py | 73 >>> +++++++++++++++++++-------- >>> test/py/cmdlib/cluster_unittest.py | 38 ++++++++++++-- >>> test/py/ganeti.client.gnt_cluster_unittest.py | 41 +++++++++++---- >>> 4 files changed, 131 insertions(+), 42 deletions(-) >>> >>> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py >>> index daf2595..887e7b6 100644 >>> --- a/lib/client/gnt_cluster.py >>> +++ b/lib/client/gnt_cluster.py >>> @@ -1044,17 +1044,19 @@ def _GetVgName(opts, enabled_disk_templates): >>> return vg_name >>> >>> >>> -def _GetDrbdHelper(opts): >>> +def _GetDrbdHelper(opts, enabled_disk_templates): >>> """Determine the DRBD usermode helper. >>> >>> """ >>> drbd_helper = opts.drbd_helper >>> - if not opts.drbd_storage and opts.drbd_helper: >>> - raise errors.OpPrereqError( >>> - "Options --no-drbd-storage and --drbd-usermode-helper >>> conflict.") >>> - >>> - if not opts.drbd_storage: >>> - drbd_helper = "" >>> + if enabled_disk_templates: >>> >> >> When can `enabled_disk_templates` be None? Note that [] would work... >> > > enabled_disk_templates is None if you use gnt-cluster modify to change a > different parameter (e.g. drbd-usermode-helper). This code basically makes > sure, _if_ the user modifies the enabled_disk_templates and the drbd_helper > at the same time, then we check for consistency, otherwise we don't and the > sanity checks agains the current enabled_disk_templates are done in > cmdlib/cluster.py. > Ah, OK, thanks for the explanation. > > >> >> >>> + drbd_enabled = constants.DT_DRBD8 in enabled_disk_templates >>> + # This raises an exception for historic reasons. It might be a good >>> idea >>> + # to allow users to set a DRBD helper when DRBD storage is not >>> enabled. >>> + if not drbd_enabled and opts.drbd_helper: >>> + raise errors.OpPrereqError( >>> + "Setting a DRBD usermode helper when DRBD is not enabled is" >>> + " not allowed.") >>> return drbd_helper >>> >>> >>> @@ -1068,7 +1070,8 @@ def SetClusterParams(opts, args): >>> @return: the desired exit code >>> >>> """ >>> - if not (opts.vg_name is not None or opts.drbd_helper or >>> + if not (opts.vg_name is not None or >>> + opts.drbd_helper is not None or >>> opts.enabled_hypervisors or opts.hvparams or >>> opts.beparams or opts.nicparams or >>> opts.ndparams or opts.diskparams or >>> @@ -1103,7 +1106,7 @@ def SetClusterParams(opts, args): >>> vg_name = _GetVgName(opts, enabled_disk_templates) >>> >>> try: >>> - drbd_helper = _GetDrbdHelper(opts) >>> + drbd_helper = _GetDrbdHelper(opts, enabled_disk_templates) >>> except errors.OpPrereqError, e: >>> ToStderr(str(e)) >>> return 1 >>> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py >>> index b563409..b0714b3 100644 >>> --- a/lib/cmdlib/cluster.py >>> +++ b/lib/cmdlib/cluster.py >>> @@ -851,37 +851,66 @@ class LUClusterSetParams(LogicalUnit): >>> CheckIpolicyVsDiskTemplates(cluster.ipolicy, >>> enabled_disk_templates) >>> >>> - def _CheckDrbdHelper(self, node_uuids): >>> + def _CheckDrbdHelperOnNodes(self, drbd_helper, node_uuids): >>> + """Checks whether the set DRBD helper actually exists on the nodes. >>> + >>> + @type drbd_helper: string >>> + @param drbd_helper: path of the drbd usermode helper binary >>> + @type node_uuids: list of strings >>> + @param node_uuids: list of node UUIDs to check for the helper >>> + >>> + """ >>> + # checks given drbd helper on all nodes >>> + helpers = self.rpc.call_drbd_helper(node_uuids) >>> + for (_, ninfo) in self.cfg.GetMultiNodeInfo(node_uuids): >>> + if ninfo.offline: >>> + self.LogInfo("Not checking drbd helper on offline node %s", >>> + ninfo.name) >>> + continue >>> + msg = helpers[ninfo.uuid].fail_msg >>> + if msg: >>> + raise errors.OpPrereqError("Error checking drbd helper on node" >>> + " '%s': %s" % (ninfo.name, msg), >>> + errors.ECODE_ENVIRON) >>> + node_helper = helpers[ninfo.uuid].payload >>> + if node_helper != drbd_helper: >>> + raise errors.OpPrereqError("Error on node '%s': drbd helper is >>> %s" % >>> + (ninfo.name, node_helper), >>> + errors.ECODE_ENVIRON) >>> + >>> + def _CheckDrbdHelper(self, node_uuids, drbd_enabled, >>> drbd_gets_enabled): >>> """Check the DRBD usermode helper. >>> >>> @type node_uuids: list of strings >>> @param node_uuids: a list of nodes' UUIDs >>> + @type drbd_enabled: boolean >>> + @param drbd_enabled: whether DRBD will be enabled after this >>> operation >>> + (no matter if it was disabled before or not) >>> + @type drbd_gets_enabled: boolen >>> + @param drbd_gets_enabled: true if DRBD was disabled before this >>> + operation, but will be enabled afterwards >>> >>> """ >>> - if self.op.drbd_helper is not None and not self.op.drbd_helper: >>> + if self.op.drbd_helper == '': >>> + if drbd_enabled: >>> + raise errors.OpPrereqError("Cannot disable drbd helper while" >>> + " DRBD is enabled.") >>> if self.cfg.HasAnyDiskOfType(constants.LD_DRBD8): >>> raise errors.OpPrereqError("Cannot disable drbd helper while" >>> " drbd-based instances exist", >>> errors.ECODE_INVAL) >>> >>> - if self.op.drbd_helper: >>> - # checks given drbd helper on all nodes >>> - helpers = self.rpc.call_drbd_helper(node_uuids) >>> - for (_, ninfo) in self.cfg.GetMultiNodeInfo(node_uuids): >>> - if ninfo.offline: >>> - self.LogInfo("Not checking drbd helper on offline node %s", >>> - ninfo.name) >>> - continue >>> - msg = helpers[ninfo.uuid].fail_msg >>> - if msg: >>> - raise errors.OpPrereqError("Error checking drbd helper on >>> node" >>> - " '%s': %s" % (ninfo.name, msg), >>> - errors.ECODE_ENVIRON) >>> - node_helper = helpers[ninfo.uuid].payload >>> - if node_helper != self.op.drbd_helper: >>> - raise errors.OpPrereqError("Error on node '%s': drbd helper >>> is %s" % >>> - (ninfo.name, node_helper), >>> - errors.ECODE_ENVIRON) >>> + else: >>> + if self.op.drbd_helper is not None and drbd_enabled: >>> + self._CheckDrbdHelperOnNodes(self.op.drbd_helper, node_uuids) >>> + else: >>> + if drbd_gets_enabled: >>> + current_drbd_helper = >>> self.cfg.GetClusterInfo().drbd_usermode_helper >>> + if current_drbd_helper is not None: >>> + self._CheckDrbdHelperOnNodes(current_drbd_helper, >>> node_uuids) >>> + else: >>> + raise errors.OpPrereqError("Cannot enable DRBD without a" >>> + " DRBD usermode helper set.") >>> >>> def CheckPrereq(self): >>> """Check prerequisites. >>> @@ -912,7 +941,9 @@ class LUClusterSetParams(LogicalUnit): >>> self.LogWarning, self.op.shared_file_storage_dir, >>> enabled_disk_templates) >>> >>> - self._CheckDrbdHelper(node_uuids) >>> + drbd_enabled = constants.DT_DRBD8 in enabled_disk_templates >>> + drbd_gets_enabled = constants.DT_DRBD8 in new_enabled_disk_templates >>> + self._CheckDrbdHelper(node_uuids, drbd_enabled, drbd_gets_enabled) >>> >>> # validate params changes >>> if self.op.beparams: >>> diff --git a/test/py/cmdlib/cluster_unittest.py >>> b/test/py/cmdlib/cluster_unittest.py >>> index ff9f14e..680eadf 100644 >>> --- a/test/py/cmdlib/cluster_unittest.py >>> +++ b/test/py/cmdlib/cluster_unittest.py >>> @@ -501,7 +501,7 @@ class TestLUClusterSetParams(CmdlibTestCase): >>> >>> self.mcpu.assertLogContainsRegex("but did not enable") >>> >>> - def testResetDrbdHelper(self): >>> + def testResetDrbdHelperDrbdDisabled(self): >>> drbd_helper = "" >>> self.cfg.SetEnabledDiskTemplates([constants.DT_DISKLESS]) >>> op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >>> @@ -509,13 +509,45 @@ class TestLUClusterSetParams(CmdlibTestCase): >>> >>> self.assertEqual(None, self.cluster.drbd_usermode_helper) >>> >>> + def testResetDrbdHelperDrbdEnabled(self): >>> + drbd_helper = "" >>> + self.cluster.enabled_disk_templates = [constants.DT_DRBD8] >>> >> >> See previous patch. >> > > Fixed there. > > >> >> >>> + op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >>> + self.ExecOpCodeExpectOpPrereqError( >>> + op, "Cannot disable drbd helper while DRBD is enabled.") >>> + >>> + def testEnableDrbdNoHelper(self): >>> + self.cluster.enabled_disk_templates = [constants.DT_DISKLESS] >>> >> >> As above. >> > > Fixed there as well. > > >> >> >>> + self.cluster.drbd_usermode_helper = None >>> + enabled_disk_templates = [constants.DT_DRBD8] >>> + op = opcodes.OpClusterSetParams( >>> + enabled_disk_templates=enabled_disk_templates) >>> + self.ExecOpCodeExpectOpPrereqError( >>> + op, "Cannot enable DRBD without a DRBD usermode helper set") >>> + >>> + def testEnableDrbdHelperSet(self): >>> + drbd_helper = "/bin/random_helper" >>> + self.rpc.call_drbd_helper.return_value = \ >>> + self.RpcResultsBuilder() \ >>> + .AddSuccessfulNode(self.master, drbd_helper) \ >>> + .Build() >>> + self.cfg.SetEnabledDiskTemplates([constants.DT_DISKLESS]) >>> >> >> Use this consistently. I'm glad that it was not as well hidden as I >> feared ;-). >> > > Hehe, don't worry. I actually think you committed it while I was working > on the series and I forgot update the earlier patches. > > >> >> >>> + self.cluster.drbd_usermode_helper = drbd_helper >>> + enabled_disk_templates = [constants.DT_DRBD8] >>> + op = opcodes.OpClusterSetParams( >>> + enabled_disk_templates=enabled_disk_templates, >>> + ipolicy={constants.IPOLICY_DTS: enabled_disk_templates}) >>> + self.ExecOpCode(op) >>> + >>> + self.assertEqual(drbd_helper, self.cluster.drbd_usermode_helper) >>> + >>> def testDrbdHelperAlreadySet(self): >>> drbd_helper = "/bin/true" >>> self.rpc.call_drbd_helper.return_value = \ >>> self.RpcResultsBuilder() \ >>> .AddSuccessfulNode(self.master, "/bin/true") \ >>> .Build() >>> - self.cluster.enabled_disk_templates = [constants.DT_DISKLESS] >>> + self.cfg.SetEnabledDiskTemplates([constants.DT_DISKLESS]) >>> op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >>> self.ExecOpCode(op) >>> >>> @@ -529,7 +561,7 @@ class TestLUClusterSetParams(CmdlibTestCase): >>> .AddSuccessfulNode(self.master, "/bin/true") \ >>> .Build() >>> self.cluster.drbd_usermode_helper = "/bin/false" >>> - self.cluster.enabled_disk_templates = [constants.DT_DRBD8] >>> + self.cfg.SetEnabledDiskTemplates([constants.DT_DRBD8]) >>> op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >>> self.ExecOpCode(op) >>> >>> diff --git a/test/py/ganeti.client.gnt_cluster_unittest.py b/test/py/ >>> ganeti.client.gnt_cluster_unittest.py >>> index 66aafcc..6aa9c62 100755 >>> --- a/test/py/ganeti.client.gnt_cluster_unittest.py >>> +++ b/test/py/ganeti.client.gnt_cluster_unittest.py >>> @@ -310,31 +310,54 @@ class InitDrbdHelper(unittest.TestCase): >>> >>> class GetDrbdHelper(unittest.TestCase): >>> >>> + def setUp(self): >>> + unittest.TestCase.setUp(self) >>> + self.enabled_disk_templates = [] >>> + >>> + def enableDrbd(self): >>> + self.enabled_disk_templates = [constants.DT_DRBD8] >>> + >>> >> >> That's really too similar to InitDrbdHelper... >> > > I argued before, why I cannot merge Init- and GetHelper, but to make you > feel better, I made a common super class for those two test classes so that > they share the helper functions and setUp method. Interdiff: > > diff --git a/test/py/ganeti.client.gnt_cluster_unittest.py b/test/py/ > ganeti.client.gnt_cluster_unittest.py > index 4ec26b5..288eff5 100755 > --- a/test/py/ganeti.client.gnt_cluster_unittest.py > +++ b/test/py/ganeti.client.gnt_cluster_unittest.py > @@ -260,7 +260,7 @@ class TestEpo(unittest.TestCase): > self.assertEqual(result, constants.EXIT_FAILURE) > > > -class InitDrbdHelper(unittest.TestCase): > +class DrbdHelperTestCase(unittest.TestCase): > > def setUp(self): > unittest.TestCase.setUp(self) > @@ -272,6 +272,9 @@ class InitDrbdHelper(unittest.TestCase): > def disableDrbd(self): > self.enabled_disk_templates = [constants.DT_DISKLESS] > > + > +class InitDrbdHelper(DrbdHelperTestCase): > + > def testNoDrbdNoHelper(self): > opts = mock.Mock() > opts.drbd_helper = None > @@ -308,17 +311,7 @@ class InitDrbdHelper(unittest.TestCase): > self.assertEquals(opts.drbd_helper, helper) > > > -class GetDrbdHelper(unittest.TestCase): > - > - def setUp(self): > - unittest.TestCase.setUp(self) > - self.enabled_disk_templates = [] > - > - def enableDrbd(self): > - self.enabled_disk_templates = [constants.DT_DRBD8] > - > - def disableDrbd(self): > - self.enabled_disk_templates = [constants.DT_DISKLESS] > +class GetDrbdHelper(DrbdHelperTestCase): > > def testNoDrbdNoHelper(self): > opts = mock.Mock() > > > >> >> >>> + def disableDrbd(self): >>> + self.enabled_disk_templates = [constants.DT_DISKLESS] >>> + >>> def testNoDrbdNoHelper(self): >>> opts = mock.Mock() >>> - opts.drbd_storage = False >>> + self.disableDrbd() >>> + opts.drbd_helper = None >>> + helper = gnt_cluster._GetDrbdHelper(opts, >>> self.enabled_disk_templates) >>> + self.assertEquals(None, helper) >>> + >>> + def testNoTemplateInfoNoHelper(self): >>> + opts = mock.Mock() >>> opts.drbd_helper = None >>> - helper = gnt_cluster._GetDrbdHelper(opts) >>> - self.assertEquals("", helper) >>> + helper = gnt_cluster._GetDrbdHelper(opts, None) >>> + self.assertEquals(None, helper) >>> + >>> + def testNoTemplateInfoHelper(self): >>> + opts = mock.Mock() >>> + opts.drbd_helper = "/bin/true" >>> + helper = gnt_cluster._GetDrbdHelper(opts, None) >>> + self.assertEquals(opts.drbd_helper, helper) >>> >>> def testNoDrbdHelper(self): >>> opts = mock.Mock() >>> - opts.drbd_storage = None >>> + self.disableDrbd() >>> opts.drbd_helper = "/bin/true" >>> - self.assertRaises(errors.OpPrereqError, gnt_cluster._GetDrbdHelper, >>> opts) >>> + self.assertRaises(errors.OpPrereqError, gnt_cluster._GetDrbdHelper, >>> opts, >>> + self.enabled_disk_templates) >>> >>> def testDrbdNoHelper(self): >>> opts = mock.Mock() >>> - opts.drbd_storage = True >>> + self.enableDrbd() >>> opts.drbd_helper = None >>> - helper = gnt_cluster._GetDrbdHelper(opts) >>> + helper = gnt_cluster._GetDrbdHelper(opts, >>> self.enabled_disk_templates) >>> self.assertEquals(None, helper) >>> >>> def testDrbdHelper(self): >>> opts = mock.Mock() >>> - opts.drbd_storage = True >>> + self.enableDrbd() >>> opts.drbd_helper = "/bin/true" >>> - helper = gnt_cluster._GetDrbdHelper(opts) >>> + helper = gnt_cluster._GetDrbdHelper(opts, >>> self.enabled_disk_templates) >>> self.assertEquals(opts.drbd_helper, helper) >> >> >>> -- >>> 1.8.3 >>> >>> >> Rest LGMT (with InitDrbdHelper and GetDrbdHelper merged as suggested in a >> previous patch). >> > > LGMT = "looks good, me too"? ;) > > About the Init/GetDrbdHelper: see my explanation before. > > LGTM, thanks (this time spelled correctly...) Cheers, Thomas Cheers, > Helga > -- 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, Christine Elizabeth Flores
