Fix range check for NFS used ratio

The nfs_used_ratio configuration parameter is supposed to be
greater than zero and less than or equal to one.  However,
current configuration checks actually allow values greater
than one because of a logic error.  Unit tests fail to detect
this issue because they find the exception that they expect
to find, but it is actually being thrown for another reason.

This commit fixes the logic error in the nfs driver code.

It also reworks all the unit tests for the do_setup method since
they suffer from the same issue that led to failure to detect
this error in the first place - they fail to do proper mocks of
submethods, and when these run the expected exception gets triggered
for the wrong reason.  We have moved these tests into their own test
class, where mock is used rather than mox.  Over time it is likely
that new tests will be implemented, or old tests reworked, with mock
instead of mox.  These can go in the new test class, which can be
renamed if that is appropriate.

Change-Id: I19ab396057fb6f60ad8e7d10944384db7e95ece6
Fixes-bug: 1445786

diff --git a/cinder/tests/unit/test_nfs.py b/cinder/tests/unit/test_nfs.py
index 594f24f50342e785c4d4c08c448c082503fdab5b..a44308097bf2bd3242b7577b7f4b9ed852089639 100644 (file)
--- a/cinder/tests/unit/test_nfs.py
+++ b/cinder/tests/unit/test_nfs.py
@@ -16,13 +16,13 @@
 
 import errno
 import os
+import testtools
 
 import mock
 import mox as mox_lib
 from mox import stubout
 from oslo_utils import units
 
-from cinder import context
 from cinder import exception
 from cinder.image import image_utils
 from cinder import test
@@ -621,56 +621,6 @@ class NfsDriverTestCase(test.TestCase):
 
         self.assertEqual(1, remotefs.LOG.error.call_count)
 
-    def test_setup_should_throw_error_if_shares_config_not_configured(self):
-        """do_setup should throw error if shares config is not configured."""
-        drv = self._driver
-        self.configuration.nfs_shares_config = self.TEST_SHARES_CONFIG_FILE
-
-        self.assertRaises(exception.NfsException,
-                          drv.do_setup, mox_lib.IsA(context.RequestContext))
-
-    def test_setup_should_throw_error_if_oversub_ratio_less_than_zero(self):
-        """do_setup should throw error if nfs_oversub_ratio is less than 0."""
-        drv = self._driver
-        self.configuration.nfs_oversub_ratio = -1
-        self.assertRaises(exception.NfsException,
-                          drv.do_setup,
-                          mox_lib.IsA(context.RequestContext))
-
-    def test_setup_should_throw_error_if_used_ratio_less_than_zero(self):
-        """do_setup should throw error if nfs_used_ratio is less than 0."""
-        drv = self._driver
-        self.configuration.nfs_used_ratio = -1
-        self.assertRaises(exception.NfsException,
-                          drv.do_setup,
-                          mox_lib.IsA(context.RequestContext))
-
-    def test_setup_should_throw_error_if_used_ratio_greater_than_one(self):
-        """do_setup should throw error if nfs_used_ratio is greater than 1."""
-        drv = self._driver
-        self.configuration.nfs_used_ratio = 2
-        self.assertRaises(exception.NfsException,
-                          drv.do_setup,
-                          mox_lib.IsA(context.RequestContext))
-
-    def test_setup_should_throw_exception_if_nfs_client_is_not_installed(self):
-        """do_setup should throw error if nfs client is not installed."""
-        mox = self._mox
-        drv = self._driver
-        self.configuration.nfs_shares_config = self.TEST_SHARES_CONFIG_FILE
-        mox.StubOutWithMock(os.path, 'exists')
-        os.path.exists(self.TEST_SHARES_CONFIG_FILE).AndReturn(True)
-        mox.StubOutWithMock(drv, '_execute')
-        drv._execute('mount.nfs', check_exit_code=False, run_as_root=False).\
-            AndRaise(OSError(errno.ENOENT, 'No such file or directory'))
-
-        mox.ReplayAll()
-
-        self.assertRaises(exception.NfsException,
-                          drv.do_setup, mox_lib.IsA(context.RequestContext))
-
-        mox.VerifyAll()
-
     def test_find_share_should_throw_error_if_there_is_no_mounted_shares(self):
         """_find_share should throw error if there is no mounted shares."""
         drv = self._driver
@@ -1137,3 +1087,178 @@ class NfsDriverTestCase(test.TestCase):
 
         self.assertEqual(min_num_attempts,
                          drv._remotefsclient.mount.call_count)
+
+
+class NfsDriverDoSetupTestCase(test.TestCase):
+
+    def setUp(self):
+        super(NfsDriverDoSetupTestCase, self).setUp()
+        self.context = mock.Mock()
+        self.create_configuration()
+
+    def create_configuration(self):
+        config = conf.Configuration(None)
+        config.append_config_values(nfs.nfs_opts)
+        self.configuration = config
+
+    def test_setup_should_throw_error_if_shares_config_not_configured(self):
+        """do_setup should throw error if shares config is not configured."""
+
+        self.override_config('nfs_shares_config', None)
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+
+        with testtools.ExpectedException(exception.NfsException,
+                                         ".*no NFS config file configured.*"):
+            drv.do_setup(self.context)
+
+        self.assertEqual(0, mock_os_path_exists.call_count)
+
+    def test_setup_should_throw_error_if_shares_file_does_not_exist(self):
+        """do_setup should throw error if shares file does not exist."""
+
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = False
+
+        with testtools.ExpectedException(exception.NfsException,
+                                         "NFS config file.*doesn't exist"):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+
+    def test_setup_should_throw_error_if_oversub_ratio_less_than_zero(self):
+        """do_setup should throw error if nfs_oversub_ratio is less than 0."""
+
+        self.override_config('nfs_oversub_ratio', -1)
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+
+        with testtools.ExpectedException(
+                exception.InvalidConfigurationValue,
+                ".*'nfs_oversub_ratio' invalid.*"):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+
+    def test_setup_oversub_ratio_default_value(self):
+        """do_setup should work with default value for nfs_oversub_ratio."""
+
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+        mock_execute = self.mock_object(drv, '_execute')
+        mock_set_security = self.mock_object(drv, 'set_nas_security_options')
+
+        drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+        self.assertEqual(1, mock_execute.call_count)
+        self.assertEqual(1, mock_set_security.call_count)
+        self.assertEqual(self.configuration.nfs_oversub_ratio, 1.0)
+
+    def test_setup_should_throw_error_if_used_ratio_less_than_zero(self):
+        """do_setup should throw error if nfs_used_ratio is less than 0."""
+
+        self.override_config('nfs_used_ratio', -1)
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+
+        with testtools.ExpectedException(
+                exception.InvalidConfigurationValue,
+                ".*'nfs_used_ratio' invalid.*"):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+
+    def test_setup_should_throw_error_if_used_ratio_greater_than_one(self):
+        """do_setup should throw error if nfs_used_ratio is greater than 1."""
+
+        self.override_config('nfs_used_ratio', 2)
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+
+        with testtools.ExpectedException(
+                exception.InvalidConfigurationValue,
+                ".*'nfs_used_ratio' invalid.*"):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+
+    def test_setup_used_ratio_default_value(self):
+        """do_setup should work with default value for nfs_used_ratio."""
+
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+        mock_execute = self.mock_object(drv, '_execute')
+        mock_set_security = self.mock_object(drv, 'set_nas_security_options')
+
+        drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+        self.assertEqual(1, mock_execute.call_count)
+        self.assertEqual(1, mock_set_security.call_count)
+        self.assertEqual(self.configuration.nfs_used_ratio, 0.95)
+
+    def test_setup_should_throw_exception_if_nfs_client_is_not_installed(self):
+        """do_setup should throw error if nfs client is not installed."""
+
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+        mock_execute = self.mock_object(drv, '_execute')
+        mock_execute.side_effect = OSError(
+            errno.ENOENT, 'No such file or directory.')
+
+        with testtools.ExpectedException(
+                exception.NfsException, 'mount.nfs is not installed'):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+        mock_execute.assert_has_calls(
+            [mock.call('mount.nfs',
+                       check_exit_code=False,
+                       run_as_root=False)])
+
+    def test_setup_should_throw_exception_if_mount_nfs_command_fails(self):
+        """do_setup should throw error if mount.nfs fails with OSError
+
+           This test covers the OSError path when mount.nfs is installed.
+        """
+
+        drv = nfs.NfsDriver(configuration=self.configuration)
+
+        mock_os_path_exists = self.mock_object(os.path, 'exists')
+        mock_os_path_exists.return_value = True
+        mock_execute = self.mock_object(drv, '_execute')
+        mock_execute.side_effect = OSError(
+            errno.EPERM, 'Operation... BROKEN')
+
+        with testtools.ExpectedException(OSError, '.*Operation... BROKEN'):
+            drv.do_setup(self.context)
+
+        mock_os_path_exists.assert_has_calls(
+            [mock.call(self.configuration.nfs_shares_config)])
+        mock_execute.assert_has_calls(
+            [mock.call('mount.nfs',
+                       check_exit_code=False,
+                       run_as_root=False)])

diff --git a/cinder/volume/drivers/nfs.py b/cinder/volume/drivers/nfs.py
index 706c212b61f9b2c2cf7ff0f37d5acd8461d813a1..cf1617ddd9d9ce9f063c5d5539c9092ad331e106 100644 (file)
--- a/cinder/volume/drivers/nfs.py
+++ b/cinder/volume/drivers/nfs.py
@@ -133,14 +133,13 @@ class NfsDriver(remotefs.RemoteFSDriver):
                     "%s") % self.configuration.nfs_oversub_ratio
 
             LOG.error(msg)
-            raise exception.NfsException(msg)
-
-        if ((not self.configuration.nfs_used_ratio > 0) and
+            raise exception.InvalidConfigurationValue(msg)
+        if not ((self.configuration.nfs_used_ratio > 0) and
                 (self.configuration.nfs_used_ratio <= 1)):
             msg = _("NFS config 'nfs_used_ratio' invalid.  Must be > 0 "
                     "and <= 1.0: %s") % self.configuration.nfs_used_ratio
             LOG.error(msg)
-            raise exception.NfsException(msg)
+            raise exception.InvalidConfigurationValue(msg)
 
         self.shares = {}  # address : options