Implement Geo replication of MR diffs

Add another data type to the Geo self-service replication framework:
Merge Request Diffs

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/215137

changelogs/unreleased/tc-external-mr-diff-ssf.yml

+---
+title: 'Geo: Add migrations for registry and details tables for external MR diff replication'
+merge_request: 34248
+author:
+type: added

db/migrate/20200827142811_create_merge_request_diff_details.rb

+# frozen_string_literal: true
+
+class CreateMergeRequestDiffDetails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless table_exists?(:merge_request_diff_details)
+      with_lock_retries do
+        create_table :merge_request_diff_details, id: false do |t|
+          t.references :merge_request_diff, primary_key: true, null: false, foreign_key: { on_delete: :cascade }
+          t.datetime_with_timezone :verification_retry_at
+          t.datetime_with_timezone :verified_at
+          t.integer :verification_retry_count, limit: 2
+          t.binary :verification_checksum, using: 'verification_checksum::bytea'
+          t.text :verification_failure
+        end
+      end
+    end
+
+    add_text_limit :merge_request_diff_details, :verification_failure, 255
+  end
+
+  def down
+    drop_table :merge_request_diff_details
+  end
+end

db/schema_migrations/20200827142811

+0e2b3433577946177876f14ec414a1653c1edeaa796eea24f12740958f964442
\ No newline at end of file

db/structure.sql

@@ -13146,6 +13146,25 @@ CREATE TABLE public.merge_request_diff_commits (
     message text
 );
 
