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 12 commits
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 .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ before_install:
# disable rvm, use system Ruby
- rvm reset
- wget https://raw.githubusercontent.com/yast/yast-devtools/master/travis-tools/travis_setup.sh
- sh ./travis_setup.sh -p "rake yast2-devtools yast2-testsuite yast2 yast2-storage yast2-proxy yast2-country yast2-packager" -g "rspec:3.3.0 yast-rake gettext rubocop:0.41.2 simplecov:0.10.0 coveralls"
- sh ./travis_setup.sh -p "ruby2.1-dev libaugeas-dev libaugeas rake yast2-devtools yast2-testsuite yast2 yast2-storage yast2-proxy yast2-country yast2-packager" -g "rspec:3.3.0 yast-rake gettext rubocop:0.41.2 simplecov:0.10.0 coveralls cfa cheetah"
script:
- rubocop
- rake check:syntax
Expand Down
2 changes: 0 additions & 2 deletions Rakefile
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
require "yast/rake"

Yast::Tasks.submit_to :sle12sp2

Yast::Tasks.configuration do |conf|
# lets ignore license check for now
conf.skip_license_check << /.*/
Expand Down
5 changes: 5 additions & 0 deletions package/yast2-network.spec
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ Requires: yast2-storage >= 2.21.11
# Packages::vnc_packages
BuildRequires: yast2-packager >= 3.1.47
Requires: yast2-packager >= 3.1.47
# cfa for parsing hosts
BuildRequires: rubygem(%rb_default_ruby_abi:cfa)
Requires: rubygem(%rb_default_ruby_abi:cfa)
# lenses are needed to use cfa
Requires: augeas-lenses

# testsuite
BuildRequires: rubygem(rspec)
Expand Down
10 changes: 2 additions & 8 deletions src/clients/host.rb
Original file line number Diff line number Diff line change
Expand Up @@ -109,15 +109,9 @@ def HostGUI
def ListHandler(_options)
# Command line output Headline
# configuration of hosts
summary = Ops.add(
Ops.add(
"\n" + _("Host Configuration Summary:") + "\n\n",
RichText.Rich2Plain(Host.Summary)
),
"\n"
)
summary = "\n" + _("Host Configuration Summary:") + "\n\n" +
RichText.Rich2Plain(Host.Summary) + "\n"

Builtins.y2debug("%1", summary)
CommandLine.Print(summary)
true
end
Expand Down
6 changes: 3 additions & 3 deletions src/include/network/lan/address.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1530,7 +1530,7 @@ def AddressDialog
ip_changed = LanItems.ipaddr !=
Ops.get_string(@settings, "IPADDR", "")
if ip_changed
Host.set_names(LanItems.ipaddr, [])
Host.remove_ip(LanItems.ipaddr)
Builtins.y2milestone("IP has changed")
end

Expand All @@ -1541,12 +1541,12 @@ def AddressDialog

if @hostname_initial != Ops.get_string(@settings, "HOSTNAME", "") || ip_changed
if Ops.get_string(@settings, "HOSTNAME", "") == ""
Host.set_names(LanItems.ipaddr, [])
Host.remove_ip(LanItems.ipaddr)
else
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
4 changes: 0 additions & 4 deletions src/include/network/services/host.rb
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,6 @@ def HostsMainDialog(standalone)
key = Ops.get_string(row, 1, "")
Host.add_name(key, value)
end
# deleted entries need to be set to [],
# so that ini-agent does not keep them in
# config file (#455862)
Builtins.foreach(deleted_items) { |d| Host.set_names(d, []) }
end
break
else
Expand Down
168 changes: 168 additions & 0 deletions src/lib/cfa/hosts.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
require "yast"
require "yast2/target_file"

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

module CFA
# class representings /etc/hosts file model. It provides helper to manipulate
# with file. It uses CFA framework and Augeas parser.
# @see http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/BaseModel
# @see http://www.rubydoc.info/github/config-files-api/config_files_api/CFA/AugeasParser
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

# returns old format of hosts used in Host module.
# @return [Hash<Array<String>>] format is hash where key is ip and value
Copy link
Member

Choose a reason for hiding this comment

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

I will rewrite this piece

# is array which contain strings and each string represent one line in hosts
# table with comma separated hostnames, where first is canonical and rest
# are aliases
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

# Returns single entry from hosts for given ip or empty array if not found
# @see {#hosts}
def host(ip)
hosts = data.select(ip_matcher(ip))

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

# deletes all occurences of given ip in host table
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

# removes hostname from all entries in hosts table.
# if it is only hostname for given ip, ip is removed
# if it is canonical part, then first alias is used as canonical hostname
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 |pair|
entry = pair[:value]
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

# returns matcher for cfa to find entries with given ip
def ip_matcher(ip)
Matcher.new { |_k, v| v["ipaddr"] == ip }
end

# returns matcher for cfa to find entries with given hostname
def hostname_matcher(hostname)
Matcher.new do |_k, v|
v["canonical"] == hostname || aliases_for(v).include?(hostname)
end
end

# returns aliases as array even if there is only one
def aliases_for(entry)
entry["alias[]"] ? entry.collection("alias").map { |a| a } : [entry["alias"]].compact
end

# generate old format string with first canonical and then aliases
# all separated by space
def single_host_entry(entry)
result = [entry["canonical"]]
result.concat(aliases_for(entry))
result.join(" ")
end

# helper to generate unique id for cfa entry
def unique_id
id = 1
loop do
return id.to_s unless data[id.to_s]
id += 1
end
end
end
end
Loading