diff --git a/lib/chef/mixin/command.rb b/lib/chef/mixin/command.rb index 55c028ff5f3..719151a6b6e 100644 --- a/lib/chef/mixin/command.rb +++ b/lib/chef/mixin/command.rb @@ -56,8 +56,17 @@ module Command # === Returns # Returns the exit status of args[:command] def run_command(args={}) - command_output = "" + status, stdout, stderr = run_command_and_return_stdout_stderr(args) + status + end + + # works same as above, except that it returns stdout and stderr + # requirement => platforms like solaris 9,10 has wierd issues where + # even in command failure the exit code is zero, so we need to lookup stderr. + def run_command_and_return_stdout_stderr(args={}) + command_output = "" + args[:ignore_failure] ||= false args[:output_on_failure] ||= false @@ -73,10 +82,10 @@ def run_command(args={}) command_output << "STDOUT: #{stdout}" command_output << "STDERR: #{stderr}" handle_command_failures(status, command_output, args) - - status + + return status, stdout, stderr end - + def output_of_command(command, args) Chef::Log.debug("Executing #{command}") stderr_string, stdout_string, status = "", "", nil diff --git a/lib/chef/platform/provider_mapping.rb b/lib/chef/platform/provider_mapping.rb index c9706fb220c..2911e322841 100644 --- a/lib/chef/platform/provider_mapping.rb +++ b/lib/chef/platform/provider_mapping.rb @@ -322,7 +322,8 @@ def platforms :default => { :group => Chef::Provider::Group::Aix, :mount => Chef::Provider::Mount::Aix, - :ifconfig => Chef::Provider::Ifconfig::Aix + :ifconfig => Chef::Provider::Ifconfig::Aix, + :cron => Chef::Provider::Cron::Aix } }, :default => { diff --git a/lib/chef/provider/cron.rb b/lib/chef/provider/cron.rb index a7218fea5a9..4430e3e18f6 100644 --- a/lib/chef/provider/cron.rb +++ b/lib/chef/provider/cron.rb @@ -45,6 +45,7 @@ def load_current_resource crontab_lines = [] @current_resource = Chef::Resource::Cron.new(@new_resource.name) @current_resource.user(@new_resource.user) + @cron_exists = false if crontab = read_crontab cron_found = false crontab.each_line do |line| @@ -93,14 +94,7 @@ def action_create newcron = String.new cron_found = false - newcron << "# Chef Name: #{new_resource.name}\n" - [ :mailto, :path, :shell, :home ].each do |v| - newcron << "#{v.to_s.upcase}=#{@new_resource.send(v)}\n" if @new_resource.send(v) - end - @new_resource.environment.each do |name, value| - newcron << "#{name}=#{value}\n" - end - newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday} #{@new_resource.command}\n" + newcron = get_crontab_entry if @cron_exists unless cron_different? @@ -202,13 +196,33 @@ def read_crontab end def write_crontab(crontab) + write_exception = false status = popen4("crontab -u #{@new_resource.user} -", :waitlast => true) do |pid, stdin, stdout, stderr| - stdin.write crontab + begin + stdin.write crontab + rescue Errno::EPIPE => e + # popen4 could yield while child has already died. + write_exception = true + Chef::Log.debug("#{e.message}") + end end - if status.exitstatus > 0 + if status.exitstatus > 0 || write_exception raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status.exitstatus}" end end + + def get_crontab_entry + newcron = "" + newcron << "# Chef Name: #{new_resource.name}\n" + [ :mailto, :path, :shell, :home ].each do |v| + newcron << "#{v.to_s.upcase}=#{@new_resource.send(v)}\n" if @new_resource.send(v) + end + @new_resource.environment.each do |name, value| + newcron << "#{name}=#{value}\n" + end + newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday} #{@new_resource.command}\n" + newcron + end end end end diff --git a/lib/chef/provider/cron/aix.rb b/lib/chef/provider/cron/aix.rb new file mode 100644 index 00000000000..473700bf2f7 --- /dev/null +++ b/lib/chef/provider/cron/aix.rb @@ -0,0 +1,48 @@ +# +# Author:: Kaustubh Deorukhkar () +# Copyright:: Copyright (c) 2013 Opscode, Inc. +# License:: Apache License, Version 2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +require "chef/provider/cron/unix" + +class Chef + class Provider + class Cron + class Aix < Chef::Provider::Cron::Unix + + private + + # For AIX we ignore env vars/[ :mailto, :path, :shell, :home ] + def get_crontab_entry + if env_vars_are_set? + raise Chef::Exceptions::Cron, "Aix cron entry does not support environment variables. Please set them in script and use script in cron." + end + + newcron = "" + newcron << "# Chef Name: #{new_resource.name}\n" + newcron << "#{@new_resource.minute} #{@new_resource.hour} #{@new_resource.day} #{@new_resource.month} #{@new_resource.weekday}" + + newcron << " #{@new_resource.command}\n" + newcron + end + + def env_vars_are_set? + @new_resource.environment.length > 0 || !@new_resource.mailto.nil? || !@new_resource.path.nil? || !@new_resource.shell.nil? || !@new_resource.home.nil? + end + end + end + end +end diff --git a/lib/chef/provider/cron/solaris.rb b/lib/chef/provider/cron/solaris.rb index e0811ba0acf..6861820676a 100644 --- a/lib/chef/provider/cron/solaris.rb +++ b/lib/chef/provider/cron/solaris.rb @@ -1,15 +1,13 @@ # -# Author:: Bryan McLellan (btm@loftninjas.org) -# Author:: Toomas Pelberg (toomasp@gmx.net) -# Copyright:: Copyright (c) 2009 Bryan McLellan -# Copyright:: Copyright (c) 2010 Toomas Pelberg +# Author:: Kaustubh Deorukhkar () +# Copyright:: Copyright (c) 2013 Opscode, Inc. # License:: Apache License, Version 2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # -# http://www.apache.org/licenses/LICENSE-2.0 +# http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, @@ -18,39 +16,7 @@ # limitations under the License. # -require 'chef/log' -require 'chef/provider' +require "chef/provider/cron/unix" -class Chef - class Provider - class Cron - class Solaris < Chef::Provider::Cron - - private - - def read_crontab - crontab = nil - status = popen4("crontab -l #{@new_resource.user}") do |pid, stdin, stdout, stderr| - crontab = stdout.read - end - if status.exitstatus > 1 - raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status.exitstatus}" - end - crontab - end - - def write_crontab(crontab) - tempcron = Tempfile.new("chef-cron") - tempcron << crontab - tempcron.flush - tempcron.chmod(0644) - status = run_command(:command => "/usr/bin/crontab #{tempcron.path}",:user => @new_resource.user) - tempcron.close! - if status.exitstatus > 0 - raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{status.exitstatus}" - end - end - end - end - end -end +# Just to create an alias so 'Chef::Provider::Cron::Solaris' is exposed and accessible to existing consumers of class. +Chef::Provider::Cron::Solaris = Chef::Provider::Cron::Unix \ No newline at end of file diff --git a/lib/chef/provider/cron/unix.rb b/lib/chef/provider/cron/unix.rb new file mode 100644 index 00000000000..1149f43f769 --- /dev/null +++ b/lib/chef/provider/cron/unix.rb @@ -0,0 +1,76 @@ +# +# Author:: Bryan McLellan (btm@loftninjas.org) +# Author:: Toomas Pelberg (toomasp@gmx.net) +# Copyright:: Copyright (c) 2009 Bryan McLellan +# Copyright:: Copyright (c) 2010 Toomas Pelberg +# License:: Apache License, Version 2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +require 'chef/log' +require 'chef/provider' + +class Chef + class Provider + class Cron + class Unix < Chef::Provider::Cron + + private + + def read_crontab + crontab = nil + status = popen4("crontab -l #{@new_resource.user}") do |pid, stdin, stdout, stderr| + crontab = stdout.read + end + if status.exitstatus > 1 + raise Chef::Exceptions::Cron, "Error determining state of #{@new_resource.name}, exit: #{status.exitstatus}" + end + crontab + end + + def write_crontab(crontab) + tempcron = Tempfile.new("chef-cron") + tempcron << crontab + tempcron.flush + tempcron.chmod(0644) + exit_status = 0 + error_message = "" + begin + status, stdout, stderr = run_command_and_return_stdout_stderr(:command => "/usr/bin/crontab #{tempcron.path}",:user => @new_resource.user) + exit_status = status.exitstatus + # solaris9, 10 on some failures for example invalid 'mins' in crontab fails with exit code of zero :( + if stderr && stderr.include?("errors detected in input, no crontab file generated") + error_message = stderr + exit_status = 1 + end + rescue Chef::Exceptions::Exec => e + Chef::Log.debug(e.message) + exit_status = 1 + error_message = e.message + rescue ArgumentError => e + # usually raised on invalid user. + Chef::Log.debug(e.message) + exit_status = 1 + error_message = e.message + end + tempcron.close! + if exit_status > 0 + raise Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: #{exit_status}, message: #{error_message}" + end + end + + end + end + end +end diff --git a/lib/chef/providers.rb b/lib/chef/providers.rb index 65d251f6876..a87aa9d1411 100644 --- a/lib/chef/providers.rb +++ b/lib/chef/providers.rb @@ -21,6 +21,7 @@ require 'chef/provider/cookbook_file' require 'chef/provider/cron' require 'chef/provider/cron/solaris' +require 'chef/provider/cron/aix' require 'chef/provider/deploy' require 'chef/provider/directory' require 'chef/provider/env' diff --git a/spec/functional/resource/cron_spec.rb b/spec/functional/resource/cron_spec.rb new file mode 100644 index 00000000000..35be2bbaa6f --- /dev/null +++ b/spec/functional/resource/cron_spec.rb @@ -0,0 +1,147 @@ +# encoding: UTF-8 +# +# Author:: Kaustubh Deorukhkar () +# Copyright:: Copyright (c) 2013 Opscode, Inc. +# License:: Apache License, Version 2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +require 'functional/resource/base' +require 'chef/mixin/shell_out' + +describe Chef::Resource::Cron, :requires_root, :unix_only do + + include Chef::Mixin::ShellOut + + # Platform specific validation routines. + def cron_should_exists(cron_name, command) + case ohai[:platform] + when "aix", "solaris", "opensolaris", "solaris2", "omnios" + expect(shell_out("crontab -l #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(0) + expect(shell_out("crontab -l #{new_resource.user} | grep \"#{command}\"").exitstatus).to eq(0) + else + expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(0) + expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{command}\"").exitstatus).to eq(0) + end + end + + def cron_should_not_exists(cron_name) + case ohai[:platform] + when "aix", "solaris", "opensolaris", "solaris2", "omnios" + expect(shell_out("crontab -l #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(1) + else + expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{cron_name}\"").exitstatus).to eq(1) + end + end + + # Actual tests + let(:new_resource) do + new_resource = Chef::Resource::Cron.new("Chef functional test cron", run_context) + new_resource.user 'root' + new_resource.minute "30" + new_resource.command "/bin/true" + new_resource + end + + let(:provider) do + provider = new_resource.provider_for_action(new_resource.action) + provider + end + + describe "create action" do + after do + new_resource.run_action(:delete) + end + + it "should create a crontab entry" do + new_resource.run_action(:create) + cron_should_exists(new_resource.name, new_resource.command) + end + end + + describe "delete action" do + before do + new_resource.run_action(:create) + end + + it "should delete a crontab entry" do + # Note that test cron is created by previous test + new_resource.run_action(:delete) + + cron_should_not_exists(new_resource.name) + end + end + + exclude_solaris = ["solaris", "opensolaris", "solaris2", "omnios"].include?(ohai[:platform]) + describe "create action with various attributes", :external => exclude_solaris do + def create_and_validate_with_attribute(resource, attribute, value) + if ohai[:platform] == 'aix' + expect {resource.run_action(:create)}.to raise_error(Chef::Exceptions::Cron, /Aix cron entry does not support environment variables. Please set them in script and use script in cron./) + else + resource.run_action(:create) + # Verify if the cron is created successfully + cron_attribute_should_exists(resource.name, attribute, value) + end + end + + def cron_attribute_should_exists(cron_name, attribute, value) + return if ['aix', 'solaris'].include?(ohai[:platform]) + # Test if the attribute exists on newly created cron + cron_should_exists(cron_name, "") + expect(shell_out("crontab -l -u #{new_resource.user} | grep \"#{attribute.upcase}=#{value}\"").exitstatus).to eq(0) + end + + after do + new_resource.run_action(:delete) + end + + it "should create a crontab entry for mailto attribute" do + new_resource.mailto "cheftest@example.com" + create_and_validate_with_attribute(new_resource, "mailto", "cheftest@example.com") + end + + it "should create a crontab entry for path attribute" do + new_resource.path "/usr/local/bin" + create_and_validate_with_attribute(new_resource, "path", "/usr/local/bin") + end + + it "should create a crontab entry for shell attribute" do + new_resource.shell "/bin/bash" + create_and_validate_with_attribute(new_resource, "shell", "/bin/bash") + end + + it "should create a crontab entry for home attribute" do + new_resource.home "/home/opscode" + create_and_validate_with_attribute(new_resource, "home", "/home/opscode") + end + end + + describe "negative tests for create action" do + def cron_create_should_raise_exception + expect { new_resource.run_action(:create) }.to raise_error(Chef::Exceptions::Cron, /Error updating state of #{new_resource.name}, exit: 1/) + cron_should_not_exists(new_resource.name) + end + + it "should not create cron with invalid minute" do + new_resource.minute "invalid" + cron_create_should_raise_exception + end + + it "should not create cron with invalid user" do + new_resource.user "1-really-really-invalid-user-name" + cron_create_should_raise_exception + end + + end +end \ No newline at end of file diff --git a/spec/unit/provider/cron/solaris_spec.rb b/spec/unit/provider/cron/unix_spec.rb similarity index 87% rename from spec/unit/provider/cron/solaris_spec.rb rename to spec/unit/provider/cron/unix_spec.rb index 498d4fb1f53..ffdfa198b6e 100644 --- a/spec/unit/provider/cron/solaris_spec.rb +++ b/spec/unit/provider/cron/unix_spec.rb @@ -20,7 +20,7 @@ require 'spec_helper' -describe Chef::Provider::Cron::Solaris do +describe Chef::Provider::Cron::Unix do before do @node = Chef::Node.new @events = Chef::EventDispatch::Dispatcher.new @@ -30,7 +30,7 @@ @new_resource.minute "30" @new_resource.command "/bin/true" - @provider = Chef::Provider::Cron::Solaris.new(@new_resource, @run_context) + @provider = Chef::Provider::Cron::Unix.new(@new_resource, @run_context) end it "should inherit from Chef::Provider:Cron" do @@ -86,7 +86,7 @@ describe "write_crontab" do before :each do @status = mock("Status", :exitstatus => 0) - @provider.stub!(:run_command).and_return(@status) + @provider.stub!(:run_command_and_return_stdout_stderr).and_return(@status, String.new, String.new) @tempfile = mock("foo", :path => "/tmp/foo", :close => true) Tempfile.stub!(:new).and_return(@tempfile) @tempfile.should_receive(:flush) @@ -95,13 +95,13 @@ end it "should call crontab for the user" do - @provider.should_receive(:run_command).with(hash_including(:user => @new_resource.user)) + @provider.should_receive(:run_command_and_return_stdout_stderr).with(hash_including(:user => @new_resource.user)) @tempfile.should_receive(:<<).with("Foo") @provider.send(:write_crontab, "Foo") end it "should call crontab with a file containing the crontab" do - @provider.should_receive(:run_command) do |args| + @provider.should_receive(:run_command_and_return_stdout_stderr) do |args| (args[:command] =~ %r{\A/usr/bin/crontab (/\S+)\z}).should be_true $1.should == "/tmp/foo" @status @@ -115,7 +115,7 @@ @status.stub!(:exitstatus).and_return(1) lambda do @provider.send(:write_crontab, "Foo") - end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1") + end.should raise_error(Chef::Exceptions::Cron, /Error updating state of #{@new_resource.name}, exit: 1/) end end end diff --git a/spec/unit/provider/cron_spec.rb b/spec/unit/provider/cron_spec.rb index 5a848a30e60..f0d800741d4 100644 --- a/spec/unit/provider/cron_spec.rb +++ b/spec/unit/provider/cron_spec.rb @@ -563,7 +563,7 @@ @provider.stub!(:read_crontab).and_return(<<-CRONTAB) 0 2 * * * /some/other/command -# Chef Name: something else +# Chef Name: cronhole some stuff * 5 * * * /bin/true # Another comment @@ -581,6 +581,7 @@ end it "should log nothing changed" do + Chef::Log.should_receive(:debug).with("Found cron '#{@new_resource.name}'") Chef::Log.should_receive(:debug).with("Skipping existing cron entry '#{@new_resource.name}'") @provider.run_action(:create) end @@ -808,5 +809,22 @@ @provider.send(:write_crontab, "Foo") end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1") end + + it "should raise an exception if the command die's and parent tries to write" do + class WriteErrPipe + def write(str) + raise Errno::EPIPE, "Test" + end + end + @status.stub!(:exitstatus).and_return(1) + @provider.stub!(:popen4).and_yield(1234, WriteErrPipe.new, StringIO.new, StringIO.new).and_return(@status) + + Chef::Log.should_receive(:debug).with("Broken pipe - Test") + + lambda do + @provider.send(:write_crontab, "Foo") + end.should raise_error(Chef::Exceptions::Cron, "Error updating state of #{@new_resource.name}, exit: 1") + end + end end