Delete network namespace on last port deletion

Since a DHCP agent can handle multiple networks, each with
their own unique network namespace, we must track in-use
ports by network ID, so we delete the namespace when the
last port on that network is removed. If we group all the
ports together then we could have stale, empty namespaces
until we either delete all the ports (unlikely) or the
dhcp-agent is restarted and does cleanup.

Regression introduced by
https://review.opendev.org/c/openstack/neutron/+/840421

Closes-bug: #2015388
Change-Id: I36991328cabcbd6fa473b8d1d140ba88c774fb23
(cherry picked from commit dfe29e6)

Author: brianphaley

neutron/agent/linux/dhcp.py

@@ -245,8 +245,9 @@ def clean_devices(self):
 class DhcpLocalProcess(DhcpBase, metaclass=abc.ABCMeta):
     PORTS = []
 
-    # Track running interfaces.
-    _interfaces = set()
+    # Track running interfaces, indexed by network ID, for example,
+    # {net-id-1: set(intf_1, intf_2), net-id-2: set(intf_3, intf_4), ...}
+    _interfaces = collections.defaultdict(set)
 
     def __init__(self, conf, network, process_monitor, version=None,
                  plugin=None, segment=None):
@@ -267,20 +268,24 @@ def __init__(self, conf, network, process_monitor, version=None,
         fileutils.ensure_tree(self.network_conf_dir, mode=0o755)
 
     @classmethod
-    def _add_running_interface(cls, interface):
-        """Safe method that add running interface"""
-        cls._interfaces.add(interface)
+    def _add_running_interface(cls, interface, network_id):
+        """Safe method that adds a given interface"""
+        cls._interfaces[network_id].add(interface)
 
     @classmethod
-    def _del_running_interface(cls, interface):
-        """Safe method that remove given interface"""
-        if interface in cls._interfaces:
-            cls._interfaces.remove(interface)
+    def _del_running_interface(cls, interface, network_id):
+        """Safe method that removes a given interface"""
+        if cls._interfaces.get(network_id):
+            if interface in cls._interfaces[network_id]:
+                cls._interfaces[network_id].remove(interface)
+            # no entries, cleanup
+            if not cls._interfaces[network_id]:
+                del cls._interfaces[network_id]
 
     @classmethod
-    def _has_running_interfaces(cls):
-        """Safe method that remove given interface"""
-        return bool(cls._interfaces)
+    def _has_running_interfaces(cls, network_id):
+        """Safe method that checks for interfaces"""
+        bool(cls._interfaces.get(network_id))
 
     @staticmethod
     def get_confs_dir(conf):
@@ -332,7 +337,8 @@ def _enable(self):
                     self.network, self.segment)
                 self.interface_name = interface_name
                 self.spawn_process()
-                self._add_running_interface(self.interface_name)
+                self._add_running_interface(self.interface_name,
+                                            self.network.id)
             return True
         except exceptions.ProcessExecutionError as error:
             LOG.debug("Spawning DHCP process for network %s failed; "
@@ -364,7 +370,7 @@ def disable(self, retain_port=False, block=False, **kwargs):
                             pm.pid, SIGTERM_TIMEOUT)
                 pm.disable(sig=str(int(signal.SIGKILL)))
                 common_utils.wait_until_true(lambda: not self.active)
-        self._del_running_interface(self.interface_name)
+        self._del_running_interface(self.interface_name, self.network.id)
         if not retain_port:
             self._destroy_namespace_and_port()
             self._remove_config_files()
@@ -376,8 +382,9 @@ def _destroy_namespace_and_port(self):
         except RuntimeError:
             LOG.warning('Failed trying to delete interface: %s',
                         self.interface_name)
-        if not self._has_running_interfaces():
-            # Delete nm only if we don't serve different segmentation id.
+        # Delete namespace only if there are no running interfaces in it,
+        # which covers the case where a network has multiple segmentation ids.
+        if not self._has_running_interfaces(self.network.id):
             try:
                 ip_lib.delete_network_namespace(self.network.namespace)
             except RuntimeError:

neutron/tests/unit/agent/linux/test_dhcp.py

@@ -719,6 +719,17 @@ def __init__(self, domain='openstacklocal'):
                       FakeRouterPort(domain=domain)]
 
 
+class FakeDualNetworkV2(object):
+    def __init__(self, domain='openstacklocal'):
+        self.id = 'dddddddd-dddd-dddd-dddd-dddddddddddd'
+        self.subnets = [FakeV4Subnet(), FakeV6SubnetDHCPStateful()]
+        self.namespace = 'qdhcp-ns-v2'
+        self.ports = [FakePort1(domain=domain), FakeV6Port(domain=domain),
+                      FakeDualPort(domain=domain),
+                      FakeRouterHAPort(),
+                      FakeRouterPort(domain=domain)]
+
+
 class FakeDeviceManagerNetwork(object):
     def __init__(self):
         self.id = 'cccccccc-cccc-cccc-cccc-cccccccccccc'
@@ -1259,6 +1270,37 @@ def test_disable(self):
 
         delete_ns.assert_called_with('qdhcp-ns')
 
+    def test_enable_disable_two_networks(self):
+        attrs_to_mock = {'active': mock.DEFAULT}
+
+        with mock.patch.multiple(LocalChild, **attrs_to_mock) as mocks:
+            mocks['active'].__get__ = mock.Mock(return_value=False)
+            lp = LocalChild(self.conf, FakeDualNetwork())
+            lp2 = LocalChild(self.conf, FakeDualNetworkV2())
+            lp.enable()
+            lp2.enable()
+            with mock.patch('neutron.agent.linux.ip_lib.'
+                            'delete_network_namespace') as delete_ns:
+                lp.disable()
+                self.rmtree.assert_called_once()
+
+            self._assert_disabled(lp)
+
+            delete_ns.assert_called_once()
+            delete_ns.assert_called_with('qdhcp-ns')
+
+            delete_ns.reset_mock()
+            self.rmtree.reset_mock()
+            with mock.patch('neutron.agent.linux.ip_lib.'
+                            'delete_network_namespace') as delete_ns:
+                lp2.disable()
+                self.rmtree.assert_called_once()
+
+            self._assert_disabled(lp2)
+
+            delete_ns.assert_called_once()
+            delete_ns.assert_called_with('qdhcp-ns-v2')
+
     def test_disable_config_dir_removed_after_destroy(self):
         parent = mock.MagicMock()
         parent.attach_mock(self.rmtree, 'rmtree')