Steal pending merge request diff commit user jobs

The background migration MigrateMergeRequestDiffCommitUsers processes
batches of merge_request_diff_commits rows in batches. These migrations
have been running for several weeks now.

Unfortunately, on GitLab.com we are running into problems with the
remaining 750 or so jobs. Some jobs take a few minutes to complete,
while others can take over an hour to complete. The scheduling interval
was originally just a few minutes. This lead to many jobs being thrown
away, as they could not be picked up fast enough when there is a long
running job. Rescheduling jobs with a greater interval hasn't proven
successful.

In this commit we add a new background migration to clean up the pending
work. This background migration picks up a single job (in reverse
order), processes it, then schedules the next job. This approach
prevents jobs from conflicting with each other if they take too long.

In addition, we change the existing migration to not process jobs that
have already been processed. This way existing jobs that have yet to be
picked up won't lead to duplicate work.

Both the old and new migration _can_ process work in parallel, as they
are different background migration classes. This shouldn't pose any
problems. For the last few weeks we've been manually running jobs in a
Rails console in parallel to the Sidekiq jobs, without any problems
surfacing.

See https://gitlab.com/gitlab-org/gitlab/-/issues/334394 for more
information.

Changelog: added

db/post_migrate/20210823113259_steal_merge_request_diff_commit_users_migration.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class StealMergeRequestDiffCommitUsersMigration < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    job = Gitlab::Database::BackgroundMigrationJob
+      .for_migration_class('MigrateMergeRequestDiffCommitUsers')
+      .pending
+      .last
+
+    return unless job
+
+    # We schedule in one hour so we don't end up running the migrations while a
+    # deployment is still wrapping up. Not that that really matters, but it
+    # prevents from too much happening during a deployment window.
+    migrate_in(1.hour, 'StealMigrateMergeRequestDiffCommitUsers', job.arguments)
+  end
+
+  def down
+    # no-op
+  end
+end

db/schema_migrations/20210823113259

+06b44a856fc970f52b19ad8eeb38f885182003eff50ef1524ecf30887f4664d9
\ No newline at end of file

lib/gitlab/background_migration/migrate_merge_request_diff_commit_users.rb

@@ -78,6 +78,8 @@ module Gitlab
       # rubocop: enable Style/Documentation
 
       def perform(start_id, stop_id)
+        return if already_processed?(start_id, stop_id)
+
         # This Hash maps user names + emails to their corresponding rows in
         # merge_request_diff_commit_users.
         user_mapping = {}
@@ -94,6 +96,13 @@ module Gitlab
         )
       end
 
+      def already_processed?(start_id, stop_id)
+        Database::BackgroundMigrationJob
+          .for_migration_execution('MigrateMergeRequestDiffCommitUsers', [start_id, stop_id])
+          .succeeded
+          .any?
+      end
+
       # Returns the data we'll use to determine what merge_request_diff_commits
       # rows to update, and what data to use for populating their
       # commit_author_id and committer_id columns.

lib/gitlab/background_migration/steal_migrate_merge_request_diff_commit_users.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # A background migration that finished any pending
+    # MigrateMergeRequestDiffCommitUsers jobs, and schedules new jobs itself.
+    #
+    # This migration exists so we can bypass rescheduling issues (e.g. jobs
+    # getting dropped after too many retries) that may occur when
+    # MigrateMergeRequestDiffCommitUsers jobs take longer than expected.
+    class StealMigrateMergeRequestDiffCommitUsers
+      def perform(start_id, stop_id)
+        MigrateMergeRequestDiffCommitUsers.new.perform(start_id, stop_id)
+        schedule_next_job
+      end
+
+      def schedule_next_job
+        # We process jobs in reverse order, so that (hopefully) we are less
+        # likely to process jobs that the regular background migration job is
+        # also processing.
+        next_job = Database::BackgroundMigrationJob
+          .for_migration_class('MigrateMergeRequestDiffCommitUsers')
+          .pending
+          .last
+
+        return unless next_job
+
+        BackgroundMigrationWorker.perform_in(
+          5.minutes,
+          'StealMigrateMergeRequestDiffCommitUsers',
+          next_job.arguments
+        )
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/migrate_merge_request_diff_commit_users_spec.rb

@@ -91,6 +91,18 @@ RSpec.describe Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers d
   end
 
   describe '#perform' do
+    it 'skips jobs that have already been completed' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [1, 10],
+        status: :succeeded
+      )
+
+      expect(migration).not_to receive(:get_data_to_update)
+
+      migration.perform(1, 10)
+    end
+
     it 'migrates the data in the range' do
       commits.create!(
         merge_request_diff_id: diff.id,

spec/lib/gitlab/background_migration/steal_migrate_merge_request_diff_commit_users_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::StealMigrateMergeRequestDiffCommitUsers do
+  let(:migration) { described_class.new }
+
+  describe '#perform' do
+    it 'processes the background migration' do
+      spy = instance_spy(
+        Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers
+      )
+
+      allow(Gitlab::BackgroundMigration::MigrateMergeRequestDiffCommitUsers)
+        .to receive(:new)
+        .and_return(spy)
+
+      expect(spy).to receive(:perform).with(1, 4)
+      expect(migration).to receive(:schedule_next_job)
+
+      migration.perform(1, 4)
+    end
+  end
+
+  describe '#schedule_next_job' do
+    it 'schedules the next job in reverse order' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [10, 20]
+      )
+
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [40, 50]
+      )
+
+      expect(BackgroundMigrationWorker)
+        .to receive(:perform_in)
+        .with(5.minutes, 'StealMigrateMergeRequestDiffCommitUsers', [40, 50])
+
+      migration.schedule_next_job
+    end
+
+    it 'does not schedule any new jobs when there are none' do
+      expect(BackgroundMigrationWorker).not_to receive(:perform_in)
+
+      migration.schedule_next_job
+    end
+  end
+end

spec/migrations/steal_merge_request_diff_commit_users_migration_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration! 'steal_merge_request_diff_commit_users_migration'
+
+RSpec.describe StealMergeRequestDiffCommitUsersMigration, :migration do
+  let(:migration) { described_class.new }
+
+  describe '#up' do
+    it 'schedules a job if there are pending jobs' do
+      Gitlab::Database::BackgroundMigrationJob.create!(
+        class_name: 'MigrateMergeRequestDiffCommitUsers',
+        arguments: [10, 20]
+      )
+
+      expect(migration)
+        .to receive(:migrate_in)
+        .with(1.hour, 'StealMigrateMergeRequestDiffCommitUsers', [10, 20])
+
+      migration.up
+    end
+
+    it 'does not schedule any jobs when all jobs have been completed' do
+      expect(migration).not_to receive(:migrate_in)
+
+      migration.up
+    end
+  end
+end