Merge branch '8-16-stable' of gitlab.com:gitlab-org/gitlab-ce into 8-16-stable

app/models/namespace.rb

@@ -4,6 +4,7 @@ class Namespace < ActiveRecord::Base
   include CacheMarkdownField
   include Sortable
   include Gitlab::ShellAdapter
+  include Gitlab::CurrentSettings
   include Routable
 
   cache_markdown_field :description, pipeline: :description
@@ -176,6 +177,10 @@ class Namespace < ActiveRecord::Base
     end
   end
 
+  def shared_runners_enabled?
+    projects.with_shared_runners.any?
+  end
+
   def full_name
     @full_name ||=
       if parent

app/models/project.rb

@@ -226,6 +226,7 @@ class Project < ActiveRecord::Base
 
   scope :with_project_feature, -> { joins('LEFT JOIN project_features ON projects.id = project_features.project_id') }
   scope :with_statistics, -> { includes(:statistics) }
+  scope :with_shared_runners, -> { where(shared_runners_enabled: true) }
 
   # "enabled" here means "not disabled". It includes private features!
   scope :with_feature_enabled, ->(feature) {
@@ -1098,12 +1099,20 @@ class Project < ActiveRecord::Base
     project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED)
   end
 
+  def shared_runners_available?
+    shared_runners_enabled?
+  end
+
+  def shared_runners
+    shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none
+  end
+
   def any_runners?(&block)
     if runners.active.any?(&block)
       return true
     end
 
-    shared_runners_enabled? && Ci::Runner.shared.active.any?(&block)
+    shared_runners.active.any?(&block)
   end
 
   def valid_runners_token?(token)

app/services/ci/register_build_service.rb

@@ -2,48 +2,72 @@ module Ci
   # This class responsible for assigning
   # proper pending build to runner on runner API request
   class RegisterBuildService
-    def execute(current_runner)
-      builds = Ci::Build.pending.unstarted
+    include Gitlab::CurrentSettings
 
+    attr_reader :runner
+
+    Result = Struct.new(:build, :valid?)
+
+    def initialize(runner)
+      @runner = runner
+    end
+
+    def execute
       builds =
-        if current_runner.shared?
-          builds.
-            # don't run projects which have not enabled shared runners and builds
-            joins(:project).where(projects: { shared_runners_enabled: true }).
-            joins('LEFT JOIN project_features ON ci_builds.gl_project_id = project_features.project_id').
-
-            # 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.gl_project_id=project_builds.gl_project_id").
-            where('project_features.builds_access_level IS NULL or project_features.builds_access_level > 0').
-            order('COALESCE(project_builds.running_builds, 0) ASC', 'ci_builds.id ASC')
+        if runner.shared?
+          builds_for_shared_runner
         else
-          # do run projects which are only assigned to this runner (FIFO)
-          builds.where(project: current_runner.projects.with_builds_enabled).order('created_at ASC')
+          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
 
-      build
+      Result.new(build, true)
 
     rescue StateMachines::InvalidTransition, ActiveRecord::StaleObjectError
-      nil
+      Result.new(build, false)
     end
 
     private
 
+    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 }).
+        joins('LEFT JOIN project_features ON ci_builds.gl_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.gl_project_id=project_builds.gl_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.with_builds_enabled).order('created_at ASC')
+    end
+
     def running_builds_for_shared_runners
       Ci::Build.running.where(runner: Ci::Runner.shared).
         group(:gl_project_id).select(:gl_project_id, 'count(*) AS running_builds')
     end
+
+    def new_builds
+      Ci::Build.pending.unstarted
+    end
+
+    def shared_runner_build_limits_feature_enabled?
+      ENV['DISABLE_SHARED_RUNNER_BUILD_MINUTES_LIMIT'].to_s != 'true'
+    end
   end
 end

changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml

+---
+title: 'Fix CI requests concurrency for newer runners that prevents from picking pending builds (from 1.9.0-rc5)'
+merge_request: 8760

lib/ci/api/builds.rb

@@ -18,24 +18,31 @@ module Ci
 
           if current_runner.is_runner_queue_value_latest?(params[:last_update])
             header 'X-GitLab-Last-Update', params[:last_update]
+            Gitlab::Metrics.add_event(:build_not_found_cached)
             return build_not_found!
           end
 
           new_update = current_runner.ensure_runner_queue_value
 
-          build = Ci::RegisterBuildService.new.execute(current_runner)
+          result = Ci::RegisterBuildService.new(current_runner).execute
 
-          if build
-            Gitlab::Metrics.add_event(:build_found,
-                                      project: build.project.path_with_namespace)
+          if result.valid?
+            if result.build
+              Gitlab::Metrics.add_event(:build_found,
+                                        project: result.build.project.path_with_namespace)
 
-            present build, with: Entities::BuildDetails
-          else
-            Gitlab::Metrics.add_event(:build_not_found)
+              present result.build, with: Entities::BuildDetails
+            else
+              Gitlab::Metrics.add_event(:build_not_found)
 
-            header 'X-GitLab-Last-Update', new_update
+              header 'X-GitLab-Last-Update', new_update
 
-            build_not_found!
+              build_not_found!
+            end
+          else
+            # We received build that is invalid due to concurrency conflict
+            Gitlab::Metrics.add_event(:build_invalid)
+            conflict!
           end
         end
 

lib/gitlab/visibility_level.rb

@@ -11,6 +11,7 @@ module Gitlab
     included do
       scope :public_only,               -> { where(visibility_level: PUBLIC) }
       scope :public_and_internal_only,  -> { where(visibility_level: [PUBLIC, INTERNAL] ) }
+      scope :non_public_only,           -> { where.not(visibility_level: PUBLIC) }
 
       scope :public_to_user, -> (user) { user && !user.external ? public_and_internal_only : public_only }
     end

spec/factories/ci/runners.rb

@@ -16,6 +16,10 @@ FactoryGirl.define do
       is_shared true
     end
 
+    trait :specific do
+      is_shared false
+    end
+
     trait :inactive do
       active false
     end

spec/factories/groups.rb

 FactoryGirl.define do
-  factory :group do
+  factory :group, class: Group, parent: :namespace do
     sequence(:name) { |n| "group#{n}" }
     path { name.downcase.gsub(/\s/, '_') }
     type 'Group'
+    owner nil
 
     trait :public do
       visibility_level Gitlab::VisibilityLevel::PUBLIC

spec/models/group_spec.rb

@@ -81,13 +81,19 @@ describe Group, models: true do
     describe 'public_only' do
       subject { described_class.public_only.to_a }
 
-      it{ is_expected.to eq([group]) }
+      it { is_expected.to eq([group]) }
     end
 
     describe 'public_and_internal_only' do
       subject { described_class.public_and_internal_only.to_a }
 
-      it{ is_expected.to match_array([group, internal_group]) }
+      it { is_expected.to match_array([group, internal_group]) }
+    end
+
+    describe 'non_public_only' do
+      subject { described_class.non_public_only.to_a }
+
+      it { is_expected.to match_array([private_group, internal_group]) }
     end
   end
 

spec/models/project_spec.rb

@@ -832,6 +832,26 @@ describe Project, models: true do
     it { expect(project.builds_enabled?).to be_truthy }
   end
 
+  describe '.with_shared_runners' do
+    subject { Project.with_shared_runners }
+
+    context 'when shared runners are enabled for project' do
+      let!(:project) { create(:empty_project, shared_runners_enabled: true) }
+
+      it "returns a project" do
+        is_expected.to eq([project])
+      end
+    end
+
+    context 'when shared runners are disabled for project' do
+      let!(:project) { create(:empty_project, shared_runners_enabled: false) }
+
+      it "returns an empty array" do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe '.cached_count', caching: true do
     let(:group)     { create(:group, :public) }
     let!(:project1) { create(:empty_project, :public, group: group) }
@@ -974,6 +994,28 @@ describe Project, models: true do
     end
   end
 
+  describe '#shared_runners' do
+    let!(:runner) { create(:ci_runner, :shared) }
+
+    subject { project.shared_runners }
+
+    context 'when shared runners are enabled for project' do
+      let!(:project) { create(:empty_project, shared_runners_enabled: true) }
+
+      it "returns a list of shared runners" do
+        is_expected.to eq([runner])
+      end
+    end
+
+    context 'when shared runners are disabled for project' do
+      let!(:project) { create(:empty_project, shared_runners_enabled: false) }
+
+      it "returns a empty list" do
+        is_expected.to be_empty
+      end
+    end
+  end
+
   describe '#visibility_level_allowed?' do
     let(:project) { create(:project, :internal) }
 

spec/requests/ci/api/builds_spec.rb

@@ -91,6 +91,20 @@ describe Ci::API::Builds do
           expect { register_builds }.to change { runner.reload.contacted_at }
         end
 
+        context 'when concurrently updating build' do
+          before do
+            expect_any_instance_of(Ci::Build).to receive(:run!).
+              and_raise(ActiveRecord::StaleObjectError.new(nil, nil))
+          end
+
+          it 'returns a conflict' do
+            register_builds info: { platform: :darwin }
+
+            expect(response).to have_http_status(409)
+            expect(response.headers).not_to have_key('X-GitLab-Last-Update')
+          end
+        end
+
         context 'registry credentials' do
           let(:registry_credentials) do
             { 'type' => 'registry',

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.build
+      end
     end
   end
 end