Commit be54e341 authored by Marius Bobin's avatar Marius Bobin

Merge branch 'remove-ff-short-circuit-assignable-for' into 'master'

Remove FF ci_runners_short_circuit_assignable_for

See merge request gitlab-org/gitlab!79346
parents 3647ee79 6cef95a9
...@@ -368,21 +368,8 @@ module Ci ...@@ -368,21 +368,8 @@ module Ci
runner_projects.any? runner_projects.any?
end 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) def match_build_if_online?(build)
active? && online? && can_pick?(build) active? && online? && matches_build?(build)
end end
def only_for?(project) def only_for?(project)
...@@ -459,6 +446,10 @@ module Ci ...@@ -459,6 +446,10 @@ module Ci
tick_runner_queue if matches_build?(build) tick_runner_queue if matches_build?(build)
end end
def matches_build?(build)
runner_matcher.matches?(build.build_matcher)
end
def uncached_contacted_at def uncached_contacted_at
read_attribute(:contacted_at) read_attribute(:contacted_at)
end end
...@@ -514,12 +505,6 @@ module Ci ...@@ -514,12 +505,6 @@ module Ci
end end
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 def no_projects
if runner_projects.any? if runner_projects.any?
errors.add(:runner, 'cannot have projects assigned') errors.add(:runner, 'cannot have projects assigned')
...@@ -543,10 +528,6 @@ module Ci ...@@ -543,10 +528,6 @@ module Ci
errors.add(:runner, 'needs to be assigned to exactly one group') errors.add(:runner, 'needs to be assigned to exactly one group')
end end
end end
def matches_build?(build)
runner_matcher.matches?(build.build_matcher)
end
end end
end end
......
...@@ -159,7 +159,7 @@ module Ci ...@@ -159,7 +159,7 @@ module Ci
return return
end end
if runner.can_pick?(build) if runner.matches_build?(build)
@metrics.increment_queue_operation(:build_can_pick) @metrics.increment_queue_operation(:build_can_pick)
else else
@metrics.increment_queue_operation(:build_not_pick) @metrics.increment_queue_operation(:build_not_pick)
......
---
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
...@@ -588,7 +588,7 @@ RSpec.describe Ci::Runner do ...@@ -588,7 +588,7 @@ RSpec.describe Ci::Runner do
end end
end end
describe '#can_pick?' do describe '#matches_build?' do
using RSpec::Parameterized::TableSyntax using RSpec::Parameterized::TableSyntax
let_it_be(:pipeline) { create(:ci_pipeline) } let_it_be(:pipeline) { create(:ci_pipeline) }
...@@ -599,31 +599,15 @@ RSpec.describe Ci::Runner do ...@@ -599,31 +599,15 @@ RSpec.describe Ci::Runner do
let(:tag_list) { [] } let(:tag_list) { [] }
let(:run_untagged) { true } let(:run_untagged) { true }
subject { runner.can_pick?(build) } subject { runner.matches_build?(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
context 'when runner does not have tags' do context 'when runner does not have tags' do
it 'can handle builds without tags' do it { is_expected.to be_truthy }
expect(runner.can_pick?(build)).to be_truthy
end
it 'cannot handle build with tags' do it 'cannot handle build with tags' do
build.tag_list = ['aa'] build.tag_list = ['aa']
expect(runner.can_pick?(build)).to be_falsey is_expected.to be_falsey
end end
end end
...@@ -634,20 +618,18 @@ RSpec.describe Ci::Runner do ...@@ -634,20 +618,18 @@ RSpec.describe Ci::Runner do
it 'can handle build with matching tags' do it 'can handle build with matching tags' do
build.tag_list = ['bb'] build.tag_list = ['bb']
expect(runner.can_pick?(build)).to be_truthy is_expected.to be_truthy
end end
it 'cannot handle build without matching tags' do it 'cannot handle build without matching tags' do
build.tag_list = ['aa'] build.tag_list = ['aa']
expect(runner.can_pick?(build)).to be_falsey is_expected.to be_falsey
end end
end end
context 'when runner can pick untagged jobs' do context 'when runner can pick untagged jobs' do
it 'can handle builds without tags' do it { is_expected.to be_truthy }
expect(runner.can_pick?(build)).to be_truthy
end
it_behaves_like 'tagged build picker' it_behaves_like 'tagged build picker'
end end
...@@ -655,9 +637,7 @@ RSpec.describe Ci::Runner do ...@@ -655,9 +637,7 @@ RSpec.describe Ci::Runner do
context 'when runner cannot pick untagged jobs' do context 'when runner cannot pick untagged jobs' do
let(:run_untagged) { false } let(:run_untagged) { false }
it 'cannot handle builds without tags' do it { is_expected.to be_falsey }
expect(runner.can_pick?(build)).to be_falsey
end
it_behaves_like 'tagged build picker' it_behaves_like 'tagged build picker'
end end
...@@ -666,64 +646,31 @@ RSpec.describe Ci::Runner do ...@@ -666,64 +646,31 @@ RSpec.describe Ci::Runner do
context 'when runner is shared' do context 'when runner is shared' do
let(:runner) { create(:ci_runner, :instance) } let(:runner) { create(:ci_runner, :instance) }
it 'can handle builds' do it { is_expected.to be_truthy }
expect(runner.can_pick?(build)).to be_truthy
end
context 'when runner is locked' do context 'when runner is locked' do
let(:runner) { create(:ci_runner, :instance, locked: true) } let(:runner) { create(:ci_runner, :instance, locked: true) }
it 'can handle builds' do it { is_expected.to be_truthy }
expect(runner.can_pick?(build)).to be_truthy
end
end end
it 'does not query for owned or instance runners' do it 'does not query for owned or instance runners' do
expect(described_class).not_to receive(:owned_or_instance_wide) expect(described_class).not_to receive(:owned_or_instance_wide)
runner.can_pick?(build) subject
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
end end
context 'when runner is not shared' do 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 context 'when runner is assigned to a project' do
it 'can handle builds' do it { is_expected.to be_truthy }
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
end end
context 'when runner is assigned to a group' do context 'when runner is assigned to a group' do
let(:group) { create(:group, projects: [build.project]) } let(:group) { create(:group, projects: [build.project]) }
let(:runner) { create(:ci_runner, :group, tag_list: tag_list, run_untagged: run_untagged, groups: [group]) } let(:runner) { create(:ci_runner, :group, tag_list: tag_list, run_untagged: run_untagged, groups: [group]) }
it 'can handle builds' do it { is_expected.to be_truthy }
expect(runner.can_pick?(build)).to be_truthy
end
it 'knows namespace id it is assigned to' do it 'knows namespace id it is assigned to' do
expect(runner.namespace_ids).to eq [group.id] expect(runner.namespace_ids).to eq [group.id]
...@@ -1264,14 +1211,6 @@ RSpec.describe Ci::Runner do ...@@ -1264,14 +1211,6 @@ RSpec.describe Ci::Runner do
runner.pick_build!(build) runner.pick_build!(build)
end end
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 end
describe 'project runner without projects is destroyable' do 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