Merge pull request #1 from stackhpc/bug/1815799-rocky

[ironic] Don't remove instance info twice in destroy

Author: markgoddard

nova/tests/unit/virt/ironic/test_driver.py

@@ -1809,7 +1809,8 @@ def fake_set_provision_state(*_):
 
         mock_node.get_by_instance_uuid.assert_called_with(
             instance.uuid, fields=ironic_driver._NODE_FIELDS)
-        mock_cleanup_deploy.assert_called_with(node, instance, network_info)
+        mock_cleanup_deploy.assert_called_with(node, instance, network_info,
+                                               remove_instance_info=False)
 
         # For states that makes sense check if set_provision_state has
         # been called
@@ -1842,7 +1843,8 @@ def test_destroy_trigger_undeploy_fail(self, mock_clean, fake_validate,
         mock_sps.side_effect = exception.NovaException()
         self.assertRaises(exception.NovaException, self.driver.destroy,
                           self.ctx, instance, None, None)
-        mock_clean.assert_called_once_with(node, instance, None)
+        mock_clean.assert_called_once_with(node, instance, None,
+                                           remove_instance_info=False)
 
     @mock.patch.object(FAKE_CLIENT.node, 'update')
     @mock.patch.object(ironic_driver.IronicDriver,
@@ -1861,7 +1863,8 @@ def test_destroy_trigger_remove_info_fail(self, mock_clean, fake_validate,
         mock_update.side_effect = SystemError('unexpected error')
         self.assertRaises(SystemError, self.driver.destroy,
                           self.ctx, instance, None, None)
-        mock_clean.assert_called_once_with(node, instance, None)
+        mock_clean.assert_called_once_with(node, instance, None,
+                                           remove_instance_info=False)
 
     @mock.patch.object(FAKE_CLIENT.node, 'set_provision_state')
     @mock.patch.object(ironic_driver.IronicDriver,
@@ -2692,6 +2695,24 @@ def test__cleanup_deploy(self, mock_call, mock_vol, mock_unvif,
         mock_call.has_calls(
             [mock.call('node.update', node.uuid, expected_patch)])
 
+    @mock.patch.object(ironic_driver.IronicDriver, '_stop_firewall')
+    @mock.patch.object(ironic_driver.IronicDriver, '_unplug_vifs')
+    @mock.patch.object(ironic_driver.IronicDriver,
+                       '_cleanup_volume_target_info')
+    @mock.patch.object(cw.IronicClientWrapper, 'call')
+    def test__cleanup_deploy_no_remove_ii(self, mock_call, mock_vol,
+                                          mock_unvif, mock_stop_fw):
+        # TODO(TheJulia): This REALLY should be updated to cover all of the
+        # calls that take place.
+        node = ironic_utils.get_test_node(driver='fake')
+        instance = fake_instance.fake_instance_obj(self.ctx,
+                                                   node=node.uuid)
+        self.driver._cleanup_deploy(node, instance, remove_instance_info=False)
+        mock_vol.assert_called_once_with(instance)
+        mock_unvif.assert_called_once_with(node, instance, None)
+        mock_stop_fw.assert_called_once_with(instance, None)
+        self.assertFalse(mock_call.called)
+
 
 class IronicDriverSyncTestCase(IronicDriverTestCase):
 

nova/virt/ironic/driver.py

@@ -471,11 +471,13 @@ def _cleanup_volume_target_info(self, instance):
                              'reason': e},
                             instance=instance)
 
-    def _cleanup_deploy(self, node, instance, network_info=None):
+    def _cleanup_deploy(self, node, instance, network_info=None,
+                        remove_instance_info=True):
         self._cleanup_volume_target_info(instance)
         self._unplug_vifs(node, instance, network_info)
         self._stop_firewall(instance, network_info)
-        self._remove_instance_info_from_node(node, instance)
+        if remove_instance_info:
+            self._remove_instance_info_from_node(node, instance)
 
     def _wait_for_active(self, instance):
         """Wait for the node to be marked as ACTIVE in Ironic."""
@@ -1264,7 +1266,12 @@ def destroy(self, context, instance, network_info,
                 #                removed from ironic node.
                 self._remove_instance_info_from_node(node, instance)
         finally:
-            self._cleanup_deploy(node, instance, network_info)
+            # NOTE(mgoddard): We don't need to remove instance info at this
+            # point since we will have already done it. The destroy will only
+            # succeed if this method returns without error, so we will end up
+            # removing the instance info eventually.
+            self._cleanup_deploy(node, instance, network_info,
+                                 remove_instance_info=False)
 
         LOG.info('Successfully unprovisioned Ironic node %s',
                  node.uuid, instance=instance)