openstack
/
neutron
Code Issues Proposed changes

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.

Change-Id: I083649ab1b9a9057ee276a7f3ba069eb667db870
Closes-bug: #2030804
This commit is contained in:
Brian Haley 2023-08-11 17:05:49 -04:00
parent 3930b9f5c5
commit 16875b5f92
4 changed files with 92 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
neutron/privileged/agent/linux/ip_lib.py
View File

@ -525,6 +525,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(
@ -712,6 +716,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)
@ -817,6 +826,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)
@ -851,6 +865,11 @@ def _command_bridge_fdb(command, mac, device, dst_ip=None, namespace=None,
kwargs['dst'] = dst_ip
with get_iproute(namespace) as ip:
return priv_linux.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)
@ -866,20 +885,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"""
_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"""
_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"""
_command_bridge_fdb('del', mac, device, dst_ip=dst_ip,
namespace=namespace, **kwargs)

7
neutron/tests/functional/agent/linux/test_bridge_lib.py
View File

@ -211,6 +211,13 @@ class FdbInterfaceTestCase(testscenarios.WithScenarios, base.BaseSudoTestCase):
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(

78
neutron/tests/functional/privileged/agent/linux/test_ip_lib.py
View File

@ -260,12 +260,10 @@ class GetLinkDevicesTestCase(functional_base.BaseSudoTestCase):
index=10000)
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)
@ -273,6 +271,23 @@ class ListIpRulesTestCase(functional_base.BaseSudoTestCase):
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))
@ -312,28 +327,7 @@ class ListIpRulesTestCase(functional_base.BaseSudoTestCase):
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']
@ -460,6 +454,23 @@ class RuleTestCase(functional_base.BaseSudoTestCase):
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):
@ -687,6 +698,21 @@ class RouteTestCase(functional_base.BaseSudoTestCase):
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):

11
neutron/tests/unit/privileged/agent/linux/test_ip_lib.py
View File

@ -159,6 +159,17 @@ class IpLibTestCase(base.BaseTestCase):
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(iproute, "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(iproute, "IPRoute") as iproute_mock:
iproute_mock.side_effect = OSError(