On Tue, Aug 27, 2013 at 12:49 PM, Helga Velroyen <[email protected]> wrote:
> 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 > 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
