Skip to content
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

Fixes #2687 - Leases file is no longer being parsed on every request #409

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@dmitri-d
Copy link
Member

commented Apr 22, 2016

This is work in progress and currently only isc dhcpd running on linux is supported (support for BSD is coming). Please ignore the failing tests, these are due to changes in internal API.

@domcleal domcleal changed the title [DO NOT MERGE] Isc dhcp leases monitoring [DO NOT MERGE] fixes #2687 - Leases file is no longer being parsed on every request Apr 22, 2016

@lzap

This comment has been minimized.

Copy link
Member

commented Apr 25, 2016

I see some huge refactoring in this PR, is this expected?

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2016

This is based on work in #390 PR. There's also a commit for #406 PR, I might rebase this PR to exclude it. You can ignore first two commits if you are only interested in dhcp changes.

@timogoebel

This comment has been minimized.

Copy link
Member

commented Apr 27, 2016

@witlessbird : I'd be happy to test this. What changes/commits do I need to apply agains 1.11.1?

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2016

@timogoebel: thanks a bunch! The easiest would be to use this branch -- all commits were made against develop (not 1.11), and the changes in the last commit depend on the changes in the first.

@theforeman-bot

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

There were the following issues with the commit message:

  • length of the first commit message line for 4950a5c exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@theforeman-bot

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

There were the following issues with the commit message:

  • length of the first commit message line for 6aadb84 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@timogoebel

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

@witlessbird : I tested this, but somehow it does not work 100%.

With your patch:

