Commit 9d390d48 authored by Aakriti Gupta's avatar Aakriti Gupta Committed by Mike Kozono

Add verification for MR diffs using SSF

- Add tests for verification
- Add verification related metrics
- Override methods in VerificationState to use
verification table instead of the replicable
model

Changelog: changed
EE: true
parent 8f0fce0e
......@@ -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
......
......@@ -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` |
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......@@ -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
---
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
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment