Match the DHCP specification of last-wins leases #155

GregSutcliffe · 2014-05-09T15:50:39Z

Just sending this PR mainly to get the tests to run on Jenkins - I'm not entirely happy with it yet...

Discussion of the issue can be found here: http://projects.theforeman.org/issues/5648

This attempts to preserve the current proxy behaviour of indexing DHCP records by IP, and only re-indexes by mac (thus updating/removing duplicates) when specifically searching for a mac record.

GregSutcliffe · 2014-05-09T16:49:19Z

Alright, tests pass - I think this might be mergable - but input on the direction we should be going is welcome

lzap · 2014-05-12T10:55:08Z

lib/proxy/dhcp/subnet.rb

@@ -91,7 +100,7 @@ def [] record
    end

    def has_mac? mac
-      records.each {|r| return r if r.mac == mac.downcase }
+      records(:by_mac).each {|r| return r if r.mac == mac.downcase }


Hmmm what's the point of building the whole hash while we use it on this line only to iterate it. That's zero time saved and extra memory consumed. I must be missing something.

I think you want to search it in O(1) rather than O(n) here :-)

In other words:

records(:by_mac)[mac.downcase]

Hmmm disregard the above, I see the point.

For clarity to other reviewers: yeah, the issue is that the records hash is keyed on IP by default. That causes issues when you GET /dhcp/:network/:mac as the same IP might belong to two macs, and you'll only get one of them.

I considered altering the code to key on mac, but then you hit the same problem when you GET /dhcp/:network/:ip. So instead I re-index on the fly for mac-centric queries. It seems to work from my tests, but I'd appreciate confirmation from others.

lzap · 2014-05-14T11:31:46Z

Ok taking for testing together with theforeman/foreman#1430 in once.

GregSutcliffe · 2014-05-14T17:30:10Z

@lzap new version just been pushed - it turns out I was relying on implicit Hash sorting in ruby 1.9 which breaks on 1.8, so I've added explicit sorting of the records, to make sure we keep them in the order they were listed in the file.

mburns72h · 2014-05-15T11:57:40Z

I've verified that this works in my environment in combination with the above mentioned foreman patch ( theforeman/foreman#1430 )

GregSutcliffe · 2014-05-15T16:25:32Z

@lzap @mburns72h New extra commit added to handle the failure when deleting hosts - see the referenced Redmine issue for an explanation of why this is necessary. Please re-test and confirm

lzap · 2014-05-15T18:37:25Z

Finally had time to finish my new discovery PR testing setup and reproduced:

ERF12-4395 [ProxyAPI::ProxyException]: Unable to retrieve DHCP entry for 52:54:00:bf:fe:77 ([Net::LeaseConflict]: 52:54:00:bf:fe:77/192.168.100.14) for proxy https://foreman.virtual.lan:8443/dhcp

Applying patches. Sorry for the delay.

mburns72h · 2014-05-15T18:48:45Z

tested the update and it works as expected, hosts delete correctly

lzap · 2014-05-15T19:29:59Z

Got this when testing:

Started POST "/discovers" for 192.168.100.17 at 2014-05-15 21:27:54 +0200
Processing by DiscoversController#create as */*
  Parameters: {"ip"=>"192.168.100.17"}
Failed to import facts for Host::Discovered: super: no superclass method `importFacts' for #<Host::Discovered:0x00000008c75940>
  Rendered text template (0.0ms)
Completed 400 Bad Request in 2937ms (Views: 0.7ms | ActiveRecord: 1.8ms)

And when I click on Refresh Facts, I get:

Started GET "/discovers/52540028bbfa/refresh_facts" for 192.168.100.1 at 2014-05-15 21:29:26 +0200
Processing by DiscoversController#refresh_facts as HTML
  Parameters: {"id"=>"52540028bbfa"}
Operation FAILED: Could not get facts from proxy: the scheme http does not accept registry part: :8443 (or bad hostname?)
  Rendered common/500.html.erb within layouts/application (2.8ms)
  Rendered layouts/base.html.erb (1.0ms)
Completed 500 Internal Server Error in 16ms (Views: 4.8ms | ActiveRecord: 2.0ms)

lzap · 2014-05-15T19:52:56Z

Hmmm so when I click on Refresh Facts, I get two requests:

==> /var/log/foreman/production.log <==


Started GET "/discovers/52540028bbfa/refresh_facts" for 192.168.100.1 at 2014-05-15 21:48:08 +0200
Processing by DiscoversController#refresh_facts as HTML
  Parameters: {"id"=>"52540028bbfa"}
retrieving facts from proxy on ip: 
Operation FAILED: Could not get facts from proxy: the scheme http does not accept registry part: :8443 (or bad hostname?)
  Rendered common/500.html.erb within layouts/application (2.7ms)
  Rendered layouts/base.html.erb (0.9ms)
Completed 500 Internal Server Error in 17ms (Views: 8.4ms | ActiveRecord: 1.5ms)

==> /var/log/httpd/foreman.virtual.lan_access_ssl.log <==
192.168.100.1 - - [15/May/2014:21:48:08 +0200] "GET /discovers/52540028bbfa/refresh_facts HTTP/1.1" 500 1417 "https://foreman.virtual.lan/discovers/52540028bbfa" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.132 Safari/537.36"
192.168.100.1 - - [15/May/2014:21:48:09 +0200] "GET /assets/application-a9b9547202305b784ffdecf30aa59b20.js HTTP/1.1" 200 389485 "https://foreman.virtual.lan/discovers/52540028bbfa/refresh_facts" "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/34.0.1847.132 Safari/537.36"

==> /var/log/foreman/production.log <==


Started POST "/discovers" for 192.168.100.17 at 2014-05-15 21:48:10 +0200
Processing by DiscoversController#create as */*
  Parameters: {"ip"=>"192.168.100.17"}
retrieving facts from proxy on ip: 192.168.100.17
Failed to import facts for Host::Discovered: super: no superclass method `importFacts' for #<Host::Discovered:0x000000088d0bf0>
  Rendered text template (0.0ms)
