From 73fa3977d7274f1c54c89971c0389f9a9f980c9a Mon Sep 17 00:00:00 2001 From: Luke Kanies Date: Tue, 11 Nov 2008 13:31:31 -0800 Subject: [PATCH] Adding caching support to parameters, and using cached attributes for file source and metadata. As hoped, this drastically simplifies the code around retaining this data. Signed-off-by: Luke Kanies --- lib/puppet/parameter.rb | 6 +++++ lib/puppet/type/file/source.rb | 45 +++++++++++++-------------------- lib/puppet/util/cacher.rb | 6 +++++ spec/unit/parameter.rb | 10 ++++++++ spec/unit/type/file/source.rb | 46 +++++++++++++++++++++------------- spec/unit/util/cacher.rb | 10 ++++++++ 6 files changed, 77 insertions(+), 46 deletions(-) diff --git a/lib/puppet/parameter.rb b/lib/puppet/parameter.rb index f82438903..5f68d44a5 100644 --- a/lib/puppet/parameter.rb +++ b/lib/puppet/parameter.rb @@ -2,6 +2,7 @@ require 'puppet/util/methodhelper' require 'puppet/util/log_paths' require 'puppet/util/logging' require 'puppet/util/docs' +require 'puppet/util/cacher' class Puppet::Parameter include Puppet::Util @@ -9,6 +10,7 @@ class Puppet::Parameter include Puppet::Util::LogPaths include Puppet::Util::Logging include Puppet::Util::MethodHelper + include Puppet::Util::Cacher # A collection of values and regexes, used for specifying # what values are allowed in a given parameter. @@ -354,6 +356,10 @@ class Puppet::Parameter self.fail(Puppet::DevError, msg) end + def expirer + resource.catalog + end + def fail(*args) type = nil if args[0].is_a?(Class) diff --git a/lib/puppet/type/file/source.rb b/lib/puppet/type/file/source.rb index ba5fcbd0e..4e67b1a8b 100755 --- a/lib/puppet/type/file/source.rb +++ b/lib/puppet/type/file/source.rb @@ -101,16 +101,13 @@ module Puppet end # Look up (if necessary) and return remote content. - def content + cached_attr(:content) do raise Puppet::DevError, "No source for content was stored with the metadata" unless metadata.source - unless defined?(@content) and @content - unless tmp = Puppet::FileServing::Content.find(metadata.source) - fail "Could not find any content at %s" % metadata.source - end - @content = tmp.content + unless tmp = Puppet::FileServing::Content.find(metadata.source) + fail "Could not find any content at %s" % metadata.source end - @content + tmp.content end # Copy the values from the source to the resource. Yay. @@ -135,12 +132,6 @@ module Puppet end end - # Remove any temporary attributes we manage. - def flush - @metadata = nil - @content = nil - end - def pinparams [:mode, :type, :owner, :group, :content] end @@ -152,24 +143,22 @@ module Puppet # Provide, and retrieve if necessary, the metadata for this file. Fail # if we can't find data about this host, and fail if there are any # problems in our query. - attr_writer :metadata - def metadata - unless defined?(@metadata) and @metadata - return @metadata = nil unless value - value.each do |source| - begin - if data = Puppet::FileServing::Metadata.find(source) - @metadata = data - @metadata.source = source - break - end - rescue => detail - fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail] + cached_attr(:metadata) do + return nil unless value + result = nil + value.each do |source| + begin + if data = Puppet::FileServing::Metadata.find(source) + result = data + result.source = source + break end + rescue => detail + fail detail, "Could not retrieve file metadata for %s: %s" % [source, detail] end - fail "Could not retrieve information from source(s) %s" % value.join(", ") unless @metadata end - return @metadata + fail "Could not retrieve information from source(s) %s" % value.join(", ") unless result + result end # Make sure we're also checking the checksum diff --git a/lib/puppet/util/cacher.rb b/lib/puppet/util/cacher.rb index af5c5ebfc..c1b3bad7a 100644 --- a/lib/puppet/util/cacher.rb +++ b/lib/puppet/util/cacher.rb @@ -47,6 +47,12 @@ module Puppet::Util::Cacher define_method(name) do cached_value(name) end + + define_method(name.to_s + "=") do |value| + # Make sure the cache timestamp is set + cache_timestamp + value_cache[name] = value + end end end diff --git a/spec/unit/parameter.rb b/spec/unit/parameter.rb index 53ea07648..94f5cfd7b 100755 --- a/spec/unit/parameter.rb +++ b/spec/unit/parameter.rb @@ -20,6 +20,16 @@ describe Puppet::Parameter do @class.value_collection.should be_instance_of(Puppet::Parameter::ValueCollection) end + it "should be able to use cached attributes" do + Puppet::Parameter.ancestors.should be_include(Puppet::Util::Cacher) + end + + it "should use the resource catalog for expiration" do + catalog = mock 'catalog' + @resource.stubs(:catalog).returns catalog + @parameter.expirer.should equal(catalog) + end + describe "when returning the value" do it "should return nil if no value is set" do @parameter.value.should be_nil diff --git a/spec/unit/type/file/source.rb b/spec/unit/type/file/source.rb index 2dcc1a557..6f2bbb8fa 100755 --- a/spec/unit/type/file/source.rb +++ b/spec/unit/type/file/source.rb @@ -6,7 +6,7 @@ source = Puppet::Type.type(:file).attrclass(:source) describe Puppet::Type.type(:file).attrclass(:source) do before do # Wow that's a messy interface to the resource. - @resource = stub 'resource', :[]= => nil, :property => nil + @resource = stub 'resource', :[]= => nil, :property => nil, :catalog => stub("catalog", :expired? => false) end it "should be a subclass of Parameter" do @@ -91,6 +91,20 @@ describe Puppet::Type.type(:file).attrclass(:source) do lambda { @source.metadata }.should raise_error(RuntimeError) end + + it "should expire the metadata appropriately" do + expirer = stub 'expired', :expired? => true + + metadata = stub 'metadata', :source= => nil + Puppet::FileServing::Metadata.expects(:find).with("/fee/booz").returns metadata + + @source = source.new(:resource => @resource, :value => ["/fee/booz"]) + @source.metadata = "foo" + + @source.stubs(:expirer).returns expirer + + @source.metadata.should_not == "foo" + end end it "should have a method for setting the desired values on the resource" do @@ -200,22 +214,6 @@ describe Puppet::Type.type(:file).attrclass(:source) do end end - describe "when flushing" do - it "should set its metadata to nil" do - @source = source.new(:resource => @resource) - @source.metadata = "foo" - @source.flush - @source.instance_variable_get("@metadata").should be_nil - end - - it "should reset its content" do - @source = source.new(:resource => @resource) - @source.instance_variable_set("@content", "foo") - @source.flush - @source.instance_variable_get("@content").should be_nil - end - end - it "should have a method for returning the content" do source.new(:resource => @resource).must respond_to(:content) end @@ -224,7 +222,7 @@ describe Puppet::Type.type(:file).attrclass(:source) do before do @source = source.new(:resource => @resource) @metadata = stub 'metadata', :source => "/my/source" - @source.metadata = @metadata + @source.stubs(:metadata).returns @metadata @content = stub 'content', :content => "foobar" end @@ -250,5 +248,17 @@ describe Puppet::Type.type(:file).attrclass(:source) do @source.expects(:fail).raises RuntimeError lambda { @source.content }.should raise_error(RuntimeError) end + + it "should expire the content appropriately" do + expirer = stub 'expired', :expired? => true + + content2 = stub 'content', :content => "secondrun" + Puppet::FileServing::Content.expects(:find).with("/my/source").times(2).returns(@content).then.returns(content2) + @source.content.should == "foobar" + + @source.stubs(:expirer).returns expirer + + @source.content.should == "secondrun" + end end end diff --git a/spec/unit/util/cacher.rb b/spec/unit/util/cacher.rb index 26fe20f84..3e8d31a24 100755 --- a/spec/unit/util/cacher.rb +++ b/spec/unit/util/cacher.rb @@ -95,5 +95,15 @@ describe Puppet::Util::Cacher do @object.stubs(:expirer).returns nil @object.instance_cache.should_not equal(@object.instance_cache) end + + it "should allow writing of the attribute" do + @object.should respond_to(:instance_cache=) + end + + it "should correctly configure timestamps for expiration when the cached attribute is written to" do + @object.instance_cache = "foo" + @expirer.expire + @object.instance_cache.should_not == "foo" + end end end