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

diff --git a/quantum/agent/dhcp_agent.py b/quantum/agent/dhcp_agent.py
index 2b968fcd937f96a2a2aa76c4a7d2a6cb78f7f4b5..9196a2fbef7d06c2eb9c53905de80771957e93c8 100644 (file)
--- a/quantum/agent/dhcp_agent.py
+++ b/quantum/agent/dhcp_agent.py
@@ -120,16 +120,22 @@ class DhcpAgent(object):
         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)
 

diff --git a/quantum/agent/linux/dhcp.py b/quantum/agent/linux/dhcp.py
index add556b0a4279d92f0a5a7c6e22468598452d0d0..ed875adbd5aa7ab23b7371113ff06db79f9d1c24 100644 (file)
--- a/quantum/agent/linux/dhcp.py
+++ b/quantum/agent/linux/dhcp.py
@@ -76,12 +76,12 @@ class DhcpBase(object):
         """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 @@ class DhcpLocalProcess(DhcpBase):
         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 @@ class DhcpLocalProcess(DhcpBase):
             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') %

diff --git a/quantum/tests/unit/test_dhcp_agent.py b/quantum/tests/unit/test_dhcp_agent.py
index e1b5bce702b8364412f414d4a1e9c9181ac64112..fe71ec51871732fb1f94ec5cf1bb2b67b879b665 100644 (file)
--- a/quantum/tests/unit/test_dhcp_agent.py
+++ b/quantum/tests/unit/test_dhcp_agent.py
@@ -46,6 +46,10 @@ fake_subnet2 = FakeModel('dddddddd-dddd-dddd-dddddddddddd',
                          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 @@ class TestDhcpAgentEventHandler(unittest.TestCase):
             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 @@ class TestDhcpAgentEventHandler(unittest.TestCase):
         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))

diff --git a/quantum/tests/unit/test_linux_dhcp.py b/quantum/tests/unit/test_linux_dhcp.py
index 93f1ae1f30ef4a6e43f2cde1f5c9990476ad439a..9ea831713865ad48d29e05cb28bedaa072d9c7b7 100644 (file)
--- a/quantum/tests/unit/test_linux_dhcp.py
+++ b/quantum/tests/unit/test_linux_dhcp.py
@@ -148,8 +148,8 @@ class TestDhcpBase(unittest.TestCase):
             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 @@ class TestDhcpBase(unittest.TestCase):
 
         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 @@ class LocalChild(dhcp.DhcpLocalProcess):
     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 @@ class TestDhcpLocalProcess(TestBase):
                             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 @@ class TestDhcpLocalProcess(TestBase):
                 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']])