Skip to content

vr: Ensuring dnsmasq.leases file is populated #4529

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 2 commits into from
Dec 14, 2020
Merged

vr: Ensuring dnsmasq.leases file is populated #4529

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a19faf9
vr: Ensuring dnsmasq.leases file is populated
davidjumani Dec 10, 2020
3f1a444
Fix pylint issues
davidjumani Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions systemvm/debian/opt/cloud/bin/cs/CsDhcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ def process(self):
self.cloud = CsFile(DHCP_HOSTS)
self.dhcp_opts = CsFile(DHCP_OPTS)
self.conf = CsFile(CLOUD_CONF)
self.dhcp_leases = CsFile(LEASES)

self.cloud.repopulate()
self.dhcp_opts.repopulate()
Expand All @@ -60,6 +61,9 @@ def process(self):
if self.cloud.commit():
restart_dnsmasq = True

if self.dhcp_leases.commit():
restart_dnsmasq = True
Copy link
Member

@weizhouapache weizhouapache Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd @davidjumani
the only issue I see is, dnsmasq will be restarted each time when conf/host/dhcp lease is changed.
if there are many vms in the network, might it be a problem ?
it is not caused by this pr , but the line above

        if self.cloud.commit():
            restart_dnsmasq = True

might it be better to restart dnsmasq when all are processed ?

Copy link
Contributor Author

@davidjumani davidjumani Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I could see at https://github.com/apache/cloudstack/blob/master/systemvm/debian/opt/cloud/bin/cs/CsFile.py#L58, a commit returns a boolean value of whether the file has changed, so if it tries to add a host which already exists in the file, it shouldn't restart dnsmasq

Copy link
Member

@weizhouapache weizhouapache Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhtyd @davidjumani no worries, it looks good.

2020-12-11 09:39:48,228 INFO     Processing JSON file cmd_line.json
2020-12-11 09:39:49,165 INFO     Wrote edited file /etc/dnsmasq.d/cloud.conf
2020-12-11 09:39:49,166 INFO     Nothing to commit. The /var/lib/misc/dnsmasq.leases file did not change
2020-12-11 09:39:49,166 INFO     Attempting to delete entries from dnsmasq.leases file for VMs which are not on dhcphosts file
2020-12-11 09:39:49,167 ERROR    Caught error while trying to delete entries from dnsmasq.leases file: [Errno 2] No such file or directory: '/etc/dhcphosts.txt'
2020-12-11 09:39:49,168 INFO     Executing: systemctl restart dnsmasq
2020-12-11 09:39:49,704 INFO     Service dnsmasq restart
2020-12-11 09:39:49,795 INFO     Nothing to commit. The /etc/dnsmasq.d/cloud.conf file did not change
2020-12-11 09:39:49,795 INFO     Nothing to commit. The /var/lib/misc/dnsmasq.leases file did not change
2020-12-11 09:39:49,795 INFO     Executing: systemctl is-active dnsmasq
2020-12-11 09:39:49,820 INFO     Executing: systemctl reload dnsmasq
2020-12-11 09:39:49,878 INFO     Service dnsmasq reload
2020-12-11 09:40:11,302 INFO     Processing JSON file ip_associations.json.a72a28ec-caed-4f3a-908f-e8d8df726d57
2020-12-11 09:40:11,910 INFO     Processing JSON file monitor_service.json.335af30e-fd9e-44b1-a6c0-248a98c480db
2020-12-11 09:40:12,587 INFO     Processing JSON file vm_dhcp_entry.json.afaa3985-b0e0-4f2f-bd73-f61282006e0e
2020-12-11 09:40:12,794 INFO     Processing JSON file vm_dhcp_entry.json.fbba4e25-3569-4513-99ab-39d9f38a80d2
2020-12-11 09:40:13,129 INFO     Processing JSON file vm_dhcp_entry.json.b6a1f368-180b-4a48-b664-22aa8e89cfba
2020-12-11 09:40:13,530 INFO     Processing JSON file vm_metadata.json.ab86c2c2-acfc-4d01-8e4b-bd11ae7101ed
2020-12-11 09:40:13,993 INFO     Processing JSON file vm_metadata.json.8750a124-ba49-4fb5-8484-50ff65971f9a
2020-12-11 09:40:14,256 INFO     Processing JSON file vm_metadata.json.72e3d268-5e11-4bcd-a693-ab8fc599718b
2020-12-11 09:40:14,713 INFO     Wrote edited file /etc/dnsmasq.d/cloud.conf
2020-12-11 09:40:14,714 INFO     Wrote edited file /var/lib/misc/dnsmasq.leases
2020-12-11 09:40:14,715 INFO     Attempting to delete entries from dnsmasq.leases file for VMs which are not on dhcphosts file
2020-12-11 09:40:14,724 INFO     Deleted 0 entries from dnsmasq.leases file
2020-12-11 09:40:14,725 INFO     Executing: systemctl restart dnsmasq
2020-12-11 09:40:14,866 INFO     Service dnsmasq restart
2020-12-11 09:40:23,530 INFO     Processing JSON file firewall_rules.json.0b59db02-5eb7-44b3-a6d7-2ab3f1ad03b5
2020-12-11 09:40:24,646 INFO     Nothing to commit. The /etc/dnsmasq.d/cloud.conf file did not change
2020-12-11 09:40:24,646 INFO     Nothing to commit. The /var/lib/misc/dnsmasq.leases file did not change
2020-12-11 09:40:24,648 INFO     Executing: systemctl is-active dnsmasq
2020-12-11 09:40:24,660 INFO     Executing: systemctl reload dnsmasq
2020-12-11 09:40:24,688 INFO     Service dnsmasq reload

