Remove logic keeping sharding_key_id up to date in runners
UPDATE: we might want to keep sharding_key_id in the form of a owner_project_id column, only containing values for project runners. We would also want to preserve UpdateProjectRunnersOwnerService so that orphaned runners are deleted.
As decided in #470872 (comment 2382046293), we'll replace sharding_key_id by organization_id. Since we had changed the logic that determines a runner's owner to use sharding_key_id for performance and flexibility, we've reverted the logic to calculate the owner in #523697 (closed), and once we've made the organization_id the sharding key for runners, we can remove the logic that keeps sharding_key_id up to date in runners.
Patch to remove logic keeping sharding_key_id up to date
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 1d4bb4ef4911..f9a848d490ee 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -381,11 +381,6 @@ def assign_to(project, current_user = nil)
begin
transaction do
- if projects.id_in(sharding_key_id).empty? && !update_project_id
- self.errors.add(:assign_to, 'Runner is orphaned and no fallback owner exists')
- next false
- end
-
self.runner_projects << ::Ci::RunnerProject.new(project: project, runner: self)
self.save!
end
@@ -588,28 +583,6 @@ def partition_id
joins(:runner_managers).merge(RunnerManager.with_upgrade_status(upgrade_status))
end
- def fallback_owner_project
- # NOTE: when a project is deleted, the respective ci_runner_projects records are not immediately
- # deleted by the LFK, so we might find join records that point to a still-existing project
- project_ids = runner_projects.order(:id).pluck(:project_id)
- projects_added_to_runner_asc = Arel.sql("array_position(ARRAY[#{project_ids.join(',')}]::bigint[], id)")
- Project.order(projects_added_to_runner_asc).find_by_id(project_ids)
- end
-
- # Ensure we have a valid sharding_key_id. Logic is similar to the one used in
- # Ci::Runners::UpdateProjectRunnersOwnerService when a project is deleted
- def update_project_id
- project_id = fallback_owner_project&.id
-
- return if project_id.nil?
-
- self.clear_memoization(:owner)
- self.sharding_key_id = project_id
-
- runner_managers.where(runner_type: :project_type).each_batch { |batch| batch.update_all(sharding_key_id: project_id) }
- taggings.where(runner_type: :project_type).update_all(sharding_key_id: project_id)
- end
-
def compute_token_expiration_instance
return unless expiration_interval = Gitlab::CurrentSettings.runner_token_expiration_interval
diff --git a/app/services/ci/runners/update_project_runners_owner_service.rb b/app/services/ci/runners/update_project_runners_owner_service.rb
deleted file mode 100644
index 5b312245f1ff..000000000000
--- a/app/services/ci/runners/update_project_runners_owner_service.rb
+++ /dev/null
@@ -1,67 +0,0 @@
-# frozen_string_literal: true
-
-module Ci
- module Runners
- # Service used to recompute runner owners after a project is deleted.
- class UpdateProjectRunnersOwnerService
- BATCH_SIZE = 1000
-
- # @param [Int] project_id: the ID of the deleted project
- def initialize(project_id)
- @project_id = project_id
- end
-
- def execute
- # Since the project was deleted in the 'main' database, let's ensure that the respective
- # ci_runner_projects join records are also gone (would be handled by LFK otherwise,
- # but it is a helpful precondition for the service's logic)
- Ci::RunnerProject.belonging_to_project(project_id).each_batch do |batch|
- batch.delete_all
- end
-
- # rubocop: disable CodeReuse/ActiveRecord -- this query is too specific to generalize on the models
- runner_projects =
- Ci::RunnerProject.where(Ci::RunnerProject.arel_table[:runner_id].eq(Ci::Runner.arel_table[:id]))
- orphaned_runners = Ci::Runner.project_type.with_sharding_key(project_id)
-
- orphaned_runners.select(:id).each_batch(of: BATCH_SIZE) do |batch|
- runners_missing_owner_project = Ci::Runner.id_in(batch.limit(BATCH_SIZE).pluck_primary_key)
- runners_with_fallback_owner = runners_missing_owner_project.where_exists(runner_projects.limit(1))
-
- Ci::Runner.transaction do
- runner_ids = runners_with_fallback_owner.limit(BATCH_SIZE).pluck_primary_key
-
- runners_with_fallback_owner.update_all(runner_id_update_query(Ci::Runner.arel_table[:id]))
- Ci::RunnerManager.project_type.for_runner(runner_ids)
- .update_all(runner_id_update_query(Ci::RunnerManager.arel_table[:runner_id]))
- Ci::RunnerTagging.project_type.for_runner(runner_ids)
- .update_all(runner_id_update_query(Ci::RunnerTagging.arel_table[:runner_id]))
-
- # Delete any orphaned runners that are still pointing to the project
- # (they are the ones which no longer have any matching ci_runner_projects records)
- # We can afford to sidestep Ci::Runner hooks since we know by definition that
- # there are no Ci::RunnerProject models pointing to these runners (it's the reason they are being deleted)
- runners_missing_owner_project.project_type.with_sharding_key(project_id).delete_all
- end
- end
- # rubocop: enable CodeReuse/ActiveRecord
-
- ServiceResponse.success
- end
-
- private
-
- attr_reader :project_id
-
- def runner_id_update_query(runner_id_column)
- # rubocop: disable CodeReuse/ActiveRecord -- this query is too specific to generalize on the models
- runner_projects = Ci::RunnerProject.where(Ci::RunnerProject.arel_table[:runner_id].eq(runner_id_column))
-
- <<~SQL
- sharding_key_id = (#{runner_projects.order(id: :asc).limit(1).select(:project_id).to_sql})
- SQL
- # rubocop: enable CodeReuse/ActiveRecord
- end
- end
- end
-end
diff --git a/app/workers/ci/runners/update_project_runners_owner_worker.rb b/app/workers/ci/runners/update_project_runners_owner_worker.rb
index 2560af1d6569..c659a54b1e0e 100644
--- a/app/workers/ci/runners/update_project_runners_owner_worker.rb
+++ b/app/workers/ci/runners/update_project_runners_owner_worker.rb
@@ -11,9 +11,7 @@ class UpdateProjectRunnersOwnerWorker
feature_category :runner
- def handle_event(event)
- ::Ci::Runners::UpdateProjectRunnersOwnerService.new(event.data[:project_id]).execute
- end
+ def handle_event(event); end
end
end
end
diff --git a/lib/gitlab/event_store.rb b/lib/gitlab/event_store.rb
index 75070c56a065..7a6dbbd7a73a 100644
--- a/lib/gitlab/event_store.rb
+++ b/lib/gitlab/event_store.rb
@@ -42,7 +42,6 @@ def configure!(store)
store.subscribe ::MergeRequests::UpdateHeadPipelineWorker, to: ::Ci::PipelineCreatedEvent
store.subscribe ::Namespaces::UpdateRootStatisticsWorker, to: ::Projects::ProjectDeletedEvent
- store.subscribe ::Ci::Runners::UpdateProjectRunnersOwnerWorker, to: ::Projects::ProjectDeletedEvent
store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::DraftStateChangeEvent
store.subscribe ::MergeRequests::ProcessAutoMergeFromEventWorker, to: ::MergeRequests::DiscussionsResolvedEvent
diff --git a/spec/services/ci/runners/update_project_runners_owner_service_spec.rb b/spec/services/ci/runners/update_project_runners_owner_service_spec.rb
deleted file mode 100644
index 6dfd98cf850b..000000000000
--- a/spec/services/ci/runners/update_project_runners_owner_service_spec.rb
+++ /dev/null
@@ -1,74 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe ::Ci::Runners::UpdateProjectRunnersOwnerService, '#execute', feature_category: :runner do
- let_it_be(:owner_group) { create(:group) }
- let_it_be(:owner_project) { create(:project, group: owner_group) }
- let_it_be(:new_projects) { create_list(:project, 2) }
-
- let_it_be(:owned_runner1) do
- create(:ci_runner, :project, projects: [owner_project, new_projects.first], tag_list: %w[tag1])
- end
-
- let_it_be(:owned_runner2) { create(:ci_runner, :project, projects: [owner_project], tag_list: %w[tag1 tag2]) }
- let_it_be(:owned_runner3) do
- create(:ci_runner, :project, projects: [owner_project, *new_projects.reverse], tag_list: %w[tag2])
- end
-
- let_it_be(:owned_runner4) do
- create(:ci_runner, :project, projects: [owner_project, *new_projects], tag_list: %w[tag1 tag2])
- end
-
- let_it_be(:other_runner) { create(:ci_runner, :project, projects: new_projects, tag_list: %w[tag1 tag2]) }
- let_it_be(:orphaned_runner) { create(:ci_runner, :project, :without_projects, tag_list: %w[tag1 tag2]) }
-
- let_it_be(:owned_runner1_manager) { create(:ci_runner_machine, runner: owned_runner1) }
- let_it_be(:owned_runner2_manager) { create(:ci_runner_machine, runner: owned_runner2) }
- let_it_be(:owned_runner3_manager) { create(:ci_runner_machine, runner: owned_runner3) }
- let_it_be(:owned_runner4_manager) { create(:ci_runner_machine, runner: owned_runner4) }
-
- let(:service) { described_class.new(owner_project.id) }
-
- subject(:execute) { service.execute }
-
- before_all do
- owner_project.destroy!
- end
-
- before do
- stub_const("#{described_class}::BATCH_SIZE", 2)
- end
-
- it 'updates sharding_key_id on affected runners', :aggregate_failures do
- expect { execute }
- # owned_runner1's owner project was deleted
- .to change { owned_runner1.reload.sharding_key_id }.from(owner_project.id).to(new_projects.first.id)
- .and change { owned_runner1_manager.reload.sharding_key_id }.from(owner_project.id).to(new_projects.first.id)
- .and change { tagging_sharding_key_id_for_runner(owned_runner1) }.from(owner_project.id).to(new_projects.first.id)
- # owned_runner2's owner project was deleted and there were no other associated projects to fall back to
- .and change { Ci::Runner.find_by_id(owned_runner2) }.to(nil) # delete, since no other project to adopt it
- .and change { Ci::RunnerManager.find_by_id(owned_runner2_manager) }.to(nil)
- .and change { Ci::RunnerTagging.find_by_runner_id(owned_runner2) }.to(nil)
- # owned_runner3's owner project was deleted
- .and change { owned_runner3.reload.sharding_key_id }.from(owner_project.id).to(new_projects.last.id)
- .and change { owned_runner3_manager.reload.sharding_key_id }.from(owner_project.id).to(new_projects.last.id)
- .and change { tagging_sharding_key_id_for_runner(owned_runner3) }.from(owner_project.id).to(new_projects.last.id)
- # owned_runner4's owner project was deleted
- .and change { owned_runner4.reload.sharding_key_id }.from(owner_project.id).to(new_projects.first.id)
- .and change { owned_runner4_manager.reload.sharding_key_id }.from(owner_project.id).to(new_projects.first.id)
- .and change { tagging_sharding_key_id_for_runner(owned_runner4) }.from(owner_project.id).to(new_projects.first.id)
- # runners whose owner project was not affected
- .and not_change { other_runner.reload.sharding_key_id }.from(new_projects.first.id)
- .and not_change { orphaned_runner.reload.sharding_key_id }
- .and not_change { tagging_sharding_key_id_for_runner(orphaned_runner) }
-
- expect(execute).to be_success
- end
-
- private
-
- def tagging_sharding_key_id_for_runner(runner)
- runner.taggings.pluck(:sharding_key_id).uniq.sole
- end
-end
diff --git a/spec/workers/ci/runners/update_project_runners_owner_worker_spec.rb b/spec/workers/ci/runners/update_project_runners_owner_worker_spec.rb
index cf9e2c93368f..d9726b0956a7 100644
--- a/spec/workers/ci/runners/update_project_runners_owner_worker_spec.rb
+++ b/spec/workers/ci/runners/update_project_runners_owner_worker_spec.rb
@@ -10,15 +10,7 @@
{ project_id: project.id, namespace_id: project.namespace_id, root_namespace_id: project.root_namespace.id }
end
- it_behaves_like 'subscribes to event' do
+ it_behaves_like 'ignores the published event' do
let(:event) { project_deleted_event }
end
-
- it 'calls Ci::Runners::UpdateProjectRunnersOwnerService' do
- expect_next_instance_of(Ci::Runners::UpdateProjectRunnersOwnerService, project.id) do |service|
- expect(service).to receive(:execute)
- end
-
- described_class.new.handle_event(project_deleted_event)
- end
end
Edited by 🤖 GitLab Bot 🤖