WIP

app/models/namespace.rb

@@ -172,6 +172,21 @@ class Namespace < ActiveRecord::Base
     end
   end
 
+  def shared_runners_minutes_limit
+    read_attribute(:shared_runners_minutes_limit) ||
+      current_application_settings.shared_runners_minutes
+  end
+
+  def shared_runners_minutes_limit_enabled?
+    shared_runners_minutes_limit.nonzero?
+  end
+
+  def shared_runners_minutes_used?
+    shared_runners_enabled? &&
+      shared_runners_minutes_limit_enabled? &&
+      shared_runners_minutes.to_i < shared_runners_minutes_limit
+  end
+
   private
 
   def repository_storage_paths
@@ -211,19 +226,4 @@ class Namespace < ActiveRecord::Base
   def full_path_changed?
     path_changed? || parent_id_changed?
   end
-
-  def shared_runners_minutes_limit
-    read_attribute(:shared_runners_minutes_limit) ||
-      current_application_settings.shared_runners_minutes
-  end
-
-  def shared_runners_minutes_limit_enabled?
-    shared_runners_minutes_limit.nonzero?
-  end
-
-  def shared_runners_minutes_used?
-    shared_runners_enabled? &&
-      shared_runners_minutes_limit_enabled? &&
-      shared_runners_minutes.to_i < shared_runners_minutes_limit
-  end
 end

app/services/ci/register_build_service.rb

@@ -4,24 +4,30 @@ module Ci
   class RegisterBuildService
     include Gitlab::CurrentSettings
 
-    def execute(current_runner)
+    attr_reader :runner
+
+    def initialize(runner)
+      @runner = runner
+    end
+
+    def execute
       builds = Ci::Build.pending.unstarted
 
       builds =
-        if current_runner.shared?
+        if runner.shared?
           builds_for_shared_runner
         else
           builds_for_specific_runner
         end
 
       build = builds.find do |build|
-        current_runner.can_pick?(build)
+        runner.can_pick?(build)
       end
 
       if build
         # In case when 2 runners try to assign the same build, second runner will be declined
         # with StateMachines::InvalidTransition or StaleObjectError when doing run! or save method.
-        build.runner_id = current_runner.id
+        build.runner_id = runner.id
         build.run!
       end
 
@@ -41,8 +47,8 @@ module Ci
         where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0').
 
         # select projects which have allowed number of shared runner minutes or are public
-        where("projects.visibility_level_field=? OR (#{builds_check_limit.to_sql})",
-          Gitlab::VisibilityLevel.PUBLIC)
+        where("projects.visibility_level=? OR (#{builds_check_limit.to_sql})=1",
+          Gitlab::VisibilityLevel::PUBLIC).
 
         # Implement fair scheduling
         # this returns builds that are ordered by number of running builds
@@ -52,11 +58,11 @@ module Ci
     end
 
     def builds_check_limit
