From 684c46437dbeb97bb71dde248d39db4dbebcd65e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jaime=20Caama=C3=B1o=20Ruiz?= <jcaamano@suse.com>
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 <jcaamano@suse.com>
(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