Restart dsnmasq on any network subnet change

When a new subnet is added to a network, the network cache
is updated with the list of subnets regardless of which ones
have DHCP enabled. This changes the index order of the subnet
list which means that the tags used for each subnet change.

This means we must restart the process because the opts file
will be using different tags than the process args. This patch
implements that change. It also sorts the subnets on the RPC
side so the agent indexes don't change if subnets aren't
added/deleted.

The previous logic was only restarting the process when DHCP
enabled subnets changed, which meant that adding a DHCP disabled
subnet would break the association between the opts file tags and
the process arg tags, which led to the reported bug.

Closes-Bug: #1581918
Change-Id: If1452c0e8fe95eb94cd78c7a05b57aead75662b5

neutron/agent/dhcp/agent.py

@@ -281,17 +281,18 @@ class DhcpAgent(manager.Manager):
         if not network:
             return
 
-        old_cidrs = set(s.cidr for s in old_network.subnets if s.enable_dhcp)
-        new_cidrs = set(s.cidr for s in network.subnets if s.enable_dhcp)
-
-        if new_cidrs and old_cidrs == new_cidrs:
+        if not any(s for s in network.subnets if s.enable_dhcp):
+            self.disable_dhcp_helper(network.id)
+            return
+        # NOTE(kevinbenton): we don't exclude dhcp disabled subnets because
+        # they still change the indexes used for tags
+        old_cidrs = [s.cidr for s in network.subnets]
+        new_cidrs = [s.cidr for s in old_network.subnets]
+        if old_cidrs == new_cidrs:
             self.call_driver('reload_allocations', network)
             self.cache.put(network)
-        elif new_cidrs:
-            if self.call_driver('restart', network):
-                self.cache.put(network)
-        else:
-            self.disable_dhcp_helper(network.id)
+        elif self.call_driver('restart', network):
+            self.cache.put(network)
 
     @utils.synchronized('dhcp-agent')
     def network_create_end(self, context, payload):

neutron/api/rpc/handlers/dhcp_rpc.py

@@ -141,7 +141,11 @@ class DhcpRpcCallback(object):
         filters = {'network_id': [network['id'] for network in networks]}
         ports = plugin.get_ports(context, filters=filters)
         filters['enable_dhcp'] = [True]
-        subnets = plugin.get_subnets(context, filters=filters)
+        # NOTE(kevinbenton): we sort these because the agent builds tags
+        # based on position in the list and has to restart the process if
+        # the order changes.
+        subnets = sorted(plugin.get_subnets(context, filters=filters),
+                         key=operator.itemgetter('id'))
 
         grouped_subnets = self._group_by_network_id(subnets)
         grouped_ports = self._group_by_network_id(ports)
@@ -166,7 +170,12 @@ class DhcpRpcCallback(object):
                       "been deleted concurrently.", network_id)
             return
         filters = dict(network_id=[network_id])
-        network['subnets'] = plugin.get_subnets(context, filters=filters)
+        # NOTE(kevinbenton): we sort these because the agent builds tags
+        # based on position in the list and has to restart the process if
+        # the order changes.
+        network['subnets'] = sorted(
+            plugin.get_subnets(context, filters=filters),
+            key=operator.itemgetter('id'))
         network['ports'] = plugin.get_ports(context, filters=filters)
         return network
 

neutron/tests/unit/agent/dhcp/test_agent.py

@@ -910,6 +910,20 @@ class TestDhcpAgentEventHandler(base.BaseTestCase):
             self.assertTrue(log.called)
             self.assertTrue(self.dhcp.schedule_resync.called)
 
+    def test_subnet_create_restarts_with_dhcp_disabled(self):
+        payload = dict(subnet=dhcp.DictModel(
+              dict(network_id=fake_network.id, enable_dhcp=False,
+                   cidr='99.99.99.0/24')))
+        self.cache.get_network_by_id.return_value = fake_network
+        new_net = copy.deepcopy(fake_network)
+        new_net.subnets.append(payload['subnet'])
+        self.plugin.get_network_info.return_value = new_net
+
+        self.dhcp.subnet_create_end(None, payload)
+
+        self.cache.assert_has_calls([mock.call.put(new_net)])
+        self.call_driver.assert_called_once_with('restart', new_net)
+
     def test_subnet_update_end(self):
         payload = dict(subnet=dict(network_id=fake_network.id))
         self.cache.get_network_by_id.return_value = fake_network

neutron/tests/unit/api/rpc/handlers/test_dhcp_rpc.py

@@ -69,7 +69,7 @@ class TestDhcpRpcCallback(base.BaseTestCase):
         plugin_retval = [{'id': 'a'}, {'id': 'b'}]
         self.plugin.get_networks.return_value = plugin_retval
         port = {'network_id': 'a'}
-        subnet = {'network_id': 'b'}
+        subnet = {'network_id': 'b', 'id': 'c'}
         self.plugin.get_ports.return_value = [port]
         self.plugin.get_subnets.return_value = [subnet]
         networks = self.callbacks.get_active_networks_info(mock.Mock(),
@@ -153,7 +153,7 @@ class TestDhcpRpcCallback(base.BaseTestCase):
     def test_get_network_info(self):
         network_retval = dict(id='a')
 
-        subnet_retval = mock.Mock()
+        subnet_retval = [dict(id='a'), dict(id='c'), dict(id='b')]
         port_retval = mock.Mock()
 
         self.plugin.get_network.return_value = network_retval
@@ -162,7 +162,8 @@ class TestDhcpRpcCallback(base.BaseTestCase):
 
         retval = self.callbacks.get_network_info(mock.Mock(), network_id='a')
         self.assertEqual(retval, network_retval)
-        self.assertEqual(retval['subnets'], subnet_retval)
+        sorted_subnet_retval = [dict(id='a'), dict(id='b'), dict(id='c')]
+        self.assertEqual(retval['subnets'], sorted_subnet_retval)
         self.assertEqual(retval['ports'], port_retval)
 
     def test_update_dhcp_port_verify_port_action_port_dict(self):