From 1549239849adf31a078be7503ab2288795e337cf Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 1 Mar 2017 13:06:35 +0100 Subject: [PATCH 01/86] add Ci::RunnerGroup join model --- app/models/ci/runner_group.rb | 8 ++++++ .../20170301101006_add_ci_runner_groups.rb | 27 +++++++++++++++++++ db/schema.rb | 13 +++++++++ 3 files changed, 48 insertions(+) create mode 100644 app/models/ci/runner_group.rb create mode 100644 db/migrate/20170301101006_add_ci_runner_groups.rb diff --git a/app/models/ci/runner_group.rb b/app/models/ci/runner_group.rb new file mode 100644 index 000000000000..87f3ba13bff0 --- /dev/null +++ b/app/models/ci/runner_group.rb @@ -0,0 +1,8 @@ +module Ci + class RunnerGroup < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :runner + belongs_to :group, class_name: '::Group' + end +end diff --git a/db/migrate/20170301101006_add_ci_runner_groups.rb b/db/migrate/20170301101006_add_ci_runner_groups.rb new file mode 100644 index 000000000000..73a135b0ee1b --- /dev/null +++ b/db/migrate/20170301101006_add_ci_runner_groups.rb @@ -0,0 +1,27 @@ +class AddCiRunnerGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :ci_runner_groups do |t| + t.integer :runner_id + t.integer :group_id + + t.timestamps_with_timezone null: false + end + + add_concurrent_index :ci_runner_groups, :runner_id + add_concurrent_foreign_key :ci_runner_groups, :ci_runners, column: :runner_id, on_delete: :cascade + + add_concurrent_index :ci_runner_groups, :group_id + add_concurrent_index :ci_runner_groups, [:runner_id, :group_id], unique: true + add_concurrent_foreign_key :ci_runner_groups, :namespaces, column: :group_id, on_delete: :cascade + end + + def down + drop_table :ci_runner_groups + end +end diff --git a/db/schema.rb b/db/schema.rb index df621956c808..9f3b27b8b6ad 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -443,6 +443,17 @@ add_index "ci_pipelines", ["status"], name: "index_ci_pipelines_on_status", using: :btree add_index "ci_pipelines", ["user_id"], name: "index_ci_pipelines_on_user_id", using: :btree + create_table "ci_runner_groups", force: :cascade do |t| + t.integer "runner_id" + t.integer "group_id" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + + add_index "ci_runner_groups", ["group_id"], name: "index_ci_runner_groups_on_group_id", using: :btree + add_index "ci_runner_groups", ["runner_id", "group_id"], name: "index_ci_runner_groups_on_runner_id_and_group_id", unique: true, using: :btree + add_index "ci_runner_groups", ["runner_id"], name: "index_ci_runner_groups_on_runner_id", using: :btree + create_table "ci_runner_projects", force: :cascade do |t| t.integer "runner_id", null: false t.datetime "created_at" @@ -2078,6 +2089,8 @@ add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade + add_foreign_key "ci_runner_groups", "ci_runners", column: "runner_id", name: "fk_d8a0baa93b", on_delete: :cascade + add_foreign_key "ci_runner_groups", "namespaces", column: "group_id", name: "fk_cdafb3bbba", on_delete: :cascade add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade -- GitLab From 9a07bc819f137a3077784a33e1b36bf6797d9547 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 1 Mar 2017 13:07:24 +0100 Subject: [PATCH 02/86] add misssing scope specs --- spec/models/ci/runner_spec.rb | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ab170e6351c2..529f200b43ad 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -49,6 +49,25 @@ end end + describe 'scopes' do + describe 'owned_or_shared' do + it 'returns the specific project runner' do + specific_project = create :project + other_project = create :project + specific_runner = create :ci_runner, :specific, projects: [specific_project] + other_runner = create :ci_runner, :specific, projects: [other_project] + + expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] + end + + it 'returns the shared projects' do + runner = create :ci_runner, :shared + + expect(described_class.owned_or_shared(0)).to eq [runner] + end + end + end + describe '#display_name' do it 'returns the description if it has a value' do runner = FactoryBot.build(:ci_runner, description: 'Linux/Ruby-1.9.3-p448') -- GitLab From 295184f6a5ff0b98340c32e0cc715dafa4d9b60c Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 1 Mar 2017 17:19:12 +0100 Subject: [PATCH 03/86] include group runners in scope --- app/models/ci/runner.rb | 31 ++++++++++++++++++++-- spec/models/ci/runner_spec.rb | 50 ++++++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 5a4c56ec0dc5..8f8dfbda412d 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -14,6 +14,8 @@ class Runner < ActiveRecord::Base has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :runner_projects + has_many :runner_groups + has_many :groups, through: :runner_groups has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build' @@ -27,8 +29,33 @@ class Runner < ActiveRecord::Base scope :ordered, ->() { order(id: :desc) } scope :owned_or_shared, ->(project_id) do - joins('LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id') - .where("ci_runner_projects.project_id = :project_id OR ci_runners.is_shared = true", project_id: project_id) + joins( + %{ + -- project runners + LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id + + -- group runners + LEFT JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id + LEFT JOIN namespaces ON namespaces.id = ci_runner_groups.group_id + LEFT JOIN projects group_projects ON group_projects.namespace_id = namespaces.id + } + ).where( + %{ + -- project runners + ci_runner_projects.project_id = :project_id + + OR + + -- group runners + group_projects.id = :project_id + + OR + + -- shared runners + ci_runners.is_shared = true + }, + project_id: project_id + ) end scope :assignable_for, ->(project) do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 529f200b43ad..477540fb3b07 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -52,19 +52,61 @@ describe 'scopes' do describe 'owned_or_shared' do it 'returns the specific project runner' do + # own specific_project = create :project - other_project = create :project specific_runner = create :ci_runner, :specific, projects: [specific_project] - other_runner = create :ci_runner, :specific, projects: [other_project] + + # other + other_project = create :project + create :ci_runner, :specific, projects: [other_project] expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] end - it 'returns the shared projects' do - runner = create :ci_runner, :shared + it 'returns the shared project runner' do + project = create :project + runner = create :ci_runner, :shared, projects: [project] expect(described_class.owned_or_shared(0)).to eq [runner] end + + it 'returns the specific group runner' do + # own + specific_group = create :group + specific_project = create :project, group: specific_group + specific_runner = create :ci_runner, :specific, groups: [specific_group] + + # other + other_group = create :group + create :project, group: other_group + create :ci_runner, :specific, groups: [other_group] + + expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] + end + + it 'returns the shared group runner' do + group = create :group + runner = create :ci_runner, :shared, groups: [group] + + expect(described_class.owned_or_shared(0)).to eq [runner] + end + + it 'returns a globally shared, a project specific and a group specific runner' do + # group specific + group = create :group + project = create :project, group: group + group_runner = create :ci_runner, :specific, groups: [group] + + # project specific + project_runner = create :ci_runner, :specific, projects: [project] + + # globally shared + shared_runner = create :ci_runner, :shared + + expect(described_class.owned_or_shared(project.id)).to match_array [ + group_runner, project_runner, shared_runner + ] + end end end -- GitLab From 40b0f5406d97d2fff5019b87a2ab22468053af20 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 6 Sep 2017 10:53:07 +0200 Subject: [PATCH 04/86] use union instead of multiple joins the unions performs much better than the joins, and the execution time is constant with a growing number of records. --- app/models/ci/runner.rb | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8f8dfbda412d..cfed4a2eeea8 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -29,33 +29,34 @@ class Runner < ActiveRecord::Base scope :ordered, ->() { order(id: :desc) } scope :owned_or_shared, ->(project_id) do - joins( + project_runners = joins( %{ - -- project runners LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id + } + ).where( + %{ + ci_runner_projects.project_id = :project_id + }, + project_id: project_id + ) - -- group runners + group_runners = joins( + %{ LEFT JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id LEFT JOIN namespaces ON namespaces.id = ci_runner_groups.group_id LEFT JOIN projects group_projects ON group_projects.namespace_id = namespaces.id } ).where( %{ - -- project runners - ci_runner_projects.project_id = :project_id - - OR - - -- group runners group_projects.id = :project_id - - OR - - -- shared runners - ci_runners.is_shared = true }, project_id: project_id ) + + shared_runners = where(is_shared: true) + + union = Gitlab::SQL::Union.new([project_runners, group_runners, shared_runners]) + from("(#{union.to_sql}) ci_runners") end scope :assignable_for, ->(project) do -- GitLab From 14c8dbc66537e59d9a01fe5e8ad64ba559254f14 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 6 Sep 2017 11:26:55 +0200 Subject: [PATCH 05/86] drop 'scopes' context from specs --- spec/models/ci/runner_spec.rb | 88 +++++++++++++++++------------------ 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 477540fb3b07..5a851a3d5598 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -49,64 +49,62 @@ end end - describe 'scopes' do - describe 'owned_or_shared' do - it 'returns the specific project runner' do - # own - specific_project = create :project - specific_runner = create :ci_runner, :specific, projects: [specific_project] + describe '.owned_or_shared' do + it 'returns the specific project runner' do + # own + specific_project = create :project + specific_runner = create :ci_runner, :specific, projects: [specific_project] - # other - other_project = create :project - create :ci_runner, :specific, projects: [other_project] + # other + other_project = create :project + create :ci_runner, :specific, projects: [other_project] - expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] - end + expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] + end - it 'returns the shared project runner' do - project = create :project - runner = create :ci_runner, :shared, projects: [project] + it 'returns the shared project runner' do + project = create :project + runner = create :ci_runner, :shared, projects: [project] - expect(described_class.owned_or_shared(0)).to eq [runner] - end + expect(described_class.owned_or_shared(0)).to eq [runner] + end - it 'returns the specific group runner' do - # own - specific_group = create :group - specific_project = create :project, group: specific_group - specific_runner = create :ci_runner, :specific, groups: [specific_group] + it 'returns the specific group runner' do + # own + specific_group = create :group + specific_project = create :project, group: specific_group + specific_runner = create :ci_runner, :specific, groups: [specific_group] - # other - other_group = create :group - create :project, group: other_group - create :ci_runner, :specific, groups: [other_group] + # other + other_group = create :group + create :project, group: other_group + create :ci_runner, :specific, groups: [other_group] - expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] - end + expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] + end - it 'returns the shared group runner' do - group = create :group - runner = create :ci_runner, :shared, groups: [group] + it 'returns the shared group runner' do + group = create :group + runner = create :ci_runner, :shared, groups: [group] - expect(described_class.owned_or_shared(0)).to eq [runner] - end + expect(described_class.owned_or_shared(0)).to eq [runner] + end - it 'returns a globally shared, a project specific and a group specific runner' do - # group specific - group = create :group - project = create :project, group: group - group_runner = create :ci_runner, :specific, groups: [group] + it 'returns a globally shared, a project specific and a group specific runner' do + # group specific + group = create :group + project = create :project, group: group + group_runner = create :ci_runner, :specific, groups: [group] - # project specific - project_runner = create :ci_runner, :specific, projects: [project] + # project specific + project_runner = create :ci_runner, :specific, projects: [project] - # globally shared - shared_runner = create :ci_runner, :shared + # globally shared + shared_runner = create :ci_runner, :shared - expect(described_class.owned_or_shared(project.id)).to match_array [ - group_runner, project_runner, shared_runner - ] - end + expect(described_class.owned_or_shared(project.id)).to match_array [ + group_runner, project_runner, shared_runner + ] end end -- GitLab From 9507f39459316719088722510a6ae11b79a4b442 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 6 Sep 2017 15:46:57 +0200 Subject: [PATCH 06/86] add runners_token column to namespaces --- app/models/group.rb | 4 ++++ ...0170906133745_add_runners_token_to_groups.rb | 17 +++++++++++++++++ db/schema.rb | 2 ++ 3 files changed, 23 insertions(+) create mode 100644 db/migrate/20170906133745_add_runners_token_to_groups.rb diff --git a/app/models/group.rb b/app/models/group.rb index 8ff781059cc0..ec27f757f463 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -9,6 +9,7 @@ class Group < Namespace include SelectForProjectAuthorization include LoadedInGroupList include GroupDescendant + include TokenAuthenticatable has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent alias_method :members, :group_members @@ -43,6 +44,9 @@ class Group < Namespace validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } + add_authentication_token_field :runners_token + before_save :ensure_runners_token + after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement diff --git a/db/migrate/20170906133745_add_runners_token_to_groups.rb b/db/migrate/20170906133745_add_runners_token_to_groups.rb new file mode 100644 index 000000000000..54d0fddd5e3a --- /dev/null +++ b/db/migrate/20170906133745_add_runners_token_to_groups.rb @@ -0,0 +1,17 @@ +class AddRunnersTokenToGroups < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column :namespaces, :runners_token, :string + + add_concurrent_index :namespaces, :runners_token, unique: true + end + + def down + remove_column :namespaces, :runners_token + end +end diff --git a/db/schema.rb b/db/schema.rb index 9f3b27b8b6ad..b2c37a65ccfa 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1270,6 +1270,7 @@ t.boolean "require_two_factor_authentication", default: false, null: false t.integer "two_factor_grace_period", default: 48, null: false t.integer "cached_markdown_version" + t.string "runners_token" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree @@ -1280,6 +1281,7 @@ add_index "namespaces", ["path"], name: "index_namespaces_on_path", using: :btree add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"} add_index "namespaces", ["require_two_factor_authentication"], name: "index_namespaces_on_require_two_factor_authentication", using: :btree + add_index "namespaces", ["runners_token"], name: "index_namespaces_on_runners_token", unique: true, using: :btree add_index "namespaces", ["type"], name: "index_namespaces_on_type", using: :btree create_table "notes", force: :cascade do |t| -- GitLab From 7fbdd17cbcd19086694f575884191a6d137838dc Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 7 Sep 2017 15:49:29 +0200 Subject: [PATCH 07/86] authorize group runners on user --- app/models/group.rb | 2 ++ app/models/user.rb | 16 ++++++++-- spec/models/user_spec.rb | 64 ++++++++++++++++++++++++++++++++-------- 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/app/models/group.rb b/app/models/group.rb index ec27f757f463..c34c913a16bf 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -29,6 +29,8 @@ class Group < Namespace has_many :labels, class_name: 'GroupLabel' has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' + has_many :runner_groups, class_name: 'Ci::RunnerGroup' + has_many :runners, through: :runner_groups, source: :runner, class_name: 'Ci::Runner' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/user.rb b/app/models/user.rb index b06681489720..0c5c0fef9d4a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -995,10 +995,17 @@ def can_be_removed? def ci_authorized_runners @ci_authorized_runners ||= begin - runner_ids = Ci::RunnerProject + project_runner_ids = Ci::RunnerProject .where(project: authorized_projects(Gitlab::Access::MASTER)) .select(:runner_id) - Ci::Runner.specific.where(id: runner_ids) + + group_runner_ids = Ci::RunnerGroup + .where(group_id: owned_or_masters_groups.select(:id)) + .select(:runner_id) + + union = Gitlab::SQL::Union.new([project_runner_ids, group_runner_ids]) + + Ci::Runner.specific.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end end @@ -1187,6 +1194,11 @@ def max_member_access_for_group(group_id) max_member_access_for_group_ids([group_id])[group_id] end + def owned_or_masters_groups + union = Gitlab::SQL::Union.new([owned_groups, masters_groups]) + Group.from("(#{union.to_sql}) namespaces") + end + protected # override, from Devise::Validatable diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 35db7616efb9..f384b688889e 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1785,14 +1785,12 @@ describe '#ci_authorized_runners' do let(:user) { create(:user) } - let(:runner) { create(:ci_runner) } + let(:runner_1) { create(:ci_runner) } + let(:runner_2) { create(:ci_runner) } - before do - project.runners << runner - end - - context 'without any projects' do - let(:project) { create(:project) } + context 'without any projects nor groups' do + let!(:project) { create(:project, runners: [runner_1]) } + let!(:group) { create(:group) } it 'does not load' do expect(user.ci_authorized_runners).to be_empty @@ -1801,10 +1799,38 @@ context 'with personal projects runners' do let(:namespace) { create(:namespace, owner: user) } - let(:project) { create(:project, namespace: namespace) } + let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) } it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner) + expect(user.ci_authorized_runners).to contain_exactly(runner_1) + end + end + + context 'with personal group runner' do + let!(:project) { create(:project, runners: [runner_1]) } + let!(:group) do + create(:group, runners: [runner_2]).tap do |group| + group.add_owner(user) + end + end + + it 'loads' do + expect(user.ci_authorized_runners).to contain_exactly(runner_2) + end + end + + context 'with personal project and group runner' do + let(:namespace) { create(:namespace, owner: user) } + let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) } + + let!(:group) do + create(:group, runners: [runner_2]).tap do |group| + group.add_owner(user) + end + end + + it 'loads' do + expect(user.ci_authorized_runners).to contain_exactly(runner_1, runner_2) end end @@ -1815,7 +1841,7 @@ end it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner) + expect(user.ci_authorized_runners).to contain_exactly(runner_1) end end @@ -1832,7 +1858,21 @@ context 'with groups projects runners' do let(:group) { create(:group) } - let(:project) { create(:project, group: group) } + let!(:project) { create(:project, group: group, runners: [runner_1]) } + + def add_user(access) + group.add_user(user, access) + end + + it_behaves_like :member + end + + context 'with groups runners' do + let!(:group) do + create(:group, runners: [runner_1]).tap do |group| + group.add_owner(user) + end + end def add_user(access) group.add_user(user, access) @@ -1842,7 +1882,7 @@ def add_user(access) end context 'with other projects runners' do - let(:project) { create(:project) } + let!(:project) { create(:project, runners: [runner_1]) } def add_user(access) project.add_role(user, access) -- GitLab From b55c3a7bc4c23618860916738702b5d62820c351 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 12 Sep 2017 11:34:34 +0200 Subject: [PATCH 08/86] support group runners in existing API endpoints --- lib/api/entities.rb | 18 ++- lib/api/runner.rb | 5 +- lib/api/runners.rb | 1 + lib/api/v3/runners.rb | 1 + spec/requests/api/runner_spec.rb | 15 +- spec/requests/api/runners_spec.rb | 219 +++++++++++++++++++-------- spec/requests/api/v3/runners_spec.rb | 57 +++++-- 7 files changed, 239 insertions(+), 77 deletions(-) diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 8aad320e376c..f28c4bcc7841 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -242,13 +242,18 @@ class AccessRequester < Grape::Entity expose :requested_at end - class Group < Grape::Entity - expose :id, :name, :path, :description, :visibility + class BasicGroupDetails < Grape::Entity + expose :id + expose :web_url + expose :name + end + + class Group < BasicGroupDetails + expose :path, :description, :visibility expose :lfs_enabled?, as: :lfs_enabled expose :avatar_url do |group, options| group.avatar_url(only_path: false) end - expose :web_url expose :request_access_enabled expose :full_name, :full_path @@ -965,6 +970,13 @@ class RunnerDetails < Runner options[:current_user].authorized_projects.where(id: runner.projects) end end + expose :groups, with: Entities::BasicGroupDetails do |runner, options| + if options[:current_user].admin? + runner.groups + else + options[:current_user].authorized_groups.where(id: runner.groups) + end + end end class RunnerRegistrationDetails < Grape::Entity diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 4d4fbe50f9f9..49d9b0b1b4fd 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -25,8 +25,11 @@ class Runner < Grape::API # Create shared runner. Requires admin access Ci::Runner.create(attributes.merge(is_shared: true)) elsif project = Project.find_by(runners_token: params[:token]) - # Create a specific runner for project. + # Create a specific runner for the project project.runners.create(attributes) + elsif group = Group.find_by(runners_token: params[:token]) + # Create a specific runner for the group + group.runners.create(attributes) end break forbidden! unless runner diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 5f2a95676051..ef4ec3f48006 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -199,6 +199,7 @@ def authenticate_delete_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner associated with more than one project") if runner.projects.count > 1 + forbidden!("Runner associated with more that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/lib/api/v3/runners.rb b/lib/api/v3/runners.rb index c6d9957d452c..24e10128b794 100644 --- a/lib/api/v3/runners.rb +++ b/lib/api/v3/runners.rb @@ -54,6 +54,7 @@ def authenticate_delete_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner associated with more than one project") if runner.projects.count > 1 + forbidden!("Runner associated with more that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 17c7a5118575..5ea110b4d826 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -45,7 +45,7 @@ context 'when project token is used' do let(:project) { create(:project) } - it 'creates runner' do + it 'creates project runner' do post api('/runners'), token: project.runners_token expect(response).to have_gitlab_http_status 201 @@ -54,6 +54,19 @@ expect(Ci::Runner.first.token).not_to eq(project.runners_token) end end + + context 'when group token is used' do + let(:group) { create(:group) } + + it 'creates a group runner' do + post api('/runners'), token: group.runners_token + + expect(response).to have_http_status 201 + expect(group.runners.size).to eq(1) + expect(Ci::Runner.first.token).not_to eq(registration_token) + expect(Ci::Runner.first.token).not_to eq(group.runners_token) + end + end end context 'when runner description is provided' do diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index d30f0cf36e2a..5a2d607960e2 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -8,22 +8,29 @@ let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } - let!(:shared_runner) { create(:ci_runner, :shared) } - let!(:unused_specific_runner) { create(:ci_runner) } + let(:group) { create(:group).tap { |group| group.add_owner(user) } } + let(:group2) { create(:group).tap { |group| group.add_owner(user) } } - let!(:specific_runner) do - create(:ci_runner).tap do |runner| + let!(:shared_runner) { create(:ci_runner, :shared, description: 'Shared runner') } + let!(:unused_project_runner) { create(:ci_runner) } + + let!(:project_runner) do + create(:ci_runner, description: 'Project runner').tap do |runner| create(:ci_runner_project, runner: runner, project: project) end end let!(:two_projects_runner) do - create(:ci_runner).tap do |runner| + create(:ci_runner, description: 'Two projects runner').tap do |runner| create(:ci_runner_project, runner: runner, project: project) create(:ci_runner_project, runner: runner, project: project2) end end + let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + + let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } + before do # Set project access for users create(:project_member, :master, user: user, project: project) @@ -37,9 +44,13 @@ get api('/runners', user) shared = json_response.any? { |r| r['is_shared'] } + descriptions = json_response.map { |runner| runner['description'] } expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers expect(json_response).to be_an Array + expect(descriptions).to contain_exactly( + 'Project runner', 'Group runner', 'Two projects runner', 'Two groups runner' + ) expect(shared).to be_falsey end @@ -129,10 +140,26 @@ context 'when runner is not shared' do it "returns runner's details" do - get api("/runners/#{specific_runner.id}", admin) + get api("/runners/#{project_runner.id}", admin) expect(response).to have_gitlab_http_status(200) - expect(json_response['description']).to eq(specific_runner.description) + expect(json_response['description']).to eq(project_runner.description) + end + + it "returns the project's details for a project runner" do + get api("/runners/#{project_runner.id}", admin) + + expect(json_response['projects'].first['id']).to eq(project.id) + end + + it "returns the group's details for a group runner" do + get api("/runners/#{group_runner.id}", admin) + + expect(json_response['groups'].first).to eq( + 'id' => group.id, + 'web_url' => group.web_url, + 'name' => group.name + ) end end @@ -146,10 +173,10 @@ context "runner project's administrative user" do context 'when runner is not shared' do it "returns runner's details" do - get api("/runners/#{specific_runner.id}", user) + get api("/runners/#{project_runner.id}", user) expect(response).to have_gitlab_http_status(200) - expect(json_response['description']).to eq(specific_runner.description) + expect(json_response['description']).to eq(project_runner.description) end end @@ -163,17 +190,40 @@ end end + context "runner group's administrative user" do + context 'when runner is not shared' do + it "returns runner's details" do + get api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(200) + expect(json_response['id']).to eq(group_runner.id) + end + end + end + context 'other authorized user' do - it "does not return runner's details" do - get api("/runners/#{specific_runner.id}", user2) + it "does not return project runner's details" do + get api("/runners/#{project_runner.id}", user2) + + expect(response).to have_http_status(403) + end + + it "does not return group runner's details" do + get api("/runners/#{group_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end end context 'unauthorized user' do - it "does not return runner's details" do - get api("/runners/#{specific_runner.id}") + it "does not return project runner's details" do + get api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it "does not return group runner's details" do + get api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -212,16 +262,16 @@ context 'when runner is not shared' do it 'updates runner' do - description = specific_runner.description - runner_queue_value = specific_runner.ensure_runner_queue_value + description = project_runner.description + runner_queue_value = project_runner.ensure_runner_queue_value - update_runner(specific_runner.id, admin, description: 'test') - specific_runner.reload + update_runner(project_runner.id, admin, description: 'test') + project_runner.reload expect(response).to have_gitlab_http_status(200) - expect(specific_runner.description).to eq('test') - expect(specific_runner.description).not_to eq(description) - expect(specific_runner.ensure_runner_queue_value) + expect(project_runner.description).to eq('test') + expect(project_runner.description).not_to eq(description) + expect(project_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) end end @@ -247,27 +297,49 @@ def update_runner(id, user, args) end context 'when runner is not shared' do - it 'does not update runner without access to it' do - put api("/runners/#{specific_runner.id}", user2), description: 'test' + it 'does not update project runner without access to it' do + put api("/runners/#{project_runner.id}", user2), description: 'test' + + expect(response).to have_http_status(403) + end + + it 'does not update group runner without access to it' do + put api("/runners/#{group_runner.id}", user2), description: 'test' expect(response).to have_gitlab_http_status(403) end - it 'updates runner with access to it' do - description = specific_runner.description - put api("/runners/#{specific_runner.id}", admin), description: 'test' - specific_runner.reload + it 'updates project runner with access to it' do + description = project_runner.description + put api("/runners/#{project_runner.id}", admin), description: 'test' + project_runner.reload + + expect(response).to have_gitlab_http_status(200) + expect(project_runner.description).to eq('test') + expect(project_runner.description).not_to eq(description) + end + + it 'updates group runner with access to it' do + description = group_runner.description + put api("/runners/#{group_runner.id}", admin), description: 'test' + group_runner.reload expect(response).to have_gitlab_http_status(200) - expect(specific_runner.description).to eq('test') - expect(specific_runner.description).not_to eq(description) + expect(group_runner.description).to eq('test') + expect(group_runner.description).not_to eq(description) end end end context 'unauthorized user' do - it 'does not delete runner' do - put api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + put api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + put api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -293,15 +365,23 @@ def update_runner(id, user, args) context 'when runner is not shared' do it 'deletes unused runner' do expect do - delete api("/runners/#{unused_specific_runner.id}", admin) + delete api("/runners/#{unused_project_runner.id}", admin) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes used runner' do + it 'deletes used project runner' do expect do - delete api("/runners/#{specific_runner.id}", admin) + delete api("/runners/#{project_runner.id}", admin) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'deletes used group runner' do + expect do + delete api("/runners/#{group_runner.id}", admin) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) @@ -325,32 +405,51 @@ def update_runner(id, user, args) context 'when runner is not shared' do it 'does not delete runner without access to it' do - delete api("/runners/#{specific_runner.id}", user2) + delete api("/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end - it 'does not delete runner with more than one associated project' do + it 'does not delete project runner with more than one associated project' do delete api("/runners/#{two_projects_runner.id}", user) expect(response).to have_gitlab_http_status(403) end - it 'deletes runner for one owned project' do + it 'deletes project runner for one owned project' do expect do - delete api("/runners/#{specific_runner.id}", user) + delete api("/runners/#{project_runner.id}", user) + + expect(response).to have_http_status(204) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'does not delete group runner with more than one associated group' do + delete api("/runners/#{two_groups_runner.id}", user) + expect(response).to have_http_status(403) + end + + it 'deletes group runner for one owned group' do + expect do + delete api("/runners/#{group_runner.id}", user) expect(response).to have_gitlab_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end it_behaves_like '412 response' do - let(:request) { api("/runners/#{specific_runner.id}", user) } + let(:request) { api("/runners/#{project_runner.id}", user) } end end end context 'unauthorized user' do - it 'does not delete runner' do - delete api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + delete api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + delete api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -361,8 +460,8 @@ def update_runner(id, user, args) set(:job_1) { create(:ci_build) } let!(:job_2) { create(:ci_build, :running, runner: shared_runner, project: project) } let!(:job_3) { create(:ci_build, :failed, runner: shared_runner, project: project) } - let!(:job_4) { create(:ci_build, :running, runner: specific_runner, project: project) } - let!(:job_5) { create(:ci_build, :failed, runner: specific_runner, project: project) } + let!(:job_4) { create(:ci_build, :running, runner: project_runner, project: project) } + let!(:job_5) { create(:ci_build, :failed, runner: project_runner, project: project) } context 'admin user' do context 'when runner exists' do @@ -380,7 +479,7 @@ def update_runner(id, user, args) context 'when runner is specific' do it 'return jobs' do - get api("/runners/#{specific_runner.id}/jobs", admin) + get api("/runners/#{project_runner.id}/jobs", admin) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -392,7 +491,7 @@ def update_runner(id, user, args) context 'when valid status is provided' do it 'return filtered jobs' do - get api("/runners/#{specific_runner.id}/jobs?status=failed", admin) + get api("/runners/#{project_runner.id}/jobs?status=failed", admin) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -405,7 +504,7 @@ def update_runner(id, user, args) context 'when invalid status is provided' do it 'return 400' do - get api("/runners/#{specific_runner.id}/jobs?status=non-existing", admin) + get api("/runners/#{project_runner.id}/jobs?status=non-existing", admin) expect(response).to have_gitlab_http_status(400) end @@ -433,7 +532,7 @@ def update_runner(id, user, args) context 'when runner is specific' do it 'return jobs' do - get api("/runners/#{specific_runner.id}/jobs", user) + get api("/runners/#{project_runner.id}/jobs", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -445,7 +544,7 @@ def update_runner(id, user, args) context 'when valid status is provided' do it 'return filtered jobs' do - get api("/runners/#{specific_runner.id}/jobs?status=failed", user) + get api("/runners/#{project_runner.id}/jobs?status=failed", user) expect(response).to have_gitlab_http_status(200) expect(response).to include_pagination_headers @@ -458,7 +557,7 @@ def update_runner(id, user, args) context 'when invalid status is provided' do it 'return 400' do - get api("/runners/#{specific_runner.id}/jobs?status=non-existing", user) + get api("/runners/#{project_runner.id}/jobs?status=non-existing", user) expect(response).to have_gitlab_http_status(400) end @@ -476,7 +575,7 @@ def update_runner(id, user, args) context 'other authorized user' do it 'does not return jobs' do - get api("/runners/#{specific_runner.id}/jobs", user2) + get api("/runners/#{project_runner.id}/jobs", user2) expect(response).to have_gitlab_http_status(403) end @@ -484,7 +583,7 @@ def update_runner(id, user, args) context 'unauthorized user' do it 'does not return jobs' do - get api("/runners/#{specific_runner.id}/jobs") + get api("/runners/#{project_runner.id}/jobs") expect(response).to have_gitlab_http_status(401) end @@ -523,7 +622,7 @@ def update_runner(id, user, args) describe 'POST /projects/:id/runners' do context 'authorized user' do - let(:specific_runner2) do + let(:project_runner2) do create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project2) end @@ -531,23 +630,23 @@ def update_runner(id, user, args) it 'enables specific runner' do expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner2.id end.to change { project.runners.count }.by(+1) expect(response).to have_gitlab_http_status(201) end it 'avoids changes when enabling already enabled runner' do expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner.id end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(409) end it 'does not enable locked runner' do - specific_runner2.update(locked: true) + project_runner2.update(locked: true) expect do - post api("/projects/#{project.id}/runners", user), runner_id: specific_runner2.id + post api("/projects/#{project.id}/runners", user), runner_id: project_runner2.id end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) @@ -562,7 +661,7 @@ def update_runner(id, user, args) context 'user is admin' do it 'enables any specific runner' do expect do - post api("/projects/#{project.id}/runners", admin), runner_id: unused_specific_runner.id + post api("/projects/#{project.id}/runners", admin), runner_id: unused_project_runner.id end.to change { project.runners.count }.by(+1) expect(response).to have_gitlab_http_status(201) end @@ -570,7 +669,7 @@ def update_runner(id, user, args) context 'user is not admin' do it 'does not enable runner without access to' do - post api("/projects/#{project.id}/runners", user), runner_id: unused_specific_runner.id + post api("/projects/#{project.id}/runners", user), runner_id: unused_project_runner.id expect(response).to have_gitlab_http_status(403) end @@ -619,7 +718,7 @@ def update_runner(id, user, args) context 'when runner have one associated projects' do it "does not disable project's runner" do expect do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}", user) + delete api("/projects/#{project.id}/runners/#{project_runner.id}", user) end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) end @@ -634,7 +733,7 @@ def update_runner(id, user, args) context 'authorized user without permissions' do it "does not disable project's runner" do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}", user2) + delete api("/projects/#{project.id}/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end @@ -642,7 +741,7 @@ def update_runner(id, user, args) context 'unauthorized user' do it "does not disable project's runner" do - delete api("/projects/#{project.id}/runners/#{specific_runner.id}") + delete api("/projects/#{project.id}/runners/#{project_runner.id}") expect(response).to have_gitlab_http_status(401) end diff --git a/spec/requests/api/v3/runners_spec.rb b/spec/requests/api/v3/runners_spec.rb index c91b097a3c77..c9a054078576 100644 --- a/spec/requests/api/v3/runners_spec.rb +++ b/spec/requests/api/v3/runners_spec.rb @@ -8,10 +8,16 @@ let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } + let(:group) { create(:group).tap { |group| group.add_owner(user) } } + let(:group2) { create(:group).tap { |group| group.add_owner(user) } } + + let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } + let!(:shared_runner) { create(:ci_runner, :shared) } let!(:unused_specific_runner) { create(:ci_runner) } - let!(:specific_runner) do + let!(:project_runner) do create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project) end @@ -51,9 +57,17 @@ end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes used runner' do + it 'deletes used project runner' do expect do - delete v3_api("/runners/#{specific_runner.id}", admin) + delete v3_api("/runners/#{project_runner.id}", admin) + + expect(response).to have_http_status(200) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'deletes used group runner' do + expect do + delete v3_api("/runners/#{group_runner.id}", admin) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -77,18 +91,31 @@ context 'when runner is not shared' do it 'does not delete runner without access to it' do - delete v3_api("/runners/#{specific_runner.id}", user2) + delete v3_api("/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end - it 'does not delete runner with more than one associated project' do + it 'does not delete project runner with more than one associated project' do delete v3_api("/runners/#{two_projects_runner.id}", user) expect(response).to have_gitlab_http_status(403) end - it 'deletes runner for one owned project' do + it 'deletes project runner for one owned project' do expect do - delete v3_api("/runners/#{specific_runner.id}", user) + delete v3_api("/runners/#{group_runner.id}", user) + + expect(response).to have_http_status(200) + end.to change { Ci::Runner.specific.count }.by(-1) + end + + it 'does not delete group runner with more than one associated project' do + delete v3_api("/runners/#{two_groups_runner.id}", user) + expect(response).to have_http_status(403) + end + + it 'deletes group runner for one owned project' do + expect do + delete v3_api("/runners/#{project_runner.id}", user) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -97,8 +124,14 @@ end context 'unauthorized user' do - it 'does not delete runner' do - delete v3_api("/runners/#{specific_runner.id}") + it 'does not delete project runner' do + delete v3_api("/runners/#{project_runner.id}") + + expect(response).to have_http_status(401) + end + + it 'does not delete group runner' do + delete v3_api("/runners/#{group_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -120,7 +153,7 @@ context 'when runner have one associated projects' do it "does not disable project's runner" do expect do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user) + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user) end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) end @@ -135,7 +168,7 @@ context 'authorized user without permissions' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user2) + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end @@ -143,7 +176,7 @@ context 'unauthorized user' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}") + delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}") expect(response).to have_gitlab_http_status(401) end -- GitLab From 850e327c70660a3935ca00c3d836f04695a408d3 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 12 Sep 2017 13:08:21 +0200 Subject: [PATCH 09/86] use INNER JOIN instead of LEFT JOIN as we're using UNION now we can use INNER JOIN. --- app/models/ci/runner.rb | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index cfed4a2eeea8..a488d253f199 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -29,22 +29,13 @@ class Runner < ActiveRecord::Base scope :ordered, ->() { order(id: :desc) } scope :owned_or_shared, ->(project_id) do - project_runners = joins( - %{ - LEFT JOIN ci_runner_projects ON ci_runner_projects.runner_id = ci_runners.id - } - ).where( - %{ - ci_runner_projects.project_id = :project_id - }, - project_id: project_id - ) + project_runners = joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) group_runners = joins( %{ - LEFT JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id - LEFT JOIN namespaces ON namespaces.id = ci_runner_groups.group_id - LEFT JOIN projects group_projects ON group_projects.namespace_id = namespaces.id + INNER JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id + INNER JOIN namespaces ON namespaces.id = ci_runner_groups.group_id + INNER JOIN projects group_projects ON group_projects.namespace_id = namespaces.id } ).where( %{ -- GitLab From 32a9c85bd9a320984a17fa29cd6aaa3b45e0bf4c Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 25 Sep 2017 13:34:45 +0200 Subject: [PATCH 10/86] revert support for v3 api --- lib/api/v3/runners.rb | 1 - spec/requests/api/v3/runners_spec.rb | 57 ++++++---------------------- 2 files changed, 12 insertions(+), 46 deletions(-) diff --git a/lib/api/v3/runners.rb b/lib/api/v3/runners.rb index 24e10128b794..c6d9957d452c 100644 --- a/lib/api/v3/runners.rb +++ b/lib/api/v3/runners.rb @@ -54,7 +54,6 @@ def authenticate_delete_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner associated with more than one project") if runner.projects.count > 1 - forbidden!("Runner associated with more that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/spec/requests/api/v3/runners_spec.rb b/spec/requests/api/v3/runners_spec.rb index c9a054078576..c91b097a3c77 100644 --- a/spec/requests/api/v3/runners_spec.rb +++ b/spec/requests/api/v3/runners_spec.rb @@ -8,16 +8,10 @@ let(:project) { create(:project, creator_id: user.id) } let(:project2) { create(:project, creator_id: user.id) } - let(:group) { create(:group).tap { |group| group.add_owner(user) } } - let(:group2) { create(:group).tap { |group| group.add_owner(user) } } - - let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } - let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } - let!(:shared_runner) { create(:ci_runner, :shared) } let!(:unused_specific_runner) { create(:ci_runner) } - let!(:project_runner) do + let!(:specific_runner) do create(:ci_runner).tap do |runner| create(:ci_runner_project, runner: runner, project: project) end @@ -57,17 +51,9 @@ end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes used project runner' do + it 'deletes used runner' do expect do - delete v3_api("/runners/#{project_runner.id}", admin) - - expect(response).to have_http_status(200) - end.to change { Ci::Runner.specific.count }.by(-1) - end - - it 'deletes used group runner' do - expect do - delete v3_api("/runners/#{group_runner.id}", admin) + delete v3_api("/runners/#{specific_runner.id}", admin) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -91,31 +77,18 @@ context 'when runner is not shared' do it 'does not delete runner without access to it' do - delete v3_api("/runners/#{project_runner.id}", user2) + delete v3_api("/runners/#{specific_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end - it 'does not delete project runner with more than one associated project' do + it 'does not delete runner with more than one associated project' do delete v3_api("/runners/#{two_projects_runner.id}", user) expect(response).to have_gitlab_http_status(403) end - it 'deletes project runner for one owned project' do + it 'deletes runner for one owned project' do expect do - delete v3_api("/runners/#{group_runner.id}", user) - - expect(response).to have_http_status(200) - end.to change { Ci::Runner.specific.count }.by(-1) - end - - it 'does not delete group runner with more than one associated project' do - delete v3_api("/runners/#{two_groups_runner.id}", user) - expect(response).to have_http_status(403) - end - - it 'deletes group runner for one owned project' do - expect do - delete v3_api("/runners/#{project_runner.id}", user) + delete v3_api("/runners/#{specific_runner.id}", user) expect(response).to have_gitlab_http_status(200) end.to change { Ci::Runner.specific.count }.by(-1) @@ -124,14 +97,8 @@ end context 'unauthorized user' do - it 'does not delete project runner' do - delete v3_api("/runners/#{project_runner.id}") - - expect(response).to have_http_status(401) - end - - it 'does not delete group runner' do - delete v3_api("/runners/#{group_runner.id}") + it 'does not delete runner' do + delete v3_api("/runners/#{specific_runner.id}") expect(response).to have_gitlab_http_status(401) end @@ -153,7 +120,7 @@ context 'when runner have one associated projects' do it "does not disable project's runner" do expect do - delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user) + delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user) end.to change { project.runners.count }.by(0) expect(response).to have_gitlab_http_status(403) end @@ -168,7 +135,7 @@ context 'authorized user without permissions' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}", user2) + delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}", user2) expect(response).to have_gitlab_http_status(403) end @@ -176,7 +143,7 @@ context 'unauthorized user' do it "does not disable project's runner" do - delete v3_api("/projects/#{project.id}/runners/#{project_runner.id}") + delete v3_api("/projects/#{project.id}/runners/#{specific_runner.id}") expect(response).to have_gitlab_http_status(401) end -- GitLab From 4b1b2f3b104df455d5d3265adca92dd09e079ee9 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 25 Sep 2017 15:28:33 +0200 Subject: [PATCH 11/86] add Ci::Runner#group? method --- app/models/ci/runner.rb | 4 ++++ spec/models/ci/runner_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index a488d253f199..6ffa9372c6e7 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -139,6 +139,10 @@ def specific? !shared? end + def group? + runner_groups.any? + end + def can_pick?(build) return false if self.ref_protected? && !build.protected? diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 5a851a3d5598..b9aafa63493e 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -642,4 +642,28 @@ def expect_redis_update expect(described_class.search(runner.description.upcase)).to eq([runner]) end end + + describe 'group?' do + it 'returns false when the runner is a project runner' do + project = create :project + runner = create(:ci_runner, description: 'Project runner').tap do |r| + create :ci_runner_project, runner: r, project: project + end + + expect(runner.group?).to be false + end + + it 'returns false when the runner is a shared runner' do + runner = create :ci_runner, :shared, description: 'Shared runner' + + expect(runner.group?).to be false + end + + it 'returns true when the runner is assigned to a group' do + group = create :group + runner = create :ci_runner, description: 'Group runner', groups: [group] + + expect(runner.group?).to be true + end + end end -- GitLab From d0842d20758e2f33d44b41a250d361853abe47f4 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 25 Sep 2017 15:28:49 +0200 Subject: [PATCH 12/86] disallow group runners to become project runners --- lib/api/runners.rb | 1 + spec/requests/api/runners_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/lib/api/runners.rb b/lib/api/runners.rb index ef4ec3f48006..84d33879c389 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -206,6 +206,7 @@ def authenticate_delete_runner!(runner) def authenticate_enable_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner is locked") if runner.locked? + forbidden!("Runner is a group runner") if runner.group? return if current_user.admin? forbidden!("No access granted") unless user_can_access_runner?(runner) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 5a2d607960e2..ab807e399a4d 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -658,6 +658,12 @@ def update_runner(id, user, args) expect(response).to have_gitlab_http_status(403) end + it 'does not enable group runner' do + post api("/projects/#{project.id}/runners", user), runner_id: group_runner.id + + expect(response).to have_http_status(403) + end + context 'user is admin' do it 'enables any specific runner' do expect do -- GitLab From 677291b6a7fea19f5b35638918b3cd0008dc8d15 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 25 Sep 2017 16:32:24 +0200 Subject: [PATCH 13/86] add group_runners_enabled to ci_runners --- ...2221_add_group_runners_enabled_to_projects.rb | 16 ++++++++++++++++ db/schema.rb | 2 ++ 2 files changed, 18 insertions(+) create mode 100644 db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb diff --git a/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb b/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb new file mode 100644 index 000000000000..8df7be39ee12 --- /dev/null +++ b/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb @@ -0,0 +1,16 @@ +class AddGroupRunnersEnabledToProjects < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_column_with_default :projects, :group_runners_enabled, :boolean, default: true + add_concurrent_index :projects, :group_runners_enabled + end + + def down + remove_column :projects, :group_runners_enabled + end +end diff --git a/db/schema.rb b/db/schema.rb index b2c37a65ccfa..43befa6d3b15 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1568,12 +1568,14 @@ t.boolean "merge_requests_rebase_enabled", default: false, null: false t.integer "jobs_cache_index" t.boolean "pages_https_only", default: true + t.boolean "group_runners_enabled", default: true, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree add_index "projects", ["created_at"], name: "index_projects_on_created_at", using: :btree add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree add_index "projects", ["description"], name: "index_projects_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} + add_index "projects", ["group_runners_enabled"], name: "index_projects_on_group_runners_enabled", using: :btree add_index "projects", ["id"], name: "index_projects_on_id_partial_for_visibility", unique: true, where: "(visibility_level = ANY (ARRAY[10, 20]))", using: :btree add_index "projects", ["last_activity_at"], name: "index_projects_on_last_activity_at", using: :btree add_index "projects", ["last_repository_check_failed"], name: "index_projects_on_last_repository_check_failed", using: :btree -- GitLab From 81c0c57acd0f065bc5b80902ee664256d4c3241f Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 25 Sep 2017 16:46:03 +0200 Subject: [PATCH 14/86] exclude group runners on projects that disabled it --- app/models/ci/runner.rb | 9 ++++++--- spec/models/ci/runner_spec.rb | 8 ++++++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6ffa9372c6e7..2f4342b79aa8 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -35,13 +35,16 @@ class Runner < ActiveRecord::Base %{ INNER JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id INNER JOIN namespaces ON namespaces.id = ci_runner_groups.group_id - INNER JOIN projects group_projects ON group_projects.namespace_id = namespaces.id + INNER JOIN projects ON projects.namespace_id = namespaces.id } ).where( %{ - group_projects.id = :project_id + projects.id = :project_id + AND + projects.group_runners_enabled = :true }, - project_id: project_id + project_id: project_id, + true: true ) shared_runners = where(is_shared: true) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index b9aafa63493e..933bd6e5f23f 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -83,6 +83,14 @@ expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] end + it 'does not return the group runner if the project has group runners disabled' do + specific_group = create :group + specific_project = create :project, group: specific_group, group_runners_enabled: false + create :ci_runner, :specific, groups: [specific_group] + + expect(described_class.owned_or_shared(specific_project.id)).to be_empty + end + it 'returns the shared group runner' do group = create :group runner = create :ci_runner, :shared, groups: [group] -- GitLab From d6167a9214b3a3c13850cdac9895c9d7577ddf25 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 09:27:27 +0200 Subject: [PATCH 15/86] split up Ci::Runner.owned_or_shared scope --- app/models/ci/runner.rb | 32 +++++++++++++------------- spec/models/ci/runner_spec.rb | 42 ++++++++++++++++++++--------------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 2f4342b79aa8..b7af33c0480d 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -21,17 +21,17 @@ class Runner < ActiveRecord::Base before_validation :set_default_values - scope :specific, ->() { where(is_shared: false) } - scope :shared, ->() { where(is_shared: true) } - scope :active, ->() { where(active: true) } - scope :paused, ->() { where(active: false) } - scope :online, ->() { where('contacted_at > ?', contact_time_deadline) } - scope :ordered, ->() { order(id: :desc) } - - scope :owned_or_shared, ->(project_id) do - project_runners = joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) - - group_runners = joins( + scope :specific, -> { where(is_shared: false) } + scope :shared, -> { where(is_shared: true) } + scope :active, -> { where(active: true) } + scope :paused, -> { where(active: false) } + scope :online, -> { where('contacted_at > ?', contact_time_deadline) } + scope :ordered, -> { order(id: :desc) } + scope :belonging_to_project, -> (project_id) { + joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) + } + scope :belonging_to_group, -> (project_id) { + joins( %{ INNER JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id INNER JOIN namespaces ON namespaces.id = ci_runner_groups.group_id @@ -43,13 +43,13 @@ class Runner < ActiveRecord::Base AND projects.group_runners_enabled = :true }, - project_id: project_id, - true: true + project_id: project_id, + true: true ) + } - shared_runners = where(is_shared: true) - - union = Gitlab::SQL::Union.new([project_runners, group_runners, shared_runners]) + scope :owned_or_shared, -> (project_id) do + union = Gitlab::SQL::Union.new([belonging_to_project(project_id), belonging_to_group(project_id), shared]) from("(#{union.to_sql}) ci_runners") end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 933bd6e5f23f..a073af7c4b28 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -49,7 +49,23 @@ end end - describe '.owned_or_shared' do + describe '.shared' do + it 'returns the shared group runner' do + group = create :group + runner = create :ci_runner, :shared, groups: [group] + + expect(described_class.shared).to eq [runner] + end + + it 'returns the shared project runner' do + project = create :project + runner = create :ci_runner, :shared, projects: [project] + + expect(described_class.shared).to eq [runner] + end + end + + describe '.belonging_to_project' do it 'returns the specific project runner' do # own specific_project = create :project @@ -59,16 +75,11 @@ other_project = create :project create :ci_runner, :specific, projects: [other_project] - expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] - end - - it 'returns the shared project runner' do - project = create :project - runner = create :ci_runner, :shared, projects: [project] - - expect(described_class.owned_or_shared(0)).to eq [runner] + expect(described_class.belonging_to_project(specific_project.id)).to eq [specific_runner] end + end + describe '.belonging_to_group' do it 'returns the specific group runner' do # own specific_group = create :group @@ -80,7 +91,7 @@ create :project, group: other_group create :ci_runner, :specific, groups: [other_group] - expect(described_class.owned_or_shared(specific_project.id)).to eq [specific_runner] + expect(described_class.belonging_to_group(specific_project.id)).to eq [specific_runner] end it 'does not return the group runner if the project has group runners disabled' do @@ -88,16 +99,11 @@ specific_project = create :project, group: specific_group, group_runners_enabled: false create :ci_runner, :specific, groups: [specific_group] - expect(described_class.owned_or_shared(specific_project.id)).to be_empty - end - - it 'returns the shared group runner' do - group = create :group - runner = create :ci_runner, :shared, groups: [group] - - expect(described_class.owned_or_shared(0)).to eq [runner] + expect(described_class.belonging_to_group(specific_project.id)).to be_empty end + end + describe '.owned_or_shared' do it 'returns a globally shared, a project specific and a group specific runner' do # group specific group = create :group -- GitLab From 8dad45a82228a6f1c87f919063d96c8b20a567e2 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 13:55:34 +0200 Subject: [PATCH 16/86] add method CI::Runner.project? --- app/models/ci/runner.rb | 4 ++++ spec/models/ci/runner_spec.rb | 26 +++++++++++++++++++++++--- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index b7af33c0480d..586740a4a2a7 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -146,6 +146,10 @@ def group? runner_groups.any? end + def project? + runner_projects.any? + end + def can_pick?(build) return false if self.ref_protected? && !build.protected? diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index a073af7c4b28..308db9e8e688 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -660,9 +660,7 @@ def expect_redis_update describe 'group?' do it 'returns false when the runner is a project runner' do project = create :project - runner = create(:ci_runner, description: 'Project runner').tap do |r| - create :ci_runner_project, runner: r, project: project - end + runner = create :ci_runner, description: 'Project runner', projects: [project] expect(runner.group?).to be false end @@ -680,4 +678,26 @@ def expect_redis_update expect(runner.group?).to be true end end + + describe 'project?' do + it 'returns false when the runner is a group prunner' do + group = create :group + runner = create :ci_runner, description: 'Group runner', groups: [group] + + expect(runner.project?).to be false + end + + it 'returns false when the runner is a shared runner' do + runner = create :ci_runner, :shared, description: 'Shared runner' + + expect(runner.project?).to be false + end + + it 'returns true when the runner is assigned to a project' do + project = create :project + runner = create :ci_runner, description: 'Group runner', projects: [project] + + expect(runner.project?).to be true + end + end end -- GitLab From eba1a05f153335cb41bbf9396c7e88336a6b6be5 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 13:57:50 +0200 Subject: [PATCH 17/86] ensure_runners_token on read instead of write 1. we don't want to migrate all existing groups 2. we generate the token when showing the runners page, as this is the first time that the token will be used. --- app/models/group.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/group.rb b/app/models/group.rb index c34c913a16bf..95e2c3a8aab2 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -47,7 +47,6 @@ class Group < Namespace validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } add_authentication_token_field :runners_token - before_save :ensure_runners_token after_create :post_create_hook after_destroy :post_destroy_hook @@ -296,6 +295,13 @@ def refresh_project_authorizations refresh_members_authorized_projects(blocking: false) end + # each existing group needs to have a `runners_token`. + # we do this on read since migrating all existing groups is not a feasible + # solution. + def runners_token + ensure_runners_token! + end + private def update_two_factor_requirement -- GitLab From 4ccf734e380d498a2322153c9a4fa09a38447094 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 13:59:51 +0200 Subject: [PATCH 18/86] show group runners on runners page --- .../projects/settings/ci_cd_controller.rb | 4 ++ .../projects/runners/_group_runners.html.haml | 19 +++++++++ app/views/projects/runners/_index.html.haml | 4 ++ app/views/projects/runners/_runner.html.haml | 2 +- spec/features/runners_spec.rb | 40 +++++++++++++++++++ 5 files changed, 68 insertions(+), 1 deletion(-) create mode 100644 app/views/projects/runners/_group_runners.html.haml diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index d80ef8113aa9..05d545e97a70 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -67,10 +67,14 @@ def define_variables def define_runners_variables @project_runners = @project.runners.ordered + @assignable_runners = current_user.ci_authorized_runners .assignable_for(project).ordered.page(params[:page]).per(20) @shared_runners = ::Ci::Runner.shared.active + @shared_runners_count = @shared_runners.count(:all) + + @group_runners = ::Ci::Runner.belonging_to_group(@project.id) end def define_secret_variables diff --git a/app/views/projects/runners/_group_runners.html.haml b/app/views/projects/runners/_group_runners.html.haml new file mode 100644 index 000000000000..dbdfda740e3f --- /dev/null +++ b/app/views/projects/runners/_group_runners.html.haml @@ -0,0 +1,19 @@ +%h3 Group Runners + +.bs-callout.bs-callout-warning + GitLab Group Runners can execute code for all the projects in this group. + They can be managed using the #{link_to 'Runners API', help_page_path('api/runners.md')}. + +- if !@project.group + This project does not belong to a group and can therefore not make use of group Runners. + +- elsif @group_runners.empty? + This group does not provide any group Runners yet. + + = render partial: 'ci/runner/how_to_setup_runner', + locals: { registration_token: @project.group.runners_token, type: 'group' } + +- else + %h4.underlined-title Available group Runners : #{@group_runners.count} + %ul.bordered-list + = render partial: 'projects/runners/runner', collection: @group_runners, as: :runner diff --git a/app/views/projects/runners/_index.html.haml b/app/views/projects/runners/_index.html.haml index f9808f7c990f..3f5119d408b5 100644 --- a/app/views/projects/runners/_index.html.haml +++ b/app/views/projects/runners/_index.html.haml @@ -23,3 +23,7 @@ = render 'projects/runners/specific_runners' .col-sm-6 = render 'projects/runners/shared_runners' +.row + .col-sm-6 + .col-sm-6 + = render 'projects/runners/group_runners' diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 6376496ee1aa..6d61da40f5b6 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -26,7 +26,7 @@ - else - runner_project = @project.runner_projects.find_by(runner_id: runner) = link_to 'Disable for this project', project_runner_project_path(@project, runner_project), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - - elsif runner.specific? + - elsif runner.project? = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f| = f.hidden_field :runner_id, value: runner.id = f.submit 'Enable for this project', class: 'btn btn-sm' diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index df65c2d2f834..f34aeb5bd5ea 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -181,4 +181,44 @@ expect(page.find('.shared-runners-description')).to have_content('Disable shared Runners') end end + + context 'group runners' do + background do + project.add_master(user) + end + + context 'project without a group' do + given(:project) { create :project } + + scenario 'group runners are not available' do + visit runners_path(project) + + expect(page).to have_content 'This project does not belong to a group and can therefore not make use of group Runners.' + end + end + + context 'project with a group but no group runner' do + given(:group) { create :group } + given(:project) { create :project, group: group } + + scenario 'group runners are not available' do + visit runners_path(project) + + expect(page).to have_content 'This group does not provide any group Runners yet.' + end + end + + context 'project with a group and a group runner' do + given(:group) { create :group } + given(:project) { create :project, group: group } + given!(:ci_runner) { create :ci_runner, groups: [group], description: 'group-runner' } + + scenario 'group runners are available' do + visit runners_path(project) + + expect(page).to have_content 'Available group Runners : 1' + expect(page).to have_content 'group-runner' + end + end + end end -- GitLab From d588adff1a3ce87355c8b5ac09a77e6fc63fe89a Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 15:51:23 +0200 Subject: [PATCH 19/86] don't filter group runners by project flag the scope `Ci::Runner.belonging_to_group` does not filter out the runners where the projects has `#group_runners_enabled` set to false anymore. it didn't show up in the runners UI anymore when group runners were disabled. this was confusing. the flag is only relevant when selecting appropriate runner for a build. --- app/models/ci/runner.rb | 10 +--------- spec/models/ci/runner_spec.rb | 8 -------- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 586740a4a2a7..9efafa8681f7 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -37,15 +37,7 @@ class Runner < ActiveRecord::Base INNER JOIN namespaces ON namespaces.id = ci_runner_groups.group_id INNER JOIN projects ON projects.namespace_id = namespaces.id } - ).where( - %{ - projects.id = :project_id - AND - projects.group_runners_enabled = :true - }, - project_id: project_id, - true: true - ) + ).where('projects.id = :project_id', project_id: project_id) } scope :owned_or_shared, -> (project_id) do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 308db9e8e688..0cc52acd44df 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -93,14 +93,6 @@ expect(described_class.belonging_to_group(specific_project.id)).to eq [specific_runner] end - - it 'does not return the group runner if the project has group runners disabled' do - specific_group = create :group - specific_project = create :project, group: specific_group, group_runners_enabled: false - create :ci_runner, :specific, groups: [specific_group] - - expect(described_class.belonging_to_group(specific_project.id)).to be_empty - end end describe '.owned_or_shared' do -- GitLab From 8d61d33d37f7b8f99eae73e4ba0b48fbe35a80dc Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 17:11:53 +0200 Subject: [PATCH 20/86] use .owned_or_shared for #assignable_for? instead of having the explicit logic duplicated from the scope we can use the scope instead. --- app/models/ci/runner.rb | 2 +- spec/models/ci/runner_spec.rb | 24 +++++++++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 9efafa8681f7..8a6de26164df 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -227,7 +227,7 @@ def tag_constraints end def assignable_for?(project_id) - is_shared? || projects.exists?(id: project_id) + self.class.owned_or_shared(project_id).where(id: self.id).any? end def accepting_tags?(build) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 0cc52acd44df..1456c44dbf6e 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -236,6 +236,13 @@ def stub_redis_runner_contacted_at(value) build.project.runners << runner end + context 'a different runner' do + it 'cannot handle builds' do + other_runner = create :ci_runner + expect(other_runner.can_pick?(build)).to be_falsey + end + end + context 'when runner does not have tags' do it 'can handle builds without tags' do expect(runner.can_pick?(build)).to be_truthy @@ -277,7 +284,7 @@ def stub_redis_runner_contacted_at(value) context 'when runner cannot pick untagged jobs' do before do - runner.run_untagged = false + runner.update_attributes!(run_untagged: false) end it 'cannot handle builds without tags' do @@ -290,7 +297,7 @@ def stub_redis_runner_contacted_at(value) context 'when runner is shared' do before do - runner.is_shared = true + runner.update_attributes!(is_shared: true) build.project.runners = [] end @@ -300,7 +307,7 @@ def stub_redis_runner_contacted_at(value) context 'when runner is locked' do before do - runner.locked = true + runner.update_attributes!(locked: true) end it 'can handle builds' do @@ -325,6 +332,17 @@ def stub_redis_runner_contacted_at(value) expect(runner.can_pick?(build)).to be_falsey end end + + context 'when runner is assigned to a group' do + before do + build.project.runners = [] + runner.groups << create(:group, projects: [build.project]) + end + + it 'can handle builds' do + expect(runner.can_pick?(build)).to be_truthy + end + end end context 'when access_level of runner is not_protected' do -- GitLab From d8675bd45f6f9b5af701343fbff7650f49559048 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 4 Oct 2017 17:37:05 +0200 Subject: [PATCH 21/86] select group runners also in build queue service --- app/services/ci/update_build_queue_service.rb | 16 +++-- .../ci/update_build_queue_service_spec.rb | 62 +++++++++++++++---- 2 files changed, 62 insertions(+), 16 deletions(-) diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 152c8ae5006c..4fb03d2fa1b4 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -7,11 +7,19 @@ def execute(build) end end - return unless build.project.shared_runners_enabled? + if build.project.group_runners_enabled? + Ci::Runner.belonging_to_group(build.project_id).each do |runner| + if runner.can_pick?(build) + runner.tick_runner_queue + end + end + end - Ci::Runner.shared.each do |runner| - if runner.can_pick?(build) - runner.tick_runner_queue + if build.project.shared_runners_enabled? + Ci::Runner.shared.each do |runner| + if runner.can_pick?(build) + runner.tick_runner_queue + end end end end diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 0da0e57dbcdb..74a23ed2a3f2 100644 --- a/spec/services/ci/update_build_queue_service_spec.rb +++ b/spec/services/ci/update_build_queue_service_spec.rb @@ -8,21 +8,19 @@ context 'when updating specific runners' do let(:runner) { create(:ci_runner) } - context 'when there are runner that can pick build' do + context 'when there is a runner that can pick build' do before do build.project.runners << runner end it 'ticks runner queue value' do - expect { subject.execute(build) } - .to change { runner.ensure_runner_queue_value } + expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } end end - context 'when there are no runners that can pick build' do + context 'when there is no runner that can pick build' do it 'does not tick runner queue value' do - expect { subject.execute(build) } - .not_to change { runner.ensure_runner_queue_value } + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } end end end @@ -30,21 +28,61 @@ context 'when updating shared runners' do let(:runner) { create(:ci_runner, :shared) } - context 'when there are runner that can pick build' do + context 'when there is no runner that can pick build' do it 'ticks runner queue value' do - expect { subject.execute(build) } - .to change { runner.ensure_runner_queue_value } + expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } end end - context 'when there are no runners that can pick build' do + context 'when there is no runner that can pick build due to tag mismatch' do before do build.tag_list = [:docker] end it 'does not tick runner queue value' do - expect { subject.execute(build) } - .not_to change { runner.ensure_runner_queue_value } + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } + end + end + + context 'when there is no runner that can pick build due to being disabled on project' do + before do + build.project.shared_runners_enabled = false + end + + it 'does not tick runner queue value' do + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } + end + end + end + + context 'when updating group runners' do + let(:group) { create :group } + let(:project) { create :project, group: group } + let(:runner) { create :ci_runner, groups: [group] } + + context 'when there is a runner that can pick build' do + it 'ticks runner queue value' do + expect { subject.execute(build) }.to change { runner.ensure_runner_queue_value } + end + end + + context 'when there is no runner that can pick build due to tag mismatch' do + before do + build.tag_list = [:docker] + end + + it 'does not tick runner queue value' do + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } + end + end + + context 'when there is no runner that can pick build due to being disabled on project' do + before do + build.project.group_runners_enabled = false + end + + it 'does not tick runner queue value' do + expect { subject.execute(build) }.not_to change { runner.ensure_runner_queue_value } end end end -- GitLab From 7584d03ab12133ab6a64b473ece56b72612abad8 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 09:55:01 +0200 Subject: [PATCH 22/86] allow disabling/enabling group runners per project --- app/controllers/projects/runners_controller.rb | 6 ++++++ .../projects/runners/_group_runners.html.haml | 10 ++++++++++ config/routes/project.rb | 1 + spec/features/runners_spec.rb | 14 ++++++++++++++ 4 files changed, 31 insertions(+) diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index c950d0f7001b..87c19eeed3e8 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -52,6 +52,12 @@ def toggle_shared_runners redirect_to project_settings_ci_cd_path(@project) end + def toggle_group_runners + project.toggle!(:group_runners_enabled) + + redirect_to project_settings_ci_cd_path(@project) + end + protected def set_runner diff --git a/app/views/projects/runners/_group_runners.html.haml b/app/views/projects/runners/_group_runners.html.haml index dbdfda740e3f..785abba945bd 100644 --- a/app/views/projects/runners/_group_runners.html.haml +++ b/app/views/projects/runners/_group_runners.html.haml @@ -4,6 +4,16 @@ GitLab Group Runners can execute code for all the projects in this group. They can be managed using the #{link_to 'Runners API', help_page_path('api/runners.md')}. + - if @project.group + %hr + - if @project.group_runners_enabled? + = link_to toggle_group_runners_project_runners_path(@project), class: 'btn btn-warning', method: :post do + Disable group Runners + - else + = link_to toggle_group_runners_project_runners_path(@project), class: 'btn btn-success', method: :post do + Enable group Runners +   for this project + - if !@project.group This project does not belong to a group and can therefore not make use of group Runners. diff --git a/config/routes/project.rb b/config/routes/project.rb index 2a1bcb8cde27..382d5b1e3c7a 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -410,6 +410,7 @@ collection do post :toggle_shared_runners + post :toggle_group_runners end end diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index f34aeb5bd5ea..dcd06a4015d7 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -219,6 +219,20 @@ expect(page).to have_content 'Available group Runners : 1' expect(page).to have_content 'group-runner' end + + scenario 'group runners may be disabled for a project' do + visit runners_path(project) + + click_on 'Disable group Runners' + + expect(page).to have_content 'Enable group Runners' + expect(project.reload.group_runners_enabled).to be false + + click_on 'Enable group Runners' + + expect(page).to have_content 'Disable group Runners' + expect(project.reload.group_runners_enabled).to be true + end end end end -- GitLab From 743c32270e2913a19999bd32d6208e80dd62dc2a Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 13:29:21 +0200 Subject: [PATCH 23/86] select group runners also in register_job_service --- app/services/ci/register_job_service.rb | 45 ++++++++--- spec/services/ci/register_job_service_spec.rb | 79 +++++++++++++++++-- 2 files changed, 110 insertions(+), 14 deletions(-) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 0b087ad73da1..000ae3539e3f 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -17,6 +17,8 @@ def execute builds = if runner.shared? builds_for_shared_runner + elsif runner.group? + builds_for_group_runner else builds_for_specific_runner end @@ -69,16 +71,33 @@ def execute 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, pending_delete: false }) + builds_for_scheduled_runner( + running_builds_for_shared_runners, + projects: { shared_runners_enabled: true } + ) + end + + def builds_for_group_runner + builds_for_scheduled_runner( + running_builds_for_group_runners, + projects: { group_runners_enabled: true } + ) + 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'). + .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") + # 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 @@ -87,7 +106,15 @@ def builds_for_specific_runner end def running_builds_for_shared_runners - Ci::Build.running.where(runner: Ci::Runner.shared) + 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) .group(:project_id).select(:project_id, 'count(*) AS running_builds') end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 8a537e83d5f2..138ef673527a 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -2,11 +2,13 @@ module Ci describe RegisterJobService do - let!(:project) { FactoryBot.create :project, shared_runners_enabled: false } - let!(:pipeline) { FactoryBot.create :ci_pipeline, project: project } - let!(:pending_job) { FactoryBot.create :ci_build, pipeline: pipeline } - let!(:shared_runner) { FactoryBot.create(:ci_runner, is_shared: true) } - let!(:specific_runner) { FactoryBot.create(:ci_runner, is_shared: false) } + let!(:project) { create :project, shared_runners_enabled: false } + let!(:group) { create :group } + let!(:pipeline) { create :ci_pipeline, project: project } + let!(:pending_job) { create :ci_build, pipeline: pipeline } + let!(:shared_runner) { create :ci_runner, is_shared: true } + let!(:specific_runner) { create :ci_runner, is_shared: false } + let!(:group_runner) { create :ci_runner, groups: [group] } before do specific_runner.assign_to(project) @@ -167,6 +169,73 @@ module Ci end end + context 'allow group runners' do + before do + project.update!(group_runners_enabled: true, group: group) + end + + context 'for multiple builds' do + let!(:project2) { create :project, group_runners_enabled: true, group: group } + 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 } + let!(:build1_project2) { create :ci_build, pipeline: pipeline2 } + let!(:build2_project2) { create :ci_build, pipeline: pipeline2 } + let!(:build1_project3) { create :ci_build, pipeline: pipeline3 } + + 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) + 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) + end + end + + context 'group runner' do + let(:build) { execute(group_runner) } + + it { expect(build).to be_kind_of(Build) } + it { expect(build).to be_valid } + it { expect(build).to be_running } + it { expect(build.runner).to eq(group_runner) } + end + end + + context 'disallow group runners' do + before do + project.update(group_runners_enabled: false) + end + + context 'group runner' do + let(:build) { execute(group_runner) } + + it { expect(build).to be_nil } + end + end + context 'when first build is stalled' do before do pending_job.update(lock_version: 0) -- GitLab From a5f5a27df5d3055612fc8c2686ef3b9ab20bd85e Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 13:53:18 +0200 Subject: [PATCH 24/86] include group runners in Project#any_runners? --- app/models/project.rb | 18 +++++--- spec/models/project_spec.rb | 87 ++++++++++++++++++++++++++----------- 2 files changed, 73 insertions(+), 32 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index cec1e705aa8f..9a096ba1a7bd 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1298,20 +1298,26 @@ def enable_ci project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED) end - def shared_runners_available? - shared_runners_enabled? - end - def shared_runners - @shared_runners ||= shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none + @shared_runners ||= shared_runners_enabled? ? Ci::Runner.shared : Ci::Runner.none end def active_shared_runners @active_shared_runners ||= shared_runners.active end + def group_runners + @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_group(self.id) : Ci::Runner.none + end + + def active_group_runners + @active_group_runners ||= group_runners.active + end + def any_runners?(&block) - active_runners.any?(&block) || active_shared_runners.any?(&block) + active_runners.any?(&block) || + active_shared_runners.any?(&block) || + active_group_runners.any?(&block) end def valid_runners_token?(token) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 2675c2f52c1b..9657e5011b18 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1139,44 +1139,79 @@ def create_pipeline end describe '#any_runners' do - let(:project) { create(:project, shared_runners_enabled: shared_runners_enabled) } - let(:specific_runner) { create(:ci_runner) } - let(:shared_runner) { create(:ci_runner, :shared) } + context 'shared runners' do + let(:project) { create :project, shared_runners_enabled: shared_runners_enabled } + let(:specific_runner) { create :ci_runner } + let(:shared_runner) { create :ci_runner, :shared } - context 'for shared runners disabled' do - let(:shared_runners_enabled) { false } + context 'for shared runners disabled' do + let(:shared_runners_enabled) { false } - it 'has no runners available' do - expect(project.any_runners?).to be_falsey - end + it 'has no runners available' do + expect(project.any_runners?).to be_falsey + end - it 'has a specific runner' do - project.runners << specific_runner - expect(project.any_runners?).to be_truthy - end + it 'has a specific runner' do + project.runners << specific_runner + expect(project.any_runners?).to be_truthy + end + + it 'has a shared runner, but they are prohibited to use' do + shared_runner + expect(project.any_runners?).to be_falsey + end - it 'has a shared runner, but they are prohibited to use' do - shared_runner - expect(project.any_runners?).to be_falsey + it 'checks the presence of specific runner' do + project.runners << specific_runner + expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy + end end - it 'checks the presence of specific runner' do - project.runners << specific_runner - expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy + context 'for shared runners enabled' do + let(:shared_runners_enabled) { true } + + it 'has a shared runner' do + shared_runner + expect(project.any_runners?).to be_truthy + end + + it 'checks the presence of shared runner' do + shared_runner + expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy + end end end - context 'for shared runners enabled' do - let(:shared_runners_enabled) { true } + context 'group runners' do + let(:project) { create :project, group_runners_enabled: group_runners_enabled } + let(:group) { create :group, projects: [project] } + let(:group_runner) { create :ci_runner, groups: [group] } + + context 'for group runners disabled' do + let(:group_runners_enabled) { false } - it 'has a shared runner' do - shared_runner - expect(project.any_runners?).to be_truthy + it 'has no runners available' do + expect(project.any_runners?).to be_falsey + end + + it 'has a group runner, but they are prohibited to use' do + group_runner + expect(project.any_runners?).to be_falsey + end end - it 'checks the presence of shared runner' do - shared_runner - expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy + context 'for group runners enabled' do + let(:group_runners_enabled) { true } + + it 'has a group runner' do + group_runner + expect(project.any_runners?).to be_truthy + end + + it 'checks the presence of group runner' do + group_runner + expect(project.any_runners? { |runner| runner == group_runner }).to be_truthy + end end end end -- GitLab From 6077c569a6cc323c5eab09486e5254c5f96ce601 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 14:25:31 +0200 Subject: [PATCH 25/86] denote group runners on admin runners page --- app/views/admin/runners/_runner.html.haml | 4 ++- app/views/admin/runners/index.html.haml | 3 ++ spec/features/admin/admin_runners_spec.rb | 41 +++++++++++++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index e1cee5849290..f1e0e3b5ad62 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -2,6 +2,8 @@ %td - if runner.shared? %span.label.label-success shared + - elsif runner.group? + %span.label.label-success group - else %span.label.label-info specific - if runner.locked? @@ -19,7 +21,7 @@ %td = runner.ip_address %td - - if runner.shared? + - if runner.shared? || runner.group? n/a - else = runner.projects.count(:all) diff --git a/app/views/admin/runners/index.html.haml b/app/views/admin/runners/index.html.haml index 9f13dbbbd82f..1a3b5e58ed50 100644 --- a/app/views/admin/runners/index.html.haml +++ b/app/views/admin/runners/index.html.haml @@ -16,6 +16,9 @@ %li %span.label.label-success shared \- Runner runs jobs from all unassigned projects + %li + %span.label.label-success group + \- Runner runs jobs from all unassigned projects in its group %li %span.label.label-info specific \- Runner runs jobs from assigned projects diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index 8de2e3d199b9..b0aa2e8b5880 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -59,6 +59,47 @@ expect(page).to have_text 'No runners found' end end + + context 'group runner' do + it 'shows the label and does not show the project count' do + group = create :group + runner = create :ci_runner, groups: [group] + + visit admin_runners_path + + within "#runner_#{runner.id}" do + expect(page).to have_selector '.label', text: 'group' + expect(page).to have_text 'n/a' + end + end + end + + context 'shared runner' do + it 'shows the label and does not show the project count' do + runner = create :ci_runner, :shared + + visit admin_runners_path + + within "#runner_#{runner.id}" do + expect(page).to have_selector '.label', text: 'shared' + expect(page).to have_text 'n/a' + end + end + end + + context 'specific runner' do + it 'shows the label and the project count' do + project = create :project + runner = create :ci_runner, projects: [project] + + visit admin_runners_path + + within "#runner_#{runner.id}" do + expect(page).to have_selector '.label', text: 'specific' + expect(page).to have_text '1' + end + end + end end describe "Runner show page" do -- GitLab From 6bd3cb044d11ee7d81195191d7cb15ff2f96e8d6 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 14:26:36 +0200 Subject: [PATCH 26/86] different text on admin runner page for group r. --- app/views/admin/runners/show.html.haml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index 37269862de60..ae5f860d0d19 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -19,6 +19,9 @@ %p If you want Runners to build only specific projects, enable them in the table below. Keep in mind that this is a one way transition. +- elsif @runner.group? + .bs-callout.bs-callout-success + %h4 This runner will process jobs from all projects in its group and subgroups - else .bs-callout.bs-callout-info %h4 This Runner will process jobs only from ASSIGNED projects -- GitLab From 9b836b83bcabb4b977afc0e06897c4f1509215b0 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 5 Oct 2017 20:53:24 +0200 Subject: [PATCH 27/86] support group hierarchies for group runners --- app/models/ci/runner.rb | 11 ++++------- spec/models/ci/runner_spec.rb | 9 +++++++++ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8a6de26164df..b220ece3092d 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -31,13 +31,10 @@ class Runner < ActiveRecord::Base joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) } scope :belonging_to_group, -> (project_id) { - joins( - %{ - INNER JOIN ci_runner_groups ON ci_runner_groups.runner_id = ci_runners.id - INNER JOIN namespaces ON namespaces.id = ci_runner_groups.group_id - INNER JOIN projects ON projects.namespace_id = namespaces.id - } - ).where('projects.id = :project_id', project_id: project_id) + project_groups = ::Group.joins(:projects).where(projects: { id: project_id }) + hierarchy_groups = Gitlab::GroupHierarchy.new(project_groups).base_and_ancestors + + joins(:groups).where(namespaces: { id: hierarchy_groups }) } scope :owned_or_shared, -> (project_id) do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 1456c44dbf6e..ba0db43a1e74 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -93,6 +93,15 @@ expect(described_class.belonging_to_group(specific_project.id)).to eq [specific_runner] end + + it 'returns the group runner from a parent group' do + parent_group = create :group + group = create :group, parent: parent_group + project = create :project, group: group + runner = create :ci_runner, :specific, groups: [parent_group] + + expect(described_class.belonging_to_group(project.id)).to eq [runner] + end end describe '.owned_or_shared' do -- GitLab From d66941002ed946b6266642515a91284dec4105c6 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 30 Oct 2017 16:17:17 +0100 Subject: [PATCH 28/86] dry up: extract method --- app/services/ci/update_build_queue_service.rb | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 4fb03d2fa1b4..ab81766abf39 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -1,26 +1,22 @@ module Ci class UpdateBuildQueueService def execute(build) - build.project.runners.each do |runner| - if runner.can_pick?(build) - runner.tick_runner_queue - end - end + tick_for(build, build.project.runners) if build.project.group_runners_enabled? - Ci::Runner.belonging_to_group(build.project_id).each do |runner| - if runner.can_pick?(build) - runner.tick_runner_queue - end - end + tick_for(build, Ci::Runner.belonging_to_group(build.project_id)) end if build.project.shared_runners_enabled? - Ci::Runner.shared.each do |runner| - if runner.can_pick?(build) - runner.tick_runner_queue - end - end + tick_for(build, Ci::Runner.shared) + end + end + + private + + def tick_for(build, runners) + runners.each do |runner| + runner.tick_runner_queue if runner.can_pick?(build) end end end -- GitLab From a2a7ad291f64a5db74c1bc21fb556e6e8862d0f3 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 8 Nov 2017 10:21:54 +0100 Subject: [PATCH 29/86] add project_settings (Project#settings) --- app/models/project.rb | 8 ++++++++ app/models/project_settings.rb | 3 +++ .../20171030155459_create_project_settings.rb | 12 +++++++++++ db/schema.rb | 7 +++++++ spec/factories/project_settings.rb | 5 +++++ spec/models/project_spec.rb | 20 +++++++++++++++++++ 6 files changed, 55 insertions(+) create mode 100644 app/models/project_settings.rb create mode 100644 db/migrate/20171030155459_create_project_settings.rb create mode 100644 spec/factories/project_settings.rb diff --git a/app/models/project.rb b/app/models/project.rb index 9a096ba1a7bd..dadd07558485 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -232,6 +232,14 @@ class Project < ActiveRecord::Base has_many :project_badges, class_name: 'ProjectBadge' + has_one :settings, -> (project) { + query = where(project_id: project) + query.presence || begin + ProjectSettings.create(project_id: project.id) + query + end + }, class_name: 'ProjectSettings' + accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :import_data diff --git a/app/models/project_settings.rb b/app/models/project_settings.rb new file mode 100644 index 000000000000..b126f66fafac --- /dev/null +++ b/app/models/project_settings.rb @@ -0,0 +1,3 @@ +class ProjectSettings < ActiveRecord::Base + belongs_to :project +end diff --git a/db/migrate/20171030155459_create_project_settings.rb b/db/migrate/20171030155459_create_project_settings.rb new file mode 100644 index 000000000000..eeb449505b8b --- /dev/null +++ b/db/migrate/20171030155459_create_project_settings.rb @@ -0,0 +1,12 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class CreateProjectSettings < ActiveRecord::Migration + DOWNTIME = false + + def change + create_table :project_settings do |t| + t.references :project, index: true, foreign_key: { on_delete: :cascade } + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 43befa6d3b15..b199342dc15b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1504,6 +1504,12 @@ add_index "project_import_data", ["project_id"], name: "index_project_import_data_on_project_id", using: :btree + create_table "project_settings", force: :cascade do |t| + t.integer "project_id" + end + + add_index "project_settings", ["project_id"], name: "index_project_settings_on_project_id", using: :btree + create_table "project_statistics", force: :cascade do |t| t.integer "project_id", null: false t.integer "namespace_id", null: false @@ -2185,6 +2191,7 @@ add_foreign_key "project_features", "projects", name: "fk_18513d9b92", on_delete: :cascade add_foreign_key "project_group_links", "projects", name: "fk_daa8cee94c", on_delete: :cascade add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade + add_foreign_key "project_settings", "projects", on_delete: :cascade add_foreign_key "project_statistics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches", name: "fk_8a3072ccb3", on_delete: :cascade add_foreign_key "protected_branch_push_access_levels", "protected_branches", name: "fk_9ffc86a3d9", on_delete: :cascade diff --git a/spec/factories/project_settings.rb b/spec/factories/project_settings.rb new file mode 100644 index 000000000000..90240ee6de09 --- /dev/null +++ b/spec/factories/project_settings.rb @@ -0,0 +1,5 @@ +FactoryBot.define do + factory :project_settings do + project + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9657e5011b18..e0f442a334cb 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -115,6 +115,26 @@ expect(subject.boards.size).to eq 1 end end + + describe '#settings' do + it 'creates lazily a settings record when the project does not have one associated' do + project = create :project + expect(ProjectSettings.count).to eq 0 + + expect(project.settings).to be_a ProjectSettings + + expect(ProjectSettings.count).to eq 1 + end + + it 'returns the associated record when the project has one associated' do + project = create :project, settings: create(:project_settings) + expect(ProjectSettings.count).to eq 1 + + expect(project.settings).to be_a ProjectSettings + + expect(ProjectSettings.count).to eq 1 + end + end end describe 'modules' do -- GitLab From dd785467393610a73da6e9fd8413bca685d9356c Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 8 Nov 2017 13:08:13 +0100 Subject: [PATCH 30/86] project#group_runner_enabled -> project_settings --- app/controllers/projects/runners_controller.rb | 2 +- app/models/project.rb | 5 +++++ ...2221_add_group_runners_enabled_to_projects.rb | 16 ---------------- .../20171030155459_create_project_settings.rb | 2 ++ db/schema.rb | 3 ++- spec/factories/projects.rb | 9 ++++++++- spec/models/project_spec.rb | 12 ++++++++++++ 7 files changed, 30 insertions(+), 19 deletions(-) delete mode 100644 db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 87c19eeed3e8..992e42d93485 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -53,7 +53,7 @@ def toggle_shared_runners end def toggle_group_runners - project.toggle!(:group_runners_enabled) + project.toggle_settings!(:group_runners_enabled) redirect_to project_settings_ci_cd_path(@project) end diff --git a/app/models/project.rb b/app/models/project.rb index dadd07558485..3daedc2b47d6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -249,6 +249,7 @@ class Project < ActiveRecord::Base delegate :members, to: :team, prefix: true delegate :add_user, :add_users, to: :team delegate :add_guest, :add_reporter, :add_developer, :add_master, :add_role, to: :team + delegate :group_runners_enabled, :group_runners_enabled=, :group_runners_enabled?, to: :settings # Validations validates :creator, presence: true, on: :create @@ -1893,6 +1894,10 @@ def licensed_features [] end + def toggle_settings!(settings_attribute) + settings.toggle!(settings_attribute) + end + private def storage diff --git a/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb b/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb deleted file mode 100644 index 8df7be39ee12..000000000000 --- a/db/migrate/20170925142221_add_group_runners_enabled_to_projects.rb +++ /dev/null @@ -1,16 +0,0 @@ -class AddGroupRunnersEnabledToProjects < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def up - add_column_with_default :projects, :group_runners_enabled, :boolean, default: true - add_concurrent_index :projects, :group_runners_enabled - end - - def down - remove_column :projects, :group_runners_enabled - end -end diff --git a/db/migrate/20171030155459_create_project_settings.rb b/db/migrate/20171030155459_create_project_settings.rb index eeb449505b8b..ebbe4c64fbd9 100644 --- a/db/migrate/20171030155459_create_project_settings.rb +++ b/db/migrate/20171030155459_create_project_settings.rb @@ -7,6 +7,8 @@ class CreateProjectSettings < ActiveRecord::Migration def change create_table :project_settings do |t| t.references :project, index: true, foreign_key: { on_delete: :cascade } + + t.boolean :group_runners_enabled, default: true, index: true end end end diff --git a/db/schema.rb b/db/schema.rb index b199342dc15b..9bb5c9d3c978 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1506,8 +1506,10 @@ create_table "project_settings", force: :cascade do |t| t.integer "project_id" + t.boolean "group_runners_enabled", default: true end + add_index "project_settings", ["group_runners_enabled"], name: "index_project_settings_on_group_runners_enabled", using: :btree add_index "project_settings", ["project_id"], name: "index_project_settings_on_project_id", using: :btree create_table "project_statistics", force: :cascade do |t| @@ -1581,7 +1583,6 @@ add_index "projects", ["created_at"], name: "index_projects_on_created_at", using: :btree add_index "projects", ["creator_id"], name: "index_projects_on_creator_id", using: :btree add_index "projects", ["description"], name: "index_projects_on_description_trigram", using: :gin, opclasses: {"description"=>"gin_trgm_ops"} - add_index "projects", ["group_runners_enabled"], name: "index_projects_on_group_runners_enabled", using: :btree add_index "projects", ["id"], name: "index_projects_on_id_partial_for_visibility", unique: true, where: "(visibility_level = ANY (ARRAY[10, 20]))", using: :btree add_index "projects", ["last_activity_at"], name: "index_projects_on_last_activity_at", using: :btree add_index "projects", ["last_repository_check_failed"], name: "index_projects_on_last_repository_check_failed", using: :btree diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 1761b6e2a3bf..ad33d09f78ab 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -15,14 +15,18 @@ namespace creator { group ? create(:user) : namespace&.owner } - # Nest Project Feature attributes transient do + # Nest Project Feature attributes wiki_access_level ProjectFeature::ENABLED builds_access_level ProjectFeature::ENABLED snippets_access_level ProjectFeature::ENABLED issues_access_level ProjectFeature::ENABLED merge_requests_access_level ProjectFeature::ENABLED repository_access_level ProjectFeature::ENABLED + + # we can't assign the delegated `#settings` attributes directly, as the + # `#settings` relation needs to be created first + group_runners_enabled nil end after(:create) do |project, evaluator| @@ -47,6 +51,9 @@ end project.group&.refresh_members_authorized_projects + + # assign the delegated `#settings` attributes after create + project.reload.group_runners_enabled = evaluator.group_runners_enabled unless evaluator.group_runners_enabled.nil? end trait :public do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index e0f442a334cb..ed28cd50e4d1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3640,4 +3640,16 @@ def domain_variable it { is_expected.not_to be_valid } end end + + describe '#toggle_settings!' do + it 'toggles the value on #settings' do + project = create :project, group_runners_enabled: false + + expect(project.group_runners_enabled).to be false + + project.toggle_settings!(:group_runners_enabled) + + expect(project.group_runners_enabled).to be true + end + end end -- GitLab From d4bda7c1a5b4c11526cc0b4f62bd4e83eebfb01f Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 9 Nov 2017 15:19:25 +0100 Subject: [PATCH 31/86] use union for Project#any_runners? --- app/models/project.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 3daedc2b47d6..220fd17fbc29 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1324,9 +1324,9 @@ def active_group_runners end def any_runners?(&block) - active_runners.any?(&block) || - active_shared_runners.any?(&block) || - active_group_runners.any?(&block) + union = Gitlab::SQL::Union.new([active_runners, active_shared_runners, active_group_runners]) + runners = Ci::Runner.from("(#{union.to_sql}) ci_runners") + runners.any?(&block) end def valid_runners_token?(token) -- GitLab From 316ccb64a738d376de57b9df76dbec732f0d9eff Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 9 Nov 2017 15:30:08 +0100 Subject: [PATCH 32/86] add `active` scope only once, inline methods --- app/models/project.rb | 14 ++------------ spec/models/project_spec.rb | 1 - 2 files changed, 2 insertions(+), 13 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 220fd17fbc29..c5e61193c041 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -225,8 +225,6 @@ class Project < ActiveRecord::Base has_many :project_deploy_tokens has_many :deploy_tokens, through: :project_deploy_tokens - has_many :active_runners, -> { active }, through: :runner_projects, source: :runner, class_name: 'Ci::Runner' - has_one :auto_devops, class_name: 'ProjectAutoDevops' has_many :custom_attributes, class_name: 'ProjectCustomAttribute' @@ -1311,21 +1309,13 @@ def shared_runners @shared_runners ||= shared_runners_enabled? ? Ci::Runner.shared : Ci::Runner.none end - def active_shared_runners - @active_shared_runners ||= shared_runners.active - end - def group_runners @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_group(self.id) : Ci::Runner.none end - def active_group_runners - @active_group_runners ||= group_runners.active - end - def any_runners?(&block) - union = Gitlab::SQL::Union.new([active_runners, active_shared_runners, active_group_runners]) - runners = Ci::Runner.from("(#{union.to_sql}) ci_runners") + union = Gitlab::SQL::Union.new([runners, shared_runners, group_runners]) + runners = Ci::Runner.from("(#{union.to_sql}) ci_runners").active runners.any?(&block) end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index ed28cd50e4d1..9c78f7b8bd68 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -63,7 +63,6 @@ it { is_expected.to have_many(:build_trace_section_names)} it { is_expected.to have_many(:runner_projects) } it { is_expected.to have_many(:runners) } - it { is_expected.to have_many(:active_runners) } it { is_expected.to have_many(:variables) } it { is_expected.to have_many(:triggers) } it { is_expected.to have_many(:pages_domains) } -- GitLab From 1acd8eb740dd070a5290d8a36c03e1b6f9691dba Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Thu, 7 Dec 2017 17:21:16 +0100 Subject: [PATCH 33/86] ci runners: assigned to either projects or group --- app/models/ci/runner.rb | 11 +++++++ spec/models/ci/runner_spec.rb | 57 +++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index b220ece3092d..3a3f41cdf355 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -50,6 +50,7 @@ class Runner < ActiveRecord::Base end validate :tag_constraints + validate :either_projects_or_group validates :access_level, presence: true acts_as_taggable @@ -227,6 +228,16 @@ def assignable_for?(project_id) self.class.owned_or_shared(project_id).where(id: self.id).any? end + def either_projects_or_group + if groups.length > 1 + errors.add(:runner, 'can only be assigned to one group') + end + + if groups.length > 0 && projects.length > 0 + errors.add(:runner, 'can only be assigned either to projects or to a group') + end + end + def accepting_tags?(build) (run_untagged? || build.has_tags?) && (build.tag_list - tag_list).empty? end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ba0db43a1e74..fb724f682a50 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -19,6 +19,63 @@ end end end + + context 'either_projects_or_group' do + it 'disallows assigning to a group if already assigned to a group' do + group = create(:group) + runner = create(:ci_runner, groups: [group]) + + runner.groups << build(:group) + + expect(runner).not_to be_valid + expect(runner.errors.full_messages).to eq ['Runner can only be assigned to one group'] + end + + it 'disallows assigning to a group if already assigned to a project' do + project = create(:project) + runner = create(:ci_runner, projects: [project]) + + runner.groups << build(:group) + + expect(runner).not_to be_valid + expect(runner.errors.full_messages).to eq ['Runner can only be assigned either to projects or to a group'] + end + + it 'disallows assigning to a project if already assigned to a group' do + group = create(:group) + runner = create(:ci_runner, groups: [group]) + + runner.projects << build(:project) + + expect(runner).not_to be_valid + expect(runner.errors.full_messages).to eq ['Runner can only be assigned either to projects or to a group'] + end + + it 'allows assigning to a group if not assigned to a group nor a project' do + runner = create(:ci_runner) + + runner.groups << build(:group) + + expect(runner).to be_valid + end + + it 'allows assigning to a project if not assigned to a group nor a project' do + runner = create(:ci_runner) + + runner.projects << build(:project) + + expect(runner).to be_valid + end + + it 'allows assigning to a project if already assigned to a project' do + project = create(:project) + runner = create(:ci_runner, projects: [project]) + + runner.projects << build(:project) + + expect(runner).to be_valid + end + end end describe '#access_level' do -- GitLab From 9bed8de9100a394257a4a55e8b87bcfd015f0fbd Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 19 Dec 2017 15:12:21 +0100 Subject: [PATCH 34/86] simplify runner selection don't differentiate between the different runner types, instead we rely on the Runner model to provide the available projects. scheduling is now applied to all runners equally. --- app/models/ci/runner.rb | 18 +++++ app/services/ci/register_job_service.rb | 70 ++++--------------- spec/models/ci/runner_spec.rb | 67 ++++++++++++++++++ spec/services/ci/register_job_service_spec.rb | 41 ++++++++++- 4 files changed, 137 insertions(+), 59 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 3a3f41cdf355..9139e5c830b6 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -94,6 +94,24 @@ def set_default_values 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 000ae3539e3f..64549ea3ce22 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -14,14 +14,7 @@ def initialize(runner) 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 @@ def execute private - def builds_for_shared_runner - builds_for_scheduled_runner( - running_builds_for_shared_runners, - projects: { shared_runners_enabled: true } - ) - end - - def builds_for_group_runner - builds_for_scheduled_runner( - running_builds_for_group_runners, - projects: { group_runners_enabled: true } - ) - 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') + 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 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)) + # New builds from the accessible projects + def new_builds + filter_builds(Ci::Build.pending.unstarted) 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 fb724f682a50..512a490d2892 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -776,4 +776,71 @@ def expect_redis_update 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 138ef673527a..0c3434253920 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 -- GitLab From c585004b59e5fbd5e925dacb7259916240d1cf5a Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 19 Dec 2017 16:40:19 +0100 Subject: [PATCH 35/86] restrict projects ci controller to project runners --- .../projects/settings/ci_cd_controller.rb | 9 +++++++-- app/models/ci/runner.rb | 7 ++++++- .../projects/settings/ci_cd_controller_spec.rb | 11 +++++++++++ spec/models/ci/runner_spec.rb | 15 +++++++++++++++ 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 05d545e97a70..72a9b74767ff 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -68,8 +68,13 @@ def define_variables def define_runners_variables @project_runners = @project.runners.ordered - @assignable_runners = current_user.ci_authorized_runners - .assignable_for(project).ordered.page(params[:page]).per(20) + @assignable_runners = current_user + .ci_authorized_runners + .assignable_for(project) + .ordered + .belonging_to_any_project + .page(params[:page]).per(20) + @shared_runners = ::Ci::Runner.shared.active @shared_runners_count = @shared_runners.count(:all) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 9139e5c830b6..428711630173 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -27,9 +27,13 @@ class Runner < ActiveRecord::Base scope :paused, -> { where(active: false) } scope :online, -> { where('contacted_at > ?', contact_time_deadline) } scope :ordered, -> { order(id: :desc) } + scope :belonging_to_project, -> (project_id) { joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) } + + scope :belonging_to_any_project, -> { joins(:runner_projects) } + scope :belonging_to_group, -> (project_id) { project_groups = ::Group.joins(:projects).where(projects: { id: project_id }) hierarchy_groups = Gitlab::GroupHierarchy.new(project_groups).base_and_ancestors @@ -46,7 +50,8 @@ class Runner < ActiveRecord::Base # FIXME: That `to_sql` is needed to workaround a weird Rails bug. # Without that, placeholders would miss one and couldn't match. where(locked: false) - .where.not("id IN (#{project.runners.select(:id).to_sql})").specific + .where.not("ci_runners.id IN (#{project.runners.select(:id).to_sql})") + .specific end validate :tag_constraints diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index 7dae9b85d784..1cf395b03284 100644 --- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb @@ -17,6 +17,17 @@ expect(response).to have_gitlab_http_status(200) expect(response).to render_template(:show) end + + it 'sets assignable project runners' do + group = create(:group, runners: [create(:ci_runner)], parent: create(:group)) + group.add_master(user) + project_runner = create(:ci_runner, projects: [create(:project, group: group)]) + create(:ci_runner, :shared) + + get :show, namespace_id: project.namespace, project_id: project + + expect(assigns(:assignable_runners)).to eq [project_runner] + end end describe '#reset_cache' do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 512a490d2892..3e85e3c92e30 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -136,6 +136,21 @@ end end + describe '.belonging_to_any_project' do + it 'returns the specific project runner' do + # project + project_project = create :project + project_runner = create :ci_runner, :specific, projects: [project_project] + + # group + group = create :group + create :project, group: group + create :ci_runner, :specific, groups: [group] + + expect(described_class.belonging_to_any_project).to eq [project_runner] + end + end + describe '.belonging_to_group' do it 'returns the specific group runner' do # own -- GitLab From bfc694f5117aa12a2224b567a74f196c320a6d75 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Tue, 19 Dec 2017 18:04:33 +0100 Subject: [PATCH 36/86] show group runners setup only to group master --- .../projects/runners/_group_runners.html.haml | 7 +- spec/features/runners_spec.rb | 82 ++++++++++++------- 2 files changed, 59 insertions(+), 30 deletions(-) diff --git a/app/views/projects/runners/_group_runners.html.haml b/app/views/projects/runners/_group_runners.html.haml index 785abba945bd..a9dfd9cc786b 100644 --- a/app/views/projects/runners/_group_runners.html.haml +++ b/app/views/projects/runners/_group_runners.html.haml @@ -20,8 +20,11 @@ - elsif @group_runners.empty? This group does not provide any group Runners yet. - = render partial: 'ci/runner/how_to_setup_runner', - locals: { registration_token: @project.group.runners_token, type: 'group' } + - if can?(current_user, :admin_pipeline, @project.group) + = render partial: 'ci/runner/how_to_setup_runner', + locals: { registration_token: @project.group.runners_token, type: 'group' } + - else + Ask your group master to setup a group Runner. - else %h4.underlined-title Available group Runners : #{@group_runners.count} diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index dcd06a4015d7..b396e1033458 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -187,51 +187,77 @@ project.add_master(user) end - context 'project without a group' do - given(:project) { create :project } + given(:group) { create :group } - scenario 'group runners are not available' do - visit runners_path(project) + context 'as project and group master' do + background do + group.add_master(user) + end + + context 'project with a group but no group runner' do + given(:project) { create :project, group: group } + + scenario 'group runners are not available' do + visit runners_path(project) - expect(page).to have_content 'This project does not belong to a group and can therefore not make use of group Runners.' + expect(page).to have_content 'This group does not provide any group Runners yet.' + + expect(page).to have_content 'Setup a group Runner manually' + expect(page).not_to have_content 'Ask your group master to setup a group Runner.' + end end end - context 'project with a group but no group runner' do - given(:group) { create :group } - given(:project) { create :project, group: group } + context 'as project master' do + context 'project without a group' do + given(:project) { create :project } - scenario 'group runners are not available' do - visit runners_path(project) + scenario 'group runners are not available' do + visit runners_path(project) - expect(page).to have_content 'This group does not provide any group Runners yet.' + expect(page).to have_content 'This project does not belong to a group and can therefore not make use of group Runners.' + end end - end - context 'project with a group and a group runner' do - given(:group) { create :group } - given(:project) { create :project, group: group } - given!(:ci_runner) { create :ci_runner, groups: [group], description: 'group-runner' } + context 'project with a group but no group runner' do + given(:group) { create :group } + given(:project) { create :project, group: group } - scenario 'group runners are available' do - visit runners_path(project) + scenario 'group runners are not available' do + visit runners_path(project) + + expect(page).to have_content 'This group does not provide any group Runners yet.' - expect(page).to have_content 'Available group Runners : 1' - expect(page).to have_content 'group-runner' + expect(page).not_to have_content 'Setup a group Runner manually' + expect(page).to have_content 'Ask your group master to setup a group Runner.' + end end - scenario 'group runners may be disabled for a project' do - visit runners_path(project) + context 'project with a group and a group runner' do + given(:group) { create :group } + given(:project) { create :project, group: group } + given!(:ci_runner) { create :ci_runner, groups: [group], description: 'group-runner' } + + scenario 'group runners are available' do + visit runners_path(project) + + expect(page).to have_content 'Available group Runners : 1' + expect(page).to have_content 'group-runner' + end - click_on 'Disable group Runners' + scenario 'group runners may be disabled for a project' do + visit runners_path(project) + + click_on 'Disable group Runners' - expect(page).to have_content 'Enable group Runners' - expect(project.reload.group_runners_enabled).to be false + expect(page).to have_content 'Enable group Runners' + expect(project.reload.group_runners_enabled).to be false - click_on 'Enable group Runners' + click_on 'Enable group Runners' - expect(page).to have_content 'Disable group Runners' - expect(project.reload.group_runners_enabled).to be true + expect(page).to have_content 'Disable group Runners' + expect(project.reload.group_runners_enabled).to be true + end end end end -- GitLab From 1a009f1bdb4d75dd511df5f15705fcf8ee9d20dd Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 20 Dec 2017 09:38:58 +0100 Subject: [PATCH 37/86] update MODELS_JSON with new Project#settings attr --- spec/lib/gitlab/import_export/all_models.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 897a5984782e..ad599c8c38ab 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -258,7 +258,6 @@ project: - builds - runner_projects - runners -- active_runners - variables - triggers - pipeline_schedules @@ -286,6 +285,7 @@ project: - internal_ids - project_deploy_tokens - deploy_tokens +- settings award_emoji: - awardable - user -- GitLab From cc4bc22ae49582652413d45aa2f5b39dd2a15a82 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Wed, 20 Dec 2017 10:08:54 +0100 Subject: [PATCH 38/86] runner can't be assigned to more than 1 group therefore we don't need the api check. --- lib/api/runners.rb | 1 - spec/requests/api/runners_spec.rb | 9 +-------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 84d33879c389..1a05bed3465e 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -199,7 +199,6 @@ def authenticate_delete_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner associated with more than one project") if runner.projects.count > 1 - forbidden!("Runner associated with more that one group") if runner.groups.count > 1 forbidden!("No access granted") unless user_can_access_runner?(runner) end diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index ab807e399a4d..89e21ba9914a 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -29,8 +29,6 @@ let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } - let!(:two_groups_runner) { create(:ci_runner, description: 'Two groups runner', groups: [group, group2]) } - before do # Set project access for users create(:project_member, :master, user: user, project: project) @@ -49,7 +47,7 @@ expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(descriptions).to contain_exactly( - 'Project runner', 'Group runner', 'Two projects runner', 'Two groups runner' + 'Project runner', 'Group runner', 'Two projects runner' ) expect(shared).to be_falsey end @@ -422,11 +420,6 @@ def update_runner(id, user, args) end.to change { Ci::Runner.specific.count }.by(-1) end - it 'does not delete group runner with more than one associated group' do - delete api("/runners/#{two_groups_runner.id}", user) - expect(response).to have_http_status(403) - end - it 'deletes group runner for one owned group' do expect do delete api("/runners/#{group_runner.id}", user) -- GitLab From e13026a378f186c3ef2b08720fa8420ead972f0f Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 26 Feb 2018 15:10:48 +0100 Subject: [PATCH 39/86] use more efficient AR length check methods --- app/models/ci/runner.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 428711630173..afd892658dc1 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -252,11 +252,11 @@ def assignable_for?(project_id) end def either_projects_or_group - if groups.length > 1 + if groups.many? errors.add(:runner, 'can only be assigned to one group') end - if groups.length > 0 && projects.length > 0 + if group? && project? errors.add(:runner, 'can only be assigned either to projects or to a group') end end -- GitLab From 9447e5c27d8f840eaf4eee9635a5149ab36d93b6 Mon Sep 17 00:00:00 2001 From: Alexis Reigel Date: Mon, 26 Feb 2018 16:20:29 +0100 Subject: [PATCH 40/86] extract method to adhere to "tell, don't ask" --- app/models/ci/runner.rb | 6 +++++ app/services/ci/update_build_queue_service.rb | 2 +- spec/models/ci/runner_spec.rb | 26 +++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index afd892658dc1..d65395380d66 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -217,6 +217,12 @@ def update_cached_info(values) end end + def invalidate_build_cache!(build) + if can_pick?(build) + tick_runner_queue + end + end + private def cleanup_runner_queue diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index ab81766abf39..05dbebc4f8d8 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -16,7 +16,7 @@ def execute(build) def tick_for(build, runners) runners.each do |runner| - runner.tick_runner_queue if runner.can_pick?(build) + runner.invalidate_build_cache!(build) end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 3e85e3c92e30..23e18efea704 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -858,4 +858,30 @@ def expect_redis_update end end end + + describe '#invalidate_build_cache!' do + context 'runner can pick the build' do + it 'calls #tick_runner_queue' do + ci_build = build :ci_build + runner = build :ci_runner + allow(runner).to receive(:can_pick?).with(ci_build).and_return(true) + + expect(runner).to receive(:tick_runner_queue) + + runner.invalidate_build_cache!(ci_build) + end + end + + context 'runner cannot pick the build' do + it 'does not call #tick_runner_queue' do + ci_build = build :ci_build + runner = build :ci_runner + allow(runner).to receive(:can_pick?).with(ci_build).and_return(false) + + expect(runner).not_to receive(:tick_runner_queue) + + runner.invalidate_build_cache!(ci_build) + end + end + end end -- GitLab From 92cbf9453a4435976bfe77cafd0f8c5f57833e59 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 26 Apr 2018 10:39:04 +0800 Subject: [PATCH 41/86] Switch to using ProjectCiCdSetting for group_runners_enabled and remove ProjectSettings --- .../projects/runners_controller.rb | 2 +- app/models/project.rb | 14 +++-------- app/models/project_settings.rb | 3 --- .../20171030155459_create_project_settings.rb | 14 ----------- db/schema.rb | 9 ------- spec/models/project_spec.rb | 24 ++----------------- 6 files changed, 6 insertions(+), 60 deletions(-) delete mode 100644 app/models/project_settings.rb delete mode 100644 db/migrate/20171030155459_create_project_settings.rb diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index 992e42d93485..b9bbe7115c4d 100644 --- a/app/controllers/projects/runners_controller.rb +++ b/app/controllers/projects/runners_controller.rb @@ -53,7 +53,7 @@ def toggle_shared_runners end def toggle_group_runners - project.toggle_settings!(:group_runners_enabled) + project.toggle_ci_cd_settings!(:group_runners_enabled) redirect_to project_settings_ci_cd_path(@project) end diff --git a/app/models/project.rb b/app/models/project.rb index dddc7fb2b273..19024b4ea85f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -236,14 +236,6 @@ class Project < ActiveRecord::Base has_many :project_badges, class_name: 'ProjectBadge' has_one :ci_cd_settings, class_name: 'ProjectCiCdSetting' - has_one :settings, -> (project) { - query = where(project_id: project) - query.presence || begin - ProjectSettings.create(project_id: project.id) - query - end - }, class_name: 'ProjectSettings' - accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true accepts_nested_attributes_for :import_data @@ -253,7 +245,7 @@ class Project < ActiveRecord::Base delegate :members, to: :team, prefix: true delegate :add_user, :add_users, to: :team delegate :add_guest, :add_reporter, :add_developer, :add_master, :add_role, to: :team - delegate :group_runners_enabled, :group_runners_enabled=, :group_runners_enabled?, to: :settings + delegate :group_runners_enabled, :group_runners_enabled=, :group_runners_enabled?, to: :ci_cd_settings # Validations validates :creator, presence: true, on: :create @@ -1879,8 +1871,8 @@ def licensed_features [] end - def toggle_settings!(settings_attribute) - settings.toggle!(settings_attribute) + def toggle_ci_cd_settings!(settings_attribute) + ci_cd_settings.toggle!(settings_attribute) end def gitlab_deploy_token diff --git a/app/models/project_settings.rb b/app/models/project_settings.rb deleted file mode 100644 index b126f66fafac..000000000000 --- a/app/models/project_settings.rb +++ /dev/null @@ -1,3 +0,0 @@ -class ProjectSettings < ActiveRecord::Base - belongs_to :project -end diff --git a/db/migrate/20171030155459_create_project_settings.rb b/db/migrate/20171030155459_create_project_settings.rb deleted file mode 100644 index ebbe4c64fbd9..000000000000 --- a/db/migrate/20171030155459_create_project_settings.rb +++ /dev/null @@ -1,14 +0,0 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. - -class CreateProjectSettings < ActiveRecord::Migration - DOWNTIME = false - - def change - create_table :project_settings do |t| - t.references :project, index: true, foreign_key: { on_delete: :cascade } - - t.boolean :group_runners_enabled, default: true, index: true - end - end -end diff --git a/db/schema.rb b/db/schema.rb index 7eebe94a8a60..233423aaa902 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1511,14 +1511,6 @@ add_index "project_import_data", ["project_id"], name: "index_project_import_data_on_project_id", using: :btree - create_table "project_settings", force: :cascade do |t| - t.integer "project_id" - t.boolean "group_runners_enabled", default: true - end - - add_index "project_settings", ["group_runners_enabled"], name: "index_project_settings_on_group_runners_enabled", using: :btree - add_index "project_settings", ["project_id"], name: "index_project_settings_on_project_id", using: :btree - create_table "project_statistics", force: :cascade do |t| t.integer "project_id", null: false t.integer "namespace_id", null: false @@ -2200,7 +2192,6 @@ add_foreign_key "project_features", "projects", name: "fk_18513d9b92", on_delete: :cascade add_foreign_key "project_group_links", "projects", name: "fk_daa8cee94c", on_delete: :cascade add_foreign_key "project_import_data", "projects", name: "fk_ffb9ee3a10", on_delete: :cascade - add_foreign_key "project_settings", "projects", on_delete: :cascade add_foreign_key "project_statistics", "projects", on_delete: :cascade add_foreign_key "protected_branch_merge_access_levels", "protected_branches", name: "fk_8a3072ccb3", on_delete: :cascade add_foreign_key "protected_branch_push_access_levels", "protected_branches", name: "fk_9ffc86a3d9", on_delete: :cascade diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 265941acbe70..7392b75e37c7 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -123,26 +123,6 @@ expect(subject.boards.size).to eq 1 end end - - describe '#settings' do - it 'creates lazily a settings record when the project does not have one associated' do - project = create :project - expect(ProjectSettings.count).to eq 0 - - expect(project.settings).to be_a ProjectSettings - - expect(ProjectSettings.count).to eq 1 - end - - it 'returns the associated record when the project has one associated' do - project = create :project, settings: create(:project_settings) - expect(ProjectSettings.count).to eq 1 - - expect(project.settings).to be_a ProjectSettings - - expect(ProjectSettings.count).to eq 1 - end - end end describe 'modules' do @@ -3595,13 +3575,13 @@ def domain_variable end end - describe '#toggle_settings!' do + describe '#toggle_ci_cd_settings!' do it 'toggles the value on #settings' do project = create :project, group_runners_enabled: false expect(project.group_runners_enabled).to be false - project.toggle_settings!(:group_runners_enabled) + project.toggle_ci_cd_settings!(:group_runners_enabled) expect(project.group_runners_enabled).to be true end -- GitLab From 77c72c688d70104c776bbb76d3490ec53e10a56d Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 26 Apr 2018 13:23:35 +0800 Subject: [PATCH 42/86] Increase PipelineSerializer query limit count to support new group runner queries --- spec/serializers/pipeline_serializer_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index f51c11b141fc..e88e86c29980 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -118,7 +118,7 @@ it 'verifies number of queries', :request_store do recorded = ActiveRecord::QueryRecorder.new { subject } - expect(recorded.count).to be_within(1).of(36) + expect(recorded.count).to be_within(1).of(44) expect(recorded.cached_count).to eq(0) end end -- GitLab From a39e994064e76e9dcf86067ef5f4173de31ed731 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 26 Apr 2018 13:32:13 +0800 Subject: [PATCH 43/86] Exclude group_runners_enabled in project import/export --- lib/gitlab/import_export/import_export.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 0d1c4f73c6e4..f1978d068848 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -107,6 +107,7 @@ excluded_attributes: - :last_repository_check_at - :storage_version - :description_html + - :group_runners_enabled snippets: - :expired_at merge_request_diff: -- GitLab From 85e04cde4155cc4d998764a031fe5cea454c2a84 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 26 Apr 2018 13:36:14 +0800 Subject: [PATCH 44/86] Remove spec/factories/project_settings.rb since model no longer exists --- spec/factories/project_settings.rb | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 spec/factories/project_settings.rb diff --git a/spec/factories/project_settings.rb b/spec/factories/project_settings.rb deleted file mode 100644 index 90240ee6de09..000000000000 --- a/spec/factories/project_settings.rb +++ /dev/null @@ -1,5 +0,0 @@ -FactoryBot.define do - factory :project_settings do - project - end -end -- GitLab From 0f9f5e82ad5c4898e7c206ff0d574a3e141f3433 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 26 Apr 2018 14:43:17 +0800 Subject: [PATCH 45/86] Fix spec/models/user_spec.rb for #ci_authorized_runners --- spec/models/user_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 79447a65e942..df2e547ce285 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1897,7 +1897,7 @@ def add_user(access) let(:group) { create(:group) } let(:another_user) { create(:user) } let(:subgroup) { create(:group, parent: group) } - let(:project) { create(:project, group: subgroup) } + let!(:project) { create(:project, group: subgroup, runners: [runner_1]) } def add_user(access) group.add_user(user, access) -- GitLab From ff219759303dc03f66bb1a160aafa2fca7ac9308 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 08:05:46 +0800 Subject: [PATCH 46/86] Tag runner_spec tests for subgroups with nested_groups --- spec/models/ci/runner_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 23e18efea704..1df11fdedf44 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -166,7 +166,7 @@ expect(described_class.belonging_to_group(specific_project.id)).to eq [specific_runner] end - it 'returns the group runner from a parent group' do + it 'returns the group runner from a parent group', :nested_groups do parent_group = create :group group = create :group, parent: parent_group project = create :project, group: group @@ -824,7 +824,7 @@ def expect_redis_update expect(project_runner.reload.accessible_projects).to eq [project_project] end - it 'returns the projects with a group and nested group runner' do + 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 -- GitLab From 329d535bbcf8ceafd3b000c5cd224173f441310b Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 08:56:13 +0800 Subject: [PATCH 47/86] Add extra spec for Project#any_runners? to test block properly --- spec/models/project_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 7392b75e37c7..ab0694e6890d 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1165,6 +1165,11 @@ def create_pipeline project.runners << specific_runner expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy end + + it 'returns false if match cannot be found' do + project.runners << specific_runner + expect(project.any_runners? { false }).to be_falsey + end end context 'for shared runners enabled' do @@ -1179,6 +1184,11 @@ def create_pipeline shared_runner expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy end + + it 'returns false if match cannot be found' do + shared_runner + expect(project.any_runners? { false }).to be_falsey + end end end @@ -1212,6 +1222,11 @@ def create_pipeline group_runner expect(project.any_runners? { |runner| runner == group_runner }).to be_truthy end + + it 'returns false if match cannot be found' do + group_runner + expect(project.any_runners? { false }).to be_falsey + end end end end -- GitLab From 3f00f19d37aec52ea6c06c93db240e72ba08a460 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 09:01:58 +0800 Subject: [PATCH 48/86] Remove timestamps from ci_runner_groups table --- db/migrate/20170301101006_add_ci_runner_groups.rb | 2 -- db/schema.rb | 2 -- 2 files changed, 4 deletions(-) diff --git a/db/migrate/20170301101006_add_ci_runner_groups.rb b/db/migrate/20170301101006_add_ci_runner_groups.rb index 73a135b0ee1b..3954f6ffb871 100644 --- a/db/migrate/20170301101006_add_ci_runner_groups.rb +++ b/db/migrate/20170301101006_add_ci_runner_groups.rb @@ -9,8 +9,6 @@ def up create_table :ci_runner_groups do |t| t.integer :runner_id t.integer :group_id - - t.timestamps_with_timezone null: false end add_concurrent_index :ci_runner_groups, :runner_id diff --git a/db/schema.rb b/db/schema.rb index 233423aaa902..b1cc7fd56d2b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -446,8 +446,6 @@ create_table "ci_runner_groups", force: :cascade do |t| t.integer "runner_id" t.integer "group_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false end add_index "ci_runner_groups", ["group_id"], name: "index_ci_runner_groups_on_group_id", using: :btree -- GitLab From 3c3e14430ba14ae45ec758d263ba882ddf32f76f Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 09:04:24 +0800 Subject: [PATCH 49/86] Remove unecessary index on ci_runner_groups.runner_id --- db/migrate/20170301101006_add_ci_runner_groups.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/db/migrate/20170301101006_add_ci_runner_groups.rb b/db/migrate/20170301101006_add_ci_runner_groups.rb index 3954f6ffb871..1c4430981a95 100644 --- a/db/migrate/20170301101006_add_ci_runner_groups.rb +++ b/db/migrate/20170301101006_add_ci_runner_groups.rb @@ -11,11 +11,9 @@ def up t.integer :group_id end - add_concurrent_index :ci_runner_groups, :runner_id - add_concurrent_foreign_key :ci_runner_groups, :ci_runners, column: :runner_id, on_delete: :cascade - add_concurrent_index :ci_runner_groups, :group_id add_concurrent_index :ci_runner_groups, [:runner_id, :group_id], unique: true + add_concurrent_foreign_key :ci_runner_groups, :ci_runners, column: :runner_id, on_delete: :cascade add_concurrent_foreign_key :ci_runner_groups, :namespaces, column: :group_id, on_delete: :cascade end -- GitLab From 8d8139862aee97d7fadc0563e7df9842f5bd46ac Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 09:15:54 +0800 Subject: [PATCH 50/86] Rename `runner.belonging_to_group(project.id) -> runner.belonging_to_parent_group_of_project(project.id)` --- app/controllers/projects/settings/ci_cd_controller.rb | 2 +- app/models/ci/runner.rb | 4 ++-- app/models/project.rb | 2 +- app/services/ci/update_build_queue_service.rb | 2 +- spec/models/ci/runner_spec.rb | 6 +++--- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 72a9b74767ff..6ddbefacae04 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -79,7 +79,7 @@ def define_runners_variables @shared_runners_count = @shared_runners.count(:all) - @group_runners = ::Ci::Runner.belonging_to_group(@project.id) + @group_runners = ::Ci::Runner.belonging_to_parent_group_of_project(@project.id) end def define_secret_variables diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d65395380d66..6904aca5e682 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -34,7 +34,7 @@ class Runner < ActiveRecord::Base scope :belonging_to_any_project, -> { joins(:runner_projects) } - scope :belonging_to_group, -> (project_id) { + scope :belonging_to_parent_group_of_project, -> (project_id) { project_groups = ::Group.joins(:projects).where(projects: { id: project_id }) hierarchy_groups = Gitlab::GroupHierarchy.new(project_groups).base_and_ancestors @@ -42,7 +42,7 @@ class Runner < ActiveRecord::Base } scope :owned_or_shared, -> (project_id) do - union = Gitlab::SQL::Union.new([belonging_to_project(project_id), belonging_to_group(project_id), shared]) + union = Gitlab::SQL::Union.new([belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), shared]) from("(#{union.to_sql}) ci_runners") end diff --git a/app/models/project.rb b/app/models/project.rb index 19024b4ea85f..8e2145a4aa34 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1297,7 +1297,7 @@ def shared_runners end def group_runners - @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_group(self.id) : Ci::Runner.none + @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_parent_group_of_project(self.id) : Ci::Runner.none end def any_runners?(&block) diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 05dbebc4f8d8..c991f04ab30a 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -4,7 +4,7 @@ def execute(build) tick_for(build, build.project.runners) if build.project.group_runners_enabled? - tick_for(build, Ci::Runner.belonging_to_group(build.project_id)) + tick_for(build, Ci::Runner.belonging_to_parent_group_of_project(build.project_id)) end if build.project.shared_runners_enabled? diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 1df11fdedf44..fbf9539e698e 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -151,7 +151,7 @@ end end - describe '.belonging_to_group' do + describe '.belonging_to_parent_group_of_project' do it 'returns the specific group runner' do # own specific_group = create :group @@ -163,7 +163,7 @@ create :project, group: other_group create :ci_runner, :specific, groups: [other_group] - expect(described_class.belonging_to_group(specific_project.id)).to eq [specific_runner] + expect(described_class.belonging_to_parent_group_of_project(specific_project.id)).to eq [specific_runner] end it 'returns the group runner from a parent group', :nested_groups do @@ -172,7 +172,7 @@ project = create :project, group: group runner = create :ci_runner, :specific, groups: [parent_group] - expect(described_class.belonging_to_group(project.id)).to eq [runner] + expect(described_class.belonging_to_parent_group_of_project(project.id)).to eq [runner] end end -- GitLab From 0077679ae88793b200bdcf69d2ca7e4d15e6f7fa Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 27 Apr 2018 10:05:51 +0800 Subject: [PATCH 51/86] Bring back shared_runners_available? method since it is overriden in EE --- app/models/project.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index 8e2145a4aa34..5c9bf8c61dd2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1292,8 +1292,12 @@ def enable_ci project_feature.update_attribute(:builds_access_level, ProjectFeature::ENABLED) end + def shared_runners_available? + shared_runners_enabled? + end + def shared_runners - @shared_runners ||= shared_runners_enabled? ? Ci::Runner.shared : Ci::Runner.none + @shared_runners ||= shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none end def group_runners -- GitLab From 87740df2ba7153439c30544f299b235632717738 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 09:39:47 +0400 Subject: [PATCH 52/86] 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. --- app/models/ci/runner.rb | 18 ----- app/services/ci/register_job_service.rb | 47 +++++++++---- spec/models/ci/runner_spec.rb | 67 ------------------- spec/services/ci/register_job_service_spec.rb | 51 +++----------- 4 files changed, 43 insertions(+), 140 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6904aca5e682..40d828b84142 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -99,24 +99,6 @@ def set_default_values 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 64549ea3ce22..55d0273847c8 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -14,7 +14,14 @@ def initialize(runner) 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 @@ def execute 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 fbf9539e698e..d6ce97a9b28f 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -792,73 +792,6 @@ def expect_redis_update 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 0c3434253920..7d3c43eeaf7a 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 -- GitLab From 8604dbe9f6768a8bb44bf1e1b144f7fd216f3641 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 10:25:26 +0400 Subject: [PATCH 53/86] Fix up db/schema.rb changes leftover and comments out of date --- db/schema.rb | 2 -- lib/gitlab/import_export/import_export.yml | 1 - spec/factories/projects.rb | 6 +++--- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index b1cc7fd56d2b..db1e41e00b35 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -450,7 +450,6 @@ add_index "ci_runner_groups", ["group_id"], name: "index_ci_runner_groups_on_group_id", using: :btree add_index "ci_runner_groups", ["runner_id", "group_id"], name: "index_ci_runner_groups_on_runner_id_and_group_id", unique: true, using: :btree - add_index "ci_runner_groups", ["runner_id"], name: "index_ci_runner_groups_on_runner_id", using: :btree create_table "ci_runner_projects", force: :cascade do |t| t.integer "runner_id", null: false @@ -1573,7 +1572,6 @@ t.boolean "merge_requests_rebase_enabled", default: false, null: false t.integer "jobs_cache_index" t.boolean "pages_https_only", default: true - t.boolean "group_runners_enabled", default: true, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index f1978d068848..0d1c4f73c6e4 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -107,7 +107,6 @@ excluded_attributes: - :last_repository_check_at - :storage_version - :description_html - - :group_runners_enabled snippets: - :expired_at merge_request_diff: diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index 3d2810bfdbaf..aed5eab80449 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -24,8 +24,8 @@ merge_requests_access_level ProjectFeature::ENABLED repository_access_level ProjectFeature::ENABLED - # we can't assign the delegated `#settings` attributes directly, as the - # `#settings` relation needs to be created first + # we can't assign the delegated `#ci_cd_settings` attributes directly, as the + # `#ci_cd_settings` relation needs to be created first group_runners_enabled nil end @@ -52,7 +52,7 @@ project.group&.refresh_members_authorized_projects - # assign the delegated `#settings` attributes after create + # assign the delegated `#ci_cd_settings` attributes after create project.reload.group_runners_enabled = evaluator.group_runners_enabled unless evaluator.group_runners_enabled.nil? end -- GitLab From 5652ff953cba9773edbcb677908fe3f18b103be3 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 10:43:29 +0400 Subject: [PATCH 54/86] Rename Runner#group? -> #assigned_to_group? and Runner#project? -> #assigned_to_project? --- app/models/ci/runner.rb | 6 +++--- app/services/ci/register_job_service.rb | 2 +- app/views/admin/runners/_runner.html.haml | 4 ++-- app/views/admin/runners/show.html.haml | 2 +- app/views/projects/runners/_runner.html.haml | 2 +- lib/api/runners.rb | 2 +- spec/models/ci/runner_spec.rb | 16 ++++++++-------- 7 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 40d828b84142..da1107951bfb 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -137,11 +137,11 @@ def specific? !shared? end - def group? + def assigned_to_group? runner_groups.any? end - def project? + def assigned_to_project? runner_projects.any? end @@ -244,7 +244,7 @@ def either_projects_or_group errors.add(:runner, 'can only be assigned to one group') end - if group? && project? + if assigned_to_group? && assigned_to_project? errors.add(:runner, 'can only be assigned either to projects or to a group') end end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 55d0273847c8..647bceb3b360 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -17,7 +17,7 @@ def execute builds = if runner.shared? builds_for_shared_runner - elsif runner.group? + elsif runner.assigned_to_group? builds_for_group_runner else builds_for_project_runner diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index f1e0e3b5ad62..6670ba6aa89f 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -2,7 +2,7 @@ %td - if runner.shared? %span.label.label-success shared - - elsif runner.group? + - elsif runner.assigned_to_group? %span.label.label-success group - else %span.label.label-info specific @@ -21,7 +21,7 @@ %td = runner.ip_address %td - - if runner.shared? || runner.group? + - if runner.shared? || runner.assigned_to_group? n/a - else = runner.projects.count(:all) diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index ae5f860d0d19..ab2c9ad1e570 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -19,7 +19,7 @@ %p If you want Runners to build only specific projects, enable them in the table below. Keep in mind that this is a one way transition. -- elsif @runner.group? +- elsif @runner.assigned_to_group? .bs-callout.bs-callout-success %h4 This runner will process jobs from all projects in its group and subgroups - else diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 6d61da40f5b6..d2598f3be072 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -26,7 +26,7 @@ - else - runner_project = @project.runner_projects.find_by(runner_id: runner) = link_to 'Disable for this project', project_runner_project_path(@project, runner_project), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - - elsif runner.project? + - elsif runner.assigned_to_project? = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f| = f.hidden_field :runner_id, value: runner.id = f.submit 'Enable for this project', class: 'btn btn-sm' diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 1a05bed3465e..11c31917fc5a 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -205,7 +205,7 @@ def authenticate_delete_runner!(runner) def authenticate_enable_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner is locked") if runner.locked? - forbidden!("Runner is a group runner") if runner.group? + forbidden!("Runner is a group runner") if runner.assigned_to_group? return if current_user.admin? forbidden!("No access granted") unless user_can_access_runner?(runner) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index d6ce97a9b28f..fb9dcce9a7c7 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -748,47 +748,47 @@ def expect_redis_update end end - describe 'group?' do + describe 'assigned_to_group?' do it 'returns false when the runner is a project runner' do project = create :project runner = create :ci_runner, description: 'Project runner', projects: [project] - expect(runner.group?).to be false + expect(runner.assigned_to_group?).to be false end it 'returns false when the runner is a shared runner' do runner = create :ci_runner, :shared, description: 'Shared runner' - expect(runner.group?).to be false + expect(runner.assigned_to_group?).to be false end it 'returns true when the runner is assigned to a group' do group = create :group runner = create :ci_runner, description: 'Group runner', groups: [group] - expect(runner.group?).to be true + expect(runner.assigned_to_group?).to be true end end - describe 'project?' do + describe 'assigned_to_project?' do it 'returns false when the runner is a group prunner' do group = create :group runner = create :ci_runner, description: 'Group runner', groups: [group] - expect(runner.project?).to be false + expect(runner.assigned_to_project?).to be false end it 'returns false when the runner is a shared runner' do runner = create :ci_runner, :shared, description: 'Shared runner' - expect(runner.project?).to be false + expect(runner.assigned_to_project?).to be false end it 'returns true when the runner is assigned to a project' do project = create :project runner = create :ci_runner, description: 'Group runner', projects: [project] - expect(runner.project?).to be true + expect(runner.assigned_to_project?).to be true end end -- GitLab From 3dbcc02db0c1fda22044a743158d4ba9e4eda637 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 11:28:33 +0400 Subject: [PATCH 55/86] Add changelog for group runners --- changelogs/unreleased/feature-runner-per-group.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-runner-per-group.yml diff --git a/changelogs/unreleased/feature-runner-per-group.yml b/changelogs/unreleased/feature-runner-per-group.yml new file mode 100644 index 000000000000..162a5fae0a44 --- /dev/null +++ b/changelogs/unreleased/feature-runner-per-group.yml @@ -0,0 +1,5 @@ +--- +title: Allow group masters to configure runners for groups +merge_request: 9646 +author: Alexis Reigel +type: added -- GitLab From 0d30b00de807df550bec947751c098317c5bb79f Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 17:00:28 +0400 Subject: [PATCH 56/86] Start persisting runner_type when creating runners --- app/models/ci/runner.rb | 6 ++++++ ...430101916_add_runner_type_to_ci_runners.rb | 9 +++++++++ ...runner_type_for_ci_runners_post_migrate.rb | 20 +++++++++++++++++++ db/schema.rb | 3 ++- lib/api/runner.rb | 6 +++--- 5 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20180430101916_add_runner_type_to_ci_runners.rb create mode 100644 db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index da1107951bfb..cdd284071728 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -67,6 +67,12 @@ class Runner < ActiveRecord::Base ref_protected: 1 } + enum runner_type: { + instance_type: 1, + group_type: 2, + project_type: 3 + } + cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout diff --git a/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb b/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb new file mode 100644 index 000000000000..8c8009f28fbf --- /dev/null +++ b/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb @@ -0,0 +1,9 @@ +class AddRunnerTypeToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_runners, :runner_type, :integer + end +end diff --git a/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb new file mode 100644 index 000000000000..8509222edc20 --- /dev/null +++ b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb @@ -0,0 +1,20 @@ +class BackfillRunnerTypeForCiRunnersPostMigrate < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + update_column_in_batches(:ci_runners, :runner_type, 1) do |table, query| + query.where(table[:is_shared].eq(true)).where(table[:runner_type].eq(nil)) + end + + update_column_in_batches(:ci_runners, :runner_type, 3) do |table, query| + query.where(table[:is_shared].eq(false)).where(table[:runner_type].eq(nil)) + end + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index db1e41e00b35..4a541b3ac819 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180425131009) do +ActiveRecord::Schema.define(version: 20180430143705) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -479,6 +479,7 @@ t.integer "access_level", default: 0, null: false t.string "ip_address" t.integer "maximum_timeout" + t.integer "runner_type" end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 49d9b0b1b4fd..67896ae1fc55 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -23,13 +23,13 @@ class Runner < Grape::API runner = if runner_registration_token_valid? # Create shared runner. Requires admin access - Ci::Runner.create(attributes.merge(is_shared: true)) + Ci::Runner.create(attributes.merge(is_shared: true, runner_type: :instance_type)) elsif project = Project.find_by(runners_token: params[:token]) # Create a specific runner for the project - project.runners.create(attributes) + project.runners.create(attributes.merge(runner_type: :project_type)) elsif group = Group.find_by(runners_token: params[:token]) # Create a specific runner for the group - group.runners.create(attributes) + group.runners.create(attributes.merge(runner_type: :group_type)) end break forbidden! unless runner -- GitLab From e98438d4efdf3aebabe5938979f4924cf7cb47a8 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 17:10:07 +0400 Subject: [PATCH 57/86] Revert changes to User#ci_authorized_users to defer group runner API changes to later --- app/models/user.rb | 16 ++-------- spec/models/user_spec.rb | 66 ++++++++-------------------------------- 2 files changed, 15 insertions(+), 67 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 0c5c0fef9d4a..b06681489720 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -995,17 +995,10 @@ def can_be_removed? def ci_authorized_runners @ci_authorized_runners ||= begin - project_runner_ids = Ci::RunnerProject + runner_ids = Ci::RunnerProject .where(project: authorized_projects(Gitlab::Access::MASTER)) .select(:runner_id) - - group_runner_ids = Ci::RunnerGroup - .where(group_id: owned_or_masters_groups.select(:id)) - .select(:runner_id) - - union = Gitlab::SQL::Union.new([project_runner_ids, group_runner_ids]) - - Ci::Runner.specific.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection + Ci::Runner.specific.where(id: runner_ids) end end @@ -1194,11 +1187,6 @@ def max_member_access_for_group(group_id) max_member_access_for_group_ids([group_id])[group_id] end - def owned_or_masters_groups - union = Gitlab::SQL::Union.new([owned_groups, masters_groups]) - Group.from("(#{union.to_sql}) namespaces") - end - protected # override, from Devise::Validatable diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index df2e547ce285..3f2eb58f0097 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1787,12 +1787,14 @@ describe '#ci_authorized_runners' do let(:user) { create(:user) } - let(:runner_1) { create(:ci_runner) } - let(:runner_2) { create(:ci_runner) } + let(:runner) { create(:ci_runner) } - context 'without any projects nor groups' do - let!(:project) { create(:project, runners: [runner_1]) } - let!(:group) { create(:group) } + before do + project.runners << runner + end + + context 'without any projects' do + let(:project) { create(:project) } it 'does not load' do expect(user.ci_authorized_runners).to be_empty @@ -1801,38 +1803,10 @@ context 'with personal projects runners' do let(:namespace) { create(:namespace, owner: user) } - let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) } + let(:project) { create(:project, namespace: namespace) } it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner_1) - end - end - - context 'with personal group runner' do - let!(:project) { create(:project, runners: [runner_1]) } - let!(:group) do - create(:group, runners: [runner_2]).tap do |group| - group.add_owner(user) - end - end - - it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner_2) - end - end - - context 'with personal project and group runner' do - let(:namespace) { create(:namespace, owner: user) } - let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) } - - let!(:group) do - create(:group, runners: [runner_2]).tap do |group| - group.add_owner(user) - end - end - - it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner_1, runner_2) + expect(user.ci_authorized_runners).to contain_exactly(runner) end end @@ -1843,7 +1817,7 @@ end it 'loads' do - expect(user.ci_authorized_runners).to contain_exactly(runner_1) + expect(user.ci_authorized_runners).to contain_exactly(runner) end end @@ -1860,21 +1834,7 @@ context 'with groups projects runners' do let(:group) { create(:group) } - let!(:project) { create(:project, group: group, runners: [runner_1]) } - - def add_user(access) - group.add_user(user, access) - end - - it_behaves_like :member - end - - context 'with groups runners' do - let!(:group) do - create(:group, runners: [runner_1]).tap do |group| - group.add_owner(user) - end - end + let(:project) { create(:project, group: group) } def add_user(access) group.add_user(user, access) @@ -1884,7 +1844,7 @@ def add_user(access) end context 'with other projects runners' do - let!(:project) { create(:project, runners: [runner_1]) } + let(:project) { create(:project) } def add_user(access) project.add_role(user, access) @@ -1897,7 +1857,7 @@ def add_user(access) let(:group) { create(:group) } let(:another_user) { create(:user) } let(:subgroup) { create(:group, parent: group) } - let!(:project) { create(:project, group: subgroup, runners: [runner_1]) } + let(:project) { create(:project, group: subgroup) } def add_user(access) group.add_user(user, access) -- GitLab From dad35d6284edfb5c5555e028405d7dc8e984b2f4 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 17:14:39 +0400 Subject: [PATCH 58/86] Remove redundant scopes in Ci::RegisterJobService#builds_for_group_runner --- app/services/ci/register_job_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 647bceb3b360..138bab88059f 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -90,7 +90,7 @@ def builds_for_project_runner 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 + projects = Project.where(namespace_id: hierarchy_groups) new_builds.where(project: projects.without_deleted.with_builds_enabled).order('created_at ASC') end -- GitLab From d8dd25a60e7c96a24d33cfe1b93be09085a1b86c Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 17:25:49 +0400 Subject: [PATCH 59/86] Introduce Project#all_runners and use in Ci::UpdateBuildQueueService --- app/models/project.rb | 9 ++++++--- app/services/ci/update_build_queue_service.rb | 10 +--------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 5c9bf8c61dd2..8abbb92da628 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1304,10 +1304,13 @@ def group_runners @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_parent_group_of_project(self.id) : Ci::Runner.none end + def all_runners + union = Gitlab::SQL::Union.new([runners, group_runners, shared_runners]) + Ci::Runner.from("(#{union.to_sql}) ci_runners") + end + def any_runners?(&block) - union = Gitlab::SQL::Union.new([runners, shared_runners, group_runners]) - runners = Ci::Runner.from("(#{union.to_sql}) ci_runners").active - runners.any?(&block) + all_runners.active.any?(&block) end def valid_runners_token?(token) diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index c991f04ab30a..674782df00ec 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -1,15 +1,7 @@ module Ci class UpdateBuildQueueService def execute(build) - tick_for(build, build.project.runners) - - if build.project.group_runners_enabled? - tick_for(build, Ci::Runner.belonging_to_parent_group_of_project(build.project_id)) - end - - if build.project.shared_runners_enabled? - tick_for(build, Ci::Runner.shared) - end + tick_for(build, build.project.all_runners) end private -- GitLab From e2b62f6e4441fe5fd0f721d5a824c62c7f40e013 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 17:27:21 +0400 Subject: [PATCH 60/86] Remove API changes for assigning group runner to project Since we are going to handle the API stuff later we can just leave this as error case since the model validations will stop this --- lib/api/runners.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 11c31917fc5a..5f2a95676051 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -205,7 +205,6 @@ def authenticate_delete_runner!(runner) def authenticate_enable_runner!(runner) forbidden!("Runner is shared") if runner.is_shared? forbidden!("Runner is locked") if runner.locked? - forbidden!("Runner is a group runner") if runner.assigned_to_group? return if current_user.admin? forbidden!("No access granted") unless user_can_access_runner?(runner) -- GitLab From 1a6d9789db04c3caa4f15ea76399c417f310a6a7 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 18:13:16 +0400 Subject: [PATCH 61/86] Remove unnecessary API specs for group runners since we do not have API support yet --- spec/requests/api/runners_spec.rb | 79 +------------------------------ 1 file changed, 1 insertion(+), 78 deletions(-) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 89e21ba9914a..f22fec315146 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -47,7 +47,7 @@ expect(response).to include_pagination_headers expect(json_response).to be_an Array expect(descriptions).to contain_exactly( - 'Project runner', 'Group runner', 'Two projects runner' + 'Project runner', 'Two projects runner' ) expect(shared).to be_falsey end @@ -149,16 +149,6 @@ expect(json_response['projects'].first['id']).to eq(project.id) end - - it "returns the group's details for a group runner" do - get api("/runners/#{group_runner.id}", admin) - - expect(json_response['groups'].first).to eq( - 'id' => group.id, - 'web_url' => group.web_url, - 'name' => group.name - ) - end end it 'returns 404 if runner does not exists' do @@ -188,29 +178,12 @@ end end - context "runner group's administrative user" do - context 'when runner is not shared' do - it "returns runner's details" do - get api("/runners/#{group_runner.id}", user) - - expect(response).to have_http_status(200) - expect(json_response['id']).to eq(group_runner.id) - end - end - end - context 'other authorized user' do it "does not return project runner's details" do get api("/runners/#{project_runner.id}", user2) expect(response).to have_http_status(403) end - - it "does not return group runner's details" do - get api("/runners/#{group_runner.id}", user2) - - expect(response).to have_gitlab_http_status(403) - end end context 'unauthorized user' do @@ -219,12 +192,6 @@ expect(response).to have_http_status(401) end - - it "does not return group runner's details" do - get api("/runners/#{group_runner.id}") - - expect(response).to have_gitlab_http_status(401) - end end end @@ -301,12 +268,6 @@ def update_runner(id, user, args) expect(response).to have_http_status(403) end - it 'does not update group runner without access to it' do - put api("/runners/#{group_runner.id}", user2), description: 'test' - - expect(response).to have_gitlab_http_status(403) - end - it 'updates project runner with access to it' do description = project_runner.description put api("/runners/#{project_runner.id}", admin), description: 'test' @@ -316,16 +277,6 @@ def update_runner(id, user, args) expect(project_runner.description).to eq('test') expect(project_runner.description).not_to eq(description) end - - it 'updates group runner with access to it' do - description = group_runner.description - put api("/runners/#{group_runner.id}", admin), description: 'test' - group_runner.reload - - expect(response).to have_gitlab_http_status(200) - expect(group_runner.description).to eq('test') - expect(group_runner.description).not_to eq(description) - end end end @@ -335,12 +286,6 @@ def update_runner(id, user, args) expect(response).to have_http_status(401) end - - it 'does not delete group runner' do - put api("/runners/#{group_runner.id}") - - expect(response).to have_gitlab_http_status(401) - end end end @@ -376,14 +321,6 @@ def update_runner(id, user, args) expect(response).to have_http_status(204) end.to change { Ci::Runner.specific.count }.by(-1) end - - it 'deletes used group runner' do - expect do - delete api("/runners/#{group_runner.id}", admin) - - expect(response).to have_gitlab_http_status(204) - end.to change { Ci::Runner.specific.count }.by(-1) - end end it 'returns 404 if runner does not exists' do @@ -420,14 +357,6 @@ def update_runner(id, user, args) end.to change { Ci::Runner.specific.count }.by(-1) end - it 'deletes group runner for one owned group' do - expect do - delete api("/runners/#{group_runner.id}", user) - - expect(response).to have_gitlab_http_status(204) - end.to change { Ci::Runner.specific.count }.by(-1) - end - it_behaves_like '412 response' do let(:request) { api("/runners/#{project_runner.id}", user) } end @@ -440,12 +369,6 @@ def update_runner(id, user, args) expect(response).to have_http_status(401) end - - it 'does not delete group runner' do - delete api("/runners/#{group_runner.id}") - - expect(response).to have_gitlab_http_status(401) - end end end -- GitLab From 36cdd1e7179aedee7af42d100a208fc1c01e6c63 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Tue, 1 May 2018 13:11:57 +0400 Subject: [PATCH 62/86] Use group_type? where possible during transition period --- app/services/ci/register_job_service.rb | 2 +- app/views/admin/runners/_runner.html.haml | 4 ++-- app/views/admin/runners/show.html.haml | 2 +- spec/services/ci/register_job_service_spec.rb | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 138bab88059f..8f8a5fbb2b01 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -17,7 +17,7 @@ def execute builds = if runner.shared? builds_for_shared_runner - elsif runner.assigned_to_group? + elsif runner.group_type? builds_for_group_runner else builds_for_project_runner diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index 6670ba6aa89f..6e76e7c27688 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -2,7 +2,7 @@ %td - if runner.shared? %span.label.label-success shared - - elsif runner.assigned_to_group? + - elsif runner.group_type? %span.label.label-success group - else %span.label.label-info specific @@ -21,7 +21,7 @@ %td = runner.ip_address %td - - if runner.shared? || runner.assigned_to_group? + - if runner.shared? || runner.group_type? n/a - else = runner.projects.count(:all) diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index ab2c9ad1e570..3b5fedfb0583 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -19,7 +19,7 @@ %p If you want Runners to build only specific projects, enable them in the table below. Keep in mind that this is a one way transition. -- elsif @runner.assigned_to_group? +- elsif @runner.group_type? .bs-callout.bs-callout-success %h4 This runner will process jobs from all projects in its group and subgroups - else diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 7d3c43eeaf7a..256d0027d72d 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -8,7 +8,7 @@ module Ci let!(:pending_job) { create :ci_build, pipeline: pipeline } let!(:shared_runner) { create :ci_runner, is_shared: true } let!(:specific_runner) { create :ci_runner, is_shared: false } - let!(:group_runner) { create :ci_runner, groups: [group] } + let!(:group_runner) { create :ci_runner, groups: [group], runner_type: :group_type } before do specific_runner.assign_to(project) -- GitLab From 7e0325896081152e7dd5033342ee4f1b485a76ea Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Tue, 1 May 2018 13:22:25 +0400 Subject: [PATCH 63/86] Remove N+1 query from app/views/projects/runners/_runner.html.haml --- app/views/projects/runners/_runner.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index d2598f3be072..385e2f8d1c21 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -26,7 +26,7 @@ - else - runner_project = @project.runner_projects.find_by(runner_id: runner) = link_to 'Disable for this project', project_runner_project_path(@project, runner_project), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - - elsif runner.assigned_to_project? + - elsif !(runner.is_shared? || runner.group_type?) = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f| = f.hidden_field :runner_id, value: runner.id = f.submit 'Enable for this project', class: 'btn btn-sm' -- GitLab From b7b823246602d6821f1773274ee6017c9f46e93f Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Tue, 1 May 2018 13:26:18 +0400 Subject: [PATCH 64/86] Simplify AddCiRunnerGroups migration --- .../20170301101006_add_ci_runner_groups.rb | 16 ++++++---------- db/schema.rb | 4 ++-- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/db/migrate/20170301101006_add_ci_runner_groups.rb b/db/migrate/20170301101006_add_ci_runner_groups.rb index 1c4430981a95..558e4d08b8fd 100644 --- a/db/migrate/20170301101006_add_ci_runner_groups.rb +++ b/db/migrate/20170301101006_add_ci_runner_groups.rb @@ -5,19 +5,15 @@ class AddCiRunnerGroups < ActiveRecord::Migration disable_ddl_transaction! - def up + def change create_table :ci_runner_groups do |t| t.integer :runner_id t.integer :group_id - end - - add_concurrent_index :ci_runner_groups, :group_id - add_concurrent_index :ci_runner_groups, [:runner_id, :group_id], unique: true - add_concurrent_foreign_key :ci_runner_groups, :ci_runners, column: :runner_id, on_delete: :cascade - add_concurrent_foreign_key :ci_runner_groups, :namespaces, column: :group_id, on_delete: :cascade - end - def down - drop_table :ci_runner_groups + t.index [:runner_id, :group_id], unique: true + t.index :group_id + t.foreign_key :ci_runners, column: :runner_id, on_delete: :cascade + t.foreign_key :namespaces, column: :group_id, on_delete: :cascade + end end end diff --git a/db/schema.rb b/db/schema.rb index 4a541b3ac819..88e9b3bd65b9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2096,8 +2096,8 @@ add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade - add_foreign_key "ci_runner_groups", "ci_runners", column: "runner_id", name: "fk_d8a0baa93b", on_delete: :cascade - add_foreign_key "ci_runner_groups", "namespaces", column: "group_id", name: "fk_cdafb3bbba", on_delete: :cascade + add_foreign_key "ci_runner_groups", "ci_runners", column: "runner_id", on_delete: :cascade + add_foreign_key "ci_runner_groups", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade -- GitLab From 0e5c1a89f0b6ecf88fc340194d292fccbde99782 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Tue, 1 May 2018 13:44:35 +0400 Subject: [PATCH 65/86] Fix spec/features/admin/admin_runners_spec.rb + test style improvements --- .../settings/ci_cd_controller_spec.rb | 20 ++++++++++++------- spec/features/admin/admin_runners_spec.rb | 6 +++--- spec/models/ci/runner_spec.rb | 8 ++++---- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index 1cf395b03284..a91c868cbaf6 100644 --- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb +++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb @@ -18,15 +18,21 @@ expect(response).to render_template(:show) end - it 'sets assignable project runners' do - group = create(:group, runners: [create(:ci_runner)], parent: create(:group)) - group.add_master(user) - project_runner = create(:ci_runner, projects: [create(:project, group: group)]) - create(:ci_runner, :shared) + context 'with group runners' do + let(:group_runner) { create(:ci_runner) } + let(:parent_group) { create(:group) } + let(:group) { create(:group, runners: [group_runner], parent: parent_group) } + let(:other_project) { create(:project, group: group) } + let!(:project_runner) { create(:ci_runner, projects: [other_project]) } + let!(:shared_runner) { create(:ci_runner, :shared) } - get :show, namespace_id: project.namespace, project_id: project + it 'sets assignable project runners only' do + group.add_master(user) + + get :show, namespace_id: project.namespace, project_id: project - expect(assigns(:assignable_runners)).to eq [project_runner] + expect(assigns(:assignable_runners)).to eq [project_runner] + end end end diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index b0aa2e8b5880..3465ccfc4238 100644 --- a/spec/features/admin/admin_runners_spec.rb +++ b/spec/features/admin/admin_runners_spec.rb @@ -61,10 +61,10 @@ end context 'group runner' do - it 'shows the label and does not show the project count' do - group = create :group - runner = create :ci_runner, groups: [group] + let(:group) { create(:group) } + let!(:runner) { create(:ci_runner, groups: [group], runner_type: :group_type) } + it 'shows the label and does not show the project count' do visit admin_runners_path within "#runner_#{runner.id}" do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index fb9dcce9a7c7..fa540f8d4fd7 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -21,8 +21,9 @@ end context 'either_projects_or_group' do + let(:group) { create(:group) } + it 'disallows assigning to a group if already assigned to a group' do - group = create(:group) runner = create(:ci_runner, groups: [group]) runner.groups << build(:group) @@ -42,7 +43,6 @@ end it 'disallows assigning to a project if already assigned to a group' do - group = create(:group) runner = create(:ci_runner, groups: [group]) runner.projects << build(:project) @@ -189,9 +189,9 @@ # globally shared shared_runner = create :ci_runner, :shared - expect(described_class.owned_or_shared(project.id)).to match_array [ + expect(described_class.owned_or_shared(project.id)).to contain_exactly( group_runner, project_runner, shared_runner - ] + ) end end -- GitLab From 2261188f48dff25c5bfbbca739c5f570849155f4 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Wed, 2 May 2018 16:42:12 +0200 Subject: [PATCH 66/86] Rename Runner#invalidate_build_cache -> Runner#pick_build --- app/models/ci/runner.rb | 2 +- app/services/ci/update_build_queue_service.rb | 2 +- spec/models/ci/runner_spec.rb | 6 +++--- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index cdd284071728..4d4aff4c8300 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -205,7 +205,7 @@ def update_cached_info(values) end end - def invalidate_build_cache!(build) + def pick_build!(build) if can_pick?(build) tick_runner_queue end diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 674782df00ec..41b1c144c3e8 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -8,7 +8,7 @@ def execute(build) def tick_for(build, runners) runners.each do |runner| - runner.invalidate_build_cache!(build) + runner.pick_build!(build) end end end diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index fa540f8d4fd7..6ad374176235 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -792,7 +792,7 @@ def expect_redis_update end end - describe '#invalidate_build_cache!' do + describe '#pick_build!' do context 'runner can pick the build' do it 'calls #tick_runner_queue' do ci_build = build :ci_build @@ -801,7 +801,7 @@ def expect_redis_update expect(runner).to receive(:tick_runner_queue) - runner.invalidate_build_cache!(ci_build) + runner.pick_build!(ci_build) end end @@ -813,7 +813,7 @@ def expect_redis_update expect(runner).not_to receive(:tick_runner_queue) - runner.invalidate_build_cache!(ci_build) + runner.pick_build!(ci_build) end end end -- GitLab From 0970e7b9608d6ada1c0fe45242ea092ea91068aa Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Wed, 2 May 2018 17:43:20 +0200 Subject: [PATCH 67/86] Rename RunnerGroup -> RunnerNamespace --- app/models/ci/runner.rb | 6 +++--- app/models/ci/runner_group.rb | 8 -------- app/models/ci/runner_namespace.rb | 9 +++++++++ app/models/group.rb | 2 -- app/models/namespace.rb | 3 +++ .../20170301101006_add_ci_runner_groups.rb | 19 ------------------- ...20170301101006_add_ci_runner_namespaces.rb | 19 +++++++++++++++++++ db/schema.rb | 12 ++++++------ 8 files changed, 40 insertions(+), 38 deletions(-) delete mode 100644 app/models/ci/runner_group.rb create mode 100644 app/models/ci/runner_namespace.rb delete mode 100644 db/migrate/20170301101006_add_ci_runner_groups.rb create mode 100644 db/migrate/20170301101006_add_ci_runner_namespaces.rb diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 4d4aff4c8300..0b47b71a2678 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -14,8 +14,8 @@ class Runner < ActiveRecord::Base has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :runner_projects - has_many :runner_groups - has_many :groups, through: :runner_groups + has_many :runner_namespaces + has_many :groups, through: :runner_namespaces has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build' @@ -144,7 +144,7 @@ def specific? end def assigned_to_group? - runner_groups.any? + runner_namespaces.any? end def assigned_to_project? diff --git a/app/models/ci/runner_group.rb b/app/models/ci/runner_group.rb deleted file mode 100644 index 87f3ba13bff0..000000000000 --- a/app/models/ci/runner_group.rb +++ /dev/null @@ -1,8 +0,0 @@ -module Ci - class RunnerGroup < ActiveRecord::Base - extend Gitlab::Ci::Model - - belongs_to :runner - belongs_to :group, class_name: '::Group' - end -end diff --git a/app/models/ci/runner_namespace.rb b/app/models/ci/runner_namespace.rb new file mode 100644 index 000000000000..3269f86e8caa --- /dev/null +++ b/app/models/ci/runner_namespace.rb @@ -0,0 +1,9 @@ +module Ci + class RunnerNamespace < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :runner + belongs_to :namespace, class_name: '::Namespace' + belongs_to :group, class_name: '::Group', foreign_key: :namespace_id + end +end diff --git a/app/models/group.rb b/app/models/group.rb index f21008e5f75e..f493836a92e7 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -29,8 +29,6 @@ class Group < Namespace has_many :labels, class_name: 'GroupLabel' has_many :variables, class_name: 'Ci::GroupVariable' has_many :custom_attributes, class_name: 'GroupCustomAttribute' - has_many :runner_groups, class_name: 'Ci::RunnerGroup' - has_many :runners, through: :runner_groups, source: :runner, class_name: 'Ci::Runner' has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent diff --git a/app/models/namespace.rb b/app/models/namespace.rb index c29a53e5ce7b..5621eeba7c42 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -21,6 +21,9 @@ class Namespace < ActiveRecord::Base has_many :projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :project_statistics + has_many :runner_namespaces, class_name: 'Ci::RunnerNamespace' + has_many :runners, through: :runner_namespaces, source: :runner, class_name: 'Ci::Runner' + # This should _not_ be `inverse_of: :namespace`, because that would also set # `user.namespace` when this user creates a group with themselves as `owner`. belongs_to :owner, class_name: "User" diff --git a/db/migrate/20170301101006_add_ci_runner_groups.rb b/db/migrate/20170301101006_add_ci_runner_groups.rb deleted file mode 100644 index 558e4d08b8fd..000000000000 --- a/db/migrate/20170301101006_add_ci_runner_groups.rb +++ /dev/null @@ -1,19 +0,0 @@ -class AddCiRunnerGroups < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - disable_ddl_transaction! - - def change - create_table :ci_runner_groups do |t| - t.integer :runner_id - t.integer :group_id - - t.index [:runner_id, :group_id], unique: true - t.index :group_id - t.foreign_key :ci_runners, column: :runner_id, on_delete: :cascade - t.foreign_key :namespaces, column: :group_id, on_delete: :cascade - end - end -end diff --git a/db/migrate/20170301101006_add_ci_runner_namespaces.rb b/db/migrate/20170301101006_add_ci_runner_namespaces.rb new file mode 100644 index 000000000000..7e7df2f4d222 --- /dev/null +++ b/db/migrate/20170301101006_add_ci_runner_namespaces.rb @@ -0,0 +1,19 @@ +class AddCiRunnerNamespaces < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def change + create_table :ci_runner_namespaces do |t| + t.integer :runner_id + t.integer :namespace_id + + t.index [:runner_id, :namespace_id], unique: true + t.index :namespace_id + t.foreign_key :ci_runners, column: :runner_id, on_delete: :cascade + t.foreign_key :namespaces, column: :namespace_id, on_delete: :cascade + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 88e9b3bd65b9..ef9d299580b8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -443,13 +443,13 @@ add_index "ci_pipelines", ["status"], name: "index_ci_pipelines_on_status", using: :btree add_index "ci_pipelines", ["user_id"], name: "index_ci_pipelines_on_user_id", using: :btree - create_table "ci_runner_groups", force: :cascade do |t| + create_table "ci_runner_namespaces", force: :cascade do |t| t.integer "runner_id" - t.integer "group_id" + t.integer "namespace_id" end - add_index "ci_runner_groups", ["group_id"], name: "index_ci_runner_groups_on_group_id", using: :btree - add_index "ci_runner_groups", ["runner_id", "group_id"], name: "index_ci_runner_groups_on_runner_id_and_group_id", unique: true, using: :btree + add_index "ci_runner_namespaces", ["namespace_id"], name: "index_ci_runner_namespaces_on_namespace_id", using: :btree + add_index "ci_runner_namespaces", ["runner_id", "namespace_id"], name: "index_ci_runner_namespaces_on_runner_id_and_namespace_id", unique: true, using: :btree create_table "ci_runner_projects", force: :cascade do |t| t.integer "runner_id", null: false @@ -2096,8 +2096,8 @@ add_foreign_key "ci_pipelines", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_3d34ab2e06", on_delete: :nullify add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade - add_foreign_key "ci_runner_groups", "ci_runners", column: "runner_id", on_delete: :cascade - add_foreign_key "ci_runner_groups", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "ci_runner_namespaces", "ci_runners", column: "runner_id", on_delete: :cascade + add_foreign_key "ci_runner_namespaces", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade -- GitLab From d188c7a26f0c9abf3bf04a8ae48dc3bebda6d169 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 08:16:31 +0200 Subject: [PATCH 68/86] Fix db/schema.rb for add_foreign_key ci_runner_namespaces namespace_id --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index ef9d299580b8..1806eeed80ec 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2097,7 +2097,7 @@ add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade add_foreign_key "ci_runner_namespaces", "ci_runners", column: "runner_id", on_delete: :cascade - add_foreign_key "ci_runner_namespaces", "namespaces", column: "group_id", on_delete: :cascade + add_foreign_key "ci_runner_namespaces", "namespaces", column: "namespace_id", on_delete: :cascade add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade -- GitLab From 80bcb376caafe9b5b1cf33e850df71a207510285 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 08:46:58 +0200 Subject: [PATCH 69/86] Again fix db/schema.rb for ci_runner_namespaces --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 1806eeed80ec..8e79469d9564 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -2097,7 +2097,7 @@ add_foreign_key "ci_pipelines", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_262d4c2d19", on_delete: :nullify add_foreign_key "ci_pipelines", "projects", name: "fk_86635dbd80", on_delete: :cascade add_foreign_key "ci_runner_namespaces", "ci_runners", column: "runner_id", on_delete: :cascade - add_foreign_key "ci_runner_namespaces", "namespaces", column: "namespace_id", on_delete: :cascade + add_foreign_key "ci_runner_namespaces", "namespaces", on_delete: :cascade add_foreign_key "ci_runner_projects", "projects", name: "fk_4478a6f1e4", on_delete: :cascade add_foreign_key "ci_stages", "ci_pipelines", column: "pipeline_id", name: "fk_fb57e6cc56", on_delete: :cascade add_foreign_key "ci_stages", "projects", name: "fk_2360681d1d", on_delete: :cascade -- GitLab From 1f7f29b7321c9cba5526ab991246f3178330b9cd Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 08:47:41 +0200 Subject: [PATCH 70/86] Style changes to spec/models/ci/runner_spec.rb --- spec/models/ci/runner_spec.rb | 70 +++++++++++++++++------------------ 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 6ad374176235..744972deb4d8 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -107,16 +107,17 @@ end describe '.shared' do + let(:group) { create(:group) } + let(:project) { create(:project) } + it 'returns the shared group runner' do - group = create :group - runner = create :ci_runner, :shared, groups: [group] + runner = create(:ci_runner, :shared, groups: [group]) expect(described_class.shared).to eq [runner] end it 'returns the shared project runner' do - project = create :project - runner = create :ci_runner, :shared, projects: [project] + runner = create(:ci_runner, :shared, projects: [project]) expect(described_class.shared).to eq [runner] end @@ -125,12 +126,12 @@ describe '.belonging_to_project' do it 'returns the specific project runner' do # own - specific_project = create :project - specific_runner = create :ci_runner, :specific, projects: [specific_project] + specific_project = create(:project) + specific_runner = create(:ci_runner, :specific, projects: [specific_project]) # other - other_project = create :project - create :ci_runner, :specific, projects: [other_project] + other_project = create(:project) + create(:ci_runner, :specific, projects: [other_project]) expect(described_class.belonging_to_project(specific_project.id)).to eq [specific_runner] end @@ -139,55 +140,54 @@ describe '.belonging_to_any_project' do it 'returns the specific project runner' do # project - project_project = create :project - project_runner = create :ci_runner, :specific, projects: [project_project] + project_project = create(:project) + project_runner = create(:ci_runner, :specific, projects: [project_project]) # group - group = create :group - create :project, group: group - create :ci_runner, :specific, groups: [group] + group = create(:group) + create(:project, group: group) + create(:ci_runner, :specific, groups: [group]) expect(described_class.belonging_to_any_project).to eq [project_runner] end end describe '.belonging_to_parent_group_of_project' do - it 'returns the specific group runner' do - # own - specific_group = create :group - specific_project = create :project, group: specific_group - specific_runner = create :ci_runner, :specific, groups: [specific_group] - - # other - other_group = create :group - create :project, group: other_group - create :ci_runner, :specific, groups: [other_group] + let(:project) { create(:project, group: group) } + let(:group) { create(:group) } + let(:runner) { create(:ci_runner, :specific, groups: [group]) } + let!(:unrelated_group) { create(:group) } + let!(:unrelated_project) { create(:project, group: unrelated_group) } + let!(:unrelated_runner) { create(:ci_runner, :specific, groups: [unrelated_group]) } - expect(described_class.belonging_to_parent_group_of_project(specific_project.id)).to eq [specific_runner] + it 'returns the specific group runner' do + expect(described_class.belonging_to_parent_group_of_project(project.id)).to contain_exactly(runner) end - it 'returns the group runner from a parent group', :nested_groups do - parent_group = create :group - group = create :group, parent: parent_group - project = create :project, group: group - runner = create :ci_runner, :specific, groups: [parent_group] + context 'with a parent group with a runner', :nested_groups do + let(:runner) { create(:ci_runner, :specific, groups: [parent_group]) } + let(:project) { create(:project, group: group) } + let(:group) { create(:group, parent: parent_group) } + let(:parent_group) { create(:group) } - expect(described_class.belonging_to_parent_group_of_project(project.id)).to eq [runner] + it 'returns the group runner from the parent group' do + expect(described_class.belonging_to_parent_group_of_project(project.id)).to contain_exactly(runner) + end end end describe '.owned_or_shared' do it 'returns a globally shared, a project specific and a group specific runner' do # group specific - group = create :group - project = create :project, group: group - group_runner = create :ci_runner, :specific, groups: [group] + group = create(:group) + project = create(:project, group: group) + group_runner = create(:ci_runner, :specific, groups: [group]) # project specific - project_runner = create :ci_runner, :specific, projects: [project] + project_runner = create(:ci_runner, :specific, projects: [project]) # globally shared - shared_runner = create :ci_runner, :shared + shared_runner = create(:ci_runner, :shared) expect(described_class.owned_or_shared(project.id)).to contain_exactly( group_runner, project_runner, shared_runner -- GitLab From a6c9db61779c71fd0a7f0f317fba13f8931ab954 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 09:38:19 +0200 Subject: [PATCH 71/86] More style improvements to spec/models/ci/runner_spec.rb --- spec/models/ci/runner_spec.rb | 81 +++++++++++++++++------------------ 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 744972deb4d8..21d7d616a3c8 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -309,7 +309,9 @@ def stub_redis_runner_contacted_at(value) describe '#can_pick?' do let(:pipeline) { create(:ci_pipeline) } let(:build) { create(:ci_build, pipeline: pipeline) } - let(:runner) { create(:ci_runner) } + let(:runner) { create(:ci_runner, tag_list: tag_list, run_untagged: run_untagged) } + let(:tag_list) { [] } + let(:run_untagged) { true } subject { runner.can_pick?(build) } @@ -319,7 +321,7 @@ def stub_redis_runner_contacted_at(value) context 'a different runner' do it 'cannot handle builds' do - other_runner = create :ci_runner + other_runner = create(:ci_runner) expect(other_runner.can_pick?(build)).to be_falsey end end @@ -337,9 +339,7 @@ def stub_redis_runner_contacted_at(value) end context 'when runner has tags' do - before do - runner.tag_list = %w(bb cc) - end + let(:tag_list) { %w(bb cc) } shared_examples 'tagged build picker' do it 'can handle build with matching tags' do @@ -364,9 +364,7 @@ def stub_redis_runner_contacted_at(value) end context 'when runner cannot pick untagged jobs' do - before do - runner.update_attributes!(run_untagged: false) - end + let(:run_untagged) { false } it 'cannot handle builds without tags' do expect(runner.can_pick?(build)).to be_falsey @@ -377,8 +375,9 @@ def stub_redis_runner_contacted_at(value) end context 'when runner is shared' do + let(:runner) { create(:ci_runner, :shared) } + before do - runner.update_attributes!(is_shared: true) build.project.runners = [] end @@ -387,9 +386,7 @@ def stub_redis_runner_contacted_at(value) end context 'when runner is locked' do - before do - runner.update_attributes!(locked: true) - end + let(:runner) { create(:ci_runner, :shared, locked: true) } it 'can handle builds' do expect(runner.can_pick?(build)).to be_truthy @@ -748,55 +745,57 @@ def expect_redis_update end end - describe 'assigned_to_group?' do - it 'returns false when the runner is a project runner' do - project = create :project - runner = create :ci_runner, description: 'Project runner', projects: [project] + describe '#assigned_to_group?' do + subject { runner.assigned_to_group? } + + context 'when project runner' do + let(:runner) { create(:ci_runner, description: 'Project runner', projects: [project]) } + let(:project) { create(:project) } - expect(runner.assigned_to_group?).to be false + it { is_expected.to be_falsey } end - it 'returns false when the runner is a shared runner' do - runner = create :ci_runner, :shared, description: 'Shared runner' + context 'when shared runner' do + let(:runner) { create(:ci_runner, :shared, description: 'Shared runner') } - expect(runner.assigned_to_group?).to be false + it { is_expected.to be_falsey } end - it 'returns true when the runner is assigned to a group' do - group = create :group - runner = create :ci_runner, description: 'Group runner', groups: [group] + context 'when group runner' do + let(:group) { create(:group) } + let(:runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } - expect(runner.assigned_to_group?).to be true + it { is_expected.to be_truthy } end end - describe 'assigned_to_project?' do - it 'returns false when the runner is a group prunner' do - group = create :group - runner = create :ci_runner, description: 'Group runner', groups: [group] + describe '#assigned_to_project?' do + subject { runner.assigned_to_project? } - expect(runner.assigned_to_project?).to be false + context 'when group runner' do + let(:runner) { create(:ci_runner, description: 'Group runner', groups: [group]) } + let(:group) { create(:group) } + it { is_expected.to be_falsey } end - it 'returns false when the runner is a shared runner' do - runner = create :ci_runner, :shared, description: 'Shared runner' - - expect(runner.assigned_to_project?).to be false + context 'when shared runner' do + let(:runner) { create(:ci_runner, :shared, description: 'Shared runner') } + it { is_expected.to be_falsey } end - it 'returns true when the runner is assigned to a project' do - project = create :project - runner = create :ci_runner, description: 'Group runner', projects: [project] + context 'when project runner' do + let(:runner) { create(:ci_runner, description: 'Group runner', projects: [project]) } + let(:project) { create(:project) } - expect(runner.assigned_to_project?).to be true + it { is_expected.to be_truthy } end end describe '#pick_build!' do context 'runner can pick the build' do it 'calls #tick_runner_queue' do - ci_build = build :ci_build - runner = build :ci_runner + ci_build = build(:ci_build) + runner = build(:ci_runner) allow(runner).to receive(:can_pick?).with(ci_build).and_return(true) expect(runner).to receive(:tick_runner_queue) @@ -807,8 +806,8 @@ def expect_redis_update context 'runner cannot pick the build' do it 'does not call #tick_runner_queue' do - ci_build = build :ci_build - runner = build :ci_runner + ci_build = build(:ci_build) + runner = build(:ci_runner) allow(runner).to receive(:can_pick?).with(ci_build).and_return(false) expect(runner).not_to receive(:tick_runner_queue) -- GitLab From 67f25c6259553e30e921de3d4d72d3e97d06d327 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 09:44:45 +0200 Subject: [PATCH 72/86] Style improvements to spec/models/project_spec.rb --- spec/models/project_spec.rb | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index ab0694e6890d..08e42b619109 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -1138,7 +1138,7 @@ def create_pipeline end end - describe '#any_runners' do + describe '#any_runners?' do context 'shared runners' do let(:project) { create :project, shared_runners_enabled: shared_runners_enabled } let(:specific_runner) { create :ci_runner } @@ -1153,21 +1153,25 @@ def create_pipeline it 'has a specific runner' do project.runners << specific_runner + expect(project.any_runners?).to be_truthy end it 'has a shared runner, but they are prohibited to use' do shared_runner + expect(project.any_runners?).to be_falsey end it 'checks the presence of specific runner' do project.runners << specific_runner + expect(project.any_runners? { |runner| runner == specific_runner }).to be_truthy end it 'returns false if match cannot be found' do project.runners << specific_runner + expect(project.any_runners? { false }).to be_falsey end end @@ -1177,16 +1181,19 @@ def create_pipeline it 'has a shared runner' do shared_runner + expect(project.any_runners?).to be_truthy end it 'checks the presence of shared runner' do shared_runner + expect(project.any_runners? { |runner| runner == shared_runner }).to be_truthy end it 'returns false if match cannot be found' do shared_runner + expect(project.any_runners? { false }).to be_falsey end end @@ -1206,6 +1213,7 @@ def create_pipeline it 'has a group runner, but they are prohibited to use' do group_runner + expect(project.any_runners?).to be_falsey end end @@ -1215,16 +1223,19 @@ def create_pipeline it 'has a group runner' do group_runner + expect(project.any_runners?).to be_truthy end it 'checks the presence of group runner' do group_runner + expect(project.any_runners? { |runner| runner == group_runner }).to be_truthy end it 'returns false if match cannot be found' do group_runner + expect(project.any_runners? { false }).to be_falsey end end @@ -3592,7 +3603,7 @@ def domain_variable describe '#toggle_ci_cd_settings!' do it 'toggles the value on #settings' do - project = create :project, group_runners_enabled: false + project = create(:project, group_runners_enabled: false) expect(project.group_runners_enabled).to be false -- GitLab From dcb67951a817db262ddcd3b777fafc4e1995fc04 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 09:45:09 +0200 Subject: [PATCH 73/86] Make assertions about runner_type in spec/requests/api/runner_spec.rb --- spec/requests/api/runner_spec.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 5ea110b4d826..27f5dff79010 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -40,6 +40,7 @@ expect(json_response['token']).to eq(runner.token) expect(runner.run_untagged).to be true expect(runner.token).not_to eq(registration_token) + expect(runner).to be_instance_type end context 'when project token is used' do @@ -50,8 +51,10 @@ expect(response).to have_gitlab_http_status 201 expect(project.runners.size).to eq(1) - expect(Ci::Runner.first.token).not_to eq(registration_token) - expect(Ci::Runner.first.token).not_to eq(project.runners_token) + runner = Ci::Runner.first + expect(runner.token).not_to eq(registration_token) + expect(runner.token).not_to eq(project.runners_token) + expect(runner).to be_project_type end end @@ -63,8 +66,10 @@ expect(response).to have_http_status 201 expect(group.runners.size).to eq(1) - expect(Ci::Runner.first.token).not_to eq(registration_token) - expect(Ci::Runner.first.token).not_to eq(group.runners_token) + runner = Ci::Runner.first + expect(runner.token).not_to eq(registration_token) + expect(runner.token).not_to eq(group.runners_token) + expect(runner).to be_group_type end end end -- GitLab From 49cbe576229b7e4003575e04006cc4132c3c0060 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 10:59:38 +0200 Subject: [PATCH 74/86] Remove Runner#belonging_to_any_project since this is no longer needed --- .../projects/settings/ci_cd_controller.rb | 1 - app/models/ci/runner.rb | 2 -- spec/models/ci/runner_spec.rb | 15 --------------- 3 files changed, 18 deletions(-) diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index 6ddbefacae04..177c8a54099a 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -72,7 +72,6 @@ def define_runners_variables .ci_authorized_runners .assignable_for(project) .ordered - .belonging_to_any_project .page(params[:page]).per(20) @shared_runners = ::Ci::Runner.shared.active diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 0b47b71a2678..2dfd038d5a8d 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -32,8 +32,6 @@ class Runner < ActiveRecord::Base joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) } - scope :belonging_to_any_project, -> { joins(:runner_projects) } - scope :belonging_to_parent_group_of_project, -> (project_id) { project_groups = ::Group.joins(:projects).where(projects: { id: project_id }) hierarchy_groups = Gitlab::GroupHierarchy.new(project_groups).base_and_ancestors diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 21d7d616a3c8..cc4d4e5e4ae4 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -137,21 +137,6 @@ end end - describe '.belonging_to_any_project' do - it 'returns the specific project runner' do - # project - project_project = create(:project) - project_runner = create(:ci_runner, :specific, projects: [project_project]) - - # group - group = create(:group) - create(:project, group: group) - create(:ci_runner, :specific, groups: [group]) - - expect(described_class.belonging_to_any_project).to eq [project_runner] - end - end - describe '.belonging_to_parent_group_of_project' do let(:project) { create(:project, group: group) } let(:group) { create(:group) } -- GitLab From dc439a3b42d6d376ca0b0853b0aab3b401922e5f Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 11:02:51 +0200 Subject: [PATCH 75/86] Add comment in _runner.html.haml about hacky runner type check --- app/views/projects/runners/_runner.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index 385e2f8d1c21..0d2c0536eb54 100644 --- a/app/views/projects/runners/_runner.html.haml +++ b/app/views/projects/runners/_runner.html.haml @@ -26,7 +26,7 @@ - else - runner_project = @project.runner_projects.find_by(runner_id: runner) = link_to 'Disable for this project', project_runner_project_path(@project, runner_project), data: { confirm: "Are you sure?" }, method: :delete, class: 'btn btn-danger btn-sm' - - elsif !(runner.is_shared? || runner.group_type?) + - elsif !(runner.is_shared? || runner.group_type?) # We can simplify this to `runner.project_type?` when migrating #runner_type is complete = form_for [@project.namespace.becomes(Namespace), @project, @project.runner_projects.new] do |f| = f.hidden_field :runner_id, value: runner.id = f.submit 'Enable for this project', class: 'btn btn-sm' -- GitLab From 7bc24ec2e524b55402fa5caeb64e75e02f97ddc5 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 11:03:19 +0200 Subject: [PATCH 76/86] Extract constants in 20180430143705_backfill_runner_type_for_ci_runners_post_migrate --- ...705_backfill_runner_type_for_ci_runners_post_migrate.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb index 8509222edc20..9186a729a40b 100644 --- a/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb +++ b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb @@ -3,14 +3,17 @@ class BackfillRunnerTypeForCiRunnersPostMigrate < ActiveRecord::Migration DOWNTIME = false + PROJECT_RUNNER_TYPE = 1 + INSTANCE_RUNNER_TYPE = 3 + disable_ddl_transaction! def up - update_column_in_batches(:ci_runners, :runner_type, 1) do |table, query| + update_column_in_batches(:ci_runners, :runner_type, PROJECT_RUNNER_TYPE) do |table, query| query.where(table[:is_shared].eq(true)).where(table[:runner_type].eq(nil)) end - update_column_in_batches(:ci_runners, :runner_type, 3) do |table, query| + update_column_in_batches(:ci_runners, :runner_type, INSTANCE_RUNNER_TYPE) do |table, query| query.where(table[:is_shared].eq(false)).where(table[:runner_type].eq(nil)) end end -- GitLab From bf790c26c58e214c27132e7a54fdf4a4cc77bdaf Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 13:39:20 +0200 Subject: [PATCH 77/86] Use factory in specs for ProjectCiCdSettings --- spec/factories/project_ci_cd_settings.rb | 10 ++++++++++ spec/factories/projects.rb | 8 +------- 2 files changed, 11 insertions(+), 7 deletions(-) create mode 100644 spec/factories/project_ci_cd_settings.rb diff --git a/spec/factories/project_ci_cd_settings.rb b/spec/factories/project_ci_cd_settings.rb new file mode 100644 index 000000000000..2e85b54e2453 --- /dev/null +++ b/spec/factories/project_ci_cd_settings.rb @@ -0,0 +1,10 @@ +FactoryBot.define do + factory :project_ci_cd_setting do + project + + initialize_with do + # ci_cd_settings are automatically created when a project is created + project&.ci_cd_settings || new + end + end +end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index aed5eab80449..e0e72e7f2ce9 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -14,6 +14,7 @@ # Associations namespace creator { group ? create(:user) : namespace&.owner } + ci_cd_settings strategy: :build, factory: :project_ci_cd_setting, project: nil transient do # Nest Project Feature attributes @@ -23,10 +24,6 @@ issues_access_level ProjectFeature::ENABLED merge_requests_access_level ProjectFeature::ENABLED repository_access_level ProjectFeature::ENABLED - - # we can't assign the delegated `#ci_cd_settings` attributes directly, as the - # `#ci_cd_settings` relation needs to be created first - group_runners_enabled nil end after(:create) do |project, evaluator| @@ -51,9 +48,6 @@ end project.group&.refresh_members_authorized_projects - - # assign the delegated `#ci_cd_settings` attributes after create - project.reload.group_runners_enabled = evaluator.group_runners_enabled unless evaluator.group_runners_enabled.nil? end trait :public do -- GitLab From af15b6f0e144762a38591c53b970e312c35fe65f Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 14:37:01 +0200 Subject: [PATCH 78/86] Fix Project#group_runners_enabled as it was doing nothing --- app/models/project.rb | 7 ++++- app/models/project_ci_cd_setting.rb | 2 +- app/services/ci/register_job_service.rb | 5 +++- spec/services/ci/register_job_service_spec.rb | 28 +++++++++++-------- 4 files changed, 28 insertions(+), 14 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 8abbb92da628..50c404c300a1 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -234,7 +234,7 @@ class Project < ActiveRecord::Base has_many :custom_attributes, class_name: 'ProjectCustomAttribute' has_many :project_badges, class_name: 'ProjectBadge' - has_one :ci_cd_settings, class_name: 'ProjectCiCdSetting' + has_one :ci_cd_settings, class_name: 'ProjectCiCdSetting', inverse_of: :project, autosave: true accepts_nested_attributes_for :variables, allow_destroy: true accepts_nested_attributes_for :project_feature, update_only: true @@ -331,6 +331,11 @@ class Project < ActiveRecord::Base scope :with_issues_available_for_user, ->(current_user) { with_feature_available_for_user(:issues, current_user) } scope :with_merge_requests_enabled, -> { with_feature_enabled(:merge_requests) } + scope :with_group_runners_enabled, -> do + joins(:ci_cd_settings) + .where(project_ci_cd_settings: { group_runners_enabled: true }) + end + enum auto_cancel_pending_pipelines: { disabled: 0, enabled: 1 } chronic_duration_attr :build_timeout_human_readable, :build_timeout, default: 3600 diff --git a/app/models/project_ci_cd_setting.rb b/app/models/project_ci_cd_setting.rb index 9f10a93148c3..588cced5781a 100644 --- a/app/models/project_ci_cd_setting.rb +++ b/app/models/project_ci_cd_setting.rb @@ -1,5 +1,5 @@ class ProjectCiCdSetting < ActiveRecord::Base - belongs_to :project + belongs_to :project, inverse_of: :ci_cd_settings # The version of the schema that first introduced this model/table. MINIMUM_SCHEMA_VERSION = 20180403035759 diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 8f8a5fbb2b01..a7d8ad93f38c 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -91,7 +91,10 @@ def builds_for_project_runner def builds_for_group_runner hierarchy_groups = Gitlab::GroupHierarchy.new(runner.groups).base_and_descendants projects = Project.where(namespace_id: hierarchy_groups) - new_builds.where(project: projects.without_deleted.with_builds_enabled).order('created_at ASC') + .with_group_runners_enabled + .with_builds_enabled + .without_deleted + new_builds.where(project: projects).order('created_at ASC') end def running_builds_for_shared_runners diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 256d0027d72d..8063bc7e1aca 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -2,13 +2,13 @@ module Ci describe RegisterJobService do - let!(:project) { create :project, shared_runners_enabled: false } - let!(:group) { create :group } - let!(:pipeline) { create :ci_pipeline, project: project } - let!(:pending_job) { create :ci_build, pipeline: pipeline } - let!(:shared_runner) { create :ci_runner, is_shared: true } - let!(:specific_runner) { create :ci_runner, is_shared: false } - let!(:group_runner) { create :ci_runner, groups: [group], runner_type: :group_type } + set(:group) { create(:group) } + set(:project) { create(:project, group: group, shared_runners_enabled: false, group_runners_enabled: false) } + set(:pipeline) { create(:ci_pipeline, project: project) } + let!(:shared_runner) { create(:ci_runner, is_shared: true) } + let!(:specific_runner) { create(:ci_runner, is_shared: false) } + let!(:group_runner) { create(:ci_runner, groups: [group], runner_type: :group_type) } + let!(:pending_job) { create(:ci_build, pipeline: pipeline) } before do specific_runner.assign_to(project) @@ -152,7 +152,7 @@ module Ci context 'disallow when builds are disabled' do before do - project.update(shared_runners_enabled: true) + project.update(shared_runners_enabled: true, group_runners_enabled: true) project.project_feature.update_attribute(:builds_access_level, ProjectFeature::DISABLED) end @@ -162,7 +162,13 @@ module Ci it { expect(build).to be_nil } end - context 'and uses specific runner' do + context 'and uses group runner' do + let(:build) { execute(group_runner) } + + it { expect(build).to be_nil } + end + + context 'and uses project runner' do let(:build) { execute(specific_runner) } it { expect(build).to be_nil } @@ -171,7 +177,7 @@ module Ci context 'allow group runners' do before do - project.update!(group_runners_enabled: true, group: group) + project.update!(group_runners_enabled: true) end context 'for multiple builds' do @@ -230,7 +236,7 @@ module Ci context 'disallow group runners' do before do - project.update(group_runners_enabled: false) + project.update!(group_runners_enabled: false) end context 'group runner' do -- GitLab From 0ae300578139c0e71e8748b6106f673e4b3d19c8 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 14:54:08 +0200 Subject: [PATCH 79/86] Order builds by id instead of created_at in RegisterJobService --- app/services/ci/register_job_service.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index a7d8ad93f38c..4291631913ae 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -85,7 +85,7 @@ def builds_for_shared_runner end def builds_for_project_runner - new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC') + new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('id ASC') end def builds_for_group_runner @@ -94,7 +94,7 @@ def builds_for_group_runner .with_group_runners_enabled .with_builds_enabled .without_deleted - new_builds.where(project: projects).order('created_at ASC') + new_builds.where(project: projects).order('id ASC') end def running_builds_for_shared_runners -- GitLab From 794ac6c5421e04056dfd336559786fb166c9fa0a Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 15:38:55 +0200 Subject: [PATCH 80/86] Revert "Use factory in specs for ProjectCiCdSettings" This reverts commit bf790c26c58e214c27132e7a54fdf4a4cc77bdaf. --- spec/factories/project_ci_cd_settings.rb | 10 ---------- spec/factories/projects.rb | 8 +++++++- 2 files changed, 7 insertions(+), 11 deletions(-) delete mode 100644 spec/factories/project_ci_cd_settings.rb diff --git a/spec/factories/project_ci_cd_settings.rb b/spec/factories/project_ci_cd_settings.rb deleted file mode 100644 index 2e85b54e2453..000000000000 --- a/spec/factories/project_ci_cd_settings.rb +++ /dev/null @@ -1,10 +0,0 @@ -FactoryBot.define do - factory :project_ci_cd_setting do - project - - initialize_with do - # ci_cd_settings are automatically created when a project is created - project&.ci_cd_settings || new - end - end -end diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index e0e72e7f2ce9..aed5eab80449 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -14,7 +14,6 @@ # Associations namespace creator { group ? create(:user) : namespace&.owner } - ci_cd_settings strategy: :build, factory: :project_ci_cd_setting, project: nil transient do # Nest Project Feature attributes @@ -24,6 +23,10 @@ issues_access_level ProjectFeature::ENABLED merge_requests_access_level ProjectFeature::ENABLED repository_access_level ProjectFeature::ENABLED + + # we can't assign the delegated `#ci_cd_settings` attributes directly, as the + # `#ci_cd_settings` relation needs to be created first + group_runners_enabled nil end after(:create) do |project, evaluator| @@ -48,6 +51,9 @@ end project.group&.refresh_members_authorized_projects + + # assign the delegated `#ci_cd_settings` attributes after create + project.reload.group_runners_enabled = evaluator.group_runners_enabled unless evaluator.group_runners_enabled.nil? end trait :public do -- GitLab From dd74237ddc8bef97e45757caf2e91f157e7dd7d2 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 17:08:06 +0200 Subject: [PATCH 81/86] Split migration to add and index namespaces.runners_token --- .../20170906133745_add_runners_token_to_groups.rb | 10 +--------- ...150427_add_index_to_namespaces_runners_token.rb | 14 ++++++++++++++ db/schema.rb | 2 +- 3 files changed, 16 insertions(+), 10 deletions(-) create mode 100644 db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb diff --git a/db/migrate/20170906133745_add_runners_token_to_groups.rb b/db/migrate/20170906133745_add_runners_token_to_groups.rb index 54d0fddd5e3a..852f4cba6705 100644 --- a/db/migrate/20170906133745_add_runners_token_to_groups.rb +++ b/db/migrate/20170906133745_add_runners_token_to_groups.rb @@ -3,15 +3,7 @@ class AddRunnersTokenToGroups < ActiveRecord::Migration DOWNTIME = false - disable_ddl_transaction! - - def up + def change add_column :namespaces, :runners_token, :string - - add_concurrent_index :namespaces, :runners_token, unique: true - end - - def down - remove_column :namespaces, :runners_token end end diff --git a/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb b/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb new file mode 100644 index 000000000000..d6c4a5c432cb --- /dev/null +++ b/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb @@ -0,0 +1,14 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddIndexToNamespacesRunnersToken < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def change + add_concurrent_index :namespaces, :runners_token, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index c88d6f3f9e92..177ad0bf23ef 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20180430143705) do +ActiveRecord::Schema.define(version: 20180503150427) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" -- GitLab From b8abe0c980d12a48dc7b25cb8f3d560b89b5dcd2 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 17:09:16 +0200 Subject: [PATCH 82/86] Remove unnecessary disable transaction in add_ci_runner_namespaces --- db/migrate/20170301101006_add_ci_runner_namespaces.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/db/migrate/20170301101006_add_ci_runner_namespaces.rb b/db/migrate/20170301101006_add_ci_runner_namespaces.rb index 7e7df2f4d222..deaf03e928ba 100644 --- a/db/migrate/20170301101006_add_ci_runner_namespaces.rb +++ b/db/migrate/20170301101006_add_ci_runner_namespaces.rb @@ -3,8 +3,6 @@ class AddCiRunnerNamespaces < ActiveRecord::Migration DOWNTIME = false - disable_ddl_transaction! - def change create_table :ci_runner_namespaces do |t| t.integer :runner_id -- GitLab From 15b10c344b1b909b2a90331926aa4082cd86045b Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 17:15:32 +0200 Subject: [PATCH 83/86] Dont remove duplicates in Runner.owned_or_shared since its not necessary --- app/models/ci/runner.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 2dfd038d5a8d..23078f1c3ed4 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -40,7 +40,10 @@ class Runner < ActiveRecord::Base } scope :owned_or_shared, -> (project_id) do - union = Gitlab::SQL::Union.new([belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), shared]) + union = Gitlab::SQL::Union.new( + [belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), shared], + remove_duplicates: false + ) from("(#{union.to_sql}) ci_runners") end -- GitLab From cb49d68c4fe565cc4fdc3a326c45e3ded548cd24 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 17:17:01 +0200 Subject: [PATCH 84/86] Use smallint for runner_type since its an enum --- db/migrate/20180430101916_add_runner_type_to_ci_runners.rb | 2 +- db/schema.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb b/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb index 8c8009f28fbf..42409349b755 100644 --- a/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb +++ b/db/migrate/20180430101916_add_runner_type_to_ci_runners.rb @@ -4,6 +4,6 @@ class AddRunnerTypeToCiRunners < ActiveRecord::Migration DOWNTIME = false def change - add_column :ci_runners, :runner_type, :integer + add_column :ci_runners, :runner_type, :smallint end end diff --git a/db/schema.rb b/db/schema.rb index 177ad0bf23ef..a37e6edc8d14 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -480,7 +480,7 @@ t.integer "access_level", default: 0, null: false t.string "ip_address" t.integer "maximum_timeout" - t.integer "runner_type" + t.integer "runner_type", limit: 2 end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree -- GitLab From ed9b285b0e1bbbfe88f685cdebd22227f37b97b9 Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Thu, 3 May 2018 18:02:38 +0200 Subject: [PATCH 85/86] Fix constants in backfill_runner_type_for_ci_runners_post_migrate.rb --- ...05_backfill_runner_type_for_ci_runners_post_migrate.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb index 9186a729a40b..38af5aae9244 100644 --- a/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb +++ b/db/post_migrate/20180430143705_backfill_runner_type_for_ci_runners_post_migrate.rb @@ -3,17 +3,17 @@ class BackfillRunnerTypeForCiRunnersPostMigrate < ActiveRecord::Migration DOWNTIME = false - PROJECT_RUNNER_TYPE = 1 - INSTANCE_RUNNER_TYPE = 3 + INSTANCE_RUNNER_TYPE = 1 + PROJECT_RUNNER_TYPE = 3 disable_ddl_transaction! def up - update_column_in_batches(:ci_runners, :runner_type, PROJECT_RUNNER_TYPE) do |table, query| + update_column_in_batches(:ci_runners, :runner_type, INSTANCE_RUNNER_TYPE) do |table, query| query.where(table[:is_shared].eq(true)).where(table[:runner_type].eq(nil)) end - update_column_in_batches(:ci_runners, :runner_type, INSTANCE_RUNNER_TYPE) do |table, query| + update_column_in_batches(:ci_runners, :runner_type, PROJECT_RUNNER_TYPE) do |table, query| query.where(table[:is_shared].eq(false)).where(table[:runner_type].eq(nil)) end end -- GitLab From 11f38dd12abedf17c09098170de295ce0b533d3b Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Fri, 4 May 2018 07:25:36 +0200 Subject: [PATCH 86/86] Make add_index_to_namespaces_runners_token migration reversible --- ...0180503150427_add_index_to_namespaces_runners_token.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb b/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb index d6c4a5c432cb..4c4e576d49f9 100644 --- a/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb +++ b/db/migrate/20180503150427_add_index_to_namespaces_runners_token.rb @@ -8,7 +8,13 @@ class AddIndexToNamespacesRunnersToken < ActiveRecord::Migration disable_ddl_transaction! - def change + def up add_concurrent_index :namespaces, :runners_token, unique: true end + + def down + if index_exists?(:namespaces, :runners_token, unique: true) + remove_index :namespaces, :runners_token + end + end end -- GitLab