diff --git a/ironic/drivers/modules/oneview/deploy.py b/ironic/drivers/modules/oneview/deploy.py index 72c23f2e19..52be56b78d 100644 --- a/ironic/drivers/modules/oneview/deploy.py +++ b/ironic/drivers/modules/oneview/deploy.py @@ -70,11 +70,17 @@ class OneViewPeriodicTasks(object): try: oneview_using = deploy_utils.is_node_in_use_by_oneview(node) except exception.OneViewError as e: + # NOTE(xavierr): Skip this node and process the + # remaining nodes. This node will be checked in + # the next periodic call. + LOG.error(_LE("Error while determining if node " "%(node_uuid)s is in use by OneView. " "Error: %(error)s"), {'node_uuid': node.uuid, 'error': e}) + continue + if oneview_using: purpose = (_LI('Updating node %(node_uuid)s in use ' 'by OneView from %(provision_state)s state ' @@ -126,11 +132,17 @@ class OneViewPeriodicTasks(object): node ) except exception.OneViewError as e: + # NOTE(xavierr): Skip this node and process the + # remaining nodes. This node will be checked in + # the next periodic call. + LOG.error(_LE("Error while determining if node " "%(node_uuid)s is in use by OneView. " "Error: %(error)s"), {'node_uuid': node.uuid, 'error': e}) + continue + if not oneview_using: purpose = (_LI('Bringing node %(node_uuid)s back from ' 'use by OneView from %(provision_state)s ' diff --git a/ironic/tests/unit/drivers/modules/oneview/test_deploy.py b/ironic/tests/unit/drivers/modules/oneview/test_deploy.py index fc95608fcd..c4d9f89cb9 100644 --- a/ironic/tests/unit/drivers/modules/oneview/test_deploy.py +++ b/ironic/tests/unit/drivers/modules/oneview/test_deploy.py @@ -19,10 +19,11 @@ import mock from oslo_utils import importutils from ironic.common import driver_factory +from ironic.common import exception +from ironic.common import states from ironic.drivers.modules.oneview import common from ironic.drivers.modules.oneview import deploy from ironic.drivers.modules.oneview import deploy_utils -from ironic import objects from ironic.tests.unit.conductor import mgr_utils from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils @@ -30,8 +31,57 @@ from ironic.tests.unit.objects import utils as obj_utils oneview_models = importutils.try_import('oneview_client.models') +METHODS = ['iter_nodes', 'update_node', 'do_provisioning_action'] -@mock.patch.object(common, 'get_oneview_client', spec_set=True, autospec=True) +oneview_error = common.SERVER_HARDWARE_ALLOCATION_ERROR +maintenance_reason = common.NODE_IN_USE_BY_ONEVIEW + +driver_internal_info = {'oneview_error': oneview_error} +nodes_taken_by_oneview = [(1, 'fake_oneview')] +nodes_freed_by_oneview = [(1, 'fake_oneview', maintenance_reason)] +nodes_taken_on_cleanfail = [(1, 'fake_oneview', driver_internal_info)] +nodes_taken_on_cleanfail_no_info = [(1, 'fake_oneview', {})] + + +def _setup_node_in_available_state(node): + node.provision_state = states.AVAILABLE + node.maintenance = False + node.maintenance_reason = None + node.save() + + +def _setup_node_in_manageable_state(node): + node.provision_state = states.MANAGEABLE + node.maintenance = True + node.maintenance_reason = common.NODE_IN_USE_BY_ONEVIEW + node.save() + + +def _setup_node_in_cleanfailed_state_with_oneview_error(node): + node.provision_state = states.CLEANFAIL + node.maintenance = False + node.maintenance_reason = None + driver_internal_info = node.driver_internal_info + oneview_error = common.SERVER_HARDWARE_ALLOCATION_ERROR + driver_internal_info['oneview_error'] = oneview_error + node.driver_internal_info = driver_internal_info + node.save() + + +def _setup_node_in_cleanfailed_state_without_oneview_error(node): + node.provision_state = states.CLEANFAIL + node.maintenance = False + node.maintenance_reason = None + node.save() + + +class OneViewDriverDeploy(deploy.OneViewPeriodicTasks): + oneview_driver = 'fake_oneview' + + +@mock.patch('ironic.objects.Node', spec_set=True, autospec=True) +@mock.patch.object(deploy_utils, 'is_node_in_use_by_oneview', + spec_set=True, autospec=True) class OneViewPeriodicTasks(db_base.DbTestCase): def setUp(self): @@ -41,104 +91,147 @@ class OneViewPeriodicTasks(db_base.DbTestCase): self.config(password='password', group='oneview') mgr_utils.mock_the_extension_manager(driver='fake_oneview') - self.driver = driver_factory.get_driver('fake_oneview') + self.driver = driver_factory.get_driver('fake_oneview') + self.deploy = OneViewDriverDeploy() + self.manager = mock.MagicMock(spec=METHODS) self.node = obj_utils.create_test_node( self.context, driver='fake_oneview', properties=db_utils.get_test_oneview_properties(), driver_info=db_utils.get_test_oneview_driver_info(), ) - self.info = common.get_oneview_info(self.node) - @mock.patch.object(objects.Node, 'get') - @mock.patch.object(deploy_utils, 'is_node_in_use_by_oneview') - def test__periodic_check_nodes_taken_by_oneview( - self, mock_is_node_in_use_by_oneview, mock_get_node, - mock_get_ov_client + def test_node_manageable_maintenance_when_in_use_by_oneview( + self, mock_is_node_in_use_by_oneview, mock_node_get ): - - manager = mock.MagicMock( - spec=['iter_nodes', 'update_node', 'do_provisioning_action'] - ) - - manager.iter_nodes.return_value = [ - (self.node.uuid, 'fake_oneview') - ] - - mock_get_node.return_value = self.node + mock_node_get.get.return_value = self.node + _setup_node_in_available_state(self.node) + self.manager.iter_nodes.return_value = nodes_taken_by_oneview mock_is_node_in_use_by_oneview.return_value = True - - class OneViewDriverDeploy(deploy.OneViewPeriodicTasks): - oneview_driver = 'fake_oneview' - - oneview_driver_deploy = OneViewDriverDeploy() - oneview_driver_deploy._periodic_check_nodes_taken_by_oneview( - manager, self.context + self.deploy._periodic_check_nodes_taken_by_oneview( + self.manager, self.context ) - self.assertTrue(manager.update_node.called) - self.assertTrue(manager.do_provisioning_action.called) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertTrue(self.manager.update_node.called) + self.assertTrue(self.manager.do_provisioning_action.called) self.assertTrue(self.node.maintenance) self.assertEqual(common.NODE_IN_USE_BY_ONEVIEW, self.node.maintenance_reason) - @mock.patch.object(deploy_utils, 'is_node_in_use_by_oneview') - def test__periodic_check_nodes_freed_by_oneview( - self, mock_is_node_in_use_by_oneview, mock_get_ov_client + def test_node_stay_available_when_not_in_use_by_oneview( + self, mock_is_node_in_use_by_oneview, mock_node_get ): - - manager = mock.MagicMock( - spec=['iter_nodes', 'update_node', 'do_provisioning_action'] - ) - - manager.iter_nodes.return_value = [ - (self.node.uuid, 'fake_oneview', - common.NODE_IN_USE_BY_ONEVIEW) - ] - + mock_node_get.get.return_value = self.node + _setup_node_in_available_state(self.node) + mock_node_get.return_value = self.node mock_is_node_in_use_by_oneview.return_value = False - - class OneViewDriverDeploy(deploy.OneViewPeriodicTasks): - oneview_driver = 'fake_oneview' - - oneview_driver_deploy = OneViewDriverDeploy() - oneview_driver_deploy._periodic_check_nodes_freed_by_oneview( - manager, self.context + self.manager.iter_nodes.return_value = nodes_taken_by_oneview + self.deploy._periodic_check_nodes_taken_by_oneview( + self.manager, self.context ) - self.assertTrue(manager.update_node.called) - self.assertTrue(manager.do_provisioning_action.called) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertFalse(self.manager.update_node.called) + self.assertFalse(self.manager.do_provisioning_action.called) self.assertFalse(self.node.maintenance) self.assertIsNone(self.node.maintenance_reason) - @mock.patch.object(objects.Node, 'get') - def test__periodic_check_nodes_taken_on_cleanfail( - self, mock_get_node, mock_get_ov_client + def test_node_stay_available_when_raise_exception( + self, mock_is_node_in_use_by_oneview, mock_node_get ): - - driver_internal_info = { - 'oneview_error': common.SERVER_HARDWARE_ALLOCATION_ERROR - } - - manager = mock.MagicMock( - spec=['iter_nodes', 'update_node', 'do_provisioning_action'] + mock_node_get.get.return_value = self.node + _setup_node_in_available_state(self.node) + side_effect = exception.OneViewError('boom') + mock_is_node_in_use_by_oneview.side_effect = side_effect + self.manager.iter_nodes.return_value = nodes_taken_by_oneview + self.deploy._periodic_check_nodes_taken_by_oneview( + self.manager, self.context ) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertFalse(self.manager.update_node.called) + self.assertFalse(self.manager.do_provisioning_action.called) + self.assertFalse(self.node.maintenance) + self.assertNotEqual(common.NODE_IN_USE_BY_ONEVIEW, + self.node.maintenance_reason) - manager.iter_nodes.return_value = [ - (self.node.uuid, 'fake_oneview', driver_internal_info) - ] - - self.node.driver_internal_info = driver_internal_info - mock_get_node.return_value = self.node - - class OneViewDriverDeploy(deploy.OneViewPeriodicTasks): - oneview_driver = 'fake_oneview' - - oneview_driver_deploy = OneViewDriverDeploy() - oneview_driver_deploy._periodic_check_nodes_taken_on_cleanfail( - manager, self.context + def test_node_available_when_not_in_use_by_oneview( + self, mock_is_node_in_use_by_oneview, mock_node_get + ): + mock_node_get.get.return_value = self.node + _setup_node_in_manageable_state(self.node) + self.manager.iter_nodes.return_value = nodes_freed_by_oneview + mock_is_node_in_use_by_oneview.return_value = False + self.deploy._periodic_check_nodes_freed_by_oneview( + self.manager, self.context ) - self.assertTrue(manager.update_node.called) - self.assertTrue(manager.do_provisioning_action.called) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertTrue(self.manager.update_node.called) + self.assertTrue(self.manager.do_provisioning_action.called) + self.assertFalse(self.node.maintenance) + self.assertIsNone(self.node.maintenance_reason) + + def test_node_stay_manageable_when_in_use_by_oneview( + self, mock_is_node_in_use_by_oneview, mock_node_get + ): + mock_node_get.get.return_value = self.node + _setup_node_in_manageable_state(self.node) + mock_is_node_in_use_by_oneview.return_value = True + self.manager.iter_nodes.return_value = nodes_freed_by_oneview + self.deploy._periodic_check_nodes_freed_by_oneview( + self.manager, self.context + ) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertFalse(self.manager.update_node.called) + self.assertFalse(self.manager.do_provisioning_action.called) self.assertTrue(self.node.maintenance) self.assertEqual(common.NODE_IN_USE_BY_ONEVIEW, self.node.maintenance_reason) - self.assertEqual({}, self.node.driver_internal_info) + + def test_node_stay_manageable_maintenance_when_raise_exception( + self, mock_is_node_in_use_by_oneview, mock_node_get + ): + mock_node_get.get.return_value = self.node + _setup_node_in_manageable_state(self.node) + side_effect = exception.OneViewError('boom') + mock_is_node_in_use_by_oneview.side_effect = side_effect + self.manager.iter_nodes.return_value = nodes_freed_by_oneview + self.deploy._periodic_check_nodes_freed_by_oneview( + self.manager, self.context + ) + mock_is_node_in_use_by_oneview.assert_called_once_with(self.node) + self.assertFalse(self.manager.update_node.called) + self.assertFalse(self.manager.do_provisioning_action.called) + self.assertTrue(self.node.maintenance) + self.assertEqual(common.NODE_IN_USE_BY_ONEVIEW, + self.node.maintenance_reason) + + def test_node_manageable_maintenance_when_oneview_error( + self, mock_is_node_in_use_by_oneview, mock_node_get + ): + mock_node_get.get.return_value = self.node + _setup_node_in_cleanfailed_state_with_oneview_error(self.node) + self.manager.iter_nodes.return_value = nodes_taken_on_cleanfail + self.deploy._periodic_check_nodes_taken_on_cleanfail( + self.manager, self.context + ) + self.assertTrue(self.manager.update_node.called) + self.assertTrue(self.manager.do_provisioning_action.called) + self.assertTrue(self.node.maintenance) + self.assertEqual(common.NODE_IN_USE_BY_ONEVIEW, + self.node.maintenance_reason) + self.assertFalse('oneview_error' in self.node.driver_internal_info) + + def test_node_stay_clean_failed_when_no_oneview_error( + self, mock_is_node_in_use_by_oneview, mock_node_get + ): + mock_node_get.get.return_value = self.node + _setup_node_in_cleanfailed_state_without_oneview_error(self.node) + self.manager.iter_nodes.return_value = nodes_taken_on_cleanfail_no_info + self.deploy._periodic_check_nodes_taken_on_cleanfail( + self.manager, self.context + ) + self.assertFalse(self.manager.update_node.called) + self.assertFalse(self.manager.do_provisioning_action.called) + self.assertFalse(self.node.maintenance) + self.assertNotEqual(common.NODE_IN_USE_BY_ONEVIEW, + self.node.maintenance_reason) + self.assertFalse('oneview_error' in self.node.driver_internal_info) diff --git a/releasenotes/notes/fix-oneview-periodics-0f535fe7a0ad83cd.yaml b/releasenotes/notes/fix-oneview-periodics-0f535fe7a0ad83cd.yaml new file mode 100644 index 0000000000..0936725b23 --- /dev/null +++ b/releasenotes/notes/fix-oneview-periodics-0f535fe7a0ad83cd.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Fixes a bug in the oneview driver where the periodic task to + check if a node is in use by oneview may end prematurely.