calvix commented on code in PR #14294:
URL: https://github.com/apache/cloudstack/pull/14294#discussion_r4168081484
##########
systemvm/debian/opt/cloud/bin/cs/CsDhcp.py:
##########
@@ -193,51 +211,51 @@ def lease_exists(self, ip):
return False
- def remove_lease(self, ip):
- if not os.path.exists(LEASES):
+ def dnsmasq_writes_leases(self):
+ # dnsmasq does not run on a backup router, and does not touch a
read-only leases file
+ # (leasefile-ro, kept on routers with IPv6 on the guest NIC, see
setup_dnsmasq)
+ if self.cl.is_redundant() and not self.cl.is_primary():
return False
+ try:
+ with open(DNSMASQ_MANAGED_LEASE) as fp:
+ return fp.read().strip() != "0"
+ except IOError:
+ return True
- removed = False
+ def remove_lease(self, ip, restart_dnsmasq=True):
+ if not os.path.exists(LEASES):
+ return False
with open(LEASES, "r+") as fp:
fcntl.flock(fp.fileno(), fcntl.LOCK_EX)
- lines = fp.readlines()
-
- fd, tmp_path = tempfile.mkstemp(
- prefix="dnsmasq.leases.",
- dir=os.path.dirname(LEASES)
- )
-
try:
- with os.fdopen(fd, "w") as tmp:
- for line in lines:
- fields = line.split()
-
- if len(fields) >= 3 and fields[2] == ip:
- removed = True
- continue
-
- tmp.write(line)
-
- if removed:
- shutil.move(tmp_path, LEASES)
-
- # reload dnsmasq
- try:
- CsHelper.service("dnsmasq", "reload")
- except Exception:
- pass
- else:
- os.remove(tmp_path)
+ lines = fp.readlines()
+ kept = [line for line in lines if not (len(line.split()) >= 3
and line.split()[2] == ip)]
+ if len(kept) == len(lines):
+ return False
+ # rewrite in place: dnsmasq keeps the file it opened at start
+ fp.seek(0)
+ fp.writelines(kept)
+ fp.truncate()
finally:
fcntl.flock(fp.fileno(), fcntl.LOCK_UN)
- return removed
+ 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 self.lease_exists(ip):
- return self.remove_lease(ip)
- return False
+ 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)
Review Comment:
You have a good point, but the restart only happens if the release didn't go
through, so when the lease is still in the file 2s after sending it.
Normally `dnsmasq` should remove it right away and nothing gets restarted.
On the backups VR and on routers with IPv6 on the guest NIC it never
restarts at all.
But you're right that if it fails for several leases in one run, it restarts
once per lease. I can change it to
send all the releases first, wait once, and then clean up and restart only
once at the end.
Want me to add that here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
