Merge "[aim] Fix retry issues and logging"

gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/extension_driver.py

@@ -13,16 +13,15 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from aim.api import resource as aim_res
+from aim import exceptions as aim_exc
 from neutron.api import extensions
+from neutron.db import api as db_api
 from neutron import manager as n_manager
 from neutron_lib import exceptions as n_exc
 from oslo_log import log
 from oslo_utils import excutils
 
-from aim.api import resource as aim_res
-from aim import exceptions as aim_exc
-
-from gbpservice._i18n import _LE
 from gbpservice._i18n import _LI
 from gbpservice.neutron import extensions as extensions_pkg
 from gbpservice.neutron.extensions import cisco_apic
@@ -70,9 +69,13 @@ class ApicExtensionDriver(api_plus.ExtensionDriver,
                     cisco_apic.EXTERNAL_NETWORK] = res_dict.pop(
                         cisco_apic.EXTERNAL_NETWORK)
             result.update(res_dict)
-        except Exception:
+        except Exception as e:
             with excutils.save_and_reraise_exception():
-                LOG.exception(_LE("APIC AIM extend_network_dict failed"))
+                if db_api.is_retriable(e):
+                    LOG.debug("APIC AIM extend_network_dict got retriable "
+                              "exception: %s", type(e))
+                else:
+                    LOG.exception("APIC AIM extend_network_dict failed")
 
     def process_create_network(self, plugin_context, data, result):
         if (data.get(cisco_apic.DIST_NAMES) and
@@ -113,9 +116,13 @@ class ApicExtensionDriver(api_plus.ExtensionDriver,
             res_dict = self.get_subnet_extn_db(session, result['id'])
             result[cisco_apic.SNAT_HOST_POOL] = (
                 res_dict.get(cisco_apic.SNAT_HOST_POOL, False))
-        except Exception:
+        except Exception as e:
             with excutils.save_and_reraise_exception():
-                LOG.exception(_LE("APIC AIM extend_subnet_dict failed"))
+                if db_api.is_retriable(e):
+                    LOG.debug("APIC AIM extend_subnet_dict got retriable "
+                              "exception: %s", type(e))
+                else:
+                    LOG.exception("APIC AIM extend_subnet_dict failed")
 
     def process_create_subnet(self, plugin_context, data, result):
         res_dict = {cisco_apic.SNAT_HOST_POOL:
@@ -135,9 +142,13 @@ class ApicExtensionDriver(api_plus.ExtensionDriver,
     def extend_address_scope_dict(self, session, base_model, result):
         try:
             self._md.extend_address_scope_dict(session, base_model, result)
-        except Exception:
+        except Exception as e:
             with excutils.save_and_reraise_exception():
-                LOG.exception(_LE("APIC AIM extend_address_scope_dict failed"))
+                if db_api.is_retriable(e):
+                    LOG.debug("APIC AIM extend_address_scope_dict got "
+                              "retriable exception: %s", type(e))
+                else:
+                    LOG.exception("APIC AIM extend_address_scope_dict failed")
 
     def process_create_address_scope(self, plugin_context, data, result):
         if (data.get(cisco_apic.DIST_NAMES) and

gbpservice/neutron/plugins/ml2plus/drivers/apic_aim/mechanism_driver.py

@@ -1418,7 +1418,7 @@ class ApicMechanismDriver(api_plus.MechanismDriver,
         return self._gbp_driver
 
     def _merge_status(self, aim_ctx, sync_state, resource):
-        status = self.aim.get_status(aim_ctx, resource)
+        status = self.aim.get_status(aim_ctx, resource, create_if_absent=False)
         if not status:
             # REVISIT(rkukura): This should only occur if the AIM
             # resource has not yet been created when

gbpservice/neutron/plugins/ml2plus/patch_neutron.py

@@ -199,9 +199,6 @@ from neutron.plugins.ml2 import models
 from sqlalchemy.orm import exc
 
 
-LOCK_NAME = 'ml2_db'
-
-
 # REVISIT: This method gets decorated in Pike for removal in Queens. So this
 # patching might need to be changed in Pike and removed in Queens.
 def patched_get_locked_port_and_binding(session, port_id):

gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping.py

@@ -1866,7 +1866,8 @@ class AIMMappingDriver(nrd.CommonNeutronBase, aim_rpc.AIMMappingRPCMixin):
         # is the only policy driver configured, and no merging
         # with any previous status is required.
         aim_ctx = aim_context.AimContext(session)
-        aim_status = self.aim.get_status(aim_ctx, aim_resource_obj)
+        aim_status = self.aim.get_status(
+            aim_ctx, aim_resource_obj, create_if_absent=False)
         if not aim_status:
             # REVIST(Sumit)
             return gp_const.STATUS_BUILD

gbpservice/neutron/services/grouppolicy/drivers/cisco/apic/aim_mapping_rpc.py

@@ -14,9 +14,9 @@ from apic_ml2.neutron.db import port_ha_ipaddress_binding as ha_ip_db
 
 from neutron.common import rpc as n_rpc
 from neutron.common import topics
+from neutron.db import api as db_api
 from neutron.plugins.ml2 import rpc as ml2_rpc
 from opflexagent import rpc as o_rpc
-from oslo_db import api as oslo_db_api
 from oslo_log import log
 
 from gbpservice._i18n import _LE
@@ -25,11 +25,6 @@ from gbpservice.neutron.services.grouppolicy.drivers.cisco.apic import (
     nova_client as nclient)
 
 LOG = log.getLogger(__name__)
-MAX_RETRIES = 5
-
-db_exc_retry = oslo_db_api.wrap_db_retry(max_retries=MAX_RETRIES,
-                                         retry_on_request=True,
-                                         retry_on_deadlock=True)
 
 
 class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin):
@@ -54,7 +49,7 @@ class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin):
             self.opflex_topic, self.opflex_endpoints, fanout=False)
         self.opflex_conn.consume_in_threads()
 
-    @db_exc_retry
+    @db_api.retry_db_errors
     def _retrieve_vrf_details(self, context, **kwargs):
         with context.session.begin(subtransactions=True):
             details = {'l3_policy_id': kwargs['vrf_id']}
@@ -81,7 +76,7 @@ class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin):
         return self._get_vrf_details(context, **kwargs)
 
     def get_gbp_details(self, context, **kwargs):
-        LOG.debug("APIC AIM MD handling get_gbp_details for: %s", kwargs)
+        LOG.debug("APIC AIM handling get_gbp_details for: %s", kwargs)
         try:
             return self._get_gbp_details(context, kwargs, kwargs.get('host'))
         except Exception as e:
@@ -130,7 +125,7 @@ class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin):
     # - self._is_dhcp_optimized(context, port);
     # - self._is_metadata_optimized(context, port);
     # - self._set_dhcp_lease_time(details)
-    @db_exc_retry
+    @db_api.retry_db_errors
     def _get_gbp_details(self, context, request, host):
         with context.session.begin(subtransactions=True):
             device = request.get('device')
@@ -200,7 +195,7 @@ class AIMMappingRPCMixin(ha_ip_db.HAIPOwnerDbMixin):
             self._set_dhcp_lease_time(details)
             details.pop('_cache', None)
 
-        LOG.debug("Details for port %s : %s", (port['id'], details))
+        LOG.debug("Details for port %s : %s", port['id'], details)
         return details
 
     def _get_owned_addresses(self, plugin_context, port_id):

gbpservice/neutron/tests/unit/plugins/ml2plus/test_apic_aim.py

@@ -2334,13 +2334,13 @@ class TestAimMapping(ApicAimTestCase):
 
 class TestSyncState(ApicAimTestCase):
     @staticmethod
-    def _get_synced_status(self, context, resource):
+    def _get_synced_status(self, context, resource, create_if_absent=True):
         status = aim_status.AciStatus.SYNCED
         return aim_status.AciStatus(resource_root=resource.root,
                                     sync_status=status)
 
     @staticmethod
-    def _get_pending_status_for_type(resource, type):
+    def _get_pending_status_for_type(resource, type, create_if_absent=True):
         status = (isinstance(resource, type) and
                   aim_status.AciStatus.SYNC_PENDING or
                   aim_status.AciStatus.SYNCED)
@@ -2348,7 +2348,7 @@ class TestSyncState(ApicAimTestCase):
                                     sync_status=status)
 
     @staticmethod
