Moving fact and plugin handling into modules

This doesn't change functionality, it just simplifies
the agent class.

I've also started the work to get the catalog handling
done using REST/the Indirector.

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

8 files changed, 596 insertions, 496 deletions

diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index 74c8d7310..f0ee101bf 100644
--- a/lib/puppet/agent.rb
+++ b/lib/puppet/agent.rb
@@ -5,6 +5,12 @@ require 'puppet/network/http_pool'
 require 'puppet/util'
 
 class Puppet::Agent
+    require 'puppet/agent/fact_handler'
+    require 'puppet/agent/plugin_handler'
+
+    include Puppet::Agent::FactHandler
+    include Puppet::Agent::PluginHandler
+
     # For benchmarking
     include Puppet::Util
 
@@ -22,66 +28,6 @@ class Puppet::Agent
         include Puppet::Util
     end
 
-    def self.facts
-        # Retrieve the facts from the central server.
-        if Puppet[:factsync]
-            self.getfacts()
-        end
-        
-        down = Puppet[:downcasefacts]
-
-        Facter.clear
-
-        # Reload everything.
-        if Facter.respond_to? :loadfacts
-            Facter.loadfacts
-        elsif Facter.respond_to? :load
-            Facter.load
-        else
-            Puppet.warning "You should upgrade your version of Facter to at least 1.3.8"
-        end
-
-        # This loads all existing facts and any new ones.  We have to remove and
-        # reload because there's no way to unload specific facts.
-        loadfacts()
-        facts = Facter.to_hash.inject({}) do |newhash, array|
-            name, fact = array
-            if down
-                newhash[name] = fact.to_s.downcase
-            else
-                newhash[name] = fact.to_s
-            end
-            newhash
-        end
-
-        facts
-    end
-
-    # Return the list of dynamic facts as an array of symbols
-    # NOTE:LAK(2008/04/10): This code is currently unused, since we now always
-    # recompile.
-    def self.dynamic_facts
-        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = Puppet.settings[:dynamicfacts].split(/\s*,\s*/).collect { |fact| fact.downcase }
-    end
-
-    # Cache the config
-    def cache(text)
-        Puppet.info "Caching catalog at %s" % self.cachefile
-        confdir = ::File.dirname(Puppet[:localconfig])
-        ::File.open(self.cachefile + ".tmp", "w", 0660) { |f|
-            f.print text
-        }
-        ::File.rename(self.cachefile + ".tmp", self.cachefile)
-    end
-
-    def cachefile
-        unless defined? @cachefile
-            @cachefile = Puppet[:localconfig] + ".yaml"
-        end
-        @cachefile
-    end
-
     def clear
         @catalog.clear(true) if @catalog
         @catalog = nil
@@ -117,74 +63,6 @@ class Puppet::Agent
     def disable
         lockfile.lock(:anonymous => true)
     end
-
-    # Retrieve the config from a remote server.  If this fails, then
-    # use the cached copy.
-    def getconfig
-        dostorage()
-
-        # Retrieve the plugins.
-        getplugins() if Puppet[:pluginsync]
-
-        facts = nil
-        Puppet::Util.benchmark(:debug, "Retrieved facts") do
-            facts = self.class.facts
-        end
-
-        raise Puppet::Error.new("Could not retrieve any facts") unless facts.length > 0
-
-        Puppet.debug("Retrieving catalog")
-
-        # If we can't retrieve the catalog, just return, which will either
-        # fail, or use the in-memory catalog.
-        unless marshalled_objects = get_actual_config(facts)
-            use_cached_config(true)
-            return
-        end
-
-        begin
-            case Puppet[:catalog_format]
-            when "marshal": objects = Marshal.load(marshalled_objects)
-            when "yaml": objects = YAML.load(marshalled_objects)
-            else
-                raise "Invalid catalog format '%s'" % Puppet[:catalog_format]
-            end
-        rescue => detail
-            msg = "Configuration could not be translated from %s" % Puppet[:catalog_format]
-            msg += "; using cached catalog" if use_cached_config(true)
-            Puppet.warning msg
-            return
-        end
-
-        self.setclasses(objects.classes)
-
-        # Clear all existing objects, so we can recreate our stack.
-        clear() if self.catalog
-
-        # Now convert the objects to a puppet catalog graph.
-        begin
-            @catalog = objects.to_catalog
-        rescue => detail
-            clear()
-            puts detail.backtrace if Puppet[:trace]
-            msg = "Configuration could not be instantiated: %s" % detail
-            msg += "; using cached catalog" if use_cached_config(true)
-            Puppet.warning msg
-            return
-        end
-
-        if ! @catalog.from_cache
-            self.cache(marshalled_objects)
-        end
-
-        # Keep the state database up to date.
-        @catalog.host_config = true
-    end
-    
-    # A simple proxy method, so it's easy to test.
-    def getplugins
-        self.class.getplugins
-    end
     
     # Just so we can specify that we are "the" instance.
     def initialize
@@ -195,6 +73,17 @@ class Puppet::Agent
         @splayed = false
     end
 
