Merge "Do not complain in firewall_group_deleted if the FW is already deleted" into stable/newton

neutron_fwaas/services/firewall/fwaas_plugin_v2.py

@@ -20,6 +20,7 @@ from oslo_config import cfg
 from oslo_log import log as logging
 import oslo_messaging
 
+from neutron_fwaas._i18n import _LI
 from neutron_fwaas.common import fwaas_constants as f_const
 from neutron_fwaas.db.firewall.v2 import firewall_db_v2
 from neutron_fwaas.extensions import firewall_v2 as fw_ext
@@ -84,18 +85,23 @@ class FirewallCallbacks(object):
     def firewall_group_deleted(self, context, fwg_id, **kwargs):
         """Agent uses this to indicate firewall is deleted."""
         LOG.debug("firewall_group_deleted() called")
-        with context.session.begin(subtransactions=True):
-            fwg_db = self.plugin._get_firewall_group(context, fwg_id)
-            # allow to delete firewalls in ERROR state
-            if fwg_db.status in (n_const.PENDING_DELETE, n_const.ERROR):
-                self.plugin.delete_db_firewall_group_object(context, fwg_id)
-                return True
-            else:
-                LOG.warning(('Firewall %(fwg)s unexpectedly deleted by '
-                             'agent, status was %(status)s'),
-                            {'fwg': fwg_id, 'status': fwg_db.status})
-                fwg_db.update({"status": n_const.ERROR})
-                return False
+        try:
+            with context.session.begin(subtransactions=True):
+                fwg_db = self.plugin._get_firewall_group(context, fwg_id)
+                # allow to delete firewalls in ERROR state
+                if fwg_db.status in (n_const.PENDING_DELETE, n_const.ERROR):
+                    self.plugin.delete_db_firewall_group_object(context,
+                                                                fwg_id)
+                    return True
+                else:
+                    LOG.warning(('Firewall %(fwg)s unexpectedly deleted by '
+                                 'agent, status was %(status)s'),
+                                {'fwg': fwg_id, 'status': fwg_db.status})
+                    fwg_db.update({"status": n_const.ERROR})
+                    return False
+        except fw_ext.FirewallGroupNotFound:
+            LOG.info(_LI('Firewall group %s already deleted'), fwg_id)
+            return True
 
     def get_firewall_groups_for_project(self, context, **kwargs):
         """Gets all firewall_groups and rules on a project."""

neutron_fwaas/tests/unit/services/firewall/test_fwaas_plugin_v2.py

@@ -157,6 +157,85 @@ class TestFirewallCallbacks(TestFirewallRouterPortBase):
                 self.assertEqual(const.ERROR, fwg_db['status'])
                 self.assertFalse(res)
 
+    def test_firewall_group_deleted(self):
+        ctx = context.get_admin_context()
+        with self.firewall_policy() as fwp:
+            fwp_id = fwp['firewall_policy']['id']
+            with self.firewall_group(
+                ingress_firewall_policy_id=fwp_id,
+                admin_state_up=test_db_firewall.ADMIN_STATE_UP,
+                do_delete=False
+            ) as fwg:
+                fwg_id = fwg['firewall_group']['id']
+                with ctx.session.begin(subtransactions=True):
+                    fwg_db = self.plugin._get_firewall_group(ctx, fwg_id)
+                    fwg_db['status'] = const.PENDING_DELETE
+
+                observed = self.callbacks.firewall_group_deleted(
+                    ctx, fwg_id, host='dummy')
+                self.assertTrue(observed)
+
+            self.assertRaises(firewall_v2.FirewallGroupNotFound,
+                              self.plugin.get_firewall_group,
+                              ctx, fwg_id)
+
+    def test_firewall_group_deleted_concurrently(self):
+        ctx = context.get_admin_context()
+        alt_ctx = context.get_admin_context()
+
+        _get_firewall_group = self.plugin._get_firewall_group
+
+        def getdelete(context, fwg_id):
+            fwg_db = _get_firewall_group(context, fwg_id)
+            # NOTE(cby): Use a different session to simulate a concurrent del
+            self.plugin.delete_db_firewall_group_object(alt_ctx, fwg_id)
+            return fwg_db
+
+        with self.firewall_policy() as fwp:
+            fwp_id = fwp['firewall_policy']['id']
+            with self.firewall_group(
+                firewall_policy_id=fwp_id,
+                admin_state_up=test_db_firewall.ADMIN_STATE_UP,
+                do_delete=False
+            ) as fwg:
+                fwg_id = fwg['firewall_group']['id']
+                with ctx.session.begin(subtransactions=True):
+                    fwg_db = self.plugin._get_firewall_group(ctx, fwg_id)
+                    fwg_db['status'] = const.PENDING_DELETE
+                    ctx.session.flush()
+
+                with mock.patch.object(
+                    self.plugin, '_get_firewall_group', side_effect=getdelete
+                ):
+                    observed = self.callbacks.firewall_group_deleted(
+                        ctx, fwg_id, host='dummy')
+                    self.assertTrue(observed)
+
+                self.assertRaises(firewall_v2.FirewallGroupNotFound,
+                                  self.plugin.get_firewall_group,
+                                  ctx, fwg_id)
+
+    def test_firewall_group_deleted_not_found(self):
+        ctx = context.get_admin_context()
+        observed = self.callbacks.firewall_group_deleted(
+            ctx, 'notfound', host='hh')
+        self.assertTrue(observed)
+
+    def test_firewall_group_deleted_error(self):
+        ctx = context.get_admin_context()
+        with self.firewall_policy() as fwp:
+            fwp_id = fwp['firewall_policy']['id']
+            with self.firewall_group(
+                firewall_policy_id=fwp_id,
+                admin_state_up=test_db_firewall.ADMIN_STATE_UP,
+            ) as fwg:
+                fwg_id = fwg['firewall_group']['id']
+                observed = self.callbacks.firewall_group_deleted(
+                    ctx, fwg_id, host='dummy')
+                self.assertFalse(observed)
+                fwg_db = self.plugin._get_firewall_group(ctx, fwg_id)
+                self.assertEqual(const.ERROR, fwg_db['status'])
+
 
 class TestFirewallPluginBasev2(TestFirewallRouterPortBase,
                              test_l3_plugin.L3NatTestCaseMixin):