From ef90d50356f7b0a3acb627857a1ccb275498beba Mon Sep 17 00:00:00 2001
From: Pedro Pombeiro <noreply@pedro.pombei.ro>
Date: Thu, 4 Jan 2024 09:55:56 +0000
Subject: [PATCH] Fix quarantined N+1 spec
---
app/finders/ci/runners_finder.rb | 2 +-
.../ci/runner_owner_project_resolver.rb | 2 +-
.../resolvers/ci/runner_projects_resolver.rb | 2 +-
app/graphql/resolvers/ci/runner_resolver.rb | 27 +--
app/graphql/resolvers/ci/runners_resolver.rb | 2 +-
spec/factories/ci/runners.rb | 3 +
.../ci/group_runners_resolver_spec.rb | 2 +-
.../ci/project_runners_resolver_spec.rb | 2 +-
.../resolvers/ci/runners_resolver_spec.rb | 10 +-
spec/models/ci/runner_spec.rb | 2 +-
spec/requests/api/graphql/ci/runner_spec.rb | 158 +++++++--------
spec/requests/api/graphql/ci/runners_spec.rb | 183 ++++++++++--------
12 files changed, 201 insertions(+), 194 deletions(-)
diff --git a/app/finders/ci/runners_finder.rb b/app/finders/ci/runners_finder.rb
index 19642f5810405..88402748083fb 100644
--- a/app/finders/ci/runners_finder.rb
+++ b/app/finders/ci/runners_finder.rb
@@ -138,7 +138,7 @@ def sort!
end
def request_tag_list!
- @runners = @runners.with_tags if !@params[:preload].present? || @params.dig(:preload, :tag_name)
+ @runners = @runners.with_tags if @params.exclude?(:preload) || @params.dig(:preload, :tag_name)
end
end
end
diff --git a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb
index f4e044b81c928..28c394278721b 100644
--- a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb
+++ b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb
@@ -34,7 +34,7 @@ def preloads
def resolve_owner
return unless runner.project_type?
- BatchLoader::GraphQL.for(runner.id).batch(key: :runner_owner_projects) do |runner_ids, loader|
+ BatchLoader::GraphQL.for(runner.id).batch do |runner_ids, loader|
# rubocop: disable CodeReuse/ActiveRecord
runner_and_projects_with_row_number =
::Ci::RunnerProject
diff --git a/app/graphql/resolvers/ci/runner_projects_resolver.rb b/app/graphql/resolvers/ci/runner_projects_resolver.rb
index c5037965e2040..99c9bba1bd674 100644
--- a/app/graphql/resolvers/ci/runner_projects_resolver.rb
+++ b/app/graphql/resolvers/ci/runner_projects_resolver.rb
@@ -28,7 +28,7 @@ def resolve_with_lookahead(**args)
return unless runner.project_type?
# rubocop:disable CodeReuse/ActiveRecord
- BatchLoader::GraphQL.for(runner.id).batch(key: :runner_projects) do |runner_ids, loader|
+ BatchLoader::GraphQL.for(runner.id).batch do |runner_ids, loader|
plucked_runner_and_project_ids = ::Ci::RunnerProject
.select(:runner_id, :project_id)
.where(runner_id: runner_ids)
diff --git a/app/graphql/resolvers/ci/runner_resolver.rb b/app/graphql/resolvers/ci/runner_resolver.rb
index 4250b069d20ec..60fb4163afeb6 100644
--- a/app/graphql/resolvers/ci/runner_resolver.rb
+++ b/app/graphql/resolvers/ci/runner_resolver.rb
@@ -6,13 +6,12 @@ class RunnerResolver < BaseResolver
include LooksAhead
type Types::Ci::RunnerType, null: true
- extras [:lookahead]
description 'Runner information.'
argument :id,
- type: ::Types::GlobalIDType[::Ci::Runner],
- required: true,
- description: 'Runner ID.'
+ type: ::Types::GlobalIDType[::Ci::Runner],
+ required: true,
+ description: 'Runner ID.'
def resolve_with_lookahead(id:)
find_runner(id: id)
@@ -21,19 +20,13 @@ def resolve_with_lookahead(id:)
private
def find_runner(id:)
- runner_id = GitlabSchema.parse_gid(id, expected_type: ::Ci::Runner).model_id.to_i
- key = {
- preload_tag_list: lookahead.selects?(:tag_list),
- preload_creator: lookahead.selects?(:created_by)
- }
-
- BatchLoader::GraphQL.for(runner_id).batch(key: key) do |ids, loader, batch|
- results = ::Ci::Runner.id_in(ids)
- results = results.with_tags if batch[:key][:preload_tag_list]
- results = results.with_creator if batch[:key][:preload_creator]
-
- results.each { |record| loader.call(record.id, record) }
- end
+ preloads = []
+ preloads << :creator if lookahead.selects?(:created_by)
+ preloads << :tags if lookahead.selects?(:tag_list)
+
+ runner_id = GitlabSchema.parse_gid(id, expected_type: ::Ci::Runner).model_id
+
+ ::Gitlab::Graphql::Loaders::BatchModelLoader.new(::Ci::Runner, runner_id, preloads).find
end
end
end
diff --git a/app/graphql/resolvers/ci/runners_resolver.rb b/app/graphql/resolvers/ci/runners_resolver.rb
index 9121c413b1f20..38d2ebe046bad 100644
--- a/app/graphql/resolvers/ci/runners_resolver.rb
+++ b/app/graphql/resolvers/ci/runners_resolver.rb
@@ -82,7 +82,7 @@ def runners_finder_params(params)
creator_id:
params[:creator_id] ? ::GitlabSchema.parse_gid(params[:creator_id], expected_type: ::User).model_id : nil,
version_prefix: params[:version_prefix],
- preload: false # we'll handle preloading ourselves
+ preload: {} # we'll handle preloading ourselves
}.compact
.merge(parent_param)
end
diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb
index 2d67a4c0e800f..63e8cec82e6c0 100644
--- a/spec/factories/ci/runners.rb
+++ b/spec/factories/ci/runners.rb
@@ -14,6 +14,7 @@
groups { [] }
projects { [] }
token_expires_at { nil }
+ creator { nil }
end
after(:build) do |runner, evaluator|
@@ -24,6 +25,8 @@
evaluator.groups.each do |group|
runner.runner_namespaces << build(:ci_runner_namespace, runner: runner, namespace: group)
end
+
+ runner.creator = evaluator.creator if evaluator.creator
end
after(:create) do |runner, evaluator|
diff --git a/spec/graphql/resolvers/ci/group_runners_resolver_spec.rb b/spec/graphql/resolvers/ci/group_runners_resolver_spec.rb
index d1eec0baeeaf3..d1726c8da6cb8 100644
--- a/spec/graphql/resolvers/ci/group_runners_resolver_spec.rb
+++ b/spec/graphql/resolvers/ci/group_runners_resolver_spec.rb
@@ -87,7 +87,7 @@
status_status: 'active',
type_type: :group_type,
tag_name: ['active_runner'],
- preload: false,
+ preload: {},
search: 'abc',
sort: 'contacted_asc',
membership: :descendants,
diff --git a/spec/graphql/resolvers/ci/project_runners_resolver_spec.rb b/spec/graphql/resolvers/ci/project_runners_resolver_spec.rb
index 85b5552117448..59ba7d4200c53 100644
--- a/spec/graphql/resolvers/ci/project_runners_resolver_spec.rb
+++ b/spec/graphql/resolvers/ci/project_runners_resolver_spec.rb
@@ -74,7 +74,7 @@
status_status: 'active',
type_type: :group_type,
tag_name: ['active_runner'],
- preload: false,
+ preload: {},
search: 'abc',
sort: 'contacted_asc',
project: project
diff --git a/spec/graphql/resolvers/ci/runners_resolver_spec.rb b/spec/graphql/resolvers/ci/runners_resolver_spec.rb
index 85a90924384b6..a0239a6ff344f 100644
--- a/spec/graphql/resolvers/ci/runners_resolver_spec.rb
+++ b/spec/graphql/resolvers/ci/runners_resolver_spec.rb
@@ -98,7 +98,7 @@
upgrade_status: 'recommended',
type_type: :instance_type,
tag_name: ['active_runner'],
- preload: false,
+ preload: {},
search: 'abc',
sort: 'contacted_asc',
creator_id: '1',
@@ -125,7 +125,7 @@
let(:expected_params) do
{
active: false,
- preload: false
+ preload: {}
}
end
@@ -145,7 +145,7 @@
let(:expected_params) do
{
active: false,
- preload: false
+ preload: {}
}
end
@@ -163,7 +163,7 @@
end
let(:expected_params) do
- { preload: false }
+ { preload: {} }
end
it 'calls RunnersFinder with expected arguments' do
@@ -181,7 +181,7 @@
let(:expected_params) do
{
- preload: false,
+ preload: {},
version_prefix: 'a.b'
}
end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 8c4d5fef24dc4..d4f7db3bddd81 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -532,7 +532,7 @@
let_it_be(:runner3) { create(:ci_runner, creator_id: 1) }
let_it_be(:runner4) { create(:ci_runner, creator_id: nil) }
- it 'returns runners with creator_id \'1\'' do
+ it "returns runners with creator_id '1'" do
is_expected.to contain_exactly(runner2, runner3)
end
end
diff --git a/spec/requests/api/graphql/ci/runner_spec.rb b/spec/requests/api/graphql/ci/runner_spec.rb
index 8262640b28339..1b6948d0380f4 100644
--- a/spec/requests/api/graphql/ci/runner_spec.rb
+++ b/spec/requests/api/graphql/ci/runner_spec.rb
@@ -876,107 +876,95 @@
end
describe 'Query limits' do
- def runner_query(runner)
- <<~SINGLE
- runner(id: "#{runner.to_global_id}") {
- #{all_graphql_fields_for('CiRunner', excluded: excluded_fields)}
- createdBy {
- id
- username
- webPath
- webUrl
- }
- groups {
- nodes {
- id
- path
- fullPath
- webUrl
- }
- }
- projects {
- nodes {
- id
- path
- fullPath
- webUrl
- }
- }
- ownerProject {
- id
- path
- fullPath
- webUrl
- }
+ let_it_be(:user2) { another_admin }
+ let_it_be(:user3) { create(:user) }
+ let_it_be(:tag_list) { %w[n_plus_1_test some_tag] }
+ let_it_be(:args) do
+ { current_user: user, token: { personal_access_token: create(:personal_access_token, user: user) } }
+ end
+
+ let_it_be(:runner1) { create(:ci_runner, tag_list: tag_list, creator: user) }
+ let_it_be(:runner2) do
+ create(:ci_runner, :group, groups: [group], tag_list: tag_list, creator: user)
+ end
+
+ let_it_be(:runner3) do
+ create(:ci_runner, :project, projects: [project1], tag_list: tag_list, creator: user)
+ end
+
+ let(:single_discrete_runners_query) do
+ multiple_discrete_runners_query([])
+ end
+
+ let(:runner_fragment) do
+ <<~QUERY
+ #{all_graphql_fields_for('CiRunner', excluded: excluded_fields)}
+ createdBy {
+ id
+ username
+ webPath
+ webUrl
}
- SINGLE
+ QUERY
end
- let(:active_project_runner2) { create(:ci_runner, :project) }
- let(:active_group_runner2) { create(:ci_runner, :group) }
+ # Exclude fields that are already hardcoded above (or tested separately),
+ # and also some fields from deeper objects which are problematic:
+ # - createdBy: Known N+1 issues, but only on exotic fields which we don't normally use
+ # - ownerProject.pipeline: Needs arguments (iid or sha)
+ # - project.productAnalyticsState: Can be requested only for 1 Project(s) at a time.
+ let(:excluded_fields) { %w[createdBy jobs pipeline productAnalyticsState] }
+
+ it 'avoids N+1 queries', :use_sql_query_cache do
+ discrete_runners_control = ActiveRecord::QueryRecorder.new(skip_cached: false) do
+ post_graphql(single_discrete_runners_query, **args)
+ end
+
+ additional_runners = setup_additional_records
+
+ expect do
+ post_graphql(multiple_discrete_runners_query(additional_runners), **args)
- # Exclude fields that are already hardcoded above
- let(:excluded_fields) { %w[createdBy jobs groups projects ownerProject] }
+ raise StandardError, flattened_errors if graphql_errors # Ensure any error in query causes test to fail
+ end.not_to exceed_query_limit(discrete_runners_control)
+ end
- let(:single_query) do
+ def runner_query(runner, nr)
<<~QUERY
- {
- instance_runner1: #{runner_query(active_instance_runner)}
- group_runner1: #{runner_query(active_group_runner)}
- project_runner1: #{runner_query(active_project_runner)}
+ runner#{nr}: runner(id: "#{runner.to_global_id}") {
+ #{runner_fragment}
}
QUERY
end
- let(:double_query) do
+ def multiple_discrete_runners_query(additional_runners)
<<~QUERY
{
- instance_runner1: #{runner_query(active_instance_runner)}
- instance_runner2: #{runner_query(inactive_instance_runner)}
- group_runner1: #{runner_query(active_group_runner)}
- group_runner2: #{runner_query(active_group_runner2)}
- project_runner1: #{runner_query(active_project_runner)}
- project_runner2: #{runner_query(active_project_runner2)}
+ #{runner_query(runner1, 1)}
+ #{runner_query(runner2, 2)}
+ #{runner_query(runner3, 3)}
+ #{additional_runners.each_with_index.map { |r, i| runner_query(r, 4 + i) }.join("\n")}
}
QUERY
end
- it 'does not execute more queries per runner', :aggregate_failures, quarantine: "https://gitlab.com/gitlab-org/gitlab/-/issues/391442" do
- # warm-up license cache and so on:
- personal_access_token = create(:personal_access_token, user: user)
- args = { current_user: user, token: { personal_access_token: personal_access_token } }
- post_graphql(double_query, **args)
-
- control = ActiveRecord::QueryRecorder.new { post_graphql(single_query, **args) }
-
- personal_access_token = create(:personal_access_token, user: another_admin)
- args = { current_user: another_admin, token: { personal_access_token: personal_access_token } }
- expect { post_graphql(double_query, **args) }.not_to exceed_query_limit(control)
-
- expect(graphql_data.count).to eq 6
- expect(graphql_data).to match(
- a_hash_including(
- 'instance_runner1' => a_graphql_entity_for(active_instance_runner),
- 'instance_runner2' => a_graphql_entity_for(inactive_instance_runner),
- 'group_runner1' => a_graphql_entity_for(
- active_group_runner,
- groups: { 'nodes' => contain_exactly(a_graphql_entity_for(group)) }
- ),
- 'group_runner2' => a_graphql_entity_for(
- active_group_runner2,
- groups: { 'nodes' => active_group_runner2.groups.map { |g| a_graphql_entity_for(g) } }
- ),
- 'project_runner1' => a_graphql_entity_for(
- active_project_runner,
- projects: { 'nodes' => active_project_runner.projects.map { |p| a_graphql_entity_for(p) } },
- owner_project: a_graphql_entity_for(active_project_runner.projects[0])
- ),
- 'project_runner2' => a_graphql_entity_for(
- active_project_runner2,
- projects: { 'nodes' => active_project_runner2.projects.map { |p| a_graphql_entity_for(p) } },
- owner_project: a_graphql_entity_for(active_project_runner2.projects[0])
- )
- ))
+ def setup_additional_records
+ # Add more runners (including owned by other users)
+ runner4 = create(:ci_runner, tag_list: tag_list + %w[tag1 tag2], creator: user2)
+ runner5 = create(:ci_runner, :group, groups: [create(:group)], tag_list: tag_list + %w[tag2 tag3], creator: user3)
+ # Add one more project to runner
+ runner3.assign_to(create(:project))
+
+ # Add more runner managers (including to existing runners)
+ runner_manager1 = create(:ci_runner_machine, runner: runner1)
+ create(:ci_runner_machine, runner: runner1)
+ create(:ci_runner_machine, runner: runner2, system_xid: runner_manager1.system_xid)
+ create(:ci_runner_machine, runner: runner3)
+ create(:ci_runner_machine, runner: runner4, version: '16.4.1')
+ create(:ci_runner_machine, runner: runner5, version: '16.4.0', system_xid: runner_manager1.system_xid)
+ create(:ci_runner_machine, runner: runner3)
+
+ [runner4, runner5]
end
end
diff --git a/spec/requests/api/graphql/ci/runners_spec.rb b/spec/requests/api/graphql/ci/runners_spec.rb
index 0fe14bef7787d..189106fae7b38 100644
--- a/spec/requests/api/graphql/ci/runners_spec.rb
+++ b/spec/requests/api/graphql/ci/runners_spec.rb
@@ -18,22 +18,34 @@
let(:fields) do
<<~QUERY
nodes {
- #{all_graphql_fields_for('CiRunner', excluded: %w[createdBy ownerProject])}
- createdBy {
- username
- webPath
- webUrl
- }
- ownerProject {
- id
- path
- fullPath
- webUrl
- }
+ #{all_graphql_fields_for('CiRunner', excluded: excluded_fields)}
}
QUERY
end
+ let(:query) do
+ %(
+ query {
+ runners {
+ #{fields}
+ }
+ }
+ )
+ end
+
+ # Exclude fields from deeper objects which are problematic:
+ # - ownerProject.pipeline: Needs arguments (iid or sha)
+ # - project.productAnalyticsState: Can be requested only for 1 Project(s) at a time.
+ let(:excluded_fields) { %w[pipeline productAnalyticsState] }
+
+ it 'returns expected runners' do
+ post_graphql(query, current_user: current_user)
+
+ expect(runners_graphql_data['nodes']).to contain_exactly(
+ *Ci::Runner.all.map { |expected_runner| a_graphql_entity_for(expected_runner) }
+ )
+ end
+
context 'with filters' do
shared_examples 'a working graphql query returning expected runners' do
it_behaves_like 'a working graphql query' do
@@ -49,31 +61,6 @@
*Array(expected_runners).map { |expected_runner| a_graphql_entity_for(expected_runner) }
)
end
-
- it 'does not execute more queries per runner', :aggregate_failures do
- # warm-up license cache and so on:
- personal_access_token = create(:personal_access_token, user: current_user)
- args = { current_user: current_user, token: { personal_access_token: personal_access_token } }
- post_graphql(query, **args)
- expect(graphql_data_at(:runners, :nodes)).not_to be_empty
-
- admin2 = create(:admin)
- personal_access_token = create(:personal_access_token, user: admin2)
- args = { current_user: admin2, token: { personal_access_token: personal_access_token } }
- control = ActiveRecord::QueryRecorder.new { post_graphql(query, **args) }
-
- runner2 = create(:ci_runner, :instance, version: '14.0.0', tag_list: %w[tag5 tag6], creator: admin2)
- runner3 = create(:ci_runner, :project, version: '14.0.1', projects: [project], tag_list: %w[tag3 tag8],
- creator: current_user)
-
- create(:ci_build, :failed, runner: runner2)
- create(:ci_runner_machine, runner: runner2, version: '16.4.1')
-
- create(:ci_build, :failed, runner: runner3)
- create(:ci_runner_machine, runner: runner3, version: '16.4.0')
-
- expect { post_graphql(query, **args) }.not_to exceed_query_limit(control)
- end
end
context 'when filtered on type and status' do
@@ -179,52 +166,88 @@ def create_ci_runner(args = {}, version:)
end
end
end
+ end
- context 'without filters' do
- context 'with managers requested for multiple runners' do
- let(:fields) do
- <<~QUERY
- nodes {
- managers {
- nodes {
- #{all_graphql_fields_for('CiRunnerManager', max_depth: 1)}
- }
- }
- }
- QUERY
- end
+ describe 'Runner query limits' do
+ let_it_be(:user) { create(:user, :admin) }
+ let_it_be(:user2) { create(:user) }
+ let_it_be(:user3) { create(:user) }
+ let_it_be(:group) { create(:group) }
+ let_it_be(:project) { create(:project) }
+ let_it_be(:tag_list) { %w[n_plus_1_test some_tag] }
+ let_it_be(:args) do
+ { current_user: user, token: { personal_access_token: create(:personal_access_token, user: user) } }
+ end
- let(:query) do
- %(
- query {
- runners {
- #{fields}
- }
- }
- )
- end
+ let_it_be(:runner1) { create(:ci_runner, tag_list: tag_list, creator: user) }
+ let_it_be(:runner2) do
+ create(:ci_runner, :group, groups: [group], tag_list: tag_list, creator: user)
+ end
- it 'does not execute more queries per runner', :aggregate_failures do
- # warm-up license cache and so on:
- personal_access_token = create(:personal_access_token, user: current_user)
- args = { current_user: current_user, token: { personal_access_token: personal_access_token } }
- post_graphql(query, **args)
- expect(graphql_data_at(:runners, :nodes)).not_to be_empty
-
- admin2 = create(:admin)
- personal_access_token = create(:personal_access_token, user: admin2)
- args = { current_user: admin2, token: { personal_access_token: personal_access_token } }
- control = ActiveRecord::QueryRecorder.new { post_graphql(query, **args) }
-
- create(:ci_runner, :instance, :with_runner_manager, version: '14.0.0', tag_list: %w[tag5 tag6],
- creator: admin2)
- create(:ci_runner, :project, :with_runner_manager, version: '14.0.1', projects: [project],
- tag_list: %w[tag3 tag8],
- creator: current_user)
-
- expect { post_graphql(query, **args) }.not_to exceed_query_limit(control)
- end
- end
+ let_it_be(:runner3) do
+ create(:ci_runner, :project, projects: [project], tag_list: tag_list, creator: user)
+ end
+
+ let(:runner_fragment) do
+ <<~QUERY
+ #{all_graphql_fields_for('CiRunner', excluded: excluded_fields)}
+ createdBy {
+ id
+ username
+ webPath
+ webUrl
+ }
+ QUERY
+ end
+
+ # Exclude fields that are already hardcoded above (or tested separately),
+ # and also some fields from deeper objects which are problematic:
+ # - createdBy: Known N+1 issues, but only on exotic fields which we don't normally use
+ # - ownerProject.pipeline: Needs arguments (iid or sha)
+ # - project.productAnalyticsState: Can be requested only for 1 Project(s) at a time.
+ let(:excluded_fields) { %w[createdBy jobs pipeline productAnalyticsState] }
+
+ let(:runners_query) do
+ <<~QUERY
+ {
+ runners {
+ nodes { #{runner_fragment} }
+ }
+ }
+ QUERY
+ end
+
+ it 'avoids N+1 queries', :use_sql_query_cache do
+ personal_access_token = create(:personal_access_token, user: user)
+ args = { current_user: user, token: { personal_access_token: personal_access_token } }
+
+ runners_control = ActiveRecord::QueryRecorder.new(skip_cached: false) { post_graphql(runners_query, **args) }
+
+ setup_additional_records
+
+ expect { post_graphql(runners_query, **args) }.not_to exceed_query_limit(runners_control)
+ end
+
+ def setup_additional_records
+ # Add more runners (including owned by other users)
+ runner4 = create(:ci_runner, tag_list: tag_list + %w[tag1 tag2], creator: user2)
+ runner5 = create(:ci_runner, :group, groups: [create(:group)], tag_list: tag_list + %w[tag2 tag3], creator: user3)
+ # Add one more project to runner
+ runner3.assign_to(create(:project))
+
+ # Add more runner managers (including to existing runners)
+ runner_manager1 = create(:ci_runner_machine, runner: runner1)
+ create(:ci_runner_machine, runner: runner1)
+ create(:ci_runner_machine, runner: runner2, system_xid: runner_manager1.system_xid)
+ create(:ci_runner_machine, runner: runner3)
+ create(:ci_runner_machine, runner: runner4, version: '16.4.1')
+ create(:ci_runner_machine, runner: runner5, version: '16.4.0', system_xid: runner_manager1.system_xid)
+ create(:ci_runner_machine, runner: runner3)
+
+ create(:ci_build, :failed, runner: runner4)
+ create(:ci_build, :failed, runner: runner5)
+
+ [runner4, runner5]
end
end
--
GitLab