Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Merge pull request #382 from masterzen/tickets/master/11044

#11044 - agent should do catalog run in a child process
  • Loading branch information...
commit fa4a1d9e932db5e28c2654dadbf8402357271150 2 parents 5e98876 + 6812ee9
@grimradical grimradical authored
View
42 lib/puppet/agent.rb
@@ -12,6 +12,7 @@ class Puppet::Agent
include Puppet::Agent::Disabler
attr_reader :client_class, :client, :splayed
+ attr_accessor :should_fork
# Just so we can specify that we are "the" instance.
def initialize(client_class)
@@ -41,14 +42,16 @@ def run(*args)
result = nil
block_run = Puppet::Application.controlled_run do
splay
- with_client do |client|
- begin
- sync.synchronize { lock { result = client.run(*args) } }
- rescue SystemExit,NoMemoryError
- raise
- rescue Exception => detail
- puts detail.backtrace if Puppet[:trace]
- Puppet.err "Could not run #{client_class}: #{detail}"
+ result = run_in_fork(should_fork) do
+ with_client do |client|
+ begin
+ sync.synchronize { lock { client.run(*args) } }
+ rescue SystemExit,NoMemoryError
+ raise
+ rescue Exception => detail
+ puts detail.backtrace if Puppet[:trace]
+ Puppet.err "Could not run #{client_class}: #{detail}"
+ end
end
end
true
@@ -93,6 +96,29 @@ def sync
@sync ||= Sync.new
end
+ def run_in_fork(forking = true)
+ return yield unless forking or Puppet.features.windows?
+
+ child_pid = Kernel.fork do
+ $0 = "puppet agent: applying configuration"
+ begin
+ exit(yield)
+ rescue SystemExit
+ exit(-1)
+ rescue NoMemoryError
+ exit(-2)
+ end
+ end
+ exit_code = Process.waitpid2(child_pid)
+ case exit_code[1].exitstatus
+ when -1
+ raise SystemExit
+ when -2
+ raise NoMemoryError
+ end
+ exit_code[1].exitstatus
+ end
+
private
# Create and yield a client instance, keeping a reference
View
3  lib/puppet/agent/locker.rb
@@ -12,9 +12,6 @@ def lock
ensure
lockfile.unlock
end
- return true
- else
- return false
end
end
View
7 lib/puppet/application/agent.rb
@@ -324,7 +324,8 @@ def onetime
@daemon.set_signal_traps
begin
- report = @agent.run
+ @agent.should_fork = false
+ exitstatus = @agent.run
rescue => detail
puts detail.backtrace if Puppet[:trace]
Puppet.err detail.to_s
@@ -332,10 +333,10 @@ def onetime
@daemon.stop(:exit => false)
- if not report
+ if not exitstatus
exit(1)
elsif options[:detailed_exitcodes] then
- exit(report.exit_status)
+ exit(exitstatus)
else
exit(0)
end
View
6 lib/puppet/application/apply.rb
@@ -226,12 +226,12 @@ def main
require 'puppet/configurer'
configurer = Puppet::Configurer.new
- report = configurer.run(:skip_plugin_download => true, :catalog => catalog)
+ exit_status = configurer.run(:skip_plugin_download => true, :catalog => catalog)
- if not report
+ if not exit_status
exit(1)
elsif options[:detailed_exitcodes] then
- exit(report.exit_status)
+ exit(exit_status)
else
exit(0)
end
View
6 lib/puppet/configurer.rb
@@ -146,7 +146,11 @@ def run(options = {})
begin
execute_prerun_command or return nil
- retrieve_and_apply_catalog(options, fact_options)
+ if retrieve_and_apply_catalog(options, fact_options)
+ report.exit_status
+ else
+ nil
+ end
rescue SystemExit,NoMemoryError
raise
rescue => detail
View
5 lib/puppet/daemon.rb
@@ -122,7 +122,10 @@ def start
raise Puppet::DevError, "Daemons must have an agent, server, or both" unless agent or server
server.start if server
- agent.start if agent
+ if agent
+ agent.should_fork = true
+ agent.start
+ end
EventLoop.current.run
end
View
8 spec/unit/agent/locker_spec.rb
@@ -39,16 +39,16 @@ class LockerTester
yielded.should be_true
end
- it "should return true when the lock method successfully locked" do
+ it "should return the block result when the lock method successfully locked" do
@locker.lockfile.expects(:lock).returns true
- @locker.lock {}.should be_true
+ @locker.lock { :result }.should == :result
end
- it "should return true when the lock method does not receive the lock" do
+ it "should return nil when the lock method does not receive the lock" do
@locker.lockfile.expects(:lock).returns false
- @locker.lock {}.should be_false
+ @locker.lock {}.should be_nil
end
it "should not yield when the lock method does not receive the lock" do
View
61 spec/unit/agent_spec.rb
@@ -158,6 +158,67 @@ def controlled_run(&block)
client.expects(:run).with("testargs")
@agent.run("testargs")
end
+
+ it "should return the agent result" do
+ client = AgentTestClient.new
+ AgentTestClient.expects(:new).returns client
+
+ @agent.expects(:lock).returns(:result)
+ @agent.run.should == :result
+ end
+
+ describe "when should_fork is true" do
+ before do
+ @agent.should_fork = true
+ Kernel.stubs(:fork)
+ Process.stubs(:waitpid2).returns [123, (stub 'process::status', :exitstatus => 0)]
+ @agent.stubs(:exit)
+ end
+
+ it "should run the agent in a forked process" do
+ client = AgentTestClient.new
+ AgentTestClient.expects(:new).returns client
+
+ client.expects(:run)
+
+ Kernel.expects(:fork).yields
+ @agent.run
+ end
+
+ it "should exit child process if child exit" do
+ client = AgentTestClient.new
+ AgentTestClient.expects(:new).returns client
+
+ client.expects(:run).raises(SystemExit)
+
+ Kernel.expects(:fork).yields
+ @agent.expects(:exit).with(-1)
+ @agent.run
+ end
+
+ it "should re-raise exit happening in the child" do
+ Process.stubs(:waitpid2).returns [123, (stub 'process::status', :exitstatus => -1)]
+ lambda { @agent.run }.should raise_error(SystemExit)
+ end
+
+ it "should re-raise NoMoreMemory happening in the child" do
+ Process.stubs(:waitpid2).returns [123, (stub 'process::status', :exitstatus => -2)]
+ lambda { @agent.run }.should raise_error(NoMemoryError)
+ end
+
+ it "should return the child exit code" do
+ Process.stubs(:waitpid2).returns [123, (stub 'process::status', :exitstatus => 777)]
+ @agent.run.should == 777
+ end
+
+ it "should return the block exit code as the child exit code" do
+ Kernel.expects(:fork).yields
+ @agent.expects(:exit).with(777)
+ @agent.run_in_fork {
+ 777
+ }
+ end
+ end
end
describe "when splaying" do
View
15 spec/unit/application/agent_spec.rb
@@ -507,6 +507,11 @@
expect { @puppetd.onetime }.to exit_with 0
end
+ it "should not let the agent fork" do
+ @agent.expects(:should_fork=).with(false)
+ expect { @puppetd.onetime }.to exit_with 0
+ end
+
it "should let the agent run" do
@agent.expects(:run).returns(:report)
expect { @puppetd.onetime }.to exit_with 0
@@ -526,18 +531,16 @@
@puppetd.options.stubs(:[]).with(:detailed_exitcodes).returns(true)
end
- it "should exit with report's computed exit status" do
+ it "should exit with agent computed exit status" do
Puppet[:noop] = false
- report = stub 'report', :exit_status => 666
- @agent.stubs(:run).returns(report)
+ @agent.stubs(:run).returns(666)
expect { @puppetd.onetime }.to exit_with 666
end
- it "should exit with the report's computer exit status, even if --noop is set." do
+ it "should exit with the agent's exit status, even if --noop is set." do
Puppet[:noop] = true
- report = stub 'report', :exit_status => 666
- @agent.stubs(:run).returns(report)
+ @agent.stubs(:run).returns(666)
expect { @puppetd.onetime }.to exit_with 666
end
View
10 spec/unit/configurer_spec.rb
@@ -111,7 +111,10 @@
Puppet[:node_name_fact] = 'my_name_fact'
@facts.values = {'my_name_fact' => 'node_name_from_fact'}
- @agent.run.host.should == 'node_name_from_fact'
+ report = Puppet::Transaction::Report.new("apply")
+
+ @agent.run(:report => report)
+ report.host.should == 'node_name_from_fact'
end
it "should pass the new report to the catalog" do
@@ -220,11 +223,12 @@
Puppet::Util::Log.destinations.should_not include(report)
end
- it "should return the report as the result of the run" do
+ it "should return the report exit_status as the result of the run" do
report = Puppet::Transaction::Report.new("apply")
Puppet::Transaction::Report.expects(:new).returns(report)
+ report.expects(:exit_status).returns(1234)
- @agent.run.should equal(report)
+ @agent.run.should == 1234
end
it "should send the transaction report even if the pre-run command fails" do
View
41 spec/unit/daemon_spec.rb
@@ -1,6 +1,7 @@
#!/usr/bin/env rspec
require 'spec_helper'
require 'puppet/daemon'
+require 'puppet/agent'
def without_warnings
flag = $VERBOSE
@@ -9,8 +10,15 @@ def without_warnings
$VERBOSE = flag
end
+class TestClient
+ def lockfile_path
+ "/dev/null"
+ end
+end
+
describe Puppet::Daemon do
before do
+ @agent = Puppet::Agent.new(TestClient.new)
@daemon = Puppet::Daemon.new
end
@@ -55,16 +63,22 @@ def without_warnings
end
it "should create its pidfile" do
- @daemon.stubs(:agent).returns stub('agent', :start => nil)
+ @daemon.agent = @agent
@daemon.expects(:create_pidfile)
@daemon.start
end
it "should start the agent if the agent is configured" do
- agent = mock 'agent'
- agent.expects(:start)
- @daemon.stubs(:agent).returns agent
+ @agent.expects(:start)
+ @daemon.agent = @agent
+
+ @daemon.start
+ end
+
+ it "should start the agent with should_fork at true" do
+ @agent.expects(:should_fork=).with(true)
+ @daemon.agent = @agent
@daemon.start
end
@@ -78,7 +92,7 @@ def without_warnings
end
it "should let the current EventLoop run" do
- @daemon.stubs(:agent).returns stub('agent', :start => nil)
+ @daemon.agent = @agent
EventLoop.current.expects(:run)
@daemon.start
@@ -213,20 +227,18 @@ def without_warnings
end
it "should do nothing if the agent is running" do
- agent = mock 'agent'
- agent.expects(:running?).returns true
+ @agent.expects(:running?).returns true
- @daemon.stubs(:agent).returns agent
+ @daemon.agent = @agent
@daemon.reload
end
it "should run the agent if one is available and it is not running" do
- agent = mock 'agent'
- agent.expects(:running?).returns false
- agent.expects :run
+ @agent.expects(:running?).returns false
+ @agent.expects :run
- @daemon.stubs(:agent).returns agent
+ @daemon.agent = @agent
@daemon.reload
end
@@ -254,9 +266,8 @@ def without_warnings
end
it "should reexec itself if the agent is not running" do
- agent = mock 'agent'
- agent.expects(:running?).returns false
- @daemon.stubs(:agent).returns agent
+ @agent.expects(:running?).returns false
+ @daemon.agent = @agent
@daemon.expects(:reexec)
@daemon.restart
Please sign in to comment.
Something went wrong with that request. Please try again.