+CREATE TABLE public.merge_request_diff_details (
+    merge_request_diff_id bigint NOT NULL,
+    verification_retry_at timestamp with time zone,
+    verified_at timestamp with time zone,
+    verification_retry_count smallint,
+    verification_checksum bytea,
+    verification_failure text,
+    CONSTRAINT check_81429e3622 CHECK ((char_length(verification_failure) <= 255))
+);
+
+CREATE SEQUENCE public.merge_request_diff_details_merge_request_diff_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.merge_request_diff_details_merge_request_diff_id_seq OWNED BY public.merge_request_diff_details.merge_request_diff_id;
+
 CREATE TABLE public.merge_request_diff_files (
     merge_request_diff_id integer NOT NULL,
     relative_order integer NOT NULL,
@@ -17248,6 +17267,8 @@ ALTER TABLE ONLY public.merge_request_blocks ALTER COLUMN id SET DEFAULT nextval
 
 ALTER TABLE ONLY public.merge_request_context_commits ALTER COLUMN id SET DEFAULT nextval('public.merge_request_context_commits_id_seq'::regclass);
 
+ALTER TABLE ONLY public.merge_request_diff_details ALTER COLUMN merge_request_diff_id SET DEFAULT nextval('public.merge_request_diff_details_merge_request_diff_id_seq'::regclass);
+
 ALTER TABLE ONLY public.merge_request_diffs ALTER COLUMN id SET DEFAULT nextval('public.merge_request_diffs_id_seq'::regclass);
 
 ALTER TABLE ONLY public.merge_request_metrics ALTER COLUMN id SET DEFAULT nextval('public.merge_request_metrics_id_seq'::regclass);
@@ -18377,6 +18398,9 @@ ALTER TABLE ONLY public.merge_request_blocks
 ALTER TABLE ONLY public.merge_request_context_commits
     ADD CONSTRAINT merge_request_context_commits_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.merge_request_diff_details
+    ADD CONSTRAINT merge_request_diff_details_pkey PRIMARY KEY (merge_request_diff_id);
+
 ALTER TABLE ONLY public.merge_request_diffs
     ADD CONSTRAINT merge_request_diffs_pkey PRIMARY KEY (id);
 
@@ -20204,6 +20228,8 @@ CREATE UNIQUE INDEX index_merge_request_diff_commits_on_mr_diff_id_and_order ON
 
 CREATE INDEX index_merge_request_diff_commits_on_sha ON public.merge_request_diff_commits USING btree (sha);
 
+CREATE INDEX index_merge_request_diff_details_on_merge_request_diff_id ON public.merge_request_diff_details USING btree (merge_request_diff_id);
+
 CREATE UNIQUE INDEX index_merge_request_diff_files_on_mr_diff_id_and_order ON public.merge_request_diff_files USING btree (merge_request_diff_id, relative_order);
 
 CREATE INDEX index_merge_request_diffs_by_id_partial ON public.merge_request_diffs USING btree (id) WHERE ((files_count > 0) AND ((NOT stored_externally) OR (stored_externally IS NULL)));
@@ -22871,6 +22897,9 @@ ALTER TABLE ONLY public.deployment_merge_requests
 ALTER TABLE ONLY public.analytics_language_trend_repository_languages
     ADD CONSTRAINT fk_rails_86cc9aef5f FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.merge_request_diff_details
+    ADD CONSTRAINT fk_rails_86f4d24ecd FOREIGN KEY (merge_request_diff_id) REFERENCES public.merge_request_diffs(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.clusters_applications_crossplane
     ADD CONSTRAINT fk_rails_87186702df FOREIGN KEY (cluster_id) REFERENCES public.clusters(id) ON DELETE CASCADE;
 

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -186,6 +186,12 @@ configuration option in `gitlab.yml`. These metrics are served from the
 | `geo_terraform_states_registry`                | Gauge   | 13.3  | Number of terraform states in the registry | `url` |
 | `global_search_bulk_cron_queue_size`           | Gauge   | 12.10 | Number of database records waiting to be synchronized to Elasticsearch | |
 | `global_search_awaiting_indexing_queue_size`   | Gauge   | 13.2  | Number of database updates waiting to be synchronized to Elasticsearch while indexing is paused | |
+| `geo_merge_request_diffs`                      | Gauge   | 13.4  | Number of merge request diffs on primary | `url` |
+| `geo_merge_request_diffs_checksummed`          | Gauge   | 13.4  | Number of merge request diffs checksummed on primary | `url` |
+| `geo_merge_request_diffs_checksum_failed`      | Gauge   | 13.4  | Number of merge request diffs failed to calculate the checksum on primary | `url` |
+| `geo_merge_request_diffs_synced`               | Gauge   | 13.4  | Number of syncable merge request diffs synced on secondary | `url` |
+| `geo_merge_request_diffs_failed`               | Gauge   | 13.4  | Number of syncable merge request diffs failed to sync on secondary | `url` |
+| `geo_merge_request_diffs_registry`             | Gauge   | 13.4  | Number of merge request diffs in the registry | `url` |
 
 ## Database load balancing metrics **(PREMIUM ONLY)**
 

doc/api/geo_nodes.md

@@ -442,6 +442,12 @@ Example response:
     "last_successful_status_check_timestamp": 1510125024,
     "version": "10.3.0",
     "revision": "33d33a096a",
+    "merge_request_diffs_count": 12,
+    "merge_request_diffs_checksummed_count": 8,
+    "merge_request_diffs_checksum_failed_count": 0,
+    "merge_request_diffs_registry_count": 12,
+    "merge_request_diffs_synced_count": 9,
+    "merge_request_diffs_failed_count": 3,
     "package_files_count": 10,
     "package_files_checksummed_count": 10,
     "package_files_checksum_failed_count": 0,

doc/development/geo/framework.md

@@ -526,7 +526,7 @@ Metrics are gathered by `Geo::MetricsUpdateWorker`, persisted in
    `GET /geo_nodes/status` example response in
    `doc/api/geo_nodes.md`.
 1. Add the same fields to `GET /geo_nodes/status` example response in
-   `doc/api/geo_nodes.md`.
+   `ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json`.
 1. Add fields `geo_widgets`, `geo_widgets_checksummed`,
    `geo_widgets_checksum_failed`, `geo_widgets_synced`,
    `geo_widgets_failed`, and `geo_widgets_registry` to

ee/app/models/ee/merge_request_diff.rb

@@ -2,6 +2,61 @@
 
 module EE
   module MergeRequestDiff
+    extend ActiveSupport::Concern
+
+    prepended do
+      include ::Gitlab::Geo::ReplicableModel
+      include ObjectStorable
+
+      STORE_COLUMN = :external_diff_store
+
+      with_replicator Geo::MergeRequestDiffReplicator
+
+      has_one :merge_request_diff_detail, inverse_of: :merge_request_diff
+
+      delegate :verification_retry_at, :verification_retry_at=,
+               :verified_at, :verified_at=,
+               :verification_checksum, :verification_checksum=,
+               :verification_failure, :verification_failure=,
+               :verification_retry_count, :verification_retry_count=,
+        to: :merge_request_diff_detail, allow_nil: true
+
+      scope :has_external_diffs, -> { with_files.where(stored_externally: true) }
+      scope :project_id_in, ->(ids) { where(merge_request_id: ::MergeRequest.where(target_project_id: ids)) }
+
+      scope :checksummed, -> { has_external_diffs.where(merge_request_diff_detail: ::MergeRequestDiffDetail.checksummed) }
+      scope :checksum_failed, -> { has_external_diffs.where(merge_request_diff_detail: ::MergeRequestDiffDetail.checksum_failed) }
+    end
+
+    class_methods do
+      def replicables_for_geo_node(node = ::Gitlab::Geo.current_node)
+        has_external_diffs.merge(selective_sync_scope(node))
+                          .merge(object_storage_scope(node))
+      end
+
+      private
+
+      def object_storage_scope(node)
+        return all if node.sync_object_storage?
+
+        with_files_stored_locally
+      end
+
+      def selective_sync_scope(node)
+        return all unless node.selective_sync?
+
+        project_id_in(node.projects)
+      end
+    end
+
+    def merge_request_diff_detail
+      super || build_merge_request_diff_detail
+    end
+
+    def local?
+      stored_externally? && external_diff_store == ExternalDiffUploader::Store::LOCAL
+    end
+
     def log_geo_deleted_event
       # Keep empty for now. Should be addressed in future
       # by https://gitlab.com/gitlab-org/gitlab/issues/33817

ee/app/models/geo/merge_request_diff_registry.rb

+# frozen_string_literal: true
+
+class Geo::MergeRequestDiffRegistry < Geo::BaseRegistry
+  include ::Geo::ReplicableRegistry
+
+  MODEL_CLASS = ::MergeRequestDiff
+  MODEL_FOREIGN_KEY = :merge_request_diff_id
+
+  self.table_name = 'merge_request_diff_registry'
+
+  belongs_to :merge_request_diff, class_name: 'MergeRequestDiff'
+end

ee/app/models/geo_node_status.rb

@@ -42,7 +42,7 @@ class GeoNodeStatus < ApplicationRecord
     }
   end
 
-  # Why are disabled classes included? See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38959/diffs#note_402656534
+  # Why are disabled classes included? See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38959#note_402656534
   def self.replicator_class_status_fields
     Gitlab::Geo::REPLICATOR_CLASSES.map do |replicable_class|
       status_fields_for(replicable_class).keys
@@ -95,7 +95,7 @@ class GeoNodeStatus < ApplicationRecord
     design_repositories_failed_count
   ) + replicator_class_status_fields).freeze
 
