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

Optimize hosts read #454

Merged
merged 28 commits into from
Oct 14, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
a76777f
use cfa instead of ini agent
jreidinger Oct 11, 2016
671bc1c
mark private internal api and present new useful api call
jreidinger Oct 11, 2016
183af41
fix adding alias in update and test it
jreidinger Oct 12, 2016
69fb68e
changes from review
jreidinger Oct 12, 2016
2814e51
chagnes from review
jreidinger Oct 12, 2016
0fea43b
update build dependencies
jreidinger Oct 12, 2016
c9b506e
add comment
jreidinger Oct 12, 2016
9cc559f
debug travis
jreidinger Oct 12, 2016
2891ea7
Revert "adapt Rakefile to submit to correct build service project in …
jreidinger Oct 12, 2016
329459d
more travis debugging
jreidinger Oct 12, 2016
8f87a1d
fix travis
jreidinger Oct 12, 2016
1fc89e6
more fixes to travis and jenkins
jreidinger Oct 12, 2016
10a186a
more fixes to travis and jenkins
jreidinger Oct 12, 2016
ce70f39
more fixes to travis and jenkins
jreidinger Oct 12, 2016
447cc1b
fix ip addr
jreidinger Oct 12, 2016
8d9d6b8
more fixes to travis and jenkins
jreidinger Oct 12, 2016
b5fd998
ensure that hosts are not nil
jreidinger Oct 12, 2016
daa9755
DRY code (thanks @mvidner)
jreidinger Oct 12, 2016
c460b5b
Improved API docs
mvidner Oct 12, 2016
43ebe40
use better names for some API calls (thanks @mvidner)
jreidinger Oct 13, 2016
5d4bfaa
consistent delete keyword
jreidinger Oct 13, 2016
8eb9bfa
fix removing hostname with test
jreidinger Oct 14, 2016
72281b1
fix appending hostname in Host#Update
jreidinger Oct 14, 2016
9bdd9af
try to fix travis
jreidinger Oct 14, 2016
b508d83
try smaller travis fix
jreidinger Oct 14, 2016
e6b65e0
revert back smaller travis fix
jreidinger Oct 14, 2016
ed6a634
changes
jreidinger Oct 14, 2016
63ed52c
Merge remote-tracking branch 'origin/master' into optimize_hosts_read
jreidinger Oct 14, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/include/network/lan/address.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1546,7 +1546,7 @@ def AddressDialog
Host.Update(
@hostname_initial,
Ops.get_string(@settings, "HOSTNAME", ""),
[Ops.get_string(@settings, "IPADDR", "")]
Ops.get_string(@settings, "IPADDR", "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check that in details yet so it is more less note for me. Here you change semantics (list of ips -> single ip)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I change API because it is used only as single parameter. I think it do not make much sense to change hostname for more ips, it means you have one hostname for more ips which looks bogus for me.

)
end
end
Expand Down
147 changes: 147 additions & 0 deletions src/lib/cfa/hosts.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,147 @@
require "yast"
require "yast2/target_file"

require "cfa/base_model"
require "cfa/matcher"
require "cfa/augeas_parser"

module CFA
class Hosts < BaseModel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the people that did not design CFA::BaseModel, a comment with the full URL of its API docs will be useful: http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/BaseModel

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

PARSER = AugeasParser.new("hosts.lns")
PATH = "/etc/hosts".freeze
include Yast::Logger

def initialize(file_handler: nil)
super(PARSER, PATH, file_handler: file_handler)
end

def hosts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and some other methods miss documentation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

matcher = Matcher.new { |k,v| k =~ /^\d*$/ }
data.select(matcher).each_with_object({}) do |host, result|
entry = host[:value]
result[entry["ipaddr"]] ||= []
result[entry["ipaddr"]] << single_host_entry(entry)
end
end

def host(ip)
hosts = data.select(ip_matcher(ip))

hosts.map do |host|
single_host_entry(host[:value])
end
end

def delete_host(ip)
entries = data.select(ip_matcher(ip))
if entries.empty?
log.info "no entry to delete for ip #{ip}"
return
end

if entries.size > 1
log.info "delete host with ip '#{ip}' removes more then one entry"
end

entries.each do |e|
log.info "deleting record #{e.inspect}"
data.delete(e[:key])
end
end

# replaces or adds new host entry. If more then one entry with given ip exists
# then replaces the last instance
def set_host(ip, canonical, aliases = [])
entries = data.select(ip_matcher(ip))
if entries.empty?
log.info "adding new entry for ip #{ip}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahem, add_entry(ip, canonical, aliases) instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, that cause another search thrue tree searching for ip

Copy link
Member

@mvidner mvidner Oct 12, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? The body of this if block is an exact copy of the body of that method, plus return.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, right, add_entry do not search for existing ips, so I will adapt

entry_line = AugeasTree.new
entry_line["ipaddr"] = ip
entry_line["canonical"] = canonical
aliases_col = entry_line.collection("alias")
aliases.each do |a|
aliases_col.add(a)
end
data.add(unique_id, entry_line)
return
end

if entries.size > 1
log.info "more then one entry with ip '#{ip}'. Replacing last one."
end

entry = entries.last[:value]
entry["ipaddr"] = ip
entry["canonical"] = canonical
# clear previous aliases
entry.delete("alias")
entry.delete("alias[]")
aliases_col = entry.collection("alias")
aliases.each do |a|
aliases_col.add(a)
end
end

# adds new entry, even if it exists
def add_host(ip, canonical, aliases = [])
log.info "adding new entry for ip #{ip}"
entry_line = AugeasTree.new
entry_line["ipaddr"] = ip
entry_line["canonical"] = canonical
aliases_col = entry_line.collection("alias")
aliases.each do |a|
aliases_col.add(a)
end
data.add(unique_id, entry_line)
end

def remove_hostname(hostname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to delete_name (my original suggestion of delete_by_name was wrong)

entries = data.select(hostname_matcher(hostname))
entries.each do |entry|
entry = entry[:value]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overwriting variable used for iterating in loop is usually not considered to be a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not? in ruby it is just variable assigned to some value, but I can create new one if you are more comfortable with it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if entry["canonical"] == hostname
aliases = aliases_for(entry)
if aliases.empty?
delete_host(entry["ipaddr"])
else
set_host(entry["ipaddr"], aliases.first, aliases[1..-1])
end
else
reduced_aliases = aliases_for(entry)
reduced_aliases.delete(hostname)
set_host(entry["ipaddr"], entry["canonical"], reduced_aliases)
end
end
end

private

def ip_matcher(ip)
Matcher.new { |k, v| v["ipaddr"] == ip }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

k is not used so it should be _

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

end

def hostname_matcher(hostname)
Matcher.new do |k, v|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

v["canonical"] == hostname || aliases_for(v).include?(hostname)
end
end

def aliases_for(entry)
entry["alias[]"] ? entry.collection("alias").map{ |a| a } : [entry["alias"]].compact
end

def single_host_entry(entry)
result = [entry["canonical"]]
result.concat(aliases_for(entry))
result.join(" ")
end

def unique_id
id = 1
loop do
return id.to_s unless data[id.to_s]
id += 1
end
end
end
end
Loading