Avoid name errors in oneview periodics

Solves a bug that can brake the periodic task execution in
oneview drivers. The problem occurs when the method to check
if a node is in use by oneview raise an exception.

When that happens, the periodic task will break its execution,
leaving the node with a wrong provision state and wrong
maintenance mode too. The expected behavior when this occurs
is skip processing the current node, iterate over the next
node and process it.

This patch changes the periodic task execution flow, ignoring
the exception and processing the next node. In the next
periodic task execution, the not processed node can be
processed and updated accordingly.

Co-Authored-By: Xavier <marcusrafael@lsd.ufcg.edu.br>

Change-Id: I4b4b350d82d9e6863e8a4f334d2f9e7f7c192c6e
Closes-Bug: 1629051

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 '

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')
+    def test_node_manageable_maintenance_when_in_use_by_oneview(
-    @mock.patch.object(deploy_utils, 'is_node_in_use_by_oneview')
+        self, mock_is_node_in_use_by_oneview, mock_node_get
-    def test__periodic_check_nodes_taken_by_oneview(
-        self, mock_is_node_in_use_by_oneview, mock_get_node,
-            mock_get_ov_client
     ):
-
+        mock_node_get.get.return_value = self.node
-        manager = mock.MagicMock(
+        _setup_node_in_available_state(self.node)
-            spec=['iter_nodes', 'update_node', 'do_provisioning_action']
+        self.manager.iter_nodes.return_value = nodes_taken_by_oneview
-        )
-
-        manager.iter_nodes.return_value = [
-            (self.node.uuid, 'fake_oneview')
-        ]
-
-        mock_get_node.return_value = self.node
         mock_is_node_in_use_by_oneview.return_value = True
-
+        self.deploy._periodic_check_nodes_taken_by_oneview(
-        class OneViewDriverDeploy(deploy.OneViewPeriodicTasks):
+            self.manager, self.context
-            oneview_driver = 'fake_oneview'
-
-        oneview_driver_deploy = OneViewDriverDeploy()
-        oneview_driver_deploy._periodic_check_nodes_taken_by_oneview(
-            manager, self.context
         )
-        self.assertTrue(manager.update_node.called)
+        mock_is_node_in_use_by_oneview.assert_called_once_with(self.node)
-        self.assertTrue(manager.do_provisioning_action.called)
+        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_node_stay_available_when_not_in_use_by_oneview(
-    def test__periodic_check_nodes_freed_by_oneview(
+        self, mock_is_node_in_use_by_oneview, mock_node_get
-        self, mock_is_node_in_use_by_oneview, mock_get_ov_client
     ):
-
+        mock_node_get.get.return_value = self.node
-        manager = mock.MagicMock(
+        _setup_node_in_available_state(self.node)
-            spec=['iter_nodes', 'update_node', 'do_provisioning_action']
+        mock_node_get.return_value = self.node
-        )
-
-        manager.iter_nodes.return_value = [
-            (self.node.uuid, 'fake_oneview',
-             common.NODE_IN_USE_BY_ONEVIEW)
-        ]
-
         mock_is_node_in_use_by_oneview.return_value = False
-
+        self.manager.iter_nodes.return_value = nodes_taken_by_oneview
-        class OneViewDriverDeploy(deploy.OneViewPeriodicTasks):
+        self.deploy._periodic_check_nodes_taken_by_oneview(
-            oneview_driver = 'fake_oneview'
+            self.manager, self.context
-
-        oneview_driver_deploy = OneViewDriverDeploy()
-        oneview_driver_deploy._periodic_check_nodes_freed_by_oneview(
-            manager, self.context
         )
-        self.assertTrue(manager.update_node.called)
+        mock_is_node_in_use_by_oneview.assert_called_once_with(self.node)
-        self.assertTrue(manager.do_provisioning_action.called)
+        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_node_stay_available_when_raise_exception(
-    def test__periodic_check_nodes_taken_on_cleanfail(
+        self, mock_is_node_in_use_by_oneview, mock_node_get
-        self, mock_get_node, mock_get_ov_client
     ):
-
+        mock_node_get.get.return_value = self.node
-        driver_internal_info = {
+        _setup_node_in_available_state(self.node)
-            'oneview_error': common.SERVER_HARDWARE_ALLOCATION_ERROR
+        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
-        manager = mock.MagicMock(
+        self.deploy._periodic_check_nodes_taken_by_oneview(
-            spec=['iter_nodes', 'update_node', 'do_provisioning_action']
+            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 = [
+    def test_node_available_when_not_in_use_by_oneview(
-            (self.node.uuid, 'fake_oneview', driver_internal_info)
+        self, mock_is_node_in_use_by_oneview, mock_node_get
-        ]
+    ):
-
+        mock_node_get.get.return_value = self.node
-        self.node.driver_internal_info = driver_internal_info
+        _setup_node_in_manageable_state(self.node)
-        mock_get_node.return_value = self.node
+        self.manager.iter_nodes.return_value = nodes_freed_by_oneview
-
+        mock_is_node_in_use_by_oneview.return_value = False
-        class OneViewDriverDeploy(deploy.OneViewPeriodicTasks):
+        self.deploy._periodic_check_nodes_freed_by_oneview(
-            oneview_driver = 'fake_oneview'
+            self.manager, self.context
-
-        oneview_driver_deploy = OneViewDriverDeploy()
-        oneview_driver_deploy._periodic_check_nodes_taken_on_cleanfail(
-            manager, self.context
         )
-        self.assertTrue(manager.update_node.called)
+        mock_is_node_in_use_by_oneview.assert_called_once_with(self.node)
-        self.assertTrue(manager.do_provisioning_action.called)
+        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)

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.