From 1993feb5e100c04f33c876ba1b9b7a9083227d8a Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Wed, 20 Oct 2021 17:08:36 -0700
Subject: [PATCH] Optimize JIRA ref lookup
In a project with JIRA activated, `ProcessCommitWorker` attempts to add
a comment to a JIRA issue if that issue is mentioned in a
commit. However, the JIRA integration would attempt to retrieve all tags
and branches and pick the first matching ref given a commit OID.
The problem with that approach is that after each push, the list of all
branches and tags are expired and could take a while to gather. Since
multiple `ProcessCommitWorker` jobs can be running at the same time,
this can lead to high I/O on Gitaly nodes since multiple
`ProcessCommitWorker` jobs can run at the same time.
We observe that we don't really need to build the entire ref list; we
can just ask Gitaly for a single matching ref for the given OID with the
newly-created `FindRefsByOID` RPC introduced in
https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3947.
Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/343035
Changelog: performance
---
Gemfile | 2 +-
Gemfile.lock | 4 +--
app/models/integrations/jira.rb | 10 ++++++-
.../development/jira_use_first_ref_by_oid.yml | 8 ++++++
lib/gitlab/git/commit.rb | 14 +++++++++-
lib/gitlab/git/repository.rb | 11 ++++++++
lib/gitlab/gitaly_client/ref_service.rb | 7 +++++
spec/lib/gitlab/git/commit_spec.rb | 8 ++++++
spec/lib/gitlab/git/repository_spec.rb | 26 +++++++++++++++++++
.../gitlab/gitaly_client/ref_service_spec.rb | 15 +++++++++++
spec/models/integrations/jira_spec.rb | 16 +++++++++++-
11 files changed, 115 insertions(+), 6 deletions(-)
create mode 100644 config/feature_flags/development/jira_use_first_ref_by_oid.yml
diff --git a/Gemfile b/Gemfile
index 11bef61d4875d..3a04f1e1395b7 100644
--- a/Gemfile
+++ b/Gemfile
@@ -476,7 +476,7 @@ end
gem 'spamcheck', '~> 0.1.0'
# Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 14.3.0.pre.rc2'
+gem 'gitaly', '~> 14.4.0.pre.rc43'
# KAS GRPC protocol definitions
gem 'kas-grpc', '~> 0.0.2'
diff --git a/Gemfile.lock b/Gemfile.lock
index 40b75863c6507..7497734d6dcff 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -450,7 +450,7 @@ GEM
rails (>= 3.2.0)
git (1.7.0)
rchardet (~> 1.8)
- gitaly (14.3.0.pre.rc2)
+ gitaly (14.4.0.pre.rc43)
grpc (~> 1.0)
github-markup (1.7.0)
gitlab (4.16.1)
@@ -1457,7 +1457,7 @@ DEPENDENCIES
gettext (~> 3.3)
gettext_i18n_rails (~> 1.8.0)
gettext_i18n_rails_js (~> 1.3)
- gitaly (~> 14.3.0.pre.rc2)
+ gitaly (~> 14.4.0.pre.rc43)
github-markup (~> 1.7.0)
gitlab-chronic (~> 0.10.5)
gitlab-dangerfiles (~> 2.3.1)
diff --git a/app/models/integrations/jira.rb b/app/models/integrations/jira.rb
index a23c6e18d71cc..26a1d1aed190f 100644
--- a/app/models/integrations/jira.rb
+++ b/app/models/integrations/jira.rb
@@ -302,6 +302,14 @@ def issue_transition_enabled?
private
+ def branch_name(noteable)
+ if Feature.enabled?(:jira_use_first_ref_by_oid, project, default_enabled: :yaml)
+ noteable.first_ref_by_oid(project.repository)
+ else
+ noteable.ref_names(project.repository).first
+ end
+ end
+
def server_info
strong_memoize(:server_info) do
client_url.present? ? jira_request { client.ServerInfo.all.attrs } : nil
@@ -495,7 +503,7 @@ def build_entity_meta(noteable)
{
id: noteable.short_id,
description: noteable.safe_message,
- branch: noteable.ref_names(project.repository).first
+ branch: branch_name(noteable)
}
elsif noteable.is_a?(MergeRequest)
{
diff --git a/config/feature_flags/development/jira_use_first_ref_by_oid.yml b/config/feature_flags/development/jira_use_first_ref_by_oid.yml
new file mode 100644
index 0000000000000..10e2ad1b8ada6
--- /dev/null
+++ b/config/feature_flags/development/jira_use_first_ref_by_oid.yml
@@ -0,0 +1,8 @@
+---
+name: jira_use_first_ref_by_oid
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/72739
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/343585
+milestone: '14.5'
+type: development
+group: group::integrations
+default_enabled: false
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index 6605e896ef1d4..267107e04e695 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -315,10 +315,18 @@ def stats
#
def ref_names(repo)
refs(repo).map do |ref|
- ref.sub(%r{^refs/(heads|remotes|tags)/}, "")
+ strip_ref_prefix(ref)
end
end
+ def first_ref_by_oid(repo)
+ ref = repo.refs_by_oid(oid: id, limit: 1)&.first
+
+ return unless ref
+
+ strip_ref_prefix(ref)
+ end
+
def message
encode! @message
end
@@ -466,6 +474,10 @@ def self.valid?(commit_id)
commit_id.match?(/\s/)
)
end
+
+ def strip_ref_prefix(ref)
+ ref.sub(%r{^refs/(heads|remotes|tags)/}, "")
+ end
end
end
end
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index dde9226a93efc..c5f3f224a3d46 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -519,6 +519,17 @@ def refs_hash
@refs_hash
end
+ # Returns matching refs for OID
+ #
+ # Limit of 0 means there is no limit.
+ def refs_by_oid(oid:, limit: 0)
+ wrapped_gitaly_errors do
+ gitaly_ref_client.find_refs_by_oid(oid: oid, limit: limit)
+ end
+ rescue CommandError, TypeError, NoRepository
+ nil
+ end
+
# Returns url for submodule
#
# Ex.
diff --git a/lib/gitlab/gitaly_client/ref_service.rb b/lib/gitlab/gitaly_client/ref_service.rb
index aa4626029b539..c28abda3843e1 100644
--- a/lib/gitlab/gitaly_client/ref_service.rb
+++ b/lib/gitlab/gitaly_client/ref_service.rb
@@ -210,6 +210,13 @@ def pack_refs
GitalyClient.call(@storage, :ref_service, :pack_refs, request, timeout: GitalyClient.long_timeout)
end
+ def find_refs_by_oid(oid:, limit:)
+ request = Gitaly::FindRefsByOIDRequest.new(repository: @gitaly_repo, sort_field: :refname, oid: oid, limit: limit)
+
+ response = GitalyClient.call(@storage, :ref_service, :find_refs_by_oid, request, timeout: GitalyClient.medium_timeout)
+ response&.refs&.to_a
+ end
+
private
def consume_refs_response(response)
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index f4dba5e8d5869..11510daf9c0b1 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -715,6 +715,14 @@
it { is_expected.not_to include("feature") }
end
+ describe '#first_ref_by_oid' do
+ let(:commit) { described_class.find(repository, 'master') }
+
+ subject { commit.first_ref_by_oid(repository) }
+
+ it { is_expected.to eq("master") }
+ end
+
describe '.get_message' do
let(:commit_ids) { %w[6d394385cf567f80a8fd85055db1ab4c5295806f cfe32cf61b73a0d5e9f13e774abde7ff789b1660] }
diff --git a/spec/lib/gitlab/git/repository_spec.rb b/spec/lib/gitlab/git/repository_spec.rb
index 8d28515d1af86..1004947e368be 100644
--- a/spec/lib/gitlab/git/repository_spec.rb
+++ b/spec/lib/gitlab/git/repository_spec.rb
@@ -1900,6 +1900,32 @@ def create_commit(blobs)
end
end
+ describe '#refs_by_oid' do
+ it 'returns a list of refs from a OID' do
+ refs = repository.refs_by_oid(oid: repository.commit.id)
+
+ expect(refs).to be_an(Array)
+ expect(refs).to include(Gitlab::Git::BRANCH_REF_PREFIX + repository.root_ref)
+ end
+
+ it 'returns a single ref from a OID' do
+ refs = repository.refs_by_oid(oid: repository.commit.id, limit: 1)
+
+ expect(refs).to be_an(Array)
+ expect(refs).to eq([Gitlab::Git::BRANCH_REF_PREFIX + repository.root_ref])
+ end
+
+ it 'returns empty for unknown ID' do
+ expect(repository.refs_by_oid(oid: Gitlab::Git::BLANK_SHA, limit: 0)).to eq([])
+ end
+
+ it 'returns nil for an empty repo' do
+ project = create(:project)
+
+ expect(project.repository.refs_by_oid(oid: SeedRepo::Commit::ID, limit: 0)).to be_nil
+ end
+ end
+
describe '#set_full_path' do
before do
repository_rugged.config["gitlab.fullpath"] = repository_path
diff --git a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
index b4355b56bac7c..d013bb2bd5586 100644
--- a/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
+++ b/spec/lib/gitlab/gitaly_client/ref_service_spec.rb
@@ -282,4 +282,19 @@
client.pack_refs
end
end
+
+ describe '#find_refs_by_oid' do
+ let(:oid) { project.repository.commit.id }
+
+ it 'sends a find_refs_by_oid message' do
+ expect_any_instance_of(Gitaly::RefService::Stub)
+ .to receive(:find_refs_by_oid)
+ .with(gitaly_request_with_params(sort_field: 'refname', oid: oid, limit: 1), kind_of(Hash))
+ .and_call_original
+
+ refs = client.find_refs_by_oid(oid: oid, limit: 1)
+
+ expect(refs.to_a).to eq([Gitlab::Git::BRANCH_REF_PREFIX + project.repository.root_ref])
+ end
+ end
end
diff --git a/spec/models/integrations/jira_spec.rb b/spec/models/integrations/jira_spec.rb
index 76220987f18d6..6401cd3acbec4 100644
--- a/spec/models/integrations/jira_spec.rb
+++ b/spec/models/integrations/jira_spec.rb
@@ -876,10 +876,24 @@ def close_issue
subject
expect(WebMock).to have_requested(:post, comment_url).with(
- body: /mentioned this issue in/
+ body: /mentioned this issue.*on branch \[master/
).once
end
+ context 'with jira_use_first_ref_by_oid feature flag disabled' do
+ before do
+ stub_feature_flags(jira_use_first_ref_by_oid: false)
+ end
+
+ it 'creates a comment on Jira' do
+ subject
+
+ expect(WebMock).to have_requested(:post, comment_url).with(
+ body: /mentioned this issue.*on branch \[master/
+ ).once
+ end
+ end
+
it 'tracks usage' do
expect(Gitlab::UsageDataCounters::HLLRedisCounter)
.to receive(:track_event)
--
GitLab