Validation before perform node deallocation

Introduces a new validation to ensure the node deallocation
process will only be performed to nodes that have Server
Profile applied in OneView. When the allocation process
fails, the machine will have no Server Profile applied in
OneView and in ironic it will have no reference to any
Server Profile. Under these conditions if the deallocation
process be performed the driver will try to delete a
nonexistent Server Profile in OneView, leaving the node
in error state in ironic.

Change-Id: I83cef7fafac4e22ebf5eb4fe83f6da26ec132c99
Closes-Bug: #1657892
(cherry picked from commit 0b23a041f1111ad112d28619fa020173805edbbe)

5 files changed, 185 insertions, 47 deletions

diff --git a/ironic/drivers/modules/oneview/deploy_utils.py b/ironic/drivers/modules/oneview/deploy_utils.py
index 25aa15d0a..4a032aefe 100644
--- a/ironic/drivers/modules/oneview/deploy_utils.py
+++ b/ironic/drivers/modules/oneview/deploy_utils.py
@@ -20,7 +20,7 @@ from oslo_log import log as logging
 from oslo_utils import importutils
 
 from ironic.common import exception
-from ironic.common.i18n import _, _LE, _LI
+from ironic.common.i18n import _, _LE, _LI, _LW
 from ironic.common import states
 from ironic.drivers.modules.oneview import common
 
@@ -354,30 +354,30 @@ def deallocate_server_hardware_from_ironic(oneview_client, node):
             Hardware to ironic
 
     """
-    oneview_info = common.get_oneview_info(node)
-
-    oneview_client.power_off(oneview_info)
 
-    applied_sp_uuid = oneview_utils.get_uuid_from_uri(
-        oneview_info.get('applied_server_profile_uri')
-    )
+    if is_node_in_use_by_ironic(oneview_client, node):
 
-    try:
-        oneview_client.delete_server_profile(applied_sp_uuid)
-        _del_applied_server_profile_uri_field(node)
-
-        LOG.info(
-            _LI("Server Profile %(server_profile_uuid)s was successfully"
-                " deleted from node %(node_uuid)s."
-                ),
-            {"node_uuid": node.uuid, "server_profile_uuid": applied_sp_uuid}
+        oneview_info = common.get_oneview_info(node)
+        server_profile_uuid = oneview_utils.get_uuid_from_uri(
+            oneview_info.get('applied_server_profile_uri')
         )
-    except oneview_exception.OneViewException as e:
 
-        msg = (_("Error while deleting applied Server Profile from node "
-                 "%(node_uuid)s. Error: %(error)s") %
-               {'node_uuid': node.uuid, 'error': e})
+        try:
+            oneview_client.power_off(oneview_info)
+            oneview_client.delete_server_profile(server_profile_uuid)
+            _del_applied_server_profile_uri_field(node)
 
-        raise exception.OneViewError(
-            node=node.uuid, reason=msg
-        )
+            LOG.info(_LI("Server Profile %(server_profile_uuid)s was deleted "
+                         "from node %(node_uuid)s in OneView."),
+                     {'server_profile_uuid': server_profile_uuid,
+                      'node_uuid': node.uuid})
+        except (ValueError, oneview_exception.OneViewException) as e:
+            msg = (_("Error while deleting applied Server Profile from node "
+                     "%(node_uuid)s. Error: %(error)s") %
+                   {'node_uuid': node.uuid, 'error': e})
+            raise exception.OneViewError(error=msg)
+
+    else:
+        LOG.warning(_LW("Cannot deallocate node %(node_uuid)s "
+                        "in OneView because it is not in use by "
+                        "ironic."), {'node_uuid': node.uuid})
diff --git a/ironic/drivers/modules/oneview/power.py b/ironic/drivers/modules/oneview/power.py
index 82c05693b..3ea0d94b4 100644
--- a/ironic/drivers/modules/oneview/power.py
+++ b/ironic/drivers/modules/oneview/power.py
@@ -114,6 +114,14 @@ class OneViewPower(base.PowerInterface):
         :raises: PowerStateFailure if the power couldn't be set to power_state.
         :raises: OneViewError if OneView fails setting the power state.
         """
