Skip to content

G
gitlab-ce
Commit 6cef95a9 authored by Fabio Pitino's avatar Fabio Pitino Committed by Marius Bobin
Browse files
Options

Skip redundant checks for runners already scoped to a project 

Remove the feature flag ci_runners_short_circuit_assignable_for
which skips checking if the runner is assignable for a project
given that the method is always used on project runners.

Changelog: changed
parent e1848581
Hide whitespace changes
Inline Side-by-side
Showing with 19 additions and 107 deletions
app/models/ci/runner.rb
View file @ 6cef95a9
......@@ -368,21 +368,8 @@ 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)
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)
active? && online? && matches_build?(build)
end
def only_for?(project)
......@@ -459,6 +446,10 @@ module Ci
tick_runner_queue if matches_build?(build)
end
def matches_build?(build)
runner_matcher.matches?(build.build_matcher)
end
def uncached_contacted_at
read_attribute(:contacted_at)
end
......@@ -514,12 +505,6 @@ 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
def no_projects
if runner_projects.any?
errors.add(:runner, 'cannot have projects assigned')
......@@ -543,10 +528,6 @@ module Ci
errors.add(:runner, 'needs to be assigned to exactly one group')
end
end
def matches_build?(build)
runner_matcher.matches?(build.build_matcher)
end
end
end
......
app/services/ci/register_job_service.rb
View file @ 6cef95a9
......@@ -159,7 +159,7 @@ module Ci
return
end
if runner.can_pick?(build)
if runner.matches_build?(build)
@metrics.increment_queue_operation(:build_can_pick)
else
@metrics.increment_queue_operation(:build_not_pick)
......
config/feature_flags/development/ci_runners_short_circuit_assignable_for.yml deleted 100644 → 0
View file @ e1848581
---
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::pipeline execution
default_enabled: false
spec/models/ci/runner_spec.rb
View file @ 6cef95a9
......@@ -588,7 +588,7 @@ RSpec.describe Ci::Runner do
end
end
describe '#can_pick?' do
describe '#matches_build?' do
using RSpec::Parameterized::TableSyntax
let_it_be(:pipeline) { create(:ci_pipeline) }
......@@ -599,31 +599,15 @@ RSpec.describe Ci::Runner do
let(:tag_list) { [] }
let(:run_untagged) { true }
subject { runner.can_pick?(build) }
context 'a different 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
end
subject { runner.matches_build?(build) }
context 'when runner does not have tags' do
it 'can handle builds without tags' do
expect(runner.can_pick?(build)).to be_truthy
end
it { is_expected.to be_truthy }
it 'cannot handle build with tags' do
build.tag_list = ['aa']
expect(runner.can_pick?(build)).to be_falsey
is_expected.to be_falsey
end
end
......@@ -634,20 +618,18 @@ RSpec.describe Ci::Runner do
it 'can handle build with matching tags' do
build.tag_list = ['bb']
expect(runner.can_pick?(build)).to be_truthy
is_expected.to be_truthy
end
it 'cannot handle build without matching tags' do
build.tag_list = ['aa']
expect(runner.can_pick?(build)).to be_falsey
is_expected.to be_falsey
end
end
context 'when runner can pick untagged jobs' do
it 'can handle builds without tags' do
expect(runner.can_pick?(build)).to be_truthy
end
it { is_expected.to be_truthy }
it_behaves_like 'tagged build picker'
end
......@@ -655,9 +637,7 @@ RSpec.describe Ci::Runner do
context 'when runner cannot pick untagged jobs' do
let(:run_untagged) { false }
it 'cannot handle builds without tags' do
expect(runner.can_pick?(build)).to be_falsey
end
it { is_expected.to be_falsey }
it_behaves_like 'tagged build picker'
end
......@@ -666,64 +646,31 @@ RSpec.describe Ci::Runner do
context 'when runner is shared' do
let(:runner) { create(:ci_runner, :instance) }
it 'can handle builds' do
expect(runner.can_pick?(build)).to be_truthy
end
it { is_expected.to be_truthy }
context 'when runner is locked' do
let(:runner) { create(:ci_runner, :instance, locked: true) }
it 'can handle builds' do
expect(runner.can_pick?(build)).to be_truthy
end
it { is_expected.to be_truthy }
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
subject
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
end
end
context 'when runner is assigned to another project' do
let(:runner_project) { create(:project) }
it 'cannot handle builds' do
expect(runner.can_pick?(build)).to be_falsey
end
it { is_expected.to be_truthy }
end
context 'when runner is assigned to a group' do
let(:group) { create(:group, projects: [build.project]) }
let(:runner) { create(:ci_runner, :group, tag_list: tag_list, run_untagged: run_untagged, groups: [group]) }
it 'can handle builds' do
expect(runner.can_pick?(build)).to be_truthy
end
it { is_expected.to be_truthy }
it 'knows namespace id it is assigned to' do
expect(runner.namespace_ids).to eq [group.id]
......@@ -1264,14 +1211,6 @@ RSpec.describe Ci::Runner do
runner.pick_build!(build)
end
end
context 'build picking improvement' do
it 'does not check if the build is assignable to a runner' do
expect(runner).not_to receive(:can_pick?)
runner.pick_build!(build)
end
end
end
describe 'project runner without projects is destroyable' 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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7