Copy link
Member

@weizhouapache weizhouapache Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I could see at https://github.com/apache/cloudstack/blob/master/systemvm/debian/opt/cloud/bin/cs/CsFile.py#L58, a commit returns a boolean value of whether the file has changed, so if it tries to add a host which already exists in the file, it shouldn't restart dnsmasq

@davidjumani from my testing, dnsmasq will be restarted each time when start a vm.
it's better to reload it. not a big problem.

2020-12-11 09:44:56,927 INFO     Processing JSON file vm_dhcp_entry.json.c0a11309-5ec0-4166-8c17-ae2c56a44b8e
2020-12-11 09:44:57,244 INFO     Nothing to commit. The /etc/dnsmasq.d/cloud.conf file did not change
2020-12-11 09:44:57,244 INFO     Wrote edited file /var/lib/misc/dnsmasq.leases
2020-12-11 09:44:57,245 INFO     Attempting to delete entries from dnsmasq.leases file for VMs which are not on dhcphosts file
2020-12-11 09:44:57,245 INFO     Deleted 0 entries from dnsmasq.leases file
2020-12-11 09:44:57,245 INFO     Executing: systemctl restart dnsmasq
2020-12-11 09:44:57,366 INFO     Service dnsmasq restart
2020-12-11 09:44:58,253 INFO     Processing JSON file vm_metadata.json.250b5ba8-fd86-4dcb-86ef-2bc6c634d224

Copy link
Contributor Author

@davidjumani davidjumani Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's linked to #3613
I can create a separate PR to change it globally, but that will require extensive testing

Copy link
Member

@weizhouapache weizhouapache Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's linked to #3613
I can create a separate PR to change it globally, but that will require extensive testing

@davidjumani yeah.
@rhtyd this is ready for merge.


self.dhcp_opts.commit()

if restart_dnsmasq:
Expand Down Expand Up @@ -186,6 +190,9 @@ def add(self, entry):
entry['ipv4_address'],
entry['host_name'],
lease))
self.dhcp_leases.search(entry['mac_address'], "0 %s %s %s *" % (entry['mac_address'],
entry['ipv4_address'],
entry['host_name']))
else:
tag = entry['ipv4_address'].replace(".", "_")
self.cloud.add("%s,set:%s,%s,%s,%s" % (entry['mac_address'],
Expand All @@ -196,6 +203,9 @@ def add(self, entry):
self.dhcp_opts.add("%s,%s" % (tag, 3))
self.dhcp_opts.add("%s,%s" % (tag, 6))
self.dhcp_opts.add("%s,%s" % (tag, 15))
self.dhcp_leases.search(entry['mac_address'], "0 %s %s %s *" % (entry['mac_address'],
Copy link
Member

@yadvr yadvr Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above - can you check if the lease should have a high expiry (80yrs or inf?) instead of *, I'm not sure what 0 and * mean here

Copy link
Contributor Author

@davidjumani davidjumani Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 is infinite lease time (generally the epoch when the lease expires), * is the client id (which is generally the mac, but can change)
Taken this format from existing leases file
https://dnsmasq-discuss.thekelleys.org.narkive.com/ia9YNLmE/dnsmasq-leases-file-format-specification

entry['ipv4_address'],
entry['host_name']))

i = IPAddress(entry['ipv4_address'])
# Calculate the device
Expand Down