openstack/neutron
Code Issues Proposed changes

Add precommit_XXX event for security group and rules

Browse Source 
Current BEFORE_CREATE/DELETE/UPDATE event is outside of the DB
transaction. Unlike the precommit primitive in ML2 mech drivers, they
don't work in the same DB transaction of resource, so if we want to
operate the DB in mech driver related to security group, there would be
more unsync issues if we use BEFORE_XXX event directly. Moving the
BEFORE_XXX event inside may also break some current codes, as maybe RPC
call included.

This patch adds new PRECOMMIT_CREATE/DELETE/UPDATE event type for
callback function, and use it in the securitygroup/rules DB transaction.
PRECOMMIT_XXX is in the DB transaction and only purpose is to do DB
operations in its callback. A CallbackFailure will be triggered when
exception comes from the callback of the new event.

Change-Id: Icd2849bd84dab6733a572e8c85f242afcefc6c78
Closes-Bug: #1522172
This commit is contained in:
Yalei Wang
2015-12-03 21:38:16 +08:00
parent 555dc187eb
commit c24e9da2f8
5 changed files with 206 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
doc/source/devref/callbacks.rst
View File

@@ -230,6 +230,13 @@ actions during the ABORT_CREATE event. It is worth noting that it is not mandato
the same callback register to both BEFORE_* and the respective ABORT_* event; as a matter of
fact, it is best to make use of different callbacks to keep the two logic separate.
As we can see from the last example, exception which is triggered in some callback will be
recorded, and it will not prevent the other remaining callbacks execution. Exception triggered in
callback of BEFORE_XXX will make notify process generate an ABORT_XXX event and call the related
callback, while exception from PRECOMMIT_XXX will not generate ABORT_XXX event. But both of them
will finally raise a unified CallbackFailure exception to the outside. For the exception triggered
from other events, like AFTER_XXX and ABORT_XXX there will no exception raised to the outside.
Unsubscribing to events
-----------------------
@@ -366,6 +373,13 @@ Is the registry thread-safe?
In this case, chances that things do go badly may be pretty slim. Making the registry
thread-safe will be considered as a future improvement.
What kind of operation I can add into callback?
For callback function of PRECOMMIT_XXX events, we can't use blocking functions or a function
that would take a long time, like communicating to SDN controller over network.
Callbacks for PRECOMMIT events are meant to execute DB operations in a transaction context, the
errors occured will be taken care by the context manager.
What kind of function can be a callback?
Anything you fancy: lambdas, 'closures', class, object or module methods. For instance:

5
neutron/callbacks/events.py
View File

@@ -16,6 +16,10 @@ BEFORE_READ = 'before_read'
BEFORE_UPDATE = 'before_update'
BEFORE_DELETE = 'before_delete'
PRECOMMIT_CREATE = 'precommit_create'
PRECOMMIT_UPDATE = 'precommit_update'
PRECOMMIT_DELETE = 'precommit_delete'
AFTER_CREATE = 'after_create'
AFTER_READ = 'after_read'
AFTER_UPDATE = 'after_update'
@@ -28,3 +32,4 @@ ABORT_DELETE = 'abort_delete'
ABORT = 'abort_'
BEFORE = 'before_'
PRECOMMIT = 'precommit_'

15
neutron/callbacks/manager.py
View File

