Move builds queuing strategies to separate files

This also improves the inteface and avoids using SimpleDelegator to make
it a bit more readable and easy to follow.

app/services/ci/queue/build_queue_service.rb

+# frozen_string_literal: true
+
+module Ci
+  module Queue
+    class BuildQueueService
+      include ::Gitlab::Utils::StrongMemoize
+
+      attr_reader :runner
+
+      def initialize(runner)
+        @runner = runner
+      end
+
+      def new_builds
+        strategy.new_builds
+      end
+
+      ##
+      # This is overridden in EE
+      #
+      def builds_for_shared_runner
+        strategy.builds_for_shared_runner
+      end
+
+      # rubocop:disable CodeReuse/ActiveRecord
+      def builds_for_group_runner
+        # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
+        groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
+
+        hierarchy_groups = Gitlab::ObjectHierarchy
+          .new(groups, options: { use_distinct: ::Feature.enabled?(:use_distinct_in_register_job_object_hierarchy) })
+          .base_and_descendants
+
+        projects = Project.where(namespace_id: hierarchy_groups)
+          .with_group_runners_enabled
+          .with_builds_enabled
+          .without_deleted
+
+        relation = new_builds.where(project: projects)
+
+        order(relation)
+      end
+
+      def builds_for_project_runner
+        relation = new_builds
+          .where(project: runner.projects.without_deleted.with_builds_enabled)
+
+        order(relation)
+      end
+
+      def builds_queued_before(relation, time)
+        relation.queued_before(time)
+      end
+
+      def builds_for_protected_runner(relation)
+        relation.ref_protected
+      end
+
+      def builds_matching_tag_ids(relation, ids)
+        strategy.builds_matching_tag_ids(relation, ids)
+      end
+
+      def builds_with_any_tags(relation)
+        strategy.builds_with_any_tags(relation)
+      end
+
+      def order(relation)
+        strategy.order(relation)
+      end
+
+      def execute(relation)
+        strategy.build_ids(relation)
+      end
+
+      private
+
+      def strategy
+        strong_memoize(:strategy) do
+          if ::Feature.enabled?(:ci_pending_builds_queue_source, runner, default_enabled: :yaml)
+            Queue::PendingBuildsStrategy.new(runner)
+          else
+            Queue::BuildsTableStrategy.new(runner)
+          end
+        end
+      end
+    end
+  end
+end
+
+Ci::Queue::BuildQueueService.prepend_mod_with('Ci::Queue::BuildQueueService')

app/services/ci/queue/builds_table_strategy.rb

+# frozen_string_literal: true
+
+module Ci
+  module Queue
+    class BuildsTableStrategy
+      attr_reader :runner
+
+      def initialize(runner)
+        @runner = runner
+      end
+
+      # rubocop:disable CodeReuse/ActiveRecord
+      def builds_for_shared_runner
+        relation = new_builds
+          # don't run projects which have not enabled shared runners and builds
+          .joins('INNER JOIN projects ON ci_builds.project_id = projects.id')
+          .where(projects: { shared_runners_enabled: true, pending_delete: false })
+          .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id')
+          .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0')
+
+        if Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml)
+          # if disaster recovery is enabled, we fallback to FIFO scheduling
+          relation.order('ci_builds.id ASC')
+        else
+          # Implement fair scheduling
+          # this returns builds that are ordered by number of running builds
+          # we prefer projects that don't use shared runners at all
+          relation
+            .joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id = project_builds.project_id")
+            .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC')
+        end
+      end
+
+      def builds_matching_tag_ids(relation, ids)
+        # pick builds that does not have other tags than runner's one
+        relation.matches_tag_ids(ids)
+      end
+
+      def builds_with_any_tags(relation)
+        # pick builds that have at least one tag
+        relation.with_any_tags
+      end
+
+      def order(relation)
+        relation.order('id ASC')
+      end
+
+      def new_builds
+        ::Ci::Build.pending.unstarted
+      end
+
+      def build_ids(relation)
+        relation.pluck(:id)
+      end
+
+      private
+
+      def running_builds_for_shared_runners
+        ::Ci::Build.running
+          .where(runner: ::Ci::Runner.instance_type)
+          .group(:project_id)
+          .select(:project_id, 'COUNT(*) AS running_builds')
+      end
+      # rubocop:enable CodeReuse/ActiveRecord
+    end
+  end
+end

