[#4026] When --use_cached_catalog is specified on a puppetd run actully use the cache

Running puppetd with --use_cached_catalog you would see messages like:

    info: Not using expired catalog for mattmac.local from cache
    notice: Using cached catalog

Both Puppet::Util::Cacher, which extends catalogs, and Puppet::Indirector::Envelope,
which extends all Indirection objects including catalogs, have their own cache
expiring mechanisms.  The Envelope mechanism was declining to use cached
catalogs without taking into account what the --use_cached_catalog
options said.  This patch fixes so it uses the cached catalog and just logs:

    debug: Using cached catalog for mattmac.local

This commit also renames a method that makes requests from request
to instantiate request, and gets rid of the extender hook on Cacher
since it was only being used on one test and probably shouldn't be used
in general.

Reviewed by: Nick Lewis

8 files changed, 42 insertions, 40 deletions

diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 6aeac7486..551d99684 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -194,7 +194,6 @@ class Puppet::Configurer
         @duration = thinmark do
             result = Puppet::Resource::Catalog.find(Puppet[:certname], fact_options.merge(:ignore_terminus => true))
         end
-        Puppet.notice "Using cached catalog"
         result
     rescue => detail
         puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index 7c3183396..97fd03ad4 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -116,7 +116,7 @@ class Puppet::Indirector::Indirection
     end
 
     # Set up our request object.
-    def request(method, key, arguments = nil)
+    def instantiate_request(method, key, arguments = nil)
         Puppet::Indirector::Request.new(self.name, method, key, arguments)
     end
 
@@ -169,24 +169,24 @@ class Puppet::Indirector::Indirection
     # remove it, we expire it and write it back out to disk.  This way people
     # can still use the expired object if they want.
     def expire(key, *args)
-        request = request(:expire, key, *args)
+        request = instantiate_request(:expire, key, *args)
 
         return nil unless cache?
 
-        return nil unless instance = cache.find(request(:find, key, *args))
+        return nil unless instance = cache.find(instantiate_request(:find, key, *args))
 
         Puppet.info "Expiring the %s cache of %s" % [self.name, instance.name]
 
         # Set an expiration date in the past
         instance.expiration = Time.now - 60
 
-        cache.save(request(:save, instance, *args))
+        cache.save(instantiate_request(:save, instance, *args))
     end
 
     # Search for an instance in the appropriate terminus, caching the
     # results if caching is configured..
     def find(key, *args)
-        request = request(:find, key, *args)
+        request = instantiate_request(:find, key, *args)
         terminus = prepare(request)
 
         begin
@@ -203,7 +203,7 @@ class Puppet::Indirector::Indirection
             result.expiration ||= self.expiration
             if cache? and request.use_cache?
                 Puppet.info "Caching %s for %s" % [self.name, request.key]
-                cache.save request(:save, result, *args)
+                cache.save instantiate_request(:save, result, *args)
             end
 
             return terminus.respond_to?(:filter) ? terminus.filter(result) : result
@@ -226,12 +226,12 @@ class Puppet::Indirector::Indirection
 
     # Remove something via the terminus.
     def destroy(key, *args)
-        request = request(:destroy, key, *args)
+        request = instantiate_request(:destroy, key, *args)
         terminus = prepare(request)
 
         result = terminus.destroy(request)
 
-        if cache? and cached = cache.find(request(:find, key, *args))
+        if cache? and cached = cache.find(instantiate_request(:find, key, *args))
             # Reuse the existing request, since it's equivalent.
             cache.destroy(request)
         end
@@ -241,7 +241,7 @@ class Puppet::Indirector::Indirection
 
     # Search for more than one instance.  Should always return an array.
     def search(key, *args)
-        request = request(:search, key, *args)
+        request = instantiate_request(:search, key, *args)
         terminus = prepare(request)
 
         if result = terminus.search(request)
@@ -256,7 +256,7 @@ class Puppet::Indirector::Indirection
     # Save the instance in the appropriate terminus.  This method is
     # normally an instance method on the indirected class.
     def save(instance, *args)
-        request = request(:save, instance, *args)
+        request = instantiate_request(:save, instance, *args)
         terminus = prepare(request)
 
         result = terminus.save(request)
diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 6f589083c..a0a871126 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -49,6 +49,11 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     # Some metadata to help us compile and generally respond to the current state.
     attr_accessor :client_version, :server_version
 
+    def expired?
+        return false if Puppet[:use_cached_catalog]
+        super
+    end
+
     # Add classes to our class list.
     def add_class(*classes)
         classes.each do |klass|
diff --git a/lib/puppet/util/cacher.rb b/lib/puppet/util/cacher.rb
index 28786ab53..3c9d24db9 100644
--- a/lib/puppet/util/cacher.rb
+++ b/lib/puppet/util/cacher.rb
@@ -18,15 +18,6 @@ module Puppet::Util::Cacher
 
     extend Expirer
 
-    # Our module has been extended in a class; we can only add the Instance methods,
-    # which become *class* methods in the class.
-    def self.extended(other)
-        class << other
-            extend ClassMethods
-            include InstanceMethods
-        end
-    end
-
     # Our module has been included in a class, which means the class gets the class methods
     # and all of its instances get the instance methods.
     def self.included(other)
diff --git a/spec/integration/indirector/direct_file_server.rb b/spec/integration/indirector/direct_file_server.rb
index 9843c7e65..c33d23e62 100755
--- a/spec/integration/indirector/direct_file_server.rb
+++ b/spec/integration/indirector/direct_file_server.rb
@@ -18,7 +18,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with the files
     it "should return an instance of the model" do
         FileTest.expects(:exists?).with(@filepath).returns(true)
 
-        @terminus.find(@terminus.indirection.request(:find, "file://host#{@filepath}")).should be_instance_of(Puppet::FileServing::Content)
+        @terminus.find(@terminus.indirection.instantiate_request(:find, "file://host#{@filepath}")).should be_instance_of(Puppet::FileServing::Content)
     end
 
     it "should return an instance capable of returning its content" do
@@ -26,7 +26,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with the files
         File.stubs(:lstat).with(@filepath).returns(stub("stat", :ftype => "file"))
         File.expects(:read).with(@filepath).returns("my content")
 
-        instance = @terminus.find(@terminus.indirection.request(:find, "file://host#{@filepath}"))
+        instance = @terminus.find(@terminus.indirection.instantiate_request(:find, "file://host#{@filepath}"))
 
         instance.content.should == "my content"
     end
@@ -45,7 +45,7 @@ describe Puppet::Indirector::DirectFileServer, " when interacting with FileServi
         File.open(File.join(@path, "one"), "w") { |f| f.print "one content" }
         File.open(File.join(@path, "two"), "w") { |f| f.print "two content" }
 
-        @request = @terminus.indirection.request(:search, "file:///%s" % @path, :recurse => true)
+        @request = @terminus.indirection.instantiate_request(:search, "file:///%s" % @path, :recurse => true)
     end
 
     after do
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 2ed51d7c4..8049d771d 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -8,7 +8,7 @@ describe "Indirection Delegator", :shared => true do
     it "should create a request object with the appropriate method name and all of the passed arguments" do
         request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
-        @indirection.expects(:request).with(@method, "mystuff", :one => :two).returns request
+        @indirection.expects(:instantiate_request).with(@method, "mystuff", :one => :two).returns request
 
         @terminus.stubs(@method)
 
@@ -24,7 +24,7 @@ describe "Indirection Delegator", :shared => true do
 
         request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
-        @indirection.stubs(:request).returns request
+        @indirection.stubs(:instantiate_request).returns request
 
         @indirection.expects(:select_terminus).with(request).returns :test_terminus
 
@@ -43,7 +43,7 @@ describe "Indirection Delegator", :shared => true do
 
         request = stub 'request', :key => "me", :options => {}
 
-        @indirection.stubs(:request).returns request
+        @indirection.stubs(:instantiate_request).returns request
 
         @indirection.expects(:select_terminus).with(request).returns nil
 
@@ -179,34 +179,34 @@ describe Puppet::Indirector::Indirection do
         end
 
         it "should have a method for creating an indirection request instance" do
-            @indirection.should respond_to(:request)
+            @indirection.should respond_to(:instantiate_request)
         end
 
         describe "creates a request" do
             it "should create it with its name as the request's indirection name" do
                 Puppet::Indirector::Request.expects(:new).with { |name, *other| @indirection.name == name }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should require a method and key" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, *other| method == :funtest and key == "yayness" }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should support optional arguments" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, other| other == {:one => :two} }