@@ -116,11 +116,16 @@ class CallbacksManager(object):
:param trigger: the trigger. A reference to the sender of the event.
"""
errors = self._notify_loop(resource, event, trigger, **kwargs)
if errors and event.startswith(events.BEFORE):
abort_event = event.replace(
events.BEFORE, events.ABORT)
self._notify_loop(resource, abort_event, trigger, **kwargs)
raise exceptions.CallbackFailure(errors=errors)
if errors:
if event.startswith(events.BEFORE):
abort_event = event.replace(
events.BEFORE, events.ABORT)
self._notify_loop(resource, abort_event, trigger, **kwargs)
raise exceptions.CallbackFailure(errors=errors)
if event.startswith(events.PRECOMMIT):
raise exceptions.CallbackFailure(errors=errors)
def clear(self):
"""Brings the manager to a clean slate."""

94
neutron/db/securitygroups_db.py
View File

@@ -122,6 +122,18 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
return self._create_bulk('security_group', context,
security_group_rule)
def _registry_notify(self, res, event, id=None, exc_cls=None, **kwargs):
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(res, event, self, **kwargs)
except exceptions.CallbackFailure as e:
if exc_cls:
reason = _('cannot perform %(event)s due to %(reason)s'), {
'event': event, 'reason': e}
raise exc_cls(reason=reason, id=id)
def create_security_group(self, context, security_group, default_sg=False):
"""Create security group.
@@ -134,15 +146,9 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
'security_group': s,
'is_default': default_sg,
}
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(
resources.SECURITY_GROUP, events.BEFORE_CREATE, self,
**kwargs)
except exceptions.CallbackFailure as e:
raise ext_sg.SecurityGroupConflict(reason=e)
self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_CREATE,
exc_cls=ext_sg.SecurityGroupConflict, **kwargs)
tenant_id = s['tenant_id']
@@ -177,6 +183,10 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
direction='egress',
ethertype=ethertype)
context.session.add(egress_rule)
self._registry_notify(resources.SECURITY_GROUP,
events.PRECOMMIT_CREATE,
exc_cls=ext_sg.SecurityGroupConflict,
**kwargs)
secgroup_dict = self._make_security_group_dict(security_group_db)
@@ -258,18 +268,15 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
'security_group_id': id,
'security_group': sg,
}
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(
resources.SECURITY_GROUP, events.BEFORE_DELETE, self,
**kwargs)
except exceptions.CallbackFailure as e:
reason = _('cannot be deleted due to %s') % e
raise ext_sg.SecurityGroupInUse(id=id, reason=reason)
self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_DELETE,
exc_cls=ext_sg.SecurityGroupInUse, id=id,
**kwargs)
with context.session.begin(subtransactions=True):
self._registry_notify(resources.SECURITY_GROUP,
events.PRECOMMIT_DELETE,
exc_cls=ext_sg.SecurityGroupInUse, id=id,
**kwargs)
context.session.delete(sg)
kwargs.pop('security_group')
@@ -284,20 +291,17 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
'security_group_id': id,
'security_group': s,
}
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(
resources.SECURITY_GROUP, events.BEFORE_UPDATE, self,
**kwargs)
except exceptions.CallbackFailure as e:
raise ext_sg.SecurityGroupConflict(reason=e)
self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_UPDATE,
exc_cls=ext_sg.SecurityGroupConflict, **kwargs)
with context.session.begin(subtransactions=True):
sg = self._get_security_group(context, id)
if sg['name'] == 'default' and 'name' in s:
raise ext_sg.SecurityGroupCannotUpdateDefault()
self._registry_notify(
resources.SECURITY_GROUP,
events.PRECOMMIT_UPDATE,
exc_cls=ext_sg.SecurityGroupConflict, **kwargs)
sg.update(s)
sg_dict = self._make_security_group_dict(sg)
@@ -378,15 +382,9 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
'context': context,
'security_group_rule': rule_dict
}
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(
resources.SECURITY_GROUP_RULE, events.BEFORE_CREATE, self,
**kwargs)
except exceptions.CallbackFailure as e:
raise ext_sg.SecurityGroupConflict(reason=e)
self._registry_notify(resources.SECURITY_GROUP_RULE,
events.BEFORE_CREATE,
exc_cls=ext_sg.SecurityGroupConflict, **kwargs)
with context.session.begin(subtransactions=True):
db = SecurityGroupRule(
@@ -401,6 +399,9 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
port_range_max=rule_dict['port_range_max'],
remote_ip_prefix=rule_dict.get('remote_ip_prefix'))
context.session.add(db)
self._registry_notify(resources.SECURITY_GROUP_RULE,
events.PRECOMMIT_CREATE,
exc_cls=ext_sg.SecurityGroupConflict, **kwargs)
res_rule_dict = self._make_security_group_rule_dict(db)
kwargs['security_group_rule'] = res_rule_dict
registry.notify(
@@ -614,20 +615,19 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase):
'context': context,
'security_group_rule_id': id
}
# NOTE(armax): a callback exception here will prevent the request
# from being processed. This is a hook point for backend's validation;
# we raise to propagate the reason for the failure.
try:
registry.notify(
resources.SECURITY_GROUP_RULE, events.BEFORE_DELETE, self,
**kwargs)
except exceptions.CallbackFailure as e:
reason = _('cannot be deleted due to %s') % e
raise ext_sg.SecurityGroupRuleInUse(id=id, reason=reason)
self._registry_notify(resources.SECURITY_GROUP_RULE,
events.BEFORE_DELETE, id=id,
exc_cls=ext_sg.SecurityGroupRuleInUse, **kwargs)
with context.session.begin(subtransactions=True):
query = self._model_query(context, SecurityGroupRule).filter(
SecurityGroupRule.id == id)
self._registry_notify(resources.SECURITY_GROUP_RULE,
events.PRECOMMIT_DELETE,
exc_cls=ext_sg.SecurityGroupRuleInUse, id=id,
**kwargs)
try:
# As there is a filter on a primary key it is not possible for
# MultipleResultsFound to be raised