+        if deploy_utils.is_node_in_use_by_oneview(self.oneview_client,
+                                                  task.node):
+            raise exception.PowerStateFailure(_(
+                "Cannot set power state '%(power_state)s' to node %(node)s. "
+                "The node is in use by OneView.") %
+                {'power_state': power_state,
+                 'node': task.node.uuid})
+
         oneview_info = common.get_oneview_info(task.node)
 
         LOG.debug('Setting power state of node %(node_uuid)s to '
diff --git a/ironic/tests/unit/drivers/modules/oneview/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/oneview/test_deploy_utils.py
index c19a61399..10e727bff 100644
--- a/ironic/tests/unit/drivers/modules/oneview/test_deploy_utils.py
+++ b/ironic/tests/unit/drivers/modules/oneview/test_deploy_utils.py
@@ -98,7 +98,12 @@ class OneViewDeployUtilsTestCase(db_base.DbTestCase):
         """`tear_down` behavior when node already has Profile applied
 
         """
-        oneview_client = mock_get_ov_client()
+        sp_uri = '/rest/server-profiles/1234556789'
+        ov_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        ov_client = mock_get_ov_client.return_value
+        ov_client.get_server_hardware_by_uuid.return_value = fake_sh
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             driver_info = task.node.driver_info
@@ -109,12 +114,12 @@ class OneViewDeployUtilsTestCase(db_base.DbTestCase):
             self.assertTrue(
                 'applied_server_profile_uri' in task.node.driver_info
             )
-            deploy_utils.tear_down(oneview_client, task)
+            deploy_utils.tear_down(ov_client, task)
             self.assertFalse(
                 'applied_server_profile_uri' in task.node.driver_info
             )
         self.assertTrue(
-            oneview_client.delete_server_profile.called
+            ov_client.delete_server_profile.called
         )
 
     # Tests for prepare_cleaning
@@ -184,7 +189,12 @@ class OneViewDeployUtilsTestCase(db_base.DbTestCase):
         """Checks if Server Profile was deleted and its uri removed
 
         """
-        oneview_client = mock_get_ov_client()
+        sp_uri = '/rest/server-profiles/1234556789'
+        ov_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        ov_client = mock_get_ov_client.return_value
+        ov_client.get_server_hardware_by_uuid.return_value = fake_sh
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
             driver_info = task.node.driver_info
@@ -193,10 +203,10 @@ class OneViewDeployUtilsTestCase(db_base.DbTestCase):
             task.node.driver_info = driver_info
 
             self.assertIn('applied_server_profile_uri', task.node.driver_info)
-            deploy_utils.tear_down_cleaning(oneview_client, task)
+            deploy_utils.tear_down_cleaning(ov_client, task)
             self.assertNotIn('applied_server_profile_uri',
                              task.node.driver_info)
-            self.assertTrue(oneview_client.delete_server_profile.called)
+            self.assertTrue(ov_client.delete_server_profile.called)
 
     # Tests for is_node_in_use_by_oneview
     def test_is_node_in_use_by_oneview(self, mock_get_ov_client):
@@ -400,3 +410,39 @@ class OneViewDeployUtilsTestCase(db_base.DbTestCase):
             self.assertTrue(
                 'applied_server_profile_uri' not in task.node.driver_info
             )
+
+    @mock.patch.object(objects.Node, 'save')
+    def test_deallocate_server_hardware_from_ironic_missing_profile_uuid(
+        self, mock_node_save, mock_get_ov_client
+    ):
+        """Test for case when server profile application fails.
+
+        Due to an error when applying Server Profile in OneView,
+        the node will have no Server Profile uuid in the
+        'applied_server_profile_uri' namespace. When the method
+        tested is called without Server Profile uuid, the client
+        will raise a ValueError when trying to delete the profile,
+        this error is converted to an OneViewError.
+        """
+
+        ov_client = mock_get_ov_client.return_value
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = 'any/applied_sp_uri/'
+        ov_client.get_server_hardware_by_uuid.return_value = fake_sh
+        ov_client.delete_server_profile.side_effect = ValueError
+        mock_get_ov_client.return_value = ov_client
+
+        with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = 'any/applied_sp_uri/'
+            task.node.driver_info = driver_info
+            self.assertRaises(
+                exception.OneViewError,
+                deploy_utils.deallocate_server_hardware_from_ironic,
+                ov_client,
+                task.node
+            )
+            self.assertTrue(ov_client.delete_server_profile.called)
+            self.assertTrue(
+                'applied_server_profile_uri' in task.node.driver_info
+            )
diff --git a/ironic/tests/unit/drivers/modules/oneview/test_power.py b/ironic/tests/unit/drivers/modules/oneview/test_power.py
index 5800a43a9..7e99dbe91 100644
--- a/ironic/tests/unit/drivers/modules/oneview/test_power.py
+++ b/ironic/tests/unit/drivers/modules/oneview/test_power.py
@@ -30,6 +30,7 @@ from ironic.tests.unit.db import base as db_base
 from ironic.tests.unit.db import utils as db_utils
 from ironic.tests.unit.objects import utils as obj_utils
 
+oneview_models = importutils.try_import('oneview_client.models')
 oneview_exceptions = importutils.try_import('oneview_client.exceptions')
 
 POWER_ON = 'On'
@@ -142,87 +143,163 @@ class OneViewPowerDriverTestCase(db_base.DbTestCase):
             )
 
     def test_set_power_on(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client = mock_get_ov_client.return_value
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
         oneview_client.power_on.return_value = POWER_ON
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.driver.power.set_power_state(task, states.POWER_ON)
-        oneview_client.power_on.assert_called_once_with(self.info)
+            self.info['applied_server_profile_uri'] = sp_uri
+            oneview_client.power_on.assert_called_once_with(self.info)
 
     def test_set_power_off(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client = mock_get_ov_client.return_value
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
         oneview_client.power_off.return_value = POWER_OFF
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.driver.power.set_power_state(task, states.POWER_OFF)
-        oneview_client.power_off.assert_called_once_with(self.info)
+            self.info['applied_server_profile_uri'] = sp_uri
+            oneview_client.power_off.assert_called_once_with(self.info)
 
     def test_set_power_on_fail(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
-        oneview_client.power_on.side_effect = \
-            oneview_exceptions.OneViewException()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
+        exc = oneview_exceptions.OneViewException()
+        oneview_client.power_on.side_effect = exc
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.assertRaises(exception.OneViewError,
                               self.driver.power.set_power_state, task,
                               states.POWER_ON)
+            self.info['applied_server_profile_uri'] = sp_uri
             oneview_client.power_on.assert_called_once_with(self.info)
 
     def test_set_power_off_fail(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
-        oneview_client.power_off.side_effect = \
-            oneview_exceptions.OneViewException()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
+        exc = oneview_exceptions.OneViewException()
+        oneview_client.power_off.side_effect = exc
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.assertRaises(exception.OneViewError,
                               self.driver.power.set_power_state, task,
                               states.POWER_OFF)
+            self.info['applied_server_profile_uri'] = sp_uri
             oneview_client.power_off.assert_called_once_with(self.info)
 
     def test_set_power_invalid_state(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
+        oneview_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
+        exc = oneview_exceptions.OneViewException()
+        oneview_client.power_off.side_effect = exc
+        self.driver.power.oneview_client = oneview_client
+
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.assertRaises(exception.InvalidParameterValue,
                               self.driver.power.set_power_state, task,
                               'fake state')
 
     def test_set_power_reboot(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
         oneview_client.power_off.return_value = POWER_OFF
         oneview_client.power_on.return_value = POWER_ON
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.driver.power.set_power_state(task, states.REBOOT)
-        oneview_client.power_off.assert_called_once_with(self.info)
-        oneview_client.power_on.assert_called_once_with(self.info)
+            self.info['applied_server_profile_uri'] = sp_uri
+            oneview_client.power_off.assert_called_once_with(self.info)
+            oneview_client.power_off.assert_called_once_with(self.info)
+            oneview_client.power_on.assert_called_once_with(self.info)
 
     def test_reboot(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
         oneview_client.power_off.return_value = POWER_OFF
         oneview_client.power_on.return_value = POWER_ON
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context, self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.driver.power.reboot(task)
-
-        oneview_client.power_off.assert_called_once_with(self.info)
-        oneview_client.power_on.assert_called_once_with(self.info)
+            self.info['applied_server_profile_uri'] = sp_uri
+            oneview_client.power_off.assert_called_once_with(self.info)
+            oneview_client.power_on.assert_called_once_with(self.info)
 
     def test_reboot_fail(self, mock_get_ov_client):
+
+        sp_uri = '/any/server-profile'
         oneview_client = mock_get_ov_client()
-        oneview_client.power_off.side_effect = \
-            oneview_exceptions.OneViewException()
+        fake_sh = oneview_models.ServerHardware()
+        fake_sh.server_profile_uri = sp_uri
+        oneview_client.get_server_hardware_by_uuid.return_value = fake_sh
+        exc = oneview_exceptions.OneViewException()
+        oneview_client.power_off.side_effect = exc
         self.driver.power.oneview_client = oneview_client
 
         with task_manager.acquire(self.context,
                                   self.node.uuid) as task:
+            driver_info = task.node.driver_info
+            driver_info['applied_server_profile_uri'] = sp_uri
+            task.node.driver_info = driver_info
             self.assertRaises(exception.OneViewError,
-                              self.driver.power.reboot,
-                              task)
-
-        oneview_client.power_off.assert_called_once_with(self.info)
-        self.assertFalse(oneview_client.power_on.called)
+                              self.driver.power.reboot, task)
+            self.info['applied_server_profile_uri'] = sp_uri
+            oneview_client.power_off.assert_called_once_with(self.info)
+            self.assertFalse(oneview_client.power_on.called)
diff --git a/releasenotes/notes/fix-oneview-deallocate-server-8256e279af837e5d.yaml b/releasenotes/notes/fix-oneview-deallocate-server-8256e279af837e5d.yaml
new file mode 100644
index 000000000..15cecc279
--- /dev/null
+++ b/releasenotes/notes/fix-oneview-deallocate-server-8256e279af837e5d.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  -
+    Fixes an issue with oneview driver trying to deallocate a node when
+    an error is encountered while performing server profile application.
+    Also ensures only those nodes that are managed by ironic can be
+    deallocated.