From: Tom Barron Date: Sat, 18 Apr 2015 13:42:10 +0000 (-0400) Subject: Fix range check for NFS used ratio X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=9a1e146849f6aa88784e2d8a224ef8e86e06a749;p=openstack-build%2Fcinder-build.git 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 594f24f50..a44308097 100644 --- 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 706c212b6..cf1617ddd 100644 --- 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