-                @indirection.request(:funtest, "yayness", :one => :two)
+                @indirection.instantiate_request(:funtest, "yayness", :one => :two)
             end
 
             it "should default to the arguments being nil" do
                 Puppet::Indirector::Request.expects(:new).with { |name, method, key, args| args.nil? }
-                @indirection.request(:funtest, "yayness")
+                @indirection.instantiate_request(:funtest, "yayness")
             end
 
             it "should return the request" do
                 request = mock 'request'
                 Puppet::Indirector::Request.expects(:new).returns request
-                @indirection.request(:funtest, "yayness").should equal(request)
+                @indirection.instantiate_request(:funtest, "yayness").should equal(request)
             end
         end
 
@@ -405,7 +405,7 @@ describe Puppet::Indirector::Indirection do
                 it "should return the result of saving to the terminus" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.stubs(:save)
                     @terminus.stubs(:save).returns @instance
@@ -415,7 +415,7 @@ describe Puppet::Indirector::Indirection do
                 it "should use a request to save the object to the cache" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.expects(:save).with(request)
                     @terminus.stubs(:save)
@@ -425,7 +425,7 @@ describe Puppet::Indirector::Indirection do
                 it "should not save to the cache if the normal save fails" do
                     request = stub 'request', :instance => @instance, :node => nil
 
