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. > > >> + 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. Cheers, Helga
