Merge branch 'mk-no-op-delete-conflicting-redirects' into 'master'
Prevent excessive DB load due to faulty DeleteConflictingRedirectRoutes background migration See merge request gitlab-org/gitlab-ce!16205
This commit is contained in:
commit
d2891822e7
|
@ -0,0 +1,6 @@
|
||||||
|
---
|
||||||
|
title: Prevent excessive DB load due to faulty DeleteConflictingRedirectRoutes background
|
||||||
|
migration
|
||||||
|
merge_request: 16205
|
||||||
|
author:
|
||||||
|
type: fixed
|
|
@ -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
|
||||||
|
|
|
@ -1,44 +1,12 @@
|
||||||
# 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
|
||||||
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
|
||||||
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
|
||||||
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
|
||||||
|
|
Loading…
Reference in New Issue