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 #7766 - performance enhancements for DHCP on windows #294
Conversation
This is in most part based on #277. I refactored the code to be more testable, added a couple of test and fixed broken tests. |
@Reedler, @briangann: any chance you could give this a try? |
count = 0 | ||
found = false | ||
anIP = nil | ||
content.each do |line| |
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.
I guess this is the line Oliver's referring to here: http://projects.theforeman.org/issues/7766#change-44781
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.
I will give this a try, I am running 1.8.7 Ruby so I didn't hit that .each bug/deprecation.Code looks good to me, kudos to the rework for testability 😀BrianAt Jun 24, 2015, 7:30:49 AM, Dominic Cleal wrote:In modules/dhcp/providers/server/native_ms.rb:
std_in.close unless std_in.nil?
std_out.close unless std_in.nil?
std_err.close unless std_in.nil?
end
report msg, response, error_only
response
- end
convert scriptfile output into a hash for fast queries
- def parse_script_output_to_hash content
processed = {}
queryResults = ""
count = 0
found = false
anIP = nil
content.each do |line|
I guess this is the line Oliver's referring to here: http://projects.theforeman.org/issues/7766#change-44781
—Reply to this email directly or view it on GitHub.
fixed issues when running on Ruby > 1.8.7 (there were a few instances of String#each calls). |
when /For vendor class \[([^\]]+)\]:/ | ||
options[:vendor] = "<#{$1}>" | ||
when /OptionId : (\d+)/ | ||
#logger.debug "optionid parsed is " + $1 |
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.
leftover debug
@domcleal: did a bunch of small refactorings you suggested. @briangann: could you give these changes a try please? |
response = std_out.readlines | ||
response += std_err.readlines | ||
end | ||
rescue TimeoutError | ||
raise Proxy::DHCP::Error.new("Netsh did not respond within #{tsecs} seconds") | ||
raise puts("Netsh did not respond within #{timeout} seconds") |
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.
raise puts
? Shouldn't it raise an exception? (Plus puts returns nil, IIRC)
@domcleal: fixed. |
Thanks, looks ready if somebody such as @briangann would be kind enough to give it another test. |
@witlessbird @domcleal I have a spare Windows server 2012 box I could use, but I'm not sure how to test this, if you can provide with a script that creates reservations I could help? |
I wrote this code a long time ago when I had ms dhcp skills. Feel free to use it or if you want I can change license or whatever. Essentially it parses the entire dhcp dump into a ruby object. https://github.com/logicminds/msdhcpdump/blob/master/test.rb (this is the test code with test dump file) Note: this was written with the little ruby skills I had at the time so don't poke too much fun at it. |
@elobato: I don't have a script. I would set up dhcp service on that machine and tried provisioning a a few vms, or something like that... |
the new code works - it retains the bulk of the original PR. performance looks great. |
No description provided.