-  # Why are disabled classes included? See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38959/diffs#note_402656534
+  # Why are disabled classes included? See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38959#note_402656534
   def self.replicator_class_prometheus_metrics
     Gitlab::Geo::REPLICATOR_CLASSES.map do |replicable_class|
       status_fields_for(replicable_class)

ee/app/models/merge_request_diff_detail.rb

+# frozen_string_literal: true
+
+class MergeRequestDiffDetail < ApplicationRecord
+  self.primary_key = :merge_request_diff_id
+
+  belongs_to :merge_request_diff, inverse_of: :merge_request_diff_detail
+
+  scope :checksummed, -> { where.not(verification_checksum: nil) }
+  scope :checksum_failed, -> { where.not(verification_failure: nil) }
+end

ee/app/replicators/geo/merge_request_diff_replicator.rb

+# frozen_string_literal: true
+
+module Geo
+  class MergeRequestDiffReplicator < Gitlab::Geo::Replicator
+    include ::Geo::BlobReplicatorStrategy
+
+    def self.model
+      ::MergeRequestDiff
+    end
+
+    def self.replication_enabled_by_default?
+      false
+    end
+
+    def self.primary_total_count
+      model.has_external_diffs.count
+    end
+
+    def carrierwave_uploader
+      model_record.external_diff
+    end
+  end
+end

