From 9d390d48b98be62beb953ce45fa3b437101cdf6c Mon Sep 17 00:00:00 2001 From: Aakriti Gupta Date: Tue, 1 Jun 2021 15:35:15 +0200 Subject: [PATCH] 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 --- .../000_inflections.rb | 1 + .../monitoring/prometheus/gitlab_metrics.md | 6 +++- ee/app/models/ee/merge_request_diff.rb | 28 ++++++++++++++++--- .../models/geo/merge_request_diff_registry.rb | 1 + ee/app/models/merge_request_diff_detail.rb | 8 ------ .../geo/merge_request_diff_replicator.rb | 5 ++++ .../geo_merge_request_diff_verification.yml | 8 ++++++ ee/lib/gitlab/geo/verification_state.rb | 24 ++++++++++++++-- .../external_merge_request_diff_registry.rb | 6 ++++ ee/spec/factories/merge_request_diffs.rb | 12 ++++++++ .../geo/merge_request_diff_registry_spec.rb | 1 + .../geo/merge_request_diff_replicator_spec.rb | 1 + 12 files changed, 85 insertions(+), 16 deletions(-) create mode 100644 ee/config/feature_flags/development/geo_merge_request_diff_verification.yml diff --git a/config/initializers_before_autoloader/000_inflections.rb b/config/initializers_before_autoloader/000_inflections.rb index de8f79b9a2954a..39905adf3902bc 100644 --- a/config/initializers_before_autoloader/000_inflections.rb +++ b/config/initializers_before_autoloader/000_inflections.rb @@ -23,6 +23,7 @@ group_wiki_repository_registry job_artifact_registry lfs_object_registry + merge_request_diff_registry package_file_registry pipeline_artifact_registry project_auto_devops diff --git a/doc/administration/monitoring/prometheus/gitlab_metrics.md b/doc/administration/monitoring/prometheus/gitlab_metrics.md index 9a168b84869fb5..f61b302882e691 100644 --- a/doc/administration/monitoring/prometheus/gitlab_metrics.md +++ b/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` | diff --git a/ee/app/models/ee/merge_request_diff.rb b/ee/app/models/ee/merge_request_diff.rb index 39bafd71ae2a5b..acc576baf21c1d 100644 --- a/ee/app/models/ee/merge_request_diff.rb +++ b/ee/app/models/ee/merge_request_diff.rb @@ -7,29 +7,34 @@ module MergeRequestDiff 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] everything that should be synced to this node, restricted by primary key def replicables_for_current_secondary(primary_key_in) @@ -40,6 +45,21 @@ def replicables_for_current_secondary(primary_key_in) .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) diff --git a/ee/app/models/geo/merge_request_diff_registry.rb b/ee/app/models/geo/merge_request_diff_registry.rb index d1309131fc4003..752ac4ac7a4500 100644 --- a/ee/app/models/geo/merge_request_diff_registry.rb +++ b/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 diff --git a/ee/app/models/merge_request_diff_detail.rb b/ee/app/models/merge_request_diff_detail.rb index 69cd95fbab60e4..82c7fc8ca82d38 100644 --- a/ee/app/models/merge_request_diff_detail.rb +++ b/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 diff --git a/ee/app/replicators/geo/merge_request_diff_replicator.rb b/ee/app/replicators/geo/merge_request_diff_replicator.rb index db8395bd520506..dbc16816bb19f0 100644 --- a/ee/app/replicators/geo/merge_request_diff_replicator.rb +++ b/ee/app/replicators/geo/merge_request_diff_replicator.rb @@ -24,5 +24,10 @@ def carrierwave_uploader 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 diff --git a/ee/config/feature_flags/development/geo_merge_request_diff_verification.yml b/ee/config/feature_flags/development/geo_merge_request_diff_verification.yml new file mode 100644 index 00000000000000..425561057387eb --- /dev/null +++ b/ee/config/feature_flags/development/geo_merge_request_diff_verification.yml @@ -0,0 +1,8 @@ +--- +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 diff --git a/ee/lib/gitlab/geo/verification_state.rb b/ee/lib/gitlab/geo/verification_state.rb index dea1f0c53600fd..c945d5f7b29101 100644 --- a/ee/lib/gitlab/geo/verification_state.rb +++ b/ee/lib/gitlab/geo/verification_state.rb @@ -36,7 +36,7 @@ module VerificationState 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 @@ def start_verification_batch_query(relation) 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 @@ def start_verification_batch_subselect(relation) 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 @@ def mark_as_verification_pending_query(relation) 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 diff --git a/ee/spec/factories/geo/external_merge_request_diff_registry.rb b/ee/spec/factories/geo/external_merge_request_diff_registry.rb index 5979a605d7019b..d4e82eb6978b8d 100644 --- a/ee/spec/factories/geo/external_merge_request_diff_registry.rb +++ b/ee/spec/factories/geo/external_merge_request_diff_registry.rb @@ -23,5 +23,11 @@ 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 diff --git a/ee/spec/factories/merge_request_diffs.rb b/ee/spec/factories/merge_request_diffs.rb index 31959dc4e3136d..48a609a21a90d8 100644 --- a/ee/spec/factories/merge_request_diffs.rb +++ b/ee/spec/factories/merge_request_diffs.rb @@ -9,5 +9,17 @@ 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 diff --git a/ee/spec/models/geo/merge_request_diff_registry_spec.rb b/ee/spec/models/geo/merge_request_diff_registry_spec.rb index d9e5f404bae2c7..255557da4e2eb4 100644 --- a/ee/spec/models/geo/merge_request_diff_registry_spec.rb +++ b/ee/spec/models/geo/merge_request_diff_registry_spec.rb @@ -10,4 +10,5 @@ end include_examples 'a Geo framework registry' + include_examples 'a Geo verifiable registry' end diff --git a/ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb b/ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb index 1e708f0ff49760..92d58a4352b0bf 100644 --- a/ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb +++ b/ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb @@ -6,4 +6,5 @@ let(:model_record) { build(:merge_request_diff, :external) } include_examples 'a blob replicator' + include_examples 'a verifiable replicator' end -- GitLab