diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb index fb3f6eec2bd..322ec096ffb 100644 --- a/app/controllers/projects/settings/ci_cd_controller.rb +++ b/app/controllers/projects/settings/ci_cd_controller.rb @@ -74,7 +74,7 @@ module Projects .ordered .page(params[:page]).per(20) - @shared_runners = ::Ci::Runner.shared.active + @shared_runners = ::Ci::Runner.instance_type.active @shared_runners_count = @shared_runners.count(:all) diff --git a/app/helpers/ci_status_helper.rb b/app/helpers/ci_status_helper.rb index 5fce97164ae..f49b5c7b51a 100644 --- a/app/helpers/ci_status_helper.rb +++ b/app/helpers/ci_status_helper.rb @@ -122,7 +122,7 @@ module CiStatusHelper def no_runners_for_project?(project) project.runners.blank? && - Ci::Runner.shared.blank? + Ci::Runner.instance_type.blank? end def render_status_with_link(type, status, path = nil, tooltip_placement: 'left', cssclass: '', container: 'body') diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8c9aacca8de..bcd0c206bca 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -2,6 +2,7 @@ module Ci class Runner < ActiveRecord::Base extend Gitlab::Ci::Model include Gitlab::SQL::Pattern + include IgnorableColumn include RedisCacheable include ChronicDurationAttribute @@ -11,6 +12,8 @@ module Ci AVAILABLE_SCOPES = %w[specific shared active paused online].freeze FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze + ignore_column :is_shared + has_many :builds has_many :runner_projects, inverse_of: :runner, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :projects, through: :runner_projects @@ -21,13 +24,16 @@ module Ci 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) } + # BACKWARD COMPATIBILITY: There are needed to maintain compatibility with `AVAILABLE_SCOPES` used by `lib/api/runners.rb` + scope :deprecated_shared, -> { instance_type } + # this should get replaced with `project_type.or(group_type)` once using Rails5 + scope :deprecated_specific, -> { where(runner_type: [runner_types[:project_type], runner_types[:group_type]]) } + scope :belonging_to_project, -> (project_id) { joins(:runner_projects).where(ci_runner_projects: { project_id: project_id }) } @@ -39,9 +45,9 @@ module Ci joins(:groups).where(namespaces: { id: hierarchy_groups }) } - scope :owned_or_shared, -> (project_id) do + scope :owned_or_instance_wide, -> (project_id) do union = Gitlab::SQL::Union.new( - [belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), shared], + [belonging_to_project(project_id), belonging_to_parent_group_of_project(project_id), instance_type], remove_duplicates: false ) from("(#{union.to_sql}) ci_runners") @@ -63,7 +69,6 @@ module Ci validate :no_groups, unless: :group_type? validate :any_project, if: :project_type? validate :exactly_one_group, if: :group_type? - validate :validate_is_shared acts_as_taggable @@ -113,8 +118,7 @@ module Ci end def assign_to(project, current_user = nil) - if shared? - self.is_shared = false if shared? + if instance_type? self.runner_type = :project_type elsif group_type? raise ArgumentError, 'Transitioning a group runner to a project runner is not supported' @@ -137,10 +141,6 @@ module Ci description end - def shared? - is_shared - end - def online? contacted_at && contacted_at > self.class.contact_time_deadline end @@ -159,10 +159,6 @@ module Ci runner_projects.count == 1 end - def specific? - !shared? - end - def assigned_to_group? runner_namespaces.any? end @@ -260,7 +256,7 @@ module Ci end def assignable_for?(project_id) - self.class.owned_or_shared(project_id).where(id: self.id).any? + self.class.owned_or_instance_wide(project_id).where(id: self.id).any? end def no_projects @@ -287,12 +283,6 @@ module Ci end end - def validate_is_shared - unless is_shared? == instance_type? - errors.add(:is_shared, 'is not equal to instance_type?') - end - end - def accepting_tags?(build) (run_untagged? || build.has_tags?) && (build.tag_list - tag_list).empty? end diff --git a/app/models/project.rb b/app/models/project.rb index d91d7dcfe9a..7304ab7b909 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1422,7 +1422,7 @@ class Project < ActiveRecord::Base end def shared_runners - @shared_runners ||= shared_runners_available? ? Ci::Runner.shared : Ci::Runner.none + @shared_runners ||= shared_runners_available? ? Ci::Runner.instance_type : Ci::Runner.none end def group_runners diff --git a/app/models/user.rb b/app/models/user.rb index 48629c58490..27a5d0278b7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1032,7 +1032,7 @@ class User < ActiveRecord::Base 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.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end end diff --git a/app/serializers/runner_entity.rb b/app/serializers/runner_entity.rb index e9999a36d8a..db26eadab2d 100644 --- a/app/serializers/runner_entity.rb +++ b/app/serializers/runner_entity.rb @@ -4,7 +4,7 @@ class RunnerEntity < Grape::Entity expose :id, :description expose :edit_path, - if: -> (*) { can?(request.current_user, :admin_build, project) && runner.specific? } do |runner| + if: -> (*) { can?(request.current_user, :admin_build, project) && runner.project_type? } do |runner| edit_project_runner_path(project, runner) end diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 9bdbb2c0d99..c0dce45e2e7 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -15,7 +15,7 @@ module Ci def execute builds = - if runner.shared? + if runner.instance_type? builds_for_shared_runner elsif runner.group_type? builds_for_group_runner @@ -99,7 +99,7 @@ module Ci end def running_builds_for_shared_runners - Ci::Build.running.where(runner: Ci::Runner.shared) + Ci::Build.running.where(runner: Ci::Runner.instance_type) .group(:project_id).select(:project_id, 'count(*) AS running_builds') end @@ -115,7 +115,7 @@ module Ci end def register_success(job) - labels = { shared_runner: runner.shared?, + labels = { shared_runner: runner.instance_type?, jobs_running_for_project: jobs_running_for_project(job) } job_queue_duration_seconds.observe(labels, Time.now - job.queued_at) unless job.queued_at.nil? @@ -123,10 +123,10 @@ module Ci end def jobs_running_for_project(job) - return '+Inf' unless runner.shared? + return '+Inf' unless runner.instance_type? # excluding currently started job - running_jobs_count = job.project.builds.running.where(runner: Ci::Runner.shared) + running_jobs_count = job.project.builds.running.where(runner: Ci::Runner.instance_type) .limit(JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET + 1).count - 1 running_jobs_count < JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET ? running_jobs_count : "#{JOBS_RUNNING_FOR_PROJECT_MAX_BUCKET}+" end diff --git a/app/views/admin/runners/_runner.html.haml b/app/views/admin/runners/_runner.html.haml index a6cd39edcf0..43937b01339 100644 --- a/app/views/admin/runners/_runner.html.haml +++ b/app/views/admin/runners/_runner.html.haml @@ -1,6 +1,6 @@ %tr{ id: dom_id(runner) } %td - - if runner.shared? + - if runner.instance_type? %span.badge.badge-success shared - elsif runner.group_type? %span.badge.badge-success group @@ -21,7 +21,7 @@ %td = runner.ip_address %td - - if runner.shared? || runner.group_type? + - if runner.instance_type? || 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 8a0c2bf4c5f..62b7a4cbd07 100644 --- a/app/views/admin/runners/show.html.haml +++ b/app/views/admin/runners/show.html.haml @@ -2,7 +2,7 @@ %h3.project-title Runner ##{@runner.id} .float-right - - if @runner.shared? + - if @runner.instance_type? %span.runner-state.runner-state-shared Shared - else @@ -13,7 +13,7 @@ - breadcrumb_title "##{@runner.id}" - @no_container = true -- if @runner.shared? +- if @runner.instance_type? .bs-callout.bs-callout-success %h4 This Runner will process jobs from ALL UNASSIGNED projects %p diff --git a/app/views/projects/runners/_runner.html.haml b/app/views/projects/runners/_runner.html.haml index a23f5d6f0c3..6ee83fae25e 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?) # We can simplify this to `runner.project_type?` when migrating #runner_type is complete + - elsif runner.project_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' diff --git a/app/views/shared/runners/show.html.haml b/app/views/shared/runners/show.html.haml index 96527fcb4f2..362569bfbaf 100644 --- a/app/views/shared/runners/show.html.haml +++ b/app/views/shared/runners/show.html.haml @@ -3,7 +3,7 @@ %h3.page-title Runner ##{@runner.id} .float-right - - if @runner.shared? + - if @runner.instance_type? %span.runner-state.runner-state-shared Shared - elsif @runner.group_type? diff --git a/changelogs/unreleased/remove-is-shared-from-ci-runners.yml b/changelogs/unreleased/remove-is-shared-from-ci-runners.yml new file mode 100644 index 00000000000..a6917431a53 --- /dev/null +++ b/changelogs/unreleased/remove-is-shared-from-ci-runners.yml @@ -0,0 +1,5 @@ +--- +title: Remove the use of `is_shared` of `Ci::Runner` +merge_request: +author: +type: other diff --git a/lib/api/entities.rb b/lib/api/entities.rb index bb48a86fe9e..d04f92885a3 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1010,7 +1010,7 @@ module API expose :description expose :ip_address expose :active - expose :is_shared + expose :instance_type?, as: :is_shared expose :name expose :online?, as: :online expose :status @@ -1024,7 +1024,7 @@ module API expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at - expose :token, if: lambda { |runner, options| options[:current_user].admin? || !runner.is_shared? } + expose :token, if: lambda { |runner, options| options[:current_user].admin? || !runner.instance_type? } expose :projects, with: Entities::BasicProjectDetails do |runner, options| if options[:current_user].admin? runner.projects diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 96a02914faa..b4b984f7b8f 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -24,13 +24,13 @@ module API attributes = if runner_registration_token_valid? # Create shared runner. Requires admin access - attributes.merge(is_shared: true, runner_type: :instance_type) + attributes.merge(runner_type: :instance_type) elsif project = Project.find_by(runners_token: params[:token]) # Create a specific runner for the project - attributes.merge(is_shared: false, runner_type: :project_type, projects: [project]) + attributes.merge(runner_type: :project_type, projects: [project]) elsif group = Group.find_by(runners_token: params[:token]) # Create a specific runner for the group - attributes.merge(is_shared: false, runner_type: :group_type, groups: [group]) + attributes.merge(runner_type: :group_type, groups: [group]) else forbidden! end diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 2b78075ddbf..2071c5a62c1 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -119,7 +119,7 @@ module API use :pagination end get ':id/runners' do - runners = filter_runners(Ci::Runner.owned_or_shared(user_project.id), params[:scope]) + runners = filter_runners(Ci::Runner.owned_or_instance_wide(user_project.id), params[:scope]) present paginate(runners), with: Entities::Runner end @@ -170,6 +170,11 @@ module API render_api_error!('Scope contains invalid value', 400) end + # Support deprecated scopes + if runners.respond_to?("deprecated_#{scope}") + scope = "deprecated_#{scope}" + end + runners.public_send(scope) # rubocop:disable GitlabSecurity/PublicSend end @@ -180,7 +185,7 @@ module API end def authenticate_show_runner!(runner) - return if runner.is_shared || current_user.admin? + return if runner.instance_type? || current_user.admin? forbidden!("No access granted") unless can?(current_user, :read_runner, runner) end diff --git a/lib/gitlab/data_builder/pipeline.rb b/lib/gitlab/data_builder/pipeline.rb index 1e283cc092b..eb246d393a1 100644 --- a/lib/gitlab/data_builder/pipeline.rb +++ b/lib/gitlab/data_builder/pipeline.rb @@ -55,7 +55,7 @@ module Gitlab id: runner.id, description: runner.description, active: runner.active?, - is_shared: runner.is_shared? + is_shared: runner.instance_type? } end end diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index 6fb621b5e51..347e4f433e2 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -6,7 +6,6 @@ FactoryBot.define do active true access_level :not_protected - is_shared true runner_type :instance_type trait :online do @@ -14,12 +13,10 @@ FactoryBot.define do end trait :instance do - is_shared true runner_type :instance_type end trait :group do - is_shared false runner_type :group_type after(:build) do |runner, evaluator| @@ -28,7 +25,6 @@ FactoryBot.define do end trait :project do - is_shared false runner_type :project_type after(:build) do |runner, evaluator| diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index f6433234573..953af2c4710 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -105,7 +105,7 @@ describe Ci::Runner do end end - describe '.shared' do + describe '.instance_type' do let(:group) { create(:group) } let(:project) { create(:project) } let!(:group_runner) { create(:ci_runner, :group, groups: [group]) } @@ -113,7 +113,7 @@ describe Ci::Runner do let!(:shared_runner) { create(:ci_runner, :instance) } it 'returns only shared runners' do - expect(described_class.shared).to contain_exactly(shared_runner) + expect(described_class.instance_type).to contain_exactly(shared_runner) end end @@ -155,7 +155,7 @@ describe Ci::Runner do end end - describe '.owned_or_shared' do + describe '.owned_or_instance_wide' do it 'returns a globally shared, a project specific and a group specific runner' do # group specific group = create(:group) @@ -168,7 +168,7 @@ describe Ci::Runner do # globally shared shared_runner = create(:ci_runner, :instance) - expect(described_class.owned_or_shared(project.id)).to contain_exactly( + expect(described_class.owned_or_instance_wide(project.id)).to contain_exactly( group_runner, project_runner, shared_runner ) end @@ -202,7 +202,6 @@ describe Ci::Runner do it 'transitions shared runner to project runner and assigns project' do expect(subject).to be_truthy - expect(runner).to be_specific expect(runner).to be_project_type expect(runner.projects).to eq([project]) expect(runner.only_for?(project)).to be_truthy diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index 0c7937feed6..b5e4b6011ea 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -89,6 +89,17 @@ describe API::Runners do end end + it 'filters runners by scope' do + get api('/runners/all?scope=shared', admin) + + shared = json_response.all? { |r| r['is_shared'] } + expect(response).to have_gitlab_http_status(200) + expect(response).to include_pagination_headers + expect(json_response).to be_an Array + expect(json_response[0]).to have_key('ip_address') + expect(shared).to be_truthy + end + it 'filters runners by scope' do get api('/runners/all?scope=specific', admin) @@ -136,7 +147,7 @@ describe API::Runners do 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.to change { Ci::Runner.project_type.count }.by(-1) end end @@ -300,7 +311,7 @@ describe API::Runners do delete api("/runners/#{shared_runner.id}", admin) expect(response).to have_gitlab_http_status(204) - end.to change { Ci::Runner.shared.count }.by(-1) + end.to change { Ci::Runner.instance_type.count }.by(-1) end it_behaves_like '412 response' do @@ -314,7 +325,7 @@ describe API::Runners 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.to change { Ci::Runner.project_type.count }.by(-1) end end @@ -349,7 +360,7 @@ describe API::Runners do delete api("/runners/#{project_runner.id}", user) expect(response).to have_http_status(204) - end.to change { Ci::Runner.specific.count }.by(-1) + end.to change { Ci::Runner.project_type.count }.by(-1) end it_behaves_like '412 response' do @@ -584,12 +595,12 @@ describe API::Runners do end end - it 'enables a shared runner' do + it 'enables a instance type runner' do expect do post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id end.to change { project.runners.count }.by(1) - expect(shared_runner.reload).not_to be_shared + expect(shared_runner.reload).not_to be_instance_type expect(response).to have_gitlab_http_status(201) end end