diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 5e1974e627..c2365cc2b5 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -573,3 +573,11 @@ class HardwareInspectionFailure(IronicException): class NodeCleaningFailure(IronicException): message = _("Failed to clean node %(node)s: %(reason)s") + + +class PathNotFound(IronicException): + message = _("Path %(dir)s does not exist.") + + +class DirectoryNotWritable(IronicException): + message = _("Directory %(dir)s is not writable.") diff --git a/ironic/common/utils.py b/ironic/common/utils.py index c1b568961d..970e6d24a9 100644 --- a/ironic/common/utils.py +++ b/ironic/common/utils.py @@ -536,3 +536,64 @@ def dd(src, dst, *args): def is_http_url(url): url = url.lower() return url.startswith('http://') or url.startswith('https://') + + +def check_dir(directory_to_check=None, required_space=1): + """Check a directory is usable. + + This function can be used by drivers to check that directories + they need to write to are usable. This should be called from the + drivers init function. This function checks that the directory + exists and then calls check_dir_writable and check_dir_free_space. + If directory_to_check is not provided the default is to use the + temp directory. + + :param directory_to_check: the directory to check. + :param required_space: amount of space to check for in MiB. + :raises: PathNotFound if directory can not be found + :raises: DirectoryNotWritable if user is unable to write to the + directory + :raises InsufficientDiskSpace: if free space is < required space + """ + # check if directory_to_check is passed in, if not set to tempdir + if directory_to_check is None: + directory_to_check = (tempfile.gettempdir() if CONF.tempdir + is None else CONF.tempdir) + + LOG.debug("checking directory: %s", directory_to_check) + + if not os.path.exists(directory_to_check): + raise exception.PathNotFound(dir=directory_to_check) + + _check_dir_writable(directory_to_check) + _check_dir_free_space(directory_to_check, required_space) + + +def _check_dir_writable(chk_dir): + """Check that the chk_dir is able to be written to. + + :param chk_dir: Directory to check + :raises: DirectoryNotWritable if user is unable to write to the + directory + """ + is_writable = os.access(chk_dir, os.W_OK) + if not is_writable: + raise exception.DirectoryNotWritable(dir=chk_dir) + + +def _check_dir_free_space(chk_dir, required_space=1): + """Check that directory has some free space. + + :param chk_dir: Directory to check + :param required_space: amount of space to check for in MiB. + :raises InsufficientDiskSpace: if free space is < required space + """ + # check that we have some free space + stat = os.statvfs(chk_dir) + # get dir free space in MiB. + free_space = float(stat.f_bsize * stat.f_bavail) / 1024 / 1024 + # check for at least required_space MiB free + if free_space < required_space: + raise exception.InsufficientDiskSpace(path=chk_dir, + required=required_space, + actual=free_space) diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index bceafe9da2..ad4104f504 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -103,7 +103,7 @@ LAST_CMD_TIME = {} TIMING_SUPPORT = None SINGLE_BRIDGE_SUPPORT = None DUAL_BRIDGE_SUPPORT = None - +TMP_DIR_CHECKED = None ipmitool_command_options = { 'timing': ['ipmitool', '-N', '0', '-R', '0', '-h'], @@ -630,6 +630,28 @@ def send_raw(task, raw_bytes): raise exception.IPMIFailure(cmd=cmd) +def _check_temp_dir(): + """Check for Valid temp directory.""" + global TMP_DIR_CHECKED + # because a temporary file is used to pass the password to ipmitool, + # we should check the directory + if TMP_DIR_CHECKED is None: + try: + utils.check_dir() + except (exception.PathNotFound, + exception.DirectoryNotWritable, + exception.InsufficientDiskSpace) as e: + TMP_DIR_CHECKED = False + err_msg = (_("Ipmitool drivers need to be able to create " + "temporary files to pass password to ipmitool. " + "Encountered error: %s") % e) + e.message = err_msg + LOG.error(err_msg) + raise + else: + TMP_DIR_CHECKED = True + + class IPMIPower(base.PowerInterface): def __init__(self): @@ -640,6 +662,7 @@ class IPMIPower(base.PowerInterface): driver=self.__class__.__name__, reason=_("Unable to locate usable ipmitool command in " "the system path when checking ipmitool version")) + _check_temp_dir() def get_properties(self): return COMMON_PROPERTIES @@ -731,6 +754,7 @@ class IPMIManagement(base.ManagementInterface): driver=self.__class__.__name__, reason=_("Unable to locate usable ipmitool command in " "the system path when checking ipmitool version")) + _check_temp_dir() def validate(self, task): """Check that 'driver_info' contains IPMI credentials. @@ -883,6 +907,7 @@ class VendorPassthru(base.VendorInterface): driver=self.__class__.__name__, reason=_("Unable to locate usable ipmitool command in " "the system path when checking ipmitool version")) + _check_temp_dir() @base.passthru(['POST']) @task_manager.require_exclusive_lock @@ -975,6 +1000,7 @@ class IPMIShellinaboxConsole(base.ConsoleInterface): driver=self.__class__.__name__, reason=_("Unable to locate usable ipmitool command in " "the system path when checking ipmitool version")) + _check_temp_dir() def get_properties(self): d = COMMON_PROPERTIES.copy() diff --git a/ironic/tests/drivers/test_ipmitool.py b/ironic/tests/drivers/test_ipmitool.py index e52d222cb6..709b445a37 100644 --- a/ironic/tests/drivers/test_ipmitool.py +++ b/ironic/tests/drivers/test_ipmitool.py @@ -57,6 +57,117 @@ BRIDGE_INFO_DICT = INFO_DICT.copy() BRIDGE_INFO_DICT.update(db_utils.get_test_ipmi_bridging_parameters()) +class IPMIToolCheckInitTestCase(base.TestCase): + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_power_init_calls(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + ipmi.IPMIPower() + mock_support.assert_called_with(mock.ANY) + mock_check_dir.assert_called_once_with() + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_power_init_calls_raises_1(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + mock_check_dir.side_effect = exception.PathNotFound(dir="foo_dir") + self.assertRaises(exception.PathNotFound, ipmi.IPMIPower) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_power_init_calls_raises_2(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + mock_check_dir.side_effect = exception.DirectoryNotWritable( + dir="foo_dir") + self.assertRaises(exception.DirectoryNotWritable, ipmi.IPMIPower) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_power_init_calls_raises_3(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + mock_check_dir.side_effect = exception.InsufficientDiskSpace( + path="foo_dir", required=1, actual=0) + self.assertRaises(exception.InsufficientDiskSpace, ipmi.IPMIPower) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_power_init_calls_already_checked(self, + mock_check_dir, + mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = True + ipmi.IPMIPower() + mock_support.assert_called_with(mock.ANY) + self.assertEqual(0, mock_check_dir.call_count) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_management_init_calls(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + + ipmi.IPMIManagement() + mock_support.assert_called_with(mock.ANY) + mock_check_dir.assert_called_once_with() + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_management_init_calls_already_checked(self, + mock_check_dir, + mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = False + + ipmi.IPMIManagement() + mock_support.assert_called_with(mock.ANY) + self.assertEqual(0, mock_check_dir.call_count) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_vendor_passthru_init_calls(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + ipmi.VendorPassthru() + mock_support.assert_called_with(mock.ANY) + mock_check_dir.assert_called_once_with() + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_vendor_passthru_init_calls_already_checked(self, + mock_check_dir, + mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = True + ipmi.VendorPassthru() + mock_support.assert_called_with(mock.ANY) + self.assertEqual(0, mock_check_dir.call_count) + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_console_init_calls(self, mock_check_dir, mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = None + ipmi.IPMIShellinaboxConsole() + mock_support.assert_called_with(mock.ANY) + mock_check_dir.assert_called_once_with() + + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) + @mock.patch.object(utils, 'check_dir', autospec=True) + def test_console_init_calls_already_checked(self, + mock_check_dir, + mock_support): + mock_support.return_value = True + ipmi.TMP_DIR_CHECKED = True + ipmi.IPMIShellinaboxConsole() + mock_support.assert_called_with(mock.ANY) + self.assertEqual(0, mock_check_dir.call_count) + + class IPMIToolCheckOptionSupportedTestCase(base.TestCase): @mock.patch.object(ipmi, '_is_option_supported') @@ -990,6 +1101,16 @@ class IPMIToolDriverTestCase(db_base.DbTestCase): driver_info=INFO_DICT) self.info = ipmi._parse_driver_info(self.node) + @mock.patch.object(ipmi, "_parse_driver_info", autospec=True) + def test_power_validate(self, mock_parse): + node = obj_utils.get_test_node(self.context, driver='fake_ipmitool', + driver_info=INFO_DICT) + mock_parse.return_value = {} + + with task_manager.acquire(self.context, node.uuid) as task: + task.driver.power.validate(task) + mock_parse.assert_called_once_with(mock.ANY) + def test_get_properties(self): expected = ipmi.COMMON_PROPERTIES self.assertEqual(expected, self.driver.power.get_properties()) diff --git a/ironic/tests/test_utils.py b/ironic/tests/test_utils.py index 3700084eba..41ff37fa4d 100644 --- a/ironic/tests/test_utils.py +++ b/ironic/tests/test_utils.py @@ -503,6 +503,114 @@ class TempFilesTestCase(base.TestCase): rmtree_mock.assert_called_once_with(tempdir_created) self.assertTrue(log_mock.error.called) + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(utils, '_check_dir_writable', autospec=True) + @mock.patch.object(utils, '_check_dir_free_space', autospec=True) + @mock.patch.object(tempfile, 'gettempdir', autospec=True) + def test_check_dir_with_conf(self, mock_gettempdir, mock_free_space, + mock_dir_writable, mock_exists): + self.config(tempdir='/fake/path') + mock_exists.return_value = True + + utils.check_dir() + self.assertFalse(mock_gettempdir.called) + mock_free_space.assert_called_once_with(CONF.tempdir, 1) + mock_exists.assert_called_once_with(CONF.tempdir) + + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(utils, '_check_dir_writable', autospec=True) + @mock.patch.object(utils, '_check_dir_free_space', autospec=True) + @mock.patch.object(tempfile, 'gettempdir', autospec=True) + def test_check_dir_with_pass_in(self, mock_gettempdir, mock_free_space, + mock_dir_writable, mock_exists): + mock_exists.return_value = True + # test passing in a directory and size + utils.check_dir(directory_to_check='/fake/path', required_space=5) + self.assertFalse(mock_gettempdir.called) + mock_free_space.assert_called_once_with('/fake/path', 5) + mock_exists.assert_called_once_with('/fake/path') + + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(utils, '_check_dir_writable', autospec=True) + @mock.patch.object(utils, '_check_dir_free_space', autospec=True) + @mock.patch.object(tempfile, 'gettempdir', autospec=True) + def test_check_dir_no_dir(self, mock_gettempdir, mock_free_space, + mock_dir_writable, mock_exists): + mock_exists.return_value = False + mock_gettempdir.return_value = "/fake/path" + + self.assertRaises(exception.PathNotFound, + utils.check_dir) + + mock_exists.assert_called_once_with(mock_gettempdir.return_value) + mock_gettempdir.assert_called_once_with() + self.assertFalse(mock_free_space.called) + self.assertFalse(mock_dir_writable.called) + + @mock.patch.object(os.path, 'exists', autospec=True) + @mock.patch.object(utils, '_check_dir_writable', autospec=True) + @mock.patch.object(utils, '_check_dir_free_space', autospec=True) + @mock.patch.object(tempfile, 'gettempdir', autospec=True) + def test_check_dir_ok(self, mock_gettempdir, mock_dir_writable, + mock_free_space, mock_exists): + mock_gettempdir.return_value = "/fake/path" + mock_exists.return_value = True + + utils.check_dir() + mock_gettempdir.assert_called_once_with() + mock_free_space.assert_called_once_with(mock_gettempdir.return_value) + mock_exists.assert_called_once_with(mock_gettempdir.return_value) + + @mock.patch.object(os, 'access', autospec=True) + def test__check_dir_writable_ok(self, mock_access): + mock_access.return_value = True + self.assertEqual(None, utils._check_dir_writable("/fake/path")) + mock_access.assert_called_once_with("/fake/path", os.W_OK) + + @mock.patch.object(os, 'access', autospec=True) + def test__check_dir_writable_not_writable(self, mock_access): + mock_access.return_value = False + + self.assertRaises(exception.DirectoryNotWritable, + utils._check_dir_writable, "/fake/path") + mock_access.assert_called_once_with("/fake/path", os.W_OK) + + @mock.patch.object(os, 'statvfs', autospec=True) + def test__check_dir_free_space_ok(self, mock_stat): + statvfs_mock_return = mock.MagicMock() + statvfs_mock_return.f_bsize = 5 + statvfs_mock_return.f_frsize = 0 + statvfs_mock_return.f_blocks = 0 + statvfs_mock_return.f_bfree = 0 + statvfs_mock_return.f_bavail = 1024 * 1024 + statvfs_mock_return.f_files = 0 + statvfs_mock_return.f_ffree = 0 + statvfs_mock_return.f_favail = 0 + statvfs_mock_return.f_flag = 0 + statvfs_mock_return.f_namemax = 0 + mock_stat.return_value = statvfs_mock_return + utils._check_dir_free_space("/fake/path") + mock_stat.assert_called_once_with("/fake/path") + + @mock.patch.object(os, 'statvfs', autospec=True) + def test_check_dir_free_space_raises(self, mock_stat): + statvfs_mock_return = mock.MagicMock() + statvfs_mock_return.f_bsize = 1 + statvfs_mock_return.f_frsize = 0 + statvfs_mock_return.f_blocks = 0 + statvfs_mock_return.f_bfree = 0 + statvfs_mock_return.f_bavail = 1024 + statvfs_mock_return.f_files = 0 + statvfs_mock_return.f_ffree = 0 + statvfs_mock_return.f_favail = 0 + statvfs_mock_return.f_flag = 0 + statvfs_mock_return.f_namemax = 0 + mock_stat.return_value = statvfs_mock_return + + self.assertRaises(exception.InsufficientDiskSpace, + utils._check_dir_free_space, "/fake/path") + mock_stat.assert_called_once_with("/fake/path") + class IsHttpUrlTestCase(base.TestCase):