7 files changed, 219 insertions, 158 deletions

diff --git a/spec/lib/gitlab/git/remote_repository_spec.rb b/spec/lib/gitlab/git/remote_repository_spec.rb
index 0506210887c..eb148cc3804 100644
--- a/spec/lib/gitlab/git/remote_repository_spec.rb
+++ b/spec/lib/gitlab/git/remote_repository_spec.rb
@@ -4,7 +4,7 @@ describe Gitlab::Git::RemoteRepository, seed_helper: true do
   let(:repository) { Gitlab::Git::Repository.new('default', TEST_REPO_PATH, '') }
   subject { described_class.new(repository) }
 
-  describe '#empty_repo?' do
+  describe '#empty?' do
     using RSpec::Parameterized::TableSyntax
 
     where(:repository, :result) do
@@ -13,7 +13,7 @@ describe Gitlab::Git::RemoteRepository, seed_helper: true do
     end
 
     with_them do
-      it { expect(subject.empty_repo?).to eq(result) }
+      it { expect(subject.empty?).to eq(result) }
     end
   end
 
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 08dd6ea80ff..f19b65a5f71 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -257,7 +257,7 @@ describe Gitlab::Git::Repository, seed_helper: true do
   end
 
   describe '#empty?' do
-    it { expect(repository.empty?).to be_falsey }
+    it { expect(repository).not_to be_empty }
   end
 
   describe '#ref_names' do
diff --git a/spec/lib/gitlab/git/storage/checker_spec.rb b/spec/lib/gitlab/git/storage/checker_spec.rb
new file mode 100644
index 00000000000..d74c3bcb04c
--- /dev/null
+++ b/spec/lib/gitlab/git/storage/checker_spec.rb
@@ -0,0 +1,132 @@
+require 'spec_helper'
+
+describe Gitlab::Git::Storage::Checker, :clean_gitlab_redis_shared_state do
+  let(:storage_name) { 'default' }
+  let(:hostname) { Gitlab::Environment.hostname }
+  let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
+
+  subject(:checker) { described_class.new(storage_name) }
+
+  def value_from_redis(name)
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.hmget(cache_key, name)
+    end.first
+  end
+
+  def set_in_redis(name, value)
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.hmset(cache_key, name, value)
+    end.first
+  end
+
+  describe '.check_all' do
+    it 'calls a check for each storage' do
+      fake_checker_default = double
+      fake_checker_broken = double
+      fake_logger = fake_logger
+
+      expect(described_class).to receive(:new).with('default', fake_logger) { fake_checker_default }
+      expect(described_class).to receive(:new).with('broken', fake_logger) { fake_checker_broken }
+      expect(fake_checker_default).to receive(:check_with_lease)
+      expect(fake_checker_broken).to receive(:check_with_lease)
+
+      described_class.check_all(fake_logger)
+    end
+
+    context 'with broken storage', :broken_storage do
+      it 'returns the results' do
+        expected_result = [
+          { storage: 'default', success: true },
+          { storage: 'broken', success: false }
+        ]
+
+        expect(described_class.check_all).to eq(expected_result)
+      end
+    end
+  end
+
+  describe '#initialize' do
+    it 'assigns the settings' do
+      expect(checker.hostname).to eq(hostname)
+      expect(checker.storage).to eq('default')
+      expect(checker.storage_path).to eq(TestEnv.repos_path)
+    end
+  end
+
+  describe '#check_with_lease' do
+    it 'only allows one check at a time' do
+      expect(checker).to receive(:check).once { sleep 1 }
+
+      thread = Thread.new { checker.check_with_lease }
+      checker.check_with_lease
+      thread.join
+    end
+
+    it 'returns a result hash' do
+      expect(checker.check_with_lease).to eq(storage: 'default', success: true)
+    end
+  end
+
+  describe '#check' do
+    it 'tracks that the storage was accessible' do
+      set_in_redis(:failure_count, 10)
+      set_in_redis(:last_failure, Time.now.to_f)
+
+      checker.check
+
+      expect(value_from_redis(:failure_count).to_i).to eq(0)
+      expect(value_from_redis(:last_failure)).to be_empty
+      expect(value_from_redis(:first_failure)).to be_empty
+    end
+
+    it 'calls the check with the correct arguments' do
+      stub_application_setting(circuitbreaker_storage_timeout: 30,
+                               circuitbreaker_access_retries: 3)
+
+      expect(Gitlab::Git::Storage::ForkedStorageCheck)
+        .to receive(:storage_available?).with(TestEnv.repos_path, 30, 3)
+              .and_call_original
+
+      checker.check
+    end
+
+    it 'returns `true`' do
+      expect(checker.check).to eq(true)
+    end
+
+    it 'maintains known storage keys' do
+      Timecop.freeze do
+        # Insert an old key to expire
+        old_entry = Time.now.to_i - 3.days.to_i
+        Gitlab::Git::Storage.redis.with do |redis|
+          redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, old_entry, 'to_be_removed')
+        end
+
+        checker.check
+
+        known_keys = Gitlab::Git::Storage.redis.with do |redis|
+          redis.zrange(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, -1)
+        end
+
+        expect(known_keys).to contain_exactly(cache_key)
+      end
+    end
+
+    context 'the storage is not available', :broken_storage do
+      let(:storage_name) { 'broken' }
+
+      it 'tracks that the storage was inaccessible' do
+        Timecop.freeze do
+          expect { checker.check }.to change { value_from_redis(:failure_count).to_i }.by(1)
+
+          expect(value_from_redis(:last_failure)).not_to be_empty
+          expect(value_from_redis(:first_failure)).not_to be_empty
+        end
+      end
+
+      it 'returns `false`' do
+        expect(checker.check).to eq(false)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index f34c9f09057..210b90bfba9 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -1,11 +1,18 @@
 require 'spec_helper'
 
-describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state: true, broken_storage: true do
+describe Gitlab::Git::Storage::CircuitBreaker, :broken_storage do
   let(:storage_name) { 'default' }
   let(:circuit_breaker) { described_class.new(storage_name, hostname) }
   let(:hostname) { Gitlab::Environment.hostname }
   let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
 
+  def set_in_redis(name, value)
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key)
+      redis.hmset(cache_key, name, value)
+    end.first
+  end
+
   before do
     # Override test-settings for the circuitbreaker with something more realistic
     # for these specs.
@@ -19,36 +26,7 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
                          )
   end
 
-  def value_from_redis(name)
-    Gitlab::Git::Storage.redis.with do |redis|
-      redis.hmget(cache_key, name)
-    end.first
-  end
-
-  def set_in_redis(name, value)
-    Gitlab::Git::Storage.redis.with do |redis|
-      redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key)
-      redis.hmset(cache_key, name, value)
-    end.first
-  end
-
-  describe '.reset_all!' do
-    it 'clears all entries form redis' do
-      set_in_redis(:failure_count, 10)
-
-      described_class.reset_all!
-
-      key_exists = Gitlab::Git::Storage.redis.with { |redis| redis.exists(cache_key) }
-
-      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
+  describe '.for_storage', :request_store do
     it 'only builds a single circuitbreaker per storage' do
       expect(described_class).to receive(:new).once.and_call_original
 
@@ -71,7 +49,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
     it 'assigns the settings' do
       expect(circuit_breaker.hostname).to eq(hostname)
       expect(circuit_breaker.storage).to eq('default')
-      expect(circuit_breaker.storage_path).to eq(TestEnv.repos_path)
     end
   end
 
@@ -91,9 +68,9 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       end
     end
 
-    describe '#failure_wait_time' do
+    describe '#check_interval' do
       it 'reads the value from settings' do
-        expect(circuit_breaker.failure_wait_time).to eq(1)
+        expect(circuit_breaker.check_interval).to eq(1)
       end
     end
 
@@ -114,12 +91,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
         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
@@ -134,19 +105,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       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
       expect { |b| circuit_breaker.perform(&b) }
         .to yield_control
@@ -170,54 +128,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
         .to raise_error(Rugged::OSError)
     end
 
-    it 'tracks that the storage was accessible' do
-      set_in_redis(:failure_count, 10)
-      set_in_redis(:last_failure, Time.now.to_f)
-
-      circuit_breaker.perform { '' }
-
-      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
-
-    it 'maintains known storage keys' do
-      Timecop.freeze do
-        # Insert an old key to expire
-        old_entry = Time.now.to_i - 3.days.to_i
-        Gitlab::Git::Storage.redis.with do |redis|
-          redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, old_entry, 'to_be_removed')
-        end
-
-        circuit_breaker.perform { '' }
-
-        known_keys = Gitlab::Git::Storage.redis.with do |redis|
-          redis.zrange(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, -1)
-        end
-
-        expect(known_keys).to contain_exactly(cache_key)
-      end
-    end
-
-    it 'only performs the accessibility check once' do
-      expect(Gitlab::Git::Storage::ForkedStorageCheck)
-        .to receive(:storage_available?).once.and_call_original
-
-      2.times { circuit_breaker.perform { '' } }
-    end
-
-    it 'calls the check with the correct arguments' do
-      stub_application_setting(circuitbreaker_storage_timeout: 30,
-                               circuitbreaker_access_retries: 3)
-
-      expect(Gitlab::Git::Storage::ForkedStorageCheck)
-        .to receive(:storage_available?).with(TestEnv.repos_path, 30, 3)
-              .and_call_original
-
-      circuit_breaker.perform { '' }
-    end
-
     context 'with the feature disabled' do
       before do
         stub_feature_flags(git_storage_circuit_breaker: false)
