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 26 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
7 changes: 6 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,13 @@ compiler:
before_install:
# disable rvm, use system Ruby
- rvm reset
# install newer augeasget repo with newer augeas, otherwise ruby-augeas fails (see https://github.com/yast/yast-network/pull/454#issuecomment-253795507)
- sudo add-apt-repository -y ppa:raphink/augeas
- sudo apt-get update
- sudo apt-get install libaugeas-dev libxml2-dev
# end of augeas install
- 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 augeas-lenses libaugeas0 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
8 changes: 8 additions & 0 deletions package/yast2-network.spec
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,12 @@ 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
BuildRequires: augeas-lenses
Requires: augeas-lenses

# testsuite
BuildRequires: rubygem(rspec)
Expand Down Expand Up @@ -91,6 +97,8 @@ rake install DESTDIR="%{buildroot}"
%{yast_schemadir}/autoyast/rnc/networking.rnc
%{yast_schemadir}/autoyast/rnc/host.rnc
%{yast_libdir}/network
%dir %{yast_libdir}/cfa/
%{yast_libdir}/cfa/hosts.rb
%{yast_ydatadir}/network

%dir %{yast_docdir}
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
200 changes: 200 additions & 0 deletions src/lib/cfa/hosts.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,200 @@
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

# The old format used by {Yast::HostClass}.
# @return [Hash{String => Array<String>}] keys are IPs,
# values are lists of lines in /etc/hosts (not names!)
# with whitespace separated hostnames, where the first one is canonical
# and the rest are aliases
#
# For example, the file contents
#
# 1.2.3.4 www.example.org www
# 1.2.3.7 log.example.org log
# 1.2.3.7 sql.example.org sql
#
# is returned as
#
# {
# "1.2.3.4" => "www.example.org www"
# "1.2.3.7" => [
# "log.example.org log",
# "sql.example.org sql"
# ]
# }
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
# @return [Array<String>]
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
# @return [void]
def delete_by_ip(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 a new host entry.
# If more than one entry with the given ip exists
# then it replaces the last instance.
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to replace only the last instance? I think the correct way is to replace all instances.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, Initially I wrongly understand code. But still there is a question. Does it make sense at all? so after this change from three entries:

1.0.0.0 hostA aliasA
1.0.0.0 hostB aliasB
1.0.0.0 hostC aliasC

you get

1.0.0.0 hostD aliasD
1.0.0.0 hostD aliasD
1.0.0.0 hostD aliasD

which looks bogus

Copy link
Member

Choose a reason for hiding this comment

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

It is bogus, you should get

1.0.0.0 hostD aliasD

Ah, now that i think of it, which of hostA hostB hostC is the canonical name anyway? We need to test that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mvidner do you see it intuitive? I really do not expect such behavior. Currently it is used only in one place Host.Update which is in fact also quite confusing. So what Update did? it removed old hostname and adds new hostname to given ip. So from my non-expert point of view I expect that in fact this set_entry is not needed and instead add_entry should be used in this case after remove_hostname (which is currently used ).

Also according to googling it looks like all of them are canonical and all of them will be resolved. So if you use hostA.suse.cz or hostB.suse.cz or hostC.suse.cz it should point you to same ip. And reverse it is first occurence, according to debian discussion - http://lists.debian.org/debian-devel/2004/06/msg00468.html so host for ip will get hostA.suse.cz

Copy link
Member Author

Choose a reason for hiding this comment

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

in fact I expect that such method always have oldhn and always have newhn and as result it just replace old hn with new hn...and maybe their aliases?

Copy link
Member

Choose a reason for hiding this comment

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

I have a suspicion that with the refactoring of Host.Update we have broken something. I will do some manual tests and report back.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I have tested changing

  • just the IP
  • just the name
  • both IP and name at the same time
    and the hosts file came out right in all cases (after I've fixed the leftover set_host→set_entry occurrences)

# @param [String] ip
# @param [String] canonical
# @param [Array<String>] aliases
# @return [void]
def set_entry(ip, canonical, aliases = [])
entries = data.select(ip_matcher(ip))
if entries.empty?
add_entry(ip, canonical, aliases)
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
# @param [String] ip
# @param [String] canonical
# @param [Array<String>] aliases
# @return [void]
def add_entry(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 the only hostname for a given ip, the ip is removed
# If it is canonical name, then the first alias becomes the canonical hostname
# @param [String] hostname
# @return [void]
def delete_hostname(hostname)
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
entry["canonical"] = aliases.first
entry.delete("alias")
entry.delete("alias[]")
aliases_col = entry.collection("alias")
aliases[1..-1].each do |a|
aliases_col.add(a)
end
end
else
reduced_aliases = aliases_for(entry)
reduced_aliases.delete(hostname)
entry.delete("alias")
entry.delete("alias[]")
aliases_col = entry.collection("alias")
aliases[1..-1].each do |a|
aliases_col.add(a)
end
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