-    def _get_failed_status_for_type(resource, type):
+    def _get_failed_status_for_type(resource, type, create_if_absent=True):
         status = (isinstance(resource, type) and
                   aim_status.AciStatus.SYNC_FAILED or
                   aim_status.AciStatus.SYNC_PENDING)
@@ -2368,7 +2368,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('synced')
 
     def test_network_bd_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.BridgeDomain)
 
@@ -2376,7 +2376,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('build')
 
     def test_network_bd_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.BridgeDomain)
 
@@ -2384,7 +2384,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('error')
 
     def test_network_epg_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.EndpointGroup)
 
@@ -2392,7 +2392,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('build')
 
     def test_network_epg_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.EndpointGroup)
 
@@ -2400,7 +2400,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('error')
 
     def test_network_vrf_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2408,7 +2408,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_network('build')
 
     def test_network_vrf_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2429,7 +2429,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_address_scope('synced')
 
     def test_address_scope_vrf_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2437,7 +2437,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_address_scope('build')
 
     def test_address_scope_vrf_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2458,7 +2458,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router('synced')
 
     def test_router_contract_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.Contract)
 
@@ -2466,7 +2466,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router('build')
 
     def test_router_contract_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.Contract)
 
@@ -2474,7 +2474,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router('error')
 
     def test_router_subject_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.ContractSubject)
 
