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... > + 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. > + 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. -- 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
