1 files changed, 143 insertions, 172 deletions

diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index 98cf7966dad..72dabca793a 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -10,18 +10,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
     # 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
+                            'path' => TestEnv.repos_path
                           },
                           'broken' => {
-                            'path' => 'tmp/tests/non-existent-repositories',
-                            'failure_count_threshold' => 10,
-                            'failure_wait_time' => 30,
-                            'failure_reset_time' => 1800,
-                            'storage_timeout' => 5
+                            'path' => 'tmp/tests/non-existent-repositories'
                           },
                           'nopath' => { 'path' => nil }
                          )
@@ -49,6 +41,10 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
 
       expect(key_exists).to be_falsey
     end
+
+    it 'does not break when there are no keys in redis' do
+      expect { described_class.reset_all! }.not_to raise_error
+    end
   end
 
   describe '.for_storage' do
@@ -75,19 +71,79 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       expect(circuit_breaker.hostname).to eq(hostname)
       expect(circuit_breaker.storage).to eq('default')
       expect(circuit_breaker.storage_path).to eq(TestEnv.repos_path)
-      expect(circuit_breaker.failure_count_threshold).to eq(10)
-      expect(circuit_breaker.failure_wait_time).to eq(30)
-      expect(circuit_breaker.failure_reset_time).to eq(1800)
-      expect(circuit_breaker.storage_timeout).to eq(5)
+    end
+  end
+
+  context 'circuitbreaker settings' do
+    before do
+      stub_application_setting(circuitbreaker_failure_count_threshold: 0,
+                               circuitbreaker_failure_wait_time: 1,
+                               circuitbreaker_failure_reset_time: 2,
+                               circuitbreaker_storage_timeout: 3,
+                               circuitbreaker_access_retries: 4,
+                               circuitbreaker_backoff_threshold: 5)
+    end
+
+    describe '#failure_count_threshold' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.failure_count_threshold).to eq(0)
+      end
+    end
+
+    describe '#failure_wait_time' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.failure_wait_time).to eq(1)
+      end
+    end
+
+    describe '#failure_reset_time' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.failure_reset_time).to eq(2)
+      end
+    end
+
+    describe '#storage_timeout' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.storage_timeout).to eq(3)
+      end
+    end
+
+    describe '#access_retries' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.access_retries).to eq(4)
+      end
+    end
+
+    describe '#backoff_threshold' do
+      it 'reads the value from settings' do
+        expect(circuit_breaker.backoff_threshold).to eq(5)
+      end
     end
   end
 
   describe '#perform' do
-    it 'raises an exception with retry time when the circuit is open' do
-      allow(circuit_breaker).to receive(:circuit_broken?).and_return(true)
+    it 'raises the correct exception when the circuit is open' do
+      set_in_redis(:last_failure, 1.day.ago.to_f)
+      set_in_redis(:failure_count, 999)
 
       expect { |b| circuit_breaker.perform(&b) }
-        .to raise_error(Gitlab::Git::Storage::CircuitOpen)
+        .to raise_error do |exception|
+        expect(exception).to be_kind_of(Gitlab::Git::Storage::CircuitOpen)
+        expect(exception.retry_after).to eq(1800)
+      end
+    end
+
+    it 'raises the correct exception when backing off' do
+      Timecop.freeze do
+        set_in_redis(:last_failure, 1.second.ago.to_f)
+        set_in_redis(:failure_count, 90)
+
+        expect { |b| circuit_breaker.perform(&b) }
+          .to raise_error do |exception|
+          expect(exception).to be_kind_of(Gitlab::Git::Storage::Failing)
+          expect(exception.retry_after).to eq(30)
+        end
+      end
     end
 
     it 'yields the block' do
@@ -97,6 +153,7 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
 
     it 'checks if the storage is available' do
       expect(circuit_breaker).to receive(:check_storage_accessible!)
+                                   .and_call_original
 
       circuit_breaker.perform { 'hello world' }
     end
@@ -112,204 +169,124 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
         .to raise_error(Rugged::OSError)
     end
 
