From 274a08c9bb35df477caf5df034588181d82b9018 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 30 Oct 2024 20:27:10 +0100 Subject: [PATCH 1/3] Use sharding_key_id to replace owner_project implementation --- .../ci/runner_owner_project_resolver.rb | 25 ++++--------- app/models/ci/runner.rb | 17 +++++++-- .../ci/runners/assign_runner_service.rb | 2 +- .../set_runner_associated_projects_service.rb | 6 +-- .../ci/runners/update_runner_service.rb | 4 +- .../set_runner_associated_projects_service.rb | 2 +- ...runner_associated_projects_service_spec.rb | 2 +- spec/models/ci/runner_spec.rb | 37 ++++++++++++++++++- ...runner_associated_projects_service_spec.rb | 14 +++---- 9 files changed, 70 insertions(+), 39 deletions(-) diff --git a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb index 28c394278721b4..ff44c59c0407ec 100644 --- a/app/graphql/resolvers/ci/runner_owner_project_resolver.rb +++ b/app/graphql/resolvers/ci/runner_owner_project_resolver.rb @@ -35,30 +35,21 @@ def resolve_owner return unless runner.project_type? BatchLoader::GraphQL.for(runner.id).batch do |runner_ids, loader| - # rubocop: disable CodeReuse/ActiveRecord - runner_and_projects_with_row_number = - ::Ci::RunnerProject - .where(runner_id: runner_ids) - .select('id, runner_id, project_id, ROW_NUMBER() OVER (PARTITION BY runner_id ORDER BY id ASC)') - runner_and_owner_projects = - ::Ci::RunnerProject - .select(:id, :runner_id, :project_id) - .from("(#{runner_and_projects_with_row_number.to_sql}) temp WHERE row_number = 1") - owner_project_id_by_runner_id = - runner_and_owner_projects - .group_by(&:runner_id) - .transform_values { |runner_projects| runner_projects.first.project_id } - project_ids = owner_project_id_by_runner_id.values.uniq + # rubocop: disable CodeReuse/ActiveRecord -- this runs on a limited number of records + runner_id_to_owner_id = + ::Ci::Runner.project_type.id_in(runner_ids) + .pluck(:id, :sharding_key_id) + .to_h + # rubocop: enable CodeReuse/ActiveRecord - projects = apply_lookahead(Project.id_in(project_ids)) + projects = apply_lookahead(Project.id_in(runner_id_to_owner_id.values)) Preloaders::ProjectPolicyPreloader.new(projects, current_user).execute projects_by_id = projects.index_by(&:id) runner_ids.each do |runner_id| - owner_project_id = owner_project_id_by_runner_id[runner_id] + owner_project_id = runner_id_to_owner_id[runner_id] loader.call(runner_id, projects_by_id[owner_project_id]) end - # rubocop: enable CodeReuse/ActiveRecord end end end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 6763afdb3a15c7..8d11be181a2834 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -384,14 +384,23 @@ def deprecated_rest_status end end - def owner_project - return unless project_type? + def owner + case runner_type + when 'instance_type' + ::User.find_by_id(creator_id) + when 'group_type' + ::Group.find_by_id(sharding_key_id) + when 'project_type' + ::Project.find_by_id(sharding_key_id) + end + end - runner_projects.order(:id).first&.project + def owner_project + owner if project_type? end def belongs_to_one_project? - runner_projects.count == 1 + runner_projects.limit(2).count(:all) == 1 end def belongs_to_more_than_one_project? diff --git a/app/services/ci/runners/assign_runner_service.rb b/app/services/ci/runners/assign_runner_service.rb index b815e1048bcd8e..99bd609f30052f 100644 --- a/app/services/ci/runners/assign_runner_service.rb +++ b/app/services/ci/runners/assign_runner_service.rb @@ -45,7 +45,7 @@ def validate reason: :not_authorized_to_add_runner_in_project) end - if runner.owner_project && project.organization_id != runner.owner_project.organization_id + if runner.owner && project.organization_id != runner.owner.organization_id return ServiceResponse.error(message: _('runner can only be assigned to projects in the same organization'), reason: :project_not_in_same_organization) end diff --git a/app/services/ci/runners/set_runner_associated_projects_service.rb b/app/services/ci/runners/set_runner_associated_projects_service.rb index cc06669a56036f..9c18f672c24f97 100644 --- a/app/services/ci/runners/set_runner_associated_projects_service.rb +++ b/app/services/ci/runners/set_runner_associated_projects_service.rb @@ -26,13 +26,11 @@ def execute private def set_associated_projects - new_project_ids = [runner.owner_project&.id].compact + project_ids + new_project_ids = [runner.owner&.id].compact + project_ids response = ServiceResponse.success runner.transaction do - # rubocop:disable CodeReuse/ActiveRecord - current_project_ids = runner.projects.ids - # rubocop:enable CodeReuse/ActiveRecord + current_project_ids = runner.project_ids # rubocop:disable CodeReuse/ActiveRecord -- reasonable use response = associate_new_projects(new_project_ids, current_project_ids) response = disassociate_old_projects(new_project_ids, current_project_ids) if response.success? diff --git a/app/services/ci/runners/update_runner_service.rb b/app/services/ci/runners/update_runner_service.rb index b01d06a3d61ccb..b304f17ab08589 100644 --- a/app/services/ci/runners/update_runner_service.rb +++ b/app/services/ci/runners/update_runner_service.rb @@ -33,9 +33,9 @@ def track_runner_event(params) kwargs = { user: current_user } case runner.runner_type when 'group_type' - kwargs[:namespace] = runner.groups.first + kwargs[:namespace] = runner.owner when 'project_type' - kwargs[:project] = runner.owner_project + kwargs[:project] = runner.owner end track_internal_event( diff --git a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb index 9a6e0890c388f0..63fd7a2398a10e 100644 --- a/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb +++ b/ee/app/services/ee/ci/runners/set_runner_associated_projects_service.rb @@ -36,7 +36,7 @@ def audit_event_service(result) def runner_path url_helpers = ::Gitlab::Routing.url_helpers - runner.owner_project ? url_helpers.project_runner_path(runner.owner_project, runner) : nil + runner.owner ? url_helpers.project_runner_path(runner.owner, runner) : nil end end end diff --git a/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index fa796c32fca9c7..4d4137156d953c 100644 --- a/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/ee/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -34,7 +34,7 @@ it 'calls audit on Auditor and returns success response', :aggregate_failures do expect(project_runner).to receive(:assign_to).with(new_project, user).once.and_return(true) expected_runner_url = ::Gitlab::Routing.url_helpers.project_runner_path( - project_runner.owner_project, + project_runner.owner, project_runner) expect(::Gitlab::Audit::Auditor).to receive(:audit).with( a_hash_including( diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index ba07ac91ad2ad7..4866e6e589d97b 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -277,6 +277,23 @@ end end + describe '#owner' do + subject(:owner) { runner.owner } + + context 'when runner does not have creator_id' do + let_it_be(:runner) { create(:ci_runner, :instance) } + + it { is_expected.to be_nil } + end + + context 'when runner has creator' do + let_it_be(:creator) { create(:user) } + let_it_be(:runner) { create(:ci_runner, :instance, creator: creator) } + + it { is_expected.to eq creator } + end + end + describe '.instance_type' do let!(:group_runner) { create(:ci_runner, :group, groups: [group]) } let!(:project_runner) { create(:ci_runner, :project, projects: [project]) } @@ -1125,8 +1142,8 @@ def does_db_update let_it_be(:project1) { create(:project) } let_it_be(:project2) { create(:project) } - describe '#owner_project' do - subject(:owner_project) { project_runner.owner_project } + describe '#owner' do + subject(:owner) { project_runner.owner } context 'with project1 as first project associated with runner' do let_it_be(:project_runner) { create(:ci_runner, :project, projects: [project1, project2]) } @@ -1638,6 +1655,22 @@ def does_db_update end end end + + describe '#owner' do + subject(:owner) { runner.owner } + + context 'with runner assigned to child_group' do + let(:runner) { child_group_runner } + + it { is_expected.to eq child_group } + end + + context 'with runner assigned to top_level_group_runner' do + let(:runner) { top_level_group_runner } + + it { is_expected.to eq top_level_group } + end + end end describe '#short_sha' do diff --git a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb index 35ef27dcd485b1..9155416bbf5125 100644 --- a/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb +++ b/spec/services/ci/runners/set_runner_associated_projects_service_spec.rb @@ -59,7 +59,7 @@ runner.reload - expect(runner.owner_project).to eq(owner_project) + expect(runner.owner).to eq(owner_project) expect(runner.runner_projects.order(:id).map(&:project_id)).to eq([owner_project, *new_projects].map(&:id)) end end @@ -72,7 +72,7 @@ runner.reload - expect(runner.owner_project).to eq(owner_project) + expect(runner.owner).to eq(owner_project) expect(runner.runner_projects.order(:id).map(&:project_id)).to eq([owner_project, *new_projects].map(&:id)) end end @@ -85,7 +85,7 @@ runner.reload - expect(runner.owner_project).to eq(owner_project) + expect(runner.owner).to eq(owner_project) expect(runner.projects.ids).to contain_exactly(owner_project.id) end end @@ -164,7 +164,7 @@ runner.reload - expect(runner.owner_project).to be_nil + expect(runner.owner).to be_nil expect(runner.projects.ids).to be_empty end @@ -176,7 +176,7 @@ runner.reload - expect(runner.owner_project).to be_nil + expect(runner.owner).to be_nil expect(runner.projects.ids).to be_empty end end @@ -204,7 +204,7 @@ runner.reload - expect(runner.owner_project).to eq(owner_project) + expect(runner.owner).to eq(owner_project) expect(runner.runner_projects.order(:id).map(&:project_id)).to eq(new_projects.map(&:id)) end end @@ -217,7 +217,7 @@ runner.reload - expect(runner.owner_project).to be_nil + expect(runner.owner).to be_nil expect(runner.projects.ids).to be_empty end end -- GitLab From 03ddeaab8318d031331a0483ee54e48de3ac3272 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Nov 2024 10:28:58 +0100 Subject: [PATCH 2/3] Memoize owner attribute --- app/models/ci/runner.rb | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 8d11be181a2834..10c1750204ca59 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -331,19 +331,18 @@ def self.runner_matchers end def runner_matcher - strong_memoize(:runner_matcher) do - Gitlab::Ci::Matching::RunnerMatcher.new({ - runner_ids: [id], - runner_type: runner_type, - public_projects_minutes_cost_factor: public_projects_minutes_cost_factor, - private_projects_minutes_cost_factor: private_projects_minutes_cost_factor, - run_untagged: run_untagged, - access_level: access_level, - tag_list: tag_list, - allowed_plan_ids: allowed_plan_ids - }) - end - end + Gitlab::Ci::Matching::RunnerMatcher.new({ + runner_ids: [id], + runner_type: runner_type, + public_projects_minutes_cost_factor: public_projects_minutes_cost_factor, + private_projects_minutes_cost_factor: private_projects_minutes_cost_factor, + run_untagged: run_untagged, + access_level: access_level, + tag_list: tag_list, + allowed_plan_ids: allowed_plan_ids + }) + end + strong_memoize_attr :runner_matcher def assign_to(project, current_user = nil) if instance_type? @@ -394,6 +393,7 @@ def owner ::Project.find_by_id(sharding_key_id) end end + strong_memoize_attr :owner def owner_project owner if project_type? @@ -506,10 +506,9 @@ def uncached_contacted_at end def namespace_ids - strong_memoize(:namespace_ids) do - runner_namespaces.pluck(:namespace_id).compact - end + runner_namespaces.pluck(:namespace_id).compact end + strong_memoize_attr :namespace_ids def compute_token_expiration case runner_type -- GitLab From 8aff2a2777a46e965f22e4739a94bba100e220eb Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 5 Nov 2024 11:16:14 +0100 Subject: [PATCH 3/3] Fix specs by clearing memoization --- app/models/ci/runner.rb | 10 +++++----- spec/services/ci/runners/assign_runner_service_spec.rb | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 10c1750204ca59..1b066ace999414 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -353,7 +353,11 @@ def assign_to(project, current_user = nil) begin transaction do - self.sharding_key_id = project.id if self.runner_projects.empty? + if self.runner_projects.empty? + self.sharding_key_id = project.id + self.clear_memoization(:owner) + end + self.runner_projects << ::Ci::RunnerProject.new(project: project, runner: self) self.save! end @@ -395,10 +399,6 @@ def owner end strong_memoize_attr :owner - def owner_project - owner if project_type? - end - def belongs_to_one_project? runner_projects.limit(2).count(:all) == 1 end diff --git a/spec/services/ci/runners/assign_runner_service_spec.rb b/spec/services/ci/runners/assign_runner_service_spec.rb index f585e78345e368..dda68e653e3e53 100644 --- a/spec/services/ci/runners/assign_runner_service_spec.rb +++ b/spec/services/ci/runners/assign_runner_service_spec.rb @@ -3,13 +3,13 @@ require 'spec_helper' RSpec.describe ::Ci::Runners::AssignRunnerService, '#execute', feature_category: :runner do - let(:service) { described_class.new(runner, new_project, user) } - let_it_be(:organization1) { create(:organization) } let_it_be(:owner_group) { create(:group, organization: organization1) } let_it_be(:owner_project) { create(:project, group: owner_group, organization: organization1) } let_it_be(:new_project) { create(:project, organization: organization1) } - let_it_be(:runner) { create(:ci_runner, :project, projects: [owner_project]) } + + let(:service) { described_class.new(runner, new_project, user) } + let(:runner) { create(:ci_runner, :project, projects: [owner_project]) } subject(:execute) { service.execute } @@ -108,7 +108,7 @@ end context 'with admin user', :enable_admin_mode do - let(:user) { create(:user, :admin) } + let_it_be(:user) { create(:user, :admin) } it 'calls assign_to on runner and returns success response' do expect(runner).to receive(:assign_to).with(new_project, user).once.and_call_original @@ -117,7 +117,7 @@ end context 'when runner is not associated with any projects' do - let_it_be(:runner) { create(:ci_runner, :project, :without_projects) } + let(:runner) { create(:ci_runner, :project, :without_projects) } it 'calls assign_to on runner and returns success response' do expect(runner).to receive(:assign_to).with(new_project, user).once.and_call_original -- GitLab