Please review pull request #382: #11044 - agent should do catalog run in a child process opened by (masterzen)
Description:
The current best practice among Puppet users is to run the puppet agent out of cron with —onetime. The rationale is that the puppet agent is a memory hog during a run, but due to the way the ruby interpreter works this memory can never be returned to the OS between runs. Running on cron makes sure puppet doesn’t hold this memory for more than the run.
A simple solution to this issue is to fork into a child process and do the catalog run there.
This process would exit at the end of the catalog execution, thus freeing the memory used during the run.
This is a new pull request of the code rebased to master.
This supersedes pull request #234.
- Opened: Sat Jan 21 22:47:59 UTC 2012
- Based on: puppetlabs:master (5e98876f5c91e7019ef9ea0842e30bdcfe8098b2)
- Requested merge: masterzen:tickets/master/11044 (6812ee9fc51f327f3a74fc6a6c0eefd9758d0134)
Diff follows:
diff --git a/lib/puppet/agent.rb b/lib/puppet/agent.rb
index b684470..d517293 100644
--- a/lib/puppet/agent.rb
+++ b/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
diff --git a/lib/puppet/agent/locker.rb b/lib/puppet/agent/locker.rb
index d41b125..42eb6fa 100644
--- a/lib/puppet/agent/locker.rb
+++ b/lib/puppet/agent/locker.rb
@@ -12,9 +12,6 @@ def lock
ensure
lockfile.unlock
end
- return true
- else
- return false
end
end
diff --git a/lib/puppet/application/agent.rb b/lib/puppet/application/agent.rb
index 74eb222..465b268 100644
--- a/lib/puppet/application/agent.rb
+++ b/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
diff --git a/lib/puppet/application/apply.rb b/lib/puppet/application/apply.rb
index eb57ce5..588248c 100644
--- a/lib/puppet/application/apply.rb
+++ b/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
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 1157eda..f314fa3 100644
--- a/lib/puppet/configurer.rb
+++ b/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
diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 22630ff..03c21a2 100755
--- a/lib/puppet/daemon.rb
+++ b/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
diff --git a/spec/unit/agent/locker_spec.rb b/spec/unit/agent/locker_spec.rb
index 9b530c0..d649be7 100755
--- a/spec/unit/agent/locker_spec.rb
+++ b/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
diff --git a/spec/unit/agent_spec.rb b/spec/unit/agent_spec.rb
index 9e2840c..48cc760 100755
--- a/spec/unit/agent_spec.rb
+++ b/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
diff --git a/spec/unit/application/agent_spec.rb b/spec/unit/application/agent_spec.rb
index f512ba4..e10f10a 100755
--- a/spec/unit/application/agent_spec.rb
+++ b/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
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
index 5ea2e4f..694b52c 100755
--- a/spec/unit/configurer_spec.rb
+++ b/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
diff --git a/spec/unit/daemon_spec.rb b/spec/unit/daemon_spec.rb
index fc43d93..da79537 100755
--- a/spec/unit/daemon_spec.rb
+++ b/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
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