-      Namespace.
-        where("namespaces.project_id = ci_builds.gl_project_id").
-        includes(:namespace_metrics).
-        where('COALESCE(namespaces.shared_runner_minutes_limit, ?, 0) == 0 OR ' \
-          'COALESCE(namespace_metrics.shared_runner_minutes, 0) < COALESCE(namespaces.shared_runner_minutes_limit, ?, 0)',
+      Namespace.reorder(nil).
+        where("namespaces.id = projects.namespace_id").
+        joins('LEFT JOIN namespace_metrics ON namespace_metrics.namespace_id = namespaces.id').
+        where('COALESCE(namespaces.shared_runners_minutes_limit, ?, 0) = 0 OR ' \
+          'COALESCE(namespace_metrics.shared_runners_minutes, 0) < COALESCE(namespaces.shared_runners_minutes_limit, ?, 0)',
           application_shared_runners_minutes, application_shared_runners_minutes).
         select('1')
     end
@@ -66,7 +72,7 @@ module Ci
     end
 
     def builds_for_specific_runner
-      new_builds.where(project: current_runner.projects.with_builds_enabled).order('created_at ASC')
+      new_builds.where(project: runner.projects.with_builds_enabled).order('created_at ASC')
     end
 
     def running_builds_for_shared_runners

app/views/admin/projects/show.html.haml

@@ -96,8 +96,8 @@
             %strong
               - if @project.shared_runners_enabled?
                 Enabled
-                - if @project.shared_runner_minutes_limit.nonzero?
-                  = @project.shared_runner_minutes_limit
+                - if @project.shared_runners_minutes_limit.nonzero?
+                  = @project.shared_runners_minutes_limit
                   total minutes
                 - elsif current_application_settings.shared_runners_minutes
                   Unlimited

app/workers/clear_shared_runner_minutes_worker.rb

@@ -3,6 +3,6 @@ class ClearSharedRunnerMinutesWorker
   include DedicatedSidekiqQueue
 
   def perform
-    NamespaceMetrics.update_all(shared_runner_minutes: 0)
+    NamespaceMetrics.update_all(shared_runners_minutes: 0)
   end
 end

config/initializers/1_settings.rb

@@ -403,9 +403,9 @@ Settings.cron_jobs['remove_unreferenced_lfs_objects_worker'] ||= Settingslogic.n
 Settings.cron_jobs['remove_unreferenced_lfs_objects_worker']['cron'] ||= '20 0 * * *'
 Settings.cron_jobs['remove_unreferenced_lfs_objects_worker']['job_class'] = 'RemoveUnreferencedLfsObjectsWorker'
 
-Settings.cron_jobs['clear_shared_runner_minutes_worker'] ||= Settingslogic.new({})
-Settings.cron_jobs['clear_shared_runner_minutes_worker']['cron'] ||= '0 0 0 * *'
-Settings.cron_jobs['clear_shared_runner_minutes_worker']['job_class'] = 'ClearSharedRunnerMinutesWorker'
+Settings.cron_jobs['clear_shared_runners_minutes_worker'] ||= Settingslogic.new({})
+Settings.cron_jobs['clear_shared_runners_minutes_worker']['cron'] ||= '0 0 0 * *'
+Settings.cron_jobs['clear_shared_runners_minutes_worker']['job_class'] = 'ClearSharedRunnerMinutesWorker'
 
 #
 # GitLab Shell

lib/ci/api/builds.rb

@@ -16,7 +16,7 @@ module Ci
           not_found! unless current_runner.active?
           update_runner_info
 
-          build = Ci::RegisterBuildService.new.execute(current_runner)
+          build = Ci::RegisterBuildService.new(current_runner).execute
 
           if build
             Gitlab::Metrics.add_event(:build_found,

spec/factories/namespaces.rb

@@ -3,5 +3,14 @@ FactoryGirl.define do
     sequence(:name) { |n| "namespace#{n}" }
     path { name.downcase.gsub(/\s/, '_') }
     owner
+
+    trait :with_limit do
+      shared_runners_minutes_limit 500
+    end
+
+    trait :with_used_limit do
+      namespace_metrics factory: :namespace_metrics, :with_used_limit
+      shared_runners_minutes_limit 500
+    end
   end
 end

spec/services/ci/register_build_service_spec.rb

@@ -2,7 +2,6 @@ require 'spec_helper'
 
 module Ci
   describe RegisterBuildService, services: true do
-    let!(:service) { RegisterBuildService.new }
     let!(:project) { FactoryGirl.create :empty_project, shared_runners_enabled: false }
     let!(:pipeline) { FactoryGirl.create :ci_pipeline, project: project }
     let!(:pending_build) { FactoryGirl.create :ci_build, pipeline: pipeline }
@@ -19,29 +18,29 @@ module Ci
           pending_build.tag_list = ["linux"]
           pending_build.save
           specific_runner.tag_list = ["linux"]
-          expect(service.execute(specific_runner)).to eq(pending_build)
+          expect(execute(specific_runner)).to eq(pending_build)
         end
 
         it "does not pick build with different tag" do
           pending_build.tag_list = ["linux"]
           pending_build.save
           specific_runner.tag_list = ["win32"]
-          expect(service.execute(specific_runner)).to be_falsey
+          expect(execute(specific_runner)).to be_falsey
         end
 
         it "picks build without tag" do
-          expect(service.execute(specific_runner)).to eq(pending_build)
+          expect(execute(specific_runner)).to eq(pending_build)
         end
 
         it "does not pick build with tag" do
           pending_build.tag_list = ["linux"]
           pending_build.save
-          expect(service.execute(specific_runner)).to be_falsey
+          expect(execute(specific_runner)).to be_falsey
         end
 
         it "pick build without tag" do
           specific_runner.tag_list = ["win32"]
-          expect(service.execute(specific_runner)).to eq(pending_build)
+          expect(execute(specific_runner)).to eq(pending_build)
         end
       end
 
@@ -56,13 +55,13 @@ module Ci
           end
 
           it 'does not pick a build' do
-            expect(service.execute(shared_runner)).to be_nil
+            expect(execute(shared_runner)).to be_nil
           end
         end
 
         context 'for specific runner' do
           it 'does not pick a build' do
-            expect(service.execute(specific_runner)).to be_nil
+            expect(execute(specific_runner)).to be_nil
           end
         end
       end
@@ -86,34 +85,34 @@ module Ci
 
           it 'prefers projects without builds first' do
             # it gets for one build from each of the projects
-            expect(service.execute(shared_runner)).to eq(build1_project1)
-            expect(service.execute(shared_runner)).to eq(build1_project2)
-            expect(service.execute(shared_runner)).to eq(build1_project3)
+            expect(execute(shared_runner)).to eq(build1_project1)
+            expect(execute(shared_runner)).to eq(build1_project2)
+            expect(execute(shared_runner)).to eq(build1_project3)
 
             # then it gets a second build from each of the projects
-            expect(service.execute(shared_runner)).to eq(build2_project1)
-            expect(service.execute(shared_runner)).to eq(build2_project2)
+            expect(execute(shared_runner)).to eq(build2_project1)
+            expect(execute(shared_runner)).to eq(build2_project2)
 
             # in the end the third build
-            expect(service.execute(shared_runner)).to eq(build3_project1)
+            expect(execute(shared_runner)).to eq(build3_project1)
           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(service.execute(shared_runner)).to eq(build1_project1)
+            expect(execute(shared_runner)).to eq(build1_project1)
             build1_project1.reload.success
-            expect(service.execute(shared_runner)).to eq(build2_project1)
+            expect(execute(shared_runner)).to eq(build2_project1)
 
-            expect(service.execute(shared_runner)).to eq(build1_project2)
+            expect(execute(shared_runner)).to eq(build1_project2)
             build1_project2.reload.success
-            expect(service.execute(shared_runner)).to eq(build2_project2)
-            expect(service.execute(shared_runner)).to eq(build1_project3)
-            expect(service.execute(shared_runner)).to eq(build3_project1)
+            expect(execute(shared_runner)).to eq(build2_project2)
+            expect(execute(shared_runner)).to eq(build1_project3)
+            expect(execute(shared_runner)).to eq(build3_project1)
           end
         end
 
         context 'shared runner' do
-          let(:build) { service.execute(shared_runner) }
+          let(:build) { execute(shared_runner) }
 
           it { expect(build).to be_kind_of(Build) }
           it { expect(build).to be_valid }
@@ -122,7 +121,7 @@ module Ci
         end
 
         context 'specific runner' do
-          let(:build) { service.execute(specific_runner) }
+          let(:build) { execute(specific_runner) }
 
           it { expect(build).to be_kind_of(Build) }
           it { expect(build).to be_valid }
@@ -137,13 +136,13 @@ module Ci
         end
 
         context 'shared runner' do
-          let(:build) { service.execute(shared_runner) }
+          let(:build) { execute(shared_runner) }
 
           it { expect(build).to be_nil }
         end
 
         context 'specific runner' do
-          let(:build) { service.execute(specific_runner) }
+          let(:build) { execute(specific_runner) }
 
           it { expect(build).to be_kind_of(Build) }
           it { expect(build).to be_valid }
@@ -159,17 +158,21 @@ module Ci
         end
 
         context 'and uses shared runner' do
-          let(:build) { service.execute(shared_runner) }
+          let(:build) { execute(shared_runner) }
 
           it { expect(build).to be_nil }
         end
 
         context 'and uses specific runner' do
-          let(:build) { service.execute(specific_runner) }
+          let(:build) { execute(specific_runner) }
 
           it { expect(build).to be_nil }
         end
       end
+
+      def execute(runner)
+        described_class.new(runner).execute
+      end
     end
   end
 end