+    # Prepare for catalog retrieval.  Downloads everything necessary, etc.
+    def prepare
+        dostorage()
+
+        download_plugins()
+
+        download_fact_plugins()
+
+        upload_facts()
+    end
+
     # Mark that we should restart.  The Puppet module checks whether we're running,
     # so this only gets called if we're in the middle of a run.
     def restart
@@ -221,6 +110,37 @@ class Puppet::Agent
         end
     end
 
+    # Get the remote catalog, yo.  Returns nil if no catalog can be found.
+    def retrieve_catalog
+        name = Facter.value("hostname")
+        catalog_class = Puppet::Resource::Catalog
+
+        # First try it with no cache, then with the cache.
+        result = nil
+        begin
+            duration = thinmark do
+                result = catalog_class.get(name, :use_cache => false)
+            end
+        rescue => detail
+            puts detail.backtrace if Puppet[:trace]
+            Puppet.err "Could not retrieve catalog from remote server: %s" % detail
+        end
+
+        begin
+            duration = thinmark do
+                result = catalog_class.get(name, :use_cache => true)
+            end
+        rescue => detail
+            puts detail.backtrace if Puppet[:trace]
+            Puppet.err "Could not retrieve catalog from cache: %s" % detail
+        end
+
+        return nil unless result
+
+        result.retrieval_duration = duration
+        return result
+    end
+
     # The code that actually runs the catalog.  
     # This just passes any options on to the catalog,
     # which accepts :tags and :ignoreschedules.
@@ -228,40 +148,35 @@ class Puppet::Agent
         got_lock = false
         splay
         Puppet::Util.sync(:puppetrun).synchronize(Sync::EX) do
-            if !lockfile.lock
-                Puppet.notice "Lock file %s exists; skipping catalog run" %
-                    lockfile.lockfile
-            else
-                got_lock = true
-                begin
-                    duration = thinmark do
-                        self.getconfig
-                    end
-                rescue => detail
-                    puts detail.backtrace if Puppet[:trace]
-                    Puppet.err "Could not retrieve catalog: %s" % detail
-                end
+            unless lockfile.lock
+                Puppet.notice "Lock file %s exists; skipping catalog run" % lockfile.lockfile
+                return
+            end
 
-                if self.catalog
-                    @catalog.retrieval_duration = duration
-                    Puppet.notice "Starting catalog run" unless @local
-                    benchmark(:notice, "Finished catalog run") do
-                        @catalog.apply(options)
-                    end
-                end
+            got_lock = true
+            unless catalog = retrieve_catalog
+                Puppet.err "Could not retrieve catalog; skipping run"
+                return
+            end
 
-                # Now close all of our existing http connections, since there's no
-                # reason to leave them lying open.
-                Puppet::Network::HttpPool.clear_http_instances
+            begin
+                benchmark(:notice, "Finished catalog run") do
+                    catalog.apply(options)
+                end
+            rescue => detail
+                puts detail.backtrace if Puppet[:trace]
+                Puppet.err "Failed to apply catalog: %s" % detail
             end
+
+            # Now close all of our existing http connections, since there's no
+            # reason to leave them lying open.
+            Puppet::Network::HttpPool.clear_http_instances
             
             lockfile.unlock
 
             # Did we get HUPped during the run?  If so, then restart now that we're
             # done with the run.
-            if self.restart?
-                Process.kill(:HUP, $$)
-            end
+            Process.kill(:HUP, $$) if self.restart?
         end
     ensure
         # Just make sure we remove the lock file if we set it.
@@ -294,113 +209,6 @@ class Puppet::Agent
 
     private
 
-    # Download files from the remote server, returning a list of all
-    # changed files.
-    def self.download(args)
-        hash = {
-            :path => args[:dest],
-            :recurse => true,
-            :source => args[:source],
-            :tag => "#{args[:name]}s",
-            :owner => Process.uid,
-            :group => Process.gid,
-            :purge => true,
-            :force => true,
-            :backup => false,
-            :noop => false
-        }
-
-        if args[:ignore]
-            hash[:ignore] = args[:ignore].split(/\s+/)
-        end
-        downconfig = Puppet::Resource::Catalog.new("downloading")
-        downconfig.add_resource Puppet::Type.type(:file).new(hash)
-        
-        Puppet.info "Retrieving #{args[:name]}s"
-
-        files = []
-        begin
-            Timeout::timeout(self.timeout) do
-                downconfig.apply do |trans|
-                    trans.changed?.find_all do |resource|
-                        yield resource if block_given?
-                        files << resource[:path]
-                    end
-                end
-            end
-        rescue Puppet::Error, Timeout::Error => detail
-            if Puppet[:debug]
-                puts detail.backtrace
-            end
-            Puppet.err "Could not retrieve %ss: %s" % [args[:name], detail]
-        end
-
-        # Now clean up after ourselves
-        downconfig.clear
-
-        return files
-    end
-
-    # Retrieve facts from the central server.
-    def self.getfacts
-        # Download the new facts
-        path = Puppet[:factpath].split(":")
-        files = []
-        download(:dest => Puppet[:factdest], :source => Puppet[:factsource],
-            :ignore => Puppet[:factsignore], :name => "fact") do |resource|
-
-            next unless path.include?(::File.dirname(resource[:path]))
-
-            files << resource[:path]
-        end
-    end
-
-    # Retrieve the plugins from the central server.  We only have to load the
-    # changed plugins, because Puppet::Type loads plugins on demand.
-    def self.getplugins
-        download(:dest => Puppet[:plugindest], :source => Puppet[:pluginsource],
-            :ignore => Puppet[:pluginsignore], :name => "plugin") do |resource|
-
-            next if FileTest.directory?(resource[:path])
-            path = resource[:path].sub(Puppet[:plugindest], '').sub(/^\/+/, '')
-            unless Puppet::Util::Autoload.loaded?(path)
-                next
-            end
-
-            begin
-                Puppet.info "Reloading downloaded file %s" % path
-                load resource[:path]
-            rescue => detail
-                Puppet.warning "Could not reload downloaded file %s: %s" %
-                    [resource[:path], detail]
-            end
-        end
-    end
-
-    def self.loaddir(dir, type)
-        return unless FileTest.directory?(dir)
-
-        Dir.entries(dir).find_all { |e| e =~ /\.rb$/ }.each do |file|
-            fqfile = ::File.join(dir, file)
-            begin
-                Puppet.info "Loading %s %s" % 
-                    [type, ::File.basename(file.sub(".rb",''))]
-                Timeout::timeout(self.timeout) do
-                    load fqfile
-                end
-            rescue => detail
-                Puppet.warning "Could not load %s %s: %s" % [type, fqfile, detail]
-            end
-        end
-    end
-
-    def self.loadfacts
-        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
-        x = Puppet[:factpath].split(":").each do |dir|
-            loaddir(dir, "fact")
-        end
-    end
-    
     def self.timeout
         timeout = Puppet[:configtimeout]
         case timeout
@@ -418,8 +226,6 @@ class Puppet::Agent
         return timeout
     end
 
-    loadfacts()
-
     # Actually retrieve the catalog, either from the server or from a
     # local master.
     def get_actual_config(facts)
@@ -489,6 +295,14 @@ class Puppet::Agent
 
     private
 
+    def retrieve_and_apply_catalog(options)
+        catalog = self.retrieve_catalog
+        Puppet.notice "Starting catalog run"
+        benchmark(:notice, "Finished catalog run") do
+            catalog.apply(options)
+        end
+    end
+
     # Use our cached config, optionally specifying whether this is
     # necessary because of a failure.
     def use_cached_config(because_of_failure = false)
diff --git a/lib/puppet/agent/downloader.rb b/lib/puppet/agent/downloader.rb
index 46a64d52d..edc5931c3 100644
--- a/lib/puppet/agent/downloader.rb
+++ b/lib/puppet/agent/downloader.rb
@@ -1,4 +1,5 @@
 require 'puppet/agent'
+require 'puppet/resource/catalog'
 
 class Puppet::Agent::Downloader
     attr_reader :name, :path, :source, :ignore
@@ -48,7 +49,7 @@ class Puppet::Agent::Downloader
     end
 
     def catalog
-        catalog = Puppet::Node::Catalog.new
+        catalog = Puppet::Resource::Catalog.new
         catalog.add_resource(file)
         catalog
     end
diff --git a/lib/puppet/agent/fact_handler.rb b/lib/puppet/agent/fact_handler.rb
new file mode 100644
index 000000000..4c9280bfc
--- /dev/null
+++ b/lib/puppet/agent/fact_handler.rb
@@ -0,0 +1,71 @@
+# Break out the code related to facts.  This module is
+# just included into the agent, but having it here makes it
+# easier to test.
+module Puppet::Agent::FactHandler
+    def download_fact_plugins?
+        Puppet[:factsync]
+    end
+
+    def upload_facts
+        # XXX down = Puppet[:downcasefacts]
+
+        reload_facter()
+
+        # This works because puppetd configures Facts to use 'facter' for
+        # finding facts and the 'rest' terminus for caching them.  Thus, we'll
+        # compile them and then "cache" them on the server.
+        Puppet::Node::Facts.find(Puppet[:certname])
+    end
+
+    # Retrieve facts from the central server.
+    def download_fact_plugins
+        return unless download_fact_plugins?
+
+        Puppet::Agent::Downloader.new("fact", Puppet[:factsource], Puppet[:factdest], Puppet[:factsignore]).evaluate
+    end
+
+    def load_fact_plugins
+        # LAK:NOTE See http://snurl.com/21zf8  [groups_google_com] 
+        x = Puppet[:factpath].split(":").each do |dir|
+            load_facts_in_dir(dir)
+        end
+    end
+
+    def load_facts_in_dir(dir)
+        return unless FileTest.directory?(dir)
+
+        Dir.chdir(dir) do
+            Dir.glob("*.rb").each do |file|
+                fqfile = ::File.join(dir, file)
+                begin
+                    Puppet.info "Loading facts in %s" % [::File.basename(file.sub(".rb",''))]
+                    Timeout::timeout(Puppet::Agent.timeout) do
+                        load file
+                    end
+                rescue => detail
+                    Puppet.warning "Could not load fact file %s: %s" % [fqfile, detail]
+                end
+            end
+        end
+    end
+
+    # Clear out all of the loaded facts and reload them from disk.
+    # NOTE: This is clumsy and shouldn't be required for later (1.5.x) versions
+    # of Facter.
+    def reload_facter
+        Facter.clear
+
+        # Reload everything.
+        if Facter.respond_to? :loadfacts
+            Facter.loadfacts
+        elsif Facter.respond_to? :load
+            Facter.load
+        else
+            Puppet.warning "You should upgrade your version of Facter to at least 1.3.8"
+        end
+
+        # This loads all existing facts and any new ones.  We have to remove and
+        # reload because there's no way to unload specific facts.
+        load_fact_plugins()
+    end
+end
diff --git a/lib/puppet/agent/plugin_handler.rb b/lib/puppet/agent/plugin_handler.rb
new file mode 100644
index 000000000..306b8b6df
--- /dev/null
+++ b/lib/puppet/agent/plugin_handler.rb
@@ -0,0 +1,25 @@
+# Break out the code related to plugins.  This module is
+# just included into the agent, but having it here makes it
+# easier to test.
+module Puppet::Agent::PluginHandler
+    def download_plugins?
+        Puppet[:pluginsync]
+    end
+
+    # Retrieve facts from the central server.
+    def download_plugins
+        return nil unless download_plugins?
+        Puppet::Agent::Downloader.new("plugin", Puppet[:pluginsource], Puppet[:plugindest], Puppet[:pluginsignore]).evaluate.each { |file| load_plugin(file) }
+    end
+
+    def load_plugin(file)
+        return if FileTest.directory?(file)
+
+        begin
+            Puppet.info "Loading downloaded plugin %s" % file
+            load file
+        rescue Exception => detail
+            Puppet.err "Could not load downloaded file %s: %s" % [file, detail]
+        end
+    end
+end
diff --git a/spec/unit/agent.rb b/spec/unit/agent.rb
index dc2fc9034..c204fa8fb 100755
--- a/spec/unit/agent.rb
+++ b/spec/unit/agent.rb
@@ -6,323 +6,295 @@
 require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/agent'
 
-describe Puppet::Agent, " when retrieving the catalog" do
-    before do
-        Puppet.settings.stubs(:use).returns(true)
-        @client = Puppet::Agent.new
-        @facts = {"one" => "two", "three" => "four"}
+describe Puppet::Agent do
+    it "should include the Plugin Handler module" do
+        Puppet::Agent.ancestors.should be_include(Puppet::Agent::PluginHandler)
     end
 
-    it "should initialize the metadata store" do
-        @client.class.stubs(:facts).returns(@facts)
-        @client.expects(:dostorage)
-        @master.stubs(:getconfig).returns(nil)
-        @client.getconfig
-    end
-
-    it "should collect facts to use for catalog retrieval" do
-        @client.stubs(:dostorage)
-        @client.class.expects(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns(nil)
-        @client.getconfig
-    end
-
-    it "should fail if no facts could be collected" do
-        @client.stubs(:dostorage)
-        @client.class.expects(:facts).returns({})
-        @master.stubs(:getconfig).returns(nil)
-        proc { @client.getconfig }.should raise_error(Puppet::Error)
-    end
-
-    it "should retrieve plugins if :pluginsync is enabled" do
-        file = "/path/to/cachefile"
-        @client.stubs(:cachefile).returns(file)
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        Puppet.settings.expects(:value).with(:pluginsync).returns(true)
-        @client.expects(:getplugins)
-        @client.stubs(:get_actual_config).returns(nil)
-        FileTest.stubs(:exist?).with(file).returns(true)
-        @client.stubs(:use_cached_config).returns(true)
-        @client.class.stubs(:facts).returns(@facts)
-        @client.getconfig
-    end
-
-    it "should use the cached catalog if no catalog could be retrieved" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).raises(ArgumentError.new("whev"))
-        @client.expects(:use_cached_config).with(true)
-        @client.getconfig
-    end
-
-    describe "when the catalog format is set to yaml" do
-        before do
-            Puppet.settings.stubs(:value).returns "foo"
-            Puppet.settings.stubs(:value).with(:pluginsync).returns false
-            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
-            Puppet.settings.stubs(:value).with(:factsync).returns false
-            Puppet.settings.stubs(:value).with(:catalog_format).returns "yaml"
-        end
-
-        it "should request a yaml-encoded catalog" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.expects(:getconfig).with { |*args| args[1] == "yaml" }
+    it "should include the Fact Handler module" do
+        Puppet::Agent.ancestors.should be_include(Puppet::Agent::FactHandler)
+    end
+end
 
-            @client.getconfig
-        end
+describe Puppet::Agent, "when executing a catalog run" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
+        @agent.stubs(:splay)
 
-        it "should load the retrieved catalog using YAML" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @lockfile = stub 'lockfile', :lock => true, :locked? => false, :lockfile => "/my/lock/file", :unlock => true
 
-            config = mock 'config'
-            YAML.expects(:load).with("myconfig").returns(config)
+        @agent.stubs(:lockfile).returns @lockfile
+    end
 
-            @client.stubs(:setclasses)
+    it "should splay" do
+        Puppet::Util.sync(:puppetrun).stubs(:synchronize)
+        @agent.expects(:splay)
+        @agent.run
+    end
 
-            config.stubs(:classes)
-            config.stubs(:to_catalog).returns(config)
-            config.stubs(:host_config=)
-            config.stubs(:from_cache).returns(true)
+    it "should use a global mutex to make sure no other thread is executing the catalog" do
+        sync = mock 'sync'
+        Puppet::Util.expects(:sync).with(:puppetrun).returns sync
 
-            @client.getconfig
-        end
+        sync.expects(:synchronize)
 
-        it "should use the cached catalog if the retrieved catalog cannot be converted from YAML" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @agent.expects(:retrieve_config).never # i.e., if we don't yield, we don't retrieve the config
+        @agent.run
+    end
 
-            YAML.expects(:load).with("myconfig").raises(ArgumentError)
+    it "should use a lockfile to make sure no other process is executing the catalog" do
+        @lockfile.expects(:lock).returns true
 
-            @client.expects(:use_cached_config).with(true)
+        @agent.expects(:retrieve_catalog)
 
-            @client.getconfig
-        end
+        @agent.run
     end
 
-    describe "from Marshal" do
-        before do
-            Puppet.settings.stubs(:value).returns "foo"
-            Puppet.settings.stubs(:value).with(:pluginsync).returns false
-            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
-            Puppet.settings.stubs(:value).with(:factsync).returns false
-            Puppet.settings.stubs(:value).with(:catalog_format).returns "marshal"
-        end
+    it "should log and do nothing if the lock cannot be acquired" do
+        @lockfile.expects(:lock).returns false
 
-        it "should load the retrieved catalog using Marshal" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @agent.expects(:retrieve_catalog).never
 
-            config = mock 'config'
-            Marshal.expects(:load).with("myconfig").returns(config)
+        Puppet.expects(:notice)
 
-            @client.stubs(:setclasses)
+        @agent.run
+    end
 
-            config.stubs(:classes)
-            config.stubs(:to_catalog).returns(config)
-            config.stubs(:host_config=)
-            config.stubs(:from_cache).returns(true)
+    it "should retrieve the catalog" do
+        @agent.expects(:retrieve_catalog)
 
-            @client.getconfig
-        end
+        @agent.run
+    end
 
-        it "should use the cached catalog if the retrieved catalog cannot be converted from Marshal" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+    it "should log a failure and do nothing if no catalog can be retrieved" do
+        @agent.expects(:retrieve_catalog).returns nil
 
-            Marshal.expects(:load).with("myconfig").raises(ArgumentError)
+        Puppet.expects(:err)
 
-            @client.expects(:use_cached_config).with(true)
+        @agent.run
+    end
 
-            @client.getconfig
-        end
+    it "should apply the catalog with all options to :run" do
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
+
+        catalog.expects(:apply).with(:one => true)
+        @agent.run :one => true
     end
+    
+    it "should benchmark how long it takes to apply the catalog" do
+        @agent.expects(:benchmark).with(:notice, "Finished catalog run")
 
-    it "should set the classes.txt file with the classes listed in the retrieved catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        config = mock 'config'
-        YAML.expects(:load).with("myconfig").returns(config)
+        catalog.expects(:apply).never # because we're not yielding
+        @agent.run
+    end
+
+    it "should remove the lock file when done applying the catalog" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        config.expects(:classes).returns(:myclasses)
-        @client.expects(:setclasses).with(:myclasses)
+        @lockfile.expects(:lock).returns true
 
-        config.stubs(:to_catalog).returns(config)
-        config.stubs(:host_config=)
-        config.stubs(:from_cache).returns(true)
+        @lockfile.expects(:unlock)
 
-        @client.getconfig
+        @agent.run
     end
 
-    it "should convert the retrieved catalog to a RAL catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should remove the lock file even if there was an exception during the run" do
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        catalog.expects(:apply).raises "eh"
 
-        @client.stubs(:setclasses)
+        @lockfile.expects(:unlock)
+        @agent.run
+    end
 
-        config = mock 'config'
+    it "should HUP itself if it should be restarted" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        Process.expects(:kill).with(:HUP, $$)
 
-        config.stubs(:host_config=)
-        config.stubs(:from_cache).returns(true)
+        @agent.expects(:restart?).returns true
 
-        @client.getconfig
+        @agent.run
     end
 
-    it "should use the cached catalog if the retrieved catalog cannot be converted to a RAL catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should not HUP itself if it should not be restarted" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        Process.expects(:kill).never
 
-        @client.stubs(:setclasses)
+        @agent.expects(:restart?).returns false
 
-        config = mock 'config'
+        @agent.run
+    end
+end
+
+describe Puppet::Agent, "when retrieving a catalog" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).raises(ArgumentError)
+        @catalog = stub 'catalog', :retrieval_duration= => nil
+    end
 
-        @client.expects(:use_cached_config).with(true)
+    it "should use the Catalog class to get its catalog" do
+        Puppet::Resource::Catalog.expects(:get).returns @catalog
 
-        @client.getconfig
+        @agent.retrieve_catalog
     end
 
-    it "should clear the failed catalog if using the cached catalog after failing to instantiate the retrieved catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should use its Facter name to retrieve the catalog" do
+        Facter.stubs(:value).returns "eh"
+        Facter.expects(:value).with("hostname").returns "myhost"
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| name == "myhost" }.returns @catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        @agent.retrieve_catalog
+    end
 
-        @client.stubs(:setclasses)
+    it "should default to returning a catalog retrieved directly from the server, skipping the cache" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns @catalog
 
-        config = mock 'config'
+        @agent.retrieve_catalog.should == @catalog
+    end
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.stubs(:to_catalog).raises(ArgumentError)
+    it "should return the cached catalog when no catalog can be retrieved from the server" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns nil
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns @catalog
 
-        @client.stubs(:use_cached_config).with(true)
+        @agent.retrieve_catalog.should == @catalog
+    end
 
-        @client.expects(:clear)
+    it "should return the cached catalog when retrieving the remote catalog throws an exception" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.raises "eh"
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns @catalog
 
-        @client.getconfig
+        @agent.retrieve_catalog.should == @catalog
     end
 
-    it "should cache the retrieved yaml catalog if it is not from the cache and is valid" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should return nil if no cached catalog is available and no catalog can be retrieved from the server" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns nil
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns nil
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        @agent.retrieve_catalog.should be_nil
+    end
 
-        @client.stubs(:setclasses)
+    it "should record the retrieval time with the catalog" do
+        @agent.expects(:thinmark).yields.then.returns 10
 
-        config = mock 'config'
+        catalog = mock 'catalog'
+        Puppet::Resource::Catalog.expects(:get).returns catalog
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        catalog.expects(:retrieval_duration=).with 10
 
-        config.stubs(:host_config=)
+        @agent.retrieve_catalog
+    end
 
-        config.expects(:from_cache).returns(false)
+    it "should update the class file with the classes contained within the catalog"
 
-        @client.expects(:cache).with("myconfig")
+    it "should mark the catalog as a host catalog"
 
-        @client.getconfig
-    end
+    it "should return nil if there is an error while retrieving the catalog" do
+        Puppet::Resource::Catalog.expects(:get).raises "eh"
 
-    it "should mark the catalog as a host catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+        @agent.retrieve_catalog.should be_nil
+    end
+end
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+describe Puppet::Agent, "when preparing for a run" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
+        @agent.stubs(:dostorage)
+        @agent.stubs(:upload_facts)
+        @facts = {"one" => "two", "three" => "four"}
+    end
 
-        @client.stubs(:setclasses)
+    it "should initialize the metadata store" do
+        @agent.class.stubs(:facts).returns(@facts)
+        @agent.expects(:dostorage)
+        @agent.prepare
+    end
 
-        config = mock 'config'
+    it "should download fact plugins" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:download_fact_plugins)
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        @agent.prepare
+    end
 
-        config.stubs(:from_cache).returns(true)
+    it "should download plugins" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:download_plugins)
 
-        config.expects(:host_config=).with(true)
+        @agent.prepare
+    end
 
-        @client.getconfig
+    it "should upload facts to use for catalog retrieval" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:upload_facts)
+        @agent.prepare
     end
 end
 
 describe Puppet::Agent, " when using the cached catalog" do
     before do
         Puppet.settings.stubs(:use).returns(true)
-        @client = Puppet::Agent.new
+        @agent = Puppet::Agent.new
         @facts = {"one" => "two", "three" => "four"}
     end
 
     it "should return do nothing and true if there is already an in-memory catalog" do
-        @client.catalog = :whatever
+        @agent.catalog = :whatever
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config.should be_true
+            @agent.use_cached_config.should be_true
         end
     end
 
     it "should return do nothing and false if it has been told there is a failure and :nocacheonfailure is enabled" do
         Puppet.settings.expects(:value).with(:usecacheonfailure).returns(false)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config(true).should be_false
+            @agent.use_cached_config(true).should be_false
         end
     end
 
     it "should return false if no cached catalog can be found" do
-        @client.expects(:retrievecache).returns(nil)
+        @agent.expects(:retrievecache).returns(nil)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should return false if the cached catalog cannot be instantiated" do
         YAML.expects(:load).raises(ArgumentError)
