vr: wait and restart dnsmasq once per run, not once per leftover lease

When the release of several leases did not take, each of them waited up to
2 s and restarted dnsmasq on its own, so a run with many leftover leases kept
DHCP and DNS on the router going down again and again. Send all the releases
first, wait for them together, then take whatever is left out of the leases
file in one pass and restart dnsmasq at most once. When dnsmasq releases the
leases, as it normally does, nothing is restarted, as before.

Author: calvix

systemvm/debian/opt/cloud/bin/cs/CsDhcp.py

@@ -163,7 +163,7 @@ def delete_leases(self):
             for host in open(DHCP_HOSTS):
                 macs_dhcphosts.append(host.split(',')[0])
 
-            removed = 0
+            released = []
             # read it all first: dnsmasq and remove_lease rewrite the file in place
             with open(LEASES) as fp:
                 leaselines = fp.readlines()
@@ -175,11 +175,11 @@ def delete_leases(self):
                     logging.info("Releasing DHCP lease for IP: %s, mac: %s", ip, mac)
                     if not self.cl.is_redundant() or self.cl.is_primary():
                         self.release_lease(ip, mac)
-                    if self.ensure_lease_removed(ip):
-                        logging.info("Lease for %s still existed after the release; removed manually", ip)
-                    removed = removed + 1
+                    released.append(ip)
                     self.del_host(ip)
-            logging.info("Deleted %s entries from dnsmasq.leases file" % str(removed))
+            if released:
+                self.ensure_leases_removed(released)
+            logging.info("Deleted %s entries from dnsmasq.leases file" % str(len(released)))
         except Exception as e:
             logging.error("Caught error while trying to delete entries from dnsmasq.leases file: %s" % e)
 
@@ -222,7 +222,7 @@ def dnsmasq_writes_leases(self):
         except IOError:
             return True
 
-    def remove_lease(self, ip, restart_dnsmasq=True):
+    def remove_lease(self, ip):
         if not os.path.exists(LEASES):
             return False
 
@@ -239,23 +239,21 @@ def remove_lease(self, ip, restart_dnsmasq=True):
                 fp.truncate()
             finally:
                 fcntl.flock(fp.fileno(), fcntl.LOCK_UN)
-
-        if restart_dnsmasq:
-            # dnsmasq reads the leases file only when it starts
-            CsHelper.service("dnsmasq", "try-restart")
         return True
 
-    def ensure_lease_removed(self, ip):
-        if not self.dnsmasq_writes_leases():
-            # nothing else takes the line out of the file
-            self.remove_lease(ip, restart_dnsmasq=False)
-            return False
-        # give dnsmasq time to drop the released lease from the file
-        for _ in range(20):
-            if not self.lease_exists(ip):
-                return False
-            time.sleep(0.1)
-        return self.remove_lease(ip)
+    def ensure_leases_removed(self, ips):
+        writes_leases = self.dnsmasq_writes_leases()
+        if writes_leases:
+            # give dnsmasq time to drop the released leases from the file
+            for _ in range(20):
+                if not any(self.lease_exists(ip) for ip in ips):
+                    return
+                time.sleep(0.1)
+        left = [ip for ip in ips if self.remove_lease(ip)]
+        if left and writes_leases:
+            logging.warning("dnsmasq did not release the leases for %s; removed them from %s", ", ".join(left), LEASES)
+            # dnsmasq reads the leases file only when it starts; once per run, however many were left
+            CsHelper.service("dnsmasq", "try-restart")
 
     def preseed(self):
         self.add_host("127.0.0.1", "localhost")

systemvm/test/TestCsDhcp.py

@@ -81,7 +81,7 @@ def test_remove_lease_in_place(self, service):
         self.assertEqual(inode, os.stat(self.leases).st_ino)
         with open(self.leases) as f:
             self.assertEqual("0 02:00:00:00:00:02 10.1.1.72 vm-b *\n", f.read())
-        service.assert_called_once_with("dnsmasq", "try-restart")
+        service.assert_not_called()
 
     def router(self, redundant=False, primary=True):
         csdhcp = CsDhcp("dhcpentry", {})
@@ -92,17 +92,28 @@ def router(self, redundant=False, primary=True):
 
     @mock.patch("cs.CsDhcp.time.sleep")
     @mock.patch("cs.CsDhcp.CsHelper.service")
-    def test_lease_left_by_dnsmasq_is_removed_and_dnsmasq_restarted(self, service, sleep):
-        self.assertTrue(self.router().ensure_lease_removed("10.1.1.71"))
+    def test_leases_left_by_dnsmasq_are_removed_with_one_wait_and_one_restart(self, service, sleep):
+        self.router().ensure_leases_removed(["10.1.1.71", "10.1.1.72"])
         self.assertEqual(20, sleep.call_count)
         service.assert_called_once_with("dnsmasq", "try-restart")
+        with open(self.leases) as f:
+            self.assertEqual("", f.read())
+
+    @mock.patch("cs.CsDhcp.time.sleep")
+    @mock.patch("cs.CsDhcp.CsHelper.service")
+    def test_released_leases_need_no_wait_or_restart(self, service, sleep):
+        with open(self.leases, "w") as f:
+            f.write("")
+        self.router().ensure_leases_removed(["10.1.1.71", "10.1.1.72"])
+        sleep.assert_not_called()
+        service.assert_not_called()
 
     @mock.patch("cs.CsDhcp.time.sleep")
     @mock.patch("cs.CsDhcp.CsHelper.service")
     def test_read_only_leases_file_is_cleaned_without_wait_or_restart(self, service, sleep):
         with open(self.managed_lease, "w") as f:
             f.write("0\n")
-        self.router().ensure_lease_removed("10.1.1.71")
+        self.router().ensure_leases_removed(["10.1.1.71"])
         sleep.assert_not_called()
         service.assert_not_called()
         with open(self.leases) as f:
@@ -137,6 +148,9 @@ def test_all_stale_leases_are_handled_while_the_file_is_rewritten(self, service,
             open(os.path.join(self.tmpdir, "dhcphosts.txt"), "w").close()
             csdhcp.delete_leases()
         self.assertEqual(1000, csdhcp.release_lease.call_count)
+        # none of them was released (release_lease is a mock): still one wait and one restart
+        self.assertEqual(20, sleep.call_count)
+        service.assert_called_once_with("dnsmasq", "try-restart")
         with open(self.leases) as f:
             self.assertEqual("", f.read())