Hi! thanks for the review.
On Mon, Aug 26, 2013 at 3:42 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 in gnt_cluster >> (related to cluster init and cluster modify) which >> deal with setting / determining the drbd usermode helper >> to make them more testable. Unit tests for the extracted >> functions are provided as well. No function changes >> otherwise. >> >> Signed-off-by: Helga Velroyen <[email protected]> >> --- >> lib/client/gnt_cluster.py | 146 >> ++++++++++++++++++-------- >> test/py/ganeti.client.gnt_cluster_unittest.py | 62 +++++++++++ >> 2 files changed, 167 insertions(+), 41 deletions(-) >> >> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py >> index 1419800..90d3220 100644 >> --- a/lib/client/gnt_cluster.py >> +++ b/lib/client/gnt_cluster.py >> @@ -76,26 +76,23 @@ def _CheckNoLvmStorageOptDeprecated(opts): >> return 1 >> >> >> -@UsesRPC >> -def InitCluster(opts, args): >> - """Initialize the cluster. >> - >> - @param opts: the command line options selected by the user >> - @type args: list >> - @param args: should contain only one element, the desired >> - cluster name >> - @rtype: int >> - @return: the desired exit code >> +def _InitEnabledDiskTemplates(opts): >> + """Initialize the list of enabled disk templates. >> >> """ >> - if _CheckNoLvmStorageOptDeprecated(opts): >> - return 1 >> - enabled_disk_templates = opts.enabled_disk_templates >> - if enabled_disk_templates: >> - enabled_disk_templates = enabled_disk_templates.split(",") >> + if opts.enabled_disk_templates: >> + return opts.enabled_disk_templates.split(",") >> else: >> - enabled_disk_templates = constants.DEFAULT_ENABLED_DISK_TEMPLATES >> + return constants.DEFAULT_ENABLED_DISK_TEMPLATES >> + >> >> +def _InitVgName(opts, enabled_disk_templates): >> + """Initialize the volume group name. >> + >> + @type enabled_disk_templates: list of strings >> + @param enabled_disk_templates: cluster-wide enabled disk templates >> + >> + """ >> vg_name = None >> if opts.vg_name is not None: >> vg_name = opts.vg_name >> @@ -105,19 +102,56 @@ def InitCluster(opts, args): >> " enable any disk template that uses lvm.") >> else: >> if utils.IsLvmEnabled(enabled_disk_templates): >> > > elif? > done. > > >> - ToStderr("LVM disk templates are enabled, but vg name not set.") >> - return 1 >> + raise errors.OpPrereqError( >> + "LVM disk templates are enabled, but vg name not set.") >> else: >> if utils.IsLvmEnabled(enabled_disk_templates): >> > > elif? > done. > > >> vg_name = constants.DEFAULT_VG >> + return vg_name >> >> + >> +def _InitDrbdHelper(opts): >> + """Initialize the DRBD usermode helper. >> + >> + """ >> if not opts.drbd_storage and opts.drbd_helper: >> - ToStderr("Options --no-drbd-storage and --drbd-usermode-helper >> conflict.") >> - return 1 >> + raise errors.OpPrereqError( >> + "Options --no-drbd-storage and --drbd-usermode-helper conflict.") >> >> - drbd_helper = opts.drbd_helper >> if opts.drbd_storage and not opts.drbd_helper: >> - drbd_helper = constants.DEFAULT_DRBD_HELPER >> + return constants.DEFAULT_DRBD_HELPER >> + >> + return opts.drbd_helper >> + >> + >> +@UsesRPC >> +def InitCluster(opts, args): >> + """Initialize the cluster. >> + >> + @param opts: the command line options selected by the user >> + @type args: list >> + @param args: should contain only one element, the desired >> + cluster name >> + @rtype: int >> + @return: the desired exit code >> + >> + """ >> + if _CheckNoLvmStorageOptDeprecated(opts): >> + return 1 >> + >> + enabled_disk_templates = _InitEnabledDiskTemplates(opts) >> + >> + try: >> + vg_name = _InitVgName(opts, enabled_disk_templates) >> + except errors.OpPrereqError, e: >> + ToStderr(str(e)) >> + return 1 >> + >> + try: >> + drbd_helper = _InitDrbdHelper(opts) >> + except errors.OpPrereqError, e: >> + ToStderr(str(e)) >> + return 1 >> > > Isn't it save to merge those two try blocks in one? > Right! Done. > > >> >> master_netdev = opts.master_netdev >> if master_netdev is None: >> @@ -971,6 +1005,49 @@ def RenewCrypto(opts, args): >> opts.force) >> >> >> +def _GetEnabledDiskTemplates(opts): >> + """Determine the list of enabled disk templates. >> + >> + """ >> + if opts.enabled_disk_templates: >> + return opts.enabled_disk_templates.split(",") >> + else: >> + return None >> + >> + >> +def _GetVgName(opts, enabled_disk_templates): >> + """Determine the volume group name. >> + >> + @type enabled_disk_templates: list of strings >> + @param enabled_disk_templates: cluster-wide enabled disk-templates >> + >> + """ >> + # consistency between vg name and enabled disk templates >> + vg_name = None >> + if opts.vg_name is not None: >> + vg_name = opts.vg_name >> + if enabled_disk_templates: >> + if vg_name and not utils.IsLvmEnabled(enabled_disk_templates): >> + ToStdout("You specified a volume group with --vg-name, but you did >> not" >> + " enable any of the following lvm-based disk templates: >> %s" % >> + utils.CommaJoin(utils.GetLvmDiskTemplates())) >> + return vg_name >> + >> > > _GetEnabledDiskTemplates and _InitEnabledDiskTemplates look very similar, > and _InitVgName and _GetVgName do look similar as well. Could we merge them > and provide something like a `default=None` parameter and call it once with > `default=constants.DEFAULT_ENABLED_DISK_TEMPLATES`? And something similar > for _GetVgName? > Hm, I am not sure about this. The methods are similar, but subtly different. I spent some hours to get the subtleties right already. The thing is that for init, you want to set a default, but for modify not, because there, an empty vgname etc. means to 'unset' the value (which you can't at init time). One could try to squeeze that into one function, but I doubt it would make debugging easier. I agree however that having two general functions for init and modify which can be used by both, vgname, drbdhelper (and maybe also enabled_disk_template) would be a good idea. I would introduce this in a separate patch for 2.9 and then adapt it for the usermode helper in 2.10. Would that be okay? > >> + >> +def _GetDrbdHelper(opts): >> + """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 = "" >> + return drbd_helper >> + >> + >> > > Same here actually. > See above. > > >> def SetClusterParams(opts, args): >> """Modify the cluster. >> >> @@ -1012,28 +1089,15 @@ def SetClusterParams(opts, args): >> if _CheckNoLvmStorageOptDeprecated(opts): >> return 1 >> >> - enabled_disk_templates = None >> - if opts.enabled_disk_templates: >> - enabled_disk_templates = opts.enabled_disk_templates.split(",") >> + enabled_disk_templates = _GetEnabledDiskTemplates(opts) >> + vg_name = _GetVgName(opts, enabled_disk_templates) >> >> - # consistency between vg name and enabled disk templates >> - vg_name = None >> - if opts.vg_name is not None: >> - vg_name = opts.vg_name >> - if enabled_disk_templates: >> - if vg_name and not utils.IsLvmEnabled(enabled_disk_templates): >> - ToStdout("You specified a volume group with --vg-name, but you did >> not" >> - " enable any of the following lvm-based disk templates: >> %s" % >> - utils.CommaJoin(utils.GetLvmDiskTemplates())) >> - >> - drbd_helper = opts.drbd_helper >> - if not opts.drbd_storage and opts.drbd_helper: >> - ToStderr("Options --no-drbd-storage and --drbd-usermode-helper >> conflict.") >> + try: >> + drbd_helper = _GetDrbdHelper(opts) >> + except errors.OpPrereqError, e: >> + ToStderr(str(e)) >> return 1 >> >> - if not opts.drbd_storage: >> - drbd_helper = "" >> - >> hvlist = opts.enabled_hypervisors >> if hvlist is not None: >> hvlist = hvlist.split(",") >> diff --git a/test/py/ganeti.client.gnt_cluster_unittest.py b/test/py/ >> ganeti.client.gnt_cluster_unittest.py >> index 1908f87..91e8935 100755 >> --- a/test/py/ganeti.client.gnt_cluster_unittest.py >> +++ b/test/py/ganeti.client.gnt_cluster_unittest.py >> @@ -24,11 +24,13 @@ >> import unittest >> import optparse >> >> +from ganeti import errors >> from ganeti.client import gnt_cluster >> from ganeti import utils >> from ganeti import compat >> from ganeti import constants >> >> +import mock >> import testutils >> >> >> @@ -258,5 +260,65 @@ class TestEpo(unittest.TestCase): >> self.assertEqual(result, constants.EXIT_FAILURE) >> >> >> +class InitDrbdHelper(unittest.TestCase): >> + >> + def testNoDrbdNoHelper(self): >> + opts = mock.Mock() >> > > Just FYI: Try looking into mock.MagickMock. It is really cool, as it can > mock any object and creates child mocks on the fly as required... Maybe it > can serve you at some point in time. > Thanks for the hint, will keep that in mind! > > >> + opts.drbd_storage = False >> + opts.drbd_helper = None >> + helper = gnt_cluster._InitDrbdHelper(opts) >> + self.assertEquals(None, helper) >> + >> + def testNoDrbdHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = None >> + opts.drbd_helper = "/bin/true" >> + self.assertRaises(errors.OpPrereqError, gnt_cluster._InitDrbdHelper, >> opts) >> + >> + def testDrbdNoHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = True >> + opts.drbd_helper = None >> + helper = gnt_cluster._InitDrbdHelper(opts) >> + self.assertEquals(constants.DEFAULT_DRBD_HELPER, helper) >> + >> + def testDrbdHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = True >> + opts.drbd_helper = "/bin/true" >> + helper = gnt_cluster._InitDrbdHelper(opts) >> + self.assertEquals(opts.drbd_helper, helper) >> + >> + >> +class GetDrbdHelper(unittest.TestCase): >> + >> + def testNoDrbdNoHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = False >> + opts.drbd_helper = None >> + helper = gnt_cluster._GetDrbdHelper(opts) >> + self.assertEquals("", helper) >> + >> + def testNoDrbdHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = None >> + opts.drbd_helper = "/bin/true" >> + self.assertRaises(errors.OpPrereqError, gnt_cluster._GetDrbdHelper, >> opts) >> + >> + def testDrbdNoHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = True >> + opts.drbd_helper = None >> + helper = gnt_cluster._GetDrbdHelper(opts) >> + self.assertEquals(None, helper) >> + >> + def testDrbdHelper(self): >> + opts = mock.Mock() >> + opts.drbd_storage = True >> + opts.drbd_helper = "/bin/true" >> + helper = gnt_cluster._GetDrbdHelper(opts) >> + self.assertEquals(opts.drbd_helper, helper) >> + >> + >> > > The test classes also look very similar... Too similar actually ;-). > They look similar here, but they will differ significantly at the end of this patch series. There are in particular differences in how the empty string is handled and whether or not the list of enabled_disk_templates is None (which is allowed for GetDrbdHelper, but not for InitDrbdHelper). I could try to share more code here, but that would only mean to detangle it again in the later patches of this series. Interdiff about the other changes of this patch: diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py index 90d3220..f3fb7a6 100644 --- a/lib/client/gnt_cluster.py +++ b/lib/client/gnt_cluster.py @@ -100,13 +100,11 @@ def _InitVgName(opts, enabled_disk_templates): if not utils.IsLvmEnabled(enabled_disk_templates): ToStdout("You specified a volume group with --vg-name, but you did not" " enable any disk template that uses lvm.") - else: - if utils.IsLvmEnabled(enabled_disk_templates): - raise errors.OpPrereqError( - "LVM disk templates are enabled, but vg name not set.") - else: - if utils.IsLvmEnabled(enabled_disk_templates): - vg_name = constants.DEFAULT_VG + elif utils.IsLvmEnabled(enabled_disk_templates): + raise errors.OpPrereqError( + "LVM disk templates are enabled, but vg name not set.") + elif utils.IsLvmEnabled(enabled_disk_templates): + vg_name = constants.DEFAULT_VG return vg_name @@ -143,11 +141,6 @@ def InitCluster(opts, args): try: vg_name = _InitVgName(opts, enabled_disk_templates) - except errors.OpPrereqError, e: - ToStderr(str(e)) - return 1 - - try: drbd_helper = _InitDrbdHelper(opts) except errors.OpPrereqError, e: ToStderr(str(e)) Cheers, Helga