-        @client.expects(:retrievecache).returns("whatever")
+        @agent.expects(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should warn if the cached catalog cannot be instantiated" do
         YAML.stubs(:load).raises(ArgumentError)
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet.expects(:warning).with { |m| m.include?("Could not load cache") }
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should clear the client if the cached catalog cannot be instantiated" do
         YAML.stubs(:load).raises(ArgumentError)
-        @client.stubs(:retrievecache).returns("whatever")
-        @client.expects(:clear)
+        @agent.stubs(:retrievecache).returns("whatever")
+        @agent.expects(:clear)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
@@ -335,9 +307,9 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_true
+            @agent.use_cached_config().should be_true
         end
     end
 
@@ -350,12 +322,12 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     it "should mark the catalog as a host_config if valid" do
@@ -367,12 +339,12 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.expects(:host_config=).with(true)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     it "should mark the catalog as from the cache if valid" do
@@ -384,19 +356,19 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     describe "when calling splay" do
         it "should do nothing if splay is not enabled" do
             Puppet.stubs(:[]).with(:splay).returns(false)
-            @client.expects(:rand).never
-            @client.send(:splay)
+            @agent.expects(:rand).never
+            @agent.send(:splay)
         end
 
         describe "when splay is enabled" do
@@ -406,30 +378,30 @@ describe Puppet::Agent, " when using the cached catalog" do
             end
 
             it "should sleep for a random time plus 1" do
-                @client.expects(:rand).with(43).returns(43)
-                @client.expects(:sleep).with(43)
-                @client.send(:splay)
+                @agent.expects(:rand).with(43).returns(43)
+                @agent.expects(:sleep).with(43)
+                @agent.send(:splay)
             end
 
             it "should inform that it is splayed" do
-                @client.stubs(:rand).with(43).returns(43)
-                @client.stubs(:sleep).with(43)
+                @agent.stubs(:rand).with(43).returns(43)
+                @agent.stubs(:sleep).with(43)
                 Puppet.expects(:info)
-                @client.send(:splay)
+                @agent.send(:splay)
             end
 
             it "should set splay = true" do
-                @client.stubs(:rand).returns(43)
-                @client.stubs(:sleep)
-                @client.send(:splay)
-                @client.send(:splayed?).should == true
+                @agent.stubs(:rand).returns(43)
+                @agent.stubs(:sleep)
+                @agent.send(:splay)
+                @agent.send(:splayed?).should == true
             end
 
             it "should do nothing if already splayed" do
-                @client.stubs(:rand).returns(43).at_most_once
-                @client.stubs(:sleep).at_most_once
-                @client.send(:splay)
-                @client.send(:splay)
+                @agent.stubs(:rand).returns(43).at_most_once
+                @agent.stubs(:sleep).at_most_once
+                @agent.send(:splay)
+                @agent.send(:splay)
             end
         end 
     end
diff --git a/spec/unit/agent/downloader.rb b/spec/unit/agent/downloader.rb
index 6b07e5bb4..28c5d030f 100755
--- a/spec/unit/agent/downloader.rb
+++ b/spec/unit/agent/downloader.rb
@@ -99,7 +99,7 @@ describe Puppet::Agent::Downloader do
 
             @dler.expects(:file).returns file
 
-            Puppet::Node::Catalog.expects(:new).returns catalog
+            Puppet::Resource::Catalog.expects(:new).returns catalog
             catalog.expects(:add_resource).with(file)
 
             @dler.catalog.should equal(catalog)
diff --git a/spec/unit/agent/fact_handler.rb b/spec/unit/agent/fact_handler.rb
new file mode 100755
index 000000000..b58f55ebc
--- /dev/null
+++ b/spec/unit/agent/fact_handler.rb
@@ -0,0 +1,117 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet/agent'
+require 'puppet/agent/fact_handler'
+
+class FactHandlerTester
+    include Puppet::Agent::FactHandler
+end
+
+describe Puppet::Agent::FactHandler do
+    before do
+        @facthandler = FactHandlerTester.new
+    end
+
+    it "should have a method for downloading fact plugins" do
+        @facthandler.should respond_to(:download_fact_plugins)
+    end
+
+    it "should have a boolean method for determining whether fact plugins should be downloaded" do
+        @facthandler.should respond_to(:download_fact_plugins?)
+    end
+
+    it "should download fact plugins when :factsync is true" do
+        Puppet.settings.expects(:value).with(:factsync).returns true
+        @facthandler.should be_download_fact_plugins
+    end
+
+    it "should not download fact plugins when :factsync is false" do
+        Puppet.settings.expects(:value).with(:factsync).returns false
+        @facthandler.should_not be_download_fact_plugins
+    end
+
+    it "should not download fact plugins when downloading is disabled" do
+        Puppet::Agent::Downloader.expects(:new).never
+        @facthandler.expects(:download_fact_plugins?).returns false
+        @facthandler.download_fact_plugins
+    end
+
+    it "should use an Agent Downloader, with the name, source, destination, and ignore set correctly, to download fact plugins when downloading is enabled" do
+        downloader = mock 'downloader'
+
+        Puppet.settings.expects(:value).with(:factsource).returns "fsource"
+        Puppet.settings.expects(:value).with(:factdest).returns "fdest"
+        Puppet.settings.expects(:value).with(:factsignore).returns "fignore"
+
+        Puppet::Agent::Downloader.expects(:new).with("fact", "fsource", "fdest", "fignore").returns downloader
+
+        downloader.expects(:evaluate)
+
+        @facthandler.expects(:download_fact_plugins?).returns true
+        @facthandler.download_fact_plugins
+    end
+
+    it "should have a method for uploading facts" do
+        @facthandler.should respond_to(:upload_facts)
+    end
+
+    it "should reload Facter and find local facts when asked to upload facts" do
+        @facthandler.expects(:reload_facter)
+
+        Puppet.settings.expects(:value).with(:certname).returns "myhost"
+        Puppet::Node::Facts.expects(:find).with("myhost")
+
+        @facthandler.upload_facts
+    end
+
+    describe "when reloading Facter" do
+        before do
+            Facter.stubs(:clear)
+            Facter.stubs(:load)
+            Facter.stubs(:loadfacts)
+        end
+
+        it "should clear Facter" do
+            Facter.expects(:clear)
+            @facthandler.reload_facter
+        end
+
+        it "should load all Facter facts" do
+            Facter.expects(:loadfacts)
+            @facthandler.reload_facter
+        end
+
+        it "should load all Puppet Fact plugins" do
+            @facthandler.expects(:load_fact_plugins)
+            @facthandler.reload_facter
+        end
+    end
+
+    it "should load each directory in the Fact path when loading fact plugins" do
+        Puppet.settings.expects(:value).with(:factpath).returns("one%stwo" % File::PATH_SEPARATOR)
+
+        @facthandler.expects(:load_facts_in_dir).with("one")
+        @facthandler.expects(:load_facts_in_dir).with("two")
+
+        @facthandler.load_fact_plugins
+    end
+
+    it "should skip files when asked to load a directory" do
+        FileTest.expects(:directory?).with("myfile").returns false
+
+        @facthandler.load_facts_in_dir("myfile")
+    end
+
+    it "should load each ruby file when asked to load a directory" do
+        FileTest.expects(:directory?).with("mydir").returns true
+        Dir.expects(:chdir).with("mydir").yields
+
+        Dir.expects(:glob).with("*.rb").returns %w{a.rb b.rb}
+
+        @facthandler.expects(:load).with("a.rb")
+        @facthandler.expects(:load).with("b.rb")
+
+        @facthandler.load_facts_in_dir("mydir")
+    end
+end
diff --git a/spec/unit/agent/plugin_handler.rb b/spec/unit/agent/plugin_handler.rb
new file mode 100755
index 000000000..44603bc6c
--- /dev/null
+++ b/spec/unit/agent/plugin_handler.rb
@@ -0,0 +1,100 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet/agent'
+require 'puppet/agent/plugin_handler'
+
+class PluginHandlerTester
+    include Puppet::Agent::PluginHandler
+end
+
+describe Puppet::Agent::PluginHandler do
+    before do
+        @pluginhandler = PluginHandlerTester.new
+    end
+
+    it "should have a method for downloading plugins" do
+        @pluginhandler.should respond_to(:download_plugins)
+    end
+
+    it "should have a boolean method for determining whether plugins should be downloaded" do
+        @pluginhandler.should respond_to(:download_plugins?)
+    end
+
+    it "should download plugins when :pluginsync is true" do
+        Puppet.settings.expects(:value).with(:pluginsync).returns true
+        @pluginhandler.should be_download_plugins
+    end
+
+    it "should not download plugins when :pluginsync is false" do
+        Puppet.settings.expects(:value).with(:pluginsync).returns false
+        @pluginhandler.should_not be_download_plugins
+    end
+
+    it "should not download plugins when downloading is disabled" do
+        Puppet::Agent::Downloader.expects(:new).never
+        @pluginhandler.expects(:download_plugins?).returns false
+        @pluginhandler.download_plugins
+    end
+
+    it "should use an Agent Downloader, with the name, source, destination, and ignore set correctly, to download plugins when downloading is enabled" do
+        downloader = mock 'downloader'
+
+        Puppet.settings.expects(:value).with(:pluginsource).returns "psource"
+        Puppet.settings.expects(:value).with(:plugindest).returns "pdest"
+        Puppet.settings.expects(:value).with(:pluginsignore).returns "pignore"
+
+        Puppet::Agent::Downloader.expects(:new).with("plugin", "psource", "pdest", "pignore").returns downloader
+
+        downloader.expects(:evaluate).returns []
+
+        @pluginhandler.expects(:download_plugins?).returns true
+        @pluginhandler.download_plugins
+    end
+
+    it "should be able to load plugins" do
+        @pluginhandler.should respond_to(:load_plugin)
+    end
+
+    it "should load each downloaded file" do
+        downloader = mock 'downloader'
+
+        Puppet::Agent::Downloader.expects(:new).returns downloader
+
+        downloader.expects(:evaluate).returns %w{one two}
+
+        @pluginhandler.expects(:download_plugins?).returns true
+
+        @pluginhandler.expects(:load_plugin).with("one")
+        @pluginhandler.expects(:load_plugin).with("two")
+
+        @pluginhandler.download_plugins
+    end
+
+    it "should load plugins when asked to do so" do
+        @pluginhandler.expects(:load).with("foo")
+
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should not try to load directories" do
+        FileTest.expects(:directory?).with("foo").returns true
+        @pluginhandler.expects(:load).never
+
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should warn but not fail if loading a file raises an exception" do
+        @pluginhandler.expects(:load).with("foo").raises "eh"
+
+        Puppet.expects(:err)
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should warn but not fail if loading a file raises a LoadError" do
+        @pluginhandler.expects(:load).with("foo").raises LoadError.new("eh")
+
+        Puppet.expects(:err)
+        @pluginhandler.load_plugin("foo")
+    end
+end