From b05a9186d1246460b21d3db26d5e7e70717e2d77 Mon Sep 17 00:00:00 2001 From: Boden R Date: Thu, 1 Aug 2019 08:14:34 -0600 Subject: [PATCH] use callback payloads for SECURITY_GROUP This patch switches over to callback payloads for SECURITY_GROUP events. To do so a few shims are put into place the handle both payload and kwarg style callbacks; these shims will be removed once all events use payloads. In addition a few UT updates are included to get the tests working properly with payloads. Change-Id: I6161a8b387812808c4d679f882a3193c93235647 --- neutron/db/securitygroups_db.py | 71 +++++++------- .../drivers/ovn/mech_driver/mech_driver.py | 29 +++--- neutron/plugins/ml2/ovo_rpc.py | 3 +- .../tests/unit/db/test_securitygroups_db.py | 94 ++++++++++++------- .../ovn/mech_driver/test_mech_driver.py | 8 +- .../unit/plugins/ml2/test_security_group.py | 4 +- 6 files changed, 124 insertions(+), 85 deletions(-) diff --git a/neutron/db/securitygroups_db.py b/neutron/db/securitygroups_db.py index 4270bb39d47..f64d86e2600 100644 --- a/neutron/db/securitygroups_db.py +++ b/neutron/db/securitygroups_db.py @@ -87,11 +87,6 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, a given tenant if it does not exist. """ s = security_group['security_group'] - kwargs = { - 'context': context, - 'security_group': s, - 'is_default': default_sg, - } self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_CREATE, exc_cls=ext_sg.SecurityGroupConflict, payload=events.DBEventPayload( @@ -148,14 +143,22 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, # fetch sg from db to load the sg rules with sg model. sg = sg_obj.SecurityGroup.get_object(context, id=sg.id) secgroup_dict = self._make_security_group_dict(sg) - kwargs['security_group'] = secgroup_dict self._registry_notify(resources.SECURITY_GROUP, events.PRECOMMIT_CREATE, exc_cls=ext_sg.SecurityGroupConflict, - **kwargs) + payload=events.DBEventPayload( + context, + resource_id=sg.id, + metadata={'is_default': default_sg}, + states=(secgroup_dict,))) + + registry.publish(resources.SECURITY_GROUP, events.AFTER_CREATE, + self, payload=events.DBEventPayload( + context, + resource_id=secgroup_dict['id'], + metadata={'is_default': default_sg}, + states=(secgroup_dict,))) - registry.notify(resources.SECURITY_GROUP, events.AFTER_CREATE, self, - **kwargs) return secgroup_dict @db_api.retry_if_session_inactive() @@ -248,11 +251,7 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, if sg['name'] == 'default' and not context.is_admin: raise ext_sg.SecurityGroupCannotRemoveDefault() - kwargs = { - 'context': context, - 'security_group_id': id, - 'security_group': sg, - } + self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_DELETE, exc_cls=ext_sg.SecurityGroupInUse, id=id, @@ -267,18 +266,24 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, # deleted ports = self._get_port_security_group_bindings(context, filters) sg = self._get_security_group(context, id) - kwargs['security_group_rule_ids'] = [r['id'] for r in sg.rules] - kwargs['security_group'] = self._make_security_group_dict(sg) + sgr_ids = [r['id'] for r in sg.rules] + sec_group = self._make_security_group_dict(sg) self._registry_notify(resources.SECURITY_GROUP, events.PRECOMMIT_DELETE, - exc_cls=ext_sg.SecurityGroupInUse, id=id, - **kwargs) + exc_cls=ext_sg.SecurityGroupInUse, + payload=events.DBEventPayload( + context, resource_id=id, + states=(sec_group,), + metadata={ + 'security_group_rule_ids': sgr_ids + })) sg.delete() - kwargs.pop('security_group') - kwargs['name'] = sg['name'] - registry.notify(resources.SECURITY_GROUP, events.AFTER_DELETE, - self, **kwargs) + registry.publish(resources.SECURITY_GROUP, events.AFTER_DELETE, + self, payload=events.DBEventPayload( + context, resource_id=id, states=(sec_group,), + metadata={'security_group_rule_ids': sgr_ids, + 'name': sg['name']})) @db_api.retry_if_session_inactive() def update_security_group(self, context, id, security_group): @@ -294,34 +299,34 @@ class SecurityGroupDbMixin(ext_sg.SecurityGroupPluginBase, if ports: raise ext_sg.SecurityGroupInUse(id=id) - kwargs = { - 'context': context, - 'security_group_id': id, - 'security_group': s, - } self._registry_notify(resources.SECURITY_GROUP, events.BEFORE_UPDATE, - exc_cls=ext_sg.SecurityGroupConflict, **kwargs) + exc_cls=ext_sg.SecurityGroupConflict, + payload=events.DBEventPayload( + context, resource_id=id, states=(s,))) with db_api.CONTEXT_WRITER.using(context): sg = self._get_security_group(context, id) if sg.name == 'default' and 'name' in s: raise ext_sg.SecurityGroupCannotUpdateDefault() sg_dict = self._make_security_group_dict(sg) - kwargs['original_security_group'] = sg_dict + original_security_group = sg_dict sg.update_fields(s) sg.update() sg_dict = self._make_security_group_dict(sg) - kwargs['security_group'] = sg_dict self._registry_notify( resources.SECURITY_GROUP, events.PRECOMMIT_UPDATE, exc_cls=ext_sg.SecurityGroupConflict, payload=events.DBEventPayload( context, request_body=s, - states=(kwargs['original_security_group'],), + states=(original_security_group,), resource_id=id, desired_state=sg_dict)) - registry.notify(resources.SECURITY_GROUP, events.AFTER_UPDATE, self, - **kwargs) + registry.publish(resources.SECURITY_GROUP, events.AFTER_UPDATE, self, + payload=events.DBEventPayload( + context, request_body=s, + states=(original_security_group, sg_dict), + resource_id=id)) + return sg_dict def _make_security_group_dict(self, security_group, fields=None): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index 52f299300ee..ded7551b839 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -374,29 +374,34 @@ class OVNMechanismDriver(api.MechanismDriver): ovn_nb_api.idl.neutron_pg_drop_event.wait() def _create_security_group_precommit(self, resource, event, trigger, - **kwargs): + payload): + context = payload.context + security_group = payload.latest_state ovn_revision_numbers_db.create_initial_revision( - kwargs['context'], kwargs['security_group']['id'], + context, security_group['id'], ovn_const.TYPE_SECURITY_GROUPS, - std_attr_id=kwargs['security_group']['standard_attr_id']) + std_attr_id=security_group['standard_attr_id']) - def _create_security_group(self, resource, event, trigger, - security_group, **kwargs): - self._ovn_client.create_security_group(kwargs['context'], + def _create_security_group(self, resource, event, trigger, payload): + context = payload.context + security_group = payload.latest_state + self._ovn_client.create_security_group(context, security_group) - def _delete_security_group(self, resource, event, trigger, - security_group_id, **kwargs): - self._ovn_client.delete_security_group(kwargs['context'], + def _delete_security_group(self, resource, event, trigger, payload): + context = payload.context + security_group_id = payload.resource_id + self._ovn_client.delete_security_group(context, security_group_id) - def _update_security_group(self, resource, event, trigger, - security_group, **kwargs): + def _update_security_group(self, resource, event, trigger, payload): # OVN doesn't care about updates to security groups, only if they # exist or not. We are bumping the revision number here so it # doesn't show as inconsistent to the maintenance periodic task + context = payload.context + security_group = payload.latest_state ovn_revision_numbers_db.bump_revision( - kwargs['context'], security_group, ovn_const.TYPE_SECURITY_GROUPS) + context, security_group, ovn_const.TYPE_SECURITY_GROUPS) def _create_sg_rule_precommit(self, resource, event, trigger, payload): diff --git a/neutron/plugins/ml2/ovo_rpc.py b/neutron/plugins/ml2/ovo_rpc.py index b617d383a0c..e4df381df32 100644 --- a/neutron/plugins/ml2/ovo_rpc.py +++ b/neutron/plugins/ml2/ovo_rpc.py @@ -40,7 +40,8 @@ class _ObjectChangeHandler(object): _PAYLOAD_RESOURCES = (resources.NETWORK, resources.ADDRESS_GROUP, resources.SECURITY_GROUP_RULE, - resources.SUBNET) + resources.SUBNET, + resources.SECURITY_GROUP) def __init__(self, resource, object_class, resource_push_api): self._resource = resource diff --git a/neutron/tests/unit/db/test_securitygroups_db.py b/neutron/tests/unit/db/test_securitygroups_db.py index 0271744f9fd..64518243dc8 100644 --- a/neutron/tests/unit/db/test_securitygroups_db.py +++ b/neutron/tests/unit/db/test_securitygroups_db.py @@ -113,8 +113,8 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): FAKE_SECGROUP) def test_update_security_group_conflict(self): - with mock.patch.object(registry, "notify") as mock_notify: - mock_notify.side_effect = exceptions.CallbackFailure(Exception()) + with mock.patch.object(registry, "publish") as mock_publish: + mock_publish.side_effect = exceptions.CallbackFailure(Exception()) secgroup = {'security_group': FAKE_SECGROUP} with testtools.ExpectedException( securitygroup.SecurityGroupConflict): @@ -301,33 +301,48 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): DEFAULT_SECGROUP_DICT.update({ 'revision_number': mock.ANY, }) - with mock.patch.object(registry, 'publish') as publish, \ - mock.patch.object(registry, "notify") as mock_notify: + with mock.patch.object(registry, 'publish') as publish: sg_dict = 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=True, - security_group=DEFAULT_SECGROUP_DICT), - mock.call('security_group', 'after_create', mock.ANY, - context=mock.ANY, is_default=True, - security_group=DEFAULT_SECGROUP_DICT), - mock.call('security_group', 'precommit_create', mock.ANY, - context=mock.ANY, is_default=False, - security_group=sg_dict), - mock.call('security_group', 'after_create', mock.ANY, - context=mock.ANY, is_default=False, - security_group=sg_dict)]) publish.assert_has_calls([ - mock.call('security_group', 'before_create', mock.ANY, + mock.call(resources.SECURITY_GROUP, 'before_create', mock.ANY, payload=mock.ANY), - mock.call('security_group', 'before_create', mock.ANY, + mock.call(resources.SECURITY_GROUP, 'before_create', mock.ANY, + payload=mock.ANY), + mock.call(resources.SECURITY_GROUP, 'precommit_create', + mock.ANY, payload=mock.ANY), + mock.call(resources.SECURITY_GROUP, 'after_create', mock.ANY, + payload=mock.ANY), + mock.call(resources.SECURITY_GROUP, 'precommit_create', + mock.ANY, payload=mock.ANY), + mock.call(resources.SECURITY_GROUP, 'after_create', mock.ANY, payload=mock.ANY)]) + payload = publish.mock_calls[0][2]['payload'] self.assertDictEqual(payload.desired_state, FAKE_SECGROUP['security_group']) + payload = publish.mock_calls[1][2]['payload'] - self.assertDictEqual(payload.desired_state, DEFAULT_SECGROUP) + self.assertDictEqual(payload.desired_state, + DEFAULT_SECGROUP) + + payload = publish.mock_calls[2][2]['payload'] + self.assertDictEqual(payload.latest_state, + DEFAULT_SECGROUP_DICT) + self.assertTrue(payload.metadata['is_default']) + + payload = publish.mock_calls[3][2]['payload'] + self.assertDictEqual(payload.latest_state, + DEFAULT_SECGROUP_DICT) + self.assertTrue(payload.metadata['is_default']) + + payload = publish.mock_calls[4][2]['payload'] + self.assertDictEqual(payload.latest_state, sg_dict) + self.assertFalse(payload.metadata['is_default']) + + payload = publish.mock_calls[5][2]['payload'] + self.assertDictEqual(payload.latest_state, sg_dict) + self.assertFalse(payload.metadata['is_default']) # Ensure that the result of create is same as get. # Especially we want to check the revision number here. @@ -350,36 +365,45 @@ class SecurityGroupDbMixinTestCase(testlib_api.SqlTestCase): sg_id = original_sg_dict['id'] with mock.patch.object(self.mixin, '_get_port_security_group_bindings'), \ - mock.patch.object(registry, "publish") as mock_notify: + mock.patch.object(registry, "publish") as mock_publish: fake_secgroup = copy.deepcopy(FAKE_SECGROUP) fake_secgroup['security_group']['name'] = 'updated_fake' fake_secgroup['security_group']['stateful'] = mock.ANY sg_dict = self.mixin.update_security_group( self.ctx, sg_id, fake_secgroup) - mock_notify.assert_has_calls( - [mock.call('security_group', 'precommit_update', mock.ANY, - payload=mock.ANY)]) - payload = mock_notify.call_args[1]['payload'] + mock_publish.assert_has_calls( + [mock.call(resources.SECURITY_GROUP, events.PRECOMMIT_UPDATE, + mock.ANY, payload=mock.ANY)]) + payload = mock_publish.call_args[1]['payload'] self.assertEqual(original_sg_dict, payload.states[0]) self.assertEqual(sg_id, payload.resource_id) - self.assertEqual(sg_dict, payload.desired_state) + self.assertEqual(sg_dict, payload.latest_state) def test_security_group_precommit_and_after_delete_event(self): sg_dict = self.mixin.create_security_group(self.ctx, FAKE_SECGROUP) - with mock.patch.object(registry, "notify") as mock_notify: + with mock.patch.object(registry, "publish") as mock_publish: self.mixin.delete_security_group(self.ctx, sg_dict['id']) sg_dict['security_group_rules'] = mock.ANY - mock_notify.assert_has_calls( + mock_publish.assert_has_calls( [mock.call('security_group', 'precommit_delete', - mock.ANY, context=mock.ANY, security_group=sg_dict, - security_group_id=sg_dict['id'], - security_group_rule_ids=[mock.ANY, mock.ANY]), + mock.ANY, payload=mock.ANY), mock.call('security_group', 'after_delete', - mock.ANY, context=mock.ANY, - security_group_id=sg_dict['id'], - security_group_rule_ids=[mock.ANY, mock.ANY], - name=sg_dict['name'])]) + mock.ANY, + payload=mock.ANY)]) + payload = mock_publish.mock_calls[1][2]['payload'] + self.assertEqual(mock.ANY, payload.context) + self.assertEqual(sg_dict, payload.latest_state) + self.assertEqual(sg_dict['id'], payload.resource_id) + self.assertEqual([mock.ANY, mock.ANY], + payload.metadata.get('security_group_rule_ids')) + + payload = mock_publish.mock_calls[2][2]['payload'] + self.assertEqual(mock.ANY, payload.context) + self.assertEqual(sg_dict, payload.latest_state) + self.assertEqual(sg_dict['id'], payload.resource_id) + self.assertEqual([mock.ANY, mock.ANY], + payload.metadata.get('security_group_rule_ids')) def test_security_group_rule_precommit_create_event_fail(self): registry.subscribe(fake_callback, resources.SECURITY_GROUP_RULE, diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index d95bf282df4..7fdb5f47f68 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -206,7 +206,8 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): return_value=stateless_supported): self.mech_driver._create_security_group( resources.SECURITY_GROUP, events.AFTER_CREATE, {}, - security_group=self.fake_sg, context=self.context) + payload=events.DBEventPayload( + self.context, states=(self.fake_sg,))) external_ids = {ovn_const.OVN_SG_EXT_ID_KEY: self.fake_sg['id']} pg_name = ovn_utils.ovn_port_group_name(self.fake_sg['id']) @@ -239,7 +240,10 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): def test__delete_security_group(self, mock_del_rev): self.mech_driver._delete_security_group( resources.SECURITY_GROUP, events.AFTER_CREATE, {}, - security_group_id=self.fake_sg['id'], context=self.context) + payload=events.DBEventPayload( + self.context, states=(self.fake_sg,), + resource_id=self.fake_sg['id'])) + pg_name = ovn_utils.ovn_port_group_name(self.fake_sg['id']) self.nb_ovn.pg_del.assert_called_once_with( diff --git a/neutron/tests/unit/plugins/ml2/test_security_group.py b/neutron/tests/unit/plugins/ml2/test_security_group.py index 7d937c431c5..e845ee7e574 100644 --- a/neutron/tests/unit/plugins/ml2/test_security_group.py +++ b/neutron/tests/unit/plugins/ml2/test_security_group.py @@ -149,8 +149,8 @@ class TestMl2SecurityGroups(Ml2SecurityGroupsTestCase, or_mock.assert_called_once_with(mock.ANY) def test_security_groups_created_outside_transaction(self): - def record_after_state(r, e, t, context, *args, **kwargs): - self.was_active = context.session.is_active + def record_after_state(r, e, t, payload=None): + self.was_active = payload.context.session.is_active registry.subscribe(record_after_state, resources.SECURITY_GROUP, events.AFTER_CREATE)