Commit f6352772 authored by Michael Kozono's avatar Michael Kozono

Make DeleteConflictingRedirectRoutes no-op

Both the post-deploy and background migration.
parent ea55445d
---
title: Prevent excessive DB load due to faulty DeleteConflictingRedirectRoutes background
migration
merge_request: 16205
author:
type: fixed
...@@ -2,36 +2,12 @@ ...@@ -2,36 +2,12 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class DeleteConflictingRedirectRoutes < ActiveRecord::Migration 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 def up
say opening_message # No-op.
# See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252
queue_background_migration_jobs_by_range_at_intervals(Route, MIGRATION, DELAY_INTERVAL, batch_size: BATCH_SIZE)
end end
def down def down
# nothing # nothing
end 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 end
# frozen_string_literal: true # frozen_string_literal: true
# rubocop:disable Metrics/LineLength
# rubocop:disable Style/Documentation # rubocop:disable Style/Documentation
module Gitlab module Gitlab
module BackgroundMigration module BackgroundMigration
class DeleteConflictingRedirectRoutesRange 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) def perform(start_id, end_id)
return unless migrate? # No-op.
# See https://gitlab.com/gitlab-com/infrastructure/issues/3460#note_53223252
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
end end
end end
end end
......
...@@ -24,17 +24,12 @@ describe Gitlab::BackgroundMigration::DeleteConflictingRedirectRoutesRange, :mig ...@@ -24,17 +24,12 @@ describe Gitlab::BackgroundMigration::DeleteConflictingRedirectRoutesRange, :mig
redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5') redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5')
end 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(redirect_routes.count).to eq(8)
expect do described_class.new.perform(1, 5)
described_class.new.perform(1, 3)
end.to change { redirect_routes.where("path like 'foo%'").count }.from(5).to(2)
expect do expect(redirect_routes.count).to eq(8)
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)
end end
end end
...@@ -10,9 +10,6 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do ...@@ -10,9 +10,6 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do
end end
before do 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: 1, source_id: 1, source_type: 'Namespace', path: 'foo1')
routes.create!(id: 2, source_id: 2, source_type: 'Namespace', path: 'foo2') routes.create!(id: 2, source_id: 2, source_type: 'Namespace', path: 'foo2')
routes.create!(id: 3, source_id: 3, source_type: 'Namespace', path: 'foo3') routes.create!(id: 3, source_id: 3, source_type: 'Namespace', path: 'foo3')
...@@ -32,27 +29,14 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do ...@@ -32,27 +29,14 @@ describe DeleteConflictingRedirectRoutes, :migration, :sidekiq do
redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5') redirect_routes.create!(source_id: 1, source_type: 'Namespace', path: 'foo5')
end 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 Sidekiq::Testing.fake! do
Timecop.freeze do Timecop.freeze do
migrate! migrate!
expect(BackgroundMigrationWorker.jobs[0]['args']).to eq([described_class::MIGRATION, [1, 2]]) expect(BackgroundMigrationWorker.jobs.size).to eq 0
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
end end
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment