Splitting the Agent class into Agent and Configurer

Once I went to add runinterval support to the Agent class,
I realized it's really two classes:  One that handles starting,
stopping, running, et al (still called Agent), and one that
handles downloading the catalog, running it, etc. (now
called Configurer).

This commit includes some additional code, but 95% of it is just moving code around.

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

7 files changed, 277 insertions, 176 deletions

diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index e2b4ebdc7..c91552b16 100644
--- a/lib/puppet/agent.rb
+++ b/lib/puppet/agent.rb
@@ -1,201 +1,74 @@
-# The client for interacting with the puppetmaster config server.
 require 'sync'
-require 'timeout'
-require 'puppet/network/http_pool'
-require 'puppet/util'
+require 'puppet/daemon'
 
+# A general class for triggering a run of another
+# class.
 class Puppet::Agent
-    require 'puppet/agent/fact_handler'
-    require 'puppet/agent/plugin_handler'
-    require 'puppet/agent/locker'
+    include Puppet::Daemon
 
-    include Puppet::Agent::FactHandler
-    include Puppet::Agent::PluginHandler
+    require 'puppet/agent/locker'
     include Puppet::Agent::Locker
 
-    # For benchmarking
-    include Puppet::Util
-
-    unless defined? @@sync
-        @@sync = Sync.new
-    end
-
-    attr_accessor :catalog
-    attr_reader :compile_time
-
-    class << self
-        # Puppetd should only have one instance running, and we need a way
-        # to retrieve it.
-        attr_accessor :instance
-        include Puppet::Util
-    end
-
-    def clear
-        @catalog.clear(true) if @catalog
-        @catalog = nil
-    end
+    attr_reader :client_class, :client
+    attr_accessor :stopping
 
-    # Initialize and load storage
-    def dostorage
-        begin
-            Puppet::Util::Storage.load
-            @compile_time ||= Puppet::Util::Storage.cache(:configuration)[:compile_time]
-        rescue => detail
-            if Puppet[:trace]
-                puts detail.backtrace
-            end
-            Puppet.err "Corrupt state file %s: %s" % [Puppet[:statefile], detail]
-            begin
-                ::File.unlink(Puppet[:statefile])
-                retry
-            rescue => detail
-                raise Puppet::Error.new("Cannot remove %s: %s" %
-                    [Puppet[:statefile], detail])
-            end
-        end
-    end
-    
     # Just so we can specify that we are "the" instance.
-    def initialize
-        Puppet.settings.use(:main, :ssl, :puppetd)
-
-        self.class.instance = self
-        @running = false
+    def initialize(client_class)
         @splayed = false
-    end
-
-    # Prepare for catalog retrieval.  Downloads everything necessary, etc.
-    def prepare
-        dostorage()
-
-        download_plugins()
 
-        download_fact_plugins()
-
-        upload_facts()
+        @client_class = client_class
     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
-        # If we're currently running, then just mark for later
-        Puppet.notice "Received signal to restart; waiting until run is complete"
-        @restart = true
+    def lockfile_path
+        client_class.lockfile_path
     end
 
-    # Should we restart?
-    def restart?
-        if defined? @restart
-            @restart
-        else
-            false
+    # Perform a run with our client.
+    def run
+        if client
+            Puppet.notice "Run of %s already in progress; skipping" % client_class
+            return
         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.find(name, :use_cache => false)
-            end
-        rescue => detail
-            puts detail.backtrace if Puppet[:trace]
-            Puppet.err "Could not retrieve catalog from remote server: %s" % detail
+        if stopping?
+            Puppet.notice "In shutdown progress; skipping run"
+            return
         end
-
-        unless result
+        splay
+        with_client do |client|
             begin
-                duration = thinmark do
-                    result = catalog_class.find(name, :use_cache => true)
-                end
+                sync.synchronize { lock { client.run } }
             rescue => detail
                 puts detail.backtrace if Puppet[:trace]
-                Puppet.err "Could not retrieve catalog from cache: %s" % detail
+                Puppet.err "Could not run %s: %s" % [client_class, detail]
             end
         end
-
-        return nil unless result
-
-        convert_catalog(result, duration)
     end
 
-    # Convert a plain resource catalog into our full host catalog.
-    def convert_catalog(result, duration)
-        catalog = result.to_ral
-        catalog.retrieval_duration = duration
-        catalog.host_config = true
-        catalog.write_class_file
-        return catalog
-    end
-
-    # The code that actually runs the catalog.  
-    # This just passes any options on to the catalog,
-    # which accepts :tags and :ignoreschedules.
-    def run(options = {})
-        got_lock = false
-        splay
-        Puppet::Util.sync(:puppetrun).synchronize(Sync::EX) do
-            got_lock = lock do
-                unless catalog = retrieve_catalog
-                    Puppet.err "Could not retrieve catalog; skipping run"
-                    return
-                end
-
-                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
-            end
-
-            unless got_lock
-                Puppet.notice "Lock file %s exists; skipping catalog run" % lockfile.lockfile
-                return
+    def shutdown
+        if self.stopping?
+            Puppet.notice "Already in shutdown"
+            return
+        end
+        self.stopping = true
+        if client and client.respond_to?(:stop)
+            begin
+                client.stop
+            rescue
+                puts detail.backtrace if Puppet[:trace]
+                Puppet.err "Could not stop %s: %s" % [client_class, 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.
-            Process.kill(:HUP, $$) if self.restart?
         end
-    end
 
-    def running?
-        lockfile.locked?
+        super
+    ensure
+        self.stopping = false
     end
 
-    private
-
-    def self.timeout
-        timeout = Puppet[:configtimeout]
-        case timeout
-        when String:
-            if timeout =~ /^\d+$/
-                timeout = Integer(timeout)
-            else
-                raise ArgumentError, "Configuration timeout must be an integer"
-            end
-        when Integer: # nothing
-        else
-            raise ArgumentError, "Configuration timeout must be an integer"
-        end
-
-        return timeout
+    def stopping?
+        stopping
     end
 
+    # Have we splayed already?
     def splayed?
         @splayed
     end
@@ -210,4 +83,44 @@ class Puppet::Agent
         sleep(time)
         @splayed = true
     end
+
+    # Start listening for events.  We're pretty much just listening for
+    # timer events here.
+    def start
+        # Create our timer.  Puppet will handle observing it and such.
+        Puppet.newtimer(
+            :interval => Puppet[:runinterval],
+            :tolerance => 1,
+            :start? => true
+        ) do
+            run()
+        end
+
+        # Run once before we start following the timer
+        run()
+    end
+
+    def sync
+        unless defined?(@sync) and @sync
+            @sync = Sync.new
+        end
+        @sync
+    end
+
+    private
+
+    # Create and yield a client instance, keeping a reference
+    # to it during the yield.
+    def with_client
+        begin
+            @client = client_class.new
+        rescue => details
+            puts detail.backtrace if Puppet[:trace]
+            Puppet.err "Could not create instance of %s: %s" % [client_class, details]
+            return
+        end
+        yield @client
+    ensure
+        @client = nil
+    end
 end
diff --git a/lib/puppet/agent/locker.rb b/lib/puppet/agent/locker.rb
index 03736b278..c24fdad64 100644
--- a/lib/puppet/agent/locker.rb
+++ b/lib/puppet/agent/locker.rb
@@ -30,9 +30,14 @@ module Puppet::Agent::Locker
 
     def lockfile
         unless defined?(@lockfile)
-            @lockfile = Puppet::Util::Pidlock.new(Puppet[:puppetdlockfile])
+            #@lockfile = Puppet::Util::Pidlock.new(Puppet[:puppetdlockfile])
+            @lockfile = Puppet::Util::Pidlock.new(lockfile_path)
         end
 
         @lockfile
     end
+
+    def running?
+        lockfile.locked?
+    end
 end
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
new file mode 100644
index 000000000..df531f494
--- /dev/null
+++ b/lib/puppet/configurer.rb
@@ -0,0 +1,185 @@
+# The client for interacting with the puppetmaster config server.
+require 'sync'
+require 'timeout'
+require 'puppet/network/http_pool'
+require 'puppet/util'
+
+class Puppet::Configurer
+    require 'puppet/configurer/fact_handler'
+    require 'puppet/configurer/plugin_handler'
+
+    include Puppet::Configurer::FactHandler
+    include Puppet::Configurer::PluginHandler
+
+    # For benchmarking
+    include Puppet::Util
+
+    attr_accessor :catalog
+    attr_reader :compile_time
+
+    # Provide more helpful strings to the logging that the Agent does
+    def self.to_s
+        "Puppet configuration client"
+    end
+
+    class << self
+        # Puppetd should only have one instance running, and we need a way
+        # to retrieve it.
+        attr_accessor :instance
+        include Puppet::Util
+    end
+
+    # How to lock instances of this class.
+    def self.lockfile_path
+        Puppet[:puppetdlockfile]
+    end
+
+    def clear
+        @catalog.clear(true) if @catalog
+        @catalog = nil
+    end
+
+    # Initialize and load storage
+    def dostorage
+        begin
+            Puppet::Util::Storage.load
+            @compile_time ||= Puppet::Util::Storage.cache(:configuration)[:compile_time]
+        rescue => detail
+            if Puppet[:trace]
+                puts detail.backtrace
+            end
+            Puppet.err "Corrupt state file %s: %s" % [Puppet[:statefile], detail]
+            begin
+                ::File.unlink(Puppet[:statefile])
+                retry
+            rescue => detail
+                raise Puppet::Error.new("Cannot remove %s: %s" %
+                    [Puppet[:statefile], detail])
+            end
+        end
+    end
+    
+    # Just so we can specify that we are "the" instance.
+    def initialize
+        Puppet.settings.use(:main, :ssl, :puppetd)
+
+        self.class.instance = self
+        @running = false
+        @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
+        # If we're currently running, then just mark for later
+        Puppet.notice "Received signal to restart; waiting until run is complete"
+        @restart = true
+    end
+
+    # Should we restart?
+    def restart?
+        if defined? @restart
+            @restart
+        else
+            false
+        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.find(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
+
+        unless result
+            begin
+                duration = thinmark do
+                    result = catalog_class.find(name, :use_cache => true)
+                end
+            rescue => detail
+                puts detail.backtrace if Puppet[:trace]
+                Puppet.err "Could not retrieve catalog from cache: %s" % detail
+            end
+        end
+
+        return nil unless result
+
+        convert_catalog(result, duration)
+    end
+
+    # Convert a plain resource catalog into our full host catalog.
+    def convert_catalog(result, duration)
+        catalog = result.to_ral
+        catalog.retrieval_duration = duration
+        catalog.host_config = true
+        catalog.write_class_file
+        return catalog
+    end
+
+    # The code that actually runs the catalog.  
+    # This just passes any options on to the catalog,
+    # which accepts :tags and :ignoreschedules.
+    def run(options = {})
+        unless catalog = retrieve_catalog
+            Puppet.err "Could not retrieve catalog; skipping run"
+            return
+        end
+
+        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
+
+        # Did we get HUPped during the run?  If so, then restart now that we're
+        # done with the run.
+        Process.kill(:HUP, $$) if self.restart?
+    end
+
+    private
+
+    def self.timeout
+        timeout = Puppet[:configtimeout]
+        case timeout
+        when String:
+            if timeout =~ /^\d+$/
+                timeout = Integer(timeout)
+            else
+                raise ArgumentError, "Configuration timeout must be an integer"
+            end
+        when Integer: # nothing
+        else
+            raise ArgumentError, "Configuration timeout must be an integer"
+        end
+
+        return timeout
+    end
+end
diff --git a/lib/puppet/agent/downloader.rb b/lib/puppet/configurer/downloader.rb
index edc5931c3..f1c4c03b1 100644
--- a/lib/puppet/agent/downloader.rb
+++ b/lib/puppet/configurer/downloader.rb
@@ -1,7 +1,7 @@
 require 'puppet/agent'
 require 'puppet/resource/catalog'
 
-class Puppet::Agent::Downloader
+class Puppet::Configurer::Downloader
     attr_reader :name, :path, :source, :ignore
 
     # Determine the timeout value to use.
diff --git a/lib/puppet/agent/fact_handler.rb b/lib/puppet/configurer/fact_handler.rb
index 266ae1815..9435cb22a 100644
--- a/lib/puppet/agent/fact_handler.rb
+++ b/lib/puppet/configurer/fact_handler.rb
@@ -3,7 +3,7 @@ require 'puppet/indirector/facts/facter'
 # 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
+module Puppet::Configurer::FactHandler
     def download_fact_plugins?
         Puppet[:factsync]
     end
@@ -23,7 +23,7 @@ module Puppet::Agent::FactHandler
     def download_fact_plugins
         return unless download_fact_plugins?
 
-        Puppet::Agent::Downloader.new("fact", Puppet[:factsource], Puppet[:factdest], Puppet[:factsignore]).evaluate
+        Puppet::Configurer::Downloader.new("fact", Puppet[:factsource], Puppet[:factdest], Puppet[:factsignore]).evaluate
     end
 
     # Clear out all of the loaded facts and reload them from disk.
diff --git a/lib/puppet/agent/plugin_handler.rb b/lib/puppet/configurer/plugin_handler.rb
index 306b8b6df..cadf300fd 100644
--- a/lib/puppet/agent/plugin_handler.rb
+++ b/lib/puppet/configurer/plugin_handler.rb
@@ -1,7 +1,7 @@
 # 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
+module Puppet::Configurer::PluginHandler
     def download_plugins?
         Puppet[:pluginsync]
     end
@@ -9,7 +9,7 @@ module Puppet::Agent::PluginHandler
     # 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) }
+        Puppet::Configurer::Downloader.new("plugin", Puppet[:pluginsource], Puppet[:plugindest], Puppet[:pluginsignore]).evaluate.each { |file| load_plugin(file) }
     end
 
     def load_plugin(file)
diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 24d743764..b0576124e 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -76,8 +76,6 @@ module Puppet::Daemon
         Puppet::Util::Log.destinations.reject { |d| d == :console }.each do |dest|
             Puppet::Util::Log.close(dest)
         end
-
-        super
     end
 end