diff options
| -rw-r--r-- | lib/puppet.rb | 7 | ||||
| -rw-r--r-- | lib/puppet/client.rb | 6 | ||||
| -rwxr-xr-x | lib/puppet/daemon.rb | 3 | ||||
| -rw-r--r-- | lib/puppet/lock.rb | 64 | ||||
| -rw-r--r-- | lib/puppet/storage.rb | 18 | ||||
| -rw-r--r-- | lib/puppet/util.rb | 58 | ||||
| -rwxr-xr-x | test/puppet/utiltest.rb | 33 | ||||
| -rwxr-xr-x | test/types/schedule.rb | 2 |
8 files changed, 169 insertions, 22 deletions
diff --git a/lib/puppet.rb b/lib/puppet.rb index 926dbb02c..dacaa0f34 100644 --- a/lib/puppet.rb +++ b/lib/puppet.rb @@ -122,10 +122,11 @@ module Puppet directory can be removed without causing harm (although it might result in spurious service restarts)."], [:rundir, "$vardir/run", "Where Puppet PID files are kept."], + [:lockdir, "$vardir/locks", "Where lock files are kept."], [:statefile, "$statedir/state.yaml", - "Where puppetd and puppetmasterd store state associated with the running - configuration. In the case of puppetmasterd, this file reflects the - state discovered through interacting with clients."], + "Where puppetd and puppetmasterd store state associated with the + running configuration. In the case of puppetmasterd, this file + reflects the state discovered through interacting with clients."], [:ssldir, "$confdir/ssl", "Where SSL certificates are kept."], [:genconfig, false, "Whether to just print a configuration to stdout and exit. Only makes diff --git a/lib/puppet/client.rb b/lib/puppet/client.rb index dcf3dd439..f0f0968bf 100644 --- a/lib/puppet/client.rb +++ b/lib/puppet/client.rb @@ -11,9 +11,9 @@ module Puppet include Puppet include SignalObserver - # FIXME the cert stuff should only come up with networking, so it - # should be in the network client, not the normal client - # but if i do that, it's hard to tell whether the certs have been initialized + # FIXME The cert stuff should only come up with networking, so it + # should be in the network client, not the normal client. But if i do + # that, it's hard to tell whether the certs have been initialized. include Puppet::Daemon attr_reader :local, :secureinit attr_accessor :schedule, :lastrun diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb index fcedff3d7..9f91e183f 100755 --- a/lib/puppet/daemon.rb +++ b/lib/puppet/daemon.rb @@ -221,7 +221,8 @@ module Puppet begin File.unlink(@pidfile) rescue => detail - Puppet.err "Could not remove PID file %s: %s" % [@pidfile, detail] + Puppet.err "Could not remove PID file %s: %s" % + [@pidfile, detail] end end diff --git a/lib/puppet/lock.rb b/lib/puppet/lock.rb new file mode 100644 index 000000000..d873d1bbf --- /dev/null +++ b/lib/puppet/lock.rb @@ -0,0 +1,64 @@ +require 'thread' +require 'sync' + +# Gotten from: +# http://path.berkeley.edu/~vjoel/ruby/solaris-bug.rb + +# Extensions to the File class for exception-safe file locking in a +# environment with multiple user threads. + +# This is here because closing a file on solaris unlocks any locks that +# other threads might have. So we have to make sure that only the last +# reader thread closes the file. +# +# The hash maps inode number to a count of reader threads +$reader_count = Hash.new(0) + +class File + # Get an exclusive (i.e., write) lock on the file, and yield to the block. + # If the lock is not available, wait for it without blocking other ruby + # threads. + def lock_exclusive + if Thread.list.size == 1 + flock(LOCK_EX) + else + # ugly hack because waiting for a lock in a Ruby thread blocks the + # process + period = 0.001 + until flock(LOCK_EX|LOCK_NB) + sleep period + period *= 2 if period < 1 + end + end + + yield self + ensure + flush + flock(LOCK_UN) + end + + # Get a shared (i.e., read) lock on the file, and yield to the block. + # If the lock is not available, wait for it without blocking other ruby + # threads. + def lock_shared + if Thread.list.size == 1 + flock(LOCK_SH) + else + # ugly hack because waiting for a lock in a Ruby thread blocks the + # process + period = 0.001 + until flock(LOCK_SH|LOCK_NB) + sleep period + period *= 2 if period < 1 + end + end + + yield self + ensure + Thread.exclusive {flock(LOCK_UN) if $reader_count[self.stat.ino] == 1} + ## for solaris, no need to unlock here--closing does it + ## but this has no effect on the bug + end +end + +# $Id$ diff --git a/lib/puppet/storage.rb b/lib/puppet/storage.rb index 0c06f5173..59fe4a1fc 100644 --- a/lib/puppet/storage.rb +++ b/lib/puppet/storage.rb @@ -1,10 +1,12 @@ require 'yaml' +require 'sync' +#require 'puppet/lockfile' module Puppet # a class for storing state class Storage include Singleton - + def initialize self.class.load end @@ -46,9 +48,7 @@ module Puppet end return end - #Puppet.debug "Loading statefile %s" % Puppet[:statefile] - Puppet::Util.lock(Puppet[:statefile]) { |file| - #@@state = Marshal.load(file) + Puppet::Util.readlock(Puppet[:statefile]) do |file| begin @@state = YAML.load(file) rescue => detail @@ -63,7 +63,7 @@ module Puppet Puppet[:statefile] end end - } + end unless @@state.is_a?(Hash) Puppet.err "State got corrupted" @@ -78,7 +78,7 @@ module Puppet end def self.store - Puppet.debug "Storing state" + #Puppet.debug "Storing state" unless FileTest.directory?(File.dirname(Puppet[:statefile])) begin Puppet.recmkdir(File.dirname(Puppet[:statefile])) @@ -94,11 +94,9 @@ module Puppet Puppet.info "Creating state file %s" % Puppet[:statefile] end - Puppet::Util.lock( - Puppet[:statefile], "w", 0600 - ) { |file| + Puppet::Util.writelock(Puppet[:statefile], 0600) do |file| file.print YAML.dump(@@state) - } + end end end end diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb index 6e74ca602..d84021a46 100644 --- a/lib/puppet/util.rb +++ b/lib/puppet/util.rb @@ -1,7 +1,12 @@ # A module to collect utility functions. +require 'sync' +require 'puppet/lock' + module Puppet module Util + # Create a sync point for any threads + @@sync = Sync.new # Execute a block as a given user or group def self.asuser(user = nil, group = nil) require 'etc' @@ -132,8 +137,43 @@ module Util end end + # Create a shared lock for reading + def self.readlock(file) + @@sync.synchronize(Sync::SH) do + File.open(file) { |f| + f.lock_shared { |lf| yield lf } + } + end + end + + def self.sync + end + + # Create an exclusive lock fro writing, and do the writing in a + # tmp file. + def self.writelock(file, mode = 0600) + tmpfile = file + ".tmp" + @@sync.synchronize(Sync::EX) do + File.open(file, "w", mode) do |rf| + rf.lock_exclusive do |lrf| + yield lrf + File.open(tmpfile, "w", mode) do |tf| + yield tf + tf.flush + end + begin + File.rename(tmpfile, file) + rescue => detail + Puppet.err "Could not rename %s to %s: %s" % + [file, tmpfile, detail] + end + end + end + end + end + # Create a lock file while something is happening - def self.lock(*opts) + def self.disabledlock(*opts) lock = opts[0] + ".lock" while File.exists?(lock) stamp = File.stat(lock).mtime.to_i @@ -141,9 +181,11 @@ module Util Puppet.notice "Lock file %s is %s seconds old; removing" % [lock, Time.now.to_i - stamp] File.delete(lock) + break + else + sleep 0.1 end #Puppet.debug "%s is locked" % opts[0] - sleep 0.1 end File.open(lock, "w") { |f| f.print " "; f.flush } writing = false @@ -155,8 +197,16 @@ module Util end begin File.open(*opts) { |file| yield file } - if writing - File.rename(tmp, orig) + begin + if writing + Puppet.warning "opts were %s" % opts.inspect + system("ls -l %s 2>/dev/null" % tmp) + system("ls -l %s 2>/dev/null" % orig) + File.rename(tmp, orig) + end + rescue => detail + Puppet.err "Could not replace %s: %s" % [orig, detail] + File.unlink(tmp) end rescue => detail Puppet.err "Storage error: %s" % detail diff --git a/test/puppet/utiltest.rb b/test/puppet/utiltest.rb index 161a831d8..5ca2e4343 100755 --- a/test/puppet/utiltest.rb +++ b/test/puppet/utiltest.rb @@ -10,6 +10,39 @@ require 'test/unit' class TestPuppetUtil < Test::Unit::TestCase include TestPuppet + + # we're getting corrupt files, probably because multiple processes + # are reading or writing the file at once + # so we need to test that + def test_multiwrite + file = tempfile() + File.open(file, "w") { |f| f.puts "starting" } + + value = {:a => :b} + threads = [] + sync = Sync.new + 9.times { |a| + threads << Thread.new { + 9.times { |b| + assert_nothing_raised { + sync.synchronize(Sync::SH) { + Puppet::Util.readlock(file) { |f| + f.read + } + } + sleep 0.01 + sync.synchronize(Sync::EX) { + Puppet::Util.writelock(file) { |f| + f.puts "%s %s" % [a, b] + } + } + } + } + } + } + threads.each { |th| th.join } + end + unless Process.uid == 0 $stderr.puts "Run as root to perform Utility tests" def test_nothing diff --git a/test/types/schedule.rb b/test/types/schedule.rb index 62ab010e3..ded677fd2 100755 --- a/test/types/schedule.rb +++ b/test/types/schedule.rb @@ -145,7 +145,7 @@ class TestSchedule < Test::Unit::TestCase "%s matched %s incorrectly" % [value.inspect, @now]) } - assert_nothing_raised("Could not parse %s" % values) { + assert_nothing_raised("Could not parse %s" % [values]) { s[:range] = values } |