130
neutron/tests/unit/db/test_securitygroups_db.py
View File

@@ -12,10 +12,13 @@
# limitations under the License.
import mock
import sqlalchemy
import testtools
from neutron.callbacks import events
from neutron.callbacks import exceptions
from neutron.callbacks import registry
from neutron.callbacks import resources
from neutron.common import constants
from neutron import context
from neutron.db import common_db_mixin
@@ -24,6 +27,21 @@ from neutron.extensions import securitygroup
from neutron.tests.unit import testlib_api
FAKE_SECGROUP = {'security_group': {"tenant_id": 'fake', 'description':
'fake', 'name': 'fake'}}
FAKE_SECGROUP_RULE = {'security_group_rule': {"tenant_id": 'fake',
'description': 'fake', 'name': 'fake', 'port_range_min':
'21', 'protocol': 'tcp', 'port_range_max': '23',
'remote_ip_prefix': '10.0.0.1', 'ethertype': 'IPv4',
'remote_group_id': None, 'security_group_id': 'None',
'direction': 'ingress'}}
def fake_callback(resource, event, *args, **kwargs):
raise KeyError('bar')
class SecurityGroupDbMixinImpl(securitygroups_db.SecurityGroupDbMixin,
common_db_mixin.CommonDbMixin):
pass
@@ -103,3 +121,115 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase):
with testtools.ExpectedException(
securitygroup.SecurityGroupEthertypeConflictWithProtocol):
self.mixin._validate_ethertype_and_protocol(rule)
def test_security_group_precommit_create_event_fail(self):
registry.subscribe(fake_callback, resources.SECURITY_GROUP,
events.PRECOMMIT_CREATE)
with mock.patch.object(sqlalchemy.orm.session.SessionTransaction,
'rollback') as mock_rollback:
self.assertRaises(securitygroup.SecurityGroupConflict,
self.mixin.create_security_group,
self.ctx, FAKE_SECGROUP)
self.assertTrue(mock_rollback.called)
def test_security_group_precommit_update_event_fail(self):
registry.subscribe(fake_callback, resources.SECURITY_GROUP,
events.PRECOMMIT_UPDATE)
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
with mock.patch.object(sqlalchemy.orm.session.SessionTransaction,
'rollback') as mock_rollback:
self.assertRaises(securitygroup.SecurityGroupConflict,
self.mixin.update_security_group,
self.ctx, sg_dict['id'], FAKE_SECGROUP)
self.assertTrue(mock_rollback.called)
def test_security_group_precommit_delete_event_fail(self):
registry.subscribe(fake_callback, resources.SECURITY_GROUP,
events.PRECOMMIT_DELETE)
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
with mock.patch.object(sqlalchemy.orm.session.SessionTransaction,
'rollback') as mock_rollback:
self.assertRaises(securitygroup.SecurityGroupInUse,
self.mixin.delete_security_group,
self.ctx, sg_dict['id'])
self.assertTrue(mock_rollback.called)
def test_security_group_precommit_create_event(self):
with mock.patch.object(registry, "notify") as mock_notify:
self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
mock_notify.assert_has_calls([mock.call('security_group',
'precommit_create', mock.ANY, context=mock.ANY,
is_default=mock.ANY, security_group=mock.ANY)])
def test_security_group_precommit_update_event(self):
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
with mock.patch.object(registry, "notify") as mock_notify:
self.mixin.update_security_group(self.ctx, sg_dict['id'],
FAKE_SECGROUP)
mock_notify.assert_has_calls([mock.call('security_group',
'precommit_update', mock.ANY, context=mock.ANY,
security_group=mock.ANY, security_group_id=sg_dict['id'])])
def test_security_group_precommit_delete_event(self):
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
with mock.patch.object(registry, "notify") as mock_notify:
self.mixin.delete_security_group(self.ctx, sg_dict['id'])
mock_notify.assert_has_calls([mock.call('security_group',
'precommit_delete', mock.ANY, context=mock.ANY,
security_group=mock.ANY, security_group_id=sg_dict['id'])])
def test_security_group_rule_precommit_create_event_fail(self):
registry.subscribe(fake_callback, resources.SECURITY_GROUP_RULE,
events.PRECOMMIT_CREATE)
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
fake_rule = FAKE_SECGROUP_RULE
fake_rule['security_group_rule']['security_group_id'] = sg_dict['id']
with mock.patch.object(sqlalchemy.orm.session.SessionTransaction,
'rollback') as mock_rollback,\
mock.patch.object(self.mixin, '_get_security_group'):
self.assertRaises(securitygroup.SecurityGroupConflict,
self.mixin.create_security_group_rule,
self.ctx, fake_rule)
self.assertTrue(mock_rollback.called)
def test_security_group_rule_precommit_delete_event_fail(self):
registry.subscribe(fake_callback, resources.SECURITY_GROUP_RULE,
events.PRECOMMIT_DELETE)
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
fake_rule = FAKE_SECGROUP_RULE
fake_rule['security_group_rule']['security_group_id'] = sg_dict['id']
with mock.patch.object(sqlalchemy.orm.session.SessionTransaction,
'rollback') as mock_rollback,\
mock.patch.object(self.mixin, '_get_security_group'):
sg_rule_dict = self.mixin.create_security_group_rule(self.ctx,
fake_rule)
self.assertRaises(securitygroup.SecurityGroupRuleInUse,
self.mixin.delete_security_group_rule, self.ctx,
sg_rule_dict['id'])
self.assertTrue(mock_rollback.called)
def test_security_group_rule_precommit_create_event(self):
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
fake_rule = FAKE_SECGROUP_RULE
fake_rule['security_group_rule']['security_group_id'] = sg_dict['id']
with mock.patch.object(registry, "notify") as mock_notify, \
mock.patch.object(self.mixin, '_get_security_group'):
self.mixin.create_security_group_rule(self.ctx,
fake_rule)
mock_notify.assert_has_calls([mock.call('security_group_rule',
'precommit_create', mock.ANY, context=mock.ANY,
security_group_rule=mock.ANY)])
def test_security_group_rule_precommit_delete_event(self):
sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP)
fake_rule = FAKE_SECGROUP_RULE
fake_rule['security_group_rule']['security_group_id'] = sg_dict['id']
with mock.patch.object(registry, "notify") as mock_notify, \
mock.patch.object(self.mixin, '_get_security_group'):
sg_rule_dict = self.mixin.create_security_group_rule(self.ctx,
fake_rule)
self.mixin.delete_security_group_rule(self.ctx,
sg_rule_dict['id'])
mock_notify.assert_has_calls([mock.call('security_group_rule',
'precommit_delete', mock.ANY, context=mock.ANY,
security_group_rule_id=mock.ANY)])