Please review pull request #747: Fix windows acceptance failures (mostly related to tmpfile paths) opened by (cprice-puppet)
Description:
- Opened: Wed May 09 00:08:16 UTC 2012
- Based on: puppetlabs:master (35f4113f7e428e81061ac139cc7b81c1c171d2f3)
- Requested merge: cprice-puppet:cleanup/master/windows-tmpfile-acceptance-failures (9cd3536429d622cf9fbc93345e216419b0d1f598)
Diff follows:
diff --git a/acceptance/tests/agent/agent_disable_lockfile.rb b/acceptance/tests/agent/agent_disable_lockfile.rb
index e7cf0c3..6881f78 100644
--- a/acceptance/tests/agent/agent_disable_lockfile.rb
+++ b/acceptance/tests/agent/agent_disable_lockfile.rb
@@ -128,8 +128,6 @@ def chmod(host, mode, path)
# test.
begin
- agent_disabled_lockfile = "/var/lib/puppet/state/agent_disabled.lock"
-
tuples = [
["reason not specified", false],
["I'm busy; go away.'", true]
@@ -148,6 +146,7 @@ def chmod(host, mode, path)
run_agent_on(agent, "--disable")
end
+ agent_disabled_lockfile = "#{agent['puppetvardir']}/state/agent_disabled.lock"
unless file_exists?(agent, agent_disabled_lockfile) then
fail_test("Failed to create disabled lock file '#{agent_disabled_lockfile}' on agent '#{agent}'")
end
@@ -175,6 +174,8 @@ def chmod(host, mode, path)
step "enable the agent (message: '#{expected_message}')" do
agents.each do |agent|
+
+ agent_disabled_lockfile = "#{agent['puppetvardir']}/state/agent_disabled.lock"
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}'")
diff --git a/acceptance/tests/pluginsync/apply_should_sync_plugins.rb b/acceptance/tests/pluginsync/apply_should_sync_plugins.rb
index 943238f..8fa8d24 100644
--- a/acceptance/tests/pluginsync/apply_should_sync_plugins.rb
+++ b/acceptance/tests/pluginsync/apply_should_sync_plugins.rb
@@ -1,22 +1,147 @@
test_name "puppet apply should pluginsync"
+###############################################################################
+# 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)
+ if host['platform'].include?('windows')
+ return;
+ end
+ 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
+###############################################################################
+
+
+
step "Create some modules in the modulepath"
-basedir = '/tmp/acceptance_pluginsync_modules'
-on agents, "rm -rf #{basedir}"
+basedir = 'tmp_acceptance_pluginsync_modules'
+module1libdir = "#{basedir}/1/a/lib"
+module2libdir = "#{basedir}/2/a/lib"
-on agents, "mkdir -p #{basedir}/1/a/lib/ #{basedir}/2/a/lib"
-# create two modules called "a", in different paths... this is intended to validate precedence in the module path;
-# if two modules are found with the same name, the one that is found earlier in the path should be used.
-create_remote_file(agents, "#{basedir}/1/a/lib/foo.rb", "#1a")
-create_remote_file(agents, "#{basedir}/2/a/lib/foo.rb", "#2a")
-on agents, puppet_apply("--modulepath=#{basedir}/1:#{basedir}/2 --pluginsync -e 'notify { \"hello\": }'") do
+begin
agents.each do |agent|
- on agent, "cat #{agent['puppetvardir']}/lib/foo.rb"
- assert_match(/#1a/, stdout, "The synced plugin was not found or the wrong version was synced")
+ create_test_file(agent, get_test_file_path(agent, "#{module1libdir}/foo.rb"), "#1a", :mkdirs => true)
+ create_test_file(agent, get_test_file_path(agent, "#{module2libdir}//foo.rb"), "#2a", :mkdirs => true)
+
+ on agent, puppet_apply("--modulepath=#{get_test_file_path(agent, "#{basedir}/1")}:#{get_test_file_path(agent, "#{basedir}/2")} --pluginsync -e 'notify { \"hello\": }'")
- on agent, "rm -f #{agent['puppetvardir']}/lib/foo.rb"
+ agent.exec("cat #{agent['puppetvardir']}/lib/foo.rb", {}) do
+ assert_match(/#1a/, stdout, "The synced plugin was not found or the wrong version was synced")
+ end
end
-end
+ensure
-on agents, "rm -rf #{basedir}"
+##########################################################################################
+# 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
diff --git a/acceptance/tests/resource/file/source_attribute.rb b/acceptance/tests/resource/file/source_attribute.rb
index 4be180d..608d289 100644
--- a/acceptance/tests/resource/file/source_attribute.rb
+++ b/acceptance/tests/resource/file/source_attribute.rb
@@ -1,5 +1,6 @@
test_name "The source attribute"
+
agents.each do |agent|
target = agent.tmpfile('source_file_test')
@@ -12,25 +13,65 @@
modulepath = stdout.split(':')[0].chomp
end
-result_file = "/tmp/#{$$}-result-file"
-source_file = File.join(modulepath, 'source_test_module', 'files', 'source_file')
-manifest = "/tmp/#{$$}-source-test.pp"
+# This is unpleasant. Because the manifest file must specify an absolute
+# path for the source property of the file resource, and because that
+# absolute path can't be the same on Windows as it is on unix, we are
+# basically forced to have two separate manifest files. This might be
+# cleaner if it were separated into two tests, but since the actual
+# functionality that we are testing is the same I decided to keep it here
+# even though it's ugly.
+windows_source_file = File.join(modulepath, 'source_test_module', 'files', 'windows_source_file')
+windows_manifest = "/tmp/#{$$}-windows-source-test.pp"
+windows_result_file = "C:/windows/temp/#{$$}-windows-result-file"
+
+posix_source_file = File.join(modulepath, 'source_test_module', 'files', 'posix_source_file')
+posix_manifest = "/tmp/#{$$}-posix-source-test.pp"
+posix_result_file = "/tmp/#{$$}-posix-result-file"
# Remove the SSL dir so we don't have cert issues
on master, "rm -rf `puppet master --configprint ssldir`"
on agents, "rm -rf `puppet agent --configprint ssldir`"
-on master, "mkdir -p #{File.dirname(source_file)}"
-on master, "echo 'the content is present' > #{source_file}"
-on master, %Q[echo "file { '#{result_file}': source => 'puppet:///modules/source_test_module/source_file', ensure => present }" > #{manifest}]
+on master, "mkdir -p #{File.dirname(windows_source_file)}"
+on master, "echo 'the content is present' > #{windows_source_file}"
+on master, "mkdir -p #{File.dirname(posix_source_file)}"
+on master, "echo 'the content is present' > #{posix_source_file}"
+
+on master, %Q[echo "file { '#{windows_result_file}': source => 'puppet:///modules/source_test_module/windows_source_file', ensure => present }" > #{windows_manifest}]
+on master, %Q[echo "file { '#{posix_result_file}': source => 'puppet:///modules/source_test_module/posix_source_file', ensure => present }" > #{posix_manifest}]
+
+# See disgusted comments above... running master once with the windows manifest
+# and then once with the posix manifest. Could potentially get around this by
+# creating a manifest with nodes or by moving the windows bits into a separate
+# test.
+ with_master_running_on master, "--autosign true --manifest #{windows_manifest} --dns_alt_names=\"puppet, $(hostname -s), $(hostname -f)\"" do
+ agents.each do |agent|
+ next unless agent['platform'].include?('windows')
+ run_agent_on agent, "--test --server #{master}", :acceptable_exit_codes => [2] do
+ on agent, "cat #{windows_result_file}" do
+ assert_match(/the content is present/, stdout, "Result file not created")
+ end
+ end
+ end
+ end
-with_master_running_on master, "--autosign true --manifest #{manifest} --dns_alt_names=\"puppet, $(hostname -s), $(hostname -f)\"" do
- run_agent_on agents, "--test --server #{master}", :acceptable_exit_codes => [2] do
- on agents, "cat #{result_file}" do
- assert_match(/the content is present/, stdout, "Result file not created")
+ #run_agent_on agents, "--test --server #{master}", :acceptable_exit_codes => [2] do
+ # on agents, "cat #{result_file}" do
+ # assert_match(/the content is present/, stdout, "Result file not created")
+ # end
+ #end
+
+ with_master_running_on master, "--autosign true --manifest #{posix_manifest} --dns_alt_names=\"puppet, $(hostname -s), $(hostname -f)\"" do
+ agents.each do |agent|
+ next if agent['platform'].include?('windows')
+ run_agent_on agent, "--test --server #{master}", :acceptable_exit_codes => [2] do
+ on agent, "cat #{posix_result_file}" do
+ assert_match(/the content is present/, stdout, "Result file not created")
+ end
+ end
end
end
-end
+
# TODO: Add tests for puppet:// URIs with multi-master/agent setups.
# step "when using a puppet://$server/ URI with a master/agent setup"
-- 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.
