From 591ee4e3611dff5f6feac3a73974da4e8d5cc69f Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Thu, 19 Oct 2017 09:05:19 +0200
Subject: Perform the stat check multiple times when checking a storage

Instead of only checking once within a timeout, check multiple times
within a timeout.

That means with a timeout of 30 seconds and 3 retries. Each try would
be allowed 20 seconds.
---
 lib/gitlab/git/storage/circuit_breaker.rb                |  2 +-
 lib/gitlab/git/storage/forked_storage_check.rb           | 13 +++++++++++--
 spec/lib/gitlab/git/storage/circuit_breaker_spec.rb      | 13 ++++++++++++-
 spec/lib/gitlab/git/storage/forked_storage_check_spec.rb | 15 +++++++++++++++
 4 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/lib/gitlab/git/storage/circuit_breaker.rb b/lib/gitlab/git/storage/circuit_breaker.rb
index 2ce97ff41f9..e53171fa3c5 100644
--- a/lib/gitlab/git/storage/circuit_breaker.rb
+++ b/lib/gitlab/git/storage/circuit_breaker.rb
@@ -91,7 +91,7 @@ module Gitlab
           return @storage_available if @storage_available
 
           if @storage_available = Gitlab::Git::Storage::ForkedStorageCheck
-                                    .storage_available?(storage_path, storage_timeout)
+                                    .storage_available?(storage_path, storage_timeout, access_retries)
             track_storage_accessible
           else
             track_storage_inaccessible
diff --git a/lib/gitlab/git/storage/forked_storage_check.rb b/lib/gitlab/git/storage/forked_storage_check.rb
index 91d8241f17b..1307f400700 100644
--- a/lib/gitlab/git/storage/forked_storage_check.rb
+++ b/lib/gitlab/git/storage/forked_storage_check.rb
@@ -4,8 +4,17 @@ module Gitlab
       module ForkedStorageCheck
         extend self
 
-        def storage_available?(path, timeout_seconds = 5)
-          status = timeout_check(path, timeout_seconds)
+        def storage_available?(path, timeout_seconds = 5, retries = 1)
+          partial_timeout = timeout_seconds / retries
+          status = timeout_check(path, partial_timeout)
+
+          # If the status check did not succeed the first time, we retry a few
+          # more times to avoid one-off failures
+          current_attempts = 1
+          while current_attempts < retries && !status.success?
+            status = timeout_check(path, partial_timeout)
+            current_attempts += 1
+          end
 
           status.success?
         end
diff --git a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
index e3f221aa863..c723ab18e67 100644
--- a/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
+++ b/spec/lib/gitlab/git/storage/circuit_breaker_spec.rb
@@ -181,13 +181,24 @@ describe Gitlab::Git::Storage::CircuitBreaker, clean_gitlab_redis_shared_state:
       expect(circuit_breaker.last_failure).to be_nil
     end
 
-    it 'only accessibility check once' do
+    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
       it 'returns the block without checking accessibility' do
         stub_feature_flags(git_storage_circuit_breaker: false)
diff --git a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb
index c708b15853a..39a5d020bb4 100644
--- a/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb
+++ b/spec/lib/gitlab/git/storage/forked_storage_check_spec.rb
@@ -33,6 +33,21 @@ describe Gitlab::Git::Storage::ForkedStorageCheck, broken_storage: true, skip_da
       expect(runtime).to be < 1.0
     end
 
+    it 'will try the specified amount of times before failing'  do
+      allow(described_class).to receive(:check_filesystem_in_process) do
+        Process.spawn("sleep 10")
+      end
+
+      expect(Process).to receive(:spawn).with('sleep 10').twice
+                           .and_call_original
+
+      runtime = Benchmark.realtime do
+        described_class.storage_available?(existing_path, 0.5, 2)
+      end
+
+      expect(runtime).to be < 1.0
+    end
+
     describe 'when using paths with spaces' do
       let(:test_dir) { Rails.root.join('tmp', 'tests', 'storage_check') }
       let(:path_with_spaces) { File.join(test_dir, 'path with spaces') }
-- 
cgit v1.2.1