ee/app/workers/geo/secondary/registry_consistency_worker.rb

@@ -20,6 +20,7 @@ module Geo
         Geo::DesignRegistry,
         Geo::JobArtifactRegistry,
         Geo::LfsObjectRegistry,
+        Geo::MergeRequestDiffRegistry,
         Geo::PackageFileRegistry,
         Geo::ProjectRegistry,
         Geo::TerraformStateRegistry,

ee/config/feature_flags/development/geo_merge_request_diff_replication.yml

+---
+name: geo_merge_request_diff_replication
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/34248
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/247100
+group: group::geo
+type: development
+default_enabled: false
\ No newline at end of file

ee/db/geo/migrate/20200827120552_create_merge_request_diff_registry.rb

+# frozen_string_literal: true
+
+class CreateMergeRequestDiffRegistry < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless table_exists?(:merge_request_diff_registry)
+      ActiveRecord::Base.transaction do
+        create_table(:merge_request_diff_registry) do |t|
+          t.datetime_with_timezone :created_at, null: false
+          t.datetime_with_timezone :retry_at
+          t.datetime_with_timezone :last_synced_at
+          t.bigint :merge_request_diff_id, null: false
+          t.integer :state, default: 0, null: false, limit: 2
+          t.integer :retry_count, default: 0, limit: 2
+          t.text :last_sync_failure
+
+          t.index :merge_request_diff_id, name: :index_merge_request_diff_registry_on_mr_diff_id
+          t.index :retry_at
+          t.index :state
+        end
+      end
+    end
+
+    add_text_limit :merge_request_diff_registry, :last_sync_failure, 255
+  end
+
+  def down
+    drop_table :merge_request_diff_registry
+  end
+end

ee/db/geo/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_07_30_133800) do
+ActiveRecord::Schema.define(version: 2020_08_27_120552) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -91,6 +91,19 @@ ActiveRecord::Schema.define(version: 2020_07_30_133800) do
     t.index ["success"], name: "index_lfs_object_registry_on_success"
   end
 
+  create_table "merge_request_diff_registry", force: :cascade do |t|
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "retry_at"
+    t.datetime_with_timezone "last_synced_at"
+    t.bigint "merge_request_diff_id", null: false
+    t.integer "state", limit: 2, default: 0, null: false
+    t.integer "retry_count", limit: 2, default: 0
+    t.text "last_sync_failure"
+    t.index ["merge_request_diff_id"], name: "index_merge_request_diff_registry_on_mr_diff_id"
+    t.index ["retry_at"], name: "index_merge_request_diff_registry_on_retry_at"
+    t.index ["state"], name: "index_merge_request_diff_registry_on_state"
+  end
+
   create_table "package_file_registry", id: :serial, force: :cascade do |t|
     t.integer "package_file_id", null: false
     t.integer "state", default: 0, null: false
@@ -180,4 +193,5 @@ ActiveRecord::Schema.define(version: 2020_07_30_133800) do
     t.index ["state"], name: "index_terraform_state_registry_on_state"
     t.index ["terraform_state_id"], name: "index_terraform_state_registry_on_terraform_state_id"
   end
+
 end

ee/lib/gitlab/geo.rb

@@ -20,6 +20,7 @@ module Gitlab
     # solutions can be found at
     # https://gitlab.com/gitlab-org/gitlab/-/issues/227693
     REPLICATOR_CLASSES = [
+      ::Geo::MergeRequestDiffReplicator,
       ::Geo::PackageFileReplicator,
       ::Geo::TerraformStateReplicator
     ].freeze

ee/lib/gitlab/geo/replicable_model.rb

@@ -12,8 +12,8 @@ module Gitlab
         after_create_commit -> { replicator.handle_after_create_commit if replicator.respond_to?(:handle_after_create_commit) }
         after_destroy -> { replicator.handle_after_destroy if replicator.respond_to?(:handle_after_destroy) }
 
