diff --git a/config/initializers_before_autoloader/000_inflections.rb b/config/initializers_before_autoloader/000_inflections.rb index de8f79b9a2954afc3a75e0072f0a21da3b8c5d19..39905adf3902bc7bd09aa92e35a90934b74dd75c 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 9a168b84869fb50870395a58b85a96f79f300b40..f61b302882e69101c13f13eb95766da524eb4d72 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 39bafd71ae2a5b543d4fe3efedfb91538418808f..acc576baf21c1da17451635605bfd671a9ed63b3 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 d1309131fc4003d6d95fc41f4ccec1d90aae1603..752ac4ac7a4500978ca1c8028bc9f220b616cb55 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 69cd95fbab60e4d6b1c13f8578906b1336cfdf75..82c7fc8ca82d38ad4a8985470bf7b41e1b31ac9f 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 db8395bd520506a52f79ba91c81f54a722464b54..dbc16816bb19f0f5b36398368ebc57c84e623a5a 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 0000000000000000000000000000000000000000..425561057387eb4daf4fea2677ecfa486c118496 --- /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 dea1f0c53600fd5eb840928e3f1aac876f7d2385..c945d5f7b29101347abb8bb4e814bfc21ab3634a 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 5979a605d7019b97f1c0ce60776179e358f1968c..d4e82eb6978b8dff607bed6bf0b261d3a1f39d51 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 31959dc4e3136d25feb25538c9f9c8c6684a1a73..48a609a21a90d84f2698be183840e092cf4acbef 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 d9e5f404bae2c783cae513593314fbeae721ab7e..255557da4e2eb4d6aade956f239b8d59605d8ad7 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 1e708f0ff497606a3fc977fa9a6fa2714e40b9db..92d58a4352b0bff811e46c418af5b20981a4c2e4 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