Optimize hosts read #454

jreidinger · 2016-10-11T13:30:59Z

WIP, will be blog entry

rationale for this pr is bug report https://bugzilla.suse.com/show_bug.cgi?id=877047
even with this change I get to less then 20% of original time and found more possible optimization in cfa that will even speed it up.

content for blog:
We get bug report, that yast2 network is slow when there is a lot of entries in /etc/hosts file. We take it as opportunity to tost new profiler support in yast. Profiler shows that problem is in SCR calls, which took a lot of time, so another oppurtinity to test new framework cfa with augeas parser. Augeas have already lense for /etc/hosts file, so it is quite easy to add it. In short here are results for network CLI with /etc/hosts containing ~ 10 000 entries:

before:

time yast2 lan.rb list
id  name,           bootproto
0   Intel Ethernet controller, DHCP


real    1m15.735s
user    1m15.076s
sys 0m0.164s

after:

time yast2 lan list
id  name,           bootproto
0   Intel Ethernet controller, DHCP


real    0m19.079s
user    0m18.348s
sys 0m0.244s

We also improved cfa itself
config-files-api/config_files_api#10 which we speed up by 50%.

mchf · 2016-10-11T14:51:06Z

src/modules/Host.rb

-          )
-          next { host => names } if names != []
-        end
+      if Ops.greater_than(SCR.Read(path(".target.size"), CFA::Hosts::PATH), 0)


when doing cleanup this should be replaced too ;-)

well, cfa currently do not have method to recognize if file is not empty or exists. It raise exception. Of course I can use some cli tool to get it

mchf · 2016-10-11T14:54:10Z

src/include/network/lan/address.rb

            else
              Host.Update(
                @hostname_initial,
                Ops.get_string(@settings, "HOSTNAME", ""),
-                [Ops.get_string(@settings, "IPADDR", "")]
+                Ops.get_string(@settings, "IPADDR", "")


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)

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.

mchf · 2016-10-11T14:56:32Z

src/lib/cfa/hosts.rb

+      super(PARSER, PATH, file_handler: file_handler)
+    end
+
+    def hosts


this and some other methods miss documentation.

mchf · 2016-10-11T14:57:13Z

src/modules/Host.rb

@@ -27,6 +27,8 @@
 # Authors:	Michal Svec <msvec@suse.cz>


should be removed ;-)

mchf · 2016-10-11T15:01:10Z

src/lib/cfa/hosts.rb

+    def remove_hostname(hostname)
+      entries = data.select(hostname_matcher(hostname))
+      entries.each do |entry|
+        entry = entry[:value]


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

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

mchf · 2016-10-11T15:02:14Z

src/lib/cfa/hosts.rb

+  private
+
+    def ip_matcher(ip)
+      Matcher.new { |k, v| v["ipaddr"] == ip }


k is not used so it should be _

mchf · 2016-10-11T15:02:25Z

src/lib/cfa/hosts.rb

+    end
+
+    def hostname_matcher(hostname)
+      Matcher.new do |k, v|


mchf · 2016-10-12T09:09:44Z

src/modules/Host.rb

      true
    end

    # Dump the Hosts settings to a map, for autoinstallation use.
    # @return autoinstallation settings
    def Export
-      return {} if @hosts.empty?
+      exported_hosts = @hosts.hosts
+      return {} if exported_hosts.empty?

      # Filter out IPs with empty hostname (so that valid autoyast
      # profile is created)(#335120)
      # FIXME: this also removes records with empty names from @hosts. Such
      # side effect is unexpected and should be removed.


if I get it correctly this FIXME is no longer true

you are right

mvidner · 2016-10-12T10:30:55Z

src/lib/cfa/hosts.rb

+require "cfa/augeas_parser"
+
+module CFA
+  class Hosts < BaseModel


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

…maintenance branch SLE-12-SP2" This reverts commit 6a761c7.

mvidner · 2016-10-12T13:44:24Z

test/data/hosts

+ff02::2         ipv6-allrouters
+ff02::3         ipv6-allhosts
+10.100.128.72   pepa.labs.suse.cz pepa pepa2
+10.254.128.01   pepa1.labs.suse.cz pepa1


The "01" is incidental complexity, make that "1".

it is just string for parsing, I do not care about it much

mvidner · 2016-10-12T13:55:38Z

src/lib/cfa/hosts.rb

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


I will rewrite this piece

coveralls · 2016-10-12T14:03:17Z

Coverage decreased (-22.0%) to 24.651% when pulling 8d9d6b8 on optimize_hosts_read into 9888c9e on master.

mvidner · 2016-10-12T14:06:19Z

src/lib/cfa/hosts.rb

+    def set_host(ip, canonical, aliases = [])
+      entries = data.select(ip_matcher(ip))
+      if entries.empty?
+        log.info "adding new entry for ip #{ip}"


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

hmm, that cause another search thrue tree searching for ip

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

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

jreidinger · 2016-10-13T05:55:11Z

regarding API, I agree with naming and will change it.

But why are you missing those two methods? Does it needed on any place? When I design API I try to avoid adding method that noone need as it is hard to properly design it. E.g. what will be return value? array? or first matching entry? And how it look like? string like now? or hash with canonical and aliases? or only canonical? and for find_by_hostname ( name is confusing for me ) does it return ip or aliases? or hash with all three ( canonical, ip and aliases) ? So if such method someone find useful, then I am not against adding it, when use case is clear.

jreidinger · 2016-10-13T06:05:33Z

@mvidner also thinking about remove_hostname and delete_by_hostname. I also found it confusing. as delete_by_ip removes entry where is given ip. That make sense and is intuitive. But delete_by_hostname do not delete entry, only in specific case. It just remove given hostname from entries where it is available, but keep ip if it have other hostnames.

so for

1.0.0.0 siteA siteB siteC

if I call delete_by_hostname("siteC") it can indicate that it remove whole this line, but it instead do just

1.0.0.0 siteA siteB

mvidner · 2016-10-13T08:04:18Z

BTW regarding the /etc/hosts editor (Network Services →Hostnames, yast2 host) - it uses a "bulk" API: read everything via Host.name_map, and then Host.clear and Host.add_name/Host.remove_ip in a loop.

jreidinger · 2016-10-13T08:14:53Z

@mvidner I know, it is a bit misoptimal as it kills comment placement in file. Better way would be direct operation on hosts model and adding, removing and replacing entries there.

mvidner · 2016-10-13T08:22:48Z

src/lib/cfa/hosts.rb

+    # If it is canonical name, then the first alias becomes the canonical hostname
+    # @param [String] hostname
+    # @return [void]
+    def remove_hostname(hostname)


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

mvidner · 2016-10-13T08:40:12Z

src/lib/cfa/hosts.rb

+
+    # Replaces or adds a new host entry.
+    # If more than one entry with the given ip exists
+    # then it replaces the last instance.


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

mvidner · 2016-10-14T12:51:33Z

I have now run the code and it says Internal Error, undefined method set_host.

This happened when I changed both the IP and the hostname at once.

mvidner · 2016-10-14T13:06:31Z

delete_hostname has no tests, that's why the set_host has escaped. set_entry also has no tests BTW.

mvidner · 2016-10-14T13:13:26Z

Regarding the augeas failure:

ruby-augeas-0.5.0 needs augeas-1.0.0: hercules-team/ruby-augeas@9025d88
and it uses this setup in its Travis file: (hercules-team/ruby-augeas@7a69aa1)

  - sudo add-apt-repository -y ppa:raphink/augeas
  - sudo apt-get update
  - sudo apt-get install libaugeas-dev libxml2-dev

coveralls · 2016-10-14T13:19:24Z

Coverage increased (+1.5%) to 48.07% when pulling 9bdd9af on optimize_hosts_read into 9888c9e on master.

coveralls · 2016-10-14T13:38:22Z

Coverage increased (+1.5%) to 48.07% when pulling e6b65e0 on optimize_hosts_read into 9888c9e on master.

mvidner

Thanks, almost there!
Please add a changelog entry.

coveralls · 2016-10-14T15:04:49Z

Coverage increased (+1.5%) to 48.07% when pulling 63ed52c on optimize_hosts_read into 430be2c on master.

jreidinger · 2016-10-14T15:21:14Z

thanks

mchf · 2016-10-14T22:39:14Z

I've obviously was not fast enough. However, I did some tests. And I get an Internal Error sometimes:

mchf · 2016-10-14T22:40:06Z

The issue was raised in installer even with already merged version of the patch.

jreidinger · 2016-10-17T06:46:58Z

checking

jreidinger · 2016-10-17T07:03:04Z

@mchf fix is in #456

jreidinger added 2 commits October 11, 2016 15:09

use cfa instead of ini agent

a76777f

mark private internal api and present new useful api call

671bc1c

mchf reviewed Oct 11, 2016

View reviewed changes

jreidinger added the blog label Oct 11, 2016

jreidinger added 2 commits October 12, 2016 09:07

fix adding alias in update and test it

183af41

changes from review

69fb68e

mchf reviewed Oct 12, 2016

View reviewed changes

jreidinger added 2 commits October 12, 2016 12:05

chagnes from review

2814e51

update build dependencies

0fea43b

mvidner reviewed Oct 12, 2016

View reviewed changes

jreidinger added 6 commits October 12, 2016 12:52

add comment

c9b506e

debug travis

9cc559f

Revert "adapt Rakefile to submit to correct build service project in …

2891ea7

…maintenance branch SLE-12-SP2" This reverts commit 6a761c7.

more travis debugging

329459d

fix travis

8f87a1d

more fixes to travis and jenkins

1fc89e6

mvidner reviewed Oct 12, 2016

View reviewed changes

jreidinger added 2 commits October 12, 2016 15:50

more fixes to travis and jenkins

10a186a

more fixes to travis and jenkins

ce70f39

mvidner reviewed Oct 12, 2016

View reviewed changes

jreidinger added 2 commits October 12, 2016 15:57

fix ip addr

447cc1b

more fixes to travis and jenkins

8d9d6b8

mvidner reviewed Oct 12, 2016

View reviewed changes

use better names for some API calls (thanks @mvidner)

43ebe40

mvidner reviewed Oct 13, 2016

View reviewed changes

consistent delete keyword

5d4bfaa

fix removing hostname with test

8eb9bfa

jreidinger added 2 commits October 14, 2016 15:13

fix appending hostname in Host#Update

72281b1

try to fix travis

9bdd9af

jreidinger added 2 commits October 14, 2016 15:26

try smaller travis fix

b508d83

revert back smaller travis fix

e6b65e0

mvidner requested changes Oct 14, 2016

View reviewed changes

jreidinger added 2 commits October 14, 2016 16:09

changes

ed6a634

Merge remote-tracking branch 'origin/master' into optimize_hosts_read

63ed52c

mvidner approved these changes Oct 14, 2016

View reviewed changes

jreidinger merged commit 5f6cb6d into master Oct 14, 2016

jreidinger deleted the optimize_hosts_read branch October 14, 2016 15:21

mvidner mentioned this pull request May 17, 2017

Do not raise an internal error in installation when updating /etc/hosts #512

Merged

Optimize hosts read #454

Optimize hosts read #454

Conversation

jreidinger commented Oct 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidner Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreidinger commented Oct 13, 2016

jreidinger commented Oct 13, 2016

mvidner commented Oct 13, 2016 • edited

jreidinger commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvidner commented Oct 14, 2016 • edited

mvidner commented Oct 14, 2016

mvidner commented Oct 14, 2016

coveralls commented Oct 14, 2016

coveralls commented Oct 14, 2016

mvidner left a comment

Choose a reason for hiding this comment

coveralls commented Oct 14, 2016

jreidinger commented Oct 14, 2016

mchf commented Oct 14, 2016 • edited

mchf commented Oct 14, 2016 • edited

jreidinger commented Oct 17, 2016

jreidinger commented Oct 17, 2016

jreidinger commented Oct 11, 2016 •

edited

mvidner Oct 12, 2016 •

edited

mvidner commented Oct 13, 2016 •

edited

mvidner commented Oct 14, 2016 •

edited

mchf commented Oct 14, 2016 •

edited

mchf commented Oct 14, 2016 •

edited