Do not complain in firewall_group_deleted if the FW is already deleted
Currently firewall_group_deleted[1] crashs if the firewall is already
deleted or deleted concurrently during firewall_deleted call. We should
avoid such behavior as there is no reason to crash if someone already
did the job for us (ie: delete the FW).
Moreover such crash is costly because it triggers a service-sync on FWaaS
l3-reference agent (at least). Typically on a L3-DVR deployment, all
firewall_deleted calls except the first one will fail so quite every
L3-DVR will perform a FWaaS service-sync.
This change updates firewall_group_deleted in order to succeed if the
firewall is already deleted or if the firewall is deleted concurrently.
[1] neutron.services.firewall.fwaas_plugin_v2.FirewallCallbacks
Change-Id: Ic0b228896c8129205224417506bb06471e432955
Closes-Bug: #1658060
(cherry picked from commit 6bf84e7afb
)
This commit is contained in:
parent
40c81acb7f
commit
f052634943
|
@ -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."""
|
||||
|
|
|
@ -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):
|
||||
|
|
Loading…
Reference in New Issue