To implement #6911 we will need to be able to make incremental descisions about order of application based only on the contents of the resource graph and local "working data." This commit begins to pull the needed structure into a method (visit_resources) while, for the moment, maintaining the original semantic.
Paired-with: Jesse Wolfe Signed-off-by: Markus Roberts <mar...@reality.com> --- Local-branch: feature/next/resource_application_order lib/puppet/resource/catalog.rb | 42 ++++---- lib/puppet/transaction.rb | 107 ++++++++++---------- lib/puppet/type/file.rb | 12 +-- lib/puppet/type/tidy.rb | 4 - .../indirector/catalog/compiler_spec.rb | 7 +- spec/integration/type/file_spec.rb | 13 ++- spec/unit/resource/catalog_spec.rb | 6 - spec/unit/transaction_spec.rb | 4 +- 8 files changed, 89 insertions(+), 106 deletions(-) diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb index a8668d8..fed0f46 100644 --- a/lib/puppet/resource/catalog.rb +++ b/lib/puppet/resource/catalog.rb @@ -61,36 +61,32 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph [$1, $2] end - # Add one or more resources to our graph and to our resource table. + # Add a resource to our graph and to our resource table. # This is actually a relatively complicated method, because it handles multiple # aspects of Catalog behaviour: # * Add the resource to the resource table # * Add the resource to the resource graph # * Add the resource to the relationship graph # * Add any aliases that make sense for the resource (e.g., name != title) - def add_resource(*resources) - resources.each do |resource| - raise ArgumentError, "Can only add objects that respond to :ref, not instances of #{resource.class}" unless resource.respond_to?(:ref) - end.each { |resource| fail_on_duplicate_type_and_title(resource) }.each do |resource| - title_key = title_key_for_ref(resource.ref) - - @transient_resources << resource if applying? - @resource_table[title_key] = resource - - # If the name and title differ, set up an alias - - if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.respond_to?(:isomorphic?) and resource.name != resource.title - self.alias(resource, resource.uniqueness_key) if resource.isomorphic? - end - - resource.catalog = self if resource.respond_to?(:catalog=) - - add_vertex(resource) - - @relationship_graph.add_vertex(resource) if @relationship_graph - - yield(resource) if block_given? + def add_resource(*resource) + add_resource(*resource[0..-2]) if resource.length > 1 + resource = resource.pop + raise ArgumentError, "Can only add objects that respond to :ref, not instances of #{resource.class}" unless resource.respond_to?(:ref) + fail_on_duplicate_type_and_title(resource) + title_key = title_key_for_ref(resource.ref) + + @transient_resources << resource if applying? + @resource_table[title_key] = resource + + # If the name and title differ, set up an alias + + if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.respond_to?(:isomorphic?) and resource.name != resource.title + self.alias(resource, resource.uniqueness_key) if resource.isomorphic? end + + resource.catalog = self if resource.respond_to?(:catalog=) + add_vertex(resource) + @relationship_graph.add_vertex(resource) if @relationship_graph end # Create an alias for a resource. diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb index eba601c..aa0eec3 100644 --- a/lib/puppet/transaction.rb +++ b/lib/puppet/transaction.rb @@ -12,7 +12,7 @@ class Puppet::Transaction require 'puppet/resource/status' attr_accessor :component, :catalog, :ignoreschedules - attr_accessor :sorted_resources, :configurator + attr_accessor :configurator # The report, once generated. attr_accessor :report @@ -57,32 +57,23 @@ class Puppet::Transaction report.resource_statuses.values.find_all { |status| status.changed }.collect { |status| catalog.resource(status.resource) } end + # Find all of the applied resources (including failed attempts). + def applied_resources + report.resource_statuses.values.collect { |status| catalog.resource(status.resource) } + end + # Copy an important relationships from the parent to the newly-generated # child resource. - def make_parent_child_relationship(resource, children) - depthfirst = resource.depthfirst? - - children.each do |gen_child| - if depthfirst - edge = [gen_child, resource] - else - edge = [resource, gen_child] - end - relationship_graph.add_vertex(gen_child) - - unless relationship_graph.edge?(edge[1], edge[0]) - relationship_graph.add_edge(*edge) - else - resource.debug "Skipping automatic relationship to #{gen_child}" - end + def make_parent_child_relationship(parent, child) + relationship_graph.add_vertex(child) + edge = parent.depthfirst? ? [child, parent] : [parent, child] + if relationship_graph.edge?(*edge.reverse) + parent.debug "Skipping automatic relationship to #{child}" + else + relationship_graph.add_edge(*edge) end end - # See if the resource generates new resources at evaluation time. - def eval_generate(resource) - generate_additional_resources(resource, :eval_generate) - end - # Evaluate a single resource. def eval_resource(resource, ancestor = nil) if skip?(resource) @@ -97,26 +88,7 @@ class Puppet::Transaction def eval_children_and_apply_resource(resource, ancestor = nil) resource_status(resource).scheduled = true - - # We need to generate first regardless, because the recursive - # actions sometimes change how the top resource is applied. - children = eval_generate(resource) - - if ! children.empty? and resource.depthfirst? - children.each do |child| - # The child will never be skipped when the parent isn't - eval_resource(child, ancestor || resource) - end - end - - # Perform the actual changes apply(resource, ancestor) - - if ! children.empty? and ! resource.depthfirst? - children.each do |child| - eval_resource(child, ancestor || resource) - end - end end # This method does all the actual work of running a transaction. It @@ -131,7 +103,7 @@ class Puppet::Transaction Puppet.info "Applying configuration version '#{catalog.version}'" if catalog.version begin - @sorted_resources.each do |resource| + visit_resources do |resource| next if stop_processing? if resource.is_a?(Puppet::Type::Component) Puppet.warning "Somehow left a component in the relationship graph" @@ -177,9 +149,31 @@ class Puppet::Transaction found_failed end + def eval_generate(resource) + return [] unless resource.respond_to?(:eval_generate) + begin + made = resource.eval_generate + rescue => detail + puts detail.backtrace if Puppet[:trace] + resource.err "Failed to generate additional resources using 'eval_generate: #{detail}" + end + parents = [resource] + [made].flatten.compact.uniq.each do |res| + begin + res.tag(*resource.tags) + @catalog.add_resource(res) + res.finish + make_parent_child_relationship(parents.reverse.find { |r| r.name == res.name[0,r.name.length]}, res) + parents << res + rescue Puppet::Resource::Catalog::DuplicateResourceError + res.info "Duplicate generated resource; skipping" + end + end + end + # A general method for recursively generating new resources from a # resource. - def generate_additional_resources(resource, method) + def generate_additional_resources(resource, method, presume_prefix_dependencies=false) return [] unless resource.respond_to?(method) begin made = resource.send(method) @@ -192,13 +186,10 @@ class Puppet::Transaction made.uniq.find_all do |res| begin res.tag(*resource.tags) - @catalog.add_resource(res) do |r| - r.finish - make_parent_child_relationship(resource, [r]) - - # Call 'generate' recursively - generate_additional_resources(r, method) - end + @catalog.add_resource(res) + res.finish + make_parent_child_relationship(resource, res) + generate_additional_resources(res, method, presume_prefix_dependencies) true rescue Puppet::Resource::Catalog::DuplicateResourceError res.info "Duplicate generated resource; skipping" @@ -269,9 +260,21 @@ class Puppet::Transaction # Then prefetch. It's important that we generate and then prefetch, # so that any generated resources also get prefetched. prefetch + end - # This will throw an error if there are cycles in the graph. - @sorted_resources = relationship_graph.topsort + def visit_resources(&block) + eval_generated = {} + while r = relationship_graph.topsort.find { |r| !applied_resources.include?(r) } + #p relationship_graph.topsort.collect { |r0| r0.title[/(-0.*)/,1] } + if !eval_generated[r] + #p [:generate,r.title] + eval_generate(r) + eval_generated[r] = true + else + #p [:apply,r.title] + yield r + end + end end def relationship_graph diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb index 2c5a95b..cbc1f77 100644 --- a/lib/puppet/type/file.rb +++ b/lib/puppet/type/file.rb @@ -462,8 +462,7 @@ Puppet::Type.newtype(:file) do # be used to copy remote files, manage local files, and/or make links # to map to another directory. def recurse - children = {} - children = recurse_local if self[:recurse] != :remote + children = (self[:recurse] == :remote) ? {} : recurse_local if self[:target] recurse_link(children) @@ -500,11 +499,7 @@ Puppet::Type.newtype(:file) do # A simple method for determining whether we should be recursing. def recurse? - return false unless @parameters.include?(:recurse) - - val = @parameters[:recurse].value - - !!(val and (val == true or val == :remote)) + self[:recurse] == true or self[:recurse] == :remote end # Recurse the target of the link. @@ -576,13 +571,10 @@ Puppet::Type.newtype(:file) do end def perform_recursion(path) - Puppet::FileServing::Metadata.indirection.search( - path, :links => self[:links], :recurse => (self[:recurse] == :remote ? true : self[:recurse]), - :recurselimit => self[:recurselimit], :ignore => self[:ignore], :checksum_type => (self[:source] || self[:content]) ? self[:checksum] : :none diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb index d2e9ebe..1a308e1 100755 --- a/lib/puppet/type/tidy.rb +++ b/lib/puppet/type/tidy.rb @@ -240,10 +240,6 @@ Puppet::Type.newtype(:tidy) do [] end - def eval_generate - [] - end - def generate return [] unless stat(self[:path]) diff --git a/spec/integration/indirector/catalog/compiler_spec.rb b/spec/integration/indirector/catalog/compiler_spec.rb index 1146c20..dafa1af 100755 --- a/spec/integration/indirector/catalog/compiler_spec.rb +++ b/spec/integration/indirector/catalog/compiler_spec.rb @@ -10,11 +10,8 @@ describe Puppet::Resource::Catalog::Compiler do before do Facter.stubs(:value).returns "something" @catalog = Puppet::Resource::Catalog.new - - @one = Puppet::Resource.new(:file, "/one") - - @two = Puppet::Resource.new(:file, "/two") - @catalog.add_resource(@one, @two) + @catalog.add_resource(@one = Puppet::Resource.new(:file, "/one")) + @catalog.add_resource(@two = Puppet::Resource.new(:file, "/two")) end after { Puppet.settings.clear } diff --git a/spec/integration/type/file_spec.rb b/spec/integration/type/file_spec.rb index 46900a0..513b96e 100755 --- a/spec/integration/type/file_spec.rb +++ b/spec/integration/type/file_spec.rb @@ -28,7 +28,8 @@ describe Puppet::Type.type(:file) do bucket = Puppet::Type.type(:filebucket).new :path => tmpfile("filebucket"), :name => "mybucket" file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => "mybucket", :content => "foo" catalog = Puppet::Resource::Catalog.new - catalog.add_resource file, bucket + catalog.add_resource file + catalog.add_resource bucket File.open(file[:path], "w") { |f| f.puts "bar" } @@ -80,7 +81,8 @@ describe Puppet::Type.type(:file) do bucket = Puppet::Type.type(:filebucket).new :path => tmpfile("filebucket"), :name => "mybucket" file = Puppet::Type.type(:file).new :path => link, :target => dest2, :ensure => :link, :backup => "mybucket" catalog = Puppet::Resource::Catalog.new - catalog.add_resource file, bucket + catalog.add_resource file + catalog.add_resource bucket File.open(dest1, "w") { |f| f.puts "whatever" } File.symlink(dest1, link) @@ -113,7 +115,8 @@ describe Puppet::Type.type(:file) do bucket = Puppet::Type.type(:filebucket).new :path => tmpfile("filebucket"), :name => "mybucket" file = Puppet::Type.type(:file).new :path => tmpfile("bucket_backs"), :backup => "mybucket", :content => "foo", :force => true catalog = Puppet::Resource::Catalog.new - catalog.add_resource file, bucket + catalog.add_resource file + catalog.add_resource bucket Dir.mkdir(file[:path]) foofile = File.join(file[:path], "foo") @@ -337,10 +340,10 @@ describe Puppet::Type.type(:file) do it "should have an edge to each resource in the relationship graph" do @catalog.apply do |trans| one = @catalog.resource(:file, File.join(@dest, "one")) - @catalog.relationship_graph.should be_edge(@file, one) + @catalog.relationship_graph.edge?(@file, one).should be two = @catalog.resource(:file, File.join(@dest, "two")) - @catalog.relationship_graph.should be_edge(@file, two) + @catalog.relationship_graph.edge?(@file, two).should be end end end diff --git a/spec/unit/resource/catalog_spec.rb b/spec/unit/resource/catalog_spec.rb index 42850c2..411d10b 100755 --- a/spec/unit/resource/catalog_spec.rb +++ b/spec/unit/resource/catalog_spec.rb @@ -398,12 +398,6 @@ describe Puppet::Resource::Catalog, "when compiling" do relgraph.should be_vertex(@one) end - it "should yield added resources if a block is provided" do - yielded = [] - @catalog.add_resource(@one, @two) { |r| yielded << r } - yielded.length.should == 2 - end - it "should set itself as the resource's catalog if it is not a relationship graph" do @one.expects(:catalog=).with(@catalog) @catalog.add_resource @one diff --git a/spec/unit/transaction_spec.rb b/spec/unit/transaction_spec.rb index 3677bfd..5867f2e 100755 --- a/spec/unit/transaction_spec.rb +++ b/spec/unit/transaction_spec.rb @@ -244,6 +244,8 @@ describe Puppet::Transaction do @catalog.stubs(:add_resource) child.expects(:tag).with("one", "two") + child.expects(:finish) + generator.expects(:depthfirst?) @transaction.generate_additional_resources(generator, :generate) end @@ -390,7 +392,7 @@ describe Puppet::Transaction do @resource = stub 'resource', :ref => 'some_ref' @catalog.add_resource @resource @transaction.stubs(:prepare) - @transaction.sorted_resources = [@resource] + @transaction.stubs(:visit_resources).yields(@resource) end it 'should stop processing if :stop_processing? is true' do -- 1.7.0.4 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to puppet-dev@googlegroups.com. To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.