Refactor for create_floatingip call

The current implementation for create_floatingip results in the
L3 API being called twice. The first pass causes additional attributes
to be populated, which are used in the second call. This patch converts
this to a single call, which gets the parameters needed and continues,
rather than call itself recursively.

Change-Id: Ie9a3a57d2186fd693e9bc1e03501f0038e316b41
Closes-Bug: #1561638
(cherry picked from commit 256eac720f)
(cherry picked from commit 5809617fd3)

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

@@ -1374,24 +1374,20 @@ class ApicMappingDriver(api.ResourceMappingDriver,
         except AttributeError:
             pass
 
-    def create_floatingip_in_nat_pool(self, context, tenant_id, floatingip):
-        """Create floating-ip in NAT pool associated with external-network"""
+    def nat_pool_iterator(self, context, tenant_id, floatingip):
+        """Get NAT pool for floating IP associated with external-network."""
         fip = floatingip['floatingip']
         f_net_id = fip['floating_network_id']
         subnets = self._get_subnets(context.elevated(),
             {'network_id': [f_net_id]})
         ext_seg = self.gbp_plugin.get_external_segments(context.elevated(),
             {'subnet_id': [s['id'] for s in subnets]}) if subnets else []
-        if not ext_seg:
-            return None
         context._plugin = self.gbp_plugin
         context._plugin_context = context
         for es in ext_seg:
-            fip_id = self._allocate_floating_ip_in_ext_seg(context,
-                tenant_id, es, f_net_id, fip.get('port_id'))
-            if fip_id:
-                return fip_id
-        raise n_exc.IpAddressGenerationFailure(net_id=f_net_id)
+            for nat_pool in self._gen_nat_pool_in_ext_seg(context,
+                    tenant_id, es):
+                yield nat_pool
 
     def _apply_policy_rule_set_rules(
             self, context, policy_rule_set, policy_rules, transaction=None):

gbpservice/neutron/services/grouppolicy/drivers/resource_mapping.py

@@ -296,47 +296,36 @@ class ResourceMappingDriver(api.PolicyDriver, local_api.LocalAPI,
             ext_sub = self._get_subnet(context._plugin_context,
                                        es['subnet_id'])
             ext_net_id = ext_sub['network_id']
-            fip_id = self._allocate_floating_ip_in_ext_seg(
-                context, tenant_id, es, ext_net_id, fixed_port,
-                router_id=l3p.get('routers', [None])[0])
+            fip_id = None
+            for nat_pool in self._gen_nat_pool_in_ext_seg(
+                context, tenant_id, es):
+                try:
+                    fip_id = self._create_floatingip(
+                        context._plugin_context, tenant_id, ext_net_id,
+                        fixed_port, subnet_id=nat_pool['subnet_id'],
+                        router_id=l3p.get('routers', [None])[0])
+                    # FIP allocated, no need to try further allocation
+                    break
+                except n_exc.IpAddressGenerationFailure as ex:
+                    LOG.warning(_("Floating allocation failed: %s"),
+                                ex.message)
             if fip_id:
                 fip_ids.append(fip_id)
         return fip_ids
 
-    def _allocate_floating_ip_in_ext_seg(self, context, tenant_id,
-                                         es, ext_net_id, fixed_port,
-                                         router_id=None):
+    def _gen_nat_pool_in_ext_seg(self, context, tenant_id, es):
         nat_pools = context._plugin.get_nat_pools(
             context._plugin_context.elevated(), {'id': es['nat_pools']})
         no_subnet_pools = []
-        fip_id = None
         for nat_pool in nat_pools:
             # For backward compatibility
             if not nat_pool['subnet_id']:
                 no_subnet_pools.append(nat_pool)
             else:
-                try:
-                    fip_id = self._create_floatingip(
-                        context._plugin_context, tenant_id, ext_net_id,
-                        fixed_port, subnet_id=nat_pool['subnet_id'],
-                        router_id=router_id)
-                    # FIP allocated, empty the no subnet pools to avoid
-                    # further allocation
-                    no_subnet_pools = []
-                    break
-                except n_exc.IpAddressGenerationFailure as ex:
-                    LOG.warn(_("Floating allocation failed: %s"),
-                             ex.message)
+                yield nat_pool
         for nat_pool in no_subnet_pools:
             # Use old allocation method
-            try:
-                fip_id = self._create_floatingip(
-                    context._plugin_context, tenant_id, ext_net_id, fixed_port)
-                break
-            except n_exc.IpAddressGenerationFailure as ex:
-                LOG.warn(_("Floating allocation failed: %s"),
-                         ex.message)
-        return fip_id
+            yield nat_pool
 
     @log.log
     def update_policy_target_precommit(self, context):

gbpservice/neutron/services/l3_router/l3_apic.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 from neutron.common import constants as q_const
+from neutron.common import exceptions as n_exc
 from neutron import context as n_ctx
 from neutron.db import common_db_mixin
 from neutron.db import extraroute_db
@@ -21,6 +22,9 @@ from neutron.db import l3_gwmode_db
 from neutron.extensions import l3
 from neutron import manager
 from neutron.plugins.common import constants
+from oslo_log import log as logging
+
+LOG = logging.getLogger(__name__)
 
 
 class ApicGBPL3ServicePlugin(common_db_mixin.CommonDbMixin,
@@ -101,9 +105,19 @@ class ApicGBPL3ServicePlugin(common_db_mixin.CommonDbMixin,
         fip = floatingip['floatingip']
         if self.apic_gbp and not fip.get('subnet_id'):
             tenant_id = self._get_tenant_id_for_create(context, fip)
-            fip_id = self.apic_gbp.create_floatingip_in_nat_pool(context,
-                tenant_id, floatingip)
-            res = self.get_floatingip(context, fip_id) if fip_id else None
+            for nat_pool in self.apic_gbp.nat_pool_iterator(context,
+                    tenant_id, floatingip):
+                if not nat_pool:
+                    continue
+                fip['subnet_id'] = nat_pool['subnet_id']
+                try:
+                    res = super(ApicGBPL3ServicePlugin,
+                                self).create_floatingip(context, floatingip)
+                except n_exc.IpAddressGenerationFailure as ex:
+                    LOG.warning(_("Floating allocation failed: %s"),
+                                ex.message)
+                if res:
+                    break
         if not res:
             res = super(ApicGBPL3ServicePlugin, self).create_floatingip(
                 context, floatingip)

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

@@ -42,6 +42,7 @@ from gbpservice.neutron.services.grouppolicy import (
 from gbpservice.neutron.services.grouppolicy import config
 from gbpservice.neutron.services.grouppolicy.drivers.cisco.apic import (
     apic_mapping as amap)
+from gbpservice.neutron.services.l3_router import l3_apic
 from gbpservice.neutron.tests.unit.services.grouppolicy import (
     test_resource_mapping as test_rmd)
 
@@ -124,6 +125,7 @@ class ApicMappingTestCase(
         plugin.is_agent_down = mock.Mock(return_value=False)
         self.driver = manager.NeutronManager.get_service_plugins()[
             'GROUP_POLICY'].policy_driver_manager.policy_drivers['apic'].obj
+        self.l3plugin = l3_apic.ApicGBPL3ServicePlugin()
         amap.ApicMappingDriver.get_base_synchronizer = mock.Mock()
         self.driver.name_mapper.name_mapper = mock.Mock()
         self.driver.name_mapper.name_mapper.tenant = echo
@@ -4117,12 +4119,11 @@ class TestNatPool(ApicMappingTestCase):
             self.api)['subnet']
 
         fip_dict = {'floating_network_id': subnet['network_id']}
-        fip_id = self.driver.create_floatingip_in_nat_pool(
+        fip = self.l3plugin.create_floatingip(
             context.get_admin_context(),
-            es['tenant_id'], {'floatingip': fip_dict})
-        self.assertIsNotNone(fip_id)
-        fip = self._get_object(
-            'floatingips', fip_id, self.ext_api)['floatingip']
+            {'floatingip': fip_dict,
+             'tenant_id': es['tenant_id']})
+        self.assertIsNotNone(fip)
         self.assertTrue(
             netaddr.IPAddress(fip['floating_ip_address']) in
             netaddr.IPNetwork('192.168.1.0/24'))