app/services/ci/build_queue_service.rb → app/services/ci/queue/pending_builds_strategy.rb

 # frozen_string_literal: true
 
 module Ci
-  class BuildQueueService < SimpleDelegator
-    attr_reader :runner
-
-    def initialize(runner)
-      @runner = runner
-
-      @strategy = begin
-        if ::Feature.enabled?(:ci_pending_builds_queue_source, runner, default_enabled: :yaml)
-          PendingBuildsTableStrategy.new(runner)
-        else
-          BuildsTableStrategy.new(runner)
-        end
-      end
-
-      super(@strategy)
-    end
-
-    ##
-    # This is overridden in EE
-    #
-    def builds_for_shared_runner
-      @strategy.builds_for_shared_runner
-    end
-
-    # rubocop:disable CodeReuse/ActiveRecord
-    def builds_for_group_runner
-      # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
-      groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
-
-      hierarchy_groups = Gitlab::ObjectHierarchy
-        .new(groups, options: { use_distinct: ::Feature.enabled?(:use_distinct_in_register_job_object_hierarchy) })
-        .base_and_descendants
-
-      projects = Project.where(namespace_id: hierarchy_groups)
-        .with_group_runners_enabled
-        .with_builds_enabled
-        .without_deleted
-
-      relation = @strategy.new_builds.where(project: projects)
-
-      @strategy.order(relation)
-    end
-
-    def builds_for_project_runner
-      relation = @strategy.new_builds
-        .where(project: runner.projects.without_deleted.with_builds_enabled)
-
-      @strategy.order(relation)
-    end
-
-    def builds_queued_before(relation, time)
-      relation.queued_before(time)
-    end
-
-    def builds_for_protected_runner(relation)
-      relation.ref_protected
-    end
-
-    class BuildsTableStrategy
-      attr_reader :runner, :common
-
-      def initialize(runner)
-        @runner = runner
-      end
-
-      def builds_for_shared_runner
-        relation = new_builds
-          # don't run projects which have not enabled shared runners and builds
-          .joins('INNER JOIN projects ON ci_builds.project_id = projects.id')
-          .where(projects: { shared_runners_enabled: true, pending_delete: false })
-          .joins('LEFT JOIN project_features ON ci_builds.project_id = project_features.project_id')
-          .where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0')
-
-        if Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml)
-          # if disaster recovery is enabled, we fallback to FIFO scheduling
-          relation.order('ci_builds.id ASC')
-        else
-          # Implement fair scheduling
-          # this returns builds that are ordered by number of running builds
-          # we prefer projects that don't use shared runners at all
-          relation
-            .joins("LEFT JOIN (#{running_builds_for_shared_runners.to_sql}) AS project_builds ON ci_builds.project_id = project_builds.project_id")
-            .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC')
-        end
-      end
-
-      def builds_matching_tag_ids(relation, ids)
-        # pick builds that does not have other tags than runner's one
-        relation.matches_tag_ids(ids)
-      end
-
-      def builds_with_any_tags(relation)
-        # pick builds that have at least one tag
-        relation.with_any_tags
-      end
-
-      def order(relation)
-        relation.order('id ASC')
-      end
-
-      def new_builds
-        ::Ci::Build.pending.unstarted
-      end
-
-      def execute(relation)
-        relation.pluck(:id)
-      end
-
-      private
-
-      def running_builds_for_shared_runners
-        ::Ci::Build.running
-          .where(runner: ::Ci::Runner.instance_type)
-          .group(:project_id)
-          .select(:project_id, 'COUNT(*) AS running_builds')
-      end
-    end
-
-    class PendingBuildsTableStrategy
+  module Queue
+    class PendingBuildsStrategy
       attr_reader :runner
 
       def initialize(runner)
         @runner = runner
       end
 
+      # rubocop:disable CodeReuse/ActiveRecord
       def builds_for_shared_runner
         relation = new_builds
           # don't run projects which have not enabled shared runners and builds
@@ -155,10 +39,6 @@ module Ci
         relation.merge(CommitStatus.with_any_tags(table: 'ci_pending_builds', column: 'build_id'))
       end
 
-      def builds_queued_before(relation, time)
-        relation.queued_before(time)
-      end
-
       def order(relation)
         relation.order('build_id ASC')
       end
@@ -167,7 +47,7 @@ module Ci
         ::Ci::PendingBuild.all
       end
 
-      def execute(relation)
+      def build_ids(relation)
         relation.pluck(:build_id)
       end
 
@@ -183,5 +63,3 @@ module Ci
     end
   end
 end
-
-Ci::BuildQueueService.prepend_mod_with('Ci::BuildQueueService')

app/services/ci/register_job_service.rb

@@ -103,7 +103,7 @@ module Ci
 
     # rubocop: disable CodeReuse/ActiveRecord
     def each_build(params, &blk)
-      queue = ::Ci::BuildQueueService.new(runner)
+      queue = ::Ci::Queue::BuildQueueService.new(runner)
 
       builds = begin
         if runner.instance_type?

ee/app/services/ee/ci/build_queue_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Ci
-    module BuildQueueService
-      extend ActiveSupport::Concern
-      extend ::Gitlab::Utils::Override
-
-      override :builds_for_shared_runner
-      def builds_for_shared_runner
-        # if disaster recovery is enabled, we disable quota
-        if ::Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml)
-          super
-        else
-          enforce_minutes_based_on_cost_factors(super)
-        end
-      end
-
-      # rubocop: disable CodeReuse/ActiveRecord
-      def enforce_minutes_based_on_cost_factors(relation)
-        visibility_relation = ::CommitStatus.where(
-          projects: { visibility_level: runner.visibility_levels_without_minutes_quota })
-
-        enforce_limits_relation = ::CommitStatus.where('EXISTS (?)', builds_check_limit)
-
-        relation.merge(visibility_relation.or(enforce_limits_relation))
-      end
-
-      def builds_check_limit
-        all_namespaces
-          .joins('LEFT JOIN namespace_statistics ON namespace_statistics.namespace_id = namespaces.id')
-          .where('COALESCE(namespaces.shared_runners_minutes_limit, ?, 0) = 0 OR ' \
-                 'COALESCE(namespace_statistics.shared_runners_seconds, 0) < ' \
-                 'COALESCE('\
-                   '(namespaces.shared_runners_minutes_limit + COALESCE(namespaces.extra_shared_runners_minutes_limit, 0)), ' \
-                   '(? + COALESCE(namespaces.extra_shared_runners_minutes_limit, 0)), ' \
-                  '0) * 60',
-                application_shared_runners_minutes, application_shared_runners_minutes)
-          .select('1')
-      end
-
-      def all_namespaces
-        if traversal_ids_enabled?
-          ::Namespace
-            .where('namespaces.id = project_namespaces.traversal_ids[1]')
-            .joins('INNER JOIN namespaces as project_namespaces ON project_namespaces.id = projects.namespace_id')
-        else
-          namespaces = ::Namespace.reorder(nil).where('namespaces.id = projects.namespace_id')
-          ::Gitlab::ObjectHierarchy.new(namespaces, options: { skip_ordering: true }).roots
-        end
-      end
-      # rubocop: enable CodeReuse/ActiveRecord
-
-      def application_shared_runners_minutes
-        ::Gitlab::CurrentSettings.shared_runners_minutes
-      end
-
-      def traversal_ids_enabled?
-        ::Feature.enabled?(:sync_traversal_ids, default_enabled: :yaml) &&
-          ::Feature.enabled?(:traversal_ids_for_quota_calculation, type: :development, default_enabled: :yaml)
-      end
-    end
-  end
-end

ee/app/services/ee/ci/queue/build_queue_service.rb

