diff --git a/changelogs/unreleased/mk-no-op-delete-conflicting-redirects.yml b/changelogs/unreleased/mk-no-op-delete-conflicting-redirects.yml new file mode 100644 index 00000000000..37fdb1df6df --- /dev/null +++ b/changelogs/unreleased/mk-no-op-delete-conflicting-redirects.yml @@ -0,0 +1,6 @@ +--- +title: Prevent excessive DB load due to faulty DeleteConflictingRedirectRoutes background + migration +merge_request: 16205 +author: +type: fixed diff --git a/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb b/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb index 3e84b295be4..033019c398e 100644 --- a/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb +++ b/db/post_migrate/20170907170235_delete_conflicting_redirect_routes.rb @@ -2,36 +2,12 @@ # for more information on how to write migrations for GitLab. class DeleteConflictingRedirectRoutes < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - MIGRATION = 'DeleteConflictingRedirectRoutesRange'.freeze - BATCH_SIZE = 200 # At 200, I expect under 20s per batch, which is under our query timeout of 60s. - DELAY_INTERVAL = 12.seconds - - disable_ddl_transaction! - - class Route < ActiveRecord::Base - include EachBatch - - self.table_name = 'routes' - end - def up - say opening_message - - queue_background_migration_jobs_by_range_at_intervals(Route, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE) + # No-op. + # See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252 end def down # nothing end - - def opening_message - <<~MSG - Clean up redirect routes that conflict with regular routes. - See initial bug fix: - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13357 - MSG - end end diff --git a/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb b/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb index a1af045a71f..21b626dde56 100644 --- a/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb +++ b/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range.rb @@ -1,44 +1,12 @@ # frozen_string_literal: true -# rubocop:disable Metrics/LineLength # rubocop:disable Style/Documentation module Gitlab module BackgroundMigration class DeleteConflictingRedirectRoutesRange - class Route < ActiveRecord::Base - self.table_name = 'routes' - end - - class RedirectRoute < ActiveRecord::Base - self.table_name = 'redirect_routes' - end - - # start_id - The start ID of the range of events to process - # end_id - The end ID of the range to process. def perform(start_id, end_id) - return unless migrate? - - conflicts = RedirectRoute.where(routes_match_redirects_clause(start_id, end_id)) - num_rows = conflicts.delete_all - - Rails.logger.info("Gitlab::BackgroundMigration::DeleteConflictingRedirectRoutesRange [#{start_id}, #{end_id}] - Deleted #{num_rows} redirect routes that were conflicting with routes.") - end - - def migrate? - Route.table_exists? && RedirectRoute.table_exists? - end - - def routes_match_redirects_clause(start_id, end_id) - <<~ROUTES_MATCH_REDIRECTS - EXISTS ( - SELECT 1 FROM routes - WHERE ( - LOWER(redirect_routes.path) = LOWER(routes.path) - OR LOWER(redirect_routes.path) LIKE LOWER(CONCAT(routes.path, '/%')) - ) - AND routes.id BETWEEN #{start_id} AND #{end_id} - ) - ROUTES_MATCH_REDIRECTS + # No-op. + # See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252 end end end diff --git a/spec/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range_spec.rb b/spec/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range_spec.rb index 5c471cbdeda..9bae7e53b71 100644 --- a/spec/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range_spec.rb +++ b/spec/lib/gitlab/background_migration/delete_conflicting_redirect_routes_range_spec.rb @@ -24,17 +24,12 @@ describe Gitlab::BackgroundMigration::DeleteConflictingRedirectRoutesRange, :mig redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5') end - it 'deletes the conflicting redirect_routes in the range' do + # No-op. See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252 + it 'NO-OP: does not delete any redirect_routes' do expect(redirect_routes.count).to eq(8) - expect do - described_class.new.perform(1, 3) - end.to change { redirect_routes.where("path like 'foo%'").count }.from(5).to(2) + described_class.new.perform(1, 5) - expect do - described_class.new.perform(4, 5) - end.to change { redirect_routes.where("path like 'foo%'").count }.from(2).to(0) - - expect(redirect_routes.count).to eq(3) + expect(redirect_routes.count).to eq(8) end end diff --git a/spec/migrations/delete_conflicting_redirect_routes_spec.rb b/spec/migrations/delete_conflicting_redirect_routes_spec.rb index 1df2477da51..8a191bd7139 100644 --- a/spec/migrations/delete_conflicting_redirect_routes_spec.rb +++ b/spec/migrations/delete_conflicting_redirect_routes_spec.rb @@ -10,9 +10,6 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do end before do - stub_const("DeleteConflictingRedirectRoutes::BATCH_SIZE", 2) - stub_const("Gitlab::Database::MigrationHelpers::BACKGROUND_MIGRATION_JOB_BUFFER_SIZE", 2) - routes.create!(id: 1, source_id: 1, source_type: 'Namespace', path: 'foo1') routes.create!(id: 2, source_id: 2, source_type: 'Namespace', path: 'foo2') routes.create!(id: 3, source_id: 3, source_type: 'Namespace', path: 'foo3') @@ -32,27 +29,14 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5') end - it 'correctly schedules background migrations' do + # No-op. See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252 + it 'NO-OP: does not schedule any background migrations' do Sidekiq::Testing.fake! do Timecop.freeze do migrate! - expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([described_class::MIGRATION, [1, 2]]) - expect(BackgroundMigrationWorker.jobs[0]['at']).to eq(12.seconds.from_now.to_f) - expect(BackgroundMigrationWorker.jobs[1]['args']).to eq([described_class::MIGRATION, [3, 4]]) - expect(BackgroundMigrationWorker.jobs[1]['at']).to eq(24.seconds.from_now.to_f) - expect(BackgroundMigrationWorker.jobs[2]['args']).to eq([described_class::MIGRATION, [5, 5]]) - expect(BackgroundMigrationWorker.jobs[2]['at']).to eq(36.seconds.from_now.to_f) - expect(BackgroundMigrationWorker.jobs.size).to eq 3 + expect(BackgroundMigrationWorker.jobs.size).to eq 0 end end end - - it 'schedules background migrations' do - Sidekiq::Testing.inline! do - expect do - migrate! - end.to change { redirect_routes.count }.from(8).to(3) - end - end end