Merge branch '54670-external-diffs-when-outdated' into 'master'

Allow external diffs to be used conditionally

Closes #54670

See merge request gitlab-org/gitlab-ce!25432

5 files changed, 253 insertions, 3 deletions

diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb
index a73f330a7a9..a1809a26265 100644
--- a/spec/factories/merge_requests.rb
+++ b/spec/factories/merge_requests.rb
@@ -46,10 +46,26 @@ FactoryBot.define do
       target_branch "improve/awesome"
     end
 
+    trait :merged_last_month do
+      merged
+
+      after(:build) do |merge_request|
+        merge_request.build_metrics.merged_at = 1.month.ago
+      end
+    end
+
     trait :closed do
       state :closed
     end
 
+    trait :closed_last_month do
+      closed
+
+      after(:build) do |merge_request|
+        merge_request.build_metrics.latest_closed_at = 1.month.ago
+      end
+    end
+
     trait :opened do
       state :opened
     end
diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb
index 53f5307ea0b..0f00ea7e85e 100644
--- a/spec/models/merge_request_diff_spec.rb
+++ b/spec/models/merge_request_diff_spec.rb
@@ -51,7 +51,104 @@ describe MergeRequestDiff do
     end
   end
 
-  describe '#latest' do
+  describe '.ids_for_external_storage_migration' do
+    set(:merge_request) { create(:merge_request) }
+    set(:outdated) { merge_request.merge_request_diff }
+    set(:latest) { merge_request.create_merge_request_diff }
+
+    set(:closed_mr) { create(:merge_request, :closed_last_month) }
+    let(:closed) { closed_mr.merge_request_diff }
+
+    set(:merged_mr) { create(:merge_request, :merged_last_month) }
+    let(:merged) { merged_mr.merge_request_diff }
+
+    set(:recently_closed_mr) { create(:merge_request, :closed) }
+    let(:closed_recently) { recently_closed_mr.merge_request_diff }
+
+    set(:recently_merged_mr) { create(:merge_request, :merged) }
+    let(:merged_recently) { recently_merged_mr.merge_request_diff }
+
+    before do
+      merge_request.update!(latest_merge_request_diff: latest)
+    end
+
+    subject { described_class.ids_for_external_storage_migration(limit: 1000) }
+
+    context 'external diffs are disabled' do
+      before do
+        stub_external_diffs_setting(enabled: false)
+      end
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'external diffs are misconfigured' do
+      before do
+        stub_external_diffs_setting(enabled: true, when: 'every second tuesday')
+      end
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'external diffs are enabled unconditionally' do
+      before do
+        stub_external_diffs_setting(enabled: true)
+      end
+
+      it { is_expected.to contain_exactly(outdated.id, latest.id, closed.id, merged.id, closed_recently.id, merged_recently.id) }
+    end
+
+    context 'external diffs are enabled for outdated diffs' do
+      before do
+        stub_external_diffs_setting(enabled: true, when: 'outdated')
+      end
+
+      it 'returns records for outdated merge request versions' do
+        is_expected.to contain_exactly(outdated.id, closed.id, merged.id)
+      end
+    end
+
+    context 'with limit' do
+      it 'respects the limit' do
+        stub_external_diffs_setting(enabled: true)
+
+        expect(described_class.ids_for_external_storage_migration(limit: 3).count).to eq(3)
+      end
+    end
+  end
+
+  describe '#migrate_files_to_external_storage!' do
+    let(:diff) { create(:merge_request).merge_request_diff }
+
+    it 'converts from in-database to external storage' do
+      expect(diff).not_to be_stored_externally
+
+      stub_external_diffs_setting(enabled: true)
+      expect(diff).to receive(:save!)
+
+      diff.migrate_files_to_external_storage!
+
+      expect(diff).to be_stored_externally
+    end
+
+    it 'does nothing with an external diff' do
+      stub_external_diffs_setting(enabled: true)
+
+      expect(diff).to be_stored_externally
+      expect(diff).not_to receive(:save!)
+
+      diff.migrate_files_to_external_storage!
+    end
+
+    it 'does nothing if external diffs are disabled' do
+      expect(diff).not_to be_stored_externally
+      expect(diff).not_to receive(:save!)
+
+      diff.migrate_files_to_external_storage!
+    end
+  end
+
+  describe '#latest?' do
     let!(:mr) { create(:merge_request, :with_diffs) }
     let!(:first_diff) { mr.merge_request_diff }
     let!(:last_diff) { mr.create_merge_request_diff }
@@ -222,14 +319,58 @@ describe MergeRequestDiff do
     include_examples 'merge request diffs'
   end
 
-  describe 'external diffs configured' do
+  describe 'external diffs always enabled' do
     before do
-      stub_external_diffs_setting(enabled: true)
+      stub_external_diffs_setting(enabled: true, when: 'always')
     end
 
     include_examples 'merge request diffs'
   end
 
