Don't always replace FAILED ServerGroup resources

ServerGroup resources work as scheduler hints for server
resources and replacement of it replaces the servers which
is undesirable in many circumstances.

- We don't allow update of ServerGroup resource properties
- If not created (CREATE_FAILED) without resource_id they will
  be replaced
- If does not exist in nova they would be created
- If exist in nova, FAILED resource status would be just
  changed to COMPLETE

Task: 45748

Change-Id: I096dff2b541a5aa8afbbbcea5161e7ca1c244039
(cherry picked from commit a0e072b3204d736b9a4a5b2ceb2157f57405ac6f)
(cherry picked from commit f9f5d2cc368e5f850b962c8d89669a50544058d1)

2 files changed, 47 insertions, 5 deletions

diff --git a/heat/engine/resources/openstack/nova/server_group.py b/heat/engine/resources/openstack/nova/server_group.py
index b4eb95662..9930033fa 100644
--- a/heat/engine/resources/openstack/nova/server_group.py
+++ b/heat/engine/resources/openstack/nova/server_group.py
@@ -106,6 +106,16 @@ class ServerGroup(resource.Resource):
                 name=name, policies=policies)
         self.resource_id_set(server_group.id)
 
+    def needs_replace_failed(self):
+        if not self.resource_id:
+            return True
+
+        with self.client_plugin().ignore_not_found:
+            self._show_resource()
+            return False
+
+        return True
+
     def physical_resource_name(self):
         name = self.properties[self.NAME]
         if name:
diff --git a/heat/tests/openstack/nova/test_server_group.py b/heat/tests/openstack/nova/test_server_group.py
index c2dfda484..88f28b5c5 100644
--- a/heat/tests/openstack/nova/test_server_group.py
+++ b/heat/tests/openstack/nova/test_server_group.py
@@ -14,6 +14,9 @@
 import json
 from unittest import mock
 
+from novaclient import exceptions
+from oslo_utils import excutils
+
 from heat.common import template_format
 from heat.engine import scheduler
 from heat.tests import common
@@ -52,12 +55,23 @@ class NovaServerGroupTest(common.HeatTestCase):
         # create mock clients and objects
         nova = mock.MagicMock()
         self.sg.client = mock.MagicMock(return_value=nova)
-        mock_plugin = mock.MagicMock()
-        self.patchobject(mock_plugin,
-                         'is_version_supported',
-                         return_value=True)
+
+        class FakeNovaPlugin(object):
+
+            @excutils.exception_filter
+            def ignore_not_found(self, ex):
+                if not isinstance(ex, exceptions.NotFound):
+                    raise ex
+
+            def is_version_supported(self, version):
+                return True
+
+            def is_conflict(self, ex):
+                return False
+
+        self.patchobject(excutils.exception_filter, '__exit__')
         self.patchobject(self.sg, 'client_plugin',
-                         return_value=mock_plugin)
+                         return_value=FakeNovaPlugin())
         self.sg_mgr = nova.server_groups
 
     def _create_sg(self, name):
@@ -109,3 +123,21 @@ class NovaServerGroupTest(common.HeatTestCase):
         self.sg.client().server_groups = s_groups
         self.assertEqual({'server_gr': 'info'}, self.sg.FnGetAtt('show'))
         s_groups.get.assert_called_once_with('test')
+
+    def test_needs_replace_failed(self):
+        self._create_sg('test')
+        self.sg.state_set(self.sg.CREATE, self.sg.FAILED)
+        mock_show_resource = self.patchobject(self.sg, '_show_resource')
+        mock_show_resource.side_effect = [exceptions.NotFound(404), None]
+
+        self.sg.resource_id = None
+        self.assertTrue(self.sg.needs_replace_failed())
+        self.assertEqual(0, mock_show_resource.call_count)
+
+        self.sg.resource_id = 'sg_id'
+        self.assertTrue(self.sg.needs_replace_failed())
+        self.assertEqual(1, mock_show_resource.call_count)
+
+        mock_show_resource.return_value = None
+        self.assertFalse(self.sg.needs_replace_failed())
+        self.assertEqual(2, mock_show_resource.call_count)