Changing how the Configurer interacts with the cache

This changes the hooks provided via the Indirector Request
for determining how the cache is used.  These hooks are only
used by the Configurer class.  They're messy, but I can't
come up with a better design, and they're at least sufficient.

Signed-off-by: Luke Kanies <luke@madstop.com>

6 files changed, 57 insertions, 21 deletions

diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 2549410cf..67c744d39 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -88,7 +88,7 @@ class Puppet::Configurer
         result = nil
         begin
             duration = thinmark do
-                result = catalog_class.find(name, :use_cache => false)
+                result = catalog_class.find(name, :ignore_cache => true)
             end
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
@@ -98,7 +98,7 @@ class Puppet::Configurer
         unless result
             begin
                 duration = thinmark do
-                    result = catalog_class.find(name, :use_cache => true)
+                    result = catalog_class.find(name, :ignore_terminus => true)
                 end
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index 18b764757..5d8cfe9b5 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -186,7 +186,7 @@ class Puppet::Indirector::Indirection
         terminus = prepare(request)
 
         # See if our instance is in the cache and up to date.
-        if cache? and request.use_cache? and cached = cache.find(request)
+        if cache? and ! request.ignore_cache? and cached = cache.find(request)
             if cached.expired?
                 Puppet.info "Not using expired %s for %s from cache; expired at %s" % [self.name, request.key, cached.expiration]
             else
@@ -196,7 +196,7 @@ class Puppet::Indirector::Indirection
         end
 
         # Otherwise, return the result from the terminus, caching if appropriate.
-        if result = terminus.find(request)
+        if ! request.ignore_terminus? and result = terminus.find(request)
             result.expiration ||= self.expiration
             if cache? and request.use_cache?
                 Puppet.info "Caching %s for %s" % [self.name, request.key]
diff --git a/lib/puppet/indirector/request.rb b/lib/puppet/indirector/request.rb
index 4149d0db9..8227db174 100644
--- a/lib/puppet/indirector/request.rb
+++ b/lib/puppet/indirector/request.rb
@@ -4,7 +4,7 @@ require 'puppet/indirector'
 # Indirection call, and as a a result also handles REST calls.  It's somewhat
 # analogous to an HTTP Request object, except tuned for our Indirector.
 class Puppet::Indirector::Request
-    attr_accessor :indirection_name, :key, :method, :options, :instance, :node, :ip, :authenticated, :use_cache
+    attr_accessor :indirection_name, :key, :method, :options, :instance, :node, :ip, :authenticated, :ignore_cache, :ignore_terminus
 
     attr_accessor :server, :port, :uri, :protocol
 
@@ -14,6 +14,19 @@ class Puppet::Indirector::Request
         ! ! authenticated
     end
 
+    # LAK:NOTE This is a messy interface to the cache, and it's only
+    # used by the Configurer class.  I decided it was better to implement
+    # it now and refactor later, when we have a better design, than
+    # to spend another month coming up with a design now that might
+    # not be any better.
+    def ignore_cache?
+        ignore_cache
+    end
+
+    def ignore_terminus?
+        ignore_terminus
+    end
+
     def initialize(indirection_name, method, key, options = {})
         options ||= {}
         raise ArgumentError, "Request options must be a hash, not %s" % options.class unless options.is_a?(Hash)
diff --git a/spec/unit/configurer.rb b/spec/unit/configurer.rb
index 92478eea8..f74cf99f0 100755
--- a/spec/unit/configurer.rb
+++ b/spec/unit/configurer.rb
@@ -85,35 +85,35 @@ describe Puppet::Configurer, "when retrieving a catalog" do
     end
 
     it "should default to returning a catalog retrieved directly from the server, skipping the cache" do
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == false }.returns @catalog
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
 
         @agent.retrieve_catalog.should == @catalog
     end
 
     it "should return the cached catalog when no catalog can be retrieved from the server" do
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == false }.returns nil
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == true }.returns @catalog
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns nil
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns @catalog
 
         @agent.retrieve_catalog.should == @catalog
     end
 
     it "should not look in the cache for a catalog if one is returned from the server" do
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == false }.returns @catalog
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == true }.never
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns @catalog
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.never
 
         @agent.retrieve_catalog.should == @catalog
     end
 
     it "should return the cached catalog when retrieving the remote catalog throws an exception" do
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == false }.raises "eh"
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == true }.returns @catalog
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.raises "eh"
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns @catalog
 
         @agent.retrieve_catalog.should == @catalog
     end
 
     it "should return nil if no cached catalog is available and no catalog can be retrieved from the server" do
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == false }.returns nil
-        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:use_cache] == true }.returns nil
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_cache] == true }.returns nil
+        Puppet::Resource::Catalog.expects(:find).with { |name, options| options[:ignore_terminus] == true }.returns nil
 
         @agent.retrieve_catalog.should be_nil
     end
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 84b4e0e24..dc4157fd2 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -6,7 +6,7 @@ require 'puppet/indirector/indirection'
 
 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 = stub 'request', :node => nil
+        request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
         @indirection.expects(:request).with(@method, "mystuff", :one => :two).returns request
 
@@ -22,7 +22,7 @@ describe "Indirection Delegator", :shared => true do
             end
         end
 
-        request = stub 'request', :key => "me", :options => {}
+        request = Puppet::Indirector::Request.new(:indirection, :find, "me")
 
         @indirection.stubs(:request).returns request
 
@@ -261,8 +261,23 @@ describe Puppet::Indirector::Indirection do
                 it "should not look in the cache if the request specifies not to use the cache" do
                     @terminus.expects(:find).returns @instance
                     @cache.expects(:find).never
+                    @cache.stubs(:save)
+
+                    @indirection.find("/my/key", :ignore_cache => true)
+                end
+
+                it "should still save to the cache even if the cache is being ignored during readin" do
+                    @terminus.expects(:find).returns @instance
+                    @cache.expects(:save)
+
+                    @indirection.find("/my/key", :ignore_cache => true)
+                end
+
+                it "should only look in the cache if the request specifies not to use the terminus" do
+                    @terminus.expects(:find).never
+                    @cache.expects(:find)
 
-                    @indirection.find("/my/key", :use_cache => false)
+                    @indirection.find("/my/key", :ignore_terminus => true)
                 end
 
                 it "should use a request to look in the cache for cached objects" do
diff --git a/spec/unit/indirector/request.rb b/spec/unit/indirector/request.rb
index 5203280d8..12e196f77 100755
--- a/spec/unit/indirector/request.rb
+++ b/spec/unit/indirector/request.rb
@@ -127,12 +127,20 @@ describe Puppet::Indirector::Request do
             end
         end
 
-        it "should allow indication that it should not use a cached instance" do
-            Puppet::Indirector::Request.new(:ind, :method, :key, :use_cache => false).should_not be_use_cache
+        it "should allow indication that it should not read a cached instance" do
+            Puppet::Indirector::Request.new(:ind, :method, :key, :ignore_cache => true).should be_ignore_cache
         end
 
-        it "should default to using cached instances" do
-            Puppet::Indirector::Request.new(:ind, :method, :key).should be_use_cache
+        it "should default to not ignoring the cache" do
+            Puppet::Indirector::Request.new(:ind, :method, :key).should_not be_ignore_cache
+        end
+
+        it "should allow indication that it should not not read an instance from the terminus" do
+            Puppet::Indirector::Request.new(:ind, :method, :key, :ignore_terminus => true).should be_ignore_terminus
+        end
+
+        it "should default to not ignoring the terminus" do
+            Puppet::Indirector::Request.new(:ind, :method, :key).should_not be_ignore_terminus
         end
     end