Commit 404a5623 authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'dev_time_failure_on_haml_to_gitaly' into 'master'

Detect n+1 issues involving Gitaly

See merge request gitlab-org/gitlab-ce!13996
parents 39dd7736 64d7ec0a
...@@ -128,7 +128,7 @@ stages: ...@@ -128,7 +128,7 @@ stages:
- export CACHE_CLASSES=true - export CACHE_CLASSES=true
- cp ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} ${KNAPSACK_REPORT_PATH} - cp ${KNAPSACK_SPINACH_SUITE_REPORT_PATH} ${KNAPSACK_REPORT_PATH}
- scripts/gitaly-test-spawn - 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: artifacts:
expire_in: 31d expire_in: 31d
when: always when: always
......
...@@ -15,10 +15,14 @@ class Projects::BranchesController < Projects::ApplicationController ...@@ -15,10 +15,14 @@ class Projects::BranchesController < Projects::ApplicationController
respond_to do |format| respond_to do |format|
format.html do format.html do
@refs_pipelines = @project.pipelines.latest_successful_for_refs(@branches.map(&:name)) @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| render
diverging_commit_counts = repository.diverging_commit_counts(branch)
[memo, diverging_commit_counts[:behind], diverging_commit_counts[:ahead]].max
end end
end end
format.json do format.json do
......
...@@ -20,7 +20,12 @@ class Projects::CommitController < Projects::ApplicationController ...@@ -20,7 +20,12 @@ class Projects::CommitController < Projects::ApplicationController
apply_diff_view_cookie! apply_diff_view_cookie!
respond_to do |format| 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.diff { render text: @commit.to_diff }
format.patch { render text: @commit.to_patch } format.patch { render text: @commit.to_patch }
end end
......
...@@ -17,6 +17,10 @@ class Projects::CompareController < Projects::ApplicationController ...@@ -17,6 +17,10 @@ class Projects::CompareController < Projects::ApplicationController
def show def show
apply_diff_view_cookie! 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 end
def diff_for_path def diff_for_path
......
...@@ -10,7 +10,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -10,7 +10,10 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
def show def show
@environment = @merge_request.environments_for(current_user).last @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 end
def diff_for_path def diff_for_path
......
...@@ -56,6 +56,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -56,6 +56,9 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
close_merge_request_without_source_project close_merge_request_without_source_project
check_if_can_be_merged check_if_can_be_merged
# Return if the response has already been rendered
return if response_body
respond_to do |format| respond_to do |format|
format.html do format.html do
# Build a note object for comment form # Build a note object for comment form
...@@ -70,6 +73,11 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -70,6 +73,11 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
labels labels
set_pipeline_variables 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 end
format.json do format.json do
......
...@@ -8,19 +8,24 @@ class Projects::NetworkController < Projects::ApplicationController ...@@ -8,19 +8,24 @@ class Projects::NetworkController < Projects::ApplicationController
before_action :assign_commit before_action :assign_commit
def show def show
@url = project_network_path(@project, @ref, @options.merge(format: :json)) # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37602
@commit_url = project_commit_path(@project, 'ae45ca32').gsub("ae45ca32", "%s") 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| respond_to do |format|
format.html do format.html do
if @options[:extended_sha1] && !@commit if @options[:extended_sha1] && !@commit
flash.now[:alert] = "Git revision '#{@options[:extended_sha1]}' does not exist." flash.now[:alert] = "Git revision '#{@options[:extended_sha1]}' does not exist."
end
end end
end
format.json do format.json do
@graph = Network::Graph.new(project, @ref, @commit, @options[:filter_ref]) @graph = Network::Graph.new(project, @ref, @commit, @options[:filter_ref])
end
end end
render
end end
end end
......
...@@ -51,13 +51,16 @@ class Projects::RefsController < Projects::ApplicationController ...@@ -51,13 +51,16 @@ class Projects::RefsController < Projects::ApplicationController
contents.push(*tree.blobs) contents.push(*tree.blobs)
contents.push(*tree.submodules) contents.push(*tree.submodules)
@logs = contents[@offset, @limit].to_a.map do |content| # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37433
file = @path ? File.join(@path, content.name) : content.name @logs = Gitlab::GitalyClient.allow_n_plus_1_calls do
last_commit = @repo.last_commit_for_path(@commit.id, file) contents[@offset, @limit].to_a.map do |content|
{ file = @path ? File.join(@path, content.name) : content.name
file_name: content.name, last_commit = @repo.last_commit_for_path(@commit.id, file)
commit: last_commit {
} file_name: content.name,
commit: last_commit
}
end
end end
offset = (@offset + @limit) offset = (@offset + @limit)
......
...@@ -13,7 +13,10 @@ class RootController < Dashboard::ProjectsController ...@@ -13,7 +13,10 @@ class RootController < Dashboard::ProjectsController
before_action :redirect_logged_user, if: -> { current_user.present? } before_action :redirect_logged_user, if: -> { current_user.present? }
def index 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 end
private private
......
...@@ -415,8 +415,11 @@ class MergeRequest < ActiveRecord::Base ...@@ -415,8 +415,11 @@ class MergeRequest < ActiveRecord::Base
end end
def create_merge_request_diff def create_merge_request_diff
merge_request_diffs.create # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37435
reload_merge_request_diff Gitlab::GitalyClient.allow_n_plus_1_calls do
merge_request_diffs.create
reload_merge_request_diff
end
end end
def reload_merge_request_diff def reload_merge_request_diff
......
...@@ -61,8 +61,11 @@ module Network ...@@ -61,8 +61,11 @@ module Network
@reserved[i] = [] @reserved[i] = []
end end
commits_sort_by_ref.each do |commit| # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37436
place_chain(commit) Gitlab::GitalyClient.allow_n_plus_1_calls do
commits_sort_by_ref.each do |commit|
place_chain(commit)
end
end end
# find parent spaces for not overlap lines # find parent spaces for not overlap lines
......
...@@ -983,6 +983,7 @@ class Repository ...@@ -983,6 +983,7 @@ class Repository
def empty_repo? def empty_repo?
!exists? || !has_visible_content? !exists? || !has_visible_content?
end end
cache_method :empty_repo?, memoize_only: true
def search_files_by_content(query, ref) def search_files_by_content(query, ref)
return [] if empty_repo? || query.blank? return [] if empty_repo? || query.blank?
......
...@@ -6,15 +6,18 @@ class DeleteMergedBranchesService < BaseService ...@@ -6,15 +6,18 @@ class DeleteMergedBranchesService < BaseService
def execute def execute
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :push_code, project) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :push_code, project)
branches = project.repository.branch_names # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37438
branches = branches.select { |branch| project.repository.merged_to_root_ref?(branch) } Gitlab::GitalyClient.allow_n_plus_1_calls do
# Prevent deletion of branches relevant to open merge requests branches = project.repository.branch_names
branches -= merge_request_branch_names branches = branches.select { |branch| project.repository.merged_to_root_ref?(branch) }
# Prevent deletion of protected branches # Prevent deletion of branches relevant to open merge requests
branches = branches.reject { |branch| project.protected_for?(branch) } branches -= merge_request_branch_names
# Prevent deletion of protected branches
branches = branches.reject { |branch| project.protected_for?(branch) }
branches.each do |branch| branches.each do |branch|
DeleteBranchService.new(project, current_user).execute(branch) DeleteBranchService.new(project, current_user).execute(branch)
end
end end
end end
......
...@@ -13,7 +13,10 @@ module MergeRequests ...@@ -13,7 +13,10 @@ module MergeRequests
merge_request.source_branch = params[:source_branch] merge_request.source_branch = params[:source_branch]
merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_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 end
def before_create(merge_request) def before_create(merge_request)
......
...@@ -4,7 +4,13 @@ module Notes ...@@ -4,7 +4,13 @@ module Notes
merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha) merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
note = Notes::BuildService.new(project, current_user, params).execute 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 # We execute commands (extracted from `params[:note]`) on the noteable
# **before** we save the note because if the note consists of commands # **before** we save the note because if the note consists of commands
......
...@@ -12,13 +12,18 @@ unless Sidekiq.server? ...@@ -12,13 +12,18 @@ unless Sidekiq.server?
config.lograge.logger = ActiveSupport::Logger.new(filename) config.lograge.logger = ActiveSupport::Logger.new(filename)
# Add request parameters to log output # Add request parameters to log output
config.lograge.custom_options = lambda do |event| config.lograge.custom_options = lambda do |event|
{ payload = {
time: event.time.utc.iso8601(3), time: event.time.utc.iso8601(3),
params: event.payload[:params].except(*%w(controller action format)), params: event.payload[:params].except(*%w(controller action format)),
remote_ip: event.payload[:remote_ip], remote_ip: event.payload[:remote_ip],
user_id: event.payload[:user_id], user_id: event.payload[:user_id],
username: event.payload[:username] username: event.payload[:username]
} }
gitaly_calls = Gitlab::GitalyClient.get_request_count
payload[:gitaly_calls] = gitaly_calls if gitaly_calls > 0
payload
end end
end end
end end
...@@ -21,7 +21,10 @@ module API ...@@ -21,7 +21,10 @@ module API
get ':id/repository/branches' do get ':id/repository/branches' do
branches = ::Kaminari.paginate_array(user_project.repository.branches.sort_by(&:name)) 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 end
resource ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do resource ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
......
...@@ -244,7 +244,10 @@ module API ...@@ -244,7 +244,10 @@ module API
end end
expose :merged do |repo_branch, options| 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 end
expose :protected do |repo_branch, options| expose :protected do |repo_branch, options|
......
...@@ -22,7 +22,10 @@ module Gitlab ...@@ -22,7 +22,10 @@ module Gitlab
end end
def diff_files 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 end
def diff_file_with_old_path(old_path) def diff_file_with_old_path(old_path)
......
...@@ -950,8 +950,8 @@ module Gitlab ...@@ -950,8 +950,8 @@ module Gitlab
@gitaly_repository_client ||= Gitlab::GitalyClient::RepositoryService.new(self) @gitaly_repository_client ||= Gitlab::GitalyClient::RepositoryService.new(self)
end end
def gitaly_migrate(method, &block) def gitaly_migrate(method, status: Gitlab::GitalyClient::MigrationStatus::OPT_IN, &block)
Gitlab::GitalyClient.migrate(method, &block) Gitlab::GitalyClient.migrate(method, status: status, &block)
rescue GRPC::NotFound => e rescue GRPC::NotFound => e
raise NoRepository.new(e) raise NoRepository.new(e)
rescue GRPC::BadStatus => e rescue GRPC::BadStatus => e
...@@ -962,14 +962,17 @@ module Gitlab ...@@ -962,14 +962,17 @@ module Gitlab
# Gitaly note: JV: Trying to get rid of the 'filter' option so we can implement this with 'git'. # 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) def branches_filter(filter: nil, sort_by: nil)
branches = rugged.branches.each(filter).map do |rugged_ref| # n+1: https://gitlab.com/gitlab-org/gitlab-ce/issues/37464
begin branches = Gitlab::GitalyClient.allow_n_plus_1_calls do
target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target) rugged.branches.each(filter).map do |rugged_ref|
Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit) begin
rescue Rugged::ReferenceError target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
# Omit invalid branch Gitlab::Git::Branch.new(self, rugged_ref.name, rugged_ref.target, target_commit)
end rescue Rugged::ReferenceError
end.compact # Omit invalid branch
end
end.compact
end
sort_branches(branches, sort_by) sort_branches(branches, sort_by)
end end
......
...@@ -10,7 +10,24 @@ module Gitlab ...@@ -10,7 +10,24 @@ module Gitlab
OPT_OUT = 3 OPT_OUT = 3
end 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 SERVER_VERSION_FILE = 'GITALY_SERVER_VERSION'.freeze
MAXIMUM_GITALY_CALLS = 30
MUTEX = Mutex.new MUTEX = Mutex.new
private_constant :MUTEX private_constant :MUTEX
...@@ -53,6 +70,8 @@ module Gitlab ...@@ -53,6 +70,8 @@ module Gitlab
# All Gitaly RPC call sites should use GitalyClient.call. This method # All Gitaly RPC call sites should use GitalyClient.call. This method
# makes sure that per-request authentication headers are set. # makes sure that per-request authentication headers are set.
def self.call(storage, service, rpc, request) def self.call(storage, service, rpc, request)
enforce_gitaly_request_limits(:call)
metadata = request_metadata(storage) metadata = request_metadata(storage)
metadata = yield(metadata) if block_given? metadata = yield(metadata) if block_given?
stub(service, storage).__send__(rpc, request, metadata) # rubocop:disable GitlabSecurity/PublicSend stub(service, storage).__send__(rpc, request, metadata) # rubocop:disable GitlabSecurity/PublicSend
...@@ -107,12 +126,100 @@ module Gitlab ...@@ -107,12 +126,100 @@ module Gitlab
private_class_method :opt_into_all_features? private_class_method :opt_into_all_features?
def self.migrate(feature, status: MigrationStatus::OPT_IN) 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) is_enabled = feature_enabled?(feature, status: status)
metric_name = feature.to_s metric_name = feature.to_s
metric_name += "_gitaly" if is_enabled metric_name += "_gitaly" if is_enabled
Gitlab::Metrics.measure(metric_name) do 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
end end
...@@ -124,5 +231,43 @@ module Gitlab ...@@ -124,5 +231,43 @@ module Gitlab
def self.encode(s) def self.encode(s)
s.dup.force_encoding(Encoding::ASCII_8BIT) s.dup.force_encoding(Encoding::ASCII_8BIT)
end 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
end end
...@@ -38,6 +38,130 @@ describe Gitlab::GitalyClient, skip_gitaly_mock: true do ...@@ -38,6 +38,130 @@ describe Gitlab::GitalyClient, skip_gitaly_mock: true do
end end
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 describe 'feature_enabled?' do
let(:feature_name) { 'my_feature' } let(:feature_name) { 'my_feature' }
let(:real_feature_name) { "gitaly_#{feature_name}" } let(:real_feature_name) { "gitaly_#{feature_name}" }
......
...@@ -103,9 +103,15 @@ describe PipelineSerializer do ...@@ -103,9 +103,15 @@ describe PipelineSerializer do
let(:project) { create(:project) } let(:project) { create(:project) }
before do before do
Ci::Pipeline::AVAILABLE_STATUSES.each do |status| # Since RequestStore.active? is true we have to allow the
create_pipeline(status) # 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 end
Gitlab::GitalyClient.reset_counts
end end
shared_examples 'no N+1 queries' do shared_examples 'no N+1 queries' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment