Merge "Always call ipam driver on subnet update" into stable/liberty

4 files changed, 124 insertions, 21 deletions

diff --git a/neutron/db/ipam_pluggable_backend.py b/neutron/db/ipam_pluggable_backend.py
index 4a47f797d5..6c845e0232 100644
--- a/neutron/db/ipam_pluggable_backend.py
+++ b/neutron/db/ipam_pluggable_backend.py
@@ -13,6 +13,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import copy
+
 import netaddr
 from oslo_db import exception as db_exc
 from oslo_log import log as logging
@@ -137,9 +139,6 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin):
         return allocated
 
     def _ipam_update_allocation_pools(self, context, ipam_driver, subnet):
-        self.validate_allocation_pools(subnet['allocation_pools'],
-                                       subnet['cidr'])
-
         factory = ipam_driver.get_subnet_request_factory()
         subnet_request = factory.get_request(context, subnet, None)
 
@@ -358,22 +357,22 @@ class IpamPluggableBackend(ipam_backend_mixin.IpamBackendMixin):
                                   port['fixed_ips'])
 
     def update_db_subnet(self, context, id, s, old_pools):
+        # 'allocation_pools' is removed from 's' in
+        # _update_subnet_allocation_pools (ipam_backend_mixin),
+        # so create unchanged copy for ipam driver
+        subnet_copy = copy.deepcopy(s)
+        subnet, changes = super(IpamPluggableBackend, self).update_db_subnet(
+            context, id, s, old_pools)
         ipam_driver = driver.Pool.get_instance(None, context)
-        if "allocation_pools" in s:
-            self._ipam_update_allocation_pools(context, ipam_driver, s)
 
-        try:
-            subnet, changes = super(IpamPluggableBackend,
-                                    self).update_db_subnet(context, id,
-                                                           s, old_pools)
-        except Exception:
-            with excutils.save_and_reraise_exception():
-                if "allocation_pools" in s and old_pools:
-                    LOG.error(
-                        _LE("An exception occurred during subnet update."
-                            "Reverting allocation pool changes"))
-                    s['allocation_pools'] = old_pools
-                    self._ipam_update_allocation_pools(context, ipam_driver, s)
+        # Set old allocation pools if no new pools are provided by user.
+        # Passing old pools allows to call ipam driver on each subnet update
+        # even if allocation pools are not changed. So custom ipam drivers
+        # are able to track other fields changes on subnet update.
+        if 'allocation_pools' not in subnet_copy:
+            subnet_copy['allocation_pools'] = old_pools
+        self._ipam_update_allocation_pools(context, ipam_driver, subnet_copy)
+
         return subnet, changes
 
     def add_auto_addrs_on_network_ports(self, context, subnet, ipam_subnet):
diff --git a/neutron/ipam/drivers/neutrondb_ipam/driver.py b/neutron/ipam/drivers/neutrondb_ipam/driver.py
index 31d30c460e..92f53bb8a9 100644
--- a/neutron/ipam/drivers/neutrondb_ipam/driver.py
+++ b/neutron/ipam/drivers/neutrondb_ipam/driver.py
@@ -351,11 +351,20 @@ class NeutronDbSubnet(ipam_base.Subnet):
                 subnet_id=self.subnet_manager.neutron_id,
                 ip_address=address)
 
+    def _no_pool_changes(self, session, pools):
+        """Check if pool updates in db are required."""
+        db_pools = self.subnet_manager.list_pools(session)
+        iprange_pools = [netaddr.IPRange(pool.first_ip, pool.last_ip)
+                         for pool in db_pools]
+        return pools == iprange_pools
+
     def update_allocation_pools(self, pools, cidr):
         # Pools have already been validated in the subnet request object which
         # was sent to the subnet pool driver. Further validation should not be
         # required.
         session = self._context.session
+        if self._no_pool_changes(session, pools):
+            return
         self.subnet_manager.delete_allocation_pools(session)
         self.create_allocation_pools(self.subnet_manager, session, pools, cidr)
         self._pools = pools
diff --git a/neutron/tests/unit/db/test_ipam_pluggable_backend.py b/neutron/tests/unit/db/test_ipam_pluggable_backend.py
index 4d17c2e776..a6aa505576 100644
--- a/neutron/tests/unit/db/test_ipam_pluggable_backend.py
+++ b/neutron/tests/unit/db/test_ipam_pluggable_backend.py
@@ -63,9 +63,11 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase):
         self.tenant_id = uuidutils.generate_uuid()
         self.subnet_id = uuidutils.generate_uuid()
 
-    def _prepare_mocks(self, address_factory=None):
+    def _prepare_mocks(self, address_factory=None, subnet_factory=None):
         if address_factory is None:
             address_factory = ipam_req.AddressRequestFactory
+        if subnet_factory is None:
+            subnet_factory = ipam_req.SubnetRequestFactory
 
         mocks = {
             'driver': mock.Mock(),
@@ -80,7 +82,7 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase):
         mocks['driver'].get_subnet.return_value = mocks['subnet']
         mocks['driver'].allocate_subnet.return_value = mocks['subnet']
         mocks['driver'].get_subnet_request_factory.return_value = (
-            ipam_req.SubnetRequestFactory)
+            subnet_factory)
         mocks['driver'].get_address_request_factory.return_value = (
             address_factory)
         mocks['subnet'].get_details.return_value = mocks['subnet_request']
@@ -91,8 +93,10 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase):
         mocks['ipam'] = ipam_pluggable_backend.IpamPluggableBackend()
         return mocks
 
-    def _prepare_mocks_with_pool_mock(self, pool_mock, address_factory=None):
-        mocks = self._prepare_mocks(address_factory=address_factory)
+    def _prepare_mocks_with_pool_mock(self, pool_mock, address_factory=None,
+                                      subnet_factory=None):
+        mocks = self._prepare_mocks(address_factory=address_factory,
+                                    subnet_factory=subnet_factory)
         pool_mock.get_instance.return_value = mocks['driver']
         return mocks
 
@@ -583,3 +587,52 @@ class TestDbBasePluginIpam(test_db_base.NeutronDbPluginV2TestCase):
                                                             ip_dict)
         # Verify incoming port dict is not changed ('id' is not added to it)
         self.assertIsNone(port_dict['port'].get('id'))
+
+    def _test_update_db_subnet(self, pool_mock, subnet, expected_subnet,
+                               old_pools):
+        subnet_factory = mock.Mock()
+        context = mock.Mock()
+
+        mocks = self._prepare_mocks_with_pool_mock(
+            pool_mock, subnet_factory=subnet_factory)
+
+        mocks['ipam'] = ipam_pluggable_backend.IpamPluggableBackend()
+        mocks['ipam'].update_db_subnet(context, id, subnet, old_pools)
+
+        mocks['driver'].get_subnet_request_factory.assert_called_once_with()
+        subnet_factory.get_request.assert_called_once_with(context,
+                                                           expected_subnet,
+                                                           None)
+
+    @mock.patch('neutron.ipam.driver.Pool')
+    def test_update_db_subnet_unchanged_pools(self, pool_mock):
+        old_pools = [netaddr.IPRange('192.1.1.2', '192.1.1.254')]
+        subnet = {'id': uuidutils.generate_uuid(),
+                  'network_id': uuidutils.generate_uuid(),
+                  'cidr': '192.1.1.0/24',
+                  'ipv6_address_mode': None,
+                  'ipv6_ra_mode': None}
+        subnet_with_pools = subnet.copy()
+        subnet_with_pools['allocation_pools'] = old_pools
+        # if subnet has no allocation pools set, then old pools has to
+        # be added to subnet dict passed to request factory
+        self._test_update_db_subnet(pool_mock, subnet, subnet_with_pools,
+                                    old_pools)
+
+    @mock.patch('neutron.ipam.driver.Pool')
+    def test_update_db_subnet_new_pools(self, pool_mock):
+        old_pools = [netaddr.IPRange('192.1.1.2', '192.1.1.254')]
+        subnet = {'id': uuidutils.generate_uuid(),
+                  'network_id': uuidutils.generate_uuid(),
+                  'cidr': '192.1.1.0/24',
+                  'allocation_pools': [
+                      netaddr.IPRange('192.1.1.10', '192.1.1.254')],
+                  'ipv6_address_mode': None,
+                  'ipv6_ra_mode': None}
+        # make a copy of subnet for validation, since update_subnet changes
+        # incoming subnet dict
+        expected_subnet = subnet.copy()
+        # validate that subnet passed to request factory is the same as
+        # incoming one, i.e. new pools in it are not overwritten by old pools
+        self._test_update_db_subnet(pool_mock, subnet, expected_subnet,
+                                    old_pools)
diff --git a/neutron/tests/unit/ipam/drivers/neutrondb_ipam/test_driver.py b/neutron/tests/unit/ipam/drivers/neutrondb_ipam/test_driver.py
index 5a3f6d6e9c..f4889387ea 100644
--- a/neutron/tests/unit/ipam/drivers/neutrondb_ipam/test_driver.py
+++ b/neutron/tests/unit/ipam/drivers/neutrondb_ipam/test_driver.py
@@ -13,12 +13,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import mock
 import netaddr
 
 from neutron.api.v2 import attributes
 from neutron.common import constants
 from neutron.common import exceptions as n_exc
 from neutron import context
+from neutron.ipam.drivers.neutrondb_ipam import db_models
 from neutron.ipam.drivers.neutrondb_ipam import driver
 from neutron.ipam import exceptions as ipam_exc
 from neutron.ipam import requests as ipam_req
@@ -444,3 +446,43 @@ class TestNeutronDbIpamSubnet(testlib_api.SqlTestCase,
         subnet_req = ipam_req.SpecificSubnetRequest(
             'tenant_id', 'meh', '192.168.0.0/24')
         self.ipam_pool.allocate_subnet(subnet_req)
+
+    def test_update_allocation_pools_with_no_pool_change(self):
+        cidr = '10.0.0.0/24'
+        ipam_subnet = self._create_and_allocate_ipam_subnet(
+            cidr)[0]
+        ipam_subnet.subnet_manager.delete_allocation_pools = mock.Mock()
+        ipam_subnet.create_allocation_pools = mock.Mock()
+        alloc_pools = [netaddr.IPRange('10.0.0.2', '10.0.0.254')]
+        # Make sure allocation pools recreation does not happen in case of
+        # unchanged allocation pools
+        ipam_subnet.update_allocation_pools(alloc_pools, cidr)
+        self.assertFalse(
+            ipam_subnet.subnet_manager.delete_allocation_pools.called)
+        self.assertFalse(ipam_subnet.create_allocation_pools.called)
+
+    def _test__no_pool_changes(self, new_pools):
+        id = 'some-id'
+        ipam_subnet = driver.NeutronDbSubnet(id, self.ctx)
+        pools = [db_models.IpamAllocationPool(ipam_subnet_id=id,
+                                              first_ip='192.168.10.20',
+                                              last_ip='192.168.10.41'),
+                 db_models.IpamAllocationPool(ipam_subnet_id=id,
+                                              first_ip='192.168.10.50',
+                                              last_ip='192.168.10.60')]
+
+        ipam_subnet.subnet_manager.list_pools = mock.Mock(return_value=pools)
+        return ipam_subnet._no_pool_changes(self.ctx.session, new_pools)
+
+    def test__no_pool_changes_negative(self):
+        pool_list = [[netaddr.IPRange('192.168.10.2', '192.168.10.254')],
+                     [netaddr.IPRange('192.168.10.20', '192.168.10.41')],
+                     [netaddr.IPRange('192.168.10.20', '192.168.10.41'),
+                      netaddr.IPRange('192.168.10.51', '192.168.10.60')]]
+        for pools in pool_list:
+            self.assertFalse(self._test__no_pool_changes(pools))
+
+    def test__no_pool_changes_positive(self):
+        pools = [netaddr.IPRange('192.168.10.20', '192.168.10.41'),
+                 netaddr.IPRange('192.168.10.50', '192.168.10.60')]
+        self.assertTrue(self._test__no_pool_changes(pools))