Merge branch '29825-reactivecaching-sidekiq-queue-overloaded-teamcity' into 'master'

Return benign data if ReactiveCache fails to connect to TeamCity

See merge request gitlab-org/gitlab!27395

app/models/project_services/teamcity_service.rb

@@ -86,7 +86,11 @@ class TeamcityService < CiService
   def calculate_reactive_cache(sha, ref)
     response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,revision:#{sha}")
 
-    { build_page: read_build_page(response), commit_status: read_commit_status(response) }
+    if response
+      { build_page: read_build_page(response), commit_status: read_commit_status(response) }
+    else
+      { build_page: teamcity_url, commit_status: :error }
+    end
   end
 
   def execute(data)
@@ -149,7 +153,7 @@ class TeamcityService < CiService
   end
 
   def get_path(path)
-    Gitlab::HTTP.get(build_url(path), verify: false, basic_auth: basic_auth)
+    Gitlab::HTTP.try_get(build_url(path), verify: false, basic_auth: basic_auth, extra_log_info: { project_id: project_id })
   end
 
   def post_to_build_queue(data, branch)

changelogs/unreleased/29825-reactivecaching-sidekiq-queue-overloaded-teamcity.yml

+---
+title: Set commit status to failed if the TeamCity connection is refused
+merge_request: 27395
+author:
+type: fixed

lib/gitlab/http.rb

@@ -25,5 +25,17 @@ module Gitlab
     rescue HTTParty::RedirectionTooDeep
       raise RedirectionTooDeep
     end
+
+    def self.try_get(path, options = {}, &block)
+      log_info = options.delete(:extra_log_info)
+      self.get(path, options, &block)
+
+    rescue *HTTP_ERRORS => e
+      extra_info = log_info || {}
+      extra_info = log_info.call(e, path, options) if log_info.respond_to?(:call)
+
+      Gitlab::ErrorTracking.log_exception(e, extra_info)
+      nil
+    end
   end
 end

spec/lib/gitlab/http_spec.rb

@@ -100,4 +100,127 @@ describe Gitlab::HTTP do
       expect { described_class.head('http://example.org') }.to raise_error(Gitlab::HTTP::RedirectionTooDeep)
     end
   end
+
+  describe '.try_get' do
+    let(:path) { 'http://example.org' }
+
+    let(:extra_log_info_proc) do
+      proc do |error, url, options|
+        { klass: error.class, url: url, options: options }
+      end
+    end
+
+    let(:request_options) do
+      {
+        verify: false,
+        basic_auth: { username: 'user', password: 'pass' }
+      }
+    end
+
+    described_class::HTTP_ERRORS.each do |exception_class|
+      context "with #{exception_class}" do
+        let(:klass) { exception_class }
+
+        context 'with path' do
+          before do
+            expect(described_class).to receive(:get)
+              .with(path, {})
+              .and_raise(klass)
+          end
+
+          it 'handles requests without extra_log_info' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), {})
+
+            expect(described_class.try_get(path)).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as hash' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { a: :b })
+
+            expect(described_class.try_get(path, extra_log_info: { a: :b })).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as proc' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { url: path, klass: klass, options: {} })
+
+            expect(described_class.try_get(path, extra_log_info: extra_log_info_proc)).to be_nil
+          end
+        end
+
+        context 'with path and options' do
+          before do
+            expect(described_class).to receive(:get)
+              .with(path, request_options)
+              .and_raise(klass)
+          end
+
+          it 'handles requests without extra_log_info' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), {})
+
+            expect(described_class.try_get(path, request_options)).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as hash' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { a: :b })
+
+            expect(described_class.try_get(path, **request_options, extra_log_info: { a: :b })).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as proc' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { klass: klass, url: path, options: request_options })
+
+            expect(described_class.try_get(path, **request_options, extra_log_info: extra_log_info_proc)).to be_nil
+          end
+        end
+
+        context 'with path, options, and block' do
+          let(:block) do
+            proc {}
+          end
+
+          before do
+            expect(described_class).to receive(:get)
+              .with(path, request_options, &block)
+              .and_raise(klass)
+          end
+
+          it 'handles requests without extra_log_info' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), {})
+
+            expect(described_class.try_get(path, request_options, &block)).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as hash' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { a: :b })
+
+            expect(described_class.try_get(path, **request_options, extra_log_info: { a: :b }, &block)).to be_nil
+          end
+
+          it 'handles requests with extra_log_info as proc' do
+            expect(Gitlab::ErrorTracking)
+              .to receive(:log_exception)
+              .with(instance_of(klass), { klass: klass, url: path, options: request_options })
+
+            expect(described_class.try_get(path, **request_options, extra_log_info: extra_log_info_proc, &block)).to be_nil
+          end
+        end
+      end
+    end
+  end
 end

spec/models/project_services/teamcity_service_spec.rb

@@ -7,6 +7,7 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
   include StubRequests
 
   let(:teamcity_url) { 'http://gitlab.com/teamcity' }
+  let(:teamcity_full_url) { 'http://gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,revision:123' }
   let(:project) { create(:project) }
 
   subject(:service) do
@@ -165,6 +166,16 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
           is_expected.to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo')
         end
       end
+
+      it 'returns the teamcity_url when teamcity is unreachable' do
+        stub_full_request(teamcity_full_url).to_raise(Errno::ECONNREFUSED)
+
+        expect(Gitlab::ErrorTracking)
+          .to receive(:log_exception)
+          .with(instance_of(Errno::ECONNREFUSED), project_id: project.id)
+
+        is_expected.to eq(teamcity_url)
+      end
     end
 
     context 'commit_status' do
@@ -205,6 +216,16 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
 
         is_expected.to eq(:error)
       end
+
+      it 'sets commit status to :error when teamcity is unreachable' do
+        stub_full_request(teamcity_full_url).to_raise(Errno::ECONNREFUSED)
+
+        expect(Gitlab::ErrorTracking)
+          .to receive(:log_exception)
+          .with(instance_of(Errno::ECONNREFUSED), project_id: project.id)
+
+        is_expected.to eq(:error)
+      end
     end
   end
 
@@ -300,7 +321,6 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
   end
 
   def stub_request(status: 200, body: nil, build_status: 'success')
-    teamcity_full_url = 'http://gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,revision:123'
     auth = %w(mic password)
 
     body ||= %Q({"build":{"status":"#{build_status}","id":"666"}})