-        scope :checksummed, -> { where('verification_checksum IS NOT NULL') }
-        scope :checksum_failed, -> { where('verification_failure IS NOT NULL') }
+        scope :checksummed, -> { where.not(verification_checksum: nil) }
+        scope :checksum_failed, -> { where.not(verification_failure: nil) }
 
         sha_attribute :verification_checksum
       end
@@ -51,7 +51,7 @@ module Gitlab
 
         return unless needs_checksum?
 
-        self.verification_checksum = self.class.hexdigest(file.path)
+        self.verification_checksum = self.class.hexdigest(replicator.carrierwave_uploader.path)
       end
 
       # Checks whether model needs checksum to be performed

ee/spec/factories/geo/external_merge_request_diff_registry.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :geo_merge_request_diff_registry, class: 'Geo::MergeRequestDiffRegistry' do
+    association :merge_request_diff, factory: :external_merge_request_diff
+
+    state { Geo::MergeRequestDiffRegistry.state_value(:pending) }
+
+    trait :synced do
+      state { Geo::MergeRequestDiffRegistry.state_value(:synced) }
+      last_synced_at { 5.days.ago }
+    end
+
+    trait :failed do
+      state { Geo::MergeRequestDiffRegistry.state_value(:failed) }
+      last_synced_at { 1.day.ago }
+      retry_count { 2 }
+      last_sync_failure { 'Random error' }
+    end
+
+    trait :started do
+      state { Geo::MergeRequestDiffRegistry.state_value(:started) }
+      last_synced_at { 1.day.ago }
+      retry_count { 0 }
+    end
+  end
+end

ee/spec/factories/merge_request_diff_details.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :merge_request_diff_detail do
+    merge_request_diff
+
+    trait(:checksummed) do
+      verification_checksum { 'abc' }
+    end
+
+    trait(:checksum_failure) do
+      verification_failure { 'Could not calculate the checksum' }
+    end
+  end
+end

ee/spec/factories/merge_request_diffs.rb

+# frozen_string_literal: true
+
+FactoryBot.modify do
+  factory :merge_request_diff do
+    trait(:checksummed) do
+      association :merge_request_diff_detail, :checksummed, strategy: :build
+    end
+
+    trait(:checksum_failure) do
+      association :merge_request_diff_detail, :checksum_failure, strategy: :build
+    end
+  end
+end

ee/spec/fixtures/api/schemas/public_api/v4/geo_node_status.json

@@ -46,6 +46,14 @@
     "wikis_checksummed_count",
     "wikis_checksum_failed_count",
     "wikis_checksummed_in_percentage",
+    "merge_request_diffs_count",
+    "merge_request_diffs_checksum_failed_count",
+    "merge_request_diffs_checksummed_in_percentage",
+    "merge_request_diffs_checksummed_count",
+    "merge_request_diffs_registry_count",
+    "merge_request_diffs_failed_count",
+    "merge_request_diffs_synced_count",
+    "merge_request_diffs_synced_in_percentage",
     "package_files_count",
     "package_files_checksum_failed_count",
     "package_files_checksummed_in_percentage",
@@ -143,6 +151,14 @@
     "wikis_checksummed_count": { "type": ["integer", "null"] },
     "wikis_checksum_failed_count": { "type": ["integer", "null"] },
     "wikis_checksummed_in_percentage": { "type": "string" },
+    "merge_request_diffs_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_checksummed_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_checksum_failed_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_registry_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_failed_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_synced_count": { "type": ["integer", "null"] },
+    "merge_request_diffs_synced_in_percentage": { "type": "string" },
+    "merge_request_diffs_checksummed_in_percentage": { "type": "string" },
     "package_files_count": { "type": ["integer", "null"] },
     "package_files_checksummed_count": { "type": ["integer", "null"] },
     "package_files_checksum_failed_count": { "type": ["integer", "null"] },

ee/spec/models/ee/merge_request_diff_spec.rb

@@ -3,5 +3,124 @@
 require 'spec_helper'
 
 RSpec.describe MergeRequestDiff do
