diff options
| -rw-r--r-- | neutron/agent/l3/dvr_fip_ns.py | 14 | ||||
| -rw-r--r-- | neutron/db/ipam_pluggable_backend.py | 33 | ||||
| -rw-r--r-- | neutron/ipam/drivers/neutrondb_ipam/driver.py | 9 | ||||
| -rw-r--r-- | neutron/plugins/ml2/managers.py | 2 | ||||
| -rw-r--r-- | neutron/plugins/ml2/plugin.py | 77 | ||||
| -rw-r--r-- | neutron/tests/functional/agent/test_l3_agent.py | 64 | ||||
| -rw-r--r-- | neutron/tests/unit/db/test_ipam_pluggable_backend.py | 61 | ||||
| -rw-r--r-- | neutron/tests/unit/db/test_l3_dvr_db.py | 2 | ||||
| -rw-r--r-- | neutron/tests/unit/ipam/drivers/neutrondb_ipam/test_driver.py | 42 | ||||
| -rw-r--r-- | neutron/tests/unit/plugins/ml2/test_plugin.py | 6 | ||||
| -rw-r--r-- | tox.ini | 2 |
11 files changed, 253 insertions, 59 deletions
diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 441005c89d..6557cc267b 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -108,6 +108,18 @@ class FipNamespace(namespaces.Namespace): prefix=FIP_EXT_DEV_PREFIX, mtu=ex_gw_port.get('mtu')) + # Remove stale fg devices + ip_wrapper = ip_lib.IPWrapper(namespace=ns_name) + devices = ip_wrapper.get_devices() + for device in devices: + name = device.name + if name.startswith(FIP_EXT_DEV_PREFIX) and name != interface_name: + ext_net_bridge = self.agent_conf.external_network_bridge + self.driver.unplug(name, + bridge=ext_net_bridge, + namespace=ns_name, + prefix=FIP_EXT_DEV_PREFIX) + ip_cidrs = common_utils.fixed_ip_cidrs(ex_gw_port['fixed_ips']) self.driver.init_l3(interface_name, ip_cidrs, namespace=ns_name, clean_connections=True) @@ -115,8 +127,6 @@ class FipNamespace(namespaces.Namespace): self.update_gateway_port(ex_gw_port) cmd = ['sysctl', '-w', 'net.ipv4.conf.%s.proxy_arp=1' % interface_name] - # TODO(Carl) mlavelle's work has self.ip_wrapper - ip_wrapper = ip_lib.IPWrapper(namespace=ns_name) ip_wrapper.netns.execute(cmd, check_exit_code=False) def create(self): 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/plugins/ml2/managers.py b/neutron/plugins/ml2/managers.py index 3c9e1c2b75..d1e18f06fb 100644 --- a/neutron/plugins/ml2/managers.py +++ b/neutron/plugins/ml2/managers.py @@ -161,7 +161,7 @@ class TypeManager(stevedore.named.NamedExtensionManager): def _extend_network_dict_provider(self, network, segments): if not segments: - LOG.error(_LE("Network %s has no segments"), network['id']) + LOG.debug("Network %s has no segments", network['id']) for attr in provider.ATTRIBUTES: network[attr] = None elif len(segments) > 1: diff --git a/neutron/plugins/ml2/plugin.py b/neutron/plugins/ml2/plugin.py index 56b4ec47a7..01fd1a6c6b 100644 --- a/neutron/plugins/ml2/plugin.py +++ b/neutron/plugins/ml2/plugin.py @@ -879,6 +879,7 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug("Deleting subnet %s", id) session = context.session + deallocated = set() while True: with session.begin(subtransactions=True): record = self._get_subnet(context, id) @@ -897,43 +898,52 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, qry_allocated = ( qry_allocated.filter(models_v2.Port.device_owner. in_(db_base_plugin_v2.AUTO_DELETE_PORT_OWNERS))) - allocated = qry_allocated.all() - # Delete all the IPAllocation that can be auto-deleted - if allocated: - for x in allocated: - session.delete(x) + allocated = set(qry_allocated.all()) LOG.debug("Ports to auto-deallocate: %s", allocated) - # Check if there are more IP allocations, unless - # is_auto_address_subnet is True. In that case the check is - # unnecessary. This additional check not only would be wasteful - # for this class of subnet, but is also error-prone since when - # the isolation level is set to READ COMMITTED allocations made - # concurrently will be returned by this query if not is_auto_addr_subnet: - alloc = self._subnet_check_ip_allocations(context, id) - if alloc: - user_alloc = self._subnet_get_user_allocation( - context, id) - if user_alloc: - LOG.info(_LI("Found port (%(port_id)s, %(ip)s) " - "having IP allocation on subnet " - "%(subnet)s, cannot delete"), - {'ip': user_alloc.ip_address, - 'port_id': user_alloc.port_id, - 'subnet': id}) - raise exc.SubnetInUse(subnet_id=id) - else: - # allocation found and it was DHCP port - # that appeared after autodelete ports were - # removed - need to restart whole operation - raise os_db_exception.RetryRequest( - exc.SubnetInUse(subnet_id=id)) + user_alloc = self._subnet_get_user_allocation( + context, id) + if user_alloc: + LOG.info(_LI("Found port (%(port_id)s, %(ip)s) " + "having IP allocation on subnet " + "%(subnet)s, cannot delete"), + {'ip': user_alloc.ip_address, + 'port_id': user_alloc.port_id, + 'subnet': id}) + raise exc.SubnetInUse(subnet_id=id) db_base_plugin_v2._check_subnet_not_used(context, id) - # If allocated is None, then all the IPAllocation were - # correctly deleted during the previous pass. - if not allocated: + # SLAAC allocations currently can not be removed using + # update_port workflow, and will persist in 'allocated'. + # So for now just make sure update_port is called once for + # them so MechanismDrivers is aware of the change. + # This way SLAAC allocation is deleted by FK on subnet deletion + # TODO(pbondar): rework update_port workflow to allow deletion + # of SLAAC allocation via update_port. + to_deallocate = allocated - deallocated + + # If to_deallocate is blank, then all known IPAllocations + # (except SLAAC allocations) were correctly deleted + # during the previous pass. + # Check if there are more IP allocations, unless + # is_auto_address_subnet is True. If transaction isolation + # level is set to READ COMMITTED allocations made + # concurrently will be returned by this query and transaction + # will be restarted. It works for REPEATABLE READ isolation + # level too because this query is executed only once during + # transaction, and if concurrent allocations are detected + # transaction gets restarted. Executing this query second time + # in transaction would result in not seeing allocations + # committed by concurrent transactions. + if not to_deallocate: + if (not is_auto_addr_subnet and + self._subnet_check_ip_allocations(context, id)): + # allocation found and it was DHCP port + # that appeared after autodelete ports were + # removed - need to restart whole operation + raise os_db_exception.RetryRequest( + exc.SubnetInUse(subnet_id=id)) network = self.get_network(context, subnet['network_id']) mech_context = driver_context.SubnetContext(self, context, subnet, @@ -951,7 +961,8 @@ class Ml2Plugin(db_base_plugin_v2.NeutronDbPluginV2, LOG.debug("Committing transaction") break - for a in allocated: + for a in to_deallocate: + deallocated.add(a) if a.port: # calling update_port() for each allocation to remove the # IP from the port and call the MechanismDrivers diff --git a/neutron/tests/functional/agent/test_l3_agent.py b/neutron/tests/functional/agent/test_l3_agent.py index fbf56385e5..f5b9185e86 100644 --- a/neutron/tests/functional/agent/test_l3_agent.py +++ b/neutron/tests/functional/agent/test_l3_agent.py @@ -32,6 +32,7 @@ import webob.exc from neutron.agent.common import config as agent_config from neutron.agent.common import ovs_lib from neutron.agent.l3 import agent as neutron_l3_agent +from neutron.agent.l3 import dvr_fip_ns from neutron.agent.l3 import dvr_snat_ns from neutron.agent.l3 import namespace_manager from neutron.agent.l3 import namespaces @@ -1135,6 +1136,69 @@ class TestDvrRouter(L3AgentTestFramework): self._assert_dvr_floating_ips(router) self._assert_snat_namespace_does_not_exist(router) + def test_dvr_router_fips_stale_gw_port(self): + self.agent.conf.agent_mode = 'dvr' + + # Create the router with external net + dvr_router_kwargs = {'ip_address': '19.4.4.3', + 'subnet_cidr': '19.4.4.0/24', + 'gateway_ip': '19.4.4.1', + 'gateway_mac': 'ca:fe:de:ab:cd:ef'} + router_info = self.generate_dvr_router_info(**dvr_router_kwargs) + external_gw_port = router_info['gw_port'] + ext_net_id = router_info['_floatingips'][0]['floating_network_id'] + self.mock_plugin_api.get_external_network_id.return_value(ext_net_id) + + # Create the fip namespace up front + stale_fip_ns = dvr_fip_ns.FipNamespace(ext_net_id, + self.agent.conf, + self.agent.driver, + self.agent.use_ipv6) + stale_fip_ns.create() + + # Add a stale fg port to the namespace + fixed_ip = external_gw_port['fixed_ips'][0] + float_subnet = external_gw_port['subnets'][0] + fip_gw_port_ip = str(netaddr.IPAddress(fixed_ip['ip_address']) + 10) + prefixlen = netaddr.IPNetwork(float_subnet['cidr']).prefixlen + stale_agent_gw_port = { + 'subnets': [{'cidr': float_subnet['cidr'], + 'gateway_ip': float_subnet['gateway_ip'], + 'id': fixed_ip['subnet_id']}], + 'network_id': external_gw_port['network_id'], + 'device_owner': l3_constants.DEVICE_OWNER_AGENT_GW, + 'mac_address': 'fa:16:3e:80:8f:89', + 'binding:host_id': self.agent.conf.host, + 'fixed_ips': [{'subnet_id': fixed_ip['subnet_id'], + 'ip_address': fip_gw_port_ip, + 'prefixlen': prefixlen}], + 'id': _uuid(), + 'device_id': _uuid()} + stale_fip_ns.create_gateway_port(stale_agent_gw_port) + + stale_dev_exists = self.device_exists_with_ips_and_mac( + stale_agent_gw_port, + stale_fip_ns.get_ext_device_name, + stale_fip_ns.get_name()) + self.assertTrue(stale_dev_exists) + + # Create the router, this shouldn't allow the duplicate port to stay + router = self.manage_router(self.agent, router_info) + + # Assert the device no longer exists + stale_dev_exists = self.device_exists_with_ips_and_mac( + stale_agent_gw_port, + stale_fip_ns.get_ext_device_name, + stale_fip_ns.get_name()) + self.assertFalse(stale_dev_exists) + + # Validate things are looking good and clean up + self._validate_fips_for_external_network( + router, router.fip_ns.get_name()) + ext_gateway_port = router_info['gw_port'] + self._delete_router(self.agent, router.router_id) + self._assert_fip_namespace_deleted(ext_gateway_port) + def test_dvr_update_floatingip_statuses(self): self.agent.conf.agent_mode = 'dvr' self._test_update_floatingip_statuses(self.generate_dvr_router_info()) 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/db/test_l3_dvr_db.py b/neutron/tests/unit/db/test_l3_dvr_db.py index f9502f43d4..aedc496cf0 100644 --- a/neutron/tests/unit/db/test_l3_dvr_db.py +++ b/neutron/tests/unit/db/test_l3_dvr_db.py @@ -579,7 +579,7 @@ class L3DvrTestCase(test_db_base_plugin_v2.NeutronDbPluginV2TestCase): elif action == 'del': self.mixin.delete_arp_entry_for_dvr_service_port( self.ctx, port) - self.assertTrue(3, l3_notify.del_arp_entry.call_count) + self.assertEqual(3, l3_notify.del_arp_entry.call_count) def test_update_arp_entry_for_dvr_service_port_added(self): action = 'add' 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)) diff --git a/neutron/tests/unit/plugins/ml2/test_plugin.py b/neutron/tests/unit/plugins/ml2/test_plugin.py index 03c0b40936..619855597a 100644 --- a/neutron/tests/unit/plugins/ml2/test_plugin.py +++ b/neutron/tests/unit/plugins/ml2/test_plugin.py @@ -431,6 +431,12 @@ class TestMl2SubnetsV2(test_plugin.TestSubnetsV2, req = self.new_delete_request('subnets', subnet_id) res = req.get_response(self.api) self.assertEqual(204, res.status_int) + # Validate chat check is called twice, i.e. after the + # first check transaction gets restarted. + calls = [mock.call(mock.ANY, subnet_id), + mock.call(mock.ANY, subnet_id)] + plugin._subnet_check_ip_allocations.assert_has_calls = ( + calls) class TestMl2DbOperationBounds(test_plugin.DbOperationBoundMixin, @@ -166,7 +166,7 @@ commands = {[testenv:docs]commands} # E126 continuation line over-indented for hanging indent # E128 continuation line under-indented for visual indent # E129 visually indented line with same indent as next logical line -# E265 block comment should start with ‘# ‘ +# E265 block comment should start with '# ' # H404 multi line docstring should start with a summary # H405 multi line docstring summary not separated with an empty line ignore = E125,E126,E128,E129,E265,H404,H405 |