Damans227 commented on code in PR #14294:
URL: https://github.com/apache/cloudstack/pull/14294#discussion_r4166202350
##########
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:
if a lot of vms are removed at once, does this restart dnsmasq once for
every leftover lease? that could mean a long wait and the router dropping dhcp
over and over
--
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]