+  using RSpec::Parameterized::TableSyntax
+  include EE::GeoHelpers
+
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :repository, group: group) }
+  let_it_be(:other_project) { create(:project, :repository) }
+
   it { is_expected.to respond_to(:log_geo_deleted_event) }
+
+  before do
+    stub_external_diffs_setting(enabled: true)
+  end
+
+  describe '.with_files_stored_locally' do
+    it 'includes states with local storage' do
+      create(:merge_request, source_project: project)
+
+      expect(described_class.with_files_stored_locally).to have_attributes(count: 1)
+    end
+
+    it 'excludes states with local storage' do
+      stub_external_diffs_object_storage(ExternalDiffUploader, direct_upload: true)
+
+      create(:merge_request, source_project: project)
+
+      expect(described_class.with_files_stored_locally).to have_attributes(count: 0)
+    end
+  end
+
+  describe '.has_external_diffs' do
+    it 'only includes diffs with files' do
+      diff_with_files = create(:merge_request).merge_request_diff
+      create(:merge_request, :without_diffs)
+
+      expect(described_class.has_external_diffs).to contain_exactly(diff_with_files)
+    end
+
+    it 'only includes externally stored diffs' do
+      external_diff = create(:merge_request).merge_request_diff
+
+      stub_external_diffs_setting(enabled: false)
+
+      create(:merge_request, :without_diffs)
+
+      expect(described_class.has_external_diffs).to contain_exactly(external_diff)
+    end
+  end
+
+  describe '.project_id_in' do
+    it 'only includes diffs for the provided projects' do
+      diff = create(:merge_request, source_project: project).merge_request_diff
+      other_diff = create(:merge_request, source_project: other_project).merge_request_diff
+      create(:merge_request)
+
+      expect(described_class.project_id_in([project, other_project])).to contain_exactly(diff, other_diff)
+    end
+  end
+
+  describe '.replicables_for_geo_node' do
+    context 'without selective sync or object storage' do
+      let(:secondary) { create(:geo_node) }
+
+      before do
+        stub_current_geo_node(secondary)
+      end
+
+      it 'excludes diffs stored in the database' do
+        stub_external_diffs_setting(enabled: false)
+
+        create(:merge_request, source_project: project)
+
+        expect(described_class.replicables_for_geo_node).to be_empty
+      end
+
+      it 'excludes empty diffs' do
+        create(:merge_request, source_project: create(:project))
+
+        expect(described_class.replicables_for_geo_node).to be_empty
+      end
+    end
+
+    where(:selective_sync_enabled, :object_storage_sync_enabled, :diff_in_object_storage, :synced_states) do
+      true  | true  | true  | 1
+      true  | true  | false | 1
+      true  | false | true  | 0
+      true  | false | false | 1
+      false | false | false | 2
+      false | false | true  | 0
+      false | true  | true  | 2
+      false | true  | false | 2
+      true  | true  | false | 1
+    end
+
+    with_them do
+      let(:secondary) do
+        node = build(:geo_node, sync_object_storage: object_storage_sync_enabled)
+
+        if selective_sync_enabled
+          node.selective_sync_type = 'namespaces'
+          node.namespaces = [group]
+        end
+
+        node.save!
+        node
+      end
+
+      before do
+        stub_current_geo_node(secondary)
+
+        stub_external_diffs_object_storage(ExternalDiffUploader, direct_upload: true) if diff_in_object_storage
+
+        create(:merge_request, source_project: project)
+        create(:merge_request, source_project: other_project)
+      end
+
+      it 'returns the proper number of merge request diff states' do
+        expect(described_class.replicables_for_geo_node).to have_attributes(count: synced_states)
+      end
+    end
+  end
 end

ee/spec/models/geo/merge_request_diff_registry_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Geo::MergeRequestDiffRegistry, :geo, type: :model do
+  let_it_be(:registry) { create(:geo_merge_request_diff_registry) }
+
+  specify 'factory is valid' do
+    expect(registry).to be_valid
+  end
+
+  include_examples 'a Geo framework registry'
+end

ee/spec/models/geo_node_status_spec.rb

@@ -1147,6 +1147,7 @@ RSpec.describe GeoNodeStatus, :geo do
   end
 
   where(:replicator, :model_factory, :registry_factory) do
+    Geo::MergeRequestDiffReplicator | :external_merge_request_diff | :geo_merge_request_diff_registry
     Geo::PackageFileReplicator | :package_file | :geo_package_file_registry
     Geo::TerraformStateReplicator | :terraform_state | :geo_terraform_state_registry
   end

ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Geo::MergeRequestDiffReplicator do
+  let(:model_record) { build(:merge_request_diff, :external) }
+
+  include_examples 'a blob replicator'
+end

ee/spec/services/geo/registry_consistency_service_spec.rb

@@ -10,14 +10,17 @@ RSpec.describe Geo::RegistryConsistencyService, :geo, :use_clean_rails_memory_st
   before do
     stub_current_geo_node(secondary)
     stub_registry_replication_config(enabled: true)
+    stub_external_diffs_setting(enabled: true)
   end
 
   def model_class_factory_name(registry_class)
-    return :project_with_design if registry_class == ::Geo::DesignRegistry
-    return :package_file_with_file if registry_class == ::Geo::PackageFileRegistry
-    return :terraform_state if registry_class == ::Geo::TerraformStateRegistry
+    default_factory_name = registry_class::MODEL_CLASS.underscore.tr('/', '_').to_sym
 
-    registry_class::MODEL_CLASS.underscore.tr('/', '_').to_sym
+    { Geo::DesignRegistry => :project_with_design,
+      Geo::MergeRequestDiffRegistry => :external_merge_request_diff,
+      Geo::PackageFileRegistry => :package_file_with_file,
+      Geo::TerraformStateRegistry => :terraform_state }
+      .fetch(registry_class, default_factory_name)
   end
 
   shared_examples 'registry consistency service' do |klass|

ee/spec/workers/geo/secondary/registry_consistency_worker_spec.rb

@@ -78,34 +78,54 @@ RSpec.describe Geo::Secondary::RegistryConsistencyWorker, :geo do
 
     # Somewhat of an integration test
     it 'creates missing registries for each registry class' do
-      job_artifact = create(:ci_job_artifact)
-      lfs_object = create(:lfs_object)
       project = create(:project)
+      container_repository = create(:container_repository, project: project)
       create(:design, project: project)
-      upload = create(:upload)
+      job_artifact = create(:ci_job_artifact)
+      lfs_object = create(:lfs_object)
+      merge_request_diff = create(:merge_request_diff, :external)
       package_file = create(:conan_package_file, :conan_package)
-      container_repository = create(:container_repository, project: project)
       terraform_state = create(:terraform_state, project: project)
+      upload = create(:upload)
 
-      expect(Geo::LfsObjectRegistry.where(lfs_object_id: lfs_object.id).count).to eq(0)
-      expect(Geo::JobArtifactRegistry.where(artifact_id: job_artifact.id).count).to eq(0)
-      expect(Geo::ProjectRegistry.where(project_id: project.id).count).to eq(0)
+      expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(0)
       expect(Geo::DesignRegistry.where(project_id: project.id).count).to eq(0)
-      expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(0)
+      expect(Geo::JobArtifactRegistry.where(artifact_id: job_artifact.id).count).to eq(0)
+      expect(Geo::LfsObjectRegistry.where(lfs_object_id: lfs_object.id).count).to eq(0)
+      expect(Geo::MergeRequestDiffRegistry.where(merge_request_diff_id: merge_request_diff.id).count).to eq(0)
       expect(Geo::PackageFileRegistry.where(package_file_id: package_file.id).count).to eq(0)
-      expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(0)
+      expect(Geo::ProjectRegistry.where(project_id: project.id).count).to eq(0)
       expect(Geo::TerraformStateRegistry.where(terraform_state_id: terraform_state.id).count).to eq(0)
+      expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(0)
 
       subject.perform
 
-      expect(Geo::LfsObjectRegistry.where(lfs_object_id: lfs_object.id).count).to eq(1)
-      expect(Geo::JobArtifactRegistry.where(artifact_id: job_artifact.id).count).to eq(1)
-      expect(Geo::ProjectRegistry.where(project_id: project.id).count).to eq(1)
+      expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(1)
       expect(Geo::DesignRegistry.where(project_id: project.id).count).to eq(1)
