diff options
author | James Lopez <james@jameslopez.es> | 2017-01-25 23:20:57 +0300 |
---|---|---|
committer | James Lopez <james@jameslopez.es> | 2017-01-25 23:20:57 +0300 |
commit | a72b48147a2d358bb5f9d084bb636c3f74d8a7dc (patch) | |
tree | 9716a156673fcccf303ade96ccd79fdce8b94b14 | |
parent | 431983f904f74e593de4591c48fa1ffe7f432408 (diff) | |
parent | 67b407b484f8ef08d85f7b9a5ef3bba35be25968 (diff) |
Merge branch '8-16-stable' of gitlab.com:gitlab-org/gitlab-ce into 8-16-stable
-rw-r--r-- | app/models/namespace.rb | 5 | ||||
-rw-r--r-- | app/models/project.rb | 11 | ||||
-rw-r--r-- | app/services/ci/register_build_service.rb | 62 | ||||
-rw-r--r-- | changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml | 3 | ||||
-rw-r--r-- | lib/ci/api/builds.rb | 25 | ||||
-rw-r--r-- | lib/gitlab/visibility_level.rb | 1 | ||||
-rw-r--r-- | spec/factories/ci/runners.rb | 4 | ||||
-rw-r--r-- | spec/factories/groups.rb | 3 | ||||
-rw-r--r-- | spec/models/group_spec.rb | 10 | ||||
-rw-r--r-- | spec/models/project_spec.rb | 42 | ||||
-rw-r--r-- | spec/requests/ci/api/builds_spec.rb | 14 | ||||
-rw-r--r-- | spec/services/ci/register_build_service_spec.rb | 55 |
12 files changed, 177 insertions, 58 deletions
diff --git a/app/models/namespace.rb b/app/models/namespace.rb index dd33975731f..296729b0835 100644 --- a/app/models/namespace.rb +++ b/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 diff --git a/app/models/project.rb b/app/models/project.rb index 1630975b0d3..63ff6516ba7 100644 --- a/app/models/project.rb +++ b/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) diff --git a/app/services/ci/register_build_service.rb b/app/services/ci/register_build_service.rb index 74b5ebf372b..6f03bf2be13 100644 --- a/app/services/ci/register_build_service.rb +++ b/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 diff --git a/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml b/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml new file mode 100644 index 00000000000..075d74b9cb8 --- /dev/null +++ b/changelogs/unreleased/fix-ci-requests-concurrency-for-newer-runners.yml @@ -0,0 +1,3 @@ +--- +title: 'Fix CI requests concurrency for newer runners that prevents from picking pending builds (from 1.9.0-rc5)' +merge_request: 8760 diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index c4bdef781f7..8b939663ffd 100644 --- a/lib/ci/api/builds.rb +++ b/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 diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index 9462f3368e6..c7953af29dd 100644 --- a/lib/gitlab/visibility_level.rb +++ b/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 diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index ed4acca23f1..c3b4aff55ba 100644 --- a/spec/factories/ci/runners.rb +++ b/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 diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb index ece6beb9fa9..86f51ffca99 100644 --- a/spec/factories/groups.rb +++ b/spec/factories/groups.rb @@ -1,8 +1,9 @@ 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 diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 45fe927202b..7402e9f8c7b 100644 --- a/spec/models/group_spec.rb +++ b/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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e93a4e62244..51f4a9859c7 100644 --- a/spec/models/project_spec.rb +++ b/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) } diff --git a/spec/requests/ci/api/builds_spec.rb b/spec/requests/ci/api/builds_spec.rb index 270c23e3f19..8dbe5f0b025 100644 --- a/spec/requests/ci/api/builds_spec.rb +++ b/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', diff --git a/spec/services/ci/register_build_service_spec.rb b/spec/services/ci/register_build_service_spec.rb index a3fc23ba177..d9f774a1095 100644 --- a/spec/services/ci/register_build_service_spec.rb +++ b/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 |