@@ -240,31 +150,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
         expect(result).to eq('hello')
       end
     end
-
-    context 'the storage is not available' do
-      let(:storage_name) { 'broken' }
-
-      it 'raises the correct exception' do
-        expect(circuit_breaker).to receive(:track_storage_inaccessible)
-
-        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
-
-      it 'tracks that the storage was inaccessible' do
-        Timecop.freeze do
-          expect { circuit_breaker.perform { '' } }.to raise_error(Gitlab::Git::Storage::Inaccessible)
-
-          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 '#circuit_broken?' do
@@ -283,32 +168,6 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
     end
   end
 
-  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(circuit_breaker.backing_off?).to be_truthy
-      end
-    end
-
-    context 'the `failure_wait_time` is set to 0' do
-      before do
-        stub_application_setting(circuitbreaker_failure_wait_time: 0)
-      end
-
-      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.backing_off?).to be_falsey
-        end
-      end
-    end
-  end
-
   describe '#last_failure' do
     it 'returns the last failure time' do
       time = Time.parse("2017-05-26 17:52:30")
diff --git a/spec/lib/gitlab/git/storage/failure_info_spec.rb b/spec/lib/gitlab/git/storage/failure_info_spec.rb
new file mode 100644
index 00000000000..bae88fdda86
--- /dev/null
+++ b/spec/lib/gitlab/git/storage/failure_info_spec.rb
@@ -0,0 +1,70 @@
+require 'spec_helper'
+
+describe Gitlab::Git::Storage::FailureInfo, :broken_storage do
+  let(:storage_name) { 'default' }
+  let(:hostname) { Gitlab::Environment.hostname }
+  let(:cache_key) { "storage_accessible:#{storage_name}:#{hostname}" }
+
+  def value_from_redis(name)
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.hmget(cache_key, name)
+    end.first
+  end
+
+  def set_in_redis(name, value)
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key)
+      redis.hmset(cache_key, name, value)
+    end.first
+  end
+
+  describe '.reset_all!' do
+    it 'clears all entries form redis' do
+      set_in_redis(:failure_count, 10)
+
+      described_class.reset_all!
+
+      key_exists = Gitlab::Git::Storage.redis.with { |redis| redis.exists(cache_key) }
+
+      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 '.load' do
+    it 'loads failure information for a storage on a host' do
+      first_failure = Time.parse("2017-11-14 17:52:30")
+      last_failure = Time.parse("2017-11-14 18:54:37")
+      failure_count = 11
+
+      set_in_redis(:first_failure, first_failure.to_i)
+      set_in_redis(:last_failure, last_failure.to_i)
+      set_in_redis(:failure_count, failure_count.to_i)
+
+      info = described_class.load(cache_key)
+
+      expect(info.first_failure).to eq(first_failure)
+      expect(info.last_failure).to eq(last_failure)
+      expect(info.failure_count).to eq(failure_count)
+    end
+  end
+
+  describe '#no_failures?' do
+    it 'is true when there are no failures' do
+      info = described_class.new(nil, nil, 0)
+
+      expect(info.no_failures?).to be_truthy
+    end
+
+    it 'is false when there are failures' do
+      info = described_class.new(Time.parse("2017-11-14 17:52:30"),
+                                 Time.parse("2017-11-14 18:54:37"),
+                                 20)
+
+      expect(info.no_failures?).to be_falsy
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/storage/health_spec.rb b/spec/lib/gitlab/git/storage/health_spec.rb
index d7a52a04fbb..bb670fc5d94 100644
--- a/spec/lib/gitlab/git/storage/health_spec.rb
+++ b/spec/lib/gitlab/git/storage/health_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Gitlab::Git::Storage::Health, clean_gitlab_redis_shared_state: true, broken_storage: true do
+describe Gitlab::Git::Storage::Health, broken_storage: true do
   let(:host1_key) { 'storage_accessible:broken:web01' }
   let(:host2_key) { 'storage_accessible:default:kiq01' }
 
diff --git a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb
index 5db37f55e03..93ad20011de 100644
--- a/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/null_circuit_breaker_spec.rb
@@ -27,7 +27,7 @@ describe Gitlab::Git::Storage::NullCircuitBreaker do
     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)) } }
+      it { expect(breaker.failure_info.no_failures?).to be_falsy }
     end
   end
 
@@ -49,7 +49,7 @@ describe Gitlab::Git::Storage::NullCircuitBreaker do
     end
 
     describe '#failure_info' do
-      it { expect(breaker.failure_info).to eq(Gitlab::Git::Storage::CircuitBreaker::FailureInfo.new(nil, 0)) }
+      it { expect(breaker.failure_info.no_failures?).to be_truthy }
     end
   end