-      expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(1)
+      expect(Geo::JobArtifactRegistry.where(artifact_id: job_artifact.id).count).to eq(1)
+      expect(Geo::LfsObjectRegistry.where(lfs_object_id: lfs_object.id).count).to eq(1)
+      expect(Geo::MergeRequestDiffRegistry.where(merge_request_diff_id: merge_request_diff.id).count).to eq(1)
       expect(Geo::PackageFileRegistry.where(package_file_id: package_file.id).count).to eq(1)
-      expect(Geo::ContainerRepositoryRegistry.where(container_repository_id: container_repository.id).count).to eq(1)
+      expect(Geo::ProjectRegistry.where(project_id: project.id).count).to eq(1)
       expect(Geo::TerraformStateRegistry.where(terraform_state_id: terraform_state.id).count).to eq(1)
+      expect(Geo::UploadRegistry.where(file_id: upload.id).count).to eq(1)
+    end
+
+    context 'when geo_merge_request_diff_replication is disabled' do
+      before do
+        stub_feature_flags(geo_merge_request_diff_replication: false)
+      end
+
+      it 'returns false' do
+        expect(subject.perform).to be_falsey
+      end
+
+      it 'does not execute RegistryConsistencyService for merge request diffs' do
+        allow(Geo::RegistryConsistencyService).to receive(:new).and_call_original
+        expect(Geo::RegistryConsistencyService).not_to receive(:new).with(Geo::MergeRequestDiffRegistry, batch_size: batch_size)
+
+        subject.perform
+      end
     end
 
     context 'when geo_terraform_state_replication is disabled' do

lib/gitlab/import_export/project/import_export.yml

@@ -190,8 +190,20 @@ excluded_attributes:
     - :stored_externally
     - :external_diff_store
     - :merge_request_id
+    - :verification_retry_at
+    - :verified_at
+    - :verification_retry_count
+    - :verification_checksum
+    - :verification_failure
   merge_request_diff_commits:
     - :merge_request_diff_id
+  merge_request_diff_detail:
+    - :merge_request_diff_id
+    - :verification_retry_at
+    - :verified_at
+    - :verification_retry_count
+    - :verification_checksum
+    - :verification_failure
   merge_request_diff_files:
     - :diff
     - :external_diff_offset

spec/factories/merge_request_diffs.rb

@@ -2,12 +2,20 @@
 
 FactoryBot.define do
   factory :merge_request_diff do
-    association :merge_request
+    association :merge_request, :without_merge_request_diff
     state { :collected }
     commits_count { 1 }
 
     base_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
     head_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
     start_commit_sha { Digest::SHA1.hexdigest(SecureRandom.hex) }
+
+    trait :external do
+      external_diff { fixture_file_upload("spec/fixtures/doc_sample.txt", "plain/txt") }
+      stored_externally { true }
+      importing { true } # this avoids setting the state to 'empty'
+    end
+
+    factory :external_merge_request_diff, traits: [:external]
   end
 end

spec/factories/merge_requests.rb

@@ -92,6 +92,16 @@ FactoryBot.define do
       target_branch { "feature_two" }
     end
 
+    trait(:without_merge_request_diff) do
+      after(:build) do |_|
+        MergeRequest.skip_callback(:create, :after, :ensure_merge_request_diff)
+      end
+
+      after(:create) do |_|
+        MergeRequest.set_callback(:create, :after, :ensure_merge_request_diff)
+      end
+    end
+
     trait :locked do
       state_id { MergeRequest.available_states[:locked] }
     end

spec/lib/gitlab/import_export/all_models.yml

@@ -179,9 +179,12 @@ external_pull_requests:
 merge_request_diff:
 - merge_request
 - merge_request_diff_commits
+- merge_request_diff_detail
 - merge_request_diff_files
 merge_request_diff_commits:
 - merge_request_diff
+merge_request_diff_detail:
+- merge_request_diff
 merge_request_diff_files:
 - merge_request_diff
 merge_request_context_commits: