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? > - 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? > 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? > > 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? > + > +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. > 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. > + 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 ;-). > if __name__ == "__main__": > testutils.GanetiTestProgram() > -- > 1.8.3 > > Cheers, Thomas -- 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
