diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 3a3f41cdf35..9139e5c830b 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -94,6 +94,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 000ae3539e3..64549ea3ce2 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -14,14 +14,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 @@ -70,62 +63,27 @@ module Ci private - def builds_for_shared_runner - builds_for_scheduled_runner( - running_builds_for_shared_runners, - projects: { shared_runners_enabled: true } - ) + 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 builds_for_group_runner - builds_for_scheduled_runner( - running_builds_for_group_runners, - projects: { group_runners_enabled: true } - ) + # New builds from the accessible projects + def new_builds + filter_builds(Ci::Build.pending.unstarted) 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') - 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)) - 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 138ef673527..0c343425392 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