From 684c46437dbeb97bb71dde248d39db4dbebcd65e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaime=20Caama=C3=B1o=20Ruiz?= Date: Tue, 23 Jan 2018 19:44:12 +0100 Subject: [PATCH] Fix missing parentid on rule delete journal record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes an error raised when deleting a security group if there are pending rules to be deleted of that group as records in the journal that were not triggered by the group deletion operation itself so that they did not have the parent security group id as data of the record. This happens when the rule and the group are deleted independently but in rapid succession. It does not happen when the rules are deleted as a consequence of deleting the group since the parentid is correctly set in that case, neither when the group is deleted some time after the rules when their records might have already been processed and removed. This problem was encountered in OPNFV functest environment but may also fix [1] and complement [2]. [1] https://jira.opendaylight.org/browse/NETVIRT-896 [2] https://review.openstack.org/#/c/536935 Change-Id: I5ccdbdea9d803334c8ae9416b40e3c4b48a8144b Closes-Bug: 1744966 Signed-off-by: Jaime Caamaño Ruiz (cherry picked from commit 2c15a7926dfd24ee363a15c53ac44dedfae7e0ce) --- networking_odl/ml2/mech_driver_v2.py | 29 +++++--- .../unit/journal/test_dependency_validations.py | 85 ++++++++++++++++++++++ .../tests/unit/ml2/test_mechanism_odl_v2.py | 24 ++++++ 3 files changed, 127 insertions(+), 11 deletions(-) diff --git a/networking_odl/ml2/mech_driver_v2.py b/networking_odl/ml2/mech_driver_v2.py index 7843cb77f..54f3d472f 100644 --- a/networking_odl/ml2/mech_driver_v2.py +++ b/networking_odl/ml2/mech_driver_v2.py @@ -191,20 +191,27 @@ class OpenDaylightMechanismDriver(api.MechanismDriver): object_uuid = (resource_dict.get('id') if operation == 'create' else res_id) - - # NOTE(yamahata): DB auto deletion - # Security Group Rule under this Security Group needs to - # be deleted. At NeutronDB layer rules are auto deleted with - # cascade='all,delete'. - if (object_type == odl_const.ODL_SG and - operation == odl_const.ODL_DELETE): - for rule_id in kwargs['security_group_rule_ids']: - journal.record(context, odl_const.ODL_SG_RULE, - rule_id, odl_const.ODL_DELETE, [object_uuid]) + data = resource_dict + + if (operation == odl_const.ODL_DELETE): + # NOTE(yamahata): DB auto deletion + # Security Group Rule under this Security Group needs to + # be deleted. At NeutronDB layer rules are auto deleted with + # cascade='all,delete'. + if (object_type == odl_const.ODL_SG): + for rule_id in kwargs['security_group_rule_ids']: + journal.record(context, odl_const.ODL_SG_RULE, + rule_id, odl_const.ODL_DELETE, + [object_uuid]) + elif (object_type == odl_const.ODL_SG_RULE): + # Set the parent security group id so that dependencies + # to this security rule deletion can be properly found + # in the journal. + data = [kwargs['security_group_id']] assert object_uuid is not None journal.record(context, object_type, object_uuid, - operation, resource_dict) + operation, data) def sync_from_callback_postcommit(self, context, operation, res_type, res_id, resource_dict, **kwargs): diff --git a/networking_odl/tests/unit/journal/test_dependency_validations.py b/networking_odl/tests/unit/journal/test_dependency_validations.py index 11ac3fa4a..65babb72a 100644 --- a/networking_odl/tests/unit/journal/test_dependency_validations.py +++ b/networking_odl/tests/unit/journal/test_dependency_validations.py @@ -53,6 +53,11 @@ _TRUNK_DATA = {'trunk_id': _TRUNK_ID, 'port_id': _PORT_ID, 'sub_ports': [{'port_id': _SUBPORT_ID}]} _BGPVPN_ID = 'BGPVPN_ID' +_SG_ID = 'SG_ID' +_SG_DATA = {'id': _SG_ID} +_SG_RULE_ID = 'SG_RULE_ID' +_SG_RULE_DATA = {'id': _SG_RULE_ID, + 'security_group_id': _SG_ID} def get_data(res_type, operation): @@ -89,6 +94,12 @@ def get_data(res_type, operation): return [{'id': _BGPVPN_ID, 'networks': networks, 'routers': routers, 'route_distinguishers': ['100:1']}] + elif res_type == const.ODL_SG: + return [_SG_DATA] + elif res_type == const.ODL_SG_RULE: + if operation == const.ODL_DELETE: + return [[_SG_RULE_ID]] + return [_SG_RULE_DATA] return [[]] @@ -164,6 +175,80 @@ class SubnetDependencyValidationsTestCase( ) +def security_rule_fail_security_group_dep(sg_op, sgr_op): + return {'expected': 1, + 'first_type': const.ODL_SG, + 'first_operation': sg_op, + 'first_id': _SG_ID, + 'second_type': const.ODL_SG_RULE, + 'second_operation': sgr_op, + 'second_id': _SG_RULE_ID} + + +def security_rule_succeed_security_group_dep(sg_op, sgr_op): + return {'expected': 0, + 'first_type': const.ODL_SG_RULE, + 'first_operation': sgr_op, + 'first_id': _SG_RULE_ID, + 'second_type': const.ODL_SG, + 'second_operation': sg_op, + 'second_id': _SG_ID} + + +class SecurityRuleDependencyValidationsTestCase( + test_base_db.ODLBaseDbTestCase, BaseDependencyValidationsTestCase): + scenarios = ( + ("security_rule_create_depends_on_older_security_group_create", + security_rule_fail_security_group_dep(const.ODL_CREATE, + const.ODL_CREATE)), + ("security_rule_create_depends_on_older_security_group_update", + security_rule_fail_security_group_dep(const.ODL_UPDATE, + const.ODL_CREATE)), + ("security_rule_create_depends_on_older_security_group_delete", + security_rule_fail_security_group_dep(const.ODL_DELETE, + const.ODL_CREATE)), + ("security_rule_create_doesnt_depend_on_newer_security_group_create", + security_rule_succeed_security_group_dep(const.ODL_CREATE, + const.ODL_CREATE)), + ("security_rule_create_doesnt_depend_on_newer_security_group_update", + security_rule_succeed_security_group_dep(const.ODL_UPDATE, + const.ODL_CREATE)), + ("security_rule_create_doesnt_depend_on_newer_security_group_delete", + security_rule_succeed_security_group_dep(const.ODL_DELETE, + const.ODL_CREATE)), + ("security_rule_update_depends_on_older_security_group_create", + security_rule_fail_security_group_dep(const.ODL_CREATE, + const.ODL_UPDATE)), + ("security_rule_update_depends_on_older_security_group_update", + security_rule_fail_security_group_dep(const.ODL_UPDATE, + const.ODL_UPDATE)), + ("security_rule_update_depends_on_older_security_group_delete", + security_rule_fail_security_group_dep(const.ODL_DELETE, + const.ODL_UPDATE)), + ("security_rule_update_doesnt_depend_on_newer_security_group_create", + security_rule_succeed_security_group_dep(const.ODL_CREATE, + const.ODL_UPDATE)), + ("security_rule_update_doesnt_depend_on_newer_security_group_update", + security_rule_succeed_security_group_dep(const.ODL_UPDATE, + const.ODL_UPDATE)), + ("security_rule_update_doesnt_depend_on_newer_security_group_delete", + security_rule_succeed_security_group_dep(const.ODL_DELETE, + const.ODL_UPDATE)), + ("security_rule_delete_doesnt_depend_on_older_security_group_create", + security_rule_succeed_security_group_dep(const.ODL_CREATE, + const.ODL_DELETE)), + ("security_rule_delete_doesnt_depend_on_older_security_group_update", + security_rule_succeed_security_group_dep(const.ODL_UPDATE, + const.ODL_DELETE)), + ("security_rule_delete_doesnt_depend_on_newer_security_group_create", + security_rule_succeed_security_group_dep(const.ODL_CREATE, + const.ODL_DELETE)), + ("security_rule_delete_doesnt_depend_on_newer_security_group_update", + security_rule_succeed_security_group_dep(const.ODL_UPDATE, + const.ODL_DELETE)), + ) + + def port_fail_network_dep(net_op, port_op): return {'expected': 1, 'first_type': const.ODL_NETWORK, diff --git a/networking_odl/tests/unit/ml2/test_mechanism_odl_v2.py b/networking_odl/tests/unit/ml2/test_mechanism_odl_v2.py index 1efe2efd0..5ff51aaee 100644 --- a/networking_odl/tests/unit/ml2/test_mechanism_odl_v2.py +++ b/networking_odl/tests/unit/ml2/test_mechanism_odl_v2.py @@ -322,6 +322,12 @@ class OpenDaylightMechanismDriverTestCase(base_v2.OpenDaylightConfigBase): security_group={'security_group_rules': {'id': SG_RULE_FAKE_ID}}, security_group_rule_ids=[SG_RULE_FAKE_ID]) + elif (object_type == odl_const.ODL_SG_RULE and + operation == odl_const.ODL_DELETE): + with self.db_session.begin(subtransactions=True): + self.mech.sync_from_callback_precommit( + plugin_context, operation, res_type, res_id, + context_, security_group_id=SG_FAKE_ID) else: with self.db_session.begin(subtransactions=True): self.mech.sync_from_callback_precommit( @@ -624,6 +630,24 @@ class OpenDaylightMechanismDriverTestCase(base_v2.OpenDaylightConfigBase): 'tenant_id': 'test-tenant', 'id': SG_FAKE_ID, 'name': 'test_sg'})]) + def test_sg_rule_delete(self): + with mock.patch.object(journal, 'record') as record: + context = self._get_mock_operation_context(odl_const.ODL_SG_RULE) + res_id = context[odl_const.ODL_SG_RULE]['id'] + rule = mock.Mock() + rule.id = SG_RULE_FAKE_ID + rule.security_group_id = SG_FAKE_ID + kwargs = {'security_group_rule_id': SG_RULE_FAKE_ID, + 'security_group_id': SG_FAKE_ID} + with self.db_session.begin(subtransactions=True): + self.mech.sync_from_callback_precommit( + self.db_context, odl_const.ODL_DELETE, + callback._RESOURCE_MAPPING[odl_const.ODL_SG_RULE], + res_id, context, **kwargs) + record.assert_has_calls( + [mock.call(mock.ANY, 'security_group_rule', + SG_RULE_FAKE_ID, 'delete', [SG_FAKE_ID])]) + def test_sync_multiple_updates(self): # add 2 updates for i in range(2): -- 2.14.3