Permalink
Browse files

Merge remote branch 'origin/teal_13_03_acu78361_patch_attachment_down…

…load'

Conflicts:
	lib/instance/cook/repose_downloader.rb
	spec/instance/instance/cook/repose_downloader_spec.rb
  • Loading branch information...
ryanwilliamson committed Mar 6, 2013
2 parents b593719 + dde256c commit 14ddd7459e36d1331837e62abfa11773db8f148a
Showing with 88 additions and 32 deletions.
  1. +49 −22 lib/instance/cook/repose_downloader.rb
  2. +39 −10 spec/instance/instance/cook/repose_downloader_spec.rb
@@ -34,6 +34,12 @@ class ReposeDownloader
CONNECTION_EXCEPTIONS = ['Errno::ECONNREFUSED', 'Errno::ETIMEDOUT', 'SocketError',
'RestClient::InternalServerError', 'RestClient::RequestTimeout']
+ # max timeout 8 (2**3) minutes for each retry
+ RETRY_BACKOFF_MAX = 3
+
+ # retry 5 times maximum
+ RETRY_MAX_ATTEMPTS = 5
+
class ConnectionException < Exception; end
class DownloadException < Exception; end
@@ -62,7 +68,7 @@ class DownloadException < Exception; end
#
# === Return
# @return [Downloader]
-
+ #
def initialize(hostnames)
raise ArgumentError, "At least one hostname must be provided" if hostnames.empty?
hostnames = [hostnames] unless hostnames.respond_to?(:each)
@@ -84,21 +90,27 @@ def initialize(hostnames)
# === Block
# @yield [] A block is mandatory
# @yieldreturn [String] The stream that is being fetched
-
+ #
def download(resource)
client = get_http_client
@size = 0
@speed = 0
@sanitized_resource = sanitize_resource(resource)
resource = parse_resource(resource)
+ attempts = 0
begin
balancer.request do |endpoint|
RightSupport::Net::SSL.with_expected_hostname(ips[endpoint]) do
logger.info("Requesting '#{sanitized_resource}' from '#{endpoint}'")
+ attempts += 1
t0 = Time.now
- client.get("https://#{endpoint}:443#{resource}", {:verify_ssl => OpenSSL::SSL::VERIFY_PEER, :ssl_ca_file => get_ca_file, :headers => {:user_agent => "RightLink v#{AgentConfig.protocol_version}"}}) do |response, request, result|
+
+ # Previously we accessed RestClient directly and used it's wrapper method to instantiate
+ # a RestClient::Request object. This wrapper was not passing all options down the stack
+ # so now we invoke the RestClient::Request object directly, passing it our desired options
+ client.execute(:method => :get, :url => "https://#{endpoint}:443#{resource}", :timeout => calculate_timeout(attempts), :verify_ssl => OpenSSL::SSL::VERIFY_PEER, :ssl_ca_file => get_ca_file, :headers => {:user_agent => "RightLink v#{AgentConfig.protocol_version}"}) do |response, request, result|
if result.kind_of?(Net::HTTPSuccess)
@size = result.content_length
@speed = @size / (Time.now - t0)
@@ -122,7 +134,7 @@ def download(resource)
#
# === Return
# @return [String] Message with last downloaded resource, download size and speed
-
+ #
def details
"Downloaded '#{@sanitized_resource}' (#{ scale(size.to_i).join(' ') }) at #{ scale(speed.to_i).join(' ') }/s"
end
@@ -141,7 +153,7 @@ def details
# === Return
# @return [Hash]
# * :key [<String>] a key (IP Address) that accepts a hostname string as it's value
-
+ #
def resolve(hostnames)
ips = {}
hostnames.each do |hostname|
@@ -172,7 +184,7 @@ def resolve(hostnames)
#
# === Block
# @return [String] The parsed URI
-
+ #
def parse_resource(resource)
resource = URI::parse(resource)
raise ArgumentError, "Invalid resource provided. Resource must be a fully qualified URL" unless resource
@@ -192,7 +204,6 @@ def parse_resource(resource)
# @return [Array] List of exception class names
def parse_exception_message(e)
- result = nil
if e.kind_of?(RightSupport::Net::NoResult)
# Expected format of exception message: "... endpoints: ('<ip address>' => <exception class name array>, ...)""
i = 0
@@ -212,23 +223,38 @@ def parse_exception_message(e)
#
# === Return
# @return [RightSupport::Net::RequestBalancer]
-
+ #
def balancer
@balancer ||= RightSupport::Net::RequestBalancer.new(
- ips.keys,
- :policy => RightSupport::Net::LB::Sticky,
- :fatal => lambda do |e|
- if RightSupport::Net::RequestBalancer::DEFAULT_FATAL_EXCEPTIONS.any? { |c| e.is_a?(c) }
- true
- elsif e.respond_to?(:http_code) && (e.http_code != nil)
- (e.http_code >= 400 && e.http_code < 500) && (e.http_code != 408 && e.http_code != 500 )
- else
- false
- end
+ ips.keys,
+ :policy => RightSupport::Net::LB::Sticky,
+ :retry => RETRY_MAX_ATTEMPTS,
+ :fatal => lambda do |e|
+ if RightSupport::Net::RequestBalancer::DEFAULT_FATAL_EXCEPTIONS.any? { |c| e.is_a?(c) }
+ true
+ elsif e.respond_to?(:http_code) && (e.http_code != nil)
+ (e.http_code >= 400 && e.http_code < 500) && (e.http_code != 408 && e.http_code != 500 )
+ else
+ false
end
+ end
)
end
+ # Exponential incremental timeout algorithm. Returns the amount of
+ # of time to wait for the next iteration
+ #
+ # === Parameters
+ # @param [String] Number of attempts
+ #
+ # === Return
+ # @return [Integer] Timeout to use for next iteration
+ #
+ def calculate_timeout(attempts)
+ timeout_exponent = [attempts, RETRY_BACKOFF_MAX].min
+ (2 ** timeout_exponent) * 60
+ end
+
# Returns a path to a CA file
#
# The CA bundle is a basically static collection of trusted certs of top-level CAs.
@@ -237,7 +263,7 @@ def balancer
#
# === Return
# @return [String] Path to a CA file
-
+ #
def get_ca_file
ca_file = File.normalize_path(File.join(File.dirname(__FILE__), 'ca-bundle.crt'))
end
@@ -249,10 +275,11 @@ def get_ca_file
#
# === Return
# @return [RestClient]
-
+ #
def get_http_client
RestClient.proxy = @proxy.to_s if @proxy
RestClient
+ RestClient::Request
end
# Return a sanitized value from given argument
@@ -265,7 +292,7 @@ def get_http_client
#
# === Return
# @return [String] 'Resource' portion of resource provided
-
+ #
def sanitize_resource(resource)
URI::split(resource)[5].split("/").last
end
@@ -280,7 +307,7 @@ def sanitize_resource(resource)
#
# === Return
# @return <[Integer], [String]> First element is scaled value, second element is scale
-
+ #
def scale(value)
case value
when 0..1023
@@ -28,7 +28,7 @@ def mock_response(message, code)
net_http_res = flexmock("Net HTTP Response")
net_http_res.should_receive(:code).and_return(code)
response = RestClient::Response.create("#{code}: #{message}", net_http_res, [])
- flexmock(RestClient).should_receive(:get).and_yield(response, nil, res)
+ flexmock(RestClient::Request).should_receive(:execute).and_yield(response, nil, res)
end
module RightScale
@@ -48,13 +48,14 @@ module RightScale
shared_examples_for 'ConnectionException' do
it 'should fail to download after retrying if a ConnectionException is raised' do
if exception
- flexmock(RestClient).should_receive(:get).and_raise(exception)
+ flexmock(RestClient::Request).should_receive(:execute).and_raise(exception)
else
mock_response(message, code)
end
- flexmock(ReposeDownloader.logger).should_receive(:info).twice
- flexmock(ReposeDownloader.logger).should_receive(:error).times(4)
+ flexmock(ReposeDownloader.logger).should_receive(:info).times(ReposeDownloader::RETRY_MAX_ATTEMPTS)
+ flexmock(ReposeDownloader.logger).should_receive(:error).times(ReposeDownloader::RETRY_MAX_ATTEMPTS + 2)
+
lambda { subject.download(attachment) { |response| response } }.should raise_error(RightScale::ReposeDownloader::ConnectionException)
end
end
@@ -65,6 +66,7 @@ module RightScale
flexmock(ReposeDownloader.logger).should_receive(:info).once
flexmock(ReposeDownloader.logger).should_receive(:error).twice
+
lambda { subject.download(attachment) { |response| response } }.should raise_error(RightScale::ReposeDownloader::DownloadException)
end
end
@@ -102,9 +104,12 @@ module RightScale
context :download do
it 'should download an attachment' do
res = Net::HTTPSuccess.new('1.1', 'bar', 200)
- flexmock(RestClient).should_receive(:get).and_yield('bar', nil, res)
+ flexmock(RestClient::Request).should_receive(:execute).and_yield('bar', nil, res)
flexmock(res).should_receive(:content_length).and_return(0)
+ # Speed up this test
+ flexmock(subject).should_receive(:calculate_timeout).and_return(0)
+
flexmock(ReposeDownloader.logger).should_receive(:info).once
flexmock(ReposeDownloader.logger).should_receive(:error).never
@@ -181,8 +186,32 @@ module RightScale
end
context :balancer do
+ let(:balancer) { subject.send(:balancer) }
+
it 'should return a RequestBalancer' do
- subject.send(:balancer).should be_a(RightSupport::Net::RequestBalancer)
+ balancer.should be_a(RightSupport::Net::RequestBalancer)
+ end
+
+ it 'should retry 5 times' do
+ balancer.inspect
+ balancer.instance_eval('@options[:retry]').should == 5
+ end
+ end
+
+ context :calculate_timeout do
+ it 'should return a doubly increasing timeout' do
+ previous_timeout = 60
+
+ (1..3).each do |i|
+ timeout = subject.send(:calculate_timeout, i)
+ timeout.should == previous_timeout * 2
+
+ previous_timeout = timeout
+ end
+ end
+
+ it 'should never return a timeout greater than RETRY_BACKOFF_MAX' do
+ subject.send(:calculate_timeout, ReposeDownloader::RETRY_MAX_ATTEMPTS).should == (2**ReposeDownloader::RETRY_BACKOFF_MAX) * 60
end
end
@@ -197,15 +226,15 @@ module RightScale
it 'should return an instance of RestClient with no proxy if one is not specified' do
client = subject.send(:get_http_client)
- client.should == RestClient
- client.proxy.should be_nil
+ client.should == RestClient::Request
+ RestClient.proxy.should be_nil
end
it 'should return an instance of RestClient with a proxy if one is specified' do
ENV['HTTPS_PROXY'] = proxy
client = subject.send(:get_http_client)
- client.should == RestClient
- client.proxy.should == proxy
+ client.should == RestClient::Request
+ RestClient.proxy.should == proxy
end
end

1 comment on commit 14ddd74

Member

rightscale-ci commented on 14ddd74 Apr 1, 2013

RS-COMPLIANCE: COMPLIANCE-COMMITMESSAGE-PASS Build: #15135(2013-04-01_06-20-45), repo: right_link, branch/tag: aqua_acu78536_productize, sha: 14ddd74, status: PASS, reason: Was a merge commit

Please sign in to comment.