+# frozen_string_literal: true
+
+module EE
+  module Ci
+    module Queue
+      module BuildQueueService
+        extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
+
+        override :builds_for_shared_runner
+        def builds_for_shared_runner
+          # if disaster recovery is enabled, we disable quota
+          if ::Feature.enabled?(:ci_queueing_disaster_recovery, runner, type: :ops, default_enabled: :yaml)
+            super
+          else
+            enforce_minutes_based_on_cost_factors(super)
+          end
+        end
+
+        # rubocop: disable CodeReuse/ActiveRecord
+        def enforce_minutes_based_on_cost_factors(relation)
+          visibility_relation = ::CommitStatus.where(
+            projects: { visibility_level: runner.visibility_levels_without_minutes_quota })
+
+          enforce_limits_relation = ::CommitStatus.where('EXISTS (?)', builds_check_limit)
+
+          relation.merge(visibility_relation.or(enforce_limits_relation))
+        end
+
+        def builds_check_limit
+          all_namespaces
+            .joins('LEFT JOIN namespace_statistics ON namespace_statistics.namespace_id = namespaces.id')
+            .where('COALESCE(namespaces.shared_runners_minutes_limit, ?, 0) = 0 OR ' \
+                   'COALESCE(namespace_statistics.shared_runners_seconds, 0) < ' \
+                   'COALESCE('\
+                     '(namespaces.shared_runners_minutes_limit + COALESCE(namespaces.extra_shared_runners_minutes_limit, 0)), ' \
+                     '(? + COALESCE(namespaces.extra_shared_runners_minutes_limit, 0)), ' \
+                    '0) * 60',
+                  application_shared_runners_minutes, application_shared_runners_minutes)
+            .select('1')
+        end
+
+        def all_namespaces
+          if traversal_ids_enabled?
+            ::Namespace
+              .where('namespaces.id = project_namespaces.traversal_ids[1]')
+              .joins('INNER JOIN namespaces as project_namespaces ON project_namespaces.id = projects.namespace_id')
+          else
+            namespaces = ::Namespace.reorder(nil).where('namespaces.id = projects.namespace_id')
+            ::Gitlab::ObjectHierarchy.new(namespaces, options: { skip_ordering: true }).roots
+          end
+        end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        def application_shared_runners_minutes
+          ::Gitlab::CurrentSettings.shared_runners_minutes
+        end
+
+        def traversal_ids_enabled?
+          ::Feature.enabled?(:sync_traversal_ids, default_enabled: :yaml) &&
+            ::Feature.enabled?(:traversal_ids_for_quota_calculation, type: :development, default_enabled: :yaml)
+        end
+      end
+    end
+  end
+end

spec/services/ci/register_job_service_spec.rb

@@ -269,7 +269,7 @@ module Ci
             let!(:unrelated_group_runner) { create(:ci_runner, :group, groups: [unrelated_group]) }
 
             it 'does not consider builds from other group runners' do
-              queue = ::Ci::BuildQueueService.new(group_runner)
+              queue = ::Ci::Queue::BuildQueueService.new(group_runner)
 
               expect(queue.builds_for_group_runner.size).to eq 6
               execute(group_runner)
@@ -301,7 +301,7 @@ module Ci
             end
 
             it 'calls DISTINCT' do
-              queue = ::Ci::BuildQueueService.new(group_runner)
+              queue = ::Ci::Queue::BuildQueueService.new(group_runner)
 
               expect(queue.builds_for_group_runner.to_sql).to include("DISTINCT")
             end
@@ -314,7 +314,7 @@ module Ci
             end
 
             it 'does not call DISTINCT' do
-              queue = ::Ci::BuildQueueService.new(group_runner)
+              queue = ::Ci::Queue::BuildQueueService.new(group_runner)
 
               expect(queue.builds_for_group_runner.to_sql).not_to include("DISTINCT")
             end
@@ -355,7 +355,7 @@ module Ci
             let!(:other_build) { create(:ci_build, :pending, :queued, pipeline: pipeline) }
 
             before do
-              allow_any_instance_of(::Ci::BuildQueueService)
+              allow_any_instance_of(::Ci::Queue::BuildQueueService)
                 .to receive(:execute)
                 .and_return(Ci::Build.where(id: [pending_job, other_build]).pluck(:id))
             end
@@ -368,7 +368,7 @@ module Ci
 
           context 'when single build is in queue' do
             before do
-              allow_any_instance_of(::Ci::BuildQueueService)
+              allow_any_instance_of(::Ci::Queue::BuildQueueService)
                 .to receive(:execute)
                 .and_return(Ci::Build.where(id: pending_job).pluck(:id))
             end
@@ -380,7 +380,7 @@ module Ci
 
           context 'when there is no build in queue' do
             before do
-              allow_any_instance_of(::Ci::BuildQueueService)
+              allow_any_instance_of(::Ci::Queue::BuildQueueService)
                 .to receive(:execute)
                 .and_return([])
             end