[Pkg-puppet-devel] [SCM] Puppet packaging for Debian branch, master, updated. debian/0.24.7-1-98-gf19c0e5
James Turnbull
james at lovedthanlost.net
Wed Apr 8 21:48:08 UTC 2009
The following commit has been merged in the master branch:
commit af3f3ae93ba1e43ac8231e4cf4e5a1f0ed8f3acb
Author: Luke Kanies <luke at madstop.com>
Date: Wed Feb 11 16:49:40 2009 -0600
Refactoring the XMLRPC::Client error-handling
I split it all into smaller, manageable chunks,
and used methods for each step, instead
of having one huge call.
Note that I made all of the tests first, then
refactored the code, so I'm confident there's no
behavior change.
I don't know that this is actually a lot cleaner,
but it seems that way to me. I'm open to
skipping this, but I think it makes the whole thing
a lot cleaner.
Signed-off-by: Luke Kanies <luke at madstop.com>
diff --git a/lib/puppet/network/xmlrpc/client.rb b/lib/puppet/network/xmlrpc/client.rb
index 37ace21..91bb03e 100644
--- a/lib/puppet/network/xmlrpc/client.rb
+++ b/lib/puppet/network/xmlrpc/client.rb
@@ -36,57 +36,7 @@ module Puppet::Network
interface.methods.each { |ary|
method = ary[0]
newclient.send(:define_method,method) { |*args|
- Puppet.debug "Calling %s.%s" % [namespace, method]
- begin
- call("%s.%s" % [namespace, method.to_s],*args)
- rescue OpenSSL::SSL::SSLError => detail
- if detail.message =~ /bad write retry/
- Puppet.warning "Transient SSL write error; restarting connection and retrying"
- self.recycle_connection
- retry
- end
- ["certificate verify failed", "hostname was not match", "hostname not match"].each do |str|
- if detail.message.include?(str)
- Puppet.warning "Certificate validation failed; consider using the certname configuration option"
- end
- end
- raise XMLRPCClientError,
- "Certificates were not trusted: %s" % detail
- rescue ::XMLRPC::FaultException => detail
- raise XMLRPCClientError, detail.faultString
- rescue Errno::ECONNREFUSED => detail
- msg = "Could not connect to %s on port %s" %
- [@host, @port]
- raise XMLRPCClientError, msg
- rescue SocketError => detail
- Puppet.err "Could not find server %s: %s" %
- [@host, detail.to_s]
- error = XMLRPCClientError.new(
- "Could not find server %s" % @host
- )
- error.set_backtrace detail.backtrace
- raise error
- rescue Errno::EPIPE, EOFError
- Puppet.warning "Other end went away; restarting connection and retrying"
- self.recycle_connection
- retry
- rescue Timeout::Error => detail
- Puppet.err "Connection timeout calling %s.%s: %s" %
- [namespace, method, detail.to_s]
- error = XMLRPCClientError.new("Connection Timeout")
- error.set_backtrace(detail.backtrace)
- raise error
- rescue => detail
- if detail.message =~ /^Wrong size\. Was \d+, should be \d+$/
- Puppet.warning "XMLRPC returned wrong size. Retrying."
- retry
- end
- Puppet.err "Could not call %s.%s: %s" %
- [namespace, method, detail.inspect]
- error = XMLRPCClientError.new(detail.to_s)
- error.set_backtrace detail.backtrace
- raise error
- end
+ make_rpc_call(namespace, method, *args)
}
}
@@ -97,13 +47,117 @@ module Puppet::Network
@clients[handler] || self.mkclient(handler)
end
+ class ErrorHandler
+ def initialize(&block)
+ metaclass.define_method(:execute, &block)
+ end
+ end
+
+ # Use a class variable so all subclasses have access to it.
+ @@error_handlers = {}
+
+ def self.error_handler(exception)
+ if handler = @@error_handlers[exception.class]
+ return handler
+ else
+ return @@error_handlers[:default]
+ end
+ end
+
+ def self.handle_error(*exceptions, &block)
+ handler = ErrorHandler.new(&block)
+
+ exceptions.each do |exception|
+ @@error_handlers[exception] = handler
+ end
+ end
+
+ handle_error(OpenSSL::SSL::SSLError) do |client, detail, namespace, method|
+ if detail.message =~ /bad write retry/
+ Puppet.warning "Transient SSL write error; restarting connection and retrying"
+ client.recycle_connection
+ return :retry
+ end
+ ["certificate verify failed", "hostname was not match", "hostname not match"].each do |str|
+ if detail.message.include?(str)
+ Puppet.warning "Certificate validation failed; consider using the certname configuration option"
+ end
+ end
+ raise XMLRPCClientError, "Certificates were not trusted: %s" % detail
+ end
+
+ handle_error(:default) do |client, detail, namespace, method|
+ if detail.message.to_s =~ /^Wrong size\. Was \d+, should be \d+$/
+ Puppet.warning "XMLRPC returned wrong size. Retrying."
+ return :retry
+ end
+ Puppet.err "Could not call %s.%s: %s" % [namespace, method, detail.inspect]
+ error = XMLRPCClientError.new(detail.to_s)
+ error.set_backtrace detail.backtrace
+ raise error
+ end
+
+ handle_error(OpenSSL::SSL::SSLError) do |client, detail, namespace, method|
+ if detail.message =~ /bad write retry/
+ Puppet.warning "Transient SSL write error; restarting connection and retrying"
+ client.recycle_connection
+ return :retry
+ end
+ ["certificate verify failed", "hostname was not match", "hostname not match"].each do |str|
+ if detail.message.include?(str)
+ Puppet.warning "Certificate validation failed; consider using the certname configuration option"
+ end
+ end
+ raise XMLRPCClientError, "Certificates were not trusted: %s" % detail
+ end
+
+ handle_error(::XMLRPC::FaultException) do |client, detail, namespace, method|
+ raise XMLRPCClientError, detail.faultString
+ end
+
+ handle_error(Errno::ECONNREFUSED) do |client, detail, namespace, method|
+ msg = "Could not connect to %s on port %s" % [client.host, client.port]
+ raise XMLRPCClientError, msg
+ end
+
+ handle_error(SocketError) do |client, detail, namespace, method|
+ Puppet.err "Could not find server %s: %s" % [@host, detail.to_s]
+ error = XMLRPCClientError.new("Could not find server %s" % client.host)
+ error.set_backtrace detail.backtrace
+ raise error
+ end
+
+ handle_error(Errno::EPIPE, EOFError) do |client, detail, namespace, method|
+ Puppet.warning "Other end went away; restarting connection and retrying"
+ client.recycle_connection
+ return :retry
+ end
+
+ handle_error(Timeout::Error) do |client, detail, namespace, method|
+ Puppet.err "Connection timeout calling %s.%s: %s" % [namespace, method, detail.to_s]
+ error = XMLRPCClientError.new("Connection Timeout")
+ error.set_backtrace(detail.backtrace)
+ raise error
+ end
+
+ def make_rpc_call(namespace, method, *args)
+ Puppet.debug "Calling %s.%s" % [namespace, method]
+ begin
+ call("%s.%s" % [namespace, method.to_s],*args)
+ rescue Exception => detail
+ retry if self.class.error_handler(detail).execute(self, detail, namespace, method) == :retry
+ end
+ end
+
def http
unless @http
- @http = Puppet::Network::HttpPool.http_instance(@host, @port, true)
+ @http = Puppet::Network::HttpPool.http_instance(host, port, true)
end
@http
end
+ attr_reader :host, :port
+
def initialize(hash = {})
hash[:Path] ||= "/RPC2"
hash[:Server] ||= Puppet[:server]
@@ -135,7 +189,11 @@ module Puppet::Network
# or we've just downloaded certs and we need to create new http instances
# with the certs added.
def recycle_connection
- @http = Puppet::Network::HttpPool.http_instance(@host, @port, true) # reset the instance
+ if http.started?
+ http.finish
+ end
+ @http = nil
+ self.http # force a new one
end
def start
diff --git a/spec/unit/network/xmlrpc/client.rb b/spec/unit/network/xmlrpc/client.rb
index a0a2e77..76ef5c7 100755
--- a/spec/unit/network/xmlrpc/client.rb
+++ b/spec/unit/network/xmlrpc/client.rb
@@ -2,12 +2,148 @@
Dir.chdir(File.dirname(__FILE__)) { (s = lambda { |f| File.exist?(f) ? require(f) : Dir.chdir("..") { s.call(f) } }).call("spec/spec_helper.rb") }
-describe Puppet::Network do
- it "should raise an XMLRPCClientError if a generated class raises a Timeout::Error" do
- http = mock 'http'
- Puppet::Network::HttpPool.stubs(:http_instance).returns http
- file = Puppet::Network::Client.file.new({:Server => "foo.com"})
- http.stubs(:post2).raises Timeout::Error
- lambda { file.retrieve }.should raise_error(Puppet::Network::XMLRPCClientError)
+describe Puppet::Network::XMLRPCClient do
+ describe "when performing the rpc call" do
+ before do
+ @client = Puppet::Network::Client.report.xmlrpc_client.new
+ @client.stubs(:call).returns "foo"
+
+ end
+
+ it "should call the specified namespace and method, with the specified arguments" do
+ @client.expects(:call).with("puppetreports.report", "eh").returns "foo"
+ @client.report("eh")
+ end
+
+ it "should return the results from the call" do
+ @client.expects(:call).returns "foo"
+ @client.report("eh").should == "foo"
+ end
+
+ describe "when returning the http instance" do
+ it "should use the http pool to create the instance" do
+ @client.instance_variable_set("@http", nil)
+ @client.expects(:host).returns "myhost"
+ @client.expects(:port).returns "myport"
+ Puppet::Network::HttpPool.expects(:http_instance).with("myhost", "myport", true).returns "http"
+
+ @client.http.should == "http"
+ end
+
+ it "should reuse existing instances" do
+ @client.http.should equal(@client.http)
+ end
+ end
+
+ describe "when recycling the connection" do
+ it "should close the existing instance if it's open" do
+ http = mock 'http'
+ @client.stubs(:http).returns http
+
+ http.expects(:started?).returns true
+ http.expects(:finish)
+
+ @client.recycle_connection
+ end
+
+ it "should force creation of a new instance" do
+ Puppet::Network::HttpPool.expects(:http_instance).returns "second_http"
+
+ @client.recycle_connection
+
+ @client.http.should == "second_http"
+ end
+ end
+
+ describe "and an exception is raised" do
+ it "should raise XMLRPCClientError if XMLRPC::FaultException is raised" do
+ error = XMLRPC::FaultException.new("foo", "bar")
+
+ @client.expects(:call).raises(error)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should raise XMLRPCClientError if Errno::ECONNREFUSED is raised" do
+ @client.expects(:call).raises(Errno::ECONNREFUSED)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if Timeout::Error is raised" do
+ Puppet.expects(:err)
+ @client.expects(:call).raises(Timeout::Error)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if SocketError is raised" do
+ Puppet.expects(:err)
+ @client.expects(:call).raises(SocketError)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log, recycle the connection, and retry if Errno::EPIPE is raised" do
+ @client.expects(:call).times(2).raises(Errno::EPIPE).then.returns "eh"
+
+ Puppet.expects(:warning)
+ @client.expects(:recycle_connection)
+
+ @client.report("eh")
+ end
+
+ it "should log, recycle the connection, and retry if EOFError is raised" do
+ @client.expects(:call).times(2).raises(EOFError).then.returns "eh"
+
+ Puppet.expects(:warning)
+ @client.expects(:recycle_connection)
+
+ @client.report("eh")
+ end
+
+ it "should log and retry if an exception containing 'Wrong size' is raised" do
+ error = RuntimeError.new("Wrong size. Was 15, should be 30")
+ @client.expects(:call).times(2).raises(error).then.returns "eh"
+
+ Puppet.expects(:warning)
+
+ @client.report("eh")
+ end
+
+ it "should raise XMLRPCClientError if OpenSSL::SSL::SSLError is raised" do
+ @client.expects(:call).raises(OpenSSL::SSL::SSLError)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log and raise XMLRPCClientError if OpenSSL::SSL::SSLError is raised with certificate issues" do
+ error = OpenSSL::SSL::SSLError.new("hostname was not match")
+ @client.expects(:call).raises(error)
+
+ Puppet.expects(:warning)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+
+ it "should log, recycle the connection, and retry if OpenSSL::SSL::SSLError is raised containing 'bad write retry'" do
+ error = OpenSSL::SSL::SSLError.new("bad write retry")
+ @client.expects(:call).times(2).raises(error).then.returns "eh"
+
+ @client.expects(:recycle_connection)
+
+ Puppet.expects(:warning)
+
+ @client.report("eh")
+ end
+
+ it "should log and raise XMLRPCClientError if any other exception is raised" do
+ @client.expects(:call).raises(RuntimeError)
+
+ Puppet.expects(:err)
+
+ lambda { @client.report("eh") }.should raise_error(Puppet::Network::XMLRPCClientError)
+ end
+ end
end
end
--
Puppet packaging for Debian
More information about the Pkg-puppet-devel
mailing list