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
  neutron/tests/unit/privileged/agent/linux/test_ip_lib.py

Change-Id: I083649ab1b9a9057ee276a7f3ba069eb667db870
Closes-bug: #2030804
(cherry picked from commit 16875b5)

Author: brianphaley

neutron/privileged/agent/linux/ip_lib.py

@@ -473,6 +473,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(
@@ -682,6 +686,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)
@@ -787,6 +796,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)
@@ -821,6 +835,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)
@@ -836,20 +855,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)

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(

neutron/tests/functional/privileged/agent/linux/test_ip_lib.py

@@ -239,19 +239,34 @@ 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)
 
     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']
@@ -439,6 +433,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):
@@ -665,6 +676,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):
 

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(