Completed 400 Bad Request in 2879ms (Views: 0.6ms | ActiveRecord: 1.1ms)

==> /var/log/httpd/foreman.virtual.lan_access_ssl.log <==
192.168.100.17 - - [15/May/2014:21:48:10 +0200] "POST /discovers HTTP/1.1" 400 127 "-" "-"

I noticed the first one is missing the IP address. The discovered host has no facts with it. Not sure if this is different bug. Will investigate tomorrow.

lzap · 2014-05-16T09:37:59Z

Ok got it, I was testing with 1.2 (old) discovery.

I was able to reproduce the bug and then with both patches (this and theforeman/foreman#1430) it was working. +1 Good catch, work, report. Thanks guys.

domcleal · 2014-05-16T11:42:41Z

Looks sensible, but I'm concerned about the use of data structures inside the subnet and lack of tests for the first commit.

GregSutcliffe · 2014-05-16T14:32:20Z

@domcleal updated for your comments, thanks for the style cleanup

GregSutcliffe · 2014-05-19T11:39:48Z

@lzap can you retest?

lzap · 2014-05-19T12:31:58Z

Ok testing now, please do not change the code anymore, only add more unit tests if needed :-)

lzap · 2014-05-19T12:37:19Z

lib/proxy/dhcp/subnet.rb

+      rescue
+        nil
+      end
+    end


Hmmm this could be in one block maybe? Or do I miss why is this in two?

validate_mac and validate_ip return errors, so you need to rescue them individually, I think. Otherwise the raise in validate_mac will prevent the validate_ip check from running at all.

self.load unless loaded? return has_mac?(record, type) if (validate_mac(record) rescue nil) return has_ip?(record, type) if (validate_ip(record) rescue nil)

thats a lot neater, done.

domcleal · 2014-05-19T12:42:37Z

untested, looks fine

lzap · 2014-05-19T13:53:35Z

I've just tested this. Provisioning do work, no conflicts at all.

domcleal · 2014-05-20T10:29:51Z

Merged as 2080b2e, ad2b465 for Foreman 1.5.1. Thanks @GregSutcliffe.

lzap reviewed May 12, 2014
View reviewed changes

GregSutcliffe changed the title ~~Do Not Merge: Match the DHCP specification of last-wins leases~~ Match the DHCP specification of last-wins leases May 16, 2014

lzap reviewed May 19, 2014
View reviewed changes

GregSutcliffe added 2 commits May 19, 2014 13:49

Fixes #5648 - Match the DHCP specification of last-lease-wins

e3e7518

Fixes #5739 - Only look at reservations when deleting a DHCP record

103972c

domcleal closed this May 20, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match the DHCP specification of last-wins leases #155

Match the DHCP specification of last-wins leases #155

GregSutcliffe commented May 9, 2014

GregSutcliffe commented May 9, 2014

lzap May 12, 2014

lzap May 12, 2014

lzap May 12, 2014

lzap May 12, 2014

GregSutcliffe May 12, 2014

lzap commented May 14, 2014

GregSutcliffe commented May 14, 2014

mburns72h commented May 15, 2014

GregSutcliffe commented May 15, 2014

lzap commented May 15, 2014

mburns72h commented May 15, 2014

lzap commented May 15, 2014

lzap commented May 15, 2014

lzap commented May 16, 2014

domcleal commented May 16, 2014

GregSutcliffe commented May 16, 2014

GregSutcliffe commented May 19, 2014

lzap commented May 19, 2014

lzap May 19, 2014

GregSutcliffe May 19, 2014

domcleal May 19, 2014

GregSutcliffe May 19, 2014

domcleal commented May 19, 2014

lzap commented May 19, 2014

domcleal commented May 20, 2014

Match the DHCP specification of last-wins leases #155

Match the DHCP specification of last-wins leases #155

Conversation

GregSutcliffe commented May 9, 2014

GregSutcliffe commented May 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented May 14, 2014

GregSutcliffe commented May 14, 2014

mburns72h commented May 15, 2014

GregSutcliffe commented May 15, 2014

lzap commented May 15, 2014

mburns72h commented May 15, 2014

lzap commented May 15, 2014

lzap commented May 15, 2014

lzap commented May 16, 2014

domcleal commented May 16, 2014

GregSutcliffe commented May 16, 2014

GregSutcliffe commented May 19, 2014

lzap commented May 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented May 19, 2014

lzap commented May 19, 2014

domcleal commented May 20, 2014