The patch looks good, but I'd like to see an integration test that covers the problem that occurred in this case -- you've got exported and virtual resources from the client and exported but not virtual resources from other hosts. That should catch any issues we might have here in the future.
On Aug 7, 2009, at 1:48 PM, Brice Figureau wrote: > > #2507 contains two issues: > * a crash when we filters-out an unwanted resource which had edges > pointing to it. > > * resources are losing their virtuality when they are transformed from > Puppet::Parser::Resource to Puppet::Resource. This means we weren't > able > to distinguish anymore between an exported resource collected in the > same > node as it was exported and an exported resource collected in > another node. > The net result is that we can't apply exported resources that are > collected in the same node because they are filtered out by the > catalog > filter (see the commits for #2391 for more information). > > The fix is to keep the virtuality of the resources so that we can > differentiate those two types of exported resources. We keep this > until > the catalog is ready to be sent, where we filter out the virtual > resouces > only, the other still exported ones needs to be sent to the client. > To be real sure, the transaction also skips virtual resources. > > Signed-off-by: Brice Figureau <[email protected]> > --- > lib/puppet/indirector/catalog/compiler.rb | 2 +- > lib/puppet/parser/resource.rb | 1 + > lib/puppet/resource.rb | 8 +++++--- > lib/puppet/resource/catalog.rb | 7 +++++-- > lib/puppet/transaction.rb | 4 ++-- > lib/puppet/type.rb | 11 ++++++++--- > spec/integration/indirector/catalog/compiler.rb | 2 +- > spec/unit/indirector/catalog/compiler.rb | 4 ++-- > spec/unit/parser/resource.rb | 5 +++++ > spec/unit/resource/catalog.rb | 11 +++++++++-- > spec/unit/transaction.rb | 4 ++-- > spec/unit/type.rb | 6 +++++- > 12 files changed, 46 insertions(+), 19 deletions(-) > > diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/ > indirector/catalog/compiler.rb > index 12da4de..4f6b060 100644 > --- a/lib/puppet/indirector/catalog/compiler.rb > +++ b/lib/puppet/indirector/catalog/compiler.rb > @@ -43,7 +43,7 @@ class Puppet::Resource::Catalog::Compiler < > Puppet::Indirector::Code > > # filter-out a catalog to remove exported resources > def filter(catalog) > - return catalog.filter { |r| r.exported? } if > catalog.respond_to?(:filter) > + return catalog.filter { |r| r.virtual? } if > catalog.respond_to?(:filter) > catalog > end > > diff --git a/lib/puppet/parser/resource.rb b/lib/puppet/parser/ > resource.rb > index 6632d2b..7218ac0 100644 > --- a/lib/puppet/parser/resource.rb > +++ b/lib/puppet/parser/resource.rb > @@ -268,6 +268,7 @@ class Puppet::Parser::Resource > result.file = self.file > result.line = self.line > result.exported = self.exported > + result.virtual = self.virtual > result.tag(*self.tags) > > return result > diff --git a/lib/puppet/resource.rb b/lib/puppet/resource.rb > index 0de089c..1340765 100644 > --- a/lib/puppet/resource.rb > +++ b/lib/puppet/resource.rb > @@ -9,7 +9,7 @@ class Puppet::Resource > include Puppet::Util::Tagging > extend Puppet::Util::Json > include Enumerable > - attr_accessor :file, :line, :catalog, :exported > + attr_accessor :file, :line, :catalog, :exported, :virtual > attr_writer :type, :title > > ATTRIBUTES = [:file, :line, :exported] > @@ -106,8 +106,10 @@ class Puppet::Resource > @parameters.each { |p,v| yield p, v } > end > > - def exported? > - exported > + %w{exported virtual}.each do |m| > + define_method(m+"?") do > + self.send(m) > + end > end > > # Create our resource. > diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/ > catalog.rb > index bb82906..6ccfe73 100644 > --- a/lib/puppet/resource/catalog.rb > +++ b/lib/puppet/resource/catalog.rb > @@ -576,8 +576,11 @@ class Puppet::Resource::Catalog < > Puppet::SimpleGraph > message = convert.to_s.gsub "_", " " > edges.each do |edge| > # Skip edges between virtual resources. > - next if edge.source.respond_to?(:virtual?) and > edge.source.virtual? > - next if edge.target.respond_to?(:virtual?) and > edge.target.virtual? > + next if virtual_not_exported?(edge.source) > + next if block_given? and yield edge.source > + > + next if virtual_not_exported?(edge.target) > + next if block_given? and yield edge.target > > unless source = map[edge.source.ref] > raise Puppet::DevError, "Could not find resource %s > when converting %s resources" % [edge.source.ref, message] > diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb > index f57eda6..d04856d 100644 > --- a/lib/puppet/transaction.rb > +++ b/lib/puppet/transaction.rb > @@ -590,8 +590,8 @@ class Transaction > resource.debug "Not scheduled" > elsif failed_dependencies?(resource) > resource.warning "Skipping because of failed dependencies" > - elsif resource.exported? > - resource.debug "Skipping because exported" > + elsif resource.virtual? > + resource.debug "Skipping because virtual" > else > return false > end > diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb > index 33b4e92..ee87c26 100644 > --- a/lib/puppet/type.rb > +++ b/lib/puppet/type.rb > @@ -1855,6 +1855,9 @@ class Type > # is the resource exported > attr_accessor :exported > > + # is the resource virtual (it should not :-)) > + attr_accessor :virtual > + > # create a log at specified level > def log(msg) > Puppet::Util::Log.create( > @@ -1888,7 +1891,7 @@ class Type > self.title = resource.ref > end > > - [:file, :line, :catalog, :exported].each do |getter| > + [:file, :line, :catalog, :exported, :virtual].each do | > getter| > setter = getter.to_s + "=" > if val = resource.send(getter) > self.send(setter, val) > @@ -2069,8 +2072,10 @@ class Type > return trans > end > > - def exported? > - exported > + %w{exported virtual}.each do |m| > + define_method(m+"?") do > + self.send(m) > + end > end > > end # Puppet::Type > diff --git a/spec/integration/indirector/catalog/compiler.rb b/spec/ > integration/indirector/catalog/compiler.rb > index f3ace8d..211b7c2 100755 > --- a/spec/integration/indirector/catalog/compiler.rb > +++ b/spec/integration/indirector/catalog/compiler.rb > @@ -11,7 +11,7 @@ describe Puppet::Resource::Catalog::Compiler do > @catalog = Puppet::Resource::Catalog.new > > @one = Puppet::Resource.new(:file, "/one") > - @one.exported = true > + @one.virtual = true > > @two = Puppet::Resource.new(:file, "/two") > @catalog.add_resource(@one, @two) > diff --git a/spec/unit/indirector/catalog/compiler.rb b/spec/unit/ > indirector/catalog/compiler.rb > index 78a8028..7f09422 100755 > --- a/spec/unit/indirector/catalog/compiler.rb > +++ b/spec/unit/indirector/catalog/compiler.rb > @@ -236,8 +236,8 @@ describe Puppet::Resource::Catalog::Compiler do > @compiler.filter(@catalog) > end > > - it "should filter out exported resources" do > - resource = mock 'resource', :exported? => true > + it "should filter out virtual resources" do > + resource = mock 'resource', :virtual? => true > @catalog.stubs(:filter).yields(resource) > > @compiler.filter(@catalog) > diff --git a/spec/unit/parser/resource.rb b/spec/unit/parser/ > resource.rb > index 8005e20..9eb8b13 100755 > --- a/spec/unit/parser/resource.rb > +++ b/spec/unit/parser/resource.rb > @@ -419,6 +419,11 @@ describe Puppet::Parser::Resource do > @parser_resource.to_resource.exported.should be_true > end > > + it "should copy over the 'virtual' value" do > + @parser_resource.virtual = true > + @parser_resource.to_resource.virtual.should be_true > + end > + > it "should convert any parser resource references to > Puppet::Resource::Reference instances" do > ref = Puppet::Parser::Resource::Reference.new(:title => > "/my/file", :type => "file") > @parser_resource = mkresource :source => > @source, :params => {:foo => "bar", :fee => ref} > diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/ > catalog.rb > index 97b6ad7..af399aa 100755 > --- a/spec/unit/resource/catalog.rb > +++ b/spec/unit/resource/catalog.rb > @@ -323,9 +323,9 @@ describe Puppet::Resource::Catalog, "when > compiling" do > end > > it "should scan each catalog resource in turn and apply > filtering block" do > - @resources.each { |r| r.expects(:exported?) } > + @resources.each { |r| r.expects(:test?) } > @original.filter do |r| > - r.exported? > + r.test? > end > end > > @@ -334,6 +334,13 @@ describe Puppet::Resource::Catalog, "when > compiling" do > r == @r1 > end.resource("File[/a]").should be_nil > end > + > + it "should not consider edges against resources that were > filtered out" do > + @original.add_edge(@r1,@r2) > + @original.filter do |r| > + r == @r1 > + end.edge(@r1,@r2).should be_empty > + end > end > > describe "when functioning as a resource container" do > diff --git a/spec/unit/transaction.rb b/spec/unit/transaction.rb > index 0e36747..7966c7a 100755 > --- a/spec/unit/transaction.rb > +++ b/spec/unit/transaction.rb > @@ -75,8 +75,8 @@ describe Puppet::Transaction do > @transaction.skip?(@resource).should be_true > end > > - it "should skip exported resource" do > - @resource.stubs(:exported?).returns true > + it "should skip virtual resource" do > + @resource.stubs(:virtual?).returns true > @transaction.skip?(@resource).should be_true > end > end > diff --git a/spec/unit/type.rb b/spec/unit/type.rb > index b179677..fe2788e 100755 > --- a/spec/unit/type.rb > +++ b/spec/unit/type.rb > @@ -72,6 +72,10 @@ describe Puppet::Type do > Puppet::Type.type(:mount).new(:name => "foo").should > respond_to(:exported?) > end > > + it "should have a method to know if the resource is virtual" do > + Puppet::Type.type(:mount).new(:name => "foo").should > respond_to(:virtual?) > + end > + > it "should consider its version to be its catalog version" do > resource = Puppet::Type.type(:mount).new(:name => "foo") > catalog = Puppet::Resource::Catalog.new > @@ -121,7 +125,7 @@ describe Puppet::Type do > Puppet::Type.type(:mount).new(resource).title.should > == "User[foo]" > end > > - [:line, :file, :catalog, :exported].each do |param| > + [:line, :file, :catalog, :exported, :virtual].each do | > param| > it "should copy '#{param}' from the resource if > present" do > resource = Puppet::Resource.new(:mount, "/foo") > resource.send(param.to_s + "=", "foo") > -- > 1.6.0.2 > > > > -- In theory, there is no difference between theory and practice; in practice, there is. -- Chuck Reid --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
