summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndrew Newdigate <andrew@gitlab.com>2017-09-19 10:55:37 +0000
committerRémy Coutable <remy@rymai.me>2017-09-19 10:55:37 +0000
commit64d7ec0a9e3ffd6233ccfbe9100f8a9391c648e5 (patch)
tree2cc89bc2d1087eb145918c6ae8cbf70878d40e97
parent39dd7736585d3e35d5c6f391e6a94c312da09056 (diff)
downloadgitlab-ce-64d7ec0a9e3ffd6233ccfbe9100f8a9391c648e5.tar.gz
Detect n+1 issues involving Gitaly
-rw-r--r--.gitlab-ci.yml2
-rw-r--r--app/controllers/projects/branches_controller.rb10
-rw-r--r--app/controllers/projects/commit_controller.rb7
-rw-r--r--app/controllers/projects/compare_controller.rb4
-rw-r--r--app/controllers/projects/merge_requests/diffs_controller.rb5
-rw-r--r--app/controllers/projects/merge_requests_controller.rb8
-rw-r--r--app/controllers/projects/network_controller.rb23
-rw-r--r--app/controllers/projects/refs_controller.rb17
-rw-r--r--app/controllers/root_controller.rb5
-rw-r--r--app/models/merge_request.rb7
-rw-r--r--app/models/network/graph.rb7
-rw-r--r--app/models/repository.rb1
-rw-r--r--app/services/delete_merged_branches_service.rb19
-rw-r--r--app/services/merge_requests/create_service.rb5
-rw-r--r--app/services/notes/create_service.rb8
-rw-r--r--config/initializers/lograge.rb7
-rw-r--r--lib/api/branches.rb5
-rw-r--r--lib/api/entities.rb5
-rw-r--r--lib/gitlab/diff/file_collection/base.rb5
-rw-r--r--lib/gitlab/git/repository.rb23
-rw-r--r--lib/gitlab/gitaly_client.rb147
-rw-r--r--spec/lib/gitlab/gitaly_client_spec.rb124
-rw-r--r--spec/serializers/pipeline_serializer_spec.rb10
23 files changed, 400 insertions, 54 deletions
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 766fb3a2ef7..1d86757ea1d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -128,7 +128,7 @@ stages:
- export CACHE_CLASSES=true
- cp ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} ${KNAPSACK_REPORT_PATH}
- scripts/gitaly-test-spawn
- - knapsack spinach "-r rerun" || retry '[[ -e tmp/spinach-rerun.txt ]] && bundle exec spinach -r rerun $(cat tmp/spinach-rerun.txt)'
+ - knapsack spinach "-r rerun" -b || retry '[[ -e tmp/spinach-rerun.txt ]] && bundle exec spinach -b -r rerun $(cat tmp/spinach-rerun.txt)'
artifacts:
expire_in: 31d
when: always
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 747768eefb1..a9cce578366 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -15,10 +15,14 @@ class Projects::BranchesController < Projects::ApplicationController
respond_to do |format|
format.html do
@refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name))
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37429
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ @max_commits = @branches.reduce(0) do |memo, branch|
+ diverging_commit_counts = repository.diverging_commit_counts(branch)
+ [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
+ end
- @max_commits = @branches.reduce(0) do |memo, branch|
- diverging_commit_counts = repository.diverging_commit_counts(branch)
- [memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
+ render
end
end
format.json do
diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index 1a775def506..a62f05db7db 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -20,7 +20,12 @@ class Projects::CommitController < Projects::ApplicationController
apply_diff_view_cookie!
respond_to do |format|
- format.html
+ format.html do
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37599
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ render
+ end
+ end
format.diff { render text: @commit.to_diff }
format.patch { render text: @commit.to_patch }
end
diff --git a/app/controllers/projects/compare_controller.rb b/app/controllers/projects/compare_controller.rb
index 3c8eaa24080..3cb4eb23981 100644
--- a/app/controllers/projects/compare_controller.rb
+++ b/app/controllers/projects/compare_controller.rb
@@ -17,6 +17,10 @@ class Projects::CompareController < Projects::ApplicationController
def show
apply_diff_view_cookie!
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37430
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ render
+ end
end
def diff_for_path
diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb
index d60a24d3f1d..7d16e77ef66 100644
--- a/app/controllers/projects/merge_requests/diffs_controller.rb
+++ b/app/controllers/projects/merge_requests/diffs_controller.rb
@@ -10,7 +10,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
def show
@environment = @merge_request.environments_for(current_user).last
- render json: { html: view_to_html_string("projects/merge_requests/diffs/_diffs") }
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37431
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ render json: { html: view_to_html_string("projects/merge_requests/diffs/_diffs") }
+ end
end
def diff_for_path
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 3aa5dadb5ca..c5204080333 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -56,6 +56,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
close_merge_request_without_source_project
check_if_can_be_merged
+ # Return if the response has already been rendered
+ return if response_body
+
respond_to do |format|
format.html do
# Build a note object for comment form
@@ -70,6 +73,11 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
labels
set_pipeline_variables
+
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37432
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ render
+ end
end
format.json do
diff --git a/app/controllers/projects/network_controller.rb b/app/controllers/projects/network_controller.rb
index dfa5e4f7f46..fb68dd771a1 100644
--- a/app/controllers/projects/network_controller.rb
+++ b/app/controllers/projects/network_controller.rb
@@ -8,19 +8,24 @@ class Projects::NetworkController < Projects::ApplicationController
before_action :assign_commit
def show
- @url = project_network_path(@project, @ref, @options.merge(format: :json))
- @commit_url = project_commit_path(@project, 'ae45ca32').gsub("ae45ca32", "%s")
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37602
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ @url = project_network_path(@project, @ref, @options.merge(format: :json))
+ @commit_url = project_commit_path(@project, 'ae45ca32').gsub("ae45ca32", "%s")
- respond_to do |format|
- format.html do
- if @options[:extended_sha1] && !@commit
- flash.now[:alert] = "Git revision '#{@options[:extended_sha1]}' does not exist."
+ respond_to do |format|
+ format.html do
+ if @options[:extended_sha1] && !@commit
+ flash.now[:alert] = "Git revision '#{@options[:extended_sha1]}' does not exist."
+ end
end
- end
- format.json do
- @graph = Network::Graph.new(project, @ref, @commit, @options[:filter_ref])
+ format.json do
+ @graph = Network::Graph.new(project, @ref, @commit, @options[:filter_ref])
+ end
end
+
+ render
end
end
diff --git a/app/controllers/projects/refs_controller.rb b/app/controllers/projects/refs_controller.rb
index 1eb78d8b522..2fd015df688 100644
--- a/app/controllers/projects/refs_controller.rb
+++ b/app/controllers/projects/refs_controller.rb
@@ -51,13 +51,16 @@ class Projects::RefsController < Projects::ApplicationController
contents.push(*tree.blobs)
contents.push(*tree.submodules)
- @logs = contents[@offset, @limit].to_a.map do |content|
- file = @path ? File.join(@path, content.name) : content.name
- last_commit = @repo.last_commit_for_path(@commit.id, file)
- {
- file_name: content.name,
- commit: last_commit
- }
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
+ @logs = Gitlab::GitalyClient.allow_n_plus_1_calls do
+ contents[@offset, @limit].to_a.map do |content|
+ file = @path ? File.join(@path, content.name) : content.name
+ last_commit = @repo.last_commit_for_path(@commit.id, file)
+ {
+ file_name: content.name,
+ commit: last_commit
+ }
+ end
end
offset = (@offset + @limit)
diff --git a/app/controllers/root_controller.rb b/app/controllers/root_controller.rb
index 1b4545e4a49..19e38993038 100644
--- a/app/controllers/root_controller.rb
+++ b/app/controllers/root_controller.rb
@@ -13,7 +13,10 @@ class RootController < Dashboard::ProjectsController
before_action :redirect_logged_user, if: -> { current_user.present? }
def index
- super
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37434
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ super
+ end
end
private
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 2a56bab48a3..31bd130dcc2 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -415,8 +415,11 @@ class MergeRequest < ActiveRecord::Base
end
def create_merge_request_diff
- merge_request_diffs.create
- reload_merge_request_diff
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37435
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ merge_request_diffs.create
+ reload_merge_request_diff
+ end
end
def reload_merge_request_diff
diff --git a/app/models/network/graph.rb b/app/models/network/graph.rb
index 3845e485413..aec7b01e23a 100644
--- a/app/models/network/graph.rb
+++ b/app/models/network/graph.rb
@@ -61,8 +61,11 @@ module Network
@reserved[i] = []
end
- commits_sort_by_ref.each do |commit|
- place_chain(commit)
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37436
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ commits_sort_by_ref.each do |commit|
+ place_chain(commit)
+ end
end
# find parent spaces for not overlap lines
diff --git a/app/models/repository.rb b/app/models/repository.rb
index af9911ea045..9d1de4f4306 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -983,6 +983,7 @@ class Repository
def empty_repo?
!exists? || !has_visible_content?
end
+ cache_method :empty_repo?, memoize_only: true
def search_files_by_content(query, ref)
return [] if empty_repo? || query.blank?
diff --git a/app/services/delete_merged_branches_service.rb b/app/services/delete_merged_branches_service.rb
index ff11bd59d29..077268b2388 100644
--- a/app/services/delete_merged_branches_service.rb
+++ b/app/services/delete_merged_branches_service.rb
@@ -6,15 +6,18 @@ class DeleteMergedBranchesService < BaseService
def execute
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :push_code, project)
- branches = project.repository.branch_names
- branches = branches.select { |branch| project.repository.merged_to_root_ref?(branch) }
- # Prevent deletion of branches relevant to open merge requests
- branches -= merge_request_branch_names
- # Prevent deletion of protected branches
- branches = branches.reject { |branch| project.protected_for?(branch) }
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37438
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ branches = project.repository.branch_names
+ branches = branches.select { |branch| project.repository.merged_to_root_ref?(branch) }
+ # Prevent deletion of branches relevant to open merge requests
+ branches -= merge_request_branch_names
+ # Prevent deletion of protected branches
+ branches = branches.reject { |branch| project.protected_for?(branch) }
- branches.each do |branch|
- DeleteBranchService.new(project, current_user).execute(branch)
+ branches.each do |branch|
+ DeleteBranchService.new(project, current_user).execute(branch)
+ end
end
end
diff --git a/app/services/merge_requests/create_service.rb b/app/services/merge_requests/create_service.rb
index 3d53fe0646b..820709583fa 100644
--- a/app/services/merge_requests/create_service.rb
+++ b/app/services/merge_requests/create_service.rb
@@ -13,7 +13,10 @@ module MergeRequests
merge_request.source_branch = params[:source_branch]
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
- create(merge_request)
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37439
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ create(merge_request)
+ end
end
def before_create(merge_request)
diff --git a/app/services/notes/create_service.rb b/app/services/notes/create_service.rb
index 06971483992..9ea28733f5f 100644
--- a/app/services/notes/create_service.rb
+++ b/app/services/notes/create_service.rb
@@ -4,7 +4,13 @@ module Notes
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
note = Notes::BuildService.new(project, current_user, params).execute
- return note unless note.valid?
+
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37440
+ note_valid = Gitlab::GitalyClient.allow_n_plus_1_calls do
+ note.valid?
+ end
+
+ return note unless note_valid
# We execute commands (extracted from `params[:note]`) on the noteable
# **before** we save the note because if the note consists of commands
diff --git a/config/initializers/lograge.rb b/config/initializers/lograge.rb
index 21fe8d72459..8560d24526f 100644
--- a/config/initializers/lograge.rb
+++ b/config/initializers/lograge.rb
@@ -12,13 +12,18 @@ unless Sidekiq.server?
config.lograge.logger = ActiveSupport::Logger.new(filename)
# Add request parameters to log output
config.lograge.custom_options = lambda do |event|
- {
+ payload = {
time: event.time.utc.iso8601(3),
params: event.payload[:params].except(*%w(controller action format)),
remote_ip: event.payload[:remote_ip],
user_id: event.payload[:user_id],
username: event.payload[:username]
}
+
+ gitaly_calls = Gitlab::GitalyClient.get_request_count
+ payload[:gitaly_calls] = gitaly_calls if gitaly_calls > 0
+
+ payload
end
end
end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 642c1140fcc..643c8e6fb8e 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -21,7 +21,10 @@ module API
get ':id/repository/branches' do
branches = ::Kaminari.paginate_array(user_project.repository.branches.sort_by(&:name))
- present paginate(branches), with: Entities::RepoBranch, project: user_project
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37442
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ present paginate(branches), with: Entities::RepoBranch, project: user_project
+ end
end
resource ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 30b115b1b56..71253f72533 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -244,7 +244,10 @@ module API
end
expose :merged do |repo_branch, options|
- options[:project].repository.merged_to_root_ref?(repo_branch.name)
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37442
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ options[:project].repository.merged_to_root_ref?(repo_branch.name)
+ end
end
expose :protected do |repo_branch, options|
diff --git a/lib/gitlab/diff/file_collection/base.rb b/lib/gitlab/diff/file_collection/base.rb
index a6007ebf531..88ae65cb468 100644
--- a/lib/gitlab/diff/file_collection/base.rb
+++ b/lib/gitlab/diff/file_collection/base.rb
@@ -22,7 +22,10 @@ module Gitlab
end
def diff_files
- @diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37445
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ @diff_files ||= @diffs.decorate! { |diff| decorate_diff!(diff) }
+ end
end
def diff_file_with_old_path(old_path)
diff --git a/lib/gitlab/git/repository.rb b/lib/gitlab/git/repository.rb
index c499ff101b5..18210bcab4e 100644
--- a/lib/gitlab/git/repository.rb
+++ b/lib/gitlab/git/repository.rb
@@ -950,8 +950,8 @@ module Gitlab
@gitaly_repository_client ||= Gitlab::GitalyClient::RepositoryService.new(self)
end
- def gitaly_migrate(method, &block)
- Gitlab::GitalyClient.migrate(method, &block)
+ def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
+ Gitlab::GitalyClient.migrate(method, status: status, &block)
rescue GRPC::NotFound => e
raise NoRepository.new(e)
rescue GRPC::BadStatus => e
@@ -962,14 +962,17 @@ module Gitlab
# Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'.
def branches_filter(filter: nil, sort_by: nil)
- branches = rugged.branches.each(filter).map do |rugged_ref|
- begin
- target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
- Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
- rescue Rugged::ReferenceError
- # Omit invalid branch
- end
- end.compact
+ # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37464
+ branches = Gitlab::GitalyClient.allow_n_plus_1_calls do
+ rugged.branches.each(filter).map do |rugged_ref|
+ begin
+ target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
+ Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
+ rescue Rugged::ReferenceError
+ # Omit invalid branch
+ end
+ end.compact
+ end
sort_branches(branches, sort_by)
end
diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb
index a3dc2cd0b60..cbd9ff406de 100644
--- a/lib/gitlab/gitaly_client.rb
+++ b/lib/gitlab/gitaly_client.rb
@@ -10,7 +10,24 @@ module Gitlab
OPT_OUT = 3
end
+ class TooManyInvocationsError < StandardError
+ attr_reader :call_site, :invocation_count, :max_call_stack
+
+ def initialize(call_site, invocation_count, max_call_stack, most_invoked_stack)
+ @call_site = call_site
+ @invocation_count = invocation_count
+ @max_call_stack = max_call_stack
+ stacks = most_invoked_stack.join('\n') if most_invoked_stack
+
+ msg = "GitalyClient##{call_site} called #{invocation_count} times from single request. Potential n+1?"
+ msg << "\nThe following call site called into Gitaly #{max_call_stack} times:\n#{stacks}\n" if stacks
+
+ super(msg)
+ end
+ end
+
SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
+ MAXIMUM_GITALY_CALLS = 30
MUTEX = Mutex.new
private_constant :MUTEX
@@ -53,6 +70,8 @@ module Gitlab
# All Gitaly RPC call sites should use GitalyClient.call. This method
# makes sure that per-request authentication headers are set.
def self.call(storage, service, rpc, request)
+ enforce_gitaly_request_limits(:call)
+
metadata = request_metadata(storage)
metadata = yield(metadata) if block_given?
stub(service, storage).__send__(rpc, request, metadata) # rubocop:disable GitlabSecurity/PublicSend
@@ -107,12 +126,100 @@ module Gitlab
private_class_method :opt_into_all_features?
def self.migrate(feature, status: MigrationStatus::OPT_IN)
+ # Enforce limits at both the `migrate` and `call` sites to ensure that
+ # problems are not hidden by a feature being disabled
+ enforce_gitaly_request_limits(:migrate)
+
is_enabled = feature_enabled?(feature, status: status)
metric_name = feature.to_s
metric_name += "_gitaly" if is_enabled
Gitlab::Metrics.measure(metric_name) do
- yield is_enabled
+ # Some migrate calls wrap other migrate calls
+ allow_n_plus_1_calls do
+ yield is_enabled
+ end
+ end
+ end
+
+ # Ensures that Gitaly is not being abuse through n+1 misuse etc
+ def self.enforce_gitaly_request_limits(call_site)
+ # Only count limits in request-response environments (not sidekiq for example)
+ return unless RequestStore.active?
+
+ # This is this actual number of times this call was made. Used for information purposes only
+ actual_call_count = increment_call_count("gitaly_#{call_site}_actual")
+
+ # Do no enforce limits in production
+ return if Rails.env.production?
+
+ # Check if this call is nested within a allow_n_plus_1_calls
+ # block and skip check if it is
+ return if get_call_count(:gitaly_call_count_exception_block_depth) > 0
+
+ # This is the count of calls outside of a `allow_n_plus_1_calls` block
+ # It is used for enforcement but not statistics
+ permitted_call_count = increment_call_count("gitaly_#{call_site}_permitted")
+
+ count_stack
+
+ return if permitted_call_count <= MAXIMUM_GITALY_CALLS
+
+ raise TooManyInvocationsError.new(call_site, actual_call_count, max_call_count, max_stacks)
+ end
+
+ def self.allow_n_plus_1_calls
+ return yield unless RequestStore.active?
+
+ begin
+ increment_call_count(:gitaly_call_count_exception_block_depth)
+ yield
+ ensure
+ decrement_call_count(:gitaly_call_count_exception_block_depth)
+ end
+ end
+
+ def self.get_call_count(key)
+ RequestStore.store[key] || 0
+ end
+ private_class_method :get_call_count
+
+ def self.increment_call_count(key)
+ RequestStore.store[key] ||= 0
+ RequestStore.store[key] += 1
+ end
+ private_class_method :increment_call_count
+
+ def self.decrement_call_count(key)
+ RequestStore.store[key] -= 1
+ end
+ private_class_method :decrement_call_count
+
+ # Returns an estimate of the number of Gitaly calls made for this
+ # request
+ def self.get_request_count
+ return 0 unless RequestStore.active?
+
+ gitaly_migrate_count = get_call_count("gitaly_migrate_actual")
+ gitaly_call_count = get_call_count("gitaly_call_actual")
+
+ # Using the maximum of migrate and call_count will provide an
+ # indicator of how many Gitaly calls will be made, even
+ # before a feature is enabled. This provides us with a single
+ # metric, but not an exact number, but this tradeoff is acceptable
+ if gitaly_migrate_count > gitaly_call_count
+ gitaly_migrate_count
+ else
+ gitaly_call_count
+ end
+ end
+
+ def self.reset_counts
+ return unless RequestStore.active?
+
+ %w[migrate call].each do |call_site|
+ RequestStore.store["gitaly_#{call_site}_actual"] = 0
+ RequestStore.store["gitaly_#{call_site}_permitted"] = 0
end
end
@@ -124,5 +231,43 @@ module Gitlab
def self.encode(s)
s.dup.force_encoding(Encoding::ASCII_8BIT)
end
+
+ # Count a stack. Used for n+1 detection
+ def self.count_stack
+ return unless RequestStore.active?
+
+ stack_string = caller.drop(1).join("\n")
+
+ RequestStore.store[:stack_counter] ||= Hash.new
+
+ count = RequestStore.store[:stack_counter][stack_string] || 0
+ RequestStore.store[:stack_counter][stack_string] = count + 1
+ end
+ private_class_method :count_stack
+
+ # Returns a count for the stack which called Gitaly the most times. Used for n+1 detection
+ def self.max_call_count
+ return 0 unless RequestStore.active?
+
+ stack_counter = RequestStore.store[:stack_counter]
+ return 0 unless stack_counter
+
+ stack_counter.values.max
+ end
+ private_class_method :max_call_count
+
+ # Returns the stacks that calls Gitaly the most times. Used for n+1 detection
+ def self.max_stacks
+ return nil unless RequestStore.active?
+
+ stack_counter = RequestStore.store[:stack_counter]
+ return nil unless stack_counter
+
+ max = max_call_count
+ return nil if max.zero?
+
+ stack_counter.select { |_, v| v == max }.keys
+ end
+ private_class_method :max_stacks
end
end
diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb
index a9b861fcff2..9a84d6e6a67 100644
--- a/spec/lib/gitlab/gitaly_client_spec.rb
+++ b/spec/lib/gitlab/gitaly_client_spec.rb
@@ -38,6 +38,130 @@ describe Gitlab::GitalyClient, skip_gitaly_mock: true do
end
end
+ describe 'allow_n_plus_1_calls' do
+ context 'when RequestStore is enabled', :request_store do
+ it 'returns the result of the allow_n_plus_1_calls block' do
+ expect(described_class.allow_n_plus_1_calls { "result" }).to eq("result")
+ end
+ end
+
+ context 'when RequestStore is not active' do
+ it 'returns the result of the allow_n_plus_1_calls block' do
+ expect(described_class.allow_n_plus_1_calls { "something" }).to eq("something")
+ end
+ end
+ end
+
+ describe 'enforce_gitaly_request_limits?' do
+ def call_gitaly(count = 1)
+ (1..count).each do
+ described_class.enforce_gitaly_request_limits(:test)
+ end
+ end
+
+ context 'when RequestStore is enabled', :request_store do
+ it 'allows up the maximum number of allowed calls' do
+ expect { call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS) }.not_to raise_error
+ end
+
+ context 'when the maximum number of calls has been reached' do
+ before do
+ call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS)
+ end
+
+ it 'fails on the next call' do
+ expect { call_gitaly(1) }.to raise_error(Gitlab::GitalyClient::TooManyInvocationsError)
+ end
+ end
+
+ it 'allows the maximum number of calls to be exceeded within an allow_n_plus_1_calls block' do
+ expect do
+ described_class.allow_n_plus_1_calls do
+ call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 1)
+ end
+ end.not_to raise_error
+ end
+
+ context 'when the maximum number of calls has been reached within an allow_n_plus_1_calls block' do
+ before do
+ described_class.allow_n_plus_1_calls do
+ call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS)
+ end
+ end
+
+ it 'allows up to the maximum number of calls outside of an allow_n_plus_1_calls block' do
+ expect { call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS) }.not_to raise_error
+ end
+
+ it 'does not allow the maximum number of calls to be exceeded outside of an allow_n_plus_1_calls block' do
+ expect { call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 1) }.to raise_error(Gitlab::GitalyClient::TooManyInvocationsError)
+ end
+ end
+ end
+
+ context 'when RequestStore is not active' do
+ it 'does not raise errors when the maximum number of allowed calls is exceeded' do
+ expect { call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 2) }.not_to raise_error
+ end
+
+ it 'does not fail when the maximum number of calls is exceeded within an allow_n_plus_1_calls block' do
+ expect do
+ described_class.allow_n_plus_1_calls do
+ call_gitaly(Gitlab::GitalyClient::MAXIMUM_GITALY_CALLS + 1)
+ end
+ end.not_to raise_error
+ end
+ end
+ end
+
+ describe 'get_request_count' do
+ context 'when RequestStore is enabled', :request_store do
+ context 'when enforce_gitaly_request_limits is called outside of allow_n_plus_1_calls blocks' do
+ before do
+ described_class.enforce_gitaly_request_limits(:call)
+ end
+
+ it 'counts gitaly calls' do
+ expect(described_class.get_request_count).to eq(1)
+ end
+ end
+
+ context 'when enforce_gitaly_request_limits is called inside and outside of allow_n_plus_1_calls blocks' do
+ before do
+ described_class.enforce_gitaly_request_limits(:call)
+ described_class.allow_n_plus_1_calls do
+ described_class.enforce_gitaly_request_limits(:call)
+ end
+ end
+
+ it 'counts gitaly calls' do
+ expect(described_class.get_request_count).to eq(2)
+ end
+ end
+
+ context 'when reset_counts is called' do
+ before do
+ described_class.enforce_gitaly_request_limits(:call)
+ described_class.reset_counts
+ end
+
+ it 'resets counts' do
+ expect(described_class.get_request_count).to eq(0)
+ end
+ end
+ end
+
+ context 'when RequestStore is not active' do
+ before do
+ described_class.enforce_gitaly_request_limits(:call)
+ end
+
+ it 'returns zero' do
+ expect(described_class.get_request_count).to eq(0)
+ end
+ end
+ end
+
describe 'feature_enabled?' do
let(:feature_name) { 'my_feature' }
let(:real_feature_name) { "gitaly_#{feature_name}" }
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index 2de8daba6b5..3baf9b1edab 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -103,9 +103,15 @@ describe PipelineSerializer do
let(:project) { create(:project) }
before do
- Ci::Pipeline::AVAILABLE_STATUSES.each do |status|
- create_pipeline(status)
+ # Since RequestStore.active? is true we have to allow the
+ # gitaly calls in this block
+ # Issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/37772
+ Gitlab::GitalyClient.allow_n_plus_1_calls do
+ Ci::Pipeline::AVAILABLE_STATUSES.each do |status|
+ create_pipeline(status)
+ end
end
+ Gitlab::GitalyClient.reset_counts
end
shared_examples 'no N+1 queries' do