Disable unforked interval runs.

Clarify error message.

Move fork and interval logic to application (specs pending).

Clean code logic, fix specs.

Allow unforked client runs with daemonize or splay

7 files changed, 161 insertions, 82 deletions

diff --git a/lib/chef/application.rb b/lib/chef/application.rb
index 0430d4acfa..c007867f2b 100644
--- a/lib/chef/application.rb
+++ b/lib/chef/application.rb
@@ -206,12 +206,56 @@ class Chef::Application
       )
       @chef_client_json = nil
 
-      @chef_client.run
+      if Chef::Config[:client_fork]
+        fork_chef_client # allowed to run client in forked process
+      else
+        @chef_client.run
+      end
       @chef_client = nil
     end
   end
 
   private
+  def fork_chef_client
+    # win32-process gem exposes some form of :fork for Process
+    # class. So we are seperately ensuring that the platform we're
+    # running on is not windows before forking.
+    if(Process.respond_to?(:fork) && !Chef::Platform.windows?)
+      Chef::Log.info "Forking chef instance to converge..."
+      pid = fork do
+        [:INT, :TERM].each {|s| trap(s, "EXIT") }
+        client_solo = Chef::Config[:solo] ? "chef-solo" : "chef-client"
+        $0 = "#{client_solo} worker: ppid=#{Process.ppid};start=#{Time.new.strftime("%R:%S")};"
+        begin
+          Chef::Log.debug "Forked instance now converging"
+          @chef_client.run
+        rescue Exception => e
+          Chef::Log.error(e.to_s)
+          exit 1
+        else
+          exit 0
+        end
+      end
+      Chef::Log.debug "Fork successful. Waiting for new chef pid: #{pid}"
+      result = Process.waitpid2(pid)
+      handle_child_exit(result)
+      Chef::Log.debug "Forked instance successfully reaped (pid: #{pid})"
+      true
+    else
+      @chef_client.run
+    end
+  end
+
+  def handle_child_exit(pid_and_status)
+    status = pid_and_status[1]
+    return true if status.success?
+    message = if status.signaled?
+      "Chef run process terminated by signal #{status.termsig} (#{Signal.list.invert[status.termsig]})"
+    else
+      "Chef run process exited unsuccessfully (exit code #{status.exitstatus})"
+    end
+    raise Exceptions::ChildConvergeError, message
+  end
 
   def apply_config(config_content, config_file_path)
     Chef::Config.from_string(config_content, config_file_path)
diff --git a/lib/chef/application/client.rb b/lib/chef/application/client.rb
index 6c06ad656d..4b96e89e42 100644
--- a/lib/chef/application/client.rb
+++ b/lib/chef/application/client.rb
@@ -259,6 +259,7 @@ class Chef::Application::Client < Chef::Application
     Chef::Config.chef_zero.host = config[:chef_zero_host] if config[:chef_zero_host]
     Chef::Config.chef_zero.port = config[:chef_zero_port] if config[:chef_zero_port]
 
+
     if Chef::Config[:daemonize]
       Chef::Config[:interval] ||= 1800
     end
@@ -268,6 +269,8 @@ class Chef::Application::Client < Chef::Application
       Chef::Config[:splay] = nil
     end
 
+    Chef::Application.fatal!(unforked_interval_error_message) if !Chef::Config[:client_fork] && Chef::Config[:interval]
+
     if Chef::Config[:json_attribs]
       config_fetcher = Chef::ConfigFetcher.new(Chef::Config[:json_attribs])
       @chef_client_json = config_fetcher.fetch_json
@@ -318,6 +321,22 @@ class Chef::Application::Client < Chef::Application
       puts "Chef version: #{::Chef::VERSION}"
     end
 
+    if !Chef::Config[:client_fork] || Chef::Config[:once]
+      begin
+        # run immediately without interval sleep, or splay
+        run_chef_client(Chef::Config[:specific_recipes])
+      rescue SystemExit
+        raise
+      rescue Exception => e
+        Chef::Application.fatal!("#{e.class}: #{e.message}", 1)
+      end
+    else
+      interval_run_chef_client
+    end
+  end
+
+  private
+  def interval_run_chef_client
     if Chef::Config[:daemonize]
       Chef::Daemon.daemonize("chef-client")
     end
@@ -358,8 +377,6 @@ class Chef::Application::Client < Chef::Application
     end
   end
 
-  private
-
   def interval_sleep
     unless SELF_PIPE.empty?
       client_sleep Chef::Config[:interval]
@@ -373,4 +390,11 @@ class Chef::Application::Client < Chef::Application
     IO.select([ SELF_PIPE[0] ], nil, nil, sec) or return
     SELF_PIPE[0].getc.chr
   end
+
+  def unforked_interval_error_message
+    "Unforked chef-client interval runs are disabled in Chef 12." +
+    "\nConfiguration settings:" +
+    "#{"\n  interval  = #{Chef::Config[:interval]} seconds" if Chef::Config[:interval]}" +
+    "\nEnable chef-client interval runs by setting `:client_fork = true` in your config file or adding `--fork` to your command line options."
+  end
 end
diff --git a/lib/chef/application/solo.rb b/lib/chef/application/solo.rb
index f0e578d5ef..cf7b5b4223 100644
--- a/lib/chef/application/solo.rb
+++ b/lib/chef/application/solo.rb
@@ -185,6 +185,8 @@ class Chef::Application::Solo < Chef::Application
       Chef::Config[:interval] ||= 1800
     end
 
+    Chef::Application.fatal!(unforked_interval_error_message) if !Chef::Config[:client_fork] && Chef::Config[:interval]
+
     if Chef::Config[:recipe_url]
       cookbooks_path = Array(Chef::Config[:cookbook_path]).detect{|e| e =~ /\/cookbooks\/*$/ }
       recipes_path = File.expand_path(File.join(cookbooks_path, '..'))
@@ -209,6 +211,22 @@ class Chef::Application::Solo < Chef::Application
   end
 
   def run_application
+    if !Chef::Config[:client_fork] || Chef::Config[:once]
+      # Run immediately without interval sleep or splay
+      begin
+        run_chef_client(Chef::Config[:specific_recipes])
+      rescue SystemExit
+        raise
+      rescue Exception => e
+        Chef::Application.fatal!("#{e.class}: #{e.message}", 1)
+      end
+    else
+      interval_run_chef_client
+    end
+  end
+
+  private
+  def interval_run_chef_client
     if Chef::Config[:daemonize]
       Chef::Daemon.daemonize("chef-client")
     end
@@ -244,8 +262,6 @@ class Chef::Application::Solo < Chef::Application
     end
   end
 
-  private
-
   def fetch_recipe_tarball(url, path)
     Chef::Log.debug("Download recipes tarball from #{url} to #{path}")
     File.open(path, 'wb') do |f|
@@ -254,4 +270,11 @@ class Chef::Application::Solo < Chef::Application
       end
     end
   end
+
+  def unforked_interval_error_message
+    "Unforked chef-client interval runs are disabled in Chef 12." +
+    "\nConfiguration settings:" +
+    "#{"\n  interval  = #{Chef::Config[:interval]} seconds" if Chef::Config[:interval]}" +
+    "\nEnable chef-client interval runs by setting `:client_fork = true` in your config file or adding `--fork` to your command line options."
+  end
 end
diff --git a/lib/chef/client.rb b/lib/chef/client.rb
index 161ecddb0f..cc712a3f3e 100644
--- a/lib/chef/client.rb
+++ b/lib/chef/client.rb
@@ -191,54 +191,6 @@ class Chef
       end
     end
 
-    # Do a full run for this Chef::Client.  Calls:
-    # * do_run
-    #
-    # This provides a wrapper around #do_run allowing the
-    # run to be optionally forked.
-    # === Returns
-    # boolean:: Return value from #do_run. Should always returns true.
-    def run
-      # win32-process gem exposes some form of :fork for Process
-      # class. So we are seperately ensuring that the platform we're
-      # running on is not windows before forking.
-      if(Chef::Config[:client_fork] && Process.respond_to?(:fork) && !Chef::Platform.windows?)
-        Chef::Log.info "Forking chef instance to converge..."
-        pid = fork do
-          [:INT, :TERM].each {|s| trap(s, "EXIT") }
-          client_solo = Chef::Config[:solo] ? "chef-solo" : "chef-client"
-          $0 = "#{client_solo} worker: ppid=#{Process.ppid};start=#{Time.new.strftime("%R:%S")};"
-          begin
-            Chef::Log.debug "Forked instance now converging"
-            do_run
-          rescue Exception => e
-            Chef::Log.error(e.to_s)
-            exit 1
-          else
-            exit 0
-          end
-        end
-        Chef::Log.debug "Fork successful. Waiting for new chef pid: #{pid}"
-        result = Process.waitpid2(pid)
-        handle_child_exit(result)
-        Chef::Log.debug "Forked instance successfully reaped (pid: #{pid})"
-        true
-      else
-        do_run
-      end
-    end
-
-    def handle_child_exit(pid_and_status)
-      status = pid_and_status[1]
-      return true if status.success?
-      message = if status.signaled?
-        "Chef run process terminated by signal #{status.termsig} (#{Signal.list.invert[status.termsig]})"
-      else
-        "Chef run process exited unsuccessfully (exit code #{status.exitstatus})"
-      end
-      raise Exceptions::ChildConvergeError, message
-    end
-
     # Instantiates a Chef::Node object, possibly loading the node's prior state
     # when using chef-client. Delegates to policy_builder
     #
@@ -383,8 +335,6 @@ class Chef
       end
     end
 
-    private
-
     # Do a full run for this Chef::Client.  Calls:
     #
     #  * run_ohai - Collect information about the system
@@ -395,7 +345,7 @@ class Chef
     #
     # === Returns
     # true:: Always returns true.
-    def do_run
+    def run
       runlock = RunLock.new(Chef::Config.lockfile)
       runlock.acquire
       # don't add code that may fail before entering this section to be sure to release lock
@@ -466,6 +416,8 @@ class Chef
       true
     end
 
+    private
+
     def empty_directory?(path)
       !File.exists?(path) || (Dir.entries(path).size <= 2)
     end
diff --git a/spec/unit/application/client_spec.rb b/spec/unit/application/client_spec.rb
index 3bc6b97d3d..f0b4f3bda8 100644
--- a/spec/unit/application/client_spec.rb
+++ b/spec/unit/application/client_spec.rb
@@ -39,6 +39,43 @@ describe Chef::Application::Client, "reconfigure" do
     ARGV.replace(@original_argv)
   end
 
+  describe "when configured to not fork the client process" do
+    before do
+      Chef::Config[:client_fork] = false
+      Chef::Config[:daemonize] = false
+      Chef::Config[:interval] = nil
+      Chef::Config[:splay] = nil
+    end
+
+    context "when interval is given" do
+      before do
+        Chef::Config[:interval] = 600
+      end
+
+      it "should terminate with message" do
+        Chef::Application.should_receive(:fatal!).with(
+"Unforked chef-client interval runs are disabled in Chef 12.
+Configuration settings:
+  interval  = 600 seconds
+Enable chef-client interval runs by setting `:client_fork = true` in your config file or adding `--fork` to your command line options."
+        )
+        @app.reconfigure
+      end
+    end
+
+    context "when configured to run once" do
+      before do
+        Chef::Config[:once] = true
+        Chef::Config[:interval] = 1000
+      end
+
+      it "should reconfigure chef-client" do
+        @app.reconfigure
+        Chef::Config[:interval].should be_nil
+      end
+    end
+  end
+
   describe "when in daemonized mode and no interval has been set" do
     before do
       Chef::Config[:daemonize] = true
diff --git a/spec/unit/application/solo_spec.rb b/spec/unit/application/solo_spec.rb
index 787f9ff43c..e29fcc9367 100644
--- a/spec/unit/application/solo_spec.rb
+++ b/spec/unit/application/solo_spec.rb
@@ -36,6 +36,31 @@ describe Chef::Application::Solo do
       Chef::Config[:solo].should be_true
     end
 
+    describe "when configured to not fork the client process" do
+      before do
+        Chef::Config[:client_fork] = false
+        Chef::Config[:daemonize] = false
+        Chef::Config[:interval] = nil
+        Chef::Config[:splay] = nil
+      end
+
+      context "when interval is given" do
+        before do
+          Chef::Config[:interval] = 600
+        end
+
+        it "should terminate with message" do
+          Chef::Application.should_receive(:fatal!).with(
+"Unforked chef-client interval runs are disabled in Chef 12.
+Configuration settings:
+  interval  = 600 seconds
+Enable chef-client interval runs by setting `:client_fork = true` in your config file or adding `--fork` to your command line options."
+          )
+          @app.reconfigure
+        end
+      end
+    end
+
     describe "when in daemonized mode and no interval has been set" do
       before do
         Chef::Config[:daemonize] = true
@@ -142,4 +167,3 @@ describe Chef::Application::Solo do
   end
 
 end
-
diff --git a/spec/unit/client_spec.rb b/spec/unit/client_spec.rb
index 37a2dc09ca..e05245c413 100644
--- a/spec/unit/client_spec.rb
+++ b/spec/unit/client_spec.rb
@@ -301,25 +301,6 @@ describe Chef::Client do
       include_examples "a successful client run"
     end
 
-    describe "when running chef-client with forking enabled", :unix_only do
-      include_examples "a successful client run" do
-        let(:process_status) do
-          double("Process::Status")
-        end
-
-        let(:enable_fork) { true }
-
-        before do
-          Process.should_receive(:waitpid2).and_return([1, process_status])
-
-          process_status.should_receive(:success?).and_return(true)
-          client.should_receive(:exit).and_return(nil)
-          client.should_receive(:fork).and_yield
-        end
-      end
-
-    end
-
     describe "when the client key already exists" do
 
       let(:api_client_exists?) { true }
@@ -389,7 +370,6 @@ describe Chef::Client do
           client.run
           node.run_list.should == Chef::RunList.new(new_runlist)
         end
-
       end
     end
 
@@ -409,11 +389,6 @@ describe Chef::Client do
       client = Chef::Client.new
       client.stub(:load_node).and_raise(Exception)
       @run_lock.should_receive(:release)
-      if(Chef::Config[:client_fork] && !windows?)
-        client.should_receive(:fork) do |&block|
-          block.call
-        end
-      end
       lambda { client.run }.should raise_error(Exception)
     end
   end