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

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

2 files changed, 137 insertions, 13 deletions

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