+  describe 'exernal diffs enabled for outdated diffs' do
+    before do
+      stub_external_diffs_setting(enabled: true, when: 'outdated')
+    end
+
+    include_examples 'merge request diffs'
+
+    it 'stores up-to-date diffs in the database' do
+      expect(diff).not_to be_stored_externally
+    end
+
+    it 'stores diffs for recently closed MRs in the database' do
+      mr = create(:merge_request, :closed)
+
+      expect(mr.merge_request_diff).not_to be_stored_externally
+    end
+
+    it 'stores diffs for recently merged MRs in the database' do
+      mr = create(:merge_request, :merged)
+
+      expect(mr.merge_request_diff).not_to be_stored_externally
+    end
+
+    it 'stores diffs for old MR versions in external storage' do
+      old_diff = diff
+      merge_request.create_merge_request_diff
+      old_diff.migrate_files_to_external_storage!
+
+      expect(old_diff).to be_stored_externally
+    end
+
+    it 'stores diffs for old closed MRs in external storage' do
+      mr = create(:merge_request, :closed_last_month)
+
+      expect(mr.merge_request_diff).to be_stored_externally
+    end
+
+    it 'stores diffs for old merged MRs in external storage' do
+      mr = create(:merge_request, :merged_last_month)
+
+      expect(mr.merge_request_diff).to be_stored_externally
+    end
+  end
+
   describe '#commit_shas' do
     it 'returns all commit SHAs using commits from the DB' do
       expect(diff_with_commits.commit_shas).not_to be_empty
diff --git a/spec/services/merge_requests/migrate_external_diffs_service_spec.rb b/spec/services/merge_requests/migrate_external_diffs_service_spec.rb
new file mode 100644
index 00000000000..40ac747e66f
--- /dev/null
+++ b/spec/services/merge_requests/migrate_external_diffs_service_spec.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MergeRequests::MigrateExternalDiffsService do
+  let(:merge_request) { create(:merge_request) }
+  let(:diff) { merge_request.merge_request_diff }
+
+  describe '.enqueue!', :sidekiq do
+    around do |example|
+      Sidekiq::Testing.fake! { example.run }
+    end
+
+    it 'enqueues nothing if external diffs are disabled' do
+      expect(diff).not_to be_stored_externally
+
+      expect { described_class.enqueue! }
+        .not_to change { MigrateExternalDiffsWorker.jobs.count }
+    end
+
+    it 'enqueues eligible in-database diffs if external diffs are enabled' do
+      expect(diff).not_to be_stored_externally
+
+      stub_external_diffs_setting(enabled: true)
+
+      expect { described_class.enqueue! }
+        .to change { MigrateExternalDiffsWorker.jobs.count }
+        .by(1)
+    end
+  end
+
+  describe '#execute' do
+    it 'migrates an in-database diff to the external store' do
+      expect(diff).not_to be_stored_externally
+
+      stub_external_diffs_setting(enabled: true)
+
+      described_class.new(diff).execute
+
+      expect(diff).to be_stored_externally
+    end
+  end
+end
diff --git a/spec/workers/migrate_external_diffs_worker_spec.rb b/spec/workers/migrate_external_diffs_worker_spec.rb
new file mode 100644
index 00000000000..88d48cad14b
--- /dev/null
+++ b/spec/workers/migrate_external_diffs_worker_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe MigrateExternalDiffsWorker do
+  let(:worker) { described_class.new }
+  let(:diff) { create(:merge_request).merge_request_diff }
+
+  describe '#perform' do
+    it 'migrates the listed diff' do
+      expect_next_instance_of(MergeRequests::MigrateExternalDiffsService) do |instance|
+        expect(instance.diff).to eq(diff)
+        expect(instance).to receive(:execute)
+      end
+
+      worker.perform(diff.id)
+    end
+
+    it 'does nothing if the diff is missing' do
+      diff.destroy
+
+      worker.perform(diff.id)
+    end
+  end
+end
diff --git a/spec/workers/schedule_migrate_external_diffs_worker_spec.rb b/spec/workers/schedule_migrate_external_diffs_worker_spec.rb
new file mode 100644
index 00000000000..9d6fecc9f4e
--- /dev/null
+++ b/spec/workers/schedule_migrate_external_diffs_worker_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe ScheduleMigrateExternalDiffsWorker do
+  include ExclusiveLeaseHelpers
+
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    it 'triggers a scan for diffs to migrate' do
+      expect(MergeRequests::MigrateExternalDiffsService).to receive(:enqueue!)
+
+      worker.perform
+    end
+
+    it 'will not run if the lease is already taken' do
+      stub_exclusive_lease_taken('schedule_migrate_external_diffs_worker', timeout: 2.hours)
+
+      expect(MergeRequests::MigrateExternalDiffsService).not_to receive(:enqueue!)
+
+      worker.perform
+    end
+  end
+end