From 2f7ecb95139b75653fdb198c7047c46afc191bf0 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Fri, 11 Aug 2023 17:05:49 -0400 Subject: [PATCH 1/5] Catch non-existent entry failures better in ip_lib The privileged/agent/linux/ip_lib.py code was not always catching "entry does not exist" type errors when deleting entries, and most of the callers were not catching it either, which could lead to random failures. Add code in the IP route, rule and bridge fdb code to catch these errors and not raise on them, other exceptions will still be raised. Also fixed delete_neigh_entry() to not raise when the given namespace does not exist to make it like all the other calls in the file. Added or modified functional tests for above cases. Conflicts: neutron/privileged/agent/linux/ip_lib.py Change-Id: I083649ab1b9a9057ee276a7f3ba069eb667db870 Closes-bug: #2030804 (cherry picked from commit 16875b5f92731a9cf2d7e819d406bfcc442339f3) --- neutron/privileged/agent/linux/ip_lib.py | 25 +++++- .../functional/agent/linux/test_bridge_lib.py | 7 ++ .../privileged/agent/linux/test_ip_lib.py | 78 ++++++++++++------- .../privileged/agent/linux/test_ip_lib.py | 11 +++ 4 files changed, 92 insertions(+), 29 deletions(-) diff --git a/neutron/privileged/agent/linux/ip_lib.py b/neutron/privileged/agent/linux/ip_lib.py index c31fed1cadb..d5d8d6a35f1 100644 --- a/neutron/privileged/agent/linux/ip_lib.py +++ b/neutron/privileged/agent/linux/ip_lib.py @@ -474,6 +474,10 @@ def delete_neigh_entry(ip_version, ip_address, mac_address, device, namespace, if e.code == errno.ENOENT: return raise + except OSError as e: + if e.errno == errno.ENOENT: + raise NetworkNamespaceNotFound(netns_name=namespace) + raise @tenacity.retry( @@ -683,6 +687,11 @@ def delete_ip_rule(namespace, **kwargs): try: with get_iproute(namespace) as ip: ip.rule('del', **kwargs) + except netlink_exceptions.NetlinkError as e: + # trying to delete a non-existent entry shouldn't raise an error + if e.code == errno.ENOENT: + return + raise except OSError as e: if e.errno == errno.ENOENT: raise NetworkNamespaceNotFound(netns_name=namespace) @@ -785,6 +794,11 @@ def delete_ip_route(namespace, cidr, ip_version, device=None, via=None, try: with get_iproute(namespace) as ip: ip.route('del', **kwargs) + except netlink_exceptions.NetlinkError as e: + # trying to delete a non-existent entry shouldn't raise an error + if e.code == errno.ESRCH: + return + raise except OSError as e: if e.errno == errno.ENOENT: raise NetworkNamespaceNotFound(netns_name=namespace) @@ -819,6 +833,11 @@ def _command_bridge_fdb(command, mac, device, dst_ip=None, namespace=None, kwargs['dst'] = dst_ip with get_iproute(namespace) as ip: return make_serializable(ip.fdb(command, **kwargs)) + except netlink_exceptions.NetlinkError as e: + # trying to delete a non-existent entry shouldn't raise an error + if command == 'del' and e.code == errno.ENOENT: + return + raise except OSError as e: if e.errno == errno.ENOENT: raise NetworkNamespaceNotFound(netns_name=namespace) @@ -834,20 +853,20 @@ def add_bridge_fdb(mac, device, dst_ip=None, namespace=None, **kwargs): @privileged.default.entrypoint def append_bridge_fdb(mac, device, dst_ip=None, namespace=None, **kwargs): - """Add a FDB entry""" + """Append a FDB entry""" return _command_bridge_fdb('append', mac, device, dst_ip=dst_ip, namespace=namespace, **kwargs) @privileged.default.entrypoint def replace_bridge_fdb(mac, device, dst_ip=None, namespace=None, **kwargs): - """Add a FDB entry""" + """Replace a FDB entry""" return _command_bridge_fdb('replace', mac, device, dst_ip=dst_ip, namespace=namespace, **kwargs) @privileged.default.entrypoint def delete_bridge_fdb(mac, device, dst_ip=None, namespace=None, **kwargs): - """Add a FDB entry""" + """Delete a FDB entry""" return _command_bridge_fdb('del', mac, device, dst_ip=dst_ip, namespace=namespace, **kwargs) diff --git a/neutron/tests/functional/agent/linux/test_bridge_lib.py b/neutron/tests/functional/agent/linux/test_bridge_lib.py index 5e565e93587..b69496448d4 100644 --- a/neutron/tests/functional/agent/linux/test_bridge_lib.py +++ b/neutron/tests/functional/agent/linux/test_bridge_lib.py @@ -211,6 +211,13 @@ def test_add_delete(self): namespace=self.namespace) self._assert_mac(self.MAC1, self.device, present=False) + try: + # This should not raise for a non-existent entry + bridge_lib.FdbInterface.delete(self.MAC1, self.device, + namespace=self.namespace) + except Exception: + self.fail('Delete FDB entry threw unexpected exception') + def test_add_delete_dst(self): self._assert_mac(self.MAC1, self.device_vxlan, present=False) bridge_lib.FdbInterface.add( diff --git a/neutron/tests/functional/privileged/agent/linux/test_ip_lib.py b/neutron/tests/functional/privileged/agent/linux/test_ip_lib.py index 29764215502..796980eeab0 100644 --- a/neutron/tests/functional/privileged/agent/linux/test_ip_lib.py +++ b/neutron/tests/functional/privileged/agent/linux/test_ip_lib.py @@ -239,12 +239,10 @@ def test_get_devices_info_veth_same_namespaces(self): self.assertEqual(veth1_2['index'], veth1_1_link) -class ListIpRulesTestCase(functional_base.BaseSudoTestCase): - - RULE_TABLES = {'default': 253, 'main': 254, 'local': 255} +class BaseIpRuleTestCase(functional_base.BaseSudoTestCase): def setUp(self): - super(ListIpRulesTestCase, self).setUp() + super().setUp() self.namespace = 'ns_test-' + uuidutils.generate_uuid() self.ns = priv_ip_lib.create_netns(self.namespace) self.addCleanup(self._remove_ns) @@ -252,6 +250,23 @@ def setUp(self): def _remove_ns(self): priv_ip_lib.remove_netns(self.namespace) + def _check_rules(self, rules, parameters, values, exception_string=None, + raise_exception=True): + for rule in rules: + if all(rule.get(parameter) == value + for parameter, value in zip(parameters, values)): + return True + else: + if raise_exception: + self.fail('Rule with %s was expected' % exception_string) + else: + return False + + +class ListIpRulesTestCase(BaseIpRuleTestCase): + + RULE_TABLES = {'default': 253, 'main': 254, 'local': 255} + def test_list_default_rules_ipv4(self): rules_ipv4 = priv_ip_lib.list_ip_rules(self.namespace, 4) self.assertEqual(3, len(rules_ipv4)) @@ -291,28 +306,7 @@ def test_list_rules_ipv6(self): self.fail('Rule added (2001:db8::1/64, table 20) not found') -class RuleTestCase(functional_base.BaseSudoTestCase): - - def setUp(self): - super(RuleTestCase, self).setUp() - self.namespace = 'ns_test-' + uuidutils.generate_uuid() - self.ns = priv_ip_lib.create_netns(self.namespace) - self.addCleanup(self._remove_ns) - - def _remove_ns(self): - priv_ip_lib.remove_netns(self.namespace) - - def _check_rules(self, rules, parameters, values, exception_string=None, - raise_exception=True): - for rule in rules: - if all(rule.get(parameter) == value - for parameter, value in zip(parameters, values)): - return True - else: - if raise_exception: - self.fail('Rule with %s was expected' % exception_string) - else: - return False +class AddIpRulesTestCase(BaseIpRuleTestCase): def test_add_rule_ip(self): ip_addresses = ['192.168.200.250', '2001::250'] @@ -433,6 +427,23 @@ def test_add_rule_exists(self): self.assertEqual(4, len(rules)) +class DeleteIpRulesTestCase(BaseIpRuleTestCase): + + def test_delete_rule_no_entry(self): + iif = 'iif_device' + priv_ip_lib.create_interface(iif, self.namespace, 'dummy') + + try: + # This should not raise for a non-existent entry + priv_ip_lib.delete_ip_rule(self.namespace, iifname=iif) + except Exception: + self.fail('Delete IP rule threw unexpected exception') + + rules = ip_lib.list_ip_rules(self.namespace, 4) + # There are always 3 rules by default + self.assertEqual(3, len(rules)) + + class GetIpAddressesTestCase(functional_base.BaseSudoTestCase): def _remove_ns(self, namespace): @@ -659,6 +670,21 @@ def test_add_multipath_route(self): n_cons.IP_VERSION_4, via=multipath) self._check_routes(['192.168.0.0/24'], gateway=multipath) + def test_delete_route_no_entry(self): + cidr = '192.168.0.0/24' + self.device.addr.add('10.1.0.1/24') + try: + # This should not raise for a non-existent entry + priv_ip_lib.delete_ip_route(self.namespace, cidr, + n_cons.IP_VERSION_4, + device=self.device_name) + except Exception: + self.fail('Delete IP route threw unexpected exception') + + routes = ip_lib.list_ip_routes(self.namespace, n_cons.IP_VERSION_4) + # There will be a single interface route since we added an IP + self.assertEqual(1, len(routes)) + class GetLinkAttributesTestCase(functional_base.BaseSudoTestCase): diff --git a/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py b/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py index 9a2177777e3..85c905dd6a5 100644 --- a/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/privileged/agent/linux/test_ip_lib.py @@ -157,6 +157,17 @@ def test_run_iproute_neigh_namespace_not_exists(self): priv_lib._run_iproute_neigh, "test_cmd", "eth0", None, test_param="test_value") + def test_run_iproute_neigh_no_entry(self): + with mock.patch.object(pyroute2, "IPRoute") as iproute_mock: + iproute_mock.side_effect = netlink_exceptions.NetlinkError( + code=errno.ENOENT) + try: + priv_lib._run_iproute_neigh( + "test_cmd", "eth0", None, test_param="test_value") + self.fail("NetlinkError exception not raised") + except netlink_exceptions.NetlinkError as e: + self.assertEqual(errno.ENOENT, e.code) + def test_run_iproute_neigh_error(self): with mock.patch.object(pyroute2, "IPRoute") as iproute_mock: iproute_mock.side_effect = OSError( From 9776e9e74b8e06d9507e299c546ba06d721f7455 Mon Sep 17 00:00:00 2001 From: Bartosz Bezak Date: Fri, 17 Nov 2023 10:33:32 +0100 Subject: [PATCH 2/5] docs: update default value of metadata workers for ml2/ovn [1] changed that value to 0 [1] https://review.opendev.org/c/openstack/neutron/+/861751 Related-Bug: #1993181 Change-Id: I7009e8a9fa8a61cc796d9592db0cf68e07e5884d (cherry picked from commit 407585f99f82c0136f66f0b4874da58500574c63) --- neutron/conf/agent/metadata/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neutron/conf/agent/metadata/config.py b/neutron/conf/agent/metadata/config.py index fb5675933c0..4ed76afefcf 100644 --- a/neutron/conf/agent/metadata/config.py +++ b/neutron/conf/agent/metadata/config.py @@ -94,7 +94,7 @@ cfg.IntOpt('metadata_workers', sample_default=' / 2', help=_('Number of separate worker processes for metadata ' - 'server (defaults to 2 when used with ML2/OVN and half ' + 'server (defaults to 0 when used with ML2/OVN and half ' 'of the number of CPUs with other backend drivers)')), cfg.IntOpt('metadata_backlog', default=4096, From 481582a7f18db1939c304a857c6d471fee121dd4 Mon Sep 17 00:00:00 2001 From: yatinkarel Date: Wed, 22 Nov 2023 21:03:02 +0530 Subject: [PATCH 3/5] [DHCP agent] Fix route to OVN metadata port for non-isolated networks This was missed in the original fix[1] during 5th patch set. When ovn metadata port exists it's ip should be used as route irrespective of subnet is isolated or not. [1] https://review.opendev.org/c/openstack/neutron/+/886988 Related-Bug: #1982569 Related-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2213862 Change-Id: Icd84685c37fffa20e4fc9c5522f77bc63e2565f2 (cherry picked from commit 56172ed5aef07d278e0f9cd6bcd5164f3f0ffa49) --- neutron/agent/linux/dhcp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 5f6c9c1e508..7132c83fdb5 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -1220,7 +1220,7 @@ def _generate_opts_per_subnet(self): if subnet_dhcp_ip: metadata_route_ip = subnet_dhcp_ip - if not isolated_subnets[subnet.id] and gateway: + elif not isolated_subnets[subnet.id] and gateway: metadata_route_ip = gateway if metadata_route_ip: From a4d2598a50ce17ea2a443faae122526118a0298d Mon Sep 17 00:00:00 2001 From: yatinkarel Date: Fri, 24 Nov 2023 17:54:49 +0530 Subject: [PATCH 4/5] [DHCP agent] Fetch OVN Metadata port from plugin OVN metadata port from NetworkCache is being used and fixed_ips for it were not available at the time network was added into the cache. So let's re fetch it if it is available in the cache. Related-Bug: #1982569 Related-Bug: https://bugzilla.redhat.com/show_bug.cgi?id=2213862 Change-Id: Ie0ec43566fa2b3e13b4917493336ce1519c9b6bb (cherry picked from commit 0afa24d9af9425036b452ca3ffac685f2a30d47d) --- neutron/agent/linux/dhcp.py | 3 ++- neutron/tests/unit/agent/linux/test_dhcp.py | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 5f6c9c1e508..323bd37125a 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -1144,7 +1144,8 @@ def _get_ovn_metadata_port_ip(self, subnet): m_ports = [port for port in self.network.ports if self._is_ovn_metadata_port(port, self.network.id)] if m_ports: - for fixed_ip in m_ports[0].fixed_ips: + port = self.device_manager.plugin.get_dhcp_port(m_ports[0].id) + for fixed_ip in port.fixed_ips: if fixed_ip.subnet_id == subnet.id: return fixed_ip.ip_address diff --git a/neutron/tests/unit/agent/linux/test_dhcp.py b/neutron/tests/unit/agent/linux/test_dhcp.py index d33b479461d..e3b6ac8497b 100644 --- a/neutron/tests/unit/agent/linux/test_dhcp.py +++ b/neutron/tests/unit/agent/linux/test_dhcp.py @@ -3126,6 +3126,8 @@ def test__generate_opts_per_subnet_no_metadata(self): def test__generate_opts_per_subnet_with_metadata_port(self): config = {'enable_isolated_metadata': False, 'force_metadata': False} + self.mock_mgr.return_value.plugin.get_dhcp_port.return_value = \ + FakeOvnMetadataPort() self._test__generate_opts_per_subnet_helper(config, True, network_class=FakeNetworkDhcpandOvnMetadataPort) From d74a5d384db9c1142de73d56b19db413a8271f86 Mon Sep 17 00:00:00 2001 From: yatinkarel Date: Thu, 30 Nov 2023 18:26:33 +0530 Subject: [PATCH 5/5] [Stable yoga Only] Drop ovs/ovn fips 9-stream jobs These jobs do run on CentOS 9-Stream and broken with pyroute2-0.6.6 in stable/yoga similar to what seen in the related bug. Related-Bug: #2023764 Change-Id: I6ca1294b0f7ec1240c1f1d1f87114db4f223caef --- zuul.d/job-templates.yaml | 2 -- zuul.d/tempest-singlenode.yaml | 36 ---------------------------------- 2 files changed, 38 deletions(-) diff --git a/zuul.d/job-templates.yaml b/zuul.d/job-templates.yaml index 0c1baf3e346..400acf694ad 100644 --- a/zuul.d/job-templates.yaml +++ b/zuul.d/job-templates.yaml @@ -100,8 +100,6 @@ - neutron-ovn-tempest-mariadb-full - neutron-ovn-tempest-ovs-release - neutron-ovn-tempest-ovs-release-ipv6-only - - neutron-ovs-tempest-fips - - neutron-ovn-tempest-ovs-release-fips - devstack-tobiko-neutron: voting: true - ironic-tempest-ipa-wholedisk-bios-agent_ipmitool-tinyipa diff --git a/zuul.d/tempest-singlenode.yaml b/zuul.d/tempest-singlenode.yaml index 709879e6bc3..f60ecf46c6d 100644 --- a/zuul.d/tempest-singlenode.yaml +++ b/zuul.d/tempest-singlenode.yaml @@ -491,39 +491,3 @@ name: neutron-ovn-tempest-ovs-release description: Job testing for devstack/tempest testing Neutron with ovn driver and latest released OVN branch parent: neutron-ovn-base - -- job: - name: neutron-ovs-tempest-fips - parent: neutron-ovs-tempest-base - nodeset: devstack-single-node-centos-9-stream - description: | - Scenario testing for a FIPS enabled Centos 9 system - pre-run: playbooks/enable-fips.yaml - vars: - nslookup_target: 'opendev.org' - configure_swap_size: 4096 - devstack_services: - br-ex-tcpdump: true - br-int-flows: true - devstack_local_conf: - test-config: - "$TEMPEST_CONFIG": - validation: - ssh_key_type: 'ecdsa' - - -- job: - name: neutron-ovn-tempest-ovs-release-fips - parent: neutron-ovn-tempest-ovs-release - nodeset: devstack-single-node-centos-9-stream - description: | - Scenario testing for a FIPS enabled Centos 9 system - pre-run: playbooks/enable-fips.yaml - vars: - nslookup_target: 'opendev.org' - configure_swap_size: 4096 - devstack_local_conf: - test-config: - "$TEMPEST_CONFIG": - validation: - ssh_key_type: 'ecdsa'