Merge branch 'jc-detect-nfs-for-rugged' into 'master'

Use Rugged if we detect storage is NFS and we can access the disk

See merge request gitlab-org/gitlab-ce!29725

changelogs/unreleased/jc-detect-nfs-for-rugged.yml

+---
+title: Use Rugged if we detect storage is NFS and we can access the disk
+merge_request: 29725
+author:
+type: performance

lib/gitlab/git/rugged_impl/blob.rb

@@ -11,10 +11,11 @@ module Gitlab
       module Blob
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
           override :tree_entry
           def tree_entry(repository, sha, path, limit)
-            if Feature.enabled?(:rugged_tree_entry)
+            if use_rugged?(repository, :rugged_tree_entry)
               rugged_tree_entry(repository, sha, path, limit)
             else
               super

lib/gitlab/git/rugged_impl/commit.rb

@@ -12,6 +12,7 @@ module Gitlab
       module Commit
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
           def rugged_find(repo, commit_id)
             obj = repo.rev_parse_target(commit_id)
@@ -34,7 +35,7 @@ module Gitlab
           override :find_commit
           def find_commit(repo, commit_id)
-            if Feature.enabled?(:rugged_find_commit)
+            if use_rugged?(repo, :rugged_find_commit)
               rugged_find(repo, commit_id)
             else
               super
@@ -43,7 +44,7 @@ module Gitlab
           override :batch_by_oid
           def batch_by_oid(repo, oids)
-            if Feature.enabled?(:rugged_list_commits_by_oid)
+            if use_rugged?(repo, :rugged_list_commits_by_oid)
               rugged_batch_by_oid(repo, oids)
             else
               super
@@ -52,6 +53,7 @@ module Gitlab
         end
         extend ::Gitlab::Utils::Override
+        include Gitlab::Git::RuggedImpl::UseRugged
         override :init_commit
         def init_commit(raw_commit)
@@ -65,7 +67,7 @@ module Gitlab
         override :commit_tree_entry
         def commit_tree_entry(path)
-          if Feature.enabled?(:rugged_commit_tree_entry)
+          if use_rugged?(@repository, :rugged_commit_tree_entry)
             rugged_tree_entry(path)
           else
             super

lib/gitlab/git/rugged_impl/repository.rb

@@ -11,6 +11,7 @@ module Gitlab
     module RuggedImpl
       module Repository
         extend ::Gitlab::Utils::Override
+        include Gitlab::Git::RuggedImpl::UseRugged
         FEATURE_FLAGS = %i(rugged_find_commit rugged_tree_entries rugged_tree_entry rugged_commit_is_ancestor rugged_commit_tree_entry rugged_list_commits_by_oid).freeze
@@ -46,7 +47,7 @@ module Gitlab
         override :ancestor?
         def ancestor?(from, to)
-          if Feature.enabled?(:rugged_commit_is_ancestor)
+          if use_rugged?(self, :rugged_commit_is_ancestor)
             rugged_is_ancestor?(from, to)
           else
             super

lib/gitlab/git/rugged_impl/tree.rb

@@ -11,10 +11,11 @@ module Gitlab
       module Tree
         module ClassMethods
           extend ::Gitlab::Utils::Override
+          include Gitlab::Git::RuggedImpl::UseRugged
           override :tree_entries
           def tree_entries(repository, sha, path, recursive)
-            if Feature.enabled?(:rugged_tree_entries)
+            if use_rugged?(repository, :rugged_tree_entries)
               tree_entries_with_flat_path_from_rugged(repository, sha, path, recursive)
             else
               super

lib/gitlab/git/rugged_impl/use_rugged.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Git
+    module RuggedImpl
+      module UseRugged
+        def use_rugged?(repo, feature_key)
+          feature = Feature.get(feature_key)
+          return feature.enabled? if Feature.persisted?(feature)
+
+          Gitlab::GitalyClient.can_use_disk?(repo.storage)
+        end
+      end
+    end
+  end
+end

lib/gitlab/gitaly_client.rb

@@ -30,6 +30,7 @@ module Gitlab
     SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'
     MAXIMUM_GITALY_CALLS = 30
     CLIENT_NAME = (Sidekiq.server? ? 'gitlab-sidekiq' : 'gitlab-web').freeze
+    GITALY_METADATA_FILENAME = '.gitaly-metadata'
     MUTEX = Mutex.new
@@ -378,6 +379,45 @@ module Gitlab
       0
     end
+    def self.storage_metadata_file_path(storage)
+      Gitlab::GitalyClient::StorageSettings.allow_disk_access do
+        File.join(
+          Gitlab.config.repositories.storages[storage].legacy_disk_path, GITALY_METADATA_FILENAME
+        )
+      end
+    end
+
+    def self.can_use_disk?(storage)
+      cached_value = MUTEX.synchronize do
+        @can_use_disk ||= {}
+        @can_use_disk[storage]
+      end
+
+      return cached_value unless cached_value.nil?
+
+      gitaly_filesystem_id = filesystem_id(storage)
+      direct_filesystem_id = filesystem_id_from_disk(storage)
+
+      MUTEX.synchronize do
+        @can_use_disk[storage] = gitaly_filesystem_id.present? &&
+          gitaly_filesystem_id == direct_filesystem_id
+      end
+    end
+
+    def self.filesystem_id(storage)
+      response = Gitlab::GitalyClient::ServerService.new(storage).info
+      storage_status = response.storage_statuses.find { |status| status.storage_name == storage }
+      storage_status.filesystem_id
+    end
+
+    def self.filesystem_id_from_disk(storage)
+      metadata_file = File.read(storage_metadata_file_path(storage))
+      metadata_hash = JSON.parse(metadata_file)
+      metadata_hash['gitaly_filesystem_id']
+    rescue Errno::ENOENT, JSON::ParserError
+      nil
+    end
+
     def self.timeout(timeout_name)
       Gitlab::CurrentSettings.current_application_settings[timeout_name]
     end

spec/lib/gitlab/cache/ci/project_pipeline_status_spec.rb

@@ -26,6 +26,7 @@ describe Gitlab::Cache::Ci::ProjectPipelineStatus, :clean_gitlab_redis_cache do
       end
       it 'loads 10 projects without hitting Gitaly call limit', :request_store do
+        allow(Gitlab::GitalyClient).to receive(:can_access_disk?).and_return(false)
         projects = Gitlab::GitalyClient.allow_n_plus_1_calls do
           (1..10).map { create(:project, :repository) }
         end

spec/lib/gitlab/git/commit_spec.rb

@@ -408,7 +408,7 @@ describe Gitlab::Git::Commit, :seed_helper do
       context 'when oids is empty' do
         it 'makes no Gitaly request' do
-          expect(Gitlab::GitalyClient).not_to receive(:call)
+          expect(Gitlab::GitalyClient).not_to receive(:call).with(repository.storage, :commit_service, :list_commits_by_oid)
           described_class.batch_by_oid(repository, [])
         end

spec/lib/gitlab/git/rugged_impl/use_rugged_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require 'json'
+require 'tempfile'
+
+describe Gitlab::Git::RuggedImpl::UseRugged, :seed_helper do
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
+  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 }
+
+  before(:all) do
+    create_gitaly_metadata_file
+  end
+
+  subject(:wrapper) do
+    klazz = Class.new { include Gitlab::Git::RuggedImpl::UseRugged }
+    klazz.new
+  end
+
+  before do
+    Gitlab::GitalyClient.instance_variable_set(:@can_use_disk, {})
+  end
+
+  context 'when feature flag is not persisted' do
+    before do
+      allow(Feature).to receive(:persisted?).with(feature_flag).and_return(false)
+    end
+
+    it 'returns true when gitaly matches disk' do
+      expect(subject.use_rugged?(repository, 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")
+
+      expect(subject.use_rugged?(repository, 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)
+
+      expect(subject.use_rugged?(repository, 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
+      expect(subject.use_rugged?(repository, feature_flag_name)).to be true
+
+      expect(Gitlab::GitalyClient).not_to receive(:filesystem_id)
+
+      subject.use_rugged?(repository, feature_flag_name)
+    end
+  end
+
+  context 'when feature flag is persisted' do
+    before do
+      allow(Feature).to receive(:persisted?).with(feature_flag).and_return(true)
+    end
+
+    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
+    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
+    end
+  end
+
+  def create_temporary_gitaly_metadata_file
+    tmp = Tempfile.new('.gitaly-metadata')
+    gitaly_metadata = {
+      "gitaly_filesystem_id" => "some-value"
+    }
+    tmp.write(gitaly_metadata.to_json)
+    tmp.flush
+    tmp.close
+    tmp.path
+  end
+
+  def create_gitaly_metadata_file
+    File.open(File.join(SEED_STORAGE_PATH, '.gitaly-metadata'), 'w+') do |f|
+      gitaly_metadata = {
+        "gitaly_filesystem_id" => SecureRandom.uuid
+      }
+      f.write(gitaly_metadata.to_json)
+    end
+  end
+end