The evaluate_definitions method was first figuring out which resources needed to be evaluated (using unevaluated_resources), and then evaluating them one by one. As a result, if evaluating one resource triggered another resource to be evaluated, the latter resource could get evaluated twice. This bug could occur, for example, if both resources were classes that were included into the node by an external node classifier, and if the first of the two classes included the second.
Modified evaluate_definitions so that it checks each resource to see whether it needs to be evaluated right before evaluating it, rather than creating a list of resources to evaluate and then evaluating them each in turn. Also added an integration test to verify the fix. Note: the evaluate_definitions method (and unevaluated_resources, which it used to call) were subject to an optimization effort in July, which I believe was unrelated to this bug (see commit dd03ac9fa29fce36eb64a5f831be8757f2f96f5c). This fix merely changes when we check whether a resource has been evaluated, not how we do the check or how many checks we perform, so it should not introduce any significant performance bottleneck. Signed-off-by: Paul Berry <[email protected]> --- lib/puppet/parser/compiler.rb | 15 +++++++-------- spec/integration/parser/compiler_spec.rb | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 8 deletions(-) diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb index e1227e7..feeaa36 100644 --- a/lib/puppet/parser/compiler.rb +++ b/lib/puppet/parser/compiler.rb @@ -251,9 +251,15 @@ class Puppet::Parser::Compiler # We return true if any resources have, so that we know to continue the # evaluate_generators loop. def evaluate_definitions + something_evaluated = false exceptwrap do - !unevaluated_resources.each { |resource| resource.evaluate }.empty? + resources.each do |resource| + next if resource.evaluated? or resource.virtual? or resource.builtin_type? + something_evaluated = true + resource.evaluate + end end + something_evaluated end # Iterate over collections and resources until we're sure that the whole @@ -464,11 +470,4 @@ class Puppet::Parser::Compiler scope.setvar name.to_s, environment[name] end end - - # Return an array of all of the unevaluated resources. These will be definitions, - # which need to get evaluated into native resources. - def unevaluated_resources - # The order of these is significant for speed due to short-circuting - resources.reject { |resource| resource.evaluated? or resource.virtual? or resource.builtin_type? } - end end diff --git a/spec/integration/parser/compiler_spec.rb b/spec/integration/parser/compiler_spec.rb index 9158ad1..f80221e 100755 --- a/spec/integration/parser/compiler_spec.rb +++ b/spec/integration/parser/compiler_spec.rb @@ -27,6 +27,27 @@ describe Puppet::Parser::Compiler do @compiler.catalog.version.should == version end + it "should not create duplicate resources when a class is referenced both directly and indirectly by the node classifier (4792)" do + Puppet[:code] = <<-PP + class foo + { + notify { foo_notify: } + include bar + } + class bar + { + notify { bar_notify: } + } + PP + + @node.stubs(:classes).returns(['foo', 'bar']) + + catalog = Puppet::Parser::Compiler.compile(@node) + + catalog.resource("Notify[foo_notify]").should_not be_nil + catalog.resource("Notify[bar_notify]").should_not be_nil + end + describe "when resolving class references" do it "should favor local scope, even if there's an included class in topscope" do Puppet[:code] = <<-PP -- 1.7.2 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
