Fixes #6412: Trying to remove a DHCP record fails due to an invalid subn...

[pccowboy]

...et check

[lzap]

Hello, thanks. So if I understand this right, if it fails to delete the DHCP record, we will carry on. [test]

[pccowboy]

A small correction - if it fails to find the DHCP record of a deleted host in the subnet under evaluation, it will not attempt to delete it from the subnet under evaluation. This allows it to carry on, and actually delete the record that is intended.

The problem arises due to the evaluation of DHCP records with a status of 'deleted'. These records are evaluated against the subnet that contains the actual host we want to delete, and if they are found, they are removed prior to executing the target deletion.

In the case that triggered this bug, the record with the DHCP deleted status was originally in a different subnet before it was removed. On a subsequent delete run for a second host, the previously deleted host could not be found in the subnet under evaluation, and line 198 never returned a value. This left the return value of find_record_by_hostname as the result of subnet.records.each (an array), which then caused the subnet.delete call to throw an error.

A better fix might be to track the subnets of hosts with a DHCP status of deleted, but this gets the job done without added complexity.

HTH.

[sodabrew]

Suggestion: replace subnet.records.each with a find: http://ruby-doc.org/core-1.8.7/Enumerable.html#method-i-find

subnet.records.find do |v|
  v.options[:hostname] == hostname
end

[sodabrew]

@lzap could you trigger a [test] run of the updated commit?

[domcleal]

[test]

[lzap]

Hey, I want to reproduce, can you elaborate on:

Take three hosts, add them into foreman via API. Then try and remove them

So am I supposed to add three new bare metal hosts via our API, and then delete them via the API?

[pccowboy]

You can do this via the API, or from the web interface. The critical
bit is that there are multiple subnets being managed.

All three hosts should be managed via DHCP.

Setup a DHCP proxy, and have it manage multiple subnets.

Put the first host into it's own subnet, 10.1.60.0.

Put the second and third hosts into subnet 10.1.65.0.

Add all 3 hosts, via web or API. Delete the host in subnet 10.1.60.0.

Delete the next host, in subnet 10.1.65.0. An exception will be
thrown. The delete will fail, unless you have my patch that lets a
delete continue if a DHCP record is not found (#174).

HTH,
David

Lukás( Zapletal mailto:notifications@github.com
Monday, June 30, 2014 3:29 AM

Hey, I want to reproduce, can you elaborate on:

/Take three hosts, add them into foreman via API. Then try and remove
them/

So am I supposed to add three new bare metal hosts via our API, and
then delete them via the API?

---

Reply to this email directly or view it on GitHub
#175 (comment).

[domcleal]

The explanation makes sense to me. This isn't deletion of DHCP records per se, it's just loading subnet data, so seems harmless enough to skip over a deleted marker that we can't find here.

@GregSutcliffe please review too

Marked for 1.5.2 release.

[GregSutcliffe]

This looks fine to me in principle, but it would be nice to get a test added so we don't break it again. @pccowboy @sodabrew does one of you have time to add something?

[sodabrew]

@GregSutcliffe I don't see any tests that exercise the ISC subnet parser...

[GregSutcliffe]

@sodabrew definitely a candidate for adding some then - I can help if needed :)

[sodabrew]

I would ask that you not block this PR today on building a test suite for the ISC DHCP module. It's a worthy goal, but out of scope - this PR is a simple fix to an incorrect programming idiom (array.each { |x| return y if z } is unequivocally wrong).

[GregSutcliffe]

@sodabrew you're entirely correct, it's the other PR that wants a simple test adding - I got a bit zealous there :)

@domcleal +1 to merge

[domcleal]

Merged as 57ea7e8 for Foreman 1.5.2, thanks for the contribution @pccowboy.