Commit fa6f5ac6 authored by Fabio Pitino's avatar Fabio Pitino

Shortcircuit expensive queries in Runner#can_pick?

When `can_pick?` is executed by a shared runner
we already know it is one of the assignable runners
for the given project_id. We don't need to run the
`assignable_for?` queries which are expensive.
parent b7414d92
......@@ -693,7 +693,7 @@ module Ci
end
def any_runners_online?
project.any_runners? { |runner| runner.active? && runner.online? && runner.can_pick?(self) }
project.any_active_runners? { |runner| runner.match_build_if_online?(self) }
end
def stuck?
......
......@@ -252,11 +252,21 @@ module Ci
runner_projects.any?
end
# TODO: remove this method in favor of `matches_build?` once feature flag is removed
# https://gitlab.com/gitlab-org/gitlab/-/issues/323317
def can_pick?(build)
# Run `matches_build?` checks before, since they are cheaper than
# `assignable_for?`.
#
matches_build?(build) && assignable_for?(build.project_id)
if Feature.enabled?(:ci_runners_short_circuit_assignable_for, self, default_enabled: :yaml)
matches_build?(build)
else
# Run `matches_build?` checks before, since they are cheaper than
# `assignable_for?`.
#
matches_build?(build) && assignable_for?(build.project_id)
end
end
def match_build_if_online?(build)
active? && online? && can_pick?(build)
end
def only_for?(project)
......@@ -355,6 +365,8 @@ module Ci
end
end
# TODO: remove this method once feature flag ci_runners_short_circuit_assignable_for
# is removed. https://gitlab.com/gitlab-org/gitlab/-/issues/323317
def assignable_for?(project_id)
self.class.owned_or_instance_wide(project_id).where(id: self.id).any?
end
......@@ -383,7 +395,6 @@ module Ci
end
end
# TODO: choose a better name and consider splitting this method into two
def matches_build?(build)
return false if self.ref_protected? && !build.protected?
......
......@@ -1697,8 +1697,8 @@ class Project < ApplicationRecord
end
end
def any_runners?(&block)
active_runners.any?(&block)
def any_active_runners?(&block)
active_runners_with_tags.any?(&block)
end
def valid_runners_token?(token)
......@@ -2699,6 +2699,12 @@ class Project < ApplicationRecord
def cache_has_external_issue_tracker
update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) if Gitlab::Database.read_write?
end
def active_runners_with_tags
strong_memoize(:active_runners_with_tags) do
active_runners.with_tags
end
end
end
Project.prepend_if_ee('EE::Project')
......@@ -99,7 +99,7 @@ class BuildDetailsEntity < JobEntity
end
expose :available do |build|
project.any_runners?
project.any_active_runners?
end
expose :settings_path, if: -> (*) { can_admin_build? } do |build|
......
---
title: Shortcircuit expensive queries in Runner#can_pick?
merge_request: 55518
author:
type: performance
---
name: ci_runners_short_circuit_assignable_for
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/55518
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/323317
milestone: '13.10'
type: development
group: group::continuous integration
default_enabled: false
......@@ -998,11 +998,11 @@ RSpec.describe Project do
end
it 'has a shared runner' do
expect(project.any_runners?).to be_truthy
expect(project.any_active_runners?).to be_truthy
end
it 'checks the presence of shared runner' do
expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy
expect(project.any_active_runners? { |runner| runner == shared_runner }).to be_truthy
end
context 'with used pipeline minutes' do
......@@ -1014,7 +1014,7 @@ RSpec.describe Project do
end
it 'does not have a shared runner' do
expect(project.any_runners?).to be_falsey
expect(project.any_active_runners?).to be_falsey
end
end
end
......
......@@ -581,7 +581,7 @@ RSpec.describe Ci::Build do
end
it 'that cannot handle build' do
expect_any_instance_of(Ci::Runner).to receive(:can_pick?).and_return(false)
expect_any_instance_of(Ci::Runner).to receive(:matches_build?).with(build).and_return(false)
is_expected.to be_falsey
end
end
......
......@@ -350,6 +350,8 @@ RSpec.describe Ci::Runner do
end
describe '#can_pick?' do
using RSpec::Parameterized::TableSyntax
let_it_be(:pipeline) { create(:ci_pipeline) }
let(:build) { create(:ci_build, pipeline: pipeline) }
let(:runner_project) { build.project }
......@@ -363,6 +365,11 @@ RSpec.describe Ci::Runner do
let(:other_project) { create(:project) }
let(:other_runner) { create(:ci_runner, :project, projects: [other_project], tag_list: tag_list, run_untagged: run_untagged) }
before do
# `can_pick?` is not used outside the runners available for the project
stub_feature_flags(ci_runners_short_circuit_assignable_for: false)
end
it 'cannot handle builds' do
expect(other_runner.can_pick?(build)).to be_falsey
end
......@@ -430,9 +437,32 @@ RSpec.describe Ci::Runner do
expect(runner.can_pick?(build)).to be_truthy
end
end
it 'does not query for owned or instance runners' do
expect(described_class).not_to receive(:owned_or_instance_wide)
runner.can_pick?(build)
end
context 'when feature flag ci_runners_short_circuit_assignable_for is disabled' do
before do
stub_feature_flags(ci_runners_short_circuit_assignable_for: false)
end
it 'does not query for owned or instance runners' do
expect(described_class).to receive(:owned_or_instance_wide).and_call_original
runner.can_pick?(build)
end
end
end
context 'when runner is not shared' do
before do
# `can_pick?` is not used outside the runners available for the project
stub_feature_flags(ci_runners_short_circuit_assignable_for: false)
end
context 'when runner is assigned to a project' do
it 'can handle builds' do
expect(runner.can_pick?(build)).to be_truthy
......@@ -500,6 +530,29 @@ RSpec.describe Ci::Runner do
it { is_expected.to be_falsey }
end
end
context 'matches tags' do
where(:run_untagged, :runner_tags, :build_tags, :result) do
true | [] | [] | true
true | [] | ['a'] | false
true | %w[a b] | ['a'] | true
true | ['a'] | %w[a b] | false
true | ['a'] | ['a'] | true
false | ['a'] | ['a'] | true
false | ['b'] | ['a'] | false
false | %w[a b] | ['a'] | true
end
with_them do
let(:tag_list) { runner_tags }
before do
build.tag_list = build_tags
end
it { is_expected.to eq(result) }
end
end
end
describe '#status' do
......
......@@ -1599,7 +1599,7 @@ RSpec.describe Project, factory_default: :keep do
end
end
describe '#any_runners?' do
describe '#any_active_runners?' do
context 'shared runners' do
let(:project) { create(:project, shared_runners_enabled: shared_runners_enabled) }
let(:specific_runner) { create(:ci_runner, :project, projects: [project]) }
......@@ -1609,31 +1609,31 @@ RSpec.describe Project, factory_default: :keep do
let(:shared_runners_enabled) { false }
it 'has no runners available' do
expect(project.any_runners?).to be_falsey
expect(project.any_active_runners?).to be_falsey
end
it 'has a specific runner' do
specific_runner
expect(project.any_runners?).to be_truthy
expect(project.any_active_runners?).to be_truthy
end
it 'has a shared runner, but they are prohibited to use' do
shared_runner
expect(project.any_runners?).to be_falsey
expect(project.any_active_runners?).to be_falsey
end
it 'checks the presence of specific runner' do
specific_runner
expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy
expect(project.any_active_runners? { |runner| runner == specific_runner }).to be_truthy
end
it 'returns false if match cannot be found' do
specific_runner
expect(project.any_runners? { false }).to be_falsey
expect(project.any_active_runners? { false }).to be_falsey
end
end
......@@ -1643,19 +1643,19 @@ RSpec.describe Project, factory_default: :keep do
it 'has a shared runner' do
shared_runner
expect(project.any_runners?).to be_truthy
expect(project.any_active_runners?).to be_truthy
end
it 'checks the presence of shared runner' do
shared_runner
expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy
expect(project.any_active_runners? { |runner| runner == shared_runner }).to be_truthy
end
it 'returns false if match cannot be found' do
shared_runner
expect(project.any_runners? { false }).to be_falsey
expect(project.any_active_runners? { false }).to be_falsey
end
end
end
......@@ -1669,13 +1669,13 @@ RSpec.describe Project, factory_default: :keep do
let(:group_runners_enabled) { false }
it 'has no runners available' do
expect(project.any_runners?).to be_falsey
expect(project.any_active_runners?).to be_falsey
end
it 'has a group runner, but they are prohibited to use' do
group_runner
expect(project.any_runners?).to be_falsey
expect(project.any_active_runners?).to be_falsey
end
end
......@@ -1685,19 +1685,19 @@ RSpec.describe Project, factory_default: :keep do
it 'has a group runner' do
group_runner
expect(project.any_runners?).to be_truthy
expect(project.any_active_runners?).to be_truthy
end
it 'checks the presence of group runner' do
group_runner
expect(project.any_runners? { |runner| runner == group_runner }).to be_truthy
expect(project.any_active_runners? { |runner| runner == group_runner }).to be_truthy
end
it 'returns false if match cannot be found' do
group_runner
expect(project.any_runners? { false }).to be_falsey
expect(project.any_active_runners? { false }).to be_falsey
end
end
end
......
......@@ -36,6 +36,7 @@ RSpec.describe Ci::UpdateBuildQueueService do
context 'when feature flag ci_reduce_queries_when_ticking_runner_queue is disabled' do
before do
stub_feature_flags(ci_reduce_queries_when_ticking_runner_queue: false)
stub_feature_flags(ci_runners_short_circuit_assignable_for: false)
end
it 'runs redundant queries using `owned_or_instance_wide` scope' 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