Skip to content
Projects
Groups
Snippets
Help
Loading...
Help
Support
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
G
gitlab-ce
Project overview
Project overview
Details
Activity
Releases
Repository
Repository
Files
Commits
Branches
Tags
Contributors
Graph
Compare
Issues
0
Issues
0
List
Boards
Labels
Milestones
Merge Requests
1
Merge Requests
1
Analytics
Analytics
Repository
Value Stream
Wiki
Wiki
Snippets
Snippets
Members
Members
Collapse sidebar
Close sidebar
Activity
Graph
Create a new issue
Commits
Issue Boards
Open sidebar
nexedi
gitlab-ce
Commits
64d7ec0a
Commit
64d7ec0a
authored
Sep 19, 2017
by
Andrew Newdigate
Committed by
Rémy Coutable
Sep 19, 2017
Browse files
Options
Browse Files
Download
Email Patches
Plain Diff
Detect n+1 issues involving Gitaly
parent
39dd7736
Changes
23
Hide whitespace changes
Inline
Side-by-side
Showing
23 changed files
with
400 additions
and
54 deletions
+400
-54
.gitlab-ci.yml
.gitlab-ci.yml
+1
-1
app/controllers/projects/branches_controller.rb
app/controllers/projects/branches_controller.rb
+7
-3
app/controllers/projects/commit_controller.rb
app/controllers/projects/commit_controller.rb
+6
-1
app/controllers/projects/compare_controller.rb
app/controllers/projects/compare_controller.rb
+4
-0
app/controllers/projects/merge_requests/diffs_controller.rb
app/controllers/projects/merge_requests/diffs_controller.rb
+4
-1
app/controllers/projects/merge_requests_controller.rb
app/controllers/projects/merge_requests_controller.rb
+8
-0
app/controllers/projects/network_controller.rb
app/controllers/projects/network_controller.rb
+14
-9
app/controllers/projects/refs_controller.rb
app/controllers/projects/refs_controller.rb
+10
-7
app/controllers/root_controller.rb
app/controllers/root_controller.rb
+4
-1
app/models/merge_request.rb
app/models/merge_request.rb
+5
-2
app/models/network/graph.rb
app/models/network/graph.rb
+5
-2
app/models/repository.rb
app/models/repository.rb
+1
-0
app/services/delete_merged_branches_service.rb
app/services/delete_merged_branches_service.rb
+11
-8
app/services/merge_requests/create_service.rb
app/services/merge_requests/create_service.rb
+4
-1
app/services/notes/create_service.rb
app/services/notes/create_service.rb
+7
-1
config/initializers/lograge.rb
config/initializers/lograge.rb
+6
-1
lib/api/branches.rb
lib/api/branches.rb
+4
-1
lib/api/entities.rb
lib/api/entities.rb
+4
-1
lib/gitlab/diff/file_collection/base.rb
lib/gitlab/diff/file_collection/base.rb
+4
-1
lib/gitlab/git/repository.rb
lib/gitlab/git/repository.rb
+13
-10
lib/gitlab/gitaly_client.rb
lib/gitlab/gitaly_client.rb
+146
-1
spec/lib/gitlab/gitaly_client_spec.rb
spec/lib/gitlab/gitaly_client_spec.rb
+124
-0
spec/serializers/pipeline_serializer_spec.rb
spec/serializers/pipeline_serializer_spec.rb
+8
-2
No files found.
.gitlab-ci.yml
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/branches_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/commit_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/compare_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/merge_requests/diffs_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/merge_requests_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/network_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/controllers/projects/refs_controller.rb
View file @
64d7ec0a
...
...
@@ -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
)
...
...
app/controllers/root_controller.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/models/merge_request.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/models/network/graph.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/models/repository.rb
View file @
64d7ec0a
...
...
@@ -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?
...
...
app/services/delete_merged_branches_service.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
app/services/merge_requests/create_service.rb
View file @
64d7ec0a
...
...
@@ -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
)
...
...
app/services/notes/create_service.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
config/initializers/lograge.rb
View file @
64d7ec0a
...
...
@@ -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
lib/api/branches.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
lib/api/entities.rb
View file @
64d7ec0a
...
...
@@ -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
|
...
...
lib/gitlab/diff/file_collection/base.rb
View file @
64d7ec0a
...
...
@@ -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
)
...
...
lib/gitlab/git/repository.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
lib/gitlab/gitaly_client.rb
View file @
64d7ec0a
...
...
@@ -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
<<
"
\n
The 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
spec/lib/gitlab/gitaly_client_spec.rb
View file @
64d7ec0a
...
...
@@ -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
}
"
}
...
...
spec/serializers/pipeline_serializer_spec.rb
View file @
64d7ec0a
...
...
@@ -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
...
...
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment