Merge branch 'ag-backend-changes-mr-diffs-verification' into 'master'

Geo: Add verification for MR diffs using SSF [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!63309

config/initializers_before_autoloader/000_inflections.rb

@@ -23,6 +23,7 @@ ActiveSupport::Inflector.inflections do |inflect|
     group_wiki_repository_registry
     job_artifact_registry
     lfs_object_registry
+    merge_request_diff_registry
     package_file_registry
     pipeline_artifact_registry
     project_auto_devops

doc/administration/monitoring/prometheus/gitlab_metrics.md

@@ -229,11 +229,15 @@ configuration option in `gitlab.yml`. These metrics are served from the
 | `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_total`       | Gauge   | 13.12 | Number of merge request diffs tried to checksum on primary | `url` |
+| `geo_merge_request_diffs_checksummed`          | Gauge   | 13.4  | Number of merge request diffs successfully 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` |
+| `geo_merge_request_diffs_verification_total`   | Gauge   | 13.12 | Number of merge request diffs verifications tried on secondary | `url` |
+| `geo_merge_request_diffs_verified`             | Gauge   | 13.12 | Number of merge request diffs verified on secondary | `url` |
+| `geo_merge_request_diffs_verification_failed`  | Gauge   | 13.12 | Number of merge request diffs verifications failed on secondary | `url` |
 | `geo_snippet_repositories`                     | Gauge   | 13.4  | Number of snippets on primary | `url` |
 | `geo_snippet_repositories_checksummed`         | Gauge   | 13.4  | Number of snippets checksummed on primary | `url` |
 | `geo_snippet_repositories_checksum_failed`     | Gauge   | 13.4  | Number of snippets failed to calculate the checksum on primary | `url` |

ee/app/models/ee/merge_request_diff.rb

@@ -7,29 +7,34 @@ module EE
     prepended do
       include ::Gitlab::Geo::ReplicableModel
       include ObjectStorable
+      include ::Gitlab::Geo::VerificationState
 
       STORE_COLUMN = :external_diff_store
 
       with_replicator Geo::MergeRequestDiffReplicator
 
-      has_one :merge_request_diff_detail, inverse_of: :merge_request_diff
+      has_one :merge_request_diff_detail, autosave: true, 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=,
+               :verification_state=, :verification_state,
+               :verification_started_at=, :verification_started_at,
         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 :verification_succeeded, -> { where(merge_request_diff_detail: ::MergeRequestDiffDetail.verification_succeeded) }
-      scope :verification_failed, -> { where(merge_request_diff_detail: ::MergeRequestDiffDetail.verification_failed) }
       scope :available_replicables, -> { has_external_diffs }
+      scope :with_verification_state, ->(state) { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_state: verification_state_value(state) }) }
+      scope :checksummed, -> { joins(:merge_request_diff_detail).where.not(merge_request_diff_details: { verification_checksum: nil } ) }
+      scope :not_checksummed, -> { joins(:merge_request_diff_detail).where(merge_request_diff_details: { verification_checksum: nil } ) }
     end
 
     class_methods do
+      extend ::Gitlab::Utils::Override
+
       # @param primary_key_in [Range, MergeRequestDiff] arg to pass to primary_key_in scope
       # @return [ActiveRecord::Relation<MergeRequestDiff>] everything that should be synced to this node, restricted by primary key
       def replicables_for_current_secondary(primary_key_in)
@@ -40,6 +45,21 @@ module EE
                              .merge(object_storage_scope(node))
       end
 
+      override :verification_state_table_name
+      def verification_state_table_name
+        'merge_request_diff_details'
+      end
+
+      override :verification_state_model_key
+      def verification_state_model_key
+        'merge_request_diff_id'
+      end
+
+      override :verification_arel_table
+      def verification_arel_table
+        MergeRequestDiffDetail.arel_table
+      end
+
       private
 
       def object_storage_scope(node)

ee/app/models/geo/merge_request_diff_registry.rb

@@ -2,6 +2,7 @@
 
 class Geo::MergeRequestDiffRegistry < Geo::BaseRegistry
   include ::Geo::ReplicableRegistry
+  include ::Geo::VerifiableRegistry
 
   MODEL_CLASS = ::MergeRequestDiff
   MODEL_FOREIGN_KEY = :merge_request_diff_id

ee/app/models/merge_request_diff_detail.rb

@@ -4,12 +4,4 @@ class MergeRequestDiffDetail < ApplicationRecord
   self.primary_key = :merge_request_diff_id
 
   belongs_to :merge_request_diff, inverse_of: :merge_request_diff_detail
-
-  # Temporarily defining `verification_succeeded` and
-  # `verification_failed` for unverified models while verification is
-  # under development to avoid breaking GeoNodeStatusCheck code.
-  # TODO: Remove these after including `Gitlab::Geo::VerificationState` on
-  # all models. https://gitlab.com/gitlab-org/gitlab/-/issues/280768
-  scope :verification_succeeded, -> { none }
-  scope :verification_failed, -> { none }
 end

ee/app/replicators/geo/merge_request_diff_replicator.rb

@@ -24,5 +24,10 @@ module Geo
     def checksummable?
       model_record.stored_externally? && super
     end
+
+    override :verification_feature_flag_enabled?
+    def self.verification_feature_flag_enabled?
+      Feature.enabled?(:geo_merge_request_diff_verification, default_enabled: :yaml)
+    end
   end
 end

ee/config/feature_flags/development/geo_merge_request_diff_verification.yml

+---
+name: geo_merge_request_diff_verification
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/63309/
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330023
+milestone: '14.0'
+type: development
+group: group::geo
+default_enabled: false

ee/lib/gitlab/geo/verification_state.rb

@@ -36,7 +36,7 @@ module Gitlab
         scope :checksummed, -> { where.not(verification_checksum: nil) }
         scope :not_checksummed, -> { where(verification_checksum: nil) }
         scope :verification_timed_out, -> { verification_started.where("verification_started_at < ?", VERIFICATION_TIMEOUT.ago) }
-        scope :retry_due, -> { where(arel_table[:verification_retry_at].eq(nil).or(arel_table[:verification_retry_at].lt(Time.current))) }
+        scope :retry_due, -> { where(verification_arel_table[:verification_retry_at].eq(nil).or(verification_arel_table[:verification_retry_at].lt(Time.current))) }
         scope :needs_verification, -> { available_verifiables.merge(with_verification_state(:verification_pending).or(with_verification_state(:verification_failed).retry_due)) }
         scope :needs_reverification, -> { verification_succeeded.where("verified_at < ?", ::Gitlab::Geo.current_node.minimum_reverification_interval.days.ago) }
         # rubocop:enable CodeReuse/ActiveRecord
@@ -183,7 +183,7 @@ module Gitlab
           started_enum_value = VERIFICATION_STATE_VALUES[:verification_started]
 
           <<~SQL.squish
-            UPDATE #{table_name}
+            UPDATE #{verification_state_table_name}
             SET "verification_state" = #{started_enum_value},
               "verification_started_at" = NOW()
             WHERE #{self.verification_state_model_key} IN (#{start_verification_batch_subselect(relation).to_sql})
@@ -206,10 +206,28 @@ module Gitlab
         end
 
         # Overridden in ReplicableRegistry
+        # This method can also be overriden in the replicable model class that
+        # includes this concern to specify the primary key of the database
+        # table that stores verification state
+        # See module EE::MergeRequestDiff for example
         def verification_state_model_key
           self.primary_key
         end
 
+        # Override this method in the class that includes this concern to specify
+        # a different database table to store verification state
+        # See module EE::MergeRequestDiff for example
+        def verification_state_table_name
+          table_name
+        end
+
+        # Override this method in the class that includes this concern to specify
+        # a different arel table to store verification state
+        # See module EE::MergeRequestDiff for example
+        def verification_arel_table
+          arel_table
+        end
+
         # Fail verification for records which started verification a long time ago
         def fail_verification_timeouts
           attrs = {
@@ -269,7 +287,7 @@ module Gitlab
           pending_enum_value = VERIFICATION_STATE_VALUES[:verification_pending]
 
           <<~SQL.squish
-            UPDATE #{table_name}
+            UPDATE #{verification_state_table_name}
             SET "verification_state" = #{pending_enum_value}
             WHERE #{self.verification_state_model_key} IN (#{relation.select(self.verification_state_model_key).to_sql})
           SQL

ee/spec/factories/geo/external_merge_request_diff_registry.rb

@@ -23,5 +23,11 @@ FactoryBot.define do
       last_synced_at { 1.day.ago }
       retry_count { 0 }
     end
+
+    trait :verification_succeeded do
+      verification_checksum { 'e079a831cab27bcda7d81cd9b48296d0c3dd92ef' }
+      verification_state { Geo::MergeRequestDiffRegistry.verification_state_value(:verification_succeeded) }
+      verified_at { 5.days.ago }
+    end
   end
 end

ee/spec/factories/merge_request_diffs.rb

@@ -9,5 +9,17 @@ FactoryBot.modify do
     trait(:checksum_failure) do
       association :merge_request_diff_detail, :checksum_failure, strategy: :build
     end
+
+    trait(:verification_succeeded) do
+      with_file
+      verification_checksum { 'abc' }
+      verification_state { ::MergeRequestDiff.verification_state_value(:verification_succeeded) }
+    end
+
+    trait(:verification_failed) do
+      with_file
+      verification_failure { 'Could not calculate the checksum' }
+      verification_state { ::MergeRequestDiff.verification_state_value(:verification_failed) }
+    end
   end
 end

ee/spec/models/geo/merge_request_diff_registry_spec.rb

@@ -10,4 +10,5 @@ RSpec.describe Geo::MergeRequestDiffRegistry, :geo, type: :model do
   end
 
   include_examples 'a Geo framework registry'
+  include_examples 'a Geo verifiable registry'
 end

ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb

@@ -6,4 +6,5 @@ RSpec.describe Geo::MergeRequestDiffReplicator do
   let(:model_record) { build(:merge_request_diff, :external) }
 
   include_examples 'a blob replicator'
+  include_examples 'a verifiable replicator'
 end