Merge branch '36549-circuit-breaker-handles-missing-storages' into 'master'

Allow the git circuit breaker to correctly handle missing repository storages

Closes #36549

See merge request gitlab-org/gitlab-ce!14417

changelogs/unreleased/36549-circuit-breaker-handles-missing-storages.yml

+---
+title: Allow the git circuit breaker to correctly handle missing repository storages
+merge_request: 14417
+author:
+type: fixed

lib/gitlab/git/storage.rb

@@ -11,6 +11,7 @@ module Gitlab
       end
 
       CircuitOpen = Class.new(Inaccessible)
+      Misconfiguration = Class.new(Inaccessible)
 
       REDIS_KEY_PREFIX = 'storage_accessible:'.freeze
 

lib/gitlab/git/storage/circuit_breaker.rb

@@ -28,14 +28,26 @@ module Gitlab
         def self.for_storage(storage)
           cached_circuitbreakers = RequestStore.fetch(:circuitbreaker_cache) do
             Hash.new do |hash, storage_name|
-              hash[storage_name] = new(storage_name)
+              hash[storage_name] = build(storage_name)
             end
           end
 
           cached_circuitbreakers[storage]
         end
 
-        def initialize(storage, hostname = Gitlab::Environment.hostname)
+        def self.build(storage, hostname = Gitlab::Environment.hostname)
+          config = Gitlab.config.repositories.storages[storage]
+
+          if !config.present?
+            NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Storage '#{storage}' is not configured"))
+          elsif !config['path'].present?
+            NullCircuitBreaker.new(storage, hostname, error: Misconfiguration.new("Path for storage '#{storage}' is not configured"))
+          else
+            new(storage, hostname)
+          end
+        end
+
+        def initialize(storage, hostname)
           @storage = storage
           @hostname = hostname
 
@@ -64,6 +76,10 @@ module Gitlab
           recent_failure || too_many_failures
         end
 
+        def failure_info
+          @failure_info ||= get_failure_info
+        end
+
         # Memoizing the `storage_available` call means we only do it once per
         # request when the storage is available.
         #
@@ -121,10 +137,12 @@ module Gitlab
           end
         end
 
-        def failure_info
-          @failure_info ||= get_failure_info
+        def cache_key
+          @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
         end
 
+        private
+
         def get_failure_info
           last_failure, failure_count = Gitlab::Git::Storage.redis.with do |redis|
             redis.hmget(cache_key, :last_failure, :failure_count)
@@ -134,10 +152,6 @@ module Gitlab
 
           FailureInfo.new(last_failure, failure_count.to_i)
         end
-
-        def cache_key
-          @cache_key ||= "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}#{storage}:#{hostname}"
-        end
       end
     end
   end

lib/gitlab/git/storage/health.rb

@@ -78,7 +78,7 @@ module Gitlab
 
         def failing_circuit_breakers
           @failing_circuit_breakers ||= failing_on_hosts.map do |hostname|
-            CircuitBreaker.new(storage_name, hostname)
+            CircuitBreaker.build(storage_name, hostname)
           end
         end
 

lib/gitlab/git/storage/null_circuit_breaker.rb

+module Gitlab
+  module Git
+    module Storage
+      class NullCircuitBreaker
+        # These will have actual values
+        attr_reader :storage,
+                    :hostname
+
+        # These will always have nil values
+        attr_reader :storage_path,
+                    :failure_wait_time,
+                    :failure_reset_time,
+                    :storage_timeout
+
+        def initialize(storage, hostname, error: nil)
+          @storage = storage
+          @hostname = hostname
+          @error = error
+        end
+
+        def perform
+          @error ? raise(@error) : yield
+        end
+
+        def circuit_broken?
+          !!@error
+        end
+
+        def failure_count_threshold
+          1
+        end
+
+        def last_failure
+          circuit_broken? ? Time.now : nil
+        end
+
+        def failure_count
+          circuit_broken? ? 1 : 0
+        end
+
+        def failure_info
+          Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(last_failure, failure_count)
+        end
+      end
+    end
+  end
+end

lib/gitlab/health_checks/fs_shards_check.rb

@@ -125,7 +125,7 @@ module Gitlab
         end
 
         def storage_circuitbreaker_test(storage_name)
-          Gitlab::Git::Storage::CircuitBreaker.new(storage_name).perform { "OK" }
+          Gitlab::Git::Storage::CircuitBreaker.build(storage_name).perform { "OK" }
         rescue Gitlab::Git::Storage::Inaccessible
           nil
         end

spec/lib/gitlab/git/storage/circuit_breaker_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do
   let(:storage_name) { 'default' }
-  let(:circuit_breaker) { described_class.new(storage_name) }
+  let(:circuit_breaker) { described_class.new(storage_name, hostname) }
   let(:hostname) { Gitlab::Environment.hostname }
   let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
 
@@ -22,7 +22,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
                             'failure_wait_time' => 30,
                             'failure_reset_time' => 1800,
                             'storage_timeout' => 5
-                          }
+                          },
+                          'nopath' => { 'path' => nil }
                          )
   end
 
@@ -59,6 +60,14 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       expect(breaker).to be_a(described_class)
       expect(described_class.for_storage('default')).to eq(breaker)
     end
+
+    it 'returns a broken circuit breaker for an unknown storage' do
+      expect(described_class.for_storage('unknown').circuit_broken?).to be_truthy
+    end
+
+    it 'returns a broken circuit breaker when the path is not set' do
+      expect(described_class.for_storage('nopath').circuit_broken?).to be_truthy
+    end
   end
 
   describe '#initialize' do

spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb

+require 'spec_helper'
+
+describe Gitlab::Git::Storage::NullCircuitBreaker do
+  let(:storage) { 'default' }
+  let(:hostname) { 'localhost' }
+  let(:error) { nil }
+
+  subject(:breaker) { described_class.new(storage, hostname, error: error) }
+
+  context 'with an error' do
+    let(:error) { Gitlab::Git::Storage::Misconfiguration.new('error') }
+
+    describe '#perform' do
+      it { expect { breaker.perform { 'ok' } }.to raise_error(error) }
+    end
+
+    describe '#circuit_broken?' do
+      it { expect(breaker.circuit_broken?).to be_truthy }
+    end
+
+    describe '#last_failure' do
+      it { Timecop.freeze { expect(breaker.last_failure).to eq(Time.now) } }
+    end
+
+    describe '#failure_count' do
+      it { expect(breaker.failure_count).to eq(breaker.failure_count_threshold) }
+    end
+
+    describe '#failure_info' do
+      it { Timecop.freeze { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(Time.now, breaker.failure_count_threshold)) } }
+    end
+  end
+
+  context 'not broken' do
+    describe '#perform' do
+      it { expect(breaker.perform { 'ok' }).to eq('ok') }
+    end
+
+    describe '#circuit_broken?' do
+      it { expect(breaker.circuit_broken?).to be_falsy }
+    end
+
+    describe '#last_failure' do
+      it { expect(breaker.last_failure).to be_nil }
+    end
+
+    describe '#failure_count' do
+      it { expect(breaker.failure_count).to eq(0) }
+    end
+
+    describe '#failure_info' do
+      it { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(nil, 0)) }
+    end
+  end
+
+  describe '#failure_count_threshold' do
+    it { expect(breaker.failure_count_threshold).to eq(1) }
+  end
+
+  it 'implements the CircuitBreaker interface' do
+    ours = described_class.public_instance_methods
+    theirs = Gitlab::Git::Storage::CircuitBreaker.public_instance_methods
+
+    # These methods are not part of the public API, but are public to allow the
+    # CircuitBreaker specs to operate. They should be made private over time.
+    exceptions = %i[
+      cache_key
+      check_storage_accessible!
+      no_failures?
+      storage_available?
+      track_storage_accessible
+      track_storage_inaccessible
+    ]
+
+    expect(theirs - ours).to contain_exactly(*exceptions)
+  end
+end

spec/lib/gitlab/health_checks/fs_shards_check_spec.rb

@@ -21,7 +21,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
 
   let(:metric_class) { Gitlab::HealthChecks::Metric }
   let(:result_class) { Gitlab::HealthChecks::Result }
-  let(:repository_storages) { [:default] }
+  let(:repository_storages) { ['default'] }
   let(:tmp_dir) { Dir.mktmpdir }
 
   let(:storages_paths) do
@@ -64,7 +64,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           allow(described_class).to receive(:storage_circuitbreaker_test) { true }
         end
 
-        it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) }
+        it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) }
       end
 
       context 'storage points to directory that has both read and write rights' do
@@ -72,7 +72,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
           FileUtils.chmod_R(0755, tmp_dir)
         end
 
-        it { is_expected.to include(result_class.new(true, nil, shard: :default)) }
+        it { is_expected.to include(result_class.new(true, nil, shard: 'default')) }
 
         it 'cleans up files used for testing' do
           expect(described_class).to receive(:storage_write_test).with(any_args).and_call_original
@@ -85,7 +85,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
             allow(described_class).to receive(:storage_read_test).with(any_args).and_return(false)
           end
 
-          it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: :default)) }
+          it { is_expected.to include(result_class.new(false, 'cannot read from storage', shard: 'default')) }
         end
 
         context 'write test fails' do
@@ -93,7 +93,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
             allow(described_class).to receive(:storage_write_test).with(any_args).and_return(false)
           end
 
-          it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: :default)) }
+          it { is_expected.to include(result_class.new(false, 'cannot write to storage', shard: 'default')) }
         end
       end
     end
@@ -109,7 +109,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         it 'provides metrics' do
           metrics = described_class.metrics
 
-          expect(metrics).to all(have_attributes(labels: { shard: :default }))
+          expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))
@@ -128,7 +128,7 @@ describe Gitlab::HealthChecks::FsShardsCheck do
         it 'provides metrics' do
           metrics = described_class.metrics
 
-          expect(metrics).to all(have_attributes(labels: { shard: :default }))
+          expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 1))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 1))
           expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 1))
@@ -156,14 +156,14 @@ describe Gitlab::HealthChecks::FsShardsCheck do
     describe '#readiness' do
       subject { described_class.readiness }
 
-      it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: :default)) }
+      it { is_expected.to include(result_class.new(false, 'cannot stat storage', shard: 'default')) }
     end
 
     describe '#metrics' do
       it 'provides metrics' do
         metrics = described_class.metrics
 
-        expect(metrics).to all(have_attributes(labels: { shard: :default }))
+        expect(metrics).to all(have_attributes(labels: { shard: 'default' }))
         expect(metrics).to include(an_object_having_attributes(name: :filesystem_accessible, value: 0))
         expect(metrics).to include(an_object_having_attributes(name: :filesystem_readable, value: 0))
         expect(metrics).to include(an_object_having_attributes(name: :filesystem_writable, value: 0))

spec/support/stub_configuration.rb

@@ -42,7 +42,7 @@ module StubConfiguration
     # Default storage is always required
     messages['default'] ||= Gitlab.config.repositories.storages.default
     messages.each do |storage_name, storage_settings|
-      storage_settings['path'] ||= TestEnv.repos_path
+      storage_settings['path'] = TestEnv.repos_path unless storage_settings.key?('path')
       storage_settings['failure_count_threshold'] ||= 10
       storage_settings['failure_wait_time'] ||= 30
       storage_settings['failure_reset_time'] ||= 1800