-                    @indirection.expects(:request).returns request
+                    @indirection.expects(:instantiate_request).returns request
 
                     @cache.expects(:save).never
                     @terminus.expects(:save).raises "eh"
@@ -457,8 +457,8 @@ describe Puppet::Indirector::Indirection do
                     destroy = stub 'destroy_request', :key => "/my/key", :node => nil
                     find = stub 'destroy_request', :key => "/my/key", :node => nil
 
-                    @indirection.expects(:request).with(:destroy, "/my/key").returns destroy
-                    @indirection.expects(:request).with(:find, "/my/key").returns find
+                    @indirection.expects(:instantiate_request).with(:destroy, "/my/key").returns destroy
+                    @indirection.expects(:instantiate_request).with(:find, "/my/key").returns find
 
                     cached = mock 'cache'
 
diff --git a/spec/unit/resource/catalog.rb b/spec/unit/resource/catalog.rb
index 39ddccd9b..0835bf982 100755
--- a/spec/unit/resource/catalog.rb
+++ b/spec/unit/resource/catalog.rb
@@ -35,6 +35,13 @@ describe Puppet::Resource::Catalog, "when compiling" do
         @catalog.write_class_file
     end
 
+    it "should not be expired if use_cached_catalog option is true" do
+        Puppet.settings[:use_cached_catalog] = true
+        @catalog = Puppet::Resource::Catalog.new("host")
+        @catalog.expiration = Time.now
+        @catalog.should_not be_expired
+    end
+
     it "should have a client_version attribute" do
         @catalog = Puppet::Resource::Catalog.new("host")
         @catalog.client_version = 5
diff --git a/spec/unit/util/cacher.rb b/spec/unit/util/cacher.rb
index eb8515e4d..d02852c13 100755
--- a/spec/unit/util/cacher.rb
+++ b/spec/unit/util/cacher.rb
@@ -151,11 +151,11 @@ describe Puppet::Util::Cacher do
 
         it "should not check for a ttl expiration if the class does not support that method" do
             klass = Class.new do
-                extend Puppet::Util::Cacher
+                include Puppet::Util::Cacher
             end
 
-            klass.singleton_class.cached_attr(:myattr) { "eh" }
-            klass.myattr
+            klass.cached_attr(:myattr) { "eh" }
+            lambda { klass.new.myattr }.should_not raise_error
         end
 
         it "should automatically expire cached attributes whose ttl has expired, even if no expirer is present" do