Merge branch 'bvl-circuitbreaker-settings-to-avoid-failures' into 'master'

Circuitbreaker settings to avoid spec-failures

Closes #36324

See merge request !13519

config/gitlab.yml.example

@@ -649,6 +649,9 @@ test:
       default:
         path: tmp/tests/repositories/
         gitaly_address: unix:tmp/tests/gitaly/gitaly.socket
+        failure_count_threshold: 999999
+        failure_wait_time: 0
+        storage_timeout: 30
       broken:
         path: tmp/tests/non-existent-repositories
         gitaly_address: unix:tmp/tests/gitaly/gitaly.socket

config/initializers/6_validations.rb

@@ -37,12 +37,12 @@ def validate_storages_config
       storage_validation_error("#{name} is not a valid storage, because it has no `path` key. Refer to gitlab.yml.example for an updated example")
     end
 
-    %w(failure_count_threshold failure_wait_time failure_reset_time storage_timeout).each do |setting|
+    %w(failure_count_threshold failure_reset_time storage_timeout).each do |setting|
       # Falling back to the defaults is fine!
       next if repository_storage[setting].nil?
 
       unless repository_storage[setting].to_f > 0
-        storage_validation_error("#{setting}, for storage `#{name}` needs to be greater than 0")
+        storage_validation_error("`#{setting}` for storage `#{name}` needs to be greater than 0")
       end
     end
   end

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

@@ -1,9 +1,30 @@
 require 'spec_helper'
 
 describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do
-  let(:circuit_breaker) { described_class.new('default') }
+  let(:storage_name) { 'default' }
+  let(:circuit_breaker) { described_class.new(storage_name) }
   let(:hostname) { Gitlab::Environment.hostname }
-  let(:cache_key) { "storage_accessible:default:#{hostname}" }
+  let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
+
+  before do
+    # Override test-settings for the circuitbreaker with something more realistic
+    # for these specs.
+    stub_storage_settings('default' => {
+                            'path' => TestEnv.repos_path,
+                            'failure_count_threshold' => 10,
+                            'failure_wait_time' => 30,
+                            'failure_reset_time' => 1800,
+                            'storage_timeout' => 5
+                          },
+                          'broken' => {
+                            'path' => 'tmp/tests/non-existent-repositories',
+                            'failure_count_threshold' => 10,
+                            'failure_wait_time' => 30,
+                            'failure_reset_time' => 1800,
+                            'storage_timeout' => 5
+                          }
+                         )
+  end
 
   def value_from_redis(name)
     Gitlab::Git::Storage.redis.with do |redis|
@@ -96,14 +117,14 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
   end
 
   describe '#circuit_broken?' do
-    it 'is closed when there is no last failure' do
+    it 'is working when there is no last failure' do
       set_in_redis(:last_failure, nil)
       set_in_redis(:failure_count, 0)
 
       expect(circuit_breaker.circuit_broken?).to be_falsey
     end
 
-    it 'is open when there was a recent failure' do
+    it 'is broken when there was a recent failure' do
       Timecop.freeze do
         set_in_redis(:last_failure, 1.second.ago.to_f)
         set_in_redis(:failure_count, 1)
@@ -112,16 +133,34 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       end
     end
 
-    it 'is open when there are to many failures' do
+    it 'is broken when there are too many failures' do
       set_in_redis(:last_failure, 1.day.ago.to_f)
       set_in_redis(:failure_count, 200)
 
       expect(circuit_breaker.circuit_broken?).to be_truthy
     end
+
+    context 'the `failure_wait_time` is set to 0' do
+      before do
+        stub_storage_settings('default' => {
+                                'failure_wait_time' => 0,
+                                'path' => TestEnv.repos_path
+                              })
+      end
+
+      it 'is working even when there is a recent failure' do
+        Timecop.freeze do
+          set_in_redis(:last_failure, 0.seconds.ago.to_f)
+          set_in_redis(:failure_count, 1)
+
+          expect(circuit_breaker.circuit_broken?).to be_falsey
+        end
+      end
+    end
   end
 
   describe "storage_available?" do
-    context 'when the storage is available' do
+    context 'the storage is available' do
       it 'tracks that the storage was accessible an raises the error' do
         expect(circuit_breaker).to receive(:track_storage_accessible)
 
@@ -136,8 +175,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       end
     end
 
-    context 'when storage is not available' do
-      let(:circuit_breaker) { described_class.new('broken') }
+    context 'storage is not available' do
+      let(:storage_name) { 'broken' }
 
       it 'tracks that the storage was inaccessible' do
         expect(circuit_breaker).to receive(:track_storage_inaccessible)
@@ -158,8 +197,8 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       end
     end
 
-    context 'when the storage is not available' do
-      let(:circuit_breaker) { described_class.new('broken') }
+    context 'the storage is not available' do
+      let(:storage_name) { 'broken' }
 
       it 'raises an error' do
         expect(circuit_breaker).to receive(:track_storage_inaccessible)

spec/support/stub_configuration.rb

@@ -39,14 +39,17 @@ module StubConfiguration
   end
 
   def stub_storage_settings(messages)
+    # 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['failure_count_threshold'] ||= 10
       storage_settings['failure_wait_time'] ||= 30
       storage_settings['failure_reset_time'] ||= 1800
       storage_settings['storage_timeout'] ||= 5
     end
 
-    allow(Gitlab.config.repositories).to receive(:storages).and_return(messages)
+    allow(Gitlab.config.repositories).to receive(:storages).and_return(Settingslogic.new(messages))
   end
 
   private