Merge branch 'steal-merge-request-diff-commit-migrations' into 'master'

Steal pending merge request diff commit user jobs

See merge request gitlab-org/gitlab!68769

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