Fixing #800 by refactoring how configurations are retrieved

from the server.  The real problem was getting all of the validation
done before any caching, which required a good bit more refactoring
than I expected.

In actuality, this commit is relatively small even though it covers
many files; most of the changes just make the code clearer or shorter.

Author: lak

CHANGELOG

@@ -1,3 +1,14 @@
+    Fixed #800 -- invalid configurations are no longer
+    cached.  This was done partially by adding a relationship
+    validation step once the entire configuration is created,
+    but it also required the previously-mentioned changes
+    to how the configuration retrieval process works.
+
+    Removed some functionality from the Master client,
+    since the local functionality has been replaced
+    with the Indirector already, and rearranging how configuration
+    retrieval is done to fix ordering and caching bugs.
+
     The node scope is now above all other scopes besides
     the 'main' scope, which should help make its variables
     visible to other classes, assuming those classes were

lib/puppet/metatype/instances.rb

@@ -99,6 +99,8 @@ def self.create(args)
                 end
             end
 
+            # If they've specified a type and called on the base, then
+            # delegate to the subclass.
             if type
                 if typeklass = self.type(type)
                     return typeklass.create(hash)
@@ -233,19 +235,22 @@ def self.hash2trans(hash)
             hash.delete :name
         end
 
-        unless title
-            raise Puppet::Error,
-                "You must specify a title for objects of type %s" % self.to_s
+        if configuration = hash[:configuration]
+            hash.delete(:configuration)
         end
 
+        raise(Puppet::Error, "You must specify a title for objects of type %s" % self.to_s) unless title
+
         if hash.include? :type
             unless self.validattr? :type
                 hash.delete :type
             end
         end
+
         # okay, now make a transobject out of hash
         begin
             trans = Puppet::TransObject.new(title, self.name.to_s)
+            trans.configuration = configuration if configuration
             hash.each { |param, value|
                 trans[param] = value
             }

lib/puppet/metatype/metaparams.rb

@@ -94,7 +94,7 @@ class Puppet::Type
 
     # We've got four relationship metaparameters, so this method is used
     # to reduce code duplication between them.
-    def store_relationship(param, values)
+    def munge_relationship(param, values)
         # We need to support values passed in as an array or as a
         # resource reference.
         result = []
@@ -194,20 +194,24 @@ def store_relationship(param, values)
             unless aliases.is_a?(Array)
                 aliases = [aliases]
             end
-            @resource.info "Adding aliases %s" % aliases.collect { |a|
-                    a.inspect
-            }.join(", ")
+
+            raise(ArgumentError, "Cannot add aliases without a configuration") unless @resource.configuration
+
+            @resource.info "Adding aliases %s" % aliases.collect { |a| a.inspect }.join(", ")
+
             aliases.each do |other|
-                if obj = @resource.class[other]
-                    unless obj == @resource
-                        self.fail(
-                            "%s can not create alias %s: object already exists" %
-                            [@resource.title, other]
-                        )
+                if obj = @resource.configuration.resource(@resource.class.name, other)
+                    unless obj.object_id == @resource.object_id
+                        self.fail("%s can not create alias %s: object already exists" % [@resource.title, other])
                     end
                     next
                 end
+
+                # LAK:FIXME Old-school, add the alias to the class.
                 @resource.class.alias(other, @resource)
+
+                # Newschool, add it to the configuration.
+                @resource.configuration.alias(@resource, other)
             end
         end
     end
@@ -247,7 +251,16 @@ def self.inherited(sub)
         end
 
         def munge(rels)
-            @resource.store_relationship(self.class.name, rels)
+            @resource.munge_relationship(self.class.name, rels)
+        end
+
+        def validate_relationship
+            @value.each do |value|
+                unless @resource.configuration.resource(*value)
+                    description = self.class.direction == :in ? "dependency" : "dependent"
+                    raise Puppet::Error, "Could not find #{description} %s[%s]" % [value[0].to_s.capitalize, value[1]]
+                end
+            end
         end
 
         # Create edges from each of our relationships.    :in

lib/puppet/network/client/master.rb

@@ -139,63 +139,57 @@ def getconfig
             facts = self.class.facts
         end
 
-        if self.configuration or FileTest.exists?(self.cachefile)
-            if self.fresh?(facts)
-                Puppet.info "Config is up to date"
-                if self.configuration
-                    return
-                end
-                if oldtext = self.retrievecache
-                    begin
-                        @configuration = YAML.load(oldtext).to_configuration
-                    rescue => detail
-                        Puppet.warning "Could not load cached configuration: %s" % detail
-                    end
-                    return
-                end
-            end
-        end
-        Puppet.debug("getting config")
+        raise Puppet::Network::ClientError.new("Could not retrieve any facts") unless facts.length > 0
 
         # Retrieve the plugins.
-        if Puppet[:pluginsync]
-            getplugins()
-        end
+        getplugins() if Puppet[:pluginsync]
 
-        unless facts.length > 0
-            raise Puppet::Network::ClientError.new(
-                "Could not retrieve any facts"
-            )
+        if (self.configuration or FileTest.exist?(self.cachefile)) and self.fresh?(facts)
+            Puppet.info "Configuration is up to date"
+            return if use_cached_config
         end
 
-        unless objects = get_actual_config(facts)
-            @configuration = nil
+        Puppet.debug("Retrieving configuration")
+
+        # If we can't retrieve the configuration, just return, which will either
+        # fail, or use the in-memory configuration.
+        unless yaml_objects = get_actual_config(facts)
+            use_cached_config(true)
             return
         end
 
-        unless objects.is_a?(Puppet::TransBucket)
-            raise NetworkClientError,
-                "Invalid returned objects of type %s" % objects.class
+        begin
+            objects = YAML.load(yaml_objects)
+        rescue => detail
+            msg = "Configuration could not be translated from yaml"
+            msg += "; using cached configuration" if use_cached_config(true)
+            Puppet.warning msg
+            return
         end
 
         self.setclasses(objects.classes)
 
         # Clear all existing objects, so we can recreate our stack.
-        if self.configuration
-            clear()
-        end
+        clear() if self.configuration
 
         # Now convert the objects to a puppet configuration graph.
-        @configuration = objects.to_configuration
+        begin
+            @configuration = objects.to_configuration
+        rescue => detail
+            clear()
+            puts detail.backtrace if Puppet[:trace]
+            msg = "Configuration could not be instantiated: %s" % detail
+            msg += "; using cached configuration" if use_cached_config(true)
+            Puppet.warning msg
+            return
+        end
 
-        if @configuration.nil?
-            raise Puppet::Error, "Configuration could not be processed"
+        if ! @configuration.from_cache
+            self.cache(yaml_objects)
         end
 
         # Keep the state database up to date.
         @configuration.host_config = true
-
-        return @configuration
     end
 
     # A simple proxy method, so it's easy to test.
@@ -270,11 +264,9 @@ def run(options = {})
                     Puppet.err "Could not retrieve configuration: %s" % detail
                 end
 
-                if defined? @configuration and @configuration
+                if self.configuration
                     @configuration.retrieval_duration = duration
-                    unless @local
-                        Puppet.notice "Starting configuration run"
-                    end
+                    Puppet.notice "Starting configuration run" unless @local
                     benchmark(:notice, "Finished configuration run") do
                         @configuration.apply(options)
                     end
@@ -500,34 +492,16 @@ def facts_changed?(facts)
     # Actually retrieve the configuration, either from the server or from a
     # local master.
     def get_actual_config(facts)
-        if @local
-            return get_local_config(facts)
-        else
-            begin
-                Timeout::timeout(self.class.timeout) do
-                    return get_remote_config(facts)
-                end
-            rescue Timeout::Error
-                Puppet.err "Configuration retrieval timed out"
-                return nil
+        begin
+            Timeout::timeout(self.class.timeout) do
+                return get_remote_config(facts)
             end
+        rescue Timeout::Error
+            Puppet.err "Configuration retrieval timed out"
+            return nil
         end
     end
 
-    # Retrieve a configuration from a local master.
-    def get_local_config(facts)
-        # If we're local, we don't have to do any of the conversion
-        # stuff.
-        objects = @driver.getconfig(facts, "yaml")
-        @compile_time = Time.now
-
-        if objects == ""
-            raise Puppet::Error, "Could not retrieve configuration"
-        end
-        
-        return objects
-    end
-    
     # Retrieve a config from a remote master.
     def get_remote_config(facts)
         textobjects = ""
@@ -545,45 +519,18 @@ def get_remote_config(facts)
                 end
 
             rescue => detail
-                puts detail.backtrace
                 Puppet.err "Could not retrieve configuration: %s" % detail
-
-                unless Puppet[:usecacheonfailure]
-                    @configuration = nil
-                    Puppet.warning "Not using cache on failed configuration"
-                    return
-                end
+                return nil
             end
         end
 
-        fromcache = false
-        if textobjects == ""
-            unless textobjects = self.retrievecache
-                raise Puppet::Error.new(
-                    "Cannot connect to server and there is no cached configuration"
-                )
-            end
-            Puppet.warning "Could not get config; using cached copy"
-            fromcache = true
-        else
-            @compile_time = Time.now
-            Puppet::Util::Storage.cache(:configuration)[:facts] = facts
-            Puppet::Util::Storage.cache(:configuration)[:compile_time] = @compile_time
-        end
+        return nil if textobjects == ""
 
-        begin
-            objects = YAML.load(textobjects)
-        rescue => detail
-            raise Puppet::Error,
-                "Could not understand configuration: %s" %
-                detail.to_s
-        end
+        @compile_time = Time.now
+        Puppet::Util::Storage.cache(:configuration)[:facts] = facts
+        Puppet::Util::Storage.cache(:configuration)[:compile_time] = @compile_time
 
-        if @cache and ! fromcache
-            self.cache(textobjects)
-        end
-        
-        return objects
+        return textobjects
     end
 
     def lockfile
@@ -609,4 +556,32 @@ def splay
         Puppet.info "Sleeping for %s seconds (splay is enabled)" % time
         sleep(time)
     end
+
+    private
+
+    # Use our cached config, optionally specifying whether this is
+    # necessary because of a failure.
+    def use_cached_config(because_of_failure = false)
+        return true if self.configuration
+
+        if because_of_failure and ! Puppet[:usecacheonfailure]
+            @configuration = nil
+            Puppet.warning "Not using cache on failed configuration"
+            return false
+        end
+
+        return false unless oldtext = self.retrievecache
+
+        begin
+            @configuration = YAML.load(oldtext).to_configuration
+            @configuration.from_cache = true
+            @configuration.host_config = true
+        rescue => detail
+            puts detail.backtrace if Puppet[:trace]
+            Puppet.warning "Could not load cached configuration: %s" % detail
+            clear
+            return false
+        end
+        return true
+    end
 end

lib/puppet/network/handler/runner.rb

@@ -43,7 +43,7 @@ def run(tags = nil, ignoreschedules = false, fg = true, client = nil, clientip =
             end
 
             if ignoreschedules
-                msg += " without schedules"
+                msg += " ignoring schedules"
             end
 
             Puppet.notice msg