Check temp dir is usable for ipmitool driver

This patch adds directory check functions to common/utils.py.
It adds a call to the new check functions in ipmitool init.
So we can test the temp directory, upon startup to ensure that
the directory is usable.

These pre-flight checks do not change the need for valid
and accurate error handling, They are to quickly inform
an operator that there is a configuration error that will
prevent the driver from performing as expected.

Also adds a missing unit test for the ipmitool drivers power
validate function "test_power_validate".

Change-Id: Iefc75203ccde1d25fd91c9cb815d871b068255a2
Partial-Bug: #1428722

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.")

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)

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()

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())

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):