Central logging now works, although there appear to be a few kinks to work out.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@732 980ebf18-57e1-0310-9a29-db15c13687c0

20 files changed, 554 insertions, 189 deletions

diff --git a/bin/puppet b/bin/puppet
index 84c910e97..58b0772b8 100755
--- a/bin/puppet
+++ b/bin/puppet
@@ -118,12 +118,10 @@ begin
             when "--debug"
                 debug = true
             when "--logdest"
-                # FIXME we should be able to have log.rb check the validity of the dst
-                case arg
-                when "syslog", "console", /^\//:
+                begin
                     Puppet[:logdest] = arg
-                else
-                    $stderr.puts "Invalid log destination %s" % arg
+                rescue => detail
+                    $stderr.puts detail.to_s
                 end
             when "--vardir"
                 Puppet[:puppetvar] = arg
diff --git a/bin/puppetd b/bin/puppetd
index 68cfad09d..98d64844f 100755
--- a/bin/puppetd
+++ b/bin/puppetd
@@ -138,8 +138,10 @@ begin
                 exit
             when "--verbose"
                 Puppet[:loglevel] = :info
+                Puppet[:logdest] = :console
             when "--debug"
                 Puppet[:loglevel] = :debug
+                Puppet[:logdest] = :console
             when "--noop"
                 Puppet[:noop] = true
             when "--ssldir"
@@ -151,12 +153,10 @@ begin
             when "--port"
                 args[:Port] = arg
             when "--logdest"
-                # FIXME we should be able to have log.rb check the validity of the dst
-                case arg
-                when "syslog", "console", /^\//:
+                begin
                     Puppet[:logdest] = arg
-                else
-                    $stderr.puts "Invalid log destination %s" % arg
+                rescue => detail
+                    $stderr.puts detail.to_s
                 end
             when "--vardir"
                 Puppet[:puppetvar] = arg
@@ -176,16 +176,28 @@ end
 
 bg = false
 
-unless Puppet[:loglevel] == :debug or Puppet[:loglevel] == :info
-    bg = true
+if Puppet[:loglevel] == :debug or Puppet[:loglevel] == :info
+    args[:Daemonize] = false
+else
+    args[:Daemonize] = true
 end
 
 args[:Server] = server
 if fqdn
     args[:FQDN] = fqdn
 end
+
+logdest = args[:Server]
+
+if args.include?(:Port)
+    logdest += ":" + args[:Port]
+end
+
+Puppet[:logdest] = logdest
+
 client = Puppet::Client::MasterClient.new(args)
 
+
 unless client.readcert
     if waitforcert
         begin
@@ -206,9 +218,6 @@ unless client.readcert
 end
 
 if bg
-    unless Puppet[:logdest] == :file
-        Puppet[:logdest] = Puppet[:logfile]
-    end
     client.daemonize
 end
 
diff --git a/bin/puppetmasterd b/bin/puppetmasterd
index a72f2327d..6471316b2 100755
--- a/bin/puppetmasterd
+++ b/bin/puppetmasterd
@@ -141,6 +141,7 @@ begin
                 Puppet[:puppetconf] = arg
             when "--debug"
                 Puppet[:debug] = true
+                Puppet[:logdest] = :console
             when "--fsconfig"
                 unless FileTest.exists?(arg)
                     $stderr.puts "File server configuration file %s does not exist" %
@@ -170,12 +171,10 @@ begin
             when "--ssldir"
                 Puppet[:ssldir] = arg
             when "--logdest"
-                # FIXME we should be able to have log.rb check the validity of the dst
-                case arg
-                when "syslog", "console", /^\//:
+                begin
                     Puppet[:logdest] = arg
-                else
-                    $stderr.puts "Invalid log destination %s" % arg
+                rescue => detail
+                    $stderr.puts detail.to_s
                 end
             when "--vardir"
                 Puppet[:puppetvar] = arg
@@ -184,6 +183,7 @@ begin
                 exit
             when "--verbose"
                 Puppet[:loglevel] = :info
+                Puppet[:logdest] = :console
             else
                 $stderr.puts "Invalid option '#{opt}'"
                 exit(1)
@@ -199,19 +199,16 @@ rescue GetoptLong::InvalidOption => detail
     exit(1)
 end
 
-bg = false
-
-unless Puppet[:loglevel] == :debug or Puppet[:loglevel] == :info
-    bg = true
-end
-
-if bg
-    Puppet[:logdest] = Puppet[:masterlog]
+if Puppet[:loglevel] == :debug or Puppet[:loglevel] == :info or parseonly
+    args[:Daemonize] = false
+else
+    args[:Daemonize] = true
 end
 
 handlers = {
     :Master => master,
-    :Status => {}
+    :Status => {},
+    :Logger => {}
 }
 
 
@@ -222,9 +219,9 @@ end
 unless fs.include?(:Config)
     if File.exists?(Puppet[:fileserverconfig])
         fs[:Config] = Puppet[:fileserverconfig]
