On Mon, Aug 26, 2013 at 3:49 PM, Thomas Thrainer <[email protected]>wrote:
> > > > On Thu, Aug 22, 2013 at 10:16 AM, Helga Velroyen <[email protected]>wrote: > >> This patch factors out the functions that deal with setting >> and modifying the DRBD usermode helper in cluster.py in >> order to make them more unittestable. The unit tests are >> provided as well. No functional changes otherwise. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> lib/cmdlib/cluster.py | 77 >> +++++++++++++++++++++++--------------- >> test/py/cmdlib/cluster_unittest.py | 26 +++++++++++++ >> 2 files changed, 72 insertions(+), 31 deletions(-) >> >> diff --git a/lib/cmdlib/cluster.py b/lib/cmdlib/cluster.py >> index 4b2bd7a..858ccff 100644 >> --- a/lib/cmdlib/cluster.py >> +++ b/lib/cmdlib/cluster.py >> @@ -851,11 +851,11 @@ class LUClusterSetParams(LogicalUnit): >> CheckIpolicyVsDiskTemplates(cluster.ipolicy, >> enabled_disk_templates) >> >> - def CheckPrereq(self): >> - """Check prerequisites. >> + def _CheckDrbdHelper(self, node_uuids): >> + """Check the DRBD usermode helper. >> >> - This checks whether the given params don't conflict and >> - if the given volume group is valid. >> + @type node_uuids: list of strings >> + @param node_uuids: a list of nodes' UUIDs >> >> """ >> if self.op.drbd_helper is not None and not self.op.drbd_helper: >> @@ -864,6 +864,32 @@ class LUClusterSetParams(LogicalUnit): >> " 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) >> + >> + def CheckPrereq(self): >> + """Check prerequisites. >> + >> + This checks whether the given params don't conflict and >> + if the given volume group is valid. >> + >> + """ >> node_uuids = self.owned_locks(locking.LEVEL_NODE) >> self.cluster = cluster = self.cfg.GetClusterInfo() >> >> @@ -886,24 +912,7 @@ class LUClusterSetParams(LogicalUnit): >> self.LogWarning, self.op.shared_file_storage_dir, >> enabled_disk_templates) >> >> - 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) >> + self._CheckDrbdHelper(node_uuids) >> >> # validate params changes >> if self.op.beparams: >> @@ -1111,17 +1120,10 @@ class LUClusterSetParams(LogicalUnit): >> else: >> self.cluster.file_storage_dir = self.op.file_storage_dir >> >> - def Exec(self, feedback_fn): >> - """Change the parameters of the cluster. >> + def _SetDrbdHelper(self, feedback_fn): >> + """Set the DRBD usermode helper. >> >> """ >> - if self.op.enabled_disk_templates: >> - self.cluster.enabled_disk_templates = \ >> - list(set(self.op.enabled_disk_templates)) >> - >> - self._SetVgName(feedback_fn) >> - self._SetFileStorageDir(feedback_fn) >> - >> if self.op.drbd_helper is not None: >> if not constants.DT_DRBD8 in self.cluster.enabled_disk_templates: >> feedback_fn("Note that you specified a drbd user helper, but did >> not" >> @@ -1134,6 +1136,19 @@ class LUClusterSetParams(LogicalUnit): >> else: >> feedback_fn("Cluster DRBD helper already in desired state," >> " not changing") >> + >> + def Exec(self, feedback_fn): >> + """Change the parameters of the cluster. >> + >> + """ >> + if self.op.enabled_disk_templates: >> + self.cluster.enabled_disk_templates = \ >> + list(set(self.op.enabled_disk_templates)) >> + >> + self._SetVgName(feedback_fn) >> + self._SetFileStorageDir(feedback_fn) >> + self._SetDrbdHelper(feedback_fn) >> + >> if self.op.hvparams: >> self.cluster.hvparams = self.new_hvparams >> if self.op.os_hvp: >> diff --git a/test/py/cmdlib/cluster_unittest.py >> b/test/py/cmdlib/cluster_unittest.py >> index 46bb2bd..ff9f14e 100644 >> --- a/test/py/cmdlib/cluster_unittest.py >> +++ b/test/py/cmdlib/cluster_unittest.py >> @@ -509,6 +509,32 @@ class TestLUClusterSetParams(CmdlibTestCase): >> >> self.assertEqual(None, 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] >> > > In one of my recent patches I introduces > `self.cfg.SetEnabledDiskTemplates` which should be used to set the list of > enabled disk templates on the cluster. It also updates the ipolicy, > otherwise we could run into random errors because they don't match... > Ah right, I apparently found that function lateron, but I don't know why I did not use it here. I think when I was writing this patch, they weren't in yet. However, fixed it. > > >> + op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >> + self.ExecOpCode(op) >> + >> + self.assertEqual(drbd_helper, self.cluster.drbd_usermode_helper) >> + self.mcpu.assertLogContainsRegex("DRBD helper already in desired >> state") >> + >> + def testSetDrbdHelper(self): >> + drbd_helper = "/bin/true" >> + self.rpc.call_drbd_helper.return_value = \ >> + self.RpcResultsBuilder() \ >> + .AddSuccessfulNode(self.master, "/bin/true") \ >> + .Build() >> + self.cluster.drbd_usermode_helper = "/bin/false" >> + self.cluster.enabled_disk_templates = [constants.DT_DRBD8] >> > > Same as above. > Sure. > > >> + op = opcodes.OpClusterSetParams(drbd_helper=drbd_helper) >> + self.ExecOpCode(op) >> + >> + self.assertEqual(drbd_helper, self.cluster.drbd_usermode_helper) >> + >> def testBeparams(self): >> beparams = {constants.BE_VCPUS: 32} >> op = opcodes.OpClusterSetParams(beparams=beparams) >> -- >> 1.8.3 >> >> > Rest LGTM, Thanks. > FYI, interdiff: diff --git a/test/py/cmdlib/cluster_unittest.py b/test/py/cmdlib/cluster_unittest.py index 680eadf..0f0c1a7 100644 --- a/test/py/cmdlib/cluster_unittest.py +++ b/test/py/cmdlib/cluster_unittest.py @@ -511,13 +511,13 @@ class TestLUClusterSetParams(CmdlibTestCase): def testResetDrbdHelperDrbdEnabled(self): drbd_helper = "" - self.cluster.enabled_disk_templates = [constants.DT_DRBD8] + self.cfg.SetEnabledDiskTemplates([constants.DT_DRBD8]) 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] + self.cfg.SetEnabledDiskTemplates([constants.DT_DISKLESS]) self.cluster.drbd_usermode_helper = None enabled_disk_templates = [constants.DT_DRBD8] op = opcodes.OpClusterSetParams(
