-
Notifications
You must be signed in to change notification settings - Fork 1.3k
vr: reload dnsmasq when start vms #5319
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.
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
Conversation
|
@blueorangutan package |
|
@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✖️ el8 ✔️ debian. SL-JID 883 |
|
@blueorangutan test matrix |
|
@weizhouapache a Trillian-Jenkins matrix job (centos7 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1660)
|
|
Trillian test result (tid-1662)
|
|
Trillian test result (tid-1661)
|
|
@blueorangutan test |
|
@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1675)
|
nvazquez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
davidjumani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self.delete_leases() | ||
|
|
||
| self.delete_leases() | ||
| self.write_hosts() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weizhouapache can you explain what this line does? Isn't deleting leases going to cause any regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhtyd
the method delete_leases was mainly introduced by #3351, which is used to remove entries from dnsmasq.leases file for VMs which have been removed (no impact on all existing vms).
in my opinion, it will not cause any regression. the only issue I see is that execution time will be increased few milliseconds if stop/start a vm(currently delete_leases is not triggered).
cloudstack/systemvm/debian/opt/cloud/bin/cs/CsDhcp.py
Lines 134 to 154 in 737f50d
| def delete_leases(self): | |
| macs_dhcphosts = [] | |
| try: | |
| logging.info("Attempting to delete entries from dnsmasq.leases file for VMs which are not on dhcphosts file") | |
| for host in open(DHCP_HOSTS): | |
| macs_dhcphosts.append(host.split(',')[0]) | |
| removed = 0 | |
| for leaseline in open(LEASES): | |
| lease = leaseline.split(' ') | |
| mac = lease[1] | |
| ip = lease[2] | |
| if mac not in macs_dhcphosts: | |
| cmd = "dhcp_release $(ip route get %s | grep eth | head -1 | awk '{print $3}') %s %s" % (ip, ip, mac) | |
| logging.info(cmd) | |
| CsHelper.execute(cmd) | |
| removed = removed + 1 | |
| self.del_host(ip) | |
| logging.info("Deleted %s entries from dnsmasq.leases file" % str(removed)) | |
| except Exception as e: | |
| logging.error("Caught error while trying to delete entries from dnsmasq.leases file: %s" % e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rhtyd anyway, I added a new commit to address your comment.
leases will be deleted only when one of config files is changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks.
|
@blueorangutan package |
|
@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 931 |
|
@blueorangutan test |
|
@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-1720)
|
yadvr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - did not test it.
|
Suggested tests if not already done - please check/confirm @weizhouapache cc @nvazquez Monitor dnsmasq service and check expected outcome for cases in both isolated network and VPC tier: (maybe more cases you can think of?)
|
@rhtyd @nvazquez I will test the scenarios. |
@rhtyd ,cc @nvazquez |
Description
This PR fixed #5208 and #3613
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?