On Tue, Aug 27, 2013 at 1:26 PM, Helga Velroyen <[email protected]> wrote:
> > > > On Mon, Aug 26, 2013 at 4:36 PM, Thomas Thrainer <[email protected]>wrote: > >> >> >> >> On Thu, Aug 22, 2013 at 10:16 AM, Helga Velroyen <[email protected]>wrote: >> >>> The logic around the DRBD usermode helper so far was that >>> setting it was only possible when DRDB was not explicitely >>> disabled. This patches changes it in a way that it is >>> consistent to how Ganeti handles the volume group name. >>> >>> Now, the user can specify a DRBD usermode helper independent >>> of whether or not DRBD is enabled or not. She will however >>> get a warning when she sets a helper without having DRBD >>> enabled. The reasoning behind this is that one might want >>> to configure a helper while not yet having set up DRBD >>> completely or whil having DRBD disabled temporarily without >>> >> s/whil/while/ >> > > Sure. > > >> loosing this piece of configuration. >>> >>> This change was done done earlier in the patch series, >>> >> s/done done/done/ >> > > Sure. > > >> because I wanted to do the refactoring in two steps, first >>> just transforming the original logic from --no-drbd-storage >>> to --enabled-disk-templates and if that goes well, adjust >>> to the more user-friendly behavior. >>> >>> Signed-off-by: Helga Velroyen <[email protected]> >>> --- >>> lib/client/gnt_cluster.py | 16 +++++----------- >>> qa/qa_cluster.py | 2 +- >>> test/py/ganeti.client.gnt_cluster_unittest.py | 8 ++++---- >>> 3 files changed, 10 insertions(+), 16 deletions(-) >>> >>> diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py >>> index 887e7b6..9bca0ac 100644 >>> --- a/lib/client/gnt_cluster.py >>> +++ b/lib/client/gnt_cluster.py >>> @@ -116,12 +116,9 @@ def _InitDrbdHelper(opts, enabled_disk_templates): >>> """ >>> drbd_enabled = constants.DT_DRBD8 in enabled_disk_templates >>> >>> - # This raises an exception due to historical reasons, one might >>> consider >>> - # letting the user set a helper without having DRBD enabled. >>> - if not drbd_enabled and opts.drbd_helper: >>> - raise errors.OpPrereqError( >>> - "Enabling the DRBD disk template and setting a drbd usermode >>> helper" >>> - " with --drbd-usermode-helper conflict.") >>> + if not drbd_enabled and opts.drbd_helper is not None: >>> + ToStdout("Note: You specified a DRBD usermode helper, while DRBD >>> storage" >>> + " is not enabled.") >>> >>> if drbd_enabled: >>> if opts.drbd_helper is None: >>> @@ -1051,12 +1048,9 @@ def _GetDrbdHelper(opts, enabled_disk_templates): >>> drbd_helper = opts.drbd_helper >>> if enabled_disk_templates: >>> drbd_enabled = constants.DT_DRBD8 in enabled_disk_templates >>> - # This raises an exception for historic reasons. It might be a good >>> idea >>> - # to allow users to set a DRBD helper when DRBD storage is not >>> enabled. >>> if not drbd_enabled and opts.drbd_helper: >>> - raise errors.OpPrereqError( >>> - "Setting a DRBD usermode helper when DRBD is not enabled is" >>> - " not allowed.") >>> + ToStdout("Note: that you specified a DRBD usermode helper while >>> DRBD is" >> >> + " not enabled.") >>> >> >> Why not copy the message from above? And "Note: that..." sounds strange. >> > > Sure, interdiff: > > diff --git a/lib/client/gnt_cluster.py b/lib/client/gnt_cluster.py > index d0f8bfd..44f921f 100644 > --- a/lib/client/gnt_cluster.py > +++ b/lib/client/gnt_cluster.py > @@ -1026,8 +1026,8 @@ def _GetDrbdHelper(opts, enabled_disk_templates): > if enabled_disk_templates: > drbd_enabled = constants.DT_DRBD8 in enabled_disk_templates > if not drbd_enabled and opts.drbd_helper: > - ToStdout("Note: that you specified a DRBD usermode helper while > DRBD is" > - " not enabled.") > + ToStdout("You specified a DRBD usermode helper with " > + " --drbd-usermode-helper while DRBD is not enabled.") > return drbd_helper > > > > >> >> >>> return drbd_helper >>> >>> >>> >>> diff --git a/qa/qa_cluster.py b/qa/qa_cluster.py >>> index 500155d..192141f 100644 >>> --- a/qa/qa_cluster.py >>> +++ b/qa/qa_cluster.py >>> @@ -621,7 +621,7 @@ def >>> _TestClusterModifyDiskTemplatesDrbdHelper(enabled_disk_templates): >>> "--enabled-disk-templates=%s" % constants.DT_PLAIN, >>> "--ipolicy-disk-templates=%s" % constants.DT_PLAIN], False), >>> (["gnt-cluster", "modify", >>> - "--drbd-usermode-helper="], True), >>> + "--drbd-usermode-helper="], False), >>> >> (["gnt-cluster", "modify", >>> "--drbd-usermode-helper=%s" % drbd_usermode_helper], False), >>> (["gnt-cluster", "modify", >>> diff --git a/test/py/ganeti.client.gnt_cluster_unittest.py b/test/py/ >>> ganeti.client.gnt_cluster_unittest.py >>> index 6aa9c62..4ec26b5 100755 >>> --- a/test/py/ganeti.client.gnt_cluster_unittest.py >>> +++ b/test/py/ganeti.client.gnt_cluster_unittest.py >>> @@ -283,8 +283,8 @@ class InitDrbdHelper(unittest.TestCase): >>> opts = mock.Mock() >>> self.disableDrbd() >>> opts.drbd_helper = "/bin/true" >>> - self.assertRaises(errors.OpPrereqError, >>> gnt_cluster._InitDrbdHelper, opts, >>> - self.enabled_disk_templates) >>> + helper = gnt_cluster._InitDrbdHelper(opts, >>> self.enabled_disk_templates) >>> + self.assertEquals(opts.drbd_helper, helper) >>> >>> def testDrbdHelperNone(self): >>> opts = mock.Mock() >>> @@ -343,8 +343,8 @@ class GetDrbdHelper(unittest.TestCase): >>> opts = mock.Mock() >>> self.disableDrbd() >>> opts.drbd_helper = "/bin/true" >>> - self.assertRaises(errors.OpPrereqError, gnt_cluster._GetDrbdHelper, >>> opts, >>> - self.enabled_disk_templates) >>> + helper = gnt_cluster._GetDrbdHelper(opts, None) >>> + self.assertEquals(opts.drbd_helper, helper) >>> >>> def testDrbdNoHelper(self): >>> opts = mock.Mock() >>> -- >>> 1.8.3 >>> >>> >> Given that this change is checked in two unit tests, I think we could >> really remove some of the QA tests. >> >> > As I said before, I suggest to make those tests configurable separately, > since they cover more than the unit tests. > I wouldn't put much more work in there. Better spend that in the long-term solution and try to write unit-tests for bootstrap... > > >> >> 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 >> > > -- 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
