Revert fair scheduling for all builds

Per discussion in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9646#note_65730532 this logic is being optimized elsewhere and it will simplify things if we make less changes to this code right now.

4 files changed, 43 insertions, 140 deletions

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 6904aca5e68..40d828b8414 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -99,24 +99,6 @@ 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 64549ea3ce2..55d0273847c 100644
--- a/app/services/ci/register_job_service.rb
+++ b/app/services/ci/register_job_service.rb
@@ -14,7 +14,14 @@ module Ci
     end
 
     def execute
-      builds = builds_for_runner
+      builds =
+        if runner.shared?
+          builds_for_shared_runner
+        elsif runner.group?
+          builds_for_group_runner
+        else
+          builds_for_project_runner
+        end
 
       valid = true
 
@@ -63,27 +70,39 @@ module Ci
 
     private
 
-    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')
+    def builds_for_shared_runner
+      new_builds.
+        # don't run projects which have not enabled shared runners and builds
+        joins(:project).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').
+
+      # 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
+      joins("LEFT JOIN (#{running_builds_for_shared_runners.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
 
-    # New builds from the accessible projects
-    def new_builds
-      filter_builds(Ci::Build.pending.unstarted)
+    def builds_for_project_runner
+      new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC')
     end
 
-    # Count running builds from the accessible projects
-    def running_projects
-      filter_builds(Ci::Build.running)
+    def builds_for_group_runner
+      hierarchy_groups = Gitlab::GroupHierarchy.new(runner.groups).base_and_descendants
+      projects = Project.where(namespace_id: hierarchy_groups).without_deleted.with_builds_enabled
+      new_builds.where(project: projects.without_deleted.with_builds_enabled).order('created_at ASC')
+    end
+
+    def running_builds_for_shared_runners
+      Ci::Build.running.where(runner: Ci::Runner.shared)
         .group(:project_id).select(:project_id, 'count(*) AS running_builds')
     end
 
-    # Filter the builds from the accessible projects
-    def filter_builds(builds)
+    def new_builds
+      builds = Ci::Build.pending.unstarted
       builds = builds.ref_protected if runner.ref_protected?
-      builds.where(project: runner.accessible_projects)
+      builds
     end
 
     def register_failure
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index fbf9539e698..d6ce97a9b28 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -792,73 +792,6 @@ describe Ci::Runner do
     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', :nested_groups 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
-
   describe '#invalidate_build_cache!' do
     context 'runner can pick the build' do
       it 'calls #tick_runner_queue' do
diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb
index 0c343425392..7d3c43eeaf7 100644
--- a/spec/services/ci/register_job_service_spec.rb
+++ b/spec/services/ci/register_job_service_spec.rb
@@ -195,56 +195,25 @@ module Ci
           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
+            expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 6
             execute(group_runner)
 
-            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 5
+            expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 5
             execute(group_runner)
 
-            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 4
+            expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 4
             execute(group_runner)
 
-            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 3
+            expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 3
             execute(group_runner)
 
-            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 2
+            expect(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 2
             execute(group_runner)
 
-            expect(described_class.new(group_runner).send(:builds_for_runner).count).to eq 1
+            expect(described_class.new(group_runner).send(:builds_for_group_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)
-            expect(execute(group_runner)).to eq(build1_project2)
-            expect(execute(group_runner)).to eq(build1_project3)
-
-            # then it gets a second build from each of the projects
-            expect(execute(group_runner)).to eq(build2_project1)
-            expect(execute(group_runner)).to eq(build2_project2)
-
-            # 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
-            # after finishing the first build for project 1, get a second build from the same project
-            expect(execute(group_runner)).to eq(build1_project1)
-            build1_project1.reload.success
-            expect(execute(group_runner)).to eq(build2_project1)
-
-            expect(execute(group_runner)).to eq(build1_project2)
-            build1_project2.reload.success
-            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(described_class.new(group_runner).send(:builds_for_group_runner).count).to eq 0
             expect(execute(group_runner)).to be_nil
           end
         end
@@ -282,7 +251,7 @@ module Ci
           let!(:other_build) { create :ci_build, pipeline: pipeline }
 
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner)
               .and_return(Ci::Build.where(id: [pending_job, other_build]))
           end
 
@@ -294,7 +263,7 @@ module Ci
 
         context 'when single build is in queue' do
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner)
               .and_return(Ci::Build.where(id: pending_job))
           end
 
@@ -305,7 +274,7 @@ module Ci
 
         context 'when there is no build in queue' do
           before do
-            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_runner)
+            allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner)
               .and_return(Ci::Build.none)
           end