Merge pull request #6914 from chef/lcg/no-fork-default-command-line

 Don't use supervisor process for one-shot / command-line runs

5 files changed, 153 insertions, 21 deletions

diff --git a/chef-config/lib/chef-config/config.rb b/chef-config/lib/chef-config/config.rb
index d5badcc58f..9d11ba9dc4 100644
--- a/chef-config/lib/chef-config/config.rb
+++ b/chef-config/lib/chef-config/config.rb
@@ -4,7 +4,7 @@
 # Author:: AJ Christensen (<aj@chef.io>)
 # Author:: Mark Mzyk (<mmzyk@chef.io>)
 # Author:: Kyle Goodwin (<kgoodwin@primerevenue.com>)
-# Copyright:: Copyright 2008-2017, Chef Software Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -440,7 +440,7 @@ module ChefConfig
     default :splay, nil
     default :why_run, false
     default :color, false
-    default :client_fork, true
+    default :client_fork, nil
     default :ez, false
     default :enable_reporting, true
     default :enable_reporting_url_fatals, false
diff --git a/lib/chef/application/client.rb b/lib/chef/application/client.rb
index 0834e5f037..706ba93ca0 100644
--- a/lib/chef/application/client.rb
+++ b/lib/chef/application/client.rb
@@ -2,7 +2,7 @@
 # Author:: AJ Christensen (<aj@chef.io)
 # Author:: Christopher Brown (<cb@chef.io>)
 # Author:: Mark Mzyk (mmzyk@chef.io)
-# Copyright:: Copyright 2008-2016, Chef Software, Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -228,8 +228,7 @@ class Chef::Application::Client < Chef::Application
   option :client_fork,
     :short        => "-f",
     :long         => "--[no-]fork",
-    :description  => "Fork client",
-    :boolean      => true
+    :description  => "Fork client"
 
   option :recipe_url,
     :long         => "--recipe-url=RECIPE_URL",
@@ -362,6 +361,11 @@ class Chef::Application::Client < Chef::Application
       Chef::Config[:splay] = nil
     end
 
+    # supervisor processes are enabled by default for interval-running processes but not for one-shot runs
+    if Chef::Config[:client_fork].nil?
+      Chef::Config[:client_fork] = !!Chef::Config[:interval]
+    end
+
     if !Chef::Config[:client_fork] && Chef::Config[:interval] && !Chef::Platform.windows?
       Chef::Application.fatal!(unforked_interval_error_message)
     end
diff --git a/lib/chef/application/solo.rb b/lib/chef/application/solo.rb
index 6b4aef42b4..f8502edc38 100644
--- a/lib/chef/application/solo.rb
+++ b/lib/chef/application/solo.rb
@@ -1,7 +1,7 @@
 #
 # Author:: AJ Christensen (<aj@chef.io>)
 # Author:: Mark Mzyk (mmzyk@chef.io)
-# Copyright:: Copyright 2008-2018, Chef Software, Inc.
+# Copyright:: Copyright 2008-2018, Chef Software Inc.
 # License:: Apache License, Version 2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -175,8 +175,7 @@ class Chef::Application::Solo < Chef::Application
   option :client_fork,
     :short        => "-f",
     :long         => "--[no-]fork",
-    :description  => "Fork client",
-    :boolean      => true
+    :description  => "Fork client"
 
   option :why_run,
     :short        => "-W",
@@ -261,6 +260,11 @@ class Chef::Application::Solo < Chef::Application
       Chef::Config[:interval] ||= 1800
     end
 
+    # supervisor processes are enabled by default for interval-running processes but not for one-shot runs
+    if Chef::Config[:client_fork].nil?
+      Chef::Config[:client_fork] = !!Chef::Config[:interval]
+    end
+
     Chef::Application.fatal!(unforked_interval_error_message) if !Chef::Config[:client_fork] && Chef::Config[:interval]
 
     if Chef::Config[:recipe_url]
diff --git a/spec/integration/client/client_spec.rb b/spec/integration/client/client_spec.rb
index de12b8ba8e..77008524c8 100644
--- a/spec/integration/client/client_spec.rb
+++ b/spec/integration/client/client_spec.rb
@@ -46,7 +46,7 @@ describe "chef-client" do
   # we're running `chef-client` from the source tree and not the external one.
   # cf. CHEF-4914
   let(:chef_client) { "ruby '#{chef_dir}/chef-client' --minimal-ohai" }
-  let(:chef_solo) { "ruby '#{chef_dir}/chef-solo' --minimal-ohai" }
+  let(:chef_solo) { "ruby '#{chef_dir}/chef-solo' --legacy-mode --minimal-ohai" }
 
   let(:critical_env_vars) { %w{_ORIGINAL_GEM_PATH GEM_PATH GEM_HOME GEM_ROOT BUNDLE_BIN_PATH BUNDLE_GEMFILE RUBYLIB RUBYOPT RUBY_ENGINE RUBY_ROOT RUBY_VERSION PATH}.map { |o| "#{o}=#{ENV[o]}" } .join(" ") }
 
@@ -624,4 +624,102 @@ EOM
       expect(command.stdout).not_to include("INFO")
     end
   end
+
+  when_the_repository "has a cookbook that knows if we're running forked" do
+    before do
+      file "cookbooks/x/recipes/default.rb", <<~EOM
+        puts Chef::Config[:client_fork] ? "WITHFORK" : "NOFORK"
+EOM
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+EOM
+    end
+
+    it "chef-client runs by default with no supervisor" do
+      command = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+
+    it "chef-solo runs by default with no supervisor" do
+      command = shell_out("#{chef_solo} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+
+    it "chef-client --no-fork does not fork" do
+      command = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" -o 'x::default' --no-fork", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+
+    it "chef-solo --no-fork does not fork" do
+      command = shell_out("#{chef_solo} -c \"#{path_to('config/client.rb')}\" -o 'x::default' --no-fork", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+
+    it "chef-client with --fork uses a supervisor" do
+      command = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" -o 'x::default' --fork", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("WITHFORK")
+    end
+
+    it "chef-solo with --fork uses a supervisor" do
+      command = shell_out("#{chef_solo} -c \"#{path_to('config/client.rb')}\" -o 'x::default' --fork", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("WITHFORK")
+    end
+  end
+
+  when_the_repository "has a cookbook that knows if we're running forked, and configures forking in config.rb" do
+    before do
+      file "cookbooks/x/recipes/default.rb", <<~EOM
+        puts Chef::Config[:client_fork] ? "WITHFORK" : "NOFORK"
+EOM
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+client_fork true
+EOM
+    end
+
+    it "chef-client uses a supervisor" do
+      command = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("WITHFORK")
+    end
+
+    it "chef-solo uses a supervisor" do
+      command = shell_out("#{chef_solo} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("WITHFORK")
+    end
+  end
+
+  when_the_repository "has a cookbook that knows if we're running forked, and configures no-forking in config.rb" do
+    before do
+      file "cookbooks/x/recipes/default.rb", <<~EOM
+        puts Chef::Config[:client_fork] ? "WITHFORK" : "NOFORK"
+EOM
+      file "config/client.rb", <<EOM
+local_mode true
+cookbook_path "#{path_to('cookbooks')}"
+client_fork false
+EOM
+    end
+
+    it "chef-client uses a supervisor" do
+      command = shell_out("#{chef_client} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+
+    it "chef-solo uses a supervisor" do
+      command = shell_out("#{chef_solo} -c \"#{path_to('config/client.rb')}\" -o 'x::default'", :cwd => chef_dir)
+      command.error!
+      expect(command.stdout).to include("NOFORK")
+    end
+  end
 end
diff --git a/spec/unit/application/client_spec.rb b/spec/unit/application/client_spec.rb
index 3f803f2033..0cd3cb3995 100644
--- a/spec/unit/application/client_spec.rb
+++ b/spec/unit/application/client_spec.rb
@@ -107,6 +107,7 @@ describe Chef::Application::Client, "reconfigure" do
     shared_examples "sets the configuration" do |cli_arguments, expected_config|
       describe cli_arguments do
         before do
+          cli_arguments ||= ""
           ARGV.replace(cli_arguments.split)
           app.reconfigure
         end
@@ -144,6 +145,36 @@ describe Chef::Application::Client, "reconfigure" do
       end
     end
 
+    describe "--[no]-fork" do
+      before do
+        Chef::Config[:interval] = nil # FIXME: we're overriding the before block setting this
+      end
+
+      context "by default" do
+        it_behaves_like "sets the configuration", "", client_fork: false
+      end
+
+      context "with --fork" do
+        it_behaves_like "sets the configuration", "--fork", client_fork: true
+      end
+
+      context "with --no-fork" do
+        it_behaves_like "sets the configuration", "--no-fork", client_fork: false
+      end
+
+      context "with an interval" do
+        it_behaves_like "sets the configuration", "--interval 1800", client_fork: true
+      end
+
+      context "with once" do
+        it_behaves_like "sets the configuration", "--once", client_fork: false
+      end
+
+      context "with daemonize", :unix_only do
+        it_behaves_like "sets the configuration", "--daemonize", client_fork: true
+      end
+    end
+
     describe "--config-option" do
       context "with a single value" do
         it_behaves_like "sets the configuration", "--config-option chef_server_url=http://example",
@@ -186,21 +217,16 @@ describe Chef::Application::Client, "reconfigure" do
       Chef::Config[:splay] = nil
     end
 
-    context "when interval is given" do
-      before do
-        Chef::Config[:interval] = 600
-        allow(ChefConfig).to receive(:windows?).and_return(false)
-      end
-
-      it "should terminate with message" do
-        expect(Chef::Application).to receive(:fatal!).with(
-"Unforked chef-client interval runs are disabled in Chef 12.
+    it "should terminal with message when interval is given" do
+      Chef::Config[:interval] = 600
+      allow(ChefConfig).to receive(:windows?).and_return(false)
+      expect(Chef::Application).to 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
+      )
+      app.reconfigure
     end
 
     context "when interval is given on windows" do