Please review pull request #680: Feature/master/3757 agent lockfiles opened by (cprice-puppet)
Description:
These commits do the following:
- re-introduce agent lockfile changes that were reverted out of 2.7.12
- create separate Puppet settings for each lockfile path, rather than relying on convention
- write out the contents of the administratively disabled lockfile as JSON to provide more structure and allow additions / changes in the future
- clean up tests a bit
- add acceptance test
Docs for the new "API" around the meaning of these files have been committed to telly pre-docs repo.
- Opened: Wed Apr 18 16:37:42 UTC 2012
- Based on: puppetlabs:master (7e22550553515cc90ae94c5e7c847f15c0313119)
- Requested merge: cprice-puppet:feature/master/3757-agent-lockfiles (d1f7abd7c4bc6bb03343d50f75f5bf6a72dcfae3)
Diff follows:
diff --git a/acceptance/tests/agent/agent_disable_lockfile.rb b/acceptance/tests/agent/agent_disable_lockfile.rb
new file mode 100644
index 0000000..e7cf0c3
--- /dev/null
+++ b/acceptance/tests/agent/agent_disable_lockfile.rb
@@ -0,0 +1,208 @@
+test_name "the agent --disable/--enable functionality should manage the agent lockfile properly"
+
+#
+# This test is intended to ensure that puppet agent --enable/--disable
+# work properly, both in terms of complying with our public "API" around
+# lockfile semantics ( http://links.puppetlabs.com/agent_lockfiles ), and
+# in terms of actually restricting or allowing new agent runs to begin.
+#
+
+###############################################################################
+# BEGIN UTILITY METHODS - ideally this stuff would live somewhere besides in
+# the actual test.
+###############################################################################
+
+# Create a file on the host.
+# Parameters:
+# [host] the host to create the file on
+# [file_path] the path to the file to be created
+# [file_content] a string containing the contents to be written to the file
+# [options] a hash containing additional behavior options. Currently supported:
+# * :mkdirs (default false) if true, attempt to create the parent directories on the remote host before writing
+# the file
+# * :owner (default 'root') the username of the user that the file should be owned by
+# * :group (default 'puppet') the name of the group that the file should be owned by
+# * :mode (default '644') the mode (file permissions) that the file should be created with
+def create_test_file(host, file_rel_path, file_content, options)
+
+ # set default options
+ options[:mkdirs] ||= false
+ options[:owner] ||= "root"
+ options[:group] ||= "puppet"
+ options[:mode] ||= "755"
+
+ file_path = get_test_file_path(host, file_rel_path)
+
+ mkdirs(host, File.dirname(file_path)) if (options[:mkdirs] == true)
+ create_remote_file(host, file_path, file_content)
+
+#
+# NOTE: we need these chown/chmod calls because the acceptance framework connects to the nodes as "root", but
+# puppet 'master' runs as user 'puppet'. Therefore, in order for puppet master to be able to read any files
+# that we've created, we have to carefully set their permissions
+#
+
+ chown(host, options[:owner], options[:group], file_path)
+ chmod(host, options[:mode], file_path)
+
+end
+
+
+# Given a relative path, returns an absolute path for a test file. Basically, this just prepends the
+# a unique temp dir path (specific to the current test execution) to your relative path.
+def get_test_file_path(host, file_rel_path)
+ File.join(@host_test_tmp_dirs[host.name], file_rel_path)
+end
+
+
+# Check for the existence of a temp file for the current test; basically, this just calls file_exists?(),
+# but prepends the path to the current test's temp dir onto the file_rel_path parameter. This allows
+# tests to be written using only a relative path to specify file locations, while still taking advantage
+# of automatic temp file cleanup at test completion.
+def test_file_exists?(host, file_rel_path)
+ file_exists?(host, get_test_file_path(host, file_rel_path))
+end
+
+def file_exists?(host, file_path)
+ host.execute("test -f \"#{file_path}\"",
+ :acceptable_exit_codes => [0, 1]) do |result|
+ return result.exit_code == 0
+ end
+end
+
+def file_contents(host, file_path)
+ host.execute("cat \"#{file_path}\"") do |result|
+ return result.stdout
+ end
+end
+
+def tmpdir(host, basename)
+ host_tmpdir = host.tmpdir(basename)
+ # we need to make sure that the puppet user can traverse this directory...
+ chmod(host, "755", host_tmpdir)
+ host_tmpdir
+end
+
+def mkdirs(host, dir_path)
+ on(host, "mkdir -p #{dir_path}")
+end
+
+def chown(host, owner, group, path)
+ on(host, "chown #{owner}:#{group} #{path}")
+end
+
+def chmod(host, mode, path)
+ on(host, "chmod #{mode} #{path}")
+end
+
+
+
+
+# pluck this out of the test case environment; not sure if there is a better way
+cur_test_file = @path
+cur_test_file_shortname = File.basename(cur_test_file, File.extname(cur_test_file))
+
+# we need one list of all of the hosts, to assist in managing temp dirs. It's possible
+# that the master is also an agent, so this will consolidate them into a unique set
+all_hosts = Set[master, *agents]
+
+# now we can create a hash of temp dirs--one per host, and unique to this test--without worrying about
+# doing it twice on any individual host
+@host_test_tmp_dirs = Hash[all_hosts.map do |host| [host.name, tmpdir(host, cur_test_file_shortname)] end ]
+
+# a silly variable for keeping track of whether or not all of the tests passed...
+all_tests_passed = false
+
+###############################################################################
+# END UTILITY METHODS
+###############################################################################
+
+
+
+###############################################################################
+# BEGIN TEST LOGIC
+###############################################################################
+
+
+# this begin block is here for handling temp file cleanup via an "ensure" block at the very end of the
+# test.
+begin
+
+ agent_disabled_lockfile = "/var/lib/puppet/state/agent_disabled.lock"
+
+ tuples = [
+ ["reason not specified", false],
+ ["I'm busy; go away.'", true]
+ ]
+
+ step "start the master" do
+ with_master_running_on(master, "--autosign true") do
+
+ tuples.each do |expected_message, explicitly_specify_message|
+
+ step "disable the agent; specify message? '#{explicitly_specify_message}', message: '#{expected_message}'" do
+ agents.each do |agent|
+ if (explicitly_specify_message)
+ run_agent_on(agent, "--disable \"#{expected_message}\"")
+ else
+ run_agent_on(agent, "--disable")
+ end
+
+ unless file_exists?(agent, agent_disabled_lockfile) then
+ fail_test("Failed to create disabled lock file '#{agent_disabled_lockfile}' on agent '#{agent}'")
+ end
+ lock_file_content = file_contents(agent, agent_disabled_lockfile)
+ # This is a hack; we should parse the JSON into a hash, but I don't think I have a library available
+ # from the acceptance test framework that I can use to do that. So I'm falling back to <gasp> regex.
+ lock_file_content_regex = /"disabled_message"\s*:\s*"#{expected_message}"/
+ unless lock_file_content =~ lock_file_content_regex
+ fail_test("Disabled lock file contents invalid; expected to match '#{lock_file_content_regex}', got '#{lock_file_content}' on agent '#{agent}'")
+ end
+ end
+ end
+
+ step "attempt to run the agent (message: '#{expected_message}')" do
+ agents.each do |agent|
+ run_agent_on(agent, "--no-daemonize --verbose --onetime --test --server #{master}",
+ :acceptable_exit_codes => [1]) do
+ disabled_regex = /administratively disabled.*'#{expected_message}'/
+ unless result.stdout =~ disabled_regex
+ fail_test("Unexpected output from attempt to run agent disabled; expecting to match '#{disabled_regex}', got '#{result.stdout}' on agent '#{agent}'")
+ end
+ end
+ end
+ end
+
+ step "enable the agent (message: '#{expected_message}')" do
+ agents.each do |agent|
+ run_agent_on(agent, "--enable")
+ if file_exists?(agent, agent_disabled_lockfile) then
+ fail_test("Failed to remove disabled lock file '#{agent_disabled_lockfile}' on agent '#{agent}'")
+ end
+ end
+
+ step "verify that we can run the agent (message: '#{expected_message}')" do
+ agents.each do |agent|
+ run_agent_on(agent)
+ end
+ end
+ end
+
+ end
+ end
+ end
+
+ all_tests_passed = true
+
+ensure
+ ##########################################################################################
+ # Clean up all of the temp files created by this test. It would be nice if this logic
+ # could be handled outside of the test itself; I envision a stanza like this one appearing
+ # in a very large number of the tests going forward unless it is handled by the framework.
+ ##########################################################################################
+ if all_tests_passed then
+ all_hosts.each do |host|
+ on(host, "rm -rf #{@host_test_tmp_dirs[host.name]}")
+ end
+ end
+end
\ No newline at end of file
diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index 08a8560..47f4fa6 100644
--- a/lib/puppet/agent.rb
+++ b/lib/puppet/agent.rb
@@ -7,6 +7,9 @@ class Puppet::Agent
require 'puppet/agent/locker'
include Puppet::Agent::Locker
+ require 'puppet/agent/disabler'
+ include Puppet::Agent::Disabler
+
attr_reader :client_class, :client, :splayed
attr_accessor :should_fork
@@ -17,10 +20,6 @@ def initialize(client_class)
@client_class = client_class
end
- def lockfile_path
- client_class.lockfile_path
- end
-
def needing_restart?
Puppet::Application.restart_requested?
end
@@ -32,7 +31,7 @@ def run(*args)
return
end
if disabled?
- Puppet.notice "Skipping run of #{client_class}; administratively disabled; use 'puppet #{client_class} --enable' to re-enable."
+ Puppet.notice "Skipping run of #{client_class}; administratively disabled (Reason: '#{disable_message}');\nUse 'puppet agent --enable' to re-enable."
return
end
diff --git a/lib/puppet/agent/disabler.rb b/lib/puppet/agent/disabler.rb
new file mode 100644
index 0000000..9f9fcb6
--- /dev/null
+++ b/lib/puppet/agent/disabler.rb
@@ -0,0 +1,51 @@
+require 'puppet/util/json_lockfile'
+
+# This module is responsible for encapsulating the logic for
+# "disabling" the puppet agent during a run; in other words,
+# keeping track of enough state to answer the question
+# "has the puppet agent been administratively disabled?"
+#
+# The implementation involves writing a lockfile with JSON
+# contents, and is considered part of the public Puppet API
+# because it used by external tools such as mcollective.
+#
+# For more information, please see docs on the website.
+# http://links.puppetlabs.com/agent_lockfiles
+module Puppet::Agent::Disabler
+ DISABLED_MESSAGE_JSON_KEY = "disabled_message"
+
+ # Let the daemon run again, freely in the filesystem.
+ def enable
+ disable_lockfile.unlock
+ end
+
+ # Stop the daemon from making any catalog runs.
+ def disable(msg=nil)
+ data = ""
+ if (! msg.nil?)
+ data[DISABLED_MESSAGE_JSON_KEY] = msg
+ end
+ disable_lockfile.lock(data)
+ end
+
+ def disabled?
+ disable_lockfile.locked?
+ end
+
+ def disable_message
+ data = ""
+ return nil if data.nil?
+ if data.has_key?(DISABLED_MESSAGE_JSON_KEY)
+ return data[DISABLED_MESSAGE_JSON_KEY]
+ end
+ nil
+ end
+
+
+ def disable_lockfile
+ @disable_lockfile ||= Puppet::Util::JsonLockfile.new(Puppet[:agent_disabled_lockfile])
+
+ @disable_lockfile
+ end
+ private :disable_lockfile
+end
diff --git a/lib/puppet/agent/locker.rb b/lib/puppet/agent/locker.rb
index e165741..53d8c12 100644
--- a/lib/puppet/agent/locker.rb
+++ b/lib/puppet/agent/locker.rb
@@ -1,17 +1,18 @@
require 'puppet/util/pidlock'
-# Break out the code related to locking the agent. This module is just
-# included into the agent, but having it here makes it easier to test.
+# This module is responsible for encapsulating the logic for
+# "locking" the puppet agent during a run; in other words,
+# keeping track of enough state to answer the question
+# "is there a puppet agent currently running?"
+#
+# The implementation involves writing a lockfile whose contents
+# are simply the PID of the running agent process. This is
+# considered part of the public Puppet API because it used
+# by external tools such as mcollective.
+#
+# For more information, please see docs on the website.
+# http://links.puppetlabs.com/agent_lockfiles
module Puppet::Agent::Locker
- # Let the daemon run again, freely in the filesystem.
- def enable
- lockfile.unlock(:anonymous => true)
- end
-
- # Stop the daemon from making any catalog runs.
- def disable
- lockfile.lock(:anonymous => true)
- end
# Yield if we get a lock, else do nothing. Return
# true/false depending on whether we get the lock.
@@ -25,17 +26,16 @@ def lock
end
end
+ def running?
+ lockfile.locked?
+ end
+
def lockfile
- @lockfile ||= Puppet::Util::Pidlock.new(lockfile_path)
+ @lockfile ||= Puppet::Util::Pidlock.new(Puppet[:agent_pidfile])
@lockfile
end
+ private :lockfile
- def running?
- lockfile.locked? and !lockfile.anonymous?
- end
- def disabled?
- lockfile.locked? and lockfile.anonymous?
- end
end
diff --git a/lib/puppet/application/agent.rb b/lib/puppet/application/agent.rb
index 90c608b..40bd55a 100644
--- a/lib/puppet/application/agent.rb
+++ b/lib/puppet/application/agent.rb
@@ -39,7 +39,12 @@ def preinit
end
option("--centrallogging")
- option("--disable")
+
+ option("--disable [MESSAGE]") do |message|
+ options[:disable] = true
+ options[:disable_message] = message
+ end
+
option("--enable")
option("--debug","-d")
option("--fqdn FQDN","-f")
@@ -92,7 +97,7 @@ def help
USAGE
-----
puppet agent [--certname <name>] [-D|--daemonize|--no-daemonize]
- [-d|--debug] [--detailed-exitcodes] [--digest <digest>] [--disable] [--enable]
+ [-d|--debug] [--detailed-exitcodes] [--digest <digest>] [--disable [message]] [--enable]
[--fingerprint] [-h|--help] [-l|--logdest syslog|<file>|console]
[--no-client] [--noop] [-o|--onetime] [-t|--test]
[-v|--verbose] [-V|--version] [-w|--waitforcert <seconds>]
@@ -196,6 +201,9 @@ def help
not want the central configuration to override the local state until
everything is tested and committed.
+ Disable can also take an optional message that will be reported by the
+ 'puppet agent' at the next disabled run.
+
'puppet agent' uses the same lock file while it is running, so no more
than one 'puppet agent' process is working at a time.
@@ -354,7 +362,7 @@ def enable_disable_client(agent)
if options[:enable]
agent.enable
elsif options[:disable]
- agent.disable
+ agent.disable(options[:disable_message] || 'reason not specified')
end
exit(0)
end
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index b34e9dd..3612f87 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -30,11 +30,6 @@ class << self
include Puppet::Util
end
- # How to lock instances of this class.
- def self.lockfile_path
- Puppet[:puppetdlockfile]
- end
-
def execute_postrun_command
execute_from_setting(:postrun_command)
end
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 4598f65..5b6f9ce 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -1019,10 +1019,15 @@ module Puppet
can be guaranteed to support this format, but it will be used for all
classes that support it.",
},
- :puppetdlockfile => {
- :default => "$statedir/puppetdlock",
+ :agent_pidfile => {
+ :default => "$statedir/agent.pid",
:type => :file,
- :desc => "A lock file to temporarily stop puppet agent from doing anything.",
+ :desc => "A lock file to indicate that a puppet agent run is currently in progress. File contains the pid of the running process.",
+ },
+ :agent_disabled_lockfile => {
+ :default => "$statedir/agent_disabled.lock",
+ :type => :file,
+ :desc => "A lock file to indicate that puppet agent runs have been administratively disabled. File contains a JSON object with state information.",
},
:usecacheonfailure => {
:default => true,
diff --git a/lib/puppet/util/json_lockfile.rb b/lib/puppet/util/json_lockfile.rb
new file mode 100644
index 0000000..523fa40
--- /dev/null
+++ b/lib/puppet/util/json_lockfile.rb
@@ -0,0 +1,41 @@
+require 'puppet/util/lockfile'
+
+# This class provides a simple API for managing a lock file
+# whose contents are a serialized JSON object. In addition
+# to querying the basic state (#locked?) of the lock, managing
+# the lock (#lock, #unlock), the contents can be retrieved at
+# any time while the lock is held (#lock_data). This can be
+# used to store structured data (state messages, etc.) about
+# the lock.
+#
+# @see Puppet::Util::Lockfile
+class Puppet::Util::JsonLockfile < Puppet::Util::Lockfile
+ # Lock the lockfile. You may optionally pass a data object, which will be
+ # retrievable for the duration of time during which the file is locked.
+ #
+ # @param [Hash] lock_data an optional Hash of data to associate with the lock.
+ # This may be used to store pids, descriptive messages, etc. The
+ # data may be retrieved at any time while the lock is held by
+ # calling the #lock_data method. <b>NOTE</b> that the JSON serialization
+ # does NOT support Symbol objects--if you pass them in, they will be
+ # serialized as Strings, so you should plan accordingly.
+ # @return [boolean] true if lock is successfully acquired, false otherwise.
+ def lock(lock_data = nil)
+ return false if locked?
+
+ super(lock_data.to_pson)
+ end
+
+ # Retrieve the (optional) lock data that was specified at the time the file
+ # was locked.
+ # @return [Object] the data object. Remember that the serialization does not
+ # support Symbol objects, so if your data Object originally contained symbols,
+ # they will be converted to Strings.
+ def lock_data
+ return nil unless file_locked?
+ file_contents = super
+ return nil if file_contents.nil?
+ PSON.parse(file_contents)
+ end
+
+end
\ No newline at end of file
diff --git a/lib/puppet/util/lockfile.rb b/lib/puppet/util/lockfile.rb
new file mode 100644
index 0000000..5c910db
--- /dev/null
+++ b/lib/puppet/util/lockfile.rb
@@ -0,0 +1,62 @@
+# This class provides a simple API for managing a lock file
+# whose contents are an (optional) String. In addition
+# to querying the basic state (#locked?) of the lock, managing
+# the lock (#lock, #unlock), the contents can be retrieved at
+# any time while the lock is held (#lock_data). This can be
+# used to store pids, messages, etc.
+#
+# @see Puppet::Util::JsonLockfile
+class Puppet::Util::Lockfile
+ attr_reader :file_path
+
+ def initialize(file_path)
+ @file_path = file_path
+ end
+
+ # Lock the lockfile. You may optionally pass a data object, which will be
+ # retrievable for the duration of time during which the file is locked.
+ #
+ # @param [String] lock_data an optional String data object to associate
+ # with the lock. This may be used to store pids, descriptive messages,
+ # etc. The data may be retrieved at any time while the lock is held by
+ # calling the #lock_data method.
+
+ # @return [boolean] true if lock is successfully acquired, false otherwise.
+ def lock(lock_data = nil)
+ return false if locked?
+
+ File.open(@file_path, 'w') { |fd| fd.print(lock_data) }
+ true
+ end
+
+ def unlock
+ if locked?
+ File.unlink(@file_path)
+ true
+ else
+ false
+ end
+ end
+
+ def locked?
+ # delegate logic to a more explicit private method
+ file_locked?
+ end
+
+ # Retrieve the (optional) lock data that was specified at the time the file
+ # was locked.
+ # @return [String] the data object.
+ def lock_data
+ return File.read(@file_path) if file_locked?
+ end
+
+ # Private, internal utility method for encapsulating the logic about
+ # whether or not the file is locked. This method can be called
+ # by other methods in this class without as much risk of accidentally
+ # being overridden by child classes.
+ # @return [boolean] true if the file is locked, false if it is not.
+ def file_locked?()
+ File.exists? @file_path
+ end
+ private :file_locked?
+end
\ No newline at end of file
diff --git a/lib/puppet/util/pidlock.rb b/lib/puppet/util/pidlock.rb
index a0fe99e..33c6bf1 100644
--- a/lib/puppet/util/pidlock.rb
+++ b/lib/puppet/util/pidlock.rb
@@ -1,69 +1,40 @@
require 'fileutils'
+require 'puppet/util/lockfile'
class Puppet::Util::Pidlock
- attr_reader :lockfile
def initialize(lockfile)
- @lockfile = lockfile
+ @lockfile = Puppet::Util::Lockfile.new(lockfile)
end
def locked?
clear_if_stale
- return true if File.exists? @lockfile
-
- # HACK! There was a temporary change to the lockfile behavior introduced in 2.7.10 and 2.7.11, and reverted
- # in 2.7.12. We need to pull some chicanery to be backwards-compatible with those versions. For more info,
- # see the comments on the method... and this hack should be removed for the 3.x series.
- handle_2_7_10_disabled_lockfile
- File.exists? @lockfile
+ @lockfile.locked?
end
def mine?
Process.pid == lock_pid
end
- def anonymous?
- return false unless File.exists?(@lockfile)
- File.read(@lockfile) == ""
- end
-
- def lock(opts = {})
- opts = {:anonymous => false}.merge(opts)
+ def lock
+ return mine? if locked?
- if locked?
- mine?
- else
- if opts[:anonymous]
- File.open(@lockfile, 'w') { |fd| true }
- else
- File.open(@lockfile, "w") { |fd| fd.write(Process.pid) }
- end
- true
- end
+ @lockfile.lock(Process.pid)
end
- def unlock(opts = {})
- return false unless locked?
-
- opts = {:anonymous => false}.merge(opts)
-
- if mine? or (opts[:anonymous] and anonymous?)
- File.unlink(@lockfile)
- true
+ def unlock()
+ if mine?
+ return @lockfile.unlock
else
false
end
end
- private
def lock_pid
- if File.exists? @lockfile
- File.read(@lockfile).to_i
- else
- nil
- end
+ @lockfile.lock_data.to_i
end
+
def clear_if_stale
return if lock_pid.nil?
@@ -74,44 +45,9 @@ def clear_if_stale
begin
Process.kill(0, lock_pid)
rescue *errors
- File.unlink(@lockfile)
+ @lockfile.unlock
end
end
+ private :clear_if_stale
-
- ######################################################################################
- # Backwards compatibility hack
- ######################################################################################
- # A change to lockfile behavior was introduced in 2.7.10 and 2.7.11; basically,
- # instead of using a single lockfile to indicate both administrative disabling of
- # the agent *and* the case where an agent run is already in progress, we started using
- # two separate lockfiles: the 'normal' one for the "run in progress" case, and a
- # separate one with a ".disabled" extension to indicate administrative disabling.
- #
- # This was determined to cause incompatibilities with mcollective, so the behavior
- # was reverted for 2.7.12. Unfortunately this leaves the possibility that someone
- # may have run "agent --disable" to administratively disable a 2.7.10 or 2.7.11
- # agent, and then upgraded to a newer version. This method exists only to
- # provide backwards compatibility. Basically, it just recognizes the 2.7.10/2.7.11
- # ".disabled" lock file, warns, and cleans it up.
- #
- # This should be removed for the 3.x series.
- #
- # For more information, please see tickets #12844, #3757, #4836, and #11057
- #
- # -- cprice 2012-03-01
- #
- def handle_2_7_10_disabled_lockfile
- disabled_lockfile_path = @lockfile + ".disabled"
- if (File.exists?(disabled_lockfile_path))
- Puppet.warning("Found special lockfile '#{disabled_lockfile_path}'; this file was " +
- "generated by a call to 'puppet agent --disable' in puppet 2.7.10 or 2.7.11. " +
- "The expected lockfile path is '#{@lockfile}'; renaming the lock file.")
- File.rename(disabled_lockfile_path, @lockfile)
- end
- end
- private :handle_2_7_10_disabled_lockfile
- ######################################################################################
- # End backwards compatibility hack
- ######################################################################################
end
diff --git a/spec/unit/agent/disabler_spec.rb b/spec/unit/agent/disabler_spec.rb
new file mode 100644
index 0000000..c93be4d
--- /dev/null
+++ b/spec/unit/agent/disabler_spec.rb
@@ -0,0 +1,68 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/agent'
+require 'puppet/agent/locker'
+
+class DisablerTester
+ include Puppet::Agent::Disabler
+end
+
+describe Puppet::Agent::Disabler do
+ before do
+ @disabler = DisablerTester.new
+ end
+
+
+ ## These tests are currently very implementation-specific, and they rely heavily on
+ ## having access to the "disable_lockfile" method. However, I've made this method private
+ ## because it really shouldn't be exposed outside of our implementation... therefore
+ ## these tests have to use a lot of ".send" calls. They should probably be cleaned up
+ ## but for the moment I wanted to make sure not to lose any of the functionality of
+ ## the tests. --cprice 2012-04-16
+
+ it "should use an JsonLockfile instance as its disable_lockfile" do
+ @disabler.send(:disable_lockfile).should be_instance_of(Puppet::Util::JsonLockfile)
+ end
+
+
+ it "should use puppet's :agent_disabled_lockfile' setting to determine its lockfile path" do
+ Puppet.expects(:[]).with(:agent_disabled_lockfile).returns("/my/lock.disabled")
+ lock = Puppet::Util::JsonLockfile.new("/my/lock.disabled")
+ Puppet::Util::JsonLockfile.expects(:new).with("/my/lock.disabled").returns lock
+
+ @disabler.send(:disable_lockfile)
+ end
+
+ it "should reuse the same lock file each time" do
+ @disabler.send(:disable_lockfile).should equal(@disabler.send(:disable_lockfile))
+ end
+
+ it "should lock the file when disabled" do
+ @disabler.send(:disable_lockfile).expects(:lock)
+
+ @disabler.disable
+ end
+
+ it "should unlock the file when enabled" do
+ @disabler.send(:disable_lockfile).expects(:unlock)
+
+ @disabler.enable
+ end
+
+ it "should check the lock if it is disabled" do
+ @disabler.send(:disable_lockfile).expects(:locked?)
+
+ @disabler.disabled?
+ end
+
+ it "should report the disable message when disabled" do
+ lockfile = PuppetSpec::Files.tmpfile("lock")
+ lock = Puppet::Util::JsonLockfile.new(lockfile)
+ Puppet.expects(:[]).with(:agent_disabled_lockfile).returns("/my/lock.disabled")
+ Puppet::Util::JsonLockfile.expects(:new).with("/my/lock.disabled").returns lock
+
+ msg = "I'm busy, go away"
+ @disabler.disable(msg)
+ @disabler.disable_message.should == msg
+ end
+end
diff --git a/spec/unit/agent/locker_spec.rb b/spec/unit/agent/locker_spec.rb
index cfbdcc8..9d3188c 100755
--- a/spec/unit/agent/locker_spec.rb
+++ b/spec/unit/agent/locker_spec.rb
@@ -8,41 +8,35 @@ class LockerTester
end
describe Puppet::Agent::Locker do
- before do
+ before do
@locker = LockerTester.new
- @locker.stubs(:lockfile_path).returns "/my/lock"
end
+ ## These tests are currently very implementation-specific, and they rely heavily on
+ ## having access to the lockfile object. However, I've made this method private
+ ## because it really shouldn't be exposed outside of our implementation... therefore
+ ## these tests have to use a lot of ".send" calls. They should probably be cleaned up
+ ## but for the moment I wanted to make sure not to lose any of the functionality of
+ ## the tests. --cprice 2012-04-16
+
it "should use a Pidlock instance as its lockfile" do
- @locker.lockfile.should be_instance_of(Puppet::Util::Pidlock)
+ @locker.send(:lockfile).should be_instance_of(Puppet::Util::Pidlock)
end
- it "should use 'lockfile_path' to determine its lockfile path" do
- @locker.expects(:lockfile_path).returns "/my/lock"
+ it "should use puppet's :agent_pidfile' setting to determine its lockfile path" do
+ Puppet.expects(:[]).with(:agent_pidfile).returns("/my/lock")
lock = Puppet::Util::Pidlock.new("/my/lock")
Puppet::Util::Pidlock.expects(:new).with("/my/lock").returns lock
- @locker.lockfile
+ @locker.send(:lockfile)
end
it "should reuse the same lock file each time" do
- @locker.lockfile.should equal(@locker.lockfile)
- end
-
- it "should use the lock file to anonymously lock the process when disabled" do
- @locker.lockfile.expects(:lock).with(:anonymous => true)
-
- @locker.disable
- end
-
- it "should use the lock file to anonymously unlock the process when enabled" do
- @locker.lockfile.expects(:unlock).with(:anonymous => true)
-
- @locker.enable
+ @locker.send(:lockfile).should equal(@locker.send(:lockfile))
end
it "should have a method that yields when a lock is attained" do
- @locker.lockfile.expects(:lock).returns true
+ @locker.send(:lockfile).expects(:lock).returns true
yielded = false
@locker.lock do
@@ -52,19 +46,19 @@ class LockerTester
end
it "should return the block result when the lock method successfully locked" do
- @locker.lockfile.expects(:lock).returns true
+ @locker.send(:lockfile).expects(:lock).returns true
@locker.lock { :result }.should == :result
end
it "should return nil when the lock method does not receive the lock" do
- @locker.lockfile.expects(:lock).returns false
+ @locker.send(:lockfile).expects(:lock).returns false
@locker.lock {}.should be_nil
end
it "should not yield when the lock method does not receive the lock" do
- @locker.lockfile.expects(:lock).returns false
+ @locker.send(:lockfile).expects(:lock).returns false
yielded = false
@locker.lock { yielded = true }
@@ -72,28 +66,28 @@ class LockerTester
end
it "should not unlock when a lock was not received" do
- @locker.lockfile.expects(:lock).returns false
- @locker.lockfile.expects(:unlock).never
+ @locker.send(:lockfile).expects(:lock).returns false
+ @locker.send(:lockfile).expects(:unlock).never
@locker.lock {}
end
it "should unlock after yielding upon obtaining a lock" do
- @locker.lockfile.stubs(:lock).returns true
- @locker.lockfile.expects(:unlock)
+ @locker.send(:lockfile).stubs(:lock).returns true
+ @locker.send(:lockfile).expects(:unlock)
@locker.lock {}
end
it "should unlock after yielding upon obtaining a lock, even if the block throws an exception" do
- @locker.lockfile.stubs(:lock).returns true
- @locker.lockfile.expects(:unlock)
+ @locker.send(:lockfile).stubs(:lock).returns true
+ @locker.send(:lockfile).expects(:unlock)
lambda { @locker.lock { raise "foo" } }.should raise_error(RuntimeError)
end
it "should be considered running if the lockfile is locked" do
- @locker.lockfile.expects(:locked?).returns true
+ @locker.send(:lockfile).expects(:locked?).returns true
@locker.should be_running
end
end
diff --git a/spec/unit/agent_backward_compatibility_spec.rb b/spec/unit/agent_backward_compatibility_spec.rb
deleted file mode 100755
index 9301e26..0000000
--- a/spec/unit/agent_backward_compatibility_spec.rb
+++ /dev/null
@@ -1,152 +0,0 @@
-#!/usr/bin/env rspec
-require 'spec_helper'
-require 'puppet/agent'
-
-
-############################################################################
-# NOTE #
-############################################################################
-# #
-# This entire spec is only here for backwards compatibility from 2.7.12+ #
-# with 2.7.10 and 2.7.11. The entire file should be able to be removed #
-# for the 3.x series. #
-# #
-# For more info, see the comments on the #handle_2_7_10_disabled_lockfile #
-# method in pidlock.rb #
-# #
-# --cprice 2012-03-01 #
-############################################################################
-
-class AgentTestClient
- def run
- # no-op
- end
- def stop
- # no-op
- end
-end
-
-describe Puppet::Agent do
- include PuppetSpec::Files
-
- let(:agent) { Puppet::Agent.new(AgentTestClient) }
-
- describe "in order to be backwards-compatibility with versions 2.7.10 and 2.7.11" do
-
- describe "when the 2.7.10/2.7.11 'disabled' lockfile exists" do
-
- # the "normal" lockfile
- let(:lockfile_path) { tmpfile("agent_spec_lockfile") }
-
- # the 2.7.10/2.7.11 "disabled" lockfile
- # (can't use PuppetSpec::Files.tmpfile here because we need the ".disabled" file to have *exactly* the same
- # path/name as the original file, plus the ".disabled" suffix.)
- let(:disabled_lockfile_path) { lockfile_path + ".disabled" }
-
- # some regexes to match log messages
- let(:warning_regex) { /^Found special lockfile '#{Regexp.escape(disabled_lockfile_path)}'.*renaming/ }
- let(:disabled_regex) { /^Skipping run of .*; administratively disabled/ }
-
- before(:each) do
- # create the 2.7.10 "disable" lockfile.
- FileUtils.touch(disabled_lockfile_path)
-
- # stub in our temp lockfile path.
- AgentTestClient.expects(:lockfile_path).returns lockfile_path
- end
-
- after(:each) do
- # manually clean up the files that we didn't create via PuppetSpec::Files.tmpfile
- begin
- File.unlink(disabled_lockfile_path)
- rescue Errno::ENOENT
- # some of the tests expect for the agent code to take care of deleting this file,
- # so it may (validly) not exist.
- end
- end
-
- describe "when the 'regular' lockfile also exists" do
- # the logic here is that if a 'regular' lockfile already exists, then there is some state that the
- # current version of puppet is responsible for dealing with. All of the tests in this block are
- # simply here to make sure that our backwards-compatibility hack does *not* interfere with this.
- #
- # Even if the ".disabled" lockfile exists--it can be dealt with at another time, when puppet is
- # in *exactly* the state that we want it to be in (mostly meaning that the 'regular' lockfile
- # does not exist.)
-
- before(:each) do
- # create the "regular" lockfile
- FileUtils.touch(lockfile_path)
- end
-
- it "should be recognized as 'disabled'" do
- agent.should be_disabled
- end
-
- it "should not try to start a new agent run" do
- AgentTestClient.expects(:new).never
- Puppet.expects(:notice).with(regexp_matches(disabled_regex))
-
- agent.run
- end
-
- it "should not delete the 2.7.10/2.7.11 lockfile" do
- agent.run
-
- File.exists?(disabled_lockfile_path).should == true
- end
-
- it "should not print the warning message" do
- Puppet.expects(:warning).with(regexp_matches(warning_regex)).never
-
- agent.run
- end
- end
-
- describe "when the 'regular' lockfile does not exist" do
- # this block of tests is for actually testing the backwards compatibility hack. This
- # is where we're in a clean state and we know it's safe(r) to muck with the lockfile
- # situation.
-
- it "should recognize that the agent is disabled" do
- agent.should be_disabled
- end
-
- describe "when an agent run is requested" do
- it "should not try to start a new agent run" do
- AgentTestClient.expects(:new).never
- Puppet.expects(:notice).with(regexp_matches(disabled_regex))
-
- agent.run
- end
-
- it "should warn, remove the 2.7.10/2.7.11 lockfile, and create the 'normal' lockfile" do
- Puppet.expects(:warning).with(regexp_matches(warning_regex))
-
- agent.run
-
- File.exists?(disabled_lockfile_path).should == false
- File.exists?(lockfile_path).should == true
- end
- end
-
- describe "when running --enable" do
- it "should recognize that the agent is disabled" do
- agent.should be_disabled
- end
-
- it "should warn and clean up the 2.7.10/2.7.11 lockfile" do
- Puppet.expects(:warning).with(regexp_matches(warning_regex))
-
- agent.enable
-
- File.exists?(disabled_lockfile_path).should == false
- File.exists?(lockfile_path).should == false
- end
- end
- end
- end
- end
-
-
-end
diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb
index 13ea756..0b3e76d 100755
--- a/spec/unit/agent_spec.rb
+++ b/spec/unit/agent_spec.rb
@@ -61,33 +61,18 @@ def controlled_run(&block)
@agent.run
end
- it "should determine its lock file path by asking the client class" do
- AgentTestClient.expects(:lockfile_path).returns "/my/lock"
- @agent.lockfile_path.should == "/my/lock"
- end
-
- it "should be considered running if the lock file is locked and not anonymous" do
+ it "should be considered running if the lock file is locked" do
lockfile = mock 'lockfile'
- @agent.expects(:lockfile).returns(lockfile).twice
+ @agent.expects(:lockfile).returns(lockfile)
lockfile.expects(:locked?).returns true
- lockfile.expects(:anonymous?).returns false
@agent.should be_running
end
- it "should be considered disabled if the lock file is locked and anonymous" do
- lockfile = mock 'lockfile'
-
- @agent.expects(:lockfile).returns(lockfile).at_least_once
- lockfile.expects(:locked?).returns(true).at_least_once
- lockfile.expects(:anonymous?).returns(true).at_least_once
-
- @agent.should be_disabled
- end
-
describe "when being run" do
before do
+ AgentTestClient.stubs(:lockfile_path).returns "/my/lock"
@agent.stubs(:running?).returns false
@agent.stubs(:disabled?).returns false
end
@@ -104,6 +89,12 @@ def controlled_run(&block)
@agent.run
end
+ it "should do nothing if disabled" do
+ @agent.expects(:disabled?).returns(true)
+ AgentTestClient.expects(:new).never
+ @agent.run
+ end
+
it "(#11057) should notify the user about why a run is skipped" do
Puppet::Application.stubs(:controlled_run).returns(false)
Puppet::Application.stubs(:run_status).returns('MOCK_RUN_STATUS')
@@ -116,7 +107,8 @@ def controlled_run(&block)
it "should display an informative message if the agent is administratively disabled" do
@agent.expects(:disabled?).returns true
- Puppet.expects(:notice).with(regexp_matches(/Skipping run of .*; administratively disabled/))
+ @agent.expects(:disable_message).returns "foo"
+ Puppet.expects(:notice).with(regexp_matches(/Skipping run of .*; administratively disabled.*\(Reason: 'foo'\)/))
@agent.run
end
diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb
index d7e2e7f..2f3a9a0 100755
--- a/spec/unit/application/agent_spec.rb
+++ b/spec/unit/application/agent_spec.rb
@@ -92,7 +92,7 @@
@puppetd.command_line.stubs(:args).returns([])
end
- [:centrallogging, :disable, :enable, :debug, :fqdn, :test, :verbose, :digest].each do |option|
+ [:centrallogging, :enable, :debug, :fqdn, :test, :verbose, :digest].each do |option|
it "should declare handle_#{option} method" do
@puppetd.should respond_to("handle_#{option}".to_sym)
end
@@ -103,6 +103,24 @@
end
end
+ describe "when handling --disable" do
+ it "should declare handle_disable method" do
+ @puppetd.should respond_to(:handle_disable)
+ end
+
+ it "should set disable to true" do
+ @puppetd.options.stubs(:[]=)
+ @puppetd.options.expects(:[]=).with(:disable, true)
+ @puppetd.handle_disable('')
+ end
+
+ it "should store disable message" do
+ @puppetd.options.stubs(:[]=)
+ @puppetd.options.expects(:[]=).with(:disable_message, "message")
+ @puppetd.handle_disable('message')
+ end
+ end
+
it "should set client to false with --no-client" do
@puppetd.handle_no_client(nil)
@puppetd.options[:client].should be_false
@@ -349,6 +367,20 @@
end
end
+ it "should pass the disable message when disabling" do
+ @puppetd.options.stubs(:[]).with(:disable).returns(true)
+ @puppetd.options.stubs(:[]).with(:disable_message).returns("message")
+ @agent.expects(:disable).with("message")
+ expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
+ end
+
+ it "should pass the default disable message when disabling without a message" do
+ @puppetd.options.stubs(:[]).with(:disable).returns(true)
+ @puppetd.options.stubs(:[]).with(:disable_message).returns(nil)
+ @agent.expects(:disable).with("reason not specified")
+ expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
+ end
+
it "should finally exit" do
expect { @puppetd.enable_disable_client(@agent) }.to exit_with 0
end
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
index 66c7f81..da3cefb 100755
--- a/spec/unit/configurer_spec.rb
+++ b/spec/unit/configurer_spec.rb
@@ -20,11 +20,6 @@
Puppet::Configurer.ancestors.should be_include(Puppet::Configurer::FactHandler)
end
- it "should use the puppetdlockfile as its lockfile path" do
- Puppet.settings.expects(:value).with(:puppetdlockfile).returns("/my/lock")
- Puppet::Configurer.lockfile_path.should == "/my/lock"
- end
-
describe "when executing a pre-run hook" do
it "should do nothing if the hook is set to an empty string" do
Puppet.settings[:prerun_command] = ""
diff --git a/spec/unit/util/json_lockfile_spec.rb b/spec/unit/util/json_lockfile_spec.rb
new file mode 100644
index 0000000..a66e386
--- /dev/null
+++ b/spec/unit/util/json_lockfile_spec.rb
@@ -0,0 +1,29 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+require 'puppet/util/json_lockfile'
+
+describe Puppet::Util::JsonLockfile do
+ require 'puppet_spec/files'
+ include PuppetSpec::Files
+
+ before(:each) do
+ @lockfile = tmpfile("lock")
+ @lock = Puppet::Util::JsonLockfile.new(@lockfile)
+ end
+
+ describe "#lock" do
+ it "should create a lock file containing a json hash" do
+ data = { "foo" => "foofoo", "bar" => "barbar" }
+ @lock.lock(data)
+
+ PSON.parse(File.read(@lockfile)).should == data
+ end
+ end
+
+ it "should return the lock data" do
+ data = { "foo" => "foofoo", "bar" => "barbar" }
+ @lock.lock(data)
+ @lock.lock_data.should == data
+ end
+end
\ No newline at end of file
diff --git a/spec/unit/util/lockfile_spec.rb b/spec/unit/util/lockfile_spec.rb
new file mode 100644
index 0000000..5c3261f
--- /dev/null
+++ b/spec/unit/util/lockfile_spec.rb
@@ -0,0 +1,76 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+require 'puppet/util/lockfile'
+
+describe Puppet::Util::Lockfile do
+ require 'puppet_spec/files'
+ include PuppetSpec::Files
+
+ before(:each) do
+ @lockfile = tmpfile("lock")
+ @lock = Puppet::Util::Lockfile.new(@lockfile)
+ end
+
+ describe "#lock" do
+ it "should return false if already locked" do
+ @lock.stubs(:locked?).returns(true)
+ @lock.lock.should be_false
+ end
+
+ it "should return true if it successfully locked" do
+ @lock.lock.should be_true
+ end
+
+ it "should create a lock file" do
+ @lock.lock
+
+ File.should be_exists(@lockfile)
+ end
+
+ it "should create a lock file containing a string" do
+ data = "" barbar"
+ @lock.lock(data)
+
+ File.read(@lockfile).should == data
+ end
+ end
+
+ describe "#unlock" do
+ it "should return true when unlocking" do
+ @lock.lock
+ @lock.unlock.should be_true
+ end
+
+ it "should return false when not locked" do
+ @lock.unlock.should be_false
+ end
+
+ it "should clear the lock file" do
+ File.open(@lockfile, 'w') { |fd| fd.print("locked") }
+ @lock.unlock
+ File.should_not be_exists(@lockfile)
+ end
+ end
+
+ it "should be locked when locked" do
+ @lock.lock
+ @lock.should be_locked
+ end
+
+ it "should not be locked when not locked" do
+ @lock.should_not be_locked
+ end
+
+ it "should not be locked when unlocked" do
+ @lock.lock
+ @lock.unlock
+ @lock.should_not be_locked
+ end
+
+ it "should return the lock data" do
+ data = "" barbar"
+ @lock.lock(data)
+ @lock.lock_data.should == data
+ end
+end
\ No newline at end of file
diff --git a/spec/unit/util/pidlock_spec.rb b/spec/unit/util/pidlock_spec.rb
new file mode 100644
index 0000000..20f8649
--- /dev/null
+++ b/spec/unit/util/pidlock_spec.rb
@@ -0,0 +1,179 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+
+require 'puppet/util/pidlock'
+
+describe Puppet::Util::Pidlock do
+ require 'puppet_spec/files'
+ include PuppetSpec::Files
+
+ before(:each) do
+ @lockfile = tmpfile("lock")
+ @lock = Puppet::Util::Pidlock.new(@lockfile)
+ end
+
+ describe "#lock" do
+ it "should not be locked at start" do
+ @lock.should_not be_locked
+ end
+
+ it "should not be mine at start" do
+ @lock.should_not be_mine
+ end
+
+ it "should become locked" do
+ @lock.lock
+ @lock.should be_locked
+ end
+
+ it "should become mine" do
+ @lock.lock
+ @lock.should be_mine
+ end
+
+ it "should be possible to lock multiple times" do
+ @lock.lock
+ lambda { @lock.lock }.should_not raise_error
+ end
+
+ it "should return true when locking" do
+ @lock.lock.should be_true
+ end
+
+ it "should return true if locked by me" do
+ @lock.lock
+ @lock.lock.should be_true
+ end
+
+
+ it "should create a lock file" do
+ @lock.lock
+ File.should be_exists(@lockfile)
+ end
+ end
+
+ describe "#unlock" do
+ it "should not be locked anymore" do
+ @lock.lock
+ @lock.unlock
+ @lock.should_not be_locked
+ end
+
+ it "should return false if not locked" do
+ @lock.unlock.should be_false
+ end
+
+ it "should return true if properly unlocked" do
+ @lock.lock
+ @lock.unlock.should be_true
+ end
+
+ it "should get rid of the lock file" do
+ @lock.lock
+ @lock.unlock
+ File.should_not be_exists(@lockfile)
+ end
+ end
+
+ describe "#locked?" do
+ it "should return true if locked" do
+ @lock.lock
+ @lock.should be_locked
+ end
+ end
+
+ describe "with a stale lock" do
+ before(:each) do
+ # fake our pid to be 1234
+ Process.stubs(:pid).returns(1234)
+ # lock the file
+ @lock.lock
+ # fake our pid to be a different pid, to simulate someone else
+ # holding the lock
+ Process.stubs(:pid).returns(6789)
+
+ Process.stubs(:kill).with(0, 6789)
+ Process.stubs(:kill).with(0, 1234).raises(Errno::ESRCH)
+ end
+
+ it "should not be locked" do
+ @lock.should_not be_locked
+ end
+
+ describe "#lock" do
+ it "should clear stale locks" do
+ @lock.locked?
+ File.should_not be_exists(@lockfile)
+ end
+
+ it "should replace with new locks" do
+ @lock.lock
+ File.should be_exists(@lockfile)
+ @lock.lock_pid.should == 6789
+ @lock.should be_mine
+ @lock.should be_locked
+ end
+ end
+
+ describe "#unlock" do
+ it "should not be allowed" do
+ @lock.unlock.should be_false
+ end
+
+ it "should not remove the lock file" do
+ @lock.unlock
+ File.should be_exists(@lockfile)
+ end
+ end
+ end
+
+ describe "with another process lock" do
+ before(:each) do
+ # fake our pid to be 1234
+ Process.stubs(:pid).returns(1234)
+ # lock the file
+ @lock.lock
+ # fake our pid to be a different pid, to simulate someone else
+ # holding the lock
+ Process.stubs(:pid).returns(6789)
+
+ Process.stubs(:kill).with(0, 6789)
+ Process.stubs(:kill).with(0, 1234)
+ end
+
+ it "should be locked" do
+ @lock.should be_locked
+ end
+
+ it "should not be mine" do
+ @lock.should_not be_mine
+ end
+
+ describe "#lock" do
+ it "should not be possible" do
+ @lock.lock.should be_false
+ end
+
+ it "should not overwrite the lock" do
+ @lock.lock
+ @lock.should_not be_mine
+ end
+ end
+
+ describe "#unlock" do
+ it "should not be possible" do
+ @lock.unlock.should be_false
+ end
+
+ it "should not remove the lock file" do
+ @lock.unlock
+ File.should be_exists(@lockfile)
+ end
+
+ it "should still not be our lock" do
+ @lock.unlock
+ @lock.should_not be_mine
+ end
+ end
+ end
+end
\ No newline at end of file
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
