From a88833574fbe77e06922cef3e7940c280268f96b Mon Sep 17 00:00:00 2001 From: Dylan Griffith Date: Mon, 30 Apr 2018 09:48:20 +0400 Subject: [PATCH] Apply changes from feature/runner-per-group and fix EE conflicts --- .../projects/runners_controller.rb | 6 + .../projects/settings/ci_cd_controller.rb | 13 +- app/models/ci/runner.rb | 63 ++++- app/models/ci/runner_group.rb | 8 + app/models/group.rb | 12 + app/models/project.rb | 15 +- app/models/user.rb | 16 +- app/services/ci/register_job_service.rb | 24 +- app/services/ci/update_build_queue_service.rb | 22 +- app/views/admin/runners/_runner.html.haml | 4 +- app/views/admin/runners/index.html.haml | 3 + app/views/admin/runners/show.html.haml | 3 + .../projects/runners/_group_runners.html.haml | 32 +++ app/views/projects/runners/_index.html.haml | 4 + app/views/projects/runners/_runner.html.haml | 2 +- .../unreleased/feature-runner-per-group.yml | 5 + config/routes/project.rb | 1 + .../20170301101006_add_ci_runner_groups.rb | 23 ++ ...70906133745_add_runners_token_to_groups.rb | 17 ++ db/schema.rb | 12 + lib/api/entities.rb | 17 +- lib/api/runner.rb | 5 +- lib/api/runners.rb | 1 + .../settings/ci_cd_controller_spec.rb | 11 + spec/factories/projects.rb | 9 +- spec/features/admin/admin_runners_spec.rb | 41 +++ spec/features/runners_spec.rb | 80 ++++++ spec/lib/gitlab/import_export/all_models.yml | 2 +- spec/models/ci/runner_spec.rb | 240 +++++++++++++++++- spec/models/project_spec.rb | 115 +++++++-- spec/models/user_spec.rb | 66 ++++- spec/requests/api/runner_spec.rb | 15 +- spec/requests/api/runners_spec.rb | 218 +++++++++++----- spec/serializers/pipeline_serializer_spec.rb | 2 +- spec/services/ci/register_job_service_spec.rb | 89 ++++++- .../ci/update_build_queue_service_spec.rb | 62 ++++- 36 files changed, 1086 insertions(+), 172 deletions(-) create mode 100644 app/models/ci/runner_group.rb create mode 100644 app/views/projects/runners/_group_runners.html.haml create mode 100644 changelogs/unreleased/feature-runner-per-group.yml create mode 100644 db/migrate/20170301101006_add_ci_runner_groups.rb create mode 100644 db/migrate/20170906133745_add_runners_token_to_groups.rb diff --git a/app/controllers/projects/runners_controller.rb b/app/controllers/projects/runners_controller.rb index c950d0f7001b8b..b9bbe7115c4d28 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_ci_cd_settings!(:group_runners_enabled) + + redirect_to project_settings_ci_cd_path(@project) + end + protected def set_runner diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index d80ef8113aa96f..6ddbefacae0419 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -67,10 +67,19 @@ 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) + + @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 05aeb6b20e9102..f1bc8462cfddb6 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -15,31 +15,48 @@ 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' 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 :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 - 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) + 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_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 + + joins(:groups).where(namespaces: { id: hierarchy_groups }) + } + + 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]) + from("(#{union.to_sql}) ci_runners") end scope :assignable_for, ->(project) do # 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 + validate :either_projects_or_group validates :access_level, presence: true acts_as_taggable @@ -121,6 +138,14 @@ def specific? !shared? end + def assigned_to_group? + runner_groups.any? + end + + def assigned_to_project? + runner_projects.any? + end + def can_pick?(build) return false if self.ref_protected? && !build.protected? @@ -175,6 +200,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 @@ -206,7 +237,17 @@ 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 either_projects_or_group + if groups.many? + errors.add(:runner, 'can only be assigned to one group') + end + + if assigned_to_group? && assigned_to_project? + errors.add(:runner, 'can only be assigned either to projects or to a group') + end end def accepting_tags?(build) diff --git a/app/models/ci/runner_group.rb b/app/models/ci/runner_group.rb new file mode 100644 index 00000000000000..87f3ba13bff021 --- /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/app/models/group.rb b/app/models/group.rb index 786098f8e45f8d..9bbea3e75c2210 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -12,6 +12,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 @@ -31,6 +32,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 :ldap_group_links, foreign_key: 'group_id', dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :hooks, dependent: :destroy, class_name: 'GroupHook' # rubocop:disable Cop/ActiveRecordDependent @@ -55,6 +58,8 @@ class Group < Namespace validates :repository_size_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0, allow_nil: true } + add_authentication_token_field :runners_token + after_create :post_create_hook after_destroy :post_destroy_hook after_save :update_two_factor_requirement @@ -338,6 +343,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 diff --git a/app/models/project.rb b/app/models/project.rb index 14911a3157bff0..e73e739664ecda 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -237,8 +237,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' @@ -254,6 +252,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: :ci_cd_settings # Validations validates :creator, presence: true, on: :create @@ -1315,12 +1314,14 @@ def shared_runners @shared_runners ||= shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none end - def active_shared_runners - @active_shared_runners ||= shared_runners.active + def group_runners + @group_runners ||= group_runners_enabled? ? Ci::Runner.belonging_to_parent_group_of_project(self.id) : Ci::Runner.none end def any_runners?(&block) - active_runners.any?(&block) || active_shared_runners.any?(&block) + union = Gitlab::SQL::Union.new([runners, shared_runners, group_runners]) + runners = Ci::Runner.from("(#{union.to_sql}) ci_runners").active + runners.any?(&block) end def valid_runners_token?(token) @@ -1884,6 +1885,10 @@ def licensed_features [] end + def toggle_ci_cd_settings!(settings_attribute) + ci_cd_settings.toggle!(settings_attribute) + end + def gitlab_deploy_token @gitlab_deploy_token ||= deploy_tokens.gitlab_deploy_token end diff --git a/app/models/user.rb b/app/models/user.rb index f546523fa836fb..2f2320f861d90a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1017,10 +1017,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 @@ -1209,6 +1216,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/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index c95310181e7c0c..cbdff2f7b1e10c 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -19,8 +19,10 @@ def execute builds = if runner.shared? builds_for_shared_runner + elsif runner.assigned_to_group? + builds_for_group_runner else - builds_for_specific_runner + builds_for_project_runner end valid = true @@ -77,17 +79,23 @@ def builds_for_shared_runner .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") + # 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 - def builds_for_specific_runner + def builds_for_project_runner new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('created_at ASC') end + 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') @@ -99,10 +107,6 @@ def new_builds builds end - def shared_runner_build_limits_feature_enabled? - ENV['DISABLE_SHARED_RUNNER_BUILD_MINUTES_LIMIT'].to_s != 'true' - end - def register_failure failed_attempt_counter.increment attempt_counter.increment diff --git a/app/services/ci/update_build_queue_service.rb b/app/services/ci/update_build_queue_service.rb index 152c8ae5006cd0..c991f04ab30a6c 100644 --- a/app/services/ci/update_build_queue_service.rb +++ b/app/services/ci/update_build_queue_service.rb @@ -1,18 +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 + 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 + end - return unless build.project.shared_runners_enabled? + private - Ci::Runner.shared.each do |runner| - if runner.can_pick?(build) - runner.tick_runner_queue - end + def tick_for(build, runners) + runners.each do |runner| + runner.invalidate_build_cache!(build) end end end diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index e1cee58492906c..6670ba6aa89f77 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.assigned_to_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.assigned_to_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 9f13dbbbd82f67..1a3b5e58ed502c 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/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml index 37269862de60ec..ab2c9ad1e570b2 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.assigned_to_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 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 00000000000000..a9dfd9cc786bac --- /dev/null +++ b/app/views/projects/runners/_group_runners.html.haml @@ -0,0 +1,32 @@ +%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 + %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. + +- elsif @group_runners.empty? + This group does not provide any group Runners yet. + + - 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} + %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 f9808f7c990f6b..3f5119d408b50e 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 6376496ee1aa8d..d2598f3be072d6 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.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/changelogs/unreleased/feature-runner-per-group.yml b/changelogs/unreleased/feature-runner-per-group.yml new file mode 100644 index 00000000000000..162a5fae0a4404 --- /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 diff --git a/config/routes/project.rb b/config/routes/project.rb index ac8097c04717d5..938e8dc6990e7a 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -453,6 +453,7 @@ collection do post :toggle_shared_runners + post :toggle_group_runners 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 00000000000000..1c4430981a9521 --- /dev/null +++ b/db/migrate/20170301101006_add_ci_runner_groups.rb @@ -0,0 +1,23 @@ +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 + 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 + end +end 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 00000000000000..54d0fddd5e3a26 --- /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 68b94d6e8f64b1..5a19eaf45f94e3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -540,6 +540,14 @@ 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" + 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 + create_table "ci_runner_projects", force: :cascade do |t| t.integer "runner_id", null: false t.datetime "created_at" @@ -1679,6 +1687,7 @@ t.integer "cached_markdown_version" t.integer "plan_id" t.integer "project_creation_level" + t.string "runners_token" end add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree @@ -1692,6 +1701,7 @@ add_index "namespaces", ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"} add_index "namespaces", ["plan_id"], name: "index_namespaces_on_plan_id", using: :btree 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| @@ -2695,6 +2705,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_sources_pipelines", "ci_builds", column: "source_job_id", name: "fk_be5624bf37", on_delete: :cascade add_foreign_key "ci_sources_pipelines", "ci_pipelines", column: "pipeline_id", name: "fk_e1bad85861", on_delete: :cascade diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 387e722f4cc806..eb65fe11f1180a 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -242,14 +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 @@ -994,6 +998,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 4d4fbe50f9f956..49d9b0b1b4fd5d 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 5f2a95676051e4..11c31917fc5a92 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -205,6 +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.assigned_to_group? return if current_user.admin? forbidden!("No access granted") unless user_can_access_runner?(runner) diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb index 7dae9b85d78435..1cf395b03284f6 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/factories/projects.rb b/spec/factories/projects.rb index 79238a2c45e5e9..601fbf4ab27487 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 `#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| @@ -47,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 diff --git a/spec/features/admin/admin_runners_spec.rb b/spec/features/admin/admin_runners_spec.rb index 8de2e3d199b987..b0aa2e8b588060 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 diff --git a/spec/features/runners_spec.rb b/spec/features/runners_spec.rb index df65c2d2f834c4..b396e1033458f7 100644 --- a/spec/features/runners_spec.rb +++ b/spec/features/runners_spec.rb @@ -181,4 +181,84 @@ 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 + + given(:group) { create :group } + + 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 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 '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) + + 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.' + + 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 + + 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 + + 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 end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index d8503d2a10178d..9e2833b2253cc6 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -281,7 +281,6 @@ project: - builds - runner_projects - runners -- active_runners - variables - triggers - pipeline_schedules @@ -325,6 +324,7 @@ project: - internal_ids - project_deploy_tokens - deploy_tokens +- settings - ci_cd_settings award_emoji: - awardable diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ab170e6351c247..fb9dcce9a7c74e 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 @@ -49,6 +106,95 @@ end end + 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 + specific_runner = create :ci_runner, :specific, projects: [specific_project] + + # other + 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 + 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 + 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.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 + 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_parent_group_of_project(project.id)).to eq [runner] + 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] + + # 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 + 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') @@ -171,6 +317,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 @@ -212,7 +365,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 @@ -225,7 +378,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 @@ -235,7 +388,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 @@ -260,6 +413,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 @@ -583,4 +747,74 @@ def expect_redis_update expect(described_class.search(runner.description.upcase)).to eq([runner]) 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] + + 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.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.assigned_to_group?).to be true + 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] + + 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.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.assigned_to_project?).to be true + 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 diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 24fcda35bf37ce..f3a2086a2407b1 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -64,7 +64,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) } @@ -1273,44 +1272,94 @@ 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 'checks the presence of specific runner' do + project.runners << specific_runner + expect(project.any_runners? { |runner| runner == specific_runner }).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 + it 'returns false if match cannot be found' do + project.runners << specific_runner + expect(project.any_runners? { false }).to be_falsey + 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 + + it 'returns false if match cannot be found' do + shared_runner + expect(project.any_runners? { false }).to be_falsey + 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 + + it 'returns false if match cannot be found' do + group_runner + expect(project.any_runners? { false }).to be_falsey + end end end end @@ -3931,6 +3980,18 @@ def domain_variable end end + 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_ci_cd_settings!(:group_runners_enabled) + + expect(project.group_runners_enabled).to be true + end + end + describe '#gitlab_deploy_token' do let(:project) { create(:project) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e2c1f4ac95510e..59fe6695cc5269 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1836,14 +1836,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 @@ -1852,10 +1850,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 @@ -1866,7 +1892,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 @@ -1883,7 +1909,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) @@ -1893,7 +1933,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) @@ -1906,7 +1946,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) diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index a822ca015bdc3f..d8416e614e3813 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 d30f0cf36e2a04..89e21ba9914a96 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -8,22 +8,27 @@ 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]) } + before do # Set project access for users create(:project_member, :master, user: user, project: project) @@ -37,9 +42,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' + ) expect(shared).to be_falsey end @@ -129,10 +138,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 +171,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 +188,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 +260,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 +295,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(specific_runner.description).to eq('test') - expect(specific_runner.description).not_to eq(description) + 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 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 +363,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/#{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/#{specific_runner.id}", admin) + 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 +403,46 @@ 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 '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 +453,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 +472,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 +484,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 +497,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 +525,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 +537,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 +550,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 +568,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 +576,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 +615,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 +623,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) @@ -559,10 +651,16 @@ 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 - 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 +668,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 +717,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 +732,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 +740,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/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index d1948dc7e1cc6e..946228b96cd50c 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(2).of(40) + expect(recorded.count).to be_within(2).of(48) expect(recorded.cached_count).to eq(0) end end diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index 8a537e83d5f2ad..7d3c43eeaf7a12 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,77 @@ 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 } + + # 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_group_runner).count).to eq 6 + execute(group_runner) + + 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_group_runner).count).to eq 4 + execute(group_runner) + + 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_group_runner).count).to eq 2 + execute(group_runner) + + 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_group_runner).count).to eq 0 + expect(execute(group_runner)).to be_nil + 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) @@ -178,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_specific_runner) + allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) .and_return(Ci::Build.where(id: [pending_job, other_build])) end @@ -190,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_specific_runner) + allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) .and_return(Ci::Build.where(id: pending_job)) end @@ -201,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_specific_runner) + allow_any_instance_of(Ci::RegisterJobService).to receive(:builds_for_project_runner) .and_return(Ci::Build.none) end diff --git a/spec/services/ci/update_build_queue_service_spec.rb b/spec/services/ci/update_build_queue_service_spec.rb index 0da0e57dbcdbde..74a23ed2a3f2b2 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