-    context 'with the feature disabled' do
-      it 'returns the block without checking accessibility' do
-        stub_feature_flags(git_storage_circuit_breaker: false)
-
-        expect(circuit_breaker).not_to receive(:circuit_broken?)
+    it 'tracks that the storage was accessible' do
+      set_in_redis(:failure_count, 10)
+      set_in_redis(:last_failure, Time.now.to_f)
 
-        result = circuit_breaker.perform { 'hello' }
+      circuit_breaker.perform { '' }
 
-        expect(result).to eq('hello')
-      end
+      expect(value_from_redis(:failure_count).to_i).to eq(0)
+      expect(value_from_redis(:last_failure)).to be_empty
+      expect(circuit_breaker.failure_count).to eq(0)
+      expect(circuit_breaker.last_failure).to be_nil
     end
-  end
 
-  describe '#circuit_broken?' do
-    it 'is working when there is no last failure' do
-      set_in_redis(:last_failure, nil)
-      set_in_redis(:failure_count, 0)
+    it 'only performs the accessibility check once' do
+      expect(Gitlab::Git::Storage::ForkedStorageCheck)
+        .to receive(:storage_available?).once.and_call_original
 
-      expect(circuit_breaker.circuit_broken?).to be_falsey
+      2.times { circuit_breaker.perform { '' } }
     end
 
-    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)
-
-        expect(circuit_breaker.circuit_broken?).to be_truthy
-      end
-    end
+    it 'calls the check with the correct arguments' do
+      stub_application_setting(circuitbreaker_storage_timeout: 30,
+                               circuitbreaker_access_retries: 3)
 
-    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(Gitlab::Git::Storage::ForkedStorageCheck)
+        .to receive(:storage_available?).with(TestEnv.repos_path, 30, 3)
+              .and_call_original
 
-      expect(circuit_breaker.circuit_broken?).to be_truthy
+      circuit_breaker.perform { '' }
     end
 
-    context 'the `failure_wait_time` is set to 0' do
+    context 'with the feature disabled' 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
+        stub_feature_flags(git_storage_circuit_breaker: false)
       end
-    end
-  end
-
-  describe "storage_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)
 
-        circuit_breaker.storage_available?
-      end
+      it 'returns the block without checking accessibility' do
+        expect(circuit_breaker).not_to receive(:check_storage_accessible!)
 
-      it 'only performs the check once' do
-        expect(Gitlab::Git::Storage::ForkedStorageCheck)
-          .to receive(:storage_available?).once.and_call_original
+        result = circuit_breaker.perform { 'hello' }
 
-        2.times { circuit_breaker.storage_available? }
+        expect(result).to eq('hello')
       end
-    end
-
-    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)
+      it 'allows enabling the feature using an ENV var' do
+        stub_env('GIT_STORAGE_CIRCUIT_BREAKER', 'true')
+        expect(circuit_breaker).to receive(:check_storage_accessible!)
 
-        circuit_breaker.storage_available?
-      end
-    end
-  end
-
-  describe '#check_storage_accessible!' do
-    it 'raises an exception with retry time when the circuit is open' do
-      allow(circuit_breaker).to receive(:circuit_broken?).and_return(true)
+        result = circuit_breaker.perform { 'hello' }
 
-      expect { circuit_breaker.check_storage_accessible! }
-        .to raise_error do |exception|
-        expect(exception).to be_kind_of(Gitlab::Git::Storage::CircuitOpen)
-        expect(exception.retry_after).to eq(30)
+        expect(result).to eq('hello')
       end
     end
 
     context 'the storage is not available' do
       let(:storage_name) { 'broken' }
 
-      it 'raises an error' do
+      it 'raises the correct exception' do
         expect(circuit_breaker).to receive(:track_storage_inaccessible)
 
-        expect { circuit_breaker.check_storage_accessible! }
+        expect { circuit_breaker.perform { '' } }
           .to raise_error do |exception|
           expect(exception).to be_kind_of(Gitlab::Git::Storage::Inaccessible)
           expect(exception.retry_after).to eq(30)
         end
       end
-    end
-  end
-
-  describe '#track_storage_inaccessible' do
-    around do |example|
-      Timecop.freeze { example.run }
-    end
 
