On Mon, Aug 26, 2013 at 3:20 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 function that checks the DRBD >> usermode helper in bootstrap (cluster init) in order to make >> it more testable. It also contains the unit tests. Otherwise, >> not functional changes. >> > > s/not/no/ > Sure. > > >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> lib/bootstrap.py | 38 >> ++++++++++++++++++++++++------------ >> test/py/ganeti.bootstrap_unittest.py | 32 ++++++++++++++++++++++++++++++ >> 2 files changed, 57 insertions(+), 13 deletions(-) >> >> diff --git a/lib/bootstrap.py b/lib/bootstrap.py >> index 1b862c5..fe5a1f4 100644 >> --- a/lib/bootstrap.py >> +++ b/lib/bootstrap.py >> @@ -472,6 +472,30 @@ def _RestrictIpolicyToEnabledDiskTemplates(ipolicy, >> enabled_disk_templates): >> ipolicy[constants.IPOLICY_DTS] = restricted_disk_templates >> >> >> +def _InitCheckDrbdHelper(drbd_helper, >> + get_helper_fn=drbd.DRBD8.GetUsermodeHelper): >> + """Checks the DRBD usermode helper. >> + >> + @type drbd_helper: string >> + @param drbd_helper: name of the DRBD usermode helper that the system >> should >> + use >> + >> + """ >> + if drbd_helper is not None: >> + try: >> + curr_helper = get_helper_fn() >> + except errors.BlockDeviceError, err: >> + raise errors.OpPrereqError("Error while checking drbd helper" >> + " (specify --no-drbd-storage if you are >> not" >> + " using drbd): %s" % str(err), >> + errors.ECODE_ENVIRON) >> + if drbd_helper != curr_helper: >> + raise errors.OpPrereqError("Error: requiring %s as drbd helper but >> %s" >> + " is the current helper" % (drbd_helper, >> + >> curr_helper), >> + errors.ECODE_INVAL) >> + >> + >> def InitCluster(cluster_name, mac_prefix, # pylint: disable=R0913, R0914 >> master_netmask, master_netdev, file_storage_dir, >> shared_file_storage_dir, candidate_pool_size, >> secondary_ip=None, >> @@ -569,19 +593,7 @@ def InitCluster(cluster_name, mac_prefix, # pylint: >> disable=R0913, R0914 >> if vgstatus: >> raise errors.OpPrereqError("Error: %s" % vgstatus, >> errors.ECODE_INVAL) >> >> - if drbd_helper is not None: >> - try: >> - curr_helper = drbd.DRBD8.GetUsermodeHelper() >> - except errors.BlockDeviceError, err: >> - raise errors.OpPrereqError("Error while checking drbd helper" >> - " (specify --no-drbd-storage if you are >> not" >> - " using drbd): %s" % str(err), >> - errors.ECODE_ENVIRON) >> - if drbd_helper != curr_helper: >> - raise errors.OpPrereqError("Error: requiring %s as drbd helper but >> %s" >> - " is the current helper" % (drbd_helper, >> - >> curr_helper), >> - errors.ECODE_INVAL) >> + _InitCheckDrbdHelper(drbd_helper) >> >> logging.debug("Stopping daemons (if any are running)") >> result = utils.RunCmd([pathutils.DAEMON_UTIL, "stop-all"]) >> diff --git a/test/py/ganeti.bootstrap_unittest.py b/test/py/ >> ganeti.bootstrap_unittest.py >> index 82c4a16..a0e5135 100755 >> --- a/test/py/ganeti.bootstrap_unittest.py >> +++ b/test/py/ganeti.bootstrap_unittest.py >> @@ -130,5 +130,37 @@ class >> TestRestrictIpolicyToEnabledDiskTemplates(unittest.TestCase): >> self.assertEqual(ipolicy[constants.IPOLICY_DTS], >> [constants.DT_PLAIN]) >> >> >> +class TestInitCheckDrbdHelper(unittest.TestCase): >> + >> + def testHelperNone(self): >> + current_helper = "/bin/helper" >> + drbd_helper = None >> + mock_helper_fn = mock.Mock(return_value=current_helper) >> + bootstrap._InitCheckDrbdHelper(drbd_helper, >> get_helper_fn=mock_helper_fn) >> + >> + def testHelperOk(self): >> + current_helper = "/bin/helper" >> + drbd_helper = "/bin/helper" >> + mock_helper_fn = mock.Mock(return_value=current_helper) >> + bootstrap._InitCheckDrbdHelper(drbd_helper, >> get_helper_fn=mock_helper_fn) >> + >> + def testWrongHelper(self): >> + current_helper = "/bin/otherhelper" >> + drbd_helper = "/bin/helper" >> + mock_helper_fn = mock.Mock(return_value=current_helper) >> + self.assertRaises(errors.OpPrereqError, >> + bootstrap._InitCheckDrbdHelper, >> + drbd_helper, get_helper_fn=mock_helper_fn) >> + >> + def testHelperCheckFails(self): >> + current_helper = "/bin/otherhelper" >> + drbd_helper = "/bin/helper" >> + mock_helper_fn = mock.Mock(return_value=current_helper) >> + mock_helper_fn.side_effect = errors.BlockDeviceError >> + self.assertRaises(errors.OpPrereqError, >> + bootstrap._InitCheckDrbdHelper, >> + drbd_helper, get_helper_fn=mock_helper_fn) >> + >> + >> if __name__ == "__main__": >> testutils.GanetiTestProgram() >> -- >> 1.8.3 >> >> > Rest LGTM. > > Note that you could also use @testutils.patch_object instead of > introducing a new parameter, as is it done (incidentally for the drbd.DRBD8 > class as well) in ganeti.storage.drbd_unittest.py:440. > Thanks for pointing to the example. I am not a fan of making the function an extra parameter just to be able to mock it. I followed your example and am sending the interdiff FYI. diff --git a/lib/bootstrap.py b/lib/bootstrap.py index e0aac86..cb2f0f4 100644 --- a/lib/bootstrap.py +++ b/lib/bootstrap.py @@ -472,8 +472,7 @@ def _RestrictIpolicyToEnabledDiskTemplates(ipolicy, enabled_disk_templates): ipolicy[constants.IPOLICY_DTS] = restricted_disk_templates -def _InitCheckDrbdHelper(drbd_helper, drbd_enabled, - get_helper_fn=drbd.DRBD8.GetUsermodeHelper): +def _InitCheckDrbdHelper(drbd_helper, drbd_enabled): """Checks the DRBD usermode helper. @type drbd_helper: string @@ -486,7 +485,7 @@ def _InitCheckDrbdHelper(drbd_helper, drbd_enabled, if drbd_helper is not None: try: - curr_helper = get_helper_fn() + curr_helper = drbd.DRBD8.GetUsermodeHelper() except errors.BlockDeviceError, err: raise errors.OpPrereqError("Error while checking drbd helper" " (disable drbd with --enabled-disk-templates" diff --git a/test/py/ganeti.bootstrap_unittest.py b/test/py/ ganeti.bootstrap_unittest.py index 577918f..60270ae 100755 --- a/test/py/ganeti.bootstrap_unittest.py +++ b/test/py/ganeti.bootstrap_unittest.py @@ -27,6 +27,7 @@ import unittest from ganeti import bootstrap from ganeti import constants +from ganeti.storage import drbd from ganeti import errors from ganeti import pathutils @@ -132,48 +133,46 @@ class TestRestrictIpolicyToEnabledDiskTemplates(unittest.TestCase): class TestInitCheckDrbdHelper(unittest.TestCase): - def testNoDrbd(self): + @testutils.patch_object(drbd.DRBD8, "GetUsermodeHelper") + def testNoDrbd(self, drbd_mock_get_usermode_helper): drbd_enabled = False drbd_helper = None - mock_helper_fn = mock.Mock() - mock_helper_fn.side_effect = NotImplemented - bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled, - get_helper_fn=mock_helper_fn) + bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled) - def testHelperNone(self): + @testutils.patch_object(drbd.DRBD8, "GetUsermodeHelper") + def testHelperNone(self, drbd_mock_get_usermode_helper): drbd_enabled = True current_helper = "/bin/helper" drbd_helper = None - mock_helper_fn = mock.Mock(return_value=current_helper) - bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled, - get_helper_fn=mock_helper_fn) + drbd_mock_get_usermode_helper.return_value=current_helper + bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled) - def testHelperOk(self): + @testutils.patch_object(drbd.DRBD8, "GetUsermodeHelper") + def testHelperOk(self, drbd_mock_get_usermode_helper): drbd_enabled = True current_helper = "/bin/helper" drbd_helper = "/bin/helper" - mock_helper_fn = mock.Mock(return_value=current_helper) - bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled, - get_helper_fn=mock_helper_fn) + drbd_mock_get_usermode_helper.return_value=current_helper + bootstrap._InitCheckDrbdHelper(drbd_helper, drbd_enabled) - def testWrongHelper(self): + @testutils.patch_object(drbd.DRBD8, "GetUsermodeHelper") + def testWrongHelper(self, drbd_mock_get_usermode_helper): drbd_enabled = True current_helper = "/bin/otherhelper" drbd_helper = "/bin/helper" - mock_helper_fn = mock.Mock(return_value=current_helper) + drbd_mock_get_usermode_helper.return_value=current_helper self.assertRaises(errors.OpPrereqError, bootstrap._InitCheckDrbdHelper, - drbd_helper, drbd_enabled, get_helper_fn=mock_helper_fn) + drbd_helper, drbd_enabled) - def testHelperCheckFails(self): + @testutils.patch_object(drbd.DRBD8, "GetUsermodeHelper") + def testHelperCheckFails(self, drbd_mock_get_usermode_helper): drbd_enabled = True - current_helper = "/bin/otherhelper" drbd_helper = "/bin/helper" - mock_helper_fn = mock.Mock(return_value=current_helper) - mock_helper_fn.side_effect = errors.BlockDeviceError + drbd_mock_get_usermode_helper.side_effect=errors.BlockDeviceError self.assertRaises(errors.OpPrereqError, bootstrap._InitCheckDrbdHelper, - drbd_helper, drbd_enabled, get_helper_fn=mock_helper_fn) + drbd_helper, drbd_enabled) Cheers, Helga