-    else
-        Puppet.notice "File server config %s does not exist; skipping file serving" %
-            Puppet[:fileserverconfig]
+    #else
+    #    Puppet.notice "File server config %s does not exist; skipping file serving" %
+    #        Puppet[:fileserverconfig]
     end
 end
 
@@ -248,10 +245,6 @@ if parseonly
     exit(0)
 end
 
-if bg
-    server.daemonize
-end
-
 trap(:INT) {
     server.shutdown
 }
diff --git a/lib/puppet.rb b/lib/puppet.rb
index 1f4b18d44..7fe07de3d 100644
--- a/lib/puppet.rb
+++ b/lib/puppet.rb
@@ -53,14 +53,13 @@ module Puppet
     @@config = Hash.new(false)
 
     # define helper messages for each of the message levels
-    Puppet::Log.levels.each { |level|
+    Puppet::Log.eachlevel { |level|
         define_method(level,proc { |args|
             if args.is_a?(Array)
                 args = args.join(" ")
             end
             Puppet::Log.create(
                 :level => level,
-                :source => "Puppet",
                 :message => args
             )
         })
@@ -95,8 +94,6 @@ module Puppet
         :parseonly      => false,
         :puppetport     => 8139,
         :masterport     => 8140,
-        :loglevel       => :notice,
-        :logdest        => :file,
     }
     if Process.uid == 0
         @defaults[:puppetconf] = "/etc/puppet"
@@ -126,8 +123,6 @@ module Puppet
             end
         when :loglevel:
             return Puppet::Log.level
-        when :logdest:
-            return Puppet::Log.destination
         else
             # allow manual override
             if @@config.include?(param)
@@ -161,7 +156,7 @@ module Puppet
         when :loglevel:
             Puppet::Log.level=(value)
         when :logdest:
-            Puppet::Log.destination=(value)
+            Puppet::Log.newdestination(value)
         else
             @@config[param] = value
         end
diff --git a/lib/puppet/client.rb b/lib/puppet/client.rb
index b47d235a8..ece131b7b 100644
--- a/lib/puppet/client.rb
+++ b/lib/puppet/client.rb
@@ -483,6 +483,22 @@ module Puppet
             end
         end
 
+        class LogClient < Puppet::Client::ProxyClient
+            @drivername = :Logger
+
+            # set up the appropriate interface methods
+            @handler = Puppet::Server::Logger
+            self.mkmethods
+
+            def initialize(hash = {})
+                if hash.include?(:Logger)
+                    hash[:Logger] = Puppet::Server::Logger.new()
+                end
+
+                super(hash)
+            end
+        end
+
         class StatusClient < Puppet::Client::ProxyClient
             # set up the appropriate interface methods
             @handler = Puppet::Server::ServerStatus
diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 509d6a69d..c69e635b4 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -2,26 +2,26 @@
 
 require 'puppet'
 
-module Puppet
+module Puppet # :nodoc:
+    # A module that handles operations common to all daemons.
     module Daemon
+        # Put the daemon into the background.
         def daemonize
-            unless Puppet[:logdest] == :file
-                raise Puppet::DevError,
-                    "You must reset log destination before daemonizing"
-            end
-
             if pid = fork()
                 Process.detach(pid)
                 exit(0)
             end
 
+            # Get rid of console logging
+            Puppet::Log.close(:console)
+
             Process.setsid
             Dir.chdir("/")
             begin
                 $stdin.reopen "/dev/null"
                 $stdout.reopen "/dev/null", "a"
                 $stderr.reopen $stdout
-                Log.reopen
+                Puppet::Log.reopen
             rescue => detail
                 File.open("/tmp/daemonout", "w") { |f|
                     f.puts "Could not start %s: %s" % [$0, detail]
@@ -29,6 +29,25 @@ module Puppet
                 Puppet.err "Could not start %s: %s" % [$0, detail]
                 exit(12)
             end
+
+            name = $0.gsub(/.+#{File::SEPARATOR}/,'')
+            @pidfile = File.join(Puppet[:puppetvar], name + ".pid")
+            if FileTest.exists?(@pidfile)
+                Puppet.info "Deleting old pid file"
+                begin
+                    File.unlink(@pidfile)
+                rescue Errno::EACCES
+                    Puppet.err "Could not delete old PID file; cannot create new one"
+                    return
+                end
+            end
+
+            begin
+                File.open(@pidfile, "w") { |f| f.puts $$ }
+            rescue => detail
+                Puppet.err "Could not create PID file: %s" % detail
+            end
+            Puppet.info "pid file is %s" % @pidfile
         end
 
         def fqdn
@@ -166,6 +185,23 @@ module Puppet
             end
             return retrieved
         end
+
+        # Shut down our server
+        def shutdown
+            # Remove our pid file
+            if defined? @pidfile and @pidfile and FileTest.exists?(@pidfile)
+                begin
+                    File.unlink(@pidfile)
+                rescue => detail
+                    Puppet.err "Could not remove PID file %s: %s" % [@pidfile, detail]
+                end
+            end
+
+            # And close all logs
+            Puppet::Log.close
+
+            super
+        end
     end
 end
 
diff --git a/lib/puppet/element.rb b/lib/puppet/element.rb
index 2dcabfbc4..a5cd59fb5 100644
--- a/lib/puppet/element.rb
+++ b/lib/puppet/element.rb
@@ -33,7 +33,7 @@ class Puppet::Element
     #---------------------------------------------------------------
 
     # create instance methods for each of the log levels, too
-    Puppet::Log.levels.each { |level|
+    Puppet::Log.eachlevel { |level|
         define_method(level,proc { |args|
             if args.is_a?(Array)
                 args = args.join(" ")
diff --git a/lib/puppet/log.rb b/lib/puppet/log.rb
index 885260f2e..ab2670f13 100644
--- a/lib/puppet/log.rb
+++ b/lib/puppet/log.rb
@@ -1,12 +1,9 @@
-# $Id$
-
 require 'syslog'
 
-module Puppet
-    #------------------------------------------------------------
-    # provide feedback of various types to the user
-    # modeled after syslog messages
-    # each level of message prints in a different color
+module Puppet # :nodoc:
+    # Pass feedback to the user.  Log levels are modeled after syslog's, and it is
+    # expected that that will be the most common log destination.  Supports
+    # multiple destinations, one of which is a remote server.
 	class Log
         PINK=""
         GREEN=""
@@ -16,13 +13,10 @@ module Puppet
         BLUE=""
         RESET=""
 
-		@@messages = Array.new
+        @levels = [:debug,:info,:notice,:warning,:err,:alert,:emerg,:crit]
+        @loglevel = 2
 
-        @@levels = [:debug,:info,:notice,:warning,:err,:alert,:emerg,:crit]
-        @@loglevel = 2
-        @@logdest = :console
-
-		@@colors = {
+		@colors = {
 			:debug => SLATE,
 			:info => GREEN,
 			:notice => PINK,
@@ -33,52 +27,91 @@ module Puppet
             :crit => RESET
 		}
 
-        def Log.close
-            if defined? @@logfile
-                @@logfile.close
-                @@logfile = nil
-            end
+        @destinations = {:syslog => Syslog.open("puppet")}
 
-            if defined? @@syslog
-                @@syslog = nil
+        # Reset all logs to basics.  Basically just closes all files and undefs
+        # all of the other objects.
+        def Log.close(dest = nil)
+            if dest
+                if @destinations.include?(dest)
+                    Puppet.warning "Closing %s" % dest
+                    if @destinations.respond_to?(:close)
+                        @destinations[dest].close
+                    end
+                    @destinations.delete(dest)
+                end
+            else
+                @destinations.each { |type, dest|
+                    if dest.respond_to?(:flush)
+                        dest.flush
+                    end
+                    if dest.respond_to?(:close)
+                        dest.close
+                    end
+                }
+                @destinations = {}
             end
+
+            Puppet.info "closed"
         end
 
+        # Flush any log destinations that support such operations.
         def Log.flush
-            if defined? @@logfile
-                @@logfile.flush
-            end
+            @destinations.each { |type, dest|
+                if dest.respond_to?(:flush)
+                    dest.flush
+                end
+            }
         end
 
+        # Create a new log message.  The primary role of this method is to
+        # avoid creating log messages below the loglevel.
         def Log.create(hash)
-            if @@levels.index(hash[:level]) >= @@loglevel 
+            if @levels.index(hash[:level]) >= @loglevel 
                 return Puppet::Log.new(hash)
             else
                 return nil
             end
         end
 
-        def Log.levels
-            return @@levels.dup
+        # Yield each valid level in turn
+        def Log.eachlevel
+            @levels.each { |level| yield level }
         end
 
-        def Log.destination
-            return @@logdest
+        # Return the current log level.
+        def Log.level
+            return @levels[@loglevel]
         end
 
-        def Log.destination=(dest)
-            if dest == "syslog" or dest == :syslog
-                unless defined? @@syslog
-                    @@syslog = Syslog.open("puppet")
-                end
-                @@logdest = :syslog
-            elsif dest =~ /^\//
-                if defined? @@logfile and @@logfile
-                    @@logfile.close
-                end
+        # Set the current log level.
+        def Log.level=(level)
+            unless level.is_a?(Symbol)
+                level = level.intern
+            end
 
-                @@logpath = dest
+            unless @levels.include?(level)
+                raise Puppet::DevError, "Invalid loglevel %s" % level
+            end
+
+            @loglevel = @levels.index(level)
+        end
 
+        # Create a new log destination.
+        def Log.newdestination(dest)
+            # Each destination can only occur once.
+            if @destinations.include?(dest)
+                return
+            end
+
+            case dest
+            when "syslog", :syslog
+                if Syslog.opened?
+                    Syslog.close
+                end
+                @destinations[:syslog] = Syslog.open("puppet")
+            when /^\// # files
+                Puppet.info "opening %s as a log" % dest
                 # first make sure the directory exists
                 unless FileTest.exist?(File.dirname(dest))
                     begin
@@ -93,80 +126,114 @@ module Puppet
                     end
                 end
 
-                Puppet[:logfile] = dest
                 begin
                     # create the log file, if it doesn't already exist
-                    @@logfile = File.open(dest,File::WRONLY|File::CREAT|File::APPEND)
+                    file = File.open(dest,File::WRONLY|File::CREAT|File::APPEND)
                 rescue => detail
                     Log.destination = :console
                     Puppet.err "Could not create log file: %s" %
                         detail
                     return
                 end
-                @@logdest = :file
+                @destinations[dest] = file
+            when "console", :console
+                @destinations[:console] = :console
+            when Puppet::Server::Logger
+                @destinations[dest] = dest
             else
-                unless @@logdest == :console or @@logdest == "console"
-                    Puppet.notice "Invalid log setting %s; setting log destination to 'console'" % @@logdest
+                Puppet.info "Treating %s as a hostname" % dest
+                args = {}
+                if dest =~ /:(\d+)/
+                    args[:Port] = $1
+                    args[:Server] = dest.sub(/:\d+/, '')
+                else
+                    args[:Server] = dest
                 end
-                @@logdest = :console
-            end
-        end
-
-        def Log.level
-            return @@levels[@@loglevel]
-        end
-
-        def Log.level=(level)
-            unless level.is_a?(Symbol)
-                level = level.intern
-            end
-
-            unless @@levels.include?(level)
-                raise Puppet::DevError, "Invalid loglevel %s" % level
+                @destinations[dest] = Puppet::Client::LogClient.new(args)
             end
-
-            @@loglevel = @@levels.index(level)
         end
 
+        # Route the actual message. FIXME There are lots of things this method should
+        # do, like caching, storing messages when there are not yet destinations,
+        # a bit more.
+        # It's worth noting that there's a potential for a loop here, if
+        # the machine somehow gets the destination set as itself.
         def Log.newmessage(msg)
-            case @@logdest
-            when :syslog:
-                if msg.source == "Puppet"
-                    @@syslog.send(msg.level,msg.to_s)
+            @destinations.each { |type, dest|
+                case dest
+                when Module # This is the Syslog module
+                    next if msg.remote
+                    if msg.source == "Puppet"
+                        dest.send(msg.level,msg.to_s)
+                    else
+                        dest.send(msg.level,"(%s) %s" % [msg.source,msg.to_s])
+                    end
+                when File:
+                    dest.puts("%s %s (%s): %s" %
+                        [msg.time,msg.source,msg.level,msg.to_s])
+                when :console
+                    if msg.source == "Puppet"
+                        puts @colors[msg.level] + "%s: %s" % [
+                            msg.level, msg.to_s
+                        ] + RESET
+                    else
+                        puts @colors[msg.level] + "%s (%s): %s" % [
+                            msg.source, msg.level, msg.to_s
+                        ] + RESET
+                    end
+                when Puppet::Client::LogClient
+                    # For now, to avoid a loop, don't send remote messages
+                    unless msg.is_a?(String) or msg.remote
+                        begin
+                            #puts "would have sent %s" % msg
+                            #puts "would have sent %s" %
+                            #    CGI.escape(Marshal::dump(msg))
+                            dest.addlog(CGI.escape(Marshal::dump(msg)))
+                            #dest.addlog(msg.to_s)
+                            sleep(0.5)
+                        rescue => detail
+                            Puppet.err detail
+                            @destinations.delete(type)
+                        end
+                    end
                 else
-                    @@syslog.send(msg.level,"(%s) %s" % [msg.source,msg.to_s])
+                    #raise Puppet::Error, "Invalid log destination %s" % dest
+                    puts "Invalid log destination %s" % dest.inspect
                 end
-            when :file:
-                unless defined? @@logfile
-                    raise Puppet::DevError,
-                        "Log file must be defined before we can log to it"
+            }
+        end
+
+        # Reopen all of our logs.
+        def Log.reopen
+            types = @destinations.keys
+            @destinations.each { |type, dest|
+                if dest.respond_to?(:close)
+                    dest.close
                 end
-                @@logfile.puts("%s %s (%s): %s" %
-                    [msg.time,msg.source,msg.level,msg.to_s])
-            else
-                if msg.source == "Puppet"
-                    puts @@colors[msg.level] + "%s: %s" % [
-                        msg.level, msg.to_s
-                    ] + RESET
-                else
-                    puts @@colors[msg.level] + "%s (%s): %s" % [
-                        msg.source, msg.level, msg.to_s
-                    ] + RESET
+            }
+            @destinations.clear
+            # We need to make sure we always end up with some kind of destination
+            begin
+                types.each { |type|
+                    Log.newdestination(type)
+                }
+            rescue => detail
+                if @destinations.empty?
+                    Log.newdestination(:syslog)
+                    Puppet.err detail.to_s
                 end
             end
         end
 
-        def Log.reopen
-            if @@logfile
-                Log.destination = @@logpath
-            end
+        # Is the passed level a valid log level?
+        def self.validlevel?(level)
+            @levels.include?(level)
         end
 
-		attr_accessor :level, :message, :source, :time, :tags, :path
+		attr_accessor :level, :message, :source, :time, :tags, :path, :remote
 
 		def initialize(args)
-			unless args.include?(:level) && args.include?(:message) &&
-						args.include?(:source) 
+			unless args.include?(:level) && args.include?(:message)
 				raise Puppet::DevError, "Puppet::Log called incorrectly"
 			end
 
@@ -182,7 +249,7 @@ module Puppet
 			@time = Time.now
 			# this should include the host name, and probly lots of other
 			# stuff, at some point
-			unless @@levels.include?(level)
+			unless self.class.validlevel?(level)
 				raise Puppet::DevError, "Invalid message level #{level}"
 			end
 
@@ -215,17 +282,9 @@ module Puppet
 		end
 
 		def to_s
-			# this probably won't stay, but until this leaves the console,
-			# i'm going to use coloring...
-			#return "#{@time} #{@source} (#{@level}): #{@message}"
-			#return @@colors[@level] + "%s %s (%s): %s" % [
-			#	@time, @source, @level, @message
-			#] + RESET
             return @message
-			#return @@colors[@level] + "%s (%s): %s" % [
-			#	@source, @level, @message
-			#] + RESET
 		end
 	end
-    #------------------------------------------------------------
 end
+
+# $Id$
diff --git a/lib/puppet/metric.rb b/lib/puppet/metric.rb
index 3feb4484c..6e2040eda 100644
--- a/lib/puppet/metric.rb
+++ b/lib/puppet/metric.rb
@@ -241,7 +241,7 @@ module Puppet # :nodoc:
             begin
                 RRD.update(self.path,args.join(":"))
             rescue => detail
-                Puppet.err "Failed to update %s: %s" % [self.name,detail]
+                raise Puppet::Error, "Failed to update %s: %s" % [self.name,detail]
             end
         end
     end
diff --git a/lib/puppet/server.rb b/lib/puppet/server.rb
index b7ed97799..4b40e2bc9 100644
--- a/lib/puppet/server.rb
+++ b/lib/puppet/server.rb
@@ -1,7 +1,3 @@
-#!/usr/local/bin/ruby -w
-
-# $Id$
-
 # the server
 #
 # allow things to connect to us and communicate, and stuff
@@ -32,6 +28,14 @@ module Puppet
             include Puppet::Daemon
 
             def initialize(hash = {})
+                daemonize = nil
+                if hash.include?(:Daemonize)
+                    daemonize = hash[:Daemonize]
+                end
+
+                if daemonize
+                    self.daemonize
+                end
                 # FIXME we should have some kind of access control here, using
                 # :RequestHandler
                 hash[:Port] ||= Puppet[:masterport]
@@ -160,4 +164,7 @@ require 'puppet/server/master'
 require 'puppet/server/ca'
 require 'puppet/server/fileserver'
 require 'puppet/server/filebucket'
+require 'puppet/server/logger'
 require 'puppet/client'
+
+# $Id$
diff --git a/lib/puppet/server/logger.rb b/lib/puppet/server/logger.rb
new file mode 100755
index 000000000..7afad92b9
--- /dev/null
+++ b/lib/puppet/server/logger.rb
@@ -0,0 +1,48 @@
+module Puppet
+class Server # :nodoc:
+    class LoggerError < RuntimeError; end
+
+    # Receive logs from remote hosts.
+    class Logger < Handler
+        @interface = XMLRPC::Service::Interface.new("puppetlogger") { |iface|
+            iface.add_method("void addlog(string)")
+        }
+
+        # accept a log message from a client, and route it accordingly
+        def addlog(message, client = nil, clientip = nil)
+            # if the client is set, then we're not local
+            if client
+                begin
+                    message = Marshal::load(CGI.unescape(message))
+                    #message = message
+                rescue => detail
+                    raise XMLRPC::FaultException.new(
+                        1, "Could not unMarshal log message from %s" % client
+                    )
+                end
+            end
+
+            # Mark it as remote, so it's not sent to syslog
+            message.remote = true
+
+            if client
+                if ! message.source or message.source == "Puppet"
+                #unless message.source or message.source != "Puppet"
+                    message.source = client
+                else
+                    puts message.source.inspect
+                end
+            else
+                puts "No client"
+            end
+
+            Puppet::Log.newmessage(message)
+
+            # This is necessary or XMLRPC gets all pukey
+            return ""
+        end
+    end
+end
+end
+
+# $Id$
diff --git a/lib/puppet/server/servlet.rb b/lib/puppet/server/servlet.rb
index 8301b5692..4474fa640 100644
--- a/lib/puppet/server/servlet.rb
+++ b/lib/puppet/server/servlet.rb
@@ -119,6 +119,7 @@ class Server
                 rescue => detail
                     #Puppet.warning obj.inspect
                     #Puppet.warning args.inspect
+                    puts detail.inspect
                     Puppet.err "Could not call: %s" % detail.to_s
                     raise XMLRPC::FaultException.new(1, detail.to_s)
                 end
diff --git a/lib/puppet/transportable.rb b/lib/puppet/transportable.rb
index 9cd0c237c..a9dea65ec 100644
--- a/lib/puppet/transportable.rb
+++ b/lib/puppet/transportable.rb
@@ -43,7 +43,7 @@ module Puppet
             end
 
             if defined? @tags and @tags
-                Puppet.debug "%s(%s) tags: %s" % [@type, @name, @tags.join(" ")]
+                #Puppet.debug "%s(%s) tags: %s" % [@type, @name, @tags.join(" ")]
                 retobj.tags = @tags
             end
 
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 403c5dd15..ca7a3cfd9 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -1177,7 +1177,7 @@ class Type < Puppet::Element
                 raise Puppet::Error, "Could not retrieve object '%s' of type '%s'" %
                     [name,type]
             end
-            self.debug("%s subscribes to %s" % [self.name,object])
+            self.debug("subscribes to %s" % [object])
 
             #unless @dependencies.include?(object)
             #    @dependencies << object
diff --git a/lib/puppet/type/exec.rb b/lib/puppet/type/exec.rb
index 30480efa3..428c1d021 100755
--- a/lib/puppet/type/exec.rb
+++ b/lib/puppet/type/exec.rb
@@ -16,6 +16,11 @@ module Puppet
                 executed command returns something else."
             @name = :returns
 
+            # Make output a bit prettier
+            def change_to_s
+                return "executed successfully"
+            end
+
             # because this command always runs,
             # we're just using retrieve to verify that the command
             # exists and such
@@ -102,7 +107,7 @@ module Puppet
 
                         # and log
                         @output.split(/\n/).each { |line|
-                            Puppet.send(loglevel, line)
+                            self.send(loglevel, line)
                         }
                     }
                 rescue Errno::ENOENT => detail
diff --git a/test/executables/puppetmasterd.rb b/test/executables/puppetmasterd.rb
index 6b67e5e37..a80190d00 100755
--- a/test/executables/puppetmasterd.rb
+++ b/test/executables/puppetmasterd.rb
@@ -29,6 +29,10 @@ class TestPuppetMasterD < Test::Unit::TestCase
     def test_normalstart
         startmasterd
 
+        pidfile = File.join(Puppet[:puppetvar], "puppetmasterd.pid")
+        assert(FileTest.exists?(pidfile), "PID file does not exist")
+
+        sleep(1)
         assert_nothing_raised {
             socket = TCPSocket.new("127.0.0.1", @@port)
             socket.close
@@ -86,7 +90,7 @@ class TestPuppetMasterD < Test::Unit::TestCase
 
     # verify that we can run puppetmasterd in parse-only mode
     def test_parseonly
-        startmasterd("--parseonly")
+        startmasterd("--parseonly > /dev/null")
         sleep(1)
 
         pid = nil
diff --git a/test/other/log.rb b/test/other/log.rb
index 4f59716d7..5caf68bec 100644
--- a/test/other/log.rb
+++ b/test/other/log.rb
@@ -13,20 +13,20 @@ require 'test/unit'
 
 class TestLog < Test::Unit::TestCase
     include TestPuppet
-    @@logfile = File.join(Puppet[:logdir], "puppettest.log")
 
     def teardown
-        system("rm -f %s" % @@logfile)
-        Puppet::Log.destination=(:console)
-        Puppet[:loglevel] = :notice
+        super
+        Puppet::Log.close
     end
 
     def getlevels
         levels = nil
         assert_nothing_raised() {
-            levels = Puppet::Log.levels
+            levels = []
+            Puppet::Log.eachlevel { |level| levels << level }
         }
-        return levels 
+        # Don't test the top levels; too annoying
+        return levels.reject { |level| level == :emerg or level == :crit }
     end
 
     def mkmsgs(levels)
@@ -42,19 +42,19 @@ class TestLog < Test::Unit::TestCase
     end
 
     def test_logfile
-        Puppet[:debug] = true if __FILE__ == $0
         fact = nil
         levels = nil
         levels = getlevels
+        logfile = tempfile()
         assert_nothing_raised() {
-            Puppet::Log.destination=(@@logfile)
+            Puppet::Log.newdestination(logfile)
         }
         msgs = mkmsgs(levels)
         assert(msgs.length == levels.length)
-        Puppet::Log.flush
+        Puppet::Log.close
         count = 0
         assert_nothing_raised() {
-            File.open(@@logfile) { |of|
+            File.open(logfile) { |of|
                 count = of.readlines.length
             }
         }
@@ -69,7 +69,7 @@ class TestLog < Test::Unit::TestCase
             }
         }
         assert_nothing_raised() {
-            Puppet::Log.destination=("syslog")
+            Puppet::Log.newdestination("syslog")
         }
         # there's really no way to verify that we got syslog messages...
         msgs = mkmsgs(levels)
@@ -79,12 +79,9 @@ class TestLog < Test::Unit::TestCase
     def test_consolelog
         Puppet[:debug] = true if __FILE__ == $0
         fact = nil
-        levels = nil
-        assert_nothing_raised() {
-            levels = Puppet::Log.levels
-        }
+        levels = getlevels
         assert_nothing_raised() {
-            Puppet::Log.destination=(:console)
+            Puppet::Log.newdestination(:console)
         }
         msgs = mkmsgs(levels)
         assert(msgs.length == levels.length)
@@ -93,7 +90,7 @@ class TestLog < Test::Unit::TestCase
 
     def test_levelmethods
         assert_nothing_raised() {
-            Puppet::Log.destination=("/dev/null")
+            Puppet::Log.newdestination("/dev/null")
         }
         getlevels.each { |level|
             assert_nothing_raised() {
@@ -112,15 +109,12 @@ class TestLog < Test::Unit::TestCase
     end
 
     def test_creatingdirs
-        curdest = Puppet[:logdest]
-
         Puppet[:logdest] = "/tmp/logtesting/logfile"
         Puppet.info "testing logs"
         assert(FileTest.directory?("/tmp/logtesting"))
         assert(FileTest.file?("/tmp/logtesting/logfile"))
 
         system("rm -rf /tmp/logtesting")
-        Puppet[:logdest] = curdest
     end
 
     def test_logtags
diff --git a/test/puppettest.rb b/test/puppettest.rb
index 5b108712d..6f6b961f8 100644
--- a/test/puppettest.rb
+++ b/test/puppettest.rb
@@ -37,9 +37,11 @@ module TestPuppet
         @@tmppids = []
 
         if $0 =~ /.+\.rb/
+            Puppet[:logdest] = :console
             Puppet[:loglevel] = :debug
             $VERBOSE = 1
         else
+            Puppet::Log.close
             Puppet[:logdest] = "/dev/null"
             Puppet[:httplog] = "/dev/null"
         end
@@ -90,6 +92,9 @@ module TestPuppet
         @@tmppids.clear
         Puppet::Type.allclear
         Puppet.clear
+
+        # reset all of the logs
+        Puppet::Log.close
     end
 
     def tempfile
@@ -276,6 +281,7 @@ module ExeTest
         }
         assert($? == 0, "Puppetmasterd exit status was %s" % $?)
         assert_equal("", output, "Puppetmasterd produced output %s" % output)
+        sleep(1)
 
         return manifest
     end
@@ -283,20 +289,38 @@ module ExeTest
     def stopmasterd(running = true)
         ps = Facter["ps"].value || "ps -ef"
 
+        pidfile = File.join(Puppet[:puppetvar], "puppetmasterd.pid")
+
         pid = nil
-        %x{#{ps}}.chomp.split(/\n/).each { |line|
-            if line =~ /ruby.+puppetmasterd/
-                next if line =~ /\.rb/ # skip the test script itself
-                ary = line.split(" ")
-                pid = ary[1].to_i
+        if FileTest.exists?(pidfile)
+            pid = File.read(pidfile).chomp.to_i
+            File.unlink(pidfile)
+        end
+
+        if running or pid
+            runningpid = nil
+            %x{#{ps}}.chomp.split(/\n/).each { |line|
+                if line =~ /ruby.+puppetmasterd/
+                    next if line =~ /\.rb/ # skip the test script itself
+                    ary = line.split(/\s+/)
+                    runningpid = ary[1].to_i
+                end
+            }
+
+            if running
+                assert(runningpid, "Process is not running")
+                assert_equal(pid, runningpid, "PIDs are not equal")
+            else
+                return
             end
-        }
+        end
 
         # we default to mandating that it's running, but teardown
         # doesn't require that
-        if running or pid
-            assert(pid)
-
+        if pid
+            if pid == $$
+                raise Puppet::Error, "Tried to kill own pid"
+            end
             assert_nothing_raised {
                 Process.kill("-INT", pid)
             }
diff --git a/test/server/logger.rb b/test/server/logger.rb
new file mode 100644
index 000000000..06b79c786
--- /dev/null
+++ b/test/server/logger.rb
@@ -0,0 +1,176 @@
+if __FILE__ == $0
+    $:.unshift '../../lib'
+    $:.unshift '..'
+    $puppetbase = "../.."
+end
+
+require 'puppet'
+require 'test/unit'
+require 'puppettest.rb'
+require 'base64'
+require 'cgi'
+
+class TestLogger < Test::Unit::TestCase
+	include ServerTest
+
+    # Test the log driver manually
+    def test_localaddlog
+        logger = nil
+        assert_nothing_raised {
+            logger = Puppet::Server::Logger.new
+        }
+
+        msg = nil
+        assert_nothing_raised {
+            msg = Puppet::Log.create(
+                :level => :info,
+                :message => "This is a message"
+            )
+        }
+
+        assert_nothing_raised {
+            logger.addlog(msg)
+        }
+    end
+
+    # Test it while replicating a remote client
+    def test_remoteaddlog
+        logger = nil
+        assert_nothing_raised {
+            logger = Puppet::Server::Logger.new
+        }
+
+        msg = nil
+        assert_nothing_raised {
+            msg = Puppet::Log.create(
+                :level => :info,
+                :message => "This is a remote message"
+            )
+        }
+
+        assert_nothing_raised {
+            msg = CGI.escape(Marshal::dump(msg))
+        }
+        assert_nothing_raised {
+            logger.addlog(msg, "localhost", "127.0.0.1")
+        }
+    end
+
+    # Now test it with a real client and server, but not remote
+    def test_localclient
+        client = nil
+        assert_nothing_raised {
+            client = Puppet::Client::LogClient.new(:Logger => true)
+        }
+
+        msg = nil
+        assert_nothing_raised {
+            msg = Puppet::Log.create(
+                :level => :info,
+                :message => "This is a logclient message"
+            )
+        }
+
+        msg = CGI.escape(Marshal::dump(msg))
+
+        assert_nothing_raised {
+            client.addlog(msg, "localhost", "127.0.0.1")
+        }
+    end
+
+    # And now test over the network
+    def test_logclient
+        Puppet.err :mark
+        pid = nil
+        clientlog = tempfile()
+        serverlog = tempfile()
+        Puppet.warning "serverlog is %s" % serverlog
+        Puppet[:logdest] = clientlog
+        Puppet::Log.close(:syslog)
+
+        # For testing
+        Puppet[:autosign] = true
+
+        logger = nil
+        # Create our server
+        assert_nothing_raised {
+            logger = Puppet::Server.new(
+                :Port => @@port,
+                :Handlers => {
+                    :CA => {}, # so that certs autogenerate
+                    :Logger => {}
+                }
+            )
+        }
+
+        # Start our server
+        serverpid = fork {
+            Puppet::Log.close(clientlog)
+            Puppet[:logdest] = serverlog
+            assert_nothing_raised() {
+                trap(:INT) { logger.shutdown }
+                logger.start
+            }
+        }
+        @@tmppids << serverpid
+        sleep(0.5)
+
+        # Start a raw xmlrpc client
+        client = nil
+        assert_nothing_raised() {
+            client = Puppet::Client::LogClient.new(
+                :Server => "localhost",
+                :Port => @@port
+            )
+            unless client.readcert
+                raise "Could not get certs"
+            end
+        }
+        retval = nil
+
+        {
+            :notice => "XMLRPC1",
+            :warning => "XMLRPC2",
+            :err => "XMLRPC3"
+        }.each { |level, str|
+            msg = CGI.escape(Marshal::dump(Puppet::Log.create(
+                :level => level,
+                :message => str
+            )))
+            assert_nothing_raised {
+                retval = client.addlog(msg)
+            }
+        }
+
+        # and now use the normal client action
+
+        # Set the log destination to be the server
+        Puppet[:logdest] = "localhost:%s" % @@port
+
+        # And now do some logging
+        assert_nothing_raised {
+            Puppet.notice "TEST1"
+            Puppet.warning "TEST2"
+            Puppet.err "TEST3"
+        }
+
+        assert_nothing_raised {
+            Process.kill("INT", serverpid)
+        }
+
+        assert(FileTest.exists?(serverlog), "Server log does not exist")
+
+        # Give it a bit to flush to disk
+        sleep(0.5)
+        content = nil
+        assert_nothing_raised {
+            content = File.read(serverlog)
+        }
+
+        %w{TEST1 TEST2 TEST3}.each { |str|
+            assert(content =~ %r{#{str}}, "Content does not match %s" % str)
+        }
+    end
+end
+
+# $Id$
diff --git a/test/types/type.rb b/test/types/type.rb
index 509a4b402..4347ea0b0 100644
--- a/test/types/type.rb
+++ b/test/types/type.rb
@@ -51,7 +51,7 @@ class TestType < Test::Unit::TestCase
 
     def test_stringvssymbols
         file = nil
-        path = "/tmp/testfile"
+        path = tempfile()
         assert_nothing_raised() {
             system("rm -f %s" % path)
             file = Puppet::Type::PFile.create(