diff --git a/doc/source/devref/callbacks.rst b/doc/source/devref/callbacks.rst index 42ebf52a452..0e4a06e64cf 100644 --- a/doc/source/devref/callbacks.rst +++ b/doc/source/devref/callbacks.rst @@ -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: diff --git a/neutron/callbacks/events.py b/neutron/callbacks/events.py index 7dfd83d5e8e..5b3209a7d23 100644 --- a/neutron/callbacks/events.py +++ b/neutron/callbacks/events.py @@ -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_' diff --git a/neutron/callbacks/manager.py b/neutron/callbacks/manager.py index 625f52070eb..7a81b10b049 100644 --- a/neutron/callbacks/manager.py +++ b/neutron/callbacks/manager.py @@ -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.""" diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 42b8040fe47..a55f7f46180 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -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 diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 59e40c74d03..7b104bdf900 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -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)])