Allow disk access if we can access storage directlyjc-allow-disk-access

Move the methods to detect if we can access disk directly into
StorageSettings. This reduces complexity by no longer needing
synchronization, as well as being able to bypass the disk access check.

2 files changed, 35 insertions, 17 deletions

diff --git a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
index e7ef9d08f80..8d18eb7073a 100644
--- a/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
+++ b/spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb
@@ -7,6 +7,8 @@ require 'tempfile'
 describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
   let(:project) { create(:project, :repository) }
   let(:repository) { project.repository }
+  let(:storage_name) { repository.storage }
+  let(:storage) { Gitlab.config.repositories.storages[storage_name] }
   let(:feature_flag_name) { 'feature-flag-name' }
   let(:feature_flag) { Feature.get(feature_flag_name) }
   let(:temp_gitaly_metadata_file) { create_temporary_gitaly_metadata_file }
@@ -21,7 +23,8 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
   end
 
   before do
-    Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {})
+    # reset the cached value before each spec
+    storage.instance_variable_set(:@can_use_disk, nil)
   end
 
   context 'when feature flag is not persisted' do
@@ -30,31 +33,29 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
     end
 
     it 'returns true when gitaly matches disk' do
-      pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338')
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
     end
 
     it 'returns false when disk access fails' do
-      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
+      allow(storage).to receive(:storage_metadata_file_path).and_return("/fake/path/doesnt/exist")
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be false
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be false
     end
 
     it "returns false when gitaly doesn't match disk" do
-      allow(Gitlab::GitalyClient).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
+      allow(storage).to receive(:storage_metadata_file_path).and_return(temp_gitaly_metadata_file)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be_falsey
 
       File.delete(temp_gitaly_metadata_file)
     end
 
     it "doesn't lead to a second rpc call because gitaly client should use the cached value" do
-      pending('temporary disabled because of https://gitlab.com/gitlab-org/gitlab-ce/issues/64338')
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
 
       expect(Gitlab::GitalyClient).not_to receive(:filesystem_id)
 
-      subject.use_rugged?(repository, feature_flag_name)
+      subject.use_rugged?(storage_name, feature_flag_name)
     end
   end
 
@@ -66,14 +67,14 @@ describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
     it 'returns false when the feature flag is off' do
       allow(feature_flag).to receive(:enabled?).and_return(false)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be_falsey
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be_falsey
     end
 
     it "returns true when feature flag is on" do
       allow(feature_flag).to receive(:enabled?).and_return(true)
       allow(Gitlab::GitalyClient).to receive(:can_use_disk?).and_return(false)
 
-      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+      expect(subject.use_rugged?(storage_name, feature_flag_name)).to be true
     end
   end
 
diff --git a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
index f2f53982b09..bc774532c09 100644
--- a/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/storage_settings_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::GitalyClient::StorageSettings do
     context 'when the storage contains no path' do
       it 'raises an error' do
         expect do
-          described_class.new("foo" => {})
+          described_class.new("default", "foo" => {})
         end.to raise_error(described_class::InvalidConfigurationError)
       end
     end
@@ -13,7 +13,7 @@ describe Gitlab::GitalyClient::StorageSettings do
     context "when the argument isn't a hash" do
       it 'raises an error' do
         expect do
-          described_class.new("test")
+          described_class.new("default", "test")
         end.to raise_error("expected a Hash, got a String")
       end
     end
@@ -21,22 +21,39 @@ describe Gitlab::GitalyClient::StorageSettings do
     context 'when the storage is valid' do
       it 'raises no error' do
         expect do
-          described_class.new("path" => Rails.root)
+          described_class.new("default", "path" => Rails.root)
         end.not_to raise_error
       end
     end
   end
 
   describe '.disk_access_denied?' do
+    let(:storage_name) { "default" }
+    subject { described_class.new(storage_name, "path" => Rails.root) }
+
+    before do
+      allow(subject).to receive(:can_use_disk?).and_return(false)
+    end
+
     context 'when Rugged is enabled', :enable_rugged do
       it 'returns false' do
-        expect(described_class.disk_access_denied?).to be_falsey
+        expect(subject.disk_access_denied?).to be_falsey
       end
     end
 
     context 'when Rugged is disabled' do
       it 'returns true' do
-        expect(described_class.disk_access_denied?).to be_truthy
+        expect(subject.disk_access_denied?).to be_truthy
+      end
+    end
+
+    context 'when disk is accessible' do
+      before do
+        allow(subject).to receive(:can_use_disk?).and_return(true)
+      end
+
+      it 'returns false' do
+        expect(subject.disk_access_denied?).to be_falsey
       end
     end
   end