simplify runner selection

don't differentiate between the different runner types, instead we rely
on the Runner model to provide the available projects.
scheduling is now applied to all runners equally.

4 files changed, 137 insertions, 59 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 3649c12f39f..e83061d40f5 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -87,6 +87,24 @@ module Ci
       self.token = SecureRandom.hex(15) if self.token.blank?
     end
 
+    def accessible_projects
+      accessible_projects =
+        if shared?
+          Project.with_shared_runners
+        elsif project?
+          projects
+        elsif group?
+          hierarchy_groups = Gitlab::GroupHierarchy.new(groups).base_and_descendants
+          Project.where(namespace_id: hierarchy_groups)
+        else
+          Project.none
+        end
+
+      accessible_projects
+        .with_builds_enabled
+        .without_deleted
+    end
+
     def assign_to(project, current_user = nil)
       self.is_shared = false if shared?
       self.save
diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb
index 268b42e51b8..beb3788e864 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -11,14 +11,7 @@ module Ci
     end
 
     def execute
-      builds =
-        if runner.shared?
-          builds_for_shared_runner
-        elsif runner.group?
-          builds_for_group_runner
-        else
-          builds_for_specific_runner
-        end
+      builds = builds_for_runner
 
       valid = true
 
@@ -67,62 +60,27 @@ module Ci
 
     private
 
-    def builds_for_shared_runner
-      builds_for_scheduled_runner(
-        running_builds_for_shared_runners,
-        projects: { shared_runners_enabled: true }
-      )
-    end
-
-    def builds_for_group_runner
-      builds_for_scheduled_runner(
-        running_builds_for_group_runners,
-        projects: { group_runners_enabled: true }
-      )
-    end
-
-    def builds_for_scheduled_runner(build_join, project_where)
-      project_where = project_where.deep_merge(projects: { pending_delete: false })
-
-      # don't run projects which have not enabled group runners and builds
-      builds = new_builds
-        .joins(:project).where(project_where)
-        .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')
-
-      # Implement fair scheduling
-      # this returns builds that are ordered by number of running builds
-      # we prefer projects that don't use group runners at all
-      builds
-        .joins("LEFT JOIN (#{build_join.to_sql}) AS project_builds ON ci_builds.project_id=project_builds.project_id")
-        .order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC')
-    end
-
-    def builds_for_specific_runner
-      new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC')
+    def builds_for_runner
+      new_builds
+        .joins("LEFT JOIN (#{running_projects.to_sql}) AS running_projects ON ci_builds.project_id=running_projects.project_id")
+        .order('COALESCE(running_projects.running_builds, 0) ASC', 'ci_builds.id ASC')
     end
 
-    def running_builds_for_shared_runners
-      running_builds_for_runners(Ci::Runner.shared)
-    end
-
-    def running_builds_for_group_runners
-      running_builds_for_runners(Ci::Runner.joins(:runner_groups))
+    # New builds from the accessible projects
+    def new_builds
+      filter_builds(Ci::Build.pending.unstarted)
     end
 
-    def running_builds_for_runners(runners)
-      Ci::Build.running.where(runner: runners)
+    # Count running builds from the accessible projects
+    def running_projects
+      filter_builds(Ci::Build.running)
         .group(:project_id).select(:project_id, 'count(*) AS running_builds')
     end
 
-    def new_builds
-      builds = Ci::Build.pending.unstarted
+    # Filter the builds from the accessible projects
+    def filter_builds(builds)
       builds = builds.ref_protected if runner.ref_protected?
-      builds
-    end
-
-    def shared_runner_build_limits_feature_enabled?
-      ENV['DISABLE_SHARED_RUNNER_BUILD_MINUTES_LIMIT'].to_s != 'true'
+      builds.where(project: runner.accessible_projects)
     end
 
     def register_failure
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index fb724f682a5..512a490d289 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -776,4 +776,71 @@ describe Ci::Runner do
       expect(runner.project?).to be true
     end
   end
+
+  describe '#accessible_projects' do
+    let!(:shared_runner) { create(:ci_runner, :shared) }
+    let!(:shared_project) { create(:project, shared_runners_enabled: true) }
+
+    let!(:project_runner) { create(:ci_runner) }
+    let!(:project_project) { create(:project, runners: [project_runner], shared_runners_enabled: false) }
+
+    let!(:group_runner) { create(:ci_runner) }
+
+    let!(:parent_group) { create(:group) }
+    let!(:parent_group_project) do
+      create(:project, group: parent_group, shared_runners_enabled: false)
+    end
+
+    let!(:group) { create :group, runners: [group_runner], parent: parent_group }
+    let!(:group_project) do
+      create(:project, group: group, shared_runners_enabled: false)
+    end
+
+    let!(:nested_group_project) do
+      nested_group = create :group, parent: group
+      create(:project, group: nested_group, shared_runners_enabled: false)
+    end
+
+    it 'returns the project with a shared runner' do
+      expect(shared_runner.reload.accessible_projects).to eq [shared_project]
+    end
+
+    it 'returns the project with a project runner' do
+      expect(project_runner.reload.accessible_projects).to eq [project_project]
+    end
+
+    it 'returns the projects with a group and nested group runner' do
+      expect(group_runner.reload.accessible_projects).to eq [group_project, nested_group_project]
+    end
+
+    context 'deleted' do
+      before do
+        shared_project.update_attributes!(pending_delete: true)
+        project_project.update_attributes!(pending_delete: true)
+        group_project.update_attributes!(pending_delete: true)
+        nested_group_project.update_attributes!(pending_delete: true)
+      end
+
+      it 'returns no projects' do
+        expect(shared_runner.reload.accessible_projects).to be_empty
+        expect(project_runner.reload.accessible_projects).to be_empty
+        expect(group_runner.reload.accessible_projects).to be_empty
+      end
+    end
+
+    context 'builds disabled' do
+      before do
+        shared_project.update_attributes!(builds_enabled: false)
+        project_project.update_attributes!(builds_enabled: false)
+        group_project.update_attributes!(builds_enabled: false)
+        nested_group_project.update_attributes!(builds_enabled: false)
+      end
+
+      it 'returns no projects' do
+        expect(shared_runner.reload.accessible_projects).to be_empty
+        expect(project_runner.reload.accessible_projects).to be_empty
+        expect(group_runner.reload.accessible_projects).to be_empty
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index 5ff56797541..edc5aaf18a8 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -179,6 +179,7 @@ module Ci
           let!(:pipeline2) { create :ci_pipeline, project: project2 }
           let!(:project3) { create :project, group_runners_enabled: true, group: group }
           let!(:pipeline3) { create :ci_pipeline, project: project3 }
+
           let!(:build1_project1) { pending_job }
           let!(:build2_project1) { create :ci_build, pipeline: pipeline }
           let!(:build3_project1) { create :ci_build, pipeline: pipeline }
@@ -186,6 +187,36 @@ module Ci
           let!(:build2_project2) { create :ci_build, pipeline: pipeline2 }
           let!(:build1_project3) { create :ci_build, pipeline: pipeline3 }
 
+          # these shouldn't influence the scheduling
+          let!(:unrelated_group) { create :group }
+          let!(:unrelated_project) { create :project, group_runners_enabled: true, group: unrelated_group }
+          let!(:unrelated_pipeline) { create :ci_pipeline, project: unrelated_project }
+          let!(:build1_unrelated_project) { create :ci_build, pipeline: unrelated_pipeline }
+          let!(:unrelated_group_runner) { create :ci_runner, groups: [unrelated_group] }
+
+          it 'does not consider builds from other group runners' do
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 6
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 5
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 4
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 3
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 2
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 1
+            execute(group_runner)
+
+            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 0
+            expect(execute(group_runner)).to be_nil
+          end
+
           it 'prefers projects without builds first' do
             # it gets for one build from each of the projects
             expect(execute(group_runner)).to eq(build1_project1)
@@ -198,6 +229,8 @@ module Ci
 
             # in the end the third build
             expect(execute(group_runner)).to eq(build3_project1)
+
+            expect(execute(group_runner)).to be_nil
           end
 
           it 'equalises number of running builds' do
@@ -211,6 +244,8 @@ module Ci
             expect(execute(group_runner)).to eq(build2_project2)
             expect(execute(group_runner)).to eq(build1_project3)
             expect(execute(group_runner)).to eq(build3_project1)
+
+            expect(execute(group_runner)).to be_nil
           end
         end
 
@@ -247,7 +282,7 @@ module Ci
           let!(:other_build) { create :ci_build, pipeline: pipeline }
 
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
               .and_return(Ci::Build.where(id: [pending_job, other_build]))
           end
 
@@ -259,7 +294,7 @@ module Ci
 
         context 'when single build is in queue' do
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
               .and_return(Ci::Build.where(id: pending_job))
           end
 
@@ -270,7 +305,7 @@ module Ci
 
         context 'when there is no build in queue' do
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_specific_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
               .and_return(Ci::Build.none)
           end