-    it 'records the failure time in redis' do
-      circuit_breaker.track_storage_inaccessible
-
-      failure_time = value_from_redis(:last_failure)
-
-      expect(Time.at(failure_time.to_i)).to be_within(1.second).of(Time.now)
-    end
-
-    it 'sets the failure time on the breaker without reloading' do
-      circuit_breaker.track_storage_inaccessible
-
-      expect(circuit_breaker).not_to receive(:get_failure_info)
-      expect(circuit_breaker.last_failure).to eq(Time.now)
-    end
-
-    it 'increments the failure count in redis' do
-      set_in_redis(:failure_count, 10)
-
-      circuit_breaker.track_storage_inaccessible
-
-      expect(value_from_redis(:failure_count).to_i).to be(11)
-    end
-
-    it 'increments the failure count on the breaker without reloading' do
-      set_in_redis(:failure_count, 10)
-
-      circuit_breaker.track_storage_inaccessible
+      it 'tracks that the storage was inaccessible' do
+        Timecop.freeze do
+          expect { circuit_breaker.perform { '' } }.to raise_error(Gitlab::Git::Storage::Inaccessible)
 
-      expect(circuit_breaker).not_to receive(:get_failure_info)
-      expect(circuit_breaker.failure_count).to eq(11)
+          expect(value_from_redis(:failure_count).to_i).to eq(1)
+          expect(value_from_redis(:last_failure)).not_to be_empty
+          expect(circuit_breaker.failure_count).to eq(1)
+          expect(circuit_breaker.last_failure).to be_within(1.second).of(Time.now)
+        end
+      end
     end
   end
 
-  describe '#track_storage_accessible' do
-    it 'sets the failure count to zero in redis' do
-      set_in_redis(:failure_count, 10)
-
-      circuit_breaker.track_storage_accessible
-
-      expect(value_from_redis(:failure_count).to_i).to be(0)
-    end
-
-    it 'sets the failure count to zero on the breaker without reloading' do
-      set_in_redis(:failure_count, 10)
-
-      circuit_breaker.track_storage_accessible
+  describe '#circuit_broken?' 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).not_to receive(:get_failure_info)
-      expect(circuit_breaker.failure_count).to eq(0)
+      expect(circuit_breaker.circuit_broken?).to be_falsey
     end
 
-    it 'removes the last failure time from redis' do
-      set_in_redis(:last_failure, Time.now.to_i)
-
-      circuit_breaker.track_storage_accessible
+    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).not_to receive(:get_failure_info)
-      expect(circuit_breaker.last_failure).to be_nil
+      expect(circuit_breaker.circuit_broken?).to be_truthy
     end
+  end
 
-    it 'removes the last failure time from the breaker without reloading' do
-      set_in_redis(:last_failure, Time.now.to_i)
-
-      circuit_breaker.track_storage_accessible
+  describe '#backing_off?' do
+    it 'is true 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, 90)
 
-      expect(value_from_redis(:last_failure)).to be_empty
+        expect(circuit_breaker.backing_off?).to be_truthy
+      end
     end
 
-    it 'wont connect to redis when there are no failures' do
-      expect(Gitlab::Git::Storage.redis).to receive(:with).once
-                                              .and_call_original
-      expect(circuit_breaker).to receive(:track_storage_accessible)
-                           .and_call_original
-
-      circuit_breaker.track_storage_accessible
-    end
-  end
+    context 'the `failure_wait_time` is set to 0' do
+      before do
+        stub_application_setting(circuitbreaker_failure_wait_time: 0)
+      end
 
-  describe '#no_failures?' do
-    it 'is false when a failure was tracked' do
-      set_in_redis(:last_failure, Time.now.to_i)
-      set_in_redis(:failure_count, 1)
+      it 'is working even when there are failures' do
+        Timecop.freeze do
+          set_in_redis(:last_failure, 0.seconds.ago.to_f)
+          set_in_redis(:failure_count, 90)
 
-      expect(circuit_breaker.no_failures?).to be_falsey
+          expect(circuit_breaker.backing_off?).to be_falsey
+        end
+      end
     end
   end
 
@@ -329,10 +306,4 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       expect(circuit_breaker.failure_count).to eq(7)
     end
   end
-
-  describe '#cache_key' do
-    it 'includes storage and host' do
-      expect(circuit_breaker.cache_key).to eq(cache_key)
-    end
-  end
 end