Move the circuitbreaker check out in a separate processbvl-circuitbreaker-process

Moving the check out of the general requests, makes sure we don't have
any slowdown in the regular requests.

To keep the process performing this checks small, the check is still
performed inside a unicorn. But that is called from a process running
on the same server.

Because the checks are now done outside normal request, we can have a
simpler failure strategy:

The check is now performed in the background every
`circuitbreaker_check_interval`. Failures are logged in redis. The
failures are reset when the check succeeds. Per check we will try
`circuitbreaker_access_retries` times within
`circuitbreaker_storage_timeout` seconds.

When the number of failures exceeds
`circuitbreaker_failure_count_threshold`, we will block access to the
storage.

After `failure_reset_time` of no checks, we will clear the stored
failures. This could happen when the process that performs the checks
is not running.

20 files changed, 460 insertions, 196 deletions

diff --git a/spec/bin/storage_check_spec.rb b/spec/bin/storage_check_spec.rb
new file mode 100644
index 00000000000..02f6fcb6e3a
--- /dev/null
+++ b/spec/bin/storage_check_spec.rb
@@ -0,0 +1,13 @@
+require 'spec_helper'
+
+describe 'bin/storage_check' do
+  it 'is executable' do
+    command = %w[bin/storage_check -t unix://the/path/to/a/unix-socket.sock -i 10 -d]
+    expected_output = 'Checking unix://the/path/to/a/unix-socket.sock every 10 seconds'
+
+    output, status = Gitlab::Popen.popen(command, Rails.root.to_s)
+
+    expect(status).to eq(0)
+    expect(output).to include(expected_output)
+  end
+end
diff --git a/spec/controllers/admin/health_check_controller_spec.rb b/spec/controllers/admin/health_check_controller_spec.rb
index 0b8e0c8a065..d15ee0021d9 100644
--- a/spec/controllers/admin/health_check_controller_spec.rb
+++ b/spec/controllers/admin/health_check_controller_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Admin::HealthCheckController, broken_storage: true do
+describe Admin::HealthCheckController do
   let(:admin) { create(:admin) }
 
   before do
@@ -17,7 +17,7 @@ describe Admin::HealthCheckController, broken_storage: true do
 
   describe 'POST reset_storage_health' do
     it 'resets all storage health information' do
-      expect(Gitlab::Git::Storage::CircuitBreaker).to receive(:reset_all!)
+      expect(Gitlab::Git::Storage::FailureInfo).to receive(:reset_all!)
 
       post :reset_storage_health
     end
diff --git a/spec/controllers/health_controller_spec.rb b/spec/controllers/health_controller_spec.rb
index 9e9cf4f2c1f..95946def5f9 100644
--- a/spec/controllers/health_controller_spec.rb
+++ b/spec/controllers/health_controller_spec.rb
@@ -14,6 +14,48 @@ describe HealthController do
     stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'false')
   end
 
+  describe '#storage_check' do
+    before do
+      allow(Gitlab::RequestContext).to receive(:client_ip).and_return(whitelisted_ip)
+    end
+
+    subject { post :storage_check }
+
+    it 'checks all the configured storages' do
+      expect(Gitlab::Git::Storage::Checker).to receive(:check_all).and_call_original
+
+      subject
+    end
+
+    it 'returns the check interval' do
+      stub_env('IN_MEMORY_APPLICATION_SETTINGS', 'true')
+      stub_application_setting(circuitbreaker_check_interval: 10)
+
+      subject
+
+      expect(json_response['check_interval']).to eq(10)
+    end
+
+    context 'with failing storages', :broken_storage do
+      before do
+        stub_storage_settings(
+          broken: { path: 'tmp/tests/non-existent-repositories' }
+        )
+      end
+
+      it 'includes the failure information' do
+        subject
+
+        expected_results = [
+          { 'storage' => 'broken', 'success' => false },
+          { 'storage' => 'default', 'success' => true }
+        ]
+
+        expect(json_response['results']).to eq(expected_results)
+      end
+    end
+  end
+
   describe '#readiness' do
     shared_context 'endpoint responding with readiness data' do
       let(:request_params) { {} }
diff --git a/spec/features/admin/admin_health_check_spec.rb b/spec/features/admin/admin_health_check_spec.rb
index 4430fc15501..ac3392b49f9 100644
--- a/spec/features/admin/admin_health_check_spec.rb
+++ b/spec/features/admin/admin_health_check_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-feature "Admin Health Check", :feature, :broken_storage do
+feature "Admin Health Check", :feature do
   include StubENV
 
   before do
@@ -36,6 +36,7 @@ feature "Admin Health Check", :feature, :broken_storage do
 
   context 'when services are up' do
     before do
+      stub_storage_settings({}) # Hide the broken storage
       visit admin_health_check_path
     end
 
@@ -56,10 +57,8 @@ feature "Admin Health Check", :feature, :broken_storage do
     end
   end
 
-  context 'with repository storage failures' do
+  context 'with repository storage failures', :broken_storage do
     before do
-      # Track a failure
-      Gitlab::Git::Storage::CircuitBreaker.for_storage('broken').perform { nil } rescue nil
       visit admin_health_check_path
     end
 
@@ -67,9 +66,10 @@ feature "Admin Health Check", :feature, :broken_storage do
       hostname = Gitlab::Environment.hostname
       maximum_failures = Gitlab::CurrentSettings.current_application_settings
                            .circuitbreaker_failure_count_threshold
+      number_of_failures = maximum_failures + 1
 
-      expect(page).to have_content('broken: failed storage access attempt on host:')
-      expect(page).to have_content("#{hostname}: 1 of #{maximum_failures} failures.")
+      expect(page).to have_content("broken: #{number_of_failures} failed storage access attempts:")
+      expect(page).to have_content("#{hostname}: #{number_of_failures} of #{maximum_failures} failures.")
     end
 
     it 'allows resetting storage failures' 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
 
diff --git a/spec/lib/gitlab/storage_check/cli_spec.rb b/spec/lib/gitlab/storage_check/cli_spec.rb
new file mode 100644
index 00000000000..6db0925899c
--- /dev/null
+++ b/spec/lib/gitlab/storage_check/cli_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Gitlab::StorageCheck::CLI do
+  let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', nil, 1, false) }
+  subject(:runner) { described_class.new(options) }
+
+  describe '#update_settings' do
+    it 'updates the interval when changed in a valid response and logs the change' do
+      fake_response = double
+      expect(fake_response).to receive(:valid?).and_return(true)
+      expect(fake_response).to receive(:check_interval).and_return(42)
+      expect(runner.logger).to receive(:info)
+
+      runner.update_settings(fake_response)
+
+      expect(options.interval).to eq(42)
+    end
+  end
+end
diff --git a/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb b/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb
new file mode 100644
index 00000000000..d869022fd31
--- /dev/null
+++ b/spec/lib/gitlab/storage_check/gitlab_caller_spec.rb
@@ -0,0 +1,46 @@
+require 'spec_helper'
+
+describe Gitlab::StorageCheck::GitlabCaller do
+  let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', nil, nil, false) }
+  subject(:gitlab_caller) { described_class.new(options) }
+
+  describe '#call!' do
+    context 'when a socket is given' do
+      it 'calls a socket' do
+        fake_connection = double
+        expect(fake_connection).to receive(:post)
+        expect(Excon).to receive(:new).with('unix://tmp/socket.sock', socket: "tmp/socket.sock") { fake_connection }
+
+        gitlab_caller.call!
+      end
+    end
+
+    context 'when a host is given' do
+      let(:options) { Gitlab::StorageCheck::Options.new('http://localhost:8080', nil, nil, false) }
+
+      it 'it calls a http response' do
+        fake_connection = double
+        expect(Excon).to receive(:new).with('http://localhost:8080', socket: nil) { fake_connection }
+        expect(fake_connection).to receive(:post)
+
+        gitlab_caller.call!
+      end
+    end
+  end
+
+  describe '#headers' do
+    it 'Adds the JSON header' do
+      headers = gitlab_caller.headers
+
+      expect(headers['Content-Type']).to eq('application/json')
+    end
+
+    context 'when a token was provided' do
+      let(:options) { Gitlab::StorageCheck::Options.new('unix://tmp/socket.sock', 'atoken', nil, false) }
+
+      it 'adds it to the headers' do
+        expect(gitlab_caller.headers['TOKEN']).to eq('atoken')
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/storage_check/option_parser_spec.rb b/spec/lib/gitlab/storage_check/option_parser_spec.rb
new file mode 100644
index 00000000000..cad4dfbefcf
--- /dev/null
+++ b/spec/lib/gitlab/storage_check/option_parser_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Gitlab::StorageCheck::OptionParser do
+  describe '.parse!' do
+    it 'assigns all options' do
+      args = %w(--target unix://tmp/hello/world.sock --token thetoken --interval 42)
+
+      options = described_class.parse!(args)
+
+      expect(options.token).to eq('thetoken')
+      expect(options.interval).to eq(42)
+      expect(options.target).to eq('unix://tmp/hello/world.sock')
+    end
+
+    it 'requires the interval to be a number' do
+      args = %w(--target unix://tmp/hello/world.sock --interval fortytwo)
+
+      expect { described_class.parse!(args) }.to raise_error(OptionParser::InvalidArgument)
+    end
+
+    it 'raises an error if the scheme is not included' do
+      args = %w(--target tmp/hello/world.sock)
+
+      expect { described_class.parse!(args) }.to raise_error(OptionParser::InvalidArgument)
+    end
+
+    it 'raises an error if both socket and host are missing' do
+      expect { described_class.parse!([]) }.to raise_error(OptionParser::InvalidArgument)
+    end
+  end
+end
diff --git a/spec/lib/gitlab/storage_check/response_spec.rb b/spec/lib/gitlab/storage_check/response_spec.rb
new file mode 100644
index 00000000000..0ff2963e443
--- /dev/null
+++ b/spec/lib/gitlab/storage_check/response_spec.rb
@@ -0,0 +1,54 @@
+require 'spec_helper'
+
+describe Gitlab::StorageCheck::Response do
+  let(:fake_json) do
+    {
+      check_interval: 42,
+      results: [
+        { storage: 'working', success: true },
+        { storage: 'skipped', success: nil },
+        { storage: 'failing', success: false }
+      ]
+    }.to_json
+  end
+
+  let(:fake_http_response) do
+    fake_response = instance_double("Excon::Response - Status check")
+    allow(fake_response).to receive(:status).and_return(200)
+    allow(fake_response).to receive(:body).and_return(fake_json)
+    allow(fake_response).to receive(:headers).and_return('Content-Type' => 'application/json')
+
+    fake_response
+  end
+  let(:response) { described_class.new(fake_http_response) }
+
+  describe '#valid?' do
+    it 'is valid for a success response with parseable JSON' do
+      expect(response).to be_valid
+    end
+  end
+
+  describe '#check_interval' do
+    it 'returns the result from the JSON' do
+      expect(response.check_interval).to eq(42)
+    end
+  end
+
+  describe '#responsive_shards' do
+    it 'contains the names of working shards' do
+      expect(response.responsive_shards).to contain_exactly('working')
+    end
+  end
+
+  describe '#skipped_shards' do
+    it 'contains the names of skipped shards' do
+      expect(response.skipped_shards).to contain_exactly('skipped')
+    end
+  end
+
+  describe '#failing_shards' do
+    it 'contains the name of failing shards' do
+      expect(response.failing_shards).to contain_exactly('failing')
+    end
+  end
+end
diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb
index 0b7e16cc33c..ef480e7a80a 100644
--- a/spec/models/application_setting_spec.rb
+++ b/spec/models/application_setting_spec.rb
@@ -115,9 +115,8 @@ describe ApplicationSetting do
     end
 
     context 'circuitbreaker settings' do
-      [:circuitbreaker_backoff_threshold,
-       :circuitbreaker_failure_count_threshold,
-       :circuitbreaker_failure_wait_time,
+      [:circuitbreaker_failure_count_threshold,
+       :circuitbreaker_check_interval,
        :circuitbreaker_failure_reset_time,
        :circuitbreaker_storage_timeout].each do |field|
         it "Validates #{field} as number" do
@@ -126,16 +125,6 @@ describe ApplicationSetting do
                            .is_greater_than_or_equal_to(0)
         end
       end
-
-      it 'requires the `backoff_threshold` to be lower than the `failure_count_threshold`' do
-        setting.circuitbreaker_failure_count_threshold = 10
-        setting.circuitbreaker_backoff_threshold = 15
-        failure_message = "The circuitbreaker backoff threshold should be lower "\
-                          "than the failure count threshold"
-
-        expect(setting).not_to be_valid
-        expect(setting.errors[:circuitbreaker_backoff_threshold]).to include(failure_message)
-      end
     end
 
     context 'repository storages' do
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 82ed1ecee33..358bc3dfb94 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -29,7 +29,9 @@ describe Repository do
   def expect_to_raise_storage_error
     expect { yield }.to raise_error do |exception|
       storage_exceptions = [Gitlab::Git::Storage::Inaccessible, Gitlab::Git::CommandError, GRPC::Unavailable]
-      expect(exception.class).to be_in(storage_exceptions)
+      known_exception = storage_exceptions.select { |e| exception.is_a?(e) }
+
+      expect(known_exception).not_to be_nil
     end
   end
 
@@ -634,9 +636,7 @@ describe Repository do
   end
 
   describe '#fetch_ref' do
-    # Setting the var here, sidesteps the stub that makes gitaly raise an error
-    # before the actual test call
-    set(:broken_repository) { create(:project, :broken_storage).repository }
+    let(:broken_repository) { create(:project, :broken_storage).repository }
 
     describe 'when storage is broken', :broken_storage  do
       it 'should raise a storage error' do
diff --git a/spec/requests/api/circuit_breakers_spec.rb b/spec/requests/api/circuit_breakers_spec.rb
index 3b858c40fd6..fe76f057115 100644
--- a/spec/requests/api/circuit_breakers_spec.rb
+++ b/spec/requests/api/circuit_breakers_spec.rb
@@ -47,7 +47,7 @@ describe API::CircuitBreakers do
 
   describe 'DELETE circuit_breakers/repository_storage' do
     it 'clears all circuit_breakers' do
-      expect(Gitlab::Git::Storage::CircuitBreaker).to receive(:reset_all!)
+      expect(Gitlab::Git::Storage::FailureInfo).to receive(:reset_all!)
 
       delete api('/circuit_breakers/repository_storage', admin)
 
diff --git a/spec/requests/api/settings_spec.rb b/spec/requests/api/settings_spec.rb
index 63175c40a18..015d4b9a491 100644
--- a/spec/requests/api/settings_spec.rb
+++ b/spec/requests/api/settings_spec.rb
@@ -54,7 +54,7 @@ describe API::Settings, 'Settings' do
           dsa_key_restriction: 2048,
           ecdsa_key_restriction: 384,
           ed25519_key_restriction: 256,
-          circuitbreaker_failure_wait_time: 2
+          circuitbreaker_check_interval: 2
 
         expect(response).to have_gitlab_http_status(200)
         expect(json_response['default_projects_limit']).to eq(3)
@@ -75,7 +75,7 @@ describe API::Settings, 'Settings' do
         expect(json_response['dsa_key_restriction']).to eq(2048)
         expect(json_response['ecdsa_key_restriction']).to eq(384)
         expect(json_response['ed25519_key_restriction']).to eq(256)
-        expect(json_response['circuitbreaker_failure_wait_time']).to eq(2)
+        expect(json_response['circuitbreaker_check_interval']).to eq(2)
       end
     end
 
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index 242a2230b67..f94fb8733d5 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -121,18 +121,6 @@ RSpec.configure do |config|
     reset_delivered_emails!
   end
 
-  # Stub the `ForkedStorageCheck.storage_available?` method unless
-  # `:broken_storage` metadata is defined
-  #
-  # This check can be slow and is unnecessary in a test environment where we
-  # know the storage is available, because we create it at runtime
-  config.before(:example) do |example|
-    unless example.metadata[:broken_storage]
-      allow(Gitlab::Git::Storage::ForkedStorageCheck)
-        .to receive(:storage_available?).and_return(true)
-    end
-  end
-
   config.around(:each, :use_clean_rails_memory_store_caching) do |example|
     caching_store = Rails.cache
     Rails.cache = ActiveSupport::Cache::MemoryStore.new
diff --git a/spec/support/stored_repositories.rb b/spec/support/stored_repositories.rb
index f3deae0f455..f9121cce985 100644
--- a/spec/support/stored_repositories.rb
+++ b/spec/support/stored_repositories.rb
@@ -12,6 +12,25 @@ RSpec.configure do |config|
       raise GRPC::Unavailable.new('Gitaly broken in this spec')
     end
 
-    Gitlab::Git::Storage::CircuitBreaker.reset_all!
+    # Track the maximum number of failures
+    first_failure = Time.parse("2017-11-14 17:52:30")
+    last_failure = Time.parse("2017-11-14 18:54:37")
+    failure_count = Gitlab::CurrentSettings
+                      .current_application_settings
+                      .circuitbreaker_failure_count_threshold + 1
+    cache_key = "#{Gitlab::Git::Storage::REDIS_KEY_PREFIX}broken:#{Gitlab::Environment.hostname}"
+
+    Gitlab::Git::Storage.redis.with do |redis|
+      redis.pipelined do
+        redis.zadd(Gitlab::Git::Storage::REDIS_KNOWN_KEYS, 0, cache_key)
+        redis.hset(cache_key, :first_failure, first_failure.to_i)
+        redis.hset(cache_key, :last_failure, last_failure.to_i)
+        redis.hset(cache_key, :failure_count, failure_count.to_i)
+      end
+    end
+  end
+
+  config.after(:each, :broken_storage) do
+    Gitlab::Git::Storage.redis.with(&:flushall)
   end
 end
diff --git a/spec/support/stub_configuration.rb b/spec/support/stub_configuration.rb
index 4ead78529c3..b36cf3c544c 100644
--- a/spec/support/stub_configuration.rb
+++ b/spec/support/stub_configuration.rb
@@ -43,6 +43,8 @@ module StubConfiguration
   end
 
   def stub_storage_settings(messages)
+    messages.deep_stringify_keys!
+
     # Default storage is always required
     messages['default'] ||= Gitlab.config.repositories.storages.default
     messages.each do |storage_name, storage_settings|