@@ -2482,7 +2482,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router('build')
 
     def test_router_subject_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.ContractSubject)
 
@@ -2508,7 +2508,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router_interface_vrf('synced')
 
     def test_router_interface_vrf_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2516,7 +2516,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router_interface_vrf('build')
 
     def test_router_interface_vrf_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.VRF)
 
@@ -2546,7 +2546,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router_interface_subnet('synced')
 
     def test_router_interface_subnet_build(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_pending_status_for_type(
                 resource, aim_resource.Subnet)
 
@@ -2554,7 +2554,7 @@ class TestSyncState(ApicAimTestCase):
             self._test_router_interface_subnet('build')
 
     def test_router_interface_subnet_error(self):
-        def get_status(self, context, resource):
+        def get_status(self, context, resource, create_if_absent=True):
             return TestSyncState._get_failed_status_for_type(
                 resource, aim_resource.Subnet)
 
@@ -2582,7 +2582,7 @@ class TestSyncState(ApicAimTestCase):
                           aim_resource.EndpointGroup,
                           aim_resource.BridgeDomain,
                           aim_resource.VRF]:
-                def get_status(self, context, resource):
+                def get_status(self, context, resource, create_if_absent=True):
                     return status_func(resource, a_res)
                 with mock.patch('aim.aim_manager.AimManager.get_status',
                                 get_status):
@@ -2610,7 +2610,7 @@ class TestSyncState(ApicAimTestCase):
         for expected_status, status_func in [
                 ('build', TestSyncState._get_pending_status_for_type),
                 ('error', TestSyncState._get_failed_status_for_type)]:
-            def get_status(self, context, resource):
+            def get_status(self, context, resource, create_if_absent=True):
                 return status_func(resource, aim_resource.Subnet)
             with mock.patch('aim.aim_manager.AimManager.get_status',
                             get_status):

gbpservice/neutron/tests/unit/services/grouppolicy/test_aim_mapping_driver.py

@@ -329,7 +329,8 @@ class AIMBaseTestCase(test_nr_base.CommonNeutronBaseTestCase,
         # different status states are correctly reflected in the L2P.
         AIM_STATUS = aim_status.AciStatus.SYNC_PENDING
 
-        def mock_get_aim_status(aim_context, aim_resource):
+        def mock_get_aim_status(aim_context, aim_resource,
+                                create_if_absent=True):
             astatus = aim_status.AciStatus(resource_root=aim_resource.root)
             astatus.sync_status = AIM_STATUS
             return astatus
@@ -896,7 +897,8 @@ class TestAIMStatus(AIMBaseTestCase):
 
     def test_status_merging(self):
 
-        def mock_get_aim_status(aim_context, aim_resource):
+        def mock_get_aim_status(aim_context, aim_resource,
+                                create_if_absent=True):
             astatus = aim_status.AciStatus(resource_root=aim_resource.root)
             if aim_resource.status == '':
                 return

test-requirements.txt

@@ -42,8 +42,7 @@ pyOpenSSL>=0.13.0,<=0.15.1
 python-heatclient
 python-keystoneclient
 
-# REVISIT: The following is being pinned to latest known working version to avoid the
-# GBP branches from breaking on accont of the churn in that repository. This is
-# a short term solution and should be reverted to the relevant branch name once
-# co-gating is enabled for the aci-integration-module repo.
--e git+https://github.com/noironetworks/aci-integration-module.git@f3002603a1e772a5e31331d847b33d74a9a69a75#egg=aci-integration-module
+# REVISIT: Until co-gating and/or stable branches are implemented for the
+# aci-integration-module repo, it may be necessary to pin to a working
+# commit.
+-e git+https://github.com/noironetworks/aci-integration-module.git@master#egg=aci-integration-module