diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 98f1931ce5ea02c72f55596819098ed0a9a371d1..f6db3ebf22fb7ce390bab17367acdc1f77647333 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -173,6 +173,8 @@ def merge_request_diff after_commit :ensure_metrics!, on: [:create, :update], unless: :importing? after_commit :expire_etag_cache, unless: :importing? + after_find :preload_branches + # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests attr_accessor :allow_broken @@ -1761,6 +1763,17 @@ def target_project_namespace end end + def preload_branches + # There are cases when MergeRequest object is only partially loaded + # Skip preloading if required fields are missing + return unless has_attribute?(:source_project_id) && has_attribute?(:target_project_id) + + Gitlab::Git::RefPreloader.collect_ref(source_project_id, Gitlab::Git::BRANCH_REF_PREFIX + self.source_branch) + Gitlab::Git::RefPreloader.collect_ref(target_project_id, Gitlab::Git::BRANCH_REF_PREFIX + self.target_branch) + + nil + end + def source_branch_exists? return false unless self.source_project diff --git a/app/models/repository.rb b/app/models/repository.rb index 9e2dfebc7cdd3f0421935eb49aea661544e6b2b0..a76ef5de4f727399a8c6cde3a201f06845a4b73e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -310,15 +310,43 @@ def ref_names branch_names + tag_names end + def lazy_ref_exists?(ref_name) + BatchLoader.for(ref_name).batch(key: self) do |ref_names, loader, _args| + # Make a single Gitaly call to check all refs at once + existing_refs = list_refs(ref_names).to_h { |r| [r.name, true] } + + # Load results for each requested ref + ref_names.each do |ref| + loader.call(ref, existing_refs.key?(ref)) + end + end + end + def branch_exists?(branch_name) return false unless raw_repository + if Feature.enabled?(:ref_existence_check_gitaly, project) + return false unless exists? + return false if branch_name.blank? + + Gitlab::Git::RefPreloader.preload_refs_for_project(project) + + return lazy_ref_exists?(Gitlab::Git::BRANCH_REF_PREFIX + branch_name).itself + end + branch_names_include?(branch_name) end def tag_exists?(tag_name) return false unless raw_repository + if Feature.enabled?(:ref_existence_check_gitaly, project) + return false unless exists? + return false if tag_name.blank? + + return lazy_ref_exists?(Gitlab::Git::TAG_REF_PREFIX + tag_name).itself + end + tag_names_include?(tag_name) end @@ -367,12 +395,14 @@ def cached_methods def expire_tags_cache expire_method_caches(%i[tag_names tag_count has_ambiguous_refs?]) + BatchLoader::Executor.clear_current if Feature.enabled?(:ref_existence_check_gitaly, project) @tags = nil @tag_names_include = nil end def expire_branches_cache expire_method_caches(%i[branch_names merged_branch_names branch_count has_visible_content? has_ambiguous_refs?]) + BatchLoader::Executor.clear_current if Feature.enabled?(:ref_existence_check_gitaly, project) expire_protected_branches_cache @local_branches = nil diff --git a/config/feature_flags/beta/ref_existence_check_gitaly.yml b/config/feature_flags/beta/ref_existence_check_gitaly.yml new file mode 100644 index 0000000000000000000000000000000000000000..0141c1e5eefc7c0c76311b7ccd5eb49a3bd608a5 --- /dev/null +++ b/config/feature_flags/beta/ref_existence_check_gitaly.yml @@ -0,0 +1,10 @@ +--- +name: ref_existence_check_gitaly +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/556674 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/198569 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/556727 +milestone: '18.4' +group: group::source code +type: beta +default_enabled: false diff --git a/lib/gitlab/git/ref_preloader.rb b/lib/gitlab/git/ref_preloader.rb new file mode 100644 index 0000000000000000000000000000000000000000..a7c864dc588b61f4fe7deaf1c5f39b1298cc65b5 --- /dev/null +++ b/lib/gitlab/git/ref_preloader.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +module Gitlab + module Git + # Collects and batches Git reference existence checks to reduce Gitaly calls. + # + # Instead of making individual Gitaly calls for each branch/tag existence check, + # this module collects refs during request processing and batches them into + # a single Gitaly call per project. + # + # Usage: + # RefPreloader.collect_ref(project_id, "refs/heads/main") + # RefPreloader.preload_refs_for_project(project) + module RefPreloader + # Collects a ref for later batch processing + def self.collect_ref(project_id, ref_name) + refs_to_preload[project_id] ||= Set.new + refs_to_preload[project_id] << ref_name + end + + # Populates batch loading of all collected refs for the given project + def self.preload_refs_for_project(project) + return unless project + return unless refs_to_preload.key?(project.id) + + refs_to_preload[project.id].each do |ref_name| + project.repository.lazy_ref_exists?(ref_name) + end + end + + # Thread-local storage for refs pending batch processing + def self.refs_to_preload + if Gitlab::SafeRequestStore.active? + Gitlab::SafeRequestStore.fetch(:refs_to_preload) { {} } + else + # Needed for tests. SafeRequestStore is disabled by default in the test environment. + # We would need to modify dozens of tests enabling :request_store before else block can be removed. + Thread.current[:refs_to_preload] ||= {} + end + end + end + end +end diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb index 0931c1b9d5bc05d3547e041533c003e22afa12c0..1d829ff96ce7b63c1a41f35948e72a16536563f2 100644 --- a/lib/gitlab/gitaly_client/ref_service.rb +++ b/lib/gitlab/gitaly_client/ref_service.rb @@ -240,7 +240,7 @@ def get_tag_signatures(tag_ids) def list_refs(patterns = [Gitlab::Git::BRANCH_REF_PREFIX], pointing_at_oids: [], peel_tags: false, dynamic_timeout: nil, sort_by: nil, pagination_params: nil) request = Gitaly::ListRefsRequest.new( repository: @gitaly_repo, - patterns: patterns, + patterns: patterns.map { |p| encode_binary(p) }, pointing_at_oids: pointing_at_oids, peel_tags: peel_tags, pagination_params: pagination_params diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 432616c6be620f7211bd455db8eb04571b462d30..9df549874b111e807224893af273deced8a3d041 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -77,9 +77,9 @@ expect(::Gitlab::GitalyClient).to receive(:allow_ref_name_caching).and_call_original - # ListCommitsByOid, RepositoryExists, HasLocalBranches, ListCommitsByRefNames + # ListCommitsByOid, RepositoryExists, HasLocalBranches, ListCommitsByRefNames, ListRefs expect { get_pipelines_index_json } - .to change { Gitlab::GitalyClient.get_request_count }.by(4) + .to change { Gitlab::GitalyClient.get_request_count }.by(5) end end diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 2f11358ff8869d30ff89cf235b63b9c3165a1167..bfc04966d06ed2875640937ac0d26c9fd2010d14 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -2024,22 +2024,34 @@ def expect_to_raise_storage_error subject { repository.branch_exists?(branch) } - it 'delegates to branch_names when the cache is empty' do - repository.expire_branches_cache + it 'batch loads requested refs' do + expect(repository).to receive(:list_refs).with(["refs/heads/#{branch}"]).and_call_original - expect(repository).to receive(:branch_names).and_call_original is_expected.to eq(true) end - it 'uses redis set caching when the cache is filled' do - repository.branch_names # ensure the branch name cache is filled + context 'when "ref_existence_check_gitaly" is disabled' do + before do + stub_feature_flags(ref_existence_check_gitaly: false) + end - expect(repository) - .to receive(:branch_names_include?) - .with(branch) - .and_call_original + it 'delegates to branch_names when the cache is empty' do + repository.expire_branches_cache - is_expected.to eq(true) + expect(repository).to receive(:branch_names).and_call_original + is_expected.to eq(true) + end + + it 'uses redis set caching when the cache is filled' do + repository.branch_names # ensure the branch name cache is filled + + expect(repository) + .to receive(:branch_names_include?) + .with(branch) + .and_call_original + + is_expected.to eq(true) + end end end @@ -2048,22 +2060,34 @@ def expect_to_raise_storage_error subject { repository.tag_exists?(tag) } - it 'delegates to tag_names when the cache is empty' do - repository.expire_tags_cache + it 'batch loads requested refs' do + expect(repository).to receive(:list_refs).with(["refs/tags/#{tag}"]).and_call_original - expect(repository).to receive(:tag_names).and_call_original is_expected.to eq(true) end - it 'uses redis set caching when the cache is filled' do - repository.tag_names # ensure the tag name cache is filled + context 'when "ref_existence_check_gitaly" is disabled' do + before do + stub_feature_flags(ref_existence_check_gitaly: false) + end - expect(repository) - .to receive(:tag_names_include?) - .with(tag) - .and_call_original + it 'delegates to tag_names when the cache is empty' do + repository.expire_tags_cache - is_expected.to eq(true) + expect(repository).to receive(:tag_names).and_call_original + is_expected.to eq(true) + end + + it 'uses redis set caching when the cache is filled' do + repository.tag_names # ensure the tag name cache is filled + + expect(repository) + .to receive(:tag_names_include?) + .with(tag) + .and_call_original + + is_expected.to eq(true) + end end end diff --git a/spec/services/git/branch_hooks_service_spec.rb b/spec/services/git/branch_hooks_service_spec.rb index 130ee6ea126f1dfd29a4acbb354834d441d209dd..98c3073b59dd6fa77bc21e0376f192c81a614e51 100644 --- a/spec/services/git/branch_hooks_service_spec.rb +++ b/spec/services/git/branch_hooks_service_spec.rb @@ -573,8 +573,8 @@ def clears_extended_cache context 'oldrev is set' do context 'Gitaly does not know about the branch' do it 'is treated as a new branch' do - allow(project.repository).to receive(:branch_names) { [] } - + allow(project.repository).to receive(:branch_exists?).with("refs/heads/#{branch}").and_return(false) + allow(project.repository).to receive(:branch_exists?).with(branch).and_return(false) expect(service).to receive(:branch_create_hooks) service.execute diff --git a/spec/workers/ci/external_pull_requests/create_pipeline_worker_spec.rb b/spec/workers/ci/external_pull_requests/create_pipeline_worker_spec.rb index 9bea143261381515214cf1e719e6eee034399228..7772817bcef83eafbfd77c796b6b780272c39505 100644 --- a/spec/workers/ci/external_pull_requests/create_pipeline_worker_spec.rb +++ b/spec/workers/ci/external_pull_requests/create_pipeline_worker_spec.rb @@ -12,6 +12,10 @@ let(:worker) { described_class.new } + before do + stub_feature_flags(ref_existence_check_gitaly: false) + end + describe '#perform' do let(:project_id) { project.id } let(:user_id) { user.id }