D, [2016-04-29T16:56:33.773647 #482] DEBUG -- : Loading subnets for 127.0.0.1
D, [2016-04-29T16:56:33.773888 #482] DEBUG -- : Loading subnet data for 172.23.9.0/255.255.255.0
E, [2016-04-29T16:56:33.774181 #482] ERROR -- : Record 172.23.9.0/172.23.9.49 not found
D, [2016-04-29T16:56:33.774222 #482] DEBUG -- : Record 172.23.9.0/172.23.9.49 not found

Without your patch, it finds the entry:

D, [2016-04-29T16:53:42.366182 #32624] DEBUG -- : Added a reservation: 172.23.9.49:00:50:XX:XX:XX:XX:XXXXX

In addition foreman_dhcp_brower Plugin shows the subnet being empty with your patch (it's not without).
How can I help with debugging this?

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

@timogoebel: thanks for testing this!

To confirm: you are using isc dhcp (these changes are not relevant for other providers. Other providers can be broken in this branch atm). If so, you should see a list of lease and host records that were processed on startup, does it include the ones you search for?

@theforeman-bot

This comment has been minimized.

Copy link
Member

commented Apr 29, 2016

There were the following issues with the commit message:

  • length of the first commit message line for 9060d7a exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2016

@timogoebel: also, check there are no errors on startup related to dhcp...

@timogoebel

This comment has been minimized.

Copy link
Member

commented May 2, 2016

@witlessbird
Yes, it initializes everything correctly and finds the subnet on startup (and I do use isc-dhcpd)

I, [2016-05-02T08:33:50.924345 #7394]  INFO -- : Successfully initialized 'tftp'
D, [2016-05-02T08:33:50.925335 #7394] DEBUG -- : Added a subnet: 172.23.9.0
[...]
I, [2016-05-02T08:33:50.985827 #7394]  INFO -- : Successfully initialized 'dhcp_isc'
I, [2016-05-02T08:33:50.986187 #7394]  INFO -- : Successfully initialized 'dhcp'
@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

@timogoebel: you should see something similar to:

D, [2016-05-03T06:08:13.500534 #21612] DEBUG -- : Added a subnet: 192.168.42.0
D, [2016-05-03T06:08:13.501780 #21612] DEBUG -- : Added a reservation: 192.168.42.193:52:54:00:8e:5f:08:foreman-test
D, [2016-05-03T06:08:13.501837 #21612] DEBUG -- : Added a reservation: 192.168.42.173:e8:2a:ea:14:88:ef:lucid-nonsense
D, [2016-05-03T06:08:13.501881 #21612] DEBUG -- : Added a reservation: 192.168.42.191:52:54:00:03:00:d8:mco-agent
D, [2016-05-03T06:08:13.501925 #21612] DEBUG -- : Added a reservation: 192.168.42.190:52:54:00:e8:2c:78:mco-master
D, [2016-05-03T06:08:13.501968 #21612] DEBUG -- : Added a reservation: 192.168.42.5:60:a4:4c:d3:b3:e0:space-monster
D, [2016-05-03T06:08:13.502013 #21612] DEBUG -- : Added a reservation: 192.168.42.2:00:24:36:9a:21:b2:stranger-here-myself
D, [2016-05-03T06:08:13.502056 #21612] DEBUG -- : Added a reservation: 192.168.42.171:c8:bc:c8:9d:d0:c5:unwitting-accomplice-lan
D, [2016-05-03T06:08:13.502097 #21612] DEBUG -- : Added a reservation: 192.168.42.170:c8:bc:c8:e3:8a:e6:unwitting-accomplice
D, [2016-05-03T06:08:13.517242 #21612] DEBUG -- : Added a lease record: 192.168.42.240:7c:d1:c3:f2:02:a3
D, [2016-05-03T06:08:13.517353 #21612] DEBUG -- : Added a lease record: 192.168.42.245:58:55:ca:d6:28:2f
D, [2016-05-03T06:08:13.517402 #21612] DEBUG -- : Added a lease record: 192.168.42.227:6c:40:08:e8:18:2b
D, [2016-05-03T06:08:13.517444 #21612] DEBUG -- : Added a lease record: 192.168.42.246:ac:bc:32:ae:86:37
D, [2016-05-03T06:08:13.517485 #21612] DEBUG -- : Added a lease record: 192.168.42.250:d0:03:4b:82:02:f6
D, [2016-05-03T06:08:13.517525 #21612] DEBUG -- : Added a lease record: 192.168.42.223:b8:8d:12:57:f0:13
D, [2016-05-03T06:08:13.517573 #21612] DEBUG -- : Added a lease record: 192.168.42.247:64:bc:0c:48:9d:64
D, [2016-05-03T06:08:13.517614 #21612] DEBUG -- : Added a lease record: 192.168.42.213:a4:77:33:a9:05:d6
D, [2016-05-03T06:08:13.517654 #21612] DEBUG -- : Added a lease record: 192.168.42.242:00:1b:67:07:95:1c
D, [2016-05-03T06:08:13.517701 #21612] DEBUG -- : Deleted a lease record: 192.168.42.247:64:bc:0c:48:9d:64
D, [2016-05-03T06:08:13.517757 #21612] DEBUG -- : Added a lease record: 192.168.42.247:64:bc:0c:48:9d:64
D, [2016-05-03T06:08:13.517797 #21612] DEBUG -- : Deleted a lease record: 192.168.42.213:a4:77:33:a9:05:d6
D, [2016-05-03T06:08:13.517834 #21612] DEBUG -- : Added a lease record: 192.168.42.213:a4:77:33:a9:05:d6
D, [2016-05-03T06:08:13.517871 #21612] DEBUG -- : Deleted a lease record: 192.168.42.242:00:1b:67:07:95:1c
D, [2016-05-03T06:08:13.517907 #21612] DEBUG -- : Added a lease record: 192.168.42.242:00:1b:67:07:95:1c
I, [2016-05-03T06:08:13.517989 #21612]  INFO -- : Successfully initialized 'dhcp_isc'

Note a bunch of host- and lease-records being added (and deleted). Do you see anything like this? If not, can you check that dhcp_isc configuration is pointing to the correct (as in actually used by isc dhcpd) leases file?

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

@timogoebel: additionally, you should periodically (depends on how busy your dhcpd is) see something similar to:

D, [2016-05-03T06:13:02.137977 #21612] DEBUG -- : caught :modify event on /var/lib/dhcpd/dhcpd.leases.
D, [2016-05-03T06:13:02.138993 #21612] DEBUG -- : Deleted a lease record: 192.168.42.242:00:1b:67:07:95:1c
D, [2016-05-03T06:13:02.139123 #21612] DEBUG -- : Added a lease record: 192.168.42.242:00:1b:67:07:95:1c
"dhcpd.leases:access"
@theforeman-bot

This comment has been minimized.

Copy link
Member

commented May 3, 2016

There were the following issues with the commit message:

  • length of the first commit message line for badb660 exceeds 65 characters

If you don't have a ticket number, please create an issue in Redmine, selecting the appropriate project.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented May 3, 2016

Fixed a bunch (not all) of tests. @timogoebel: I may have fixed the issue you were seeing. Out of curiosity -- are you running smart-proxy under ruby 1.8.7?

@timogoebel

This comment has been minimized.

Copy link
Member

commented May 4, 2016

@witlessbird : Thanks. If I find the time today, I'll retest the latest version and let you know how it works.

I'm using default ruby on rhel 7.2.

$ ruby --version
ruby 2.0.0p598 (2014-11-13) [x86_64-linux]
@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

I plan to review it soon if it hasn't been done by then, not forgotten about it.

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

[test]

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

test failure has been fixed in #442.

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2016

rebased, should fix test failures.

@@ -102,15 +101,10 @@ def load_subnet_data
load_subnet
load_subnet_data

content_type :json
record = server.find_record(@subnet.network, params[:record])
log_halt 404, "Record #{params[:network]}/#{params[:record]} not found" unless record
server.del_record @subnet, record

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 27, 2016

Contributor

I'm unsure if this is correct with the native_ms provider - it looks like del_record will return the string response from the execute call to delete the record. Both the libvirt and ISC providers look like they're going to return nil, but the execute response won't be valid JSON if that's what's returned.

This comment has been minimized.

Copy link
@dmitri-d

dmitri-d Jul 28, 2016

Author Member

Agreed, responses to DELETE shouldn't have body anyway.

@@ -21,6 +21,10 @@ def values(*path)
path.empty? ? get_all_values(@root) : get_all_values(get(@root, path))
end

def clear

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 27, 2016

Contributor

Also add a test to test/memory_store_test.rb

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2016

Thanks, it generally looks very good. My only main concern is that the new Proxy::DHCP::ISC::IscStateChangesObserver class doesn't appear to have any test coverage, but it could do with some.

I think we can live without coverage of the platform-specific observers, unless you'd like to mock those too (which is probably quite straight forward), but this contains a mix of logic for parsing includes out of files, plus adding, removing and updating records in the subnet service which is important to have coverage of.

@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2016

Responded to feedback:

I think we can live without coverage of the platform-specific observers, unless you'd like to mock those too

I promise I'll think about how to best test those -- it's a bit tricky as they essentially create an event loop. Can we continue without these tests for the time being please?

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

I think we can live without coverage of the platform-specific observers, unless you'd like to mock those too

I promise I'll think about how to best test those -- it's a bit tricky as they essentially create an event loop. Can we continue without these tests for the time being please?

Indeed, that's fine.

Thanks for the update, the testing is much improved. Subject to #444 fixing tests first, this is reviewed.

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

[test] with json_pure fixed.

end

def close
fd.close unless fd.nil?

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 29, 2016

Contributor

I think @fd also has to be set to nil here, otherwise the old fd is used when hosts_and_leases is called again from leases_recreated:

E, [2016-07-29T10:53:27.693723 #5691] ERROR -- : Error occured when monitoring /tmp/dhcp.leases
D, [2016-07-29T10:53:27.693750 #5691] DEBUG -- : closed stream (IOError)
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/leases_file.rb:13:in `read'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/leases_file.rb:13:in `hosts_and_leases'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/isc_state_changes_observer.rb:35:in `block in leases_recreated'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_common/subnet_service.rb:140:in `block in group_changes'
/home/dcleal/.rvm/rubies/ruby-2.3.0/lib/ruby/2.3.0/monitor.rb:214:in `mon_synchronize'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_common/subnet_service.rb:140:in `group_changes'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/isc_state_changes_observer.rb:27:in `leases_recreated'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/inotify_leases_file_observer.rb:25:in `block (2 levels) in monitor_leases'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/inotify_leases_file_observer.rb:17:in `each'
/home/dcleal/code/foreman/smart-proxy/modules/dhcp_isc/inotify_leases_file_observer.rb:17:in `block in monitor_leases'

This comment has been minimized.

Copy link
@dmitri-d

dmitri-d Jul 29, 2016

Author Member

Duh!

Fixes #2687: Leases file is no longer parsed on every request
  Also: refs 12392, #12359, #12425, #1090
@dmitri-d

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2016

  • Proxy::DHCP::ISC::LeasesFile#close resets file descriptor now
  • Added a test to verify the behavior above
  • rebased
@domcleal

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2016

Merged as 7bd71b5, thanks @witlessbird.

@domcleal domcleal closed this Jul 29, 2016

@dmitri-d dmitri-d deleted the dmitri-d:isc_dhcp-leases-monitoring branch Aug 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.