From db293cf3b9e9dd129d8c65aa39272425651addae Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Mon, 29 Oct 2007 12:31:01 -0500 Subject: Fixing a bit of indentation and commenting in the xmlrpc/client file --- lib/puppet/network/xmlrpc/client.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/puppet/network/xmlrpc/client.rb b/lib/puppet/network/xmlrpc/client.rb index c652dbf8d..ab4117b0e 100644 --- a/lib/puppet/network/xmlrpc/client.rb +++ b/lib/puppet/network/xmlrpc/client.rb @@ -128,12 +128,12 @@ module Puppet::Network 120 # a two minute timeout, instead of 30 seconds ) - # We overwrite the uninitialized @http here with a cached one. + # We overwrite the uninitialized @http here with a cached one. key = "%s%s" % [hash[:Server], hash[:Port]] if @@http_cache[key] - @http = @@http_cache[key] + @http = @@http_cache[key] else - @@http_cache[key] = @http + @@http_cache[key] = @http end end @@ -150,4 +150,3 @@ module Puppet::Network end end end - -- cgit From 826efe8d453c1cc45a980603fffc10c91fa0e267 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Wed, 31 Oct 2007 11:34:06 -0500 Subject: The configurations should now be functional again -- file recursion was previously not working, because the relationship graph was setting itself as a resource's primary configuration, which caused it to try creating its own relationship graph. I've now found that the current code is about 5x slower than the released code, so now I hope to resolve that. --- lib/puppet/node/configuration.rb | 13 ++++++++++++- lib/puppet/transaction.rb | 7 +------ lib/puppet/type/pfile.rb | 9 +++------ 3 files changed, 16 insertions(+), 13 deletions(-) (limited to 'lib') diff --git a/lib/puppet/node/configuration.rb b/lib/puppet/node/configuration.rb index f2bbfca00..1365767b6 100644 --- a/lib/puppet/node/configuration.rb +++ b/lib/puppet/node/configuration.rb @@ -33,6 +33,11 @@ class Puppet::Node::Configuration < Puppet::PGraph # that the host configuration needs. attr_accessor :host_config + # Whether this graph is another configuration's relationship graph. + # We don't want to accidentally create a relationship graph for another + # relationship graph. + attr_accessor :is_relationship_graph + # Add classes to our class list. def add_class(*classes) classes.each do |klass| @@ -56,7 +61,7 @@ class Puppet::Node::Configuration < Puppet::PGraph else @resource_table[ref] = resource end - resource.configuration = self + resource.configuration = self unless is_relationship_graph add_vertex!(resource) end end @@ -167,6 +172,9 @@ class Puppet::Node::Configuration < Puppet::PGraph @transient_resources << resource if applying? add_resource(resource) + if @relationship_graph + @relationship_graph.add_resource(resource) unless @relationship_graph.resource(resource.ref) + end resource end @@ -273,6 +281,8 @@ class Puppet::Node::Configuration < Puppet::PGraph # Create a graph of all of the relationships in our configuration. def relationship_graph + raise(Puppet::DevError, "Tried get a relationship graph for a relationship graph") if self.is_relationship_graph + unless defined? @relationship_graph and @relationship_graph # It's important that we assign the graph immediately, because # the debug messages below use the relationships in the @@ -281,6 +291,7 @@ class Puppet::Node::Configuration < Puppet::PGraph # then we get into an infinite loop. @relationship_graph = Puppet::Node::Configuration.new @relationship_graph.host_config = host_config? + @relationship_graph.is_relationship_graph = true # First create the dependency graph self.vertices.each do |vertex| diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb index 3c72f6e97..e30159c24 100644 --- a/lib/puppet/transaction.rb +++ b/lib/puppet/transaction.rb @@ -202,12 +202,7 @@ class Transaction end if children - children.each do |child| - child.finish - # Make sure that the vertex is in the relationship graph. - relationship_graph.add_resource(child) unless relationship_graph.resource(child.ref) - child.configuration = relationship_graph - end + children.each { |child| child.finish } @generated += children return children end diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb index f0a805525..73c60bd14 100644 --- a/lib/puppet/type/pfile.rb +++ b/lib/puppet/type/pfile.rb @@ -574,9 +574,8 @@ module Puppet # Create a new file or directory object as a child to the current # object. def newchild(path, local, hash = {}) - unless configuration - raise Puppet::DevError, "File recursion cannot happen without a configuration" - end + raise(Puppet::DevError, "File recursion cannot happen without a configuration") unless configuration + # make local copy of arguments args = symbolize_options(@arghash) @@ -655,9 +654,7 @@ module Puppet # LAK:FIXME This shouldn't be necessary, but as long as we're # modeling the relationship graph specifically, it is. configuration.relationship_graph.add_edge! self, child - unless child.parent - raise "Did not set parent of %s" % child.ref - end + return child end -- cgit From ef9949502a1e393d14e8289c507ee3f3509a5350 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Wed, 31 Oct 2007 12:54:28 -0500 Subject: Caching the 'parent' value, which resulted in a drastic performance increase. However, profiling has shown that GRATR just isn't going to cut it. I think I'm going to have to replace it as my graphing base class to avoid the horrible, horrible performance I keep encountering. --- lib/puppet/type.rb | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) (limited to 'lib') diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb index 0a6ccdaa4..b7ff1f664 100644 --- a/lib/puppet/type.rb +++ b/lib/puppet/type.rb @@ -316,19 +316,22 @@ class Type def parent return nil unless configuration - # This is kinda weird. - if implicit? - parents = configuration.relationship_graph.adjacent(self, :direction => :in) - else - parents = configuration.adjacent(self, :direction => :in) - end - if parents - # We should never have more than one parent, so let's just ignore - # it if we happen to. - return parents.shift - else - return nil + unless defined?(@parent) + # This is kinda weird. + if implicit? + parents = configuration.relationship_graph.adjacent(self, :direction => :in) + else + parents = configuration.adjacent(self, :direction => :in) + end + if parents + # We should never have more than one parent, so let's just ignore + # it if we happen to. + @parent = parents.shift + else + @parent = nil + end end + @parent end # Return the "type[name]" style reference. -- cgit From 3f21e93599653e3a6c82ab0a131ce250503a771e Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Wed, 7 Nov 2007 16:25:04 -0600 Subject: Adding a new graphing base class, because the GRATR stuff is just too slow. This class has just about no iteration, and vertex deletation is dramatically (as in, 1000x) faster). Here are the results of some very simplistic graph operations: Vertex tests (add and remove 1000 vertices): gratr: Add: 0.01; Remove: 9.70 simple: Add: 0.02; Remove: 0.01 Edge tests (add and remove 1000 edges): gratr: Add: 0.02; Remove: 0.03 simple: Add: 0.07; Remove: 0.02 I expect I can get the cost of the edge addition down some, but even as it is, it's a couple of orders of magnitude faster. This doesn't even count things like searching, which I did some other testing on and got consistently faster results (somewhere between 1.5x and 1500x faster, depending on how the test was set up and how big the graph was). --- lib/puppet/simple_graph.rb | 177 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 lib/puppet/simple_graph.rb (limited to 'lib') diff --git a/lib/puppet/simple_graph.rb b/lib/puppet/simple_graph.rb new file mode 100644 index 000000000..c8fa40e06 --- /dev/null +++ b/lib/puppet/simple_graph.rb @@ -0,0 +1,177 @@ +# Created by Luke A. Kanies on 2007-11-07. +# Copyright (c) 2007. All rights reserved. + +require 'puppet/external/gratr/dot' +require 'puppet/relationship' +require 'puppet/external/gratr/search' + +# A hopefully-faster graph class to replace the use of GRATR. +class Puppet::SimpleGraph + include GRATR::Graph::Search + + # An internal class for handling a vertex's edges. + class VertexWrapper + attr_accessor :in, :out, :vertex + + # Remove all references to everything. + def clear + @adjacencies[:in].clear + @adjacencies[:out].clear + @vertex = nil + end + + def initialize(vertex) + @vertex = vertex + @adjacencies = {:in => Hash.new { |h,k| h[k] = [] }, :out => Hash.new { |h,k| h[k] = [] }} + #@adjacencies = {:in => [], :out => []} + end + + # Find adjacent vertices or edges. + def adjacent(options) + direction = options[:direction] || :out + options[:type] ||= :vertices + + return @adjacencies[direction].values.flatten if options[:type] == :edges + + return @adjacencies[direction].keys + end + + # Add an edge to our list. + def add_edge(direction, edge) + @adjacencies[direction][other_vertex(direction, edge)] << edge + end + + # Return all known edges. + def edges + [:in, :out].collect { |dir| @adjacencies[dir].values }.flatten + end + + # Test whether we share an edge with a given vertex. + def has_edge?(direction, vertex) + return true if @adjacencies[direction][vertex].length > 0 + return false + end + + # The other vertex in the edge. + def other_vertex(direction, edge) + method = direction == :in ? :source : :target + edge.send(method) + end + + # Remove an edge from our list. Assumes that we've already checked + # that the edge is valid. + def remove_edge(direction, edge) + @adjacencies[direction][other_vertex(direction, edge)].delete(edge) + end + end + + def initialize + @vertices = {} + @edges = [] + end + + # Clear our graph. + def clear + @vertices.each { |vertex, wrapper| wrapper.clear } + @vertices.clear + @edges.clear + end + + # Whether our graph is directed. Always true. (Used by the GRATR search lib.) + def directed? + true + end + + # Return the size of the graph. Used by GRATR. + def size + @vertices.length + end + + # Return the graph as an array. Again, used by GRATR. + def to_a + @vertices.keys + end + + # Add a new vertex to the graph. + def add_vertex!(vertex) + return false if vertex?(vertex) + setup_vertex(vertex) + true # don't return the VertexWrapper instance. + end + + # Remove a vertex from the graph. + def remove_vertex!(vertex) + return nil unless vertex?(vertex) + @vertices[vertex].edges.each { |edge| remove_edge!(edge) } + @vertices[vertex].clear + @vertices.delete(vertex) + end + + # Test whether a given vertex is in the graph. + def vertex?(vertex) + @vertices.include?(vertex) + end + + # Return a list of all vertices. + def vertices + @vertices.keys + end + + # Add a new edge. The graph user has to create the edge instance, + # since they have to specify what kind of edge it is. + def add_edge!(source, target = nil) + if target + edge = Puppet::Relationship.new(source, target) + else + edge = source + end + [edge.source, edge.target].each { |vertex| setup_vertex(vertex) unless vertex?(vertex) } + @vertices[edge.source].add_edge :out, edge + @vertices[edge.target].add_edge :in, edge + @edges << edge + true + end + + # Find a matching edge. Note that this only finds the first edge, + # not all of them or whatever. + def edge(source, target) + @edges.each_with_index { |test_edge, index| return test_edge if test_edge.source == source and test_edge.target == target } + end + + # Is there an edge between the two vertices? + def edge?(source, target) + return false unless vertex?(source) and vertex?(target) + + @vertices[source].has_edge?(:out, target) + end + + def edges + @edges.dup + end + + # Remove an edge from our graph. + def remove_edge!(edge) + @vertices[edge.source].remove_edge(:out, edge) + @vertices[edge.target].remove_edge(:in, edge) + + # Here we are looking for an exact edge, so we don't want to use ==, because + # it's too darn expensive (in testing, deleting 3000 edges went from 6 seconds to + # 0.05 seconds with this change). + @edges.each_with_index { |test_edge, index| @edges.delete_at(index) and break if edge.equal?(test_edge) } + nil + end + + # Find adjacent edges. + def adjacent(vertex, options = {}) + return [] unless wrapper = @vertices[vertex] + return wrapper.adjacent(options) + end + + private + + # An internal method that skips the validation, so we don't have + # duplicate validation calls. + def setup_vertex(vertex) + @vertices[vertex] = VertexWrapper.new(vertex) + end +end -- cgit From 065a1d0281ba326674e37a00d8ced1e3a2dd57e4 Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Wed, 7 Nov 2007 17:48:50 -0600 Subject: Switching the graph base class from GRATR::Digraph to Puppet::SimpleGraph, which should dramatically enhance performance. It should be largely functionally equivalent, with the only difference being that edges are no longer deduplicated. --- lib/puppet/node/configuration.rb | 3 +- lib/puppet/pgraph.rb | 13 ++----- lib/puppet/simple_graph.rb | 82 ++++++++++++++++++++++++++++++++++++++-- lib/puppet/transaction.rb | 4 +- 4 files changed, 86 insertions(+), 16 deletions(-) (limited to 'lib') diff --git a/lib/puppet/node/configuration.rb b/lib/puppet/node/configuration.rb index 1365767b6..e49090d70 100644 --- a/lib/puppet/node/configuration.rb +++ b/lib/puppet/node/configuration.rb @@ -83,6 +83,7 @@ class Puppet::Node::Configuration < Puppet::PGraph transaction.addtimes :config_retrieval => @retrieval_duration + begin transaction.evaluate rescue Puppet::Error => detail @@ -304,7 +305,7 @@ class Puppet::Node::Configuration < Puppet::PGraph # Lastly, add in any autorequires @relationship_graph.vertices.each do |vertex| vertex.autorequire.each do |edge| - unless @relationship_graph.edge?(edge) + unless @relationship_graph.edge?(edge.source, edge.target) # don't let automatic relationships conflict with manual ones. unless @relationship_graph.edge?(edge.target, edge.source) vertex.debug "Autorequiring %s" % [edge.source] @relationship_graph.add_edge!(edge) diff --git a/lib/puppet/pgraph.rb b/lib/puppet/pgraph.rb index ca45aa2b3..49fd21401 100644 --- a/lib/puppet/pgraph.rb +++ b/lib/puppet/pgraph.rb @@ -4,11 +4,13 @@ require 'puppet/external/gratr/digraph' require 'puppet/external/gratr/import' require 'puppet/external/gratr/dot' + require 'puppet/relationship' +require 'puppet/simple_graph' # This class subclasses a graph class in order to handle relationships # among resources. -class Puppet::PGraph < GRATR::Digraph +class Puppet::PGraph < Puppet::SimpleGraph # This is the type used for splicing. attr_accessor :container_type @@ -23,13 +25,6 @@ class Puppet::PGraph < GRATR::Digraph @reversal = nil super end - - def clear - @vertex_dict.clear - if defined? @edge_number - @edge_number.clear - end - end # Make sure whichever edge has a label keeps the label def copy_label(source, target, label) @@ -149,7 +144,7 @@ class Puppet::PGraph < GRATR::Digraph # Now get rid of the edge, so remove_vertex! works correctly. remove_edge!(edge) Puppet.debug "%s: %s => %s: %s" % [container, - edge.source, edge.target, edge?(edge)] + edge.source, edge.target, edge?(edge.source, edge.target)] end end remove_vertex!(container) diff --git a/lib/puppet/simple_graph.rb b/lib/puppet/simple_graph.rb index c8fa40e06..48bf4991e 100644 --- a/lib/puppet/simple_graph.rb +++ b/lib/puppet/simple_graph.rb @@ -54,8 +54,11 @@ class Puppet::SimpleGraph # The other vertex in the edge. def other_vertex(direction, edge) - method = direction == :in ? :source : :target - edge.send(method) + case direction + when :in: edge.source + else + edge.target + end end # Remove an edge from our list. Assumes that we've already checked @@ -82,6 +85,17 @@ class Puppet::SimpleGraph true end + # Return a reversed version of this graph. + def reversal + result = self.class.new + vertices.each { |vertex| result.add_vertex!(vertex) } + edges.each do |edge| + newedge = edge.class.new(edge.target, edge.source, edge.label) + result.add_edge!(newedge) + end + result + end + # Return the size of the graph. Used by GRATR. def size @vertices.length @@ -119,9 +133,9 @@ class Puppet::SimpleGraph # Add a new edge. The graph user has to create the edge instance, # since they have to specify what kind of edge it is. - def add_edge!(source, target = nil) + def add_edge!(source, target = nil, label = nil) if target - edge = Puppet::Relationship.new(source, target) + edge = Puppet::Relationship.new(source, target, label) else edge = source end @@ -138,6 +152,11 @@ class Puppet::SimpleGraph @edges.each_with_index { |test_edge, index| return test_edge if test_edge.source == source and test_edge.target == target } end + def edge_label(source, target) + return nil unless edge = edge(source, target) + edge.label + end + # Is there an edge between the two vertices? def edge?(source, target) return false unless vertex?(source) and vertex?(target) @@ -174,4 +193,59 @@ class Puppet::SimpleGraph def setup_vertex(vertex) @vertices[vertex] = VertexWrapper.new(vertex) end + + public + + # LAK:FIXME This is just a paste of the GRATR code with slight modifications. + + # Return a DOT::DOTDigraph for directed graphs or a DOT::DOTSubgraph for an + # undirected Graph. _params_ can contain any graph property specified in + # rdot.rb. If an edge or vertex label is a kind of Hash then the keys + # which match +dot+ properties will be used as well. + def to_dot_graph (params = {}) + params['name'] ||= self.class.name.gsub(/:/,'_') + fontsize = params['fontsize'] ? params['fontsize'] : '8' + graph = (directed? ? DOT::DOTDigraph : DOT::DOTSubgraph).new(params) + edge_klass = directed? ? DOT::DOTDirectedEdge : DOT::DOTEdge + vertices.each do |v| + name = v.to_s + params = {'name' => '"'+name+'"', + 'fontsize' => fontsize, + 'label' => name} + v_label = vertex_label(v) + params.merge!(v_label) if v_label and v_label.kind_of? Hash + graph << DOT::DOTNode.new(params) + end + edges.each do |e| + params = {'from' => '"'+ e.source.to_s + '"', + 'to' => '"'+ e.target.to_s + '"', + 'fontsize' => fontsize } + e_label = edge_label(e) + params.merge!(e_label) if e_label and e_label.kind_of? Hash + graph << edge_klass.new(params) + end + graph + end + + # Output the dot format as a string + def to_dot (params={}) to_dot_graph(params).to_s; end + + # Call +dotty+ for the graph which is written to the file 'graph.dot' + # in the # current directory. + def dotty (params = {}, dotfile = 'graph.dot') + File.open(dotfile, 'w') {|f| f << to_dot(params) } + system('dotty', dotfile) + end + + # Use +dot+ to create a graphical representation of the graph. Returns the + # filename of the graphics file. + def write_to_graphic_file (fmt='png', dotfile='graph') + src = dotfile + '.dot' + dot = dotfile + '.' + fmt + + File.open(src, 'w') {|f| f << self.to_dot << "\n"} + + system( "dot -T#{fmt} #{src} -o #{dot}" ) + dot + end end diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb index e30159c24..c8fc2f199 100644 --- a/lib/puppet/transaction.rb +++ b/lib/puppet/transaction.rb @@ -294,7 +294,7 @@ class Transaction # necessary events. def evaluate @count = 0 - + # Start logging. Puppet::Util::Log.newdestination(@report) @@ -478,7 +478,7 @@ class Transaction # types, just providers. def prefetch prefetchers = {} - @configuration.each do |resource| + @configuration.vertices.each do |resource| if provider = resource.provider and provider.class.respond_to?(:prefetch) prefetchers[provider.class] ||= {} prefetchers[provider.class][resource.title] = resource -- cgit