Delete duplicate internal devices in router namespace

When neutron router-interface-delete <router> <subnet> is ran
during L3-agent restart, the agent may fail to delete the old
internal device. After the restart, when the command "neutron
router-interface-add <router> <subnet>" is ran again, the
router ends up having two internal devices configured with the
same IP address.

Closes-Bug: #1244853
Change-Id: I0d7e2db6aa7dae26d0fc3fe1b1527762cb1e3b23

neutron/agent/l3_agent.py

@@ -385,6 +385,13 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
         prefixlen = netaddr.IPNetwork(port['subnet']['cidr']).prefixlen
         port['ip_cidr'] = "%s/%s" % (ips[0]['ip_address'], prefixlen)
 
+    def _get_existing_internal_devices(self, ri):
+        ip_wrapper = ip_lib.IPWrapper(root_helper=self.root_helper,
+                                      namespace=ri.ns_name())
+        ip_devs = ip_wrapper.get_devices(exclude_loopback=True)
+        return [ip_dev.name for ip_dev in ip_devs if
+                ip_dev.name.startswith(INTERNAL_DEV_PREFIX)]
+
     def process_router(self, ri):
         ri.iptables_manager.defer_apply_on()
         ex_gw_port = self._get_ex_gw_port(ri)
@@ -407,6 +414,17 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager):
             self.internal_network_removed(ri, p['id'], p['ip_cidr'])
             ri.internal_ports.remove(p)
 
+        current_internal_devs = set(self._get_existing_internal_devices(ri))
+        current_port_devs = set([self.get_internal_device_name(id) for
+                                 id in current_port_ids])
+        stale_devs = current_internal_devs - current_port_devs
+        for stale_dev in stale_devs:
+            LOG.debug(_('Deleting stale internal router device: %s'),
+                      stale_dev)
+            self.driver.unplug(stale_dev,
+                               namespace=ri.ns_name(),
+                               prefix=INTERNAL_DEV_PREFIX)
+
         internal_cidrs = [p['ip_cidr'] for p in ri.internal_ports]
         # TODO(salv-orlando): RouterInfo would be a better place for
         # this logic too

neutron/tests/unit/test_l3_agent.py

@@ -15,6 +15,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import contextlib
 import copy
 
 import mock
@@ -727,6 +728,64 @@ class TestBasicRouterOperations(base.BaseTestCase):
         self.assertThat(nat_rules.index(jump_float_rule),
                         matchers.LessThan(nat_rules.index(internal_net_rule)))
 
+    def test_process_router_delete_stale_internal_devices(self):
+        class FakeDev(object):
+            def __init__(self, name):
+                self.name = name
+
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        stale_devlist = [FakeDev('qr-a1b2c3d4-e5'),
+                         FakeDev('qr-b2c3d4e5-f6')]
+        stale_devnames = [dev.name for dev in stale_devlist]
+
+        get_devices_return = [FakeDev('qg-a1b2c3d4-e5'),
+                              FakeDev('qg-b2c3d4e5-f6')]
+        get_devices_return.extend(stale_devlist)
+        self.mock_ip.get_devices.return_value = get_devices_return
+
+        router = self._prepare_router_data(enable_snat=True,
+                                           num_internal_ports=1)
+        ri = l3_agent.RouterInfo(router['id'],
+                                 self.conf.root_helper,
+                                 self.conf.use_namespaces,
+                                 router=router)
+
+        internal_ports = ri.router.get(l3_constants.INTERFACE_KEY, [])
+        self.assertEqual(len(internal_ports), 1)
+        internal_port = internal_ports[0]
+
+        with contextlib.nested(mock.patch.object(l3_agent.L3NATAgent,
+                                                 'internal_network_removed'),
+                               mock.patch.object(l3_agent.L3NATAgent,
+                                                 'internal_network_added'),
+                               mock.patch.object(l3_agent.L3NATAgent,
+                                                 'external_gateway_removed'),
+                               mock.patch.object(l3_agent.L3NATAgent,
+                                                 'external_gateway_added')
+                               ) as (internal_network_removed,
+                                     internal_network_added,
+                                     external_gateway_removed,
+                                     external_gateway_added):
+
+            agent.process_router(ri)
+
+            self.assertEqual(external_gateway_added.call_count, 1)
+            self.assertFalse(external_gateway_removed.called)
+            self.assertFalse(internal_network_removed.called)
+            internal_network_added.assert_called_once_with(
+                ri,
+                internal_port['network_id'],
+                internal_port['id'],
+                internal_port['ip_cidr'],
+                internal_port['mac_address'])
+            self.assertEqual(self.mock_driver.unplug.call_count,
+                             len(stale_devnames))
+            calls = [mock.call(stale_devname,
+                               namespace=ri.ns_name(),
+                               prefix=l3_agent.INTERNAL_DEV_PREFIX)
+                     for stale_devname in stale_devnames]
+            self.mock_driver.unplug.assert_has_calls(calls, any_order=True)
+
     def test_routers_with_admin_state_down(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
         self.plugin_api.get_external_network_id.return_value = None