diff --git a/neutron/agent/linux/interface.py b/neutron/agent/linux/interface.py index 0c838e6accd..c9de1c60757 100644 --- a/neutron/agent/linux/interface.py +++ b/neutron/agent/linux/interface.py @@ -365,7 +365,7 @@ def _add_device_to_namespace(self, ip_wrapper, device, namespace): namespace_obj = ip_wrapper.ensure_namespace(namespace) for i in range(9): try: - namespace_obj.add_device_to_namespace(device) + namespace_obj.add_device_to_namespace(device, is_ovs_port=True) break except ip_lib.NetworkInterfaceNotFound: # NOTE(slaweq): if the exception was NetworkInterfaceNotFound diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 10bd33d9e12..196dc17e218 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -259,7 +259,22 @@ def ensure_namespace(self, name): return ip def namespace_is_empty(self): - return not self.get_devices() + try: + return not self.get_devices() + except OSError as e: + # This can happen if we previously got terminated in the middle of + # removing this namespace. In this case the bind mount of the + # namespace under /var/run/netns will be removed, but the namespace + # file is still there. As the bind mount is gone we can no longer + # access the namespace to validate that it is empty. But since it + # should have already been removed we are sure that the check has + # passed the last time and since the namespace is unuseable that + # can not have changed. + # Future calls to pyroute2 to remove that namespace will clean up + # the leftover file. + if e.errno == errno.EINVAL: + return True + raise e def garbage_collect_namespace(self): """Conditionally destroy the namespace if it is empty.""" @@ -269,9 +284,9 @@ def garbage_collect_namespace(self): return True return False - def add_device_to_namespace(self, device): + def add_device_to_namespace(self, device, is_ovs_port=False): if self.namespace: - device.link.set_netns(self.namespace) + device.link.set_netns(self.namespace, is_ovs_port=is_ovs_port) def add_vlan(self, name, physical_interface, vlan_id): privileged.create_interface(name, @@ -461,10 +476,15 @@ def set_down(self): privileged.set_link_attribute( self.name, self._parent.namespace, state='down') - def set_netns(self, namespace): + def set_netns(self, namespace, is_ovs_port=False): privileged.set_link_attribute( self.name, self._parent.namespace, net_ns_fd=namespace) self._parent.namespace = namespace + if is_ovs_port: + # NOTE(slaweq): because of the "shy port" which may dissapear for + # short time after it's moved to the namespace we need to wait + # a bit before checking if port really exists in the namespace + time.sleep(1) common_utils.wait_until_true(lambda: self.exists, timeout=5, sleep=0.5) diff --git a/neutron/agent/ovn/metadata/agent.py b/neutron/agent/ovn/metadata/agent.py index 1745239701b..861715d8e14 100644 --- a/neutron/agent/ovn/metadata/agent.py +++ b/neutron/agent/ovn/metadata/agent.py @@ -430,7 +430,10 @@ def sync(self, provision=True): ns.startswith(NS_PREFIX) and ns not in metadata_namespaces] for ns in unused_namespaces: - self.teardown_datapath(self._get_datapath_name(ns)) + try: + self.teardown_datapath(self._get_datapath_name(ns)) + except Exception: + LOG.exception('Error unable to destroy namespace: %s', ns) # resync all network namespaces based on the associated datapaths, # even those that are already running. This is to make sure diff --git a/neutron/db/db_base_plugin_v2.py b/neutron/db/db_base_plugin_v2.py index 514aa82c0c6..ddd856e94e3 100644 --- a/neutron/db/db_base_plugin_v2.py +++ b/neutron/db/db_base_plugin_v2.py @@ -643,32 +643,39 @@ def _validate_subnet(self, context, s, cur_subnet=None): "supported if enable_dhcp is True.") raise exc.InvalidInput(error_message=error_message) - if validators.is_attr_set(s.get('gateway_ip')): - self._validate_ip_version(ip_ver, s['gateway_ip'], 'gateway_ip') - if has_cidr: - is_gateway_not_valid = ( - ipam.utils.check_gateway_invalid_in_subnet( - s['cidr'], s['gateway_ip'])) - if is_gateway_not_valid: - error_message = _("Gateway is not valid on subnet") - raise exc.InvalidInput(error_message=error_message) - # Ensure the gateway IP is not assigned to any port - # skip this check in case of create (s parameter won't have id) + gateway_ip = s.get('gateway_ip', constants.ATTR_NOT_SPECIFIED) + if validators.is_attr_set(gateway_ip) or gateway_ip is None: + # Validate the gateway IP, if defined in the request. + if s['gateway_ip']: + self._validate_ip_version(ip_ver, gateway_ip, 'gateway_ip') + if has_cidr: + is_gateway_not_valid = ( + ipam.utils.check_gateway_invalid_in_subnet( + s['cidr'], gateway_ip)) + if is_gateway_not_valid: + error_message = _("Gateway is not valid on subnet") + raise exc.InvalidInput(error_message=error_message) + + # Ensure the current subnet gateway IP is not assigned to any port. + # The subnet gateway IP cannot be modified or removed if in use + # (assigned to a router interface). + # Skip this check in case of create (s parameter won't have id). # NOTE(salv-orlando): There is slight chance of a race, when # a subnet-update and a router-interface-add operation are # executed concurrently - s_gateway_ip = netaddr.IPAddress(s['gateway_ip']) + s_gateway_ip = (netaddr.IPAddress(gateway_ip) if gateway_ip else + None) if (cur_subnet and s_gateway_ip != cur_subnet['gateway_ip'] and not ipv6_utils.is_ipv6_pd_enabled(s)): - gateway_ip = str(cur_subnet['gateway_ip']) + current_gateway_ip = str(cur_subnet['gateway_ip']) alloc = port_obj.IPAllocation.get_alloc_routerports( - context, cur_subnet['id'], gateway_ip=gateway_ip, + context, cur_subnet['id'], gateway_ip=current_gateway_ip, first=True) if alloc and alloc.port_id: raise exc.GatewayIpInUse( - ip_address=gateway_ip, + ip_address=current_gateway_ip, port_id=alloc.port_id) if validators.is_attr_set(s.get('dns_nameservers')): diff --git a/neutron/objects/router.py b/neutron/objects/router.py index 60cf54e2972..0539f806761 100644 --- a/neutron/objects/router.py +++ b/neutron/objects/router.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import itertools - import netaddr from neutron_lib.api.definitions import availability_zone as az_def @@ -409,20 +407,11 @@ def get_scoped_floating_ips(cls, context, router_ids): # Filter out on router_ids query = query.filter(l3.FloatingIP.router_id.in_(router_ids)) - return cls._unique_floatingip_iterator(context, query) - @classmethod - def _unique_floatingip_iterator(cls, context, query): - """Iterates over only one row per floating ip. Ignores others.""" - # Group rows by fip id. They must be sorted by same. - q = query.order_by(l3.FloatingIP.id) - keyfunc = lambda row: row[0]['id'] - group_iterator = itertools.groupby(q, keyfunc) - - # Just hit the first row of each group - for key, value in group_iterator: - # pylint: disable=stop-iteration-return - row = list(next(value)) + # Remove duplicate rows based on FIP IDs + query = query.group_by(l3.FloatingIP.id) + + for row in query: yield (cls._load_object(context, row[0]), row[1]) @classmethod diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py index baf23aa4abb..39d51f3a867 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py @@ -670,6 +670,8 @@ def notify(self, event, row, updates=None): class BaseOvnSbIdl(Ml2OvnIdlBase): @classmethod def from_server(cls, connection_string, helper): + if 'Chassis_Private' in helper.schema_json['tables']: + helper.register_table('Chassis_Private') helper.register_table('Chassis') helper.register_table('Encap') helper.register_table('Port_Binding') diff --git a/neutron/tests/unit/agent/linux/test_interface.py b/neutron/tests/unit/agent/linux/test_interface.py index 0d9a7bebb45..84fe6c5af8c 100644 --- a/neutron/tests/unit/agent/linux/test_interface.py +++ b/neutron/tests/unit/agent/linux/test_interface.py @@ -470,11 +470,11 @@ def device_exists(dev, namespace=None): expected.extend( [mock.call().ensure_namespace(namespace), mock.call().ensure_namespace().add_device_to_namespace( - mock.ANY), + mock.ANY, is_ovs_port=True), mock.call().ensure_namespace().add_device_to_namespace( - mock.ANY), + mock.ANY, is_ovs_port=True), mock.call().ensure_namespace().add_device_to_namespace( - mock.ANY)]) + mock.ANY, is_ovs_port=True)]) expected.extend([ mock.call(namespace=namespace), mock.call().device('tap0'), diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index d1c74fb3f78..ac4cf03225f 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -357,6 +357,21 @@ def test_garbage_collect_namespace_existing_not_empty(self): self.assertNotIn(mock.call().delete('ns'), ip_ns_cmd_cls.mock_calls) + def test_garbage_collect_namespace_existing_broken(self): + with mock.patch.object(ip_lib, 'IpNetnsCommand') as ip_ns_cmd_cls: + ip_ns_cmd_cls.return_value.exists.return_value = True + + ip = ip_lib.IPWrapper(namespace='ns') + + with mock.patch.object(ip, 'get_devices', + side_effect=OSError(errno.EINVAL, None) + ) as mock_get_devices: + self.assertTrue(ip.garbage_collect_namespace()) + + mock_get_devices.assert_called_once_with() + expected = [mock.call().delete('ns')] + ip_ns_cmd_cls.assert_has_calls(expected) + @mock.patch.object(priv_lib, 'create_interface') def test_add_vlan(self, create): retval = ip_lib.IPWrapper().add_vlan('eth0.1', 'eth0', '1') @@ -507,7 +522,8 @@ def fake_create_interface(ifname, namespace, kind, **kwargs): def test_add_device_to_namespace(self): dev = mock.Mock() ip_lib.IPWrapper(namespace='ns').add_device_to_namespace(dev) - dev.assert_has_calls([mock.call.link.set_netns('ns')]) + dev.assert_has_calls( + [mock.call.link.set_netns('ns', is_ovs_port=False)]) def test_add_device_to_namespace_is_none(self): dev = mock.Mock() diff --git a/neutron/tests/unit/agent/ovn/metadata/test_agent.py b/neutron/tests/unit/agent/ovn/metadata/test_agent.py index 6df7da702dc..9bf9f0db52a 100644 --- a/neutron/tests/unit/agent/ovn/metadata/test_agent.py +++ b/neutron/tests/unit/agent/ovn/metadata/test_agent.py @@ -134,6 +134,31 @@ def test_sync_teardown_namespace(self): lnn.assert_called_once_with() tdp.assert_called_once_with('3') + def test_sync_teardown_namespace_does_not_crash_on_error(self): + """Test that sync tears down unneeded metadata namespaces. + Even if that fails it continues to provision other datapaths + """ + with mock.patch.object( + self.agent, 'provision_datapath') as pdp,\ + mock.patch.object( + ip_lib, 'list_network_namespaces', + return_value=['ovnmeta-1', 'ovnmeta-2', 'ovnmeta-3', + 'ns1', 'ns2']) as lnn,\ + mock.patch.object( + self.agent, 'teardown_datapath', + side_effect=Exception()) as tdp: + self.agent.sync() + + pdp.assert_has_calls( + [ + mock.call(p.datapath) + for p in self.ports + ], + any_order=True + ) + lnn.assert_called_once_with() + tdp.assert_called_once_with('3') + def test_get_networks_datapaths(self): """Test get_networks_datapaths returns only datapath objects for the networks containing vif ports of type ''(blank) and 'external'. diff --git a/neutron/tests/unit/db/test_l3_db.py b/neutron/tests/unit/db/test_l3_db.py index 652ad8ae95c..4579fefd34f 100644 --- a/neutron/tests/unit/db/test_l3_db.py +++ b/neutron/tests/unit/db/test_l3_db.py @@ -252,27 +252,6 @@ def test__make_floatingip_dict_with_scope(self, make_fip_dict): 'fixed_ip_address_scope': mock.sentinel.address_scope_id, 'id': mock.sentinel.fip_ip}, result) - def test__unique_floatingip_iterator(self): - context = mock.MagicMock() - query = mock.MagicMock() - query.order_by().__iter__.return_value = [ - ({'id': 'id1'}, 'scope1'), - ({'id': 'id1'}, 'scope1'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id3'}, 'scope3')] - query.reset_mock() - with mock.patch.object( - l3_obj.FloatingIP, '_load_object', - side_effect=({'id': 'id1'}, {'id': 'id2'}, {'id': 'id3'})): - result = list( - l3_obj.FloatingIP._unique_floatingip_iterator(context, query)) - query.order_by.assert_called_once_with(l3_models.FloatingIP.id) - self.assertEqual([({'id': 'id1'}, 'scope1'), - ({'id': 'id2'}, 'scope2'), - ({'id': 'id3'}, 'scope3')], result) - @mock.patch.object(directory, 'get_plugin') def test_prevent_l3_port_deletion_port_not_found(self, gp): # port not found doesn't prevent diff --git a/neutron/tests/unit/objects/test_router.py b/neutron/tests/unit/objects/test_router.py index 88714e70113..5c86aec2fcb 100644 --- a/neutron/tests/unit/objects/test_router.py +++ b/neutron/tests/unit/objects/test_router.py @@ -12,8 +12,11 @@ # License for the specific language governing permissions and limitations # under the License. +from itertools import chain from unittest import mock +import netaddr + from neutron_lib.db import api as db_api from oslo_utils import uuidutils @@ -293,6 +296,51 @@ def test_v1_2_to_v1_1_drops_qos_network_policy_id(self): self.assertNotIn('qos_network_policy_id', obj_v1_1['versioned_object.data']) + def test_get_scoped_floating_ips(self): + def compare_results(router_ids, original_fips): + self.assertCountEqual( + original_fips, + [ + fip[0].id + for fip in router.FloatingIP.get_scoped_floating_ips( + self.context, router_ids) + ] + ) + + # Setup three routers, networks and external networks + routers = {} + for i in range(3): + router_id = self._create_test_router_id(name=f'router-{i}') + routers[router_id] = [] + net_id = self._create_test_network_id() + fip_net_id = self._create_external_network_id() + + # Create three subnets and three FIPs using the + # aforementioned networks and routers + for j in range(3): + self._create_test_subnet_id(net_id) + fip = router.FloatingIP( + self.context, + floating_ip_address=netaddr.IPAddress(f'10.{i}.{j}.3'), + floating_network_id=fip_net_id, + floating_port_id=self._create_test_port_id( + network_id=fip_net_id), + fixed_port_id=self._create_test_port_id( + network_id=net_id), + router_id=router_id, + ) + fip.create() + routers[router_id].append(fip.id) + + # For each router we created, fetch the fips and ensure the + # results match what we originally created + for router_id, original_fips in routers.items(): + compare_results([router_id], original_fips) + + # Now try to fetch all the fips for all the routers at once + original_fips = list(chain.from_iterable(routers.values())) + compare_results(routers.keys(), original_fips) + class DvrFipGatewayPortAgentBindingTestCase( obj_test_base.BaseObjectIfaceTestCase): diff --git a/releasenotes/notes/forbid-subnet-gwip-deletion-router-interface-072a18373f920ed9.yaml b/releasenotes/notes/forbid-subnet-gwip-deletion-router-interface-072a18373f920ed9.yaml new file mode 100644 index 00000000000..f830f329520 --- /dev/null +++ b/releasenotes/notes/forbid-subnet-gwip-deletion-router-interface-072a18373f920ed9.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + [`bug 2036423 `_] + Now it is not possible to delete a subnet gateway IP if that subnet has a + router interface; the subnet gateway IP modification was already forbidden.