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/ > loosing this piece of configuration. > > This change was done done earlier in the patch series, > s/done done/done/ > 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. > 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. 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
