Merge branch 'fp-reduce-runners-queries-when-picking-builds' into 'master'

Shortcircuit expensive queries in Runner#can_pick? [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!55518

app/models/ci/build.rb

@@ -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?

app/models/ci/runner.rb

@@ -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?
 

app/models/project.rb

@@ -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)
@@ -2700,6 +2700,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')

app/serializers/build_details_entity.rb

@@ -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|

changelogs/unreleased/fp-reduce-runners-queries-when-picking-builds.yml

+---
+title: Shortcircuit expensive queries in Runner#can_pick?
+merge_request: 55518
+author:
+type: performance

config/feature_flags/development/ci_runners_short_circuit_assignable_for.yml

+---
+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

ee/spec/models/project_spec.rb

@@ -1011,11 +1011,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
@@ -1027,7 +1027,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

spec/models/ci/build_spec.rb

@@ -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

spec/models/ci/runner_spec.rb

@@ -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

spec/models/project_spec.rb

@@ -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

spec/services/ci/update_build_queue_service_spec.rb

@@ -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