restart dnsmasq when subnet cidr set changes

fixes bug 1045828

The original bug occurred because the set of subnet cidrs changed
without a restart of dnsmasq.  As a result, dnsmasq would not respond
with offers for fixed_ips assigned in the unknown cidr ranges even when
the underlying host file had properly updated.

This patch addresses the bug by restarting dnsmasq when the set of
subnet cidrs changes.  When the set does not change, it will reload the
options and allocations.

Change-Id: I0e257208750703f4a32c51d1323786e76e676bb6

quantum/agent/dhcp_agent.py

@@ -120,16 +120,22 @@ def refresh_dhcp_helper(self, network_id):
         of the network.
 
         """
-        if not self.cache.get_network_by_id(network_id):
+        old_network = self.cache.get_network_by_id(network_id)
+        if not old_network:
             # DHCP current not running for network.
-            self.enable_dhcp_helper(network_id)
+            return self.enable_dhcp_helper(network_id)
 
         network = self.plugin_rpc.get_network_info(network_id)
-        for subnet in network.subnets:
-            if subnet.enable_dhcp:
-                self.cache.put(network)
-                self.call_driver('enable', network)
-                break
+
+        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:
+            self.call_driver('reload_allocations', network)
+            self.cache.put(network)
+        elif new_cidrs:
+            self.call_driver('restart', network)
+            self.cache.put(network)
         else:
             self.disable_dhcp_helper(network.id)
 

quantum/agent/linux/dhcp.py

@@ -76,12 +76,12 @@ def enable(self):
         """Enables DHCP for this network."""
 
     @abc.abstractmethod
-    def disable(self):
+    def disable(self, retain_port=False):
         """Disable dhcp for this network."""
 
     def restart(self):
         """Restart the dhcp service for the network."""
-        self.disable()
+        self.disable(retain_port=True)
         self.enable()
 
     @abc.abstractproperty
@@ -108,12 +108,12 @@ def enable(self):
         interface_name = self.device_delegate.setup(self.network,
                                                     reuse_existing=True)
         if self.active:
-            self.reload_allocations()
+            self.restart()
         elif self._enable_dhcp():
             self.interface_name = interface_name
             self.spawn_process()
 
-    def disable(self):
+    def disable(self, retain_port=False):
         """Disable DHCP for this network by killing the local process."""
         pid = self.pid
 
@@ -125,7 +125,8 @@ def disable(self):
             else:
                 utils.execute(cmd, self.root_helper)
 
-            self.device_delegate.destroy(self.network, self.interface_name)
+            if not retain_port:
+                self.device_delegate.destroy(self.network, self.interface_name)
 
         elif pid:
             LOG.debug(_('DHCP for %s pid %d is stale, ignoring command') %

quantum/tests/unit/test_dhcp_agent.py

@@ -46,6 +46,10 @@ def __str__(self):
                          network_id='12345678-1234-5678-1234567890ab',
                          enable_dhcp=False)
 
+fake_subnet3 = FakeModel('bbbbbbbb-1111-2222-bbbbbbbbbbbb',
+                         network_id='12345678-1234-5678-1234567890ab',
+                         cidr='192.168.1.1/24', enable_dhcp=True)
+
 fake_fixed_ip = FakeModel('', subnet=fake_subnet1, ip_address='172.9.9.9')
 
 fake_port1 = FakeModel('12345678-1234-aaaa-1234567890ab',
@@ -218,18 +222,21 @@ def test_network_delete_end(self):
             disable.assertCalledOnceWith(fake_network.id)
 
     def test_refresh_dhcp_helper_no_dhcp_enabled_networks(self):
-        network = FakeModel('12345678-1234-5678-1234567890ab',
+        network = FakeModel('net-id',
                             tenant_id='aaaaaaaa-aaaa-aaaa-aaaaaaaaaaaa',
                             admin_state_up=True,
                             subnets=[],
                             ports=[])
 
+        self.cache.get_network_by_id.return_value = network
         self.plugin.get_network_info.return_value = network
         with mock.patch.object(self.dhcp, 'disable_dhcp_helper') as disable:
             self.dhcp.refresh_dhcp_helper(network.id)
             disable.called_once_with_args(network.id)
             self.assertFalse(self.cache.called)
             self.assertFalse(self.call_driver.called)
+            self.cache.assert_has_calls(
+                [mock.call.get_network_by_id('net-id')])
 
     def test_subnet_update_end(self):
         payload = dict(subnet=dict(network_id=fake_network.id))
@@ -239,17 +246,47 @@ def test_subnet_update_end(self):
         self.dhcp.subnet_update_end(payload)
 
         self.cache.assert_has_calls([mock.call.put(fake_network)])
-        self.call_driver.assert_called_once_with('enable', fake_network)
+        self.call_driver.assert_called_once_with('reload_allocations',
+                                                 fake_network)
+
+    def test_subnet_update_end(self):
+        new_state = FakeModel(fake_network.id,
+                              tenant_id=fake_network.tenant_id,
+                              admin_state_up=True,
+                              subnets=[fake_subnet1, fake_subnet3],
+                              ports=[fake_port1])
+
+        payload = dict(subnet=dict(network_id=fake_network.id))
+        self.cache.get_network_by_id.return_value = fake_network
+        self.plugin.get_network_info.return_value = new_state
+
+        self.dhcp.subnet_update_end(payload)
+
+        self.cache.assert_has_calls([mock.call.put(new_state)])
+        self.call_driver.assert_called_once_with('restart',
+                                                 new_state)
 
     def test_subnet_update_end_delete_payload(self):
+        prev_state = FakeModel(fake_network.id,
+                               tenant_id=fake_network.tenant_id,
+                               admin_state_up=True,
+                               subnets=[fake_subnet1, fake_subnet3],
+                               ports=[fake_port1])
+
         payload = dict(subnet_id=fake_subnet1.id)
-        self.cache.get_network_by_subnet_id.return_value = fake_network
+        self.cache.get_network_by_subnet_id.return_value = prev_state
+        self.cache.get_network_by_id.return_value = prev_state
         self.plugin.get_network_info.return_value = fake_network
 
         self.dhcp.subnet_delete_end(payload)
 
-        self.cache.assert_has_calls([mock.call.put(fake_network)])
-        self.call_driver.assert_called_once_with('enable', fake_network)
+        self.cache.assert_has_calls([
+            mock.call.get_network_by_subnet_id(
+                'bbbbbbbb-bbbb-bbbb-bbbbbbbbbbbb'),
+            mock.call.get_network_by_id('12345678-1234-5678-1234567890ab'),
+            mock.call.put(fake_network)])
+        self.call_driver.assert_called_once_with('restart',
+                                                 fake_network)
 
     def test_port_update_end(self):
         payload = dict(port=vars(fake_port2))

quantum/tests/unit/test_linux_dhcp.py

@@ -148,8 +148,8 @@ def __init__(self):
             def enable(self):
                 self.called.append('enable')
 
-            def disable(self):
-                self.called.append('disable')
+            def disable(self, retain_port=False):
+                self.called.append('disable %s' % retain_port)
 
             def reload_allocations(self):
                 pass
@@ -160,7 +160,7 @@ def active(self):
 
         c = SubClass()
         c.restart()
-        self.assertEquals(c.called, ['disable', 'enable'])
+        self.assertEquals(c.called, ['disable True', 'enable'])
 
 
 class LocalChild(dhcp.DhcpLocalProcess):
@@ -173,6 +173,9 @@ def __init__(self, *args, **kwargs):
     def reload_allocations(self):
         self.called.append('reload')
 
+    def restart(self):
+        self.called.append('restart')
+
     def spawn_process(self):
         self.called.append('spawn')
 
@@ -248,7 +251,7 @@ def test_enable_already_active(self):
                             device_delegate=delegate)
             lp.enable()
 
-            self.assertEqual(lp.called, ['reload'])
+            self.assertEqual(lp.called, ['restart'])
 
     def test_enable(self):
         delegate = mock.Mock(return_value='tap0')
@@ -297,6 +300,23 @@ def test_disable_unknown_network(self):
                 msg = log.call_args[0][0]
                 self.assertIn('No DHCP', msg)
 
+    def test_disable_retain_port(self):
+        attrs_to_mock = dict([(a, mock.DEFAULT) for a in
+                              ['active', 'interface_name', 'pid']])
+        delegate = mock.Mock()
+        network = FakeDualNetwork()
+        with mock.patch.multiple(LocalChild, **attrs_to_mock) as mocks:
+            mocks['active'].__get__ = mock.Mock(return_value=True)
+            mocks['pid'].__get__ = mock.Mock(return_value=5)
+            mocks['interface_name'].__get__ = mock.Mock(return_value='tap0')
+            lp = LocalChild(self.conf, network, device_delegate=delegate,
+                            namespace='qdhcp-ns')
+            lp.disable(retain_port=True)
+
+        self.assertFalse(delegate.called)
+        exp_args = ['ip', 'netns', 'exec', 'qdhcp-ns', 'kill', '-9', 5]
+        self.execute.assert_called_once_with(exp_args, root_helper='sudo')
+
     def test_disable(self):
         attrs_to_mock = dict([(a, mock.DEFAULT) for a in
                               ['active', 'interface_name', 'pid']])