From 1377018a11d0be309f1224710cc8b3a5481423d3 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:27:30 -0700 Subject: [PATCH 1/6] Refactor verify after save In preparation for mutable replicables, especially Git repos. --- .../concerns/geo/blob_replicator_strategy.rb | 9 ++++ .../concerns/geo/verifiable_replicator.rb | 22 +++++++--- .../verifiable_replicator_shared_examples.rb | 42 ++++++++++++++++--- 3 files changed, 62 insertions(+), 11 deletions(-) diff --git a/ee/app/models/concerns/geo/blob_replicator_strategy.rb b/ee/app/models/concerns/geo/blob_replicator_strategy.rb index af6d4aae11381a..b76aa9390b1851 100644 --- a/ee/app/models/concerns/geo/blob_replicator_strategy.rb +++ b/ee/app/models/concerns/geo/blob_replicator_strategy.rb @@ -92,5 +92,14 @@ def deleted_params def checksummable? carrierwave_uploader.file_storage? && file_exists? end + + # Return whether it's immutable + # + # @return [Boolean] whether the replicable is immutable + def immutable? + # Most blobs are supposed to be immutable. + # Override this in your specific Replicator class if needed. + true + end end end diff --git a/ee/app/models/concerns/geo/verifiable_replicator.rb b/ee/app/models/concerns/geo/verifiable_replicator.rb index 2bd81dd4c08a61..6be2508e1842bc 100644 --- a/ee/app/models/concerns/geo/verifiable_replicator.rb +++ b/ee/app/models/concerns/geo/verifiable_replicator.rb @@ -189,7 +189,7 @@ def consume_event_checksum_succeeded(**params) # Schedules a verification job after a model record is created/updated def after_verifiable_update - verify_async if should_primary_verify? + verify_async if should_primary_verify_after_save? end def verify_async @@ -242,10 +242,22 @@ def calculate_checksum private - def should_primary_verify? - self.class.verification_enabled? && - primary_checksum.nil? && # Some models may populate this as part of creating the record - checksummable? + def should_primary_verify_after_save? + return false unless self.class.verification_enabled? + + # Optimization: If the data is immutable, then there is no need to + # recalculate checksum when a record is created (some models calculate + # checksum as part of creation) or updated. Note that reverification + # should still run as usual. + return false if immutable? && primary_checksum.present? + + checksummable? + end + + # @abstract + # @return [Boolean] whether the replicable is supposed to be immutable + def immutable? + raise NotImplementedError, "#{self.class} does not implement #{__method__}" end # @abstract diff --git a/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb index b91da6f3ead127..801ea4698f34cf 100644 --- a/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb @@ -341,14 +341,44 @@ end describe '#after_verifiable_update' do - it 'calls verify_async if needed' do - allow(described_class).to receive(:verification_enabled?).and_return(true) - allow(replicator).to receive(:primary_checksum).and_return(nil) - allow(replicator).to receive(:checksummable?).and_return(true) + using RSpec::Parameterized::TableSyntax + + where(:verification_enabled, :immutable, :checksum, :checksummable, :expect_verify_async) do + true | true | nil | true | true + true | true | nil | false | false + true | true | 'abc123' | true | false + true | true | 'abc123' | false | false + true | false | nil | true | true + true | false | nil | false | false + true | false | 'abc123' | true | true + true | false | 'abc123' | false | false + false | true | nil | true | false + false | true | nil | false | false + false | true | 'abc123' | true | false + false | true | 'abc123' | false | false + false | false | nil | true | false + false | false | nil | false | false + false | false | 'abc123' | true | false + false | false | 'abc123' | false | false + end + + with_them do + before do + allow(described_class).to receive(:verification_enabled?).and_return(verification_enabled) + allow(replicator).to receive(:immutable?).and_return(immutable) + allow(replicator).to receive(:primary_checksum).and_return(checksum) + allow(replicator).to receive(:checksummable?).and_return(checksummable) + end - expect(replicator).to receive(:verify_async) + it 'calls verify_async only if needed' do + if expect_verify_async + expect(replicator).to receive(:verify_async) + else + expect(replicator).not_to receive(:verify_async) + end - replicator.after_verifiable_update + replicator.after_verifiable_update + end end end -- GitLab From 85f0382274d7e0ad4f8789f534170781342314e4 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:31:55 -0700 Subject: [PATCH 2/6] Fix test setup The negative test was improperly passing, because model_record didn't exist. --- .../repository_replicator_strategy_shared_examples.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb index b42e5a71fd123f..71227b812db89e 100644 --- a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb @@ -71,14 +71,16 @@ end describe 'updated event consumption' do + before do + model_record.save! + end + context 'in replicables_for_current_secondary list' do it 'runs SnippetRepositorySyncService service' do - model_record.save! - + allow(replicator).to receive(:in_replicables_for_current_secondary?).and_return(true) sync_service = double expect(sync_service).to receive(:execute) - expect(::Geo::FrameworkRepositorySyncService) .to receive(:new).with(replicator) .and_return(sync_service) @@ -89,6 +91,8 @@ context 'not in replicables_for_current_secondary list' do it 'runs SnippetRepositorySyncService service' do + allow(replicator).to receive(:in_replicables_for_current_secondary?).and_return(false) + expect(::Geo::FrameworkRepositorySyncService) .not_to receive(:new) -- GitLab From 6a8e9b2ad4d68e3fa77b3240453da912ef4b9ce7 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:33:22 -0700 Subject: [PATCH 3/6] Isolate the test In preparation for adding verification to another replicator. --- ee/spec/lib/gitlab/geo_spec.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/ee/spec/lib/gitlab/geo_spec.rb b/ee/spec/lib/gitlab/geo_spec.rb index cd618c5873a9f4..d78153728ea764 100644 --- a/ee/spec/lib/gitlab/geo_spec.rb +++ b/ee/spec/lib/gitlab/geo_spec.rb @@ -406,7 +406,7 @@ context 'when there are no Replicator classes with verification enabled' do it 'returns the total capacity' do - stub_feature_flags(geo_package_file_verification: false) + allow(described_class).to receive(:verification_enabled_replicator_classes).and_return([]) expect(described_class.verification_max_capacity_per_replicator_class).to eq(verification_max_capacity) end @@ -414,10 +414,20 @@ context 'when there is 1 Replicator class with verification enabled' do it 'returns half capacity' do + allow(described_class).to receive(:verification_enabled_replicator_classes).and_return(['a replicator class']) + expect(described_class.verification_max_capacity_per_replicator_class).to eq(verification_max_capacity / 2) end end + context 'when there are 2 Replicator classes with verification enabled' do + it 'returns a third of total capacity' do + allow(described_class).to receive(:verification_enabled_replicator_classes).and_return(['a replicator class', 'another replicator class']) + + expect(described_class.verification_max_capacity_per_replicator_class).to eq(verification_max_capacity / 3) + end + end + context 'when total capacity is set lower than the number of Replicators' do let(:verification_max_capacity) { 1 } -- GitLab From 5a07fda1403b65e4f27ebe5ded542038db2b95b7 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:36:10 -0700 Subject: [PATCH 4/6] Geo: Verify Snippets Leveraging the work done for Package File Verification. Most of the changes are to the database schemas, and to specs. Verification is behind the feature flag "geo_snippet_repository_verification". --- ..._and_started_at_to_snippet_repositories.rb | 12 ++++++ ...ication_indexes_to_snippet_repositories.rb | 27 +++++++++++++ db/schema_migrations/20210313045617 | 1 + db/schema_migrations/20210313045845 | 1 + db/structure.sql | 10 +++++ .../geo/repository_replicator_strategy.rb | 25 ++++++++++++ ee/app/models/ee/snippet_repository.rb | 1 + .../models/geo/snippet_repository_registry.rb | 1 + .../geo/snippet_repository_replicator.rb | 6 +++ .../geo_snippet_repository_verification.yml | 8 ++++ ...fication_to_snippet_repository_registry.rb | 17 +++++++++ ..._indexes_to_snippet_repository_registry.rb | 24 ++++++++++++ ee/db/geo/schema.rb | 38 ++++++++++++++++++- ee/lib/gitlab/geo/replicator.rb | 2 + .../lib/gitlab/geo/replicable_model_spec.rb | 1 + .../geo/snippet_repository_replicator_spec.rb | 4 +- ...lob_replicator_strategy_shared_examples.rb | 11 ++++-- .../replicable_model_shared_examples.rb | 12 +++--- ...ory_replicator_strategy_shared_examples.rb | 11 ++++-- .../verifiable_replicator_shared_examples.rb | 3 ++ .../backfill_snippet_repositories_spec.rb | 2 +- 21 files changed, 202 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20210313045617_add_verification_state_and_started_at_to_snippet_repositories.rb create mode 100644 db/migrate/20210313045845_add_verification_indexes_to_snippet_repositories.rb create mode 100644 db/schema_migrations/20210313045617 create mode 100644 db/schema_migrations/20210313045845 create mode 100644 ee/config/feature_flags/development/geo_snippet_repository_verification.yml create mode 100644 ee/db/geo/migrate/20210313050709_add_verification_to_snippet_repository_registry.rb create mode 100644 ee/db/geo/migrate/20210313051642_add_verification_indexes_to_snippet_repository_registry.rb diff --git a/db/migrate/20210313045617_add_verification_state_and_started_at_to_snippet_repositories.rb b/db/migrate/20210313045617_add_verification_state_and_started_at_to_snippet_repositories.rb new file mode 100644 index 00000000000000..e2d6dff23fa4a5 --- /dev/null +++ b/db/migrate/20210313045617_add_verification_state_and_started_at_to_snippet_repositories.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class AddVerificationStateAndStartedAtToSnippetRepositories < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + change_table(:snippet_repositories) do |t| + t.integer :verification_state, default: 0, limit: 2, null: false + t.column :verification_started_at, :datetime_with_timezone + end + end +end diff --git a/db/migrate/20210313045845_add_verification_indexes_to_snippet_repositories.rb b/db/migrate/20210313045845_add_verification_indexes_to_snippet_repositories.rb new file mode 100644 index 00000000000000..ebbc1126aa27bf --- /dev/null +++ b/db/migrate/20210313045845_add_verification_indexes_to_snippet_repositories.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +class AddVerificationIndexesToSnippetRepositories < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + VERIFICATION_STATE_INDEX_NAME = "index_snippet_repositories_verification_state" + PENDING_VERIFICATION_INDEX_NAME = "index_snippet_repositories_pending_verification" + FAILED_VERIFICATION_INDEX_NAME = "index_snippet_repositories_failed_verification" + NEEDS_VERIFICATION_INDEX_NAME = "index_snippet_repositories_needs_verification" + + disable_ddl_transaction! + + def up + add_concurrent_index :snippet_repositories, :verification_state, name: VERIFICATION_STATE_INDEX_NAME + add_concurrent_index :snippet_repositories, :verified_at, where: "(verification_state = 0)", order: { verified_at: 'ASC NULLS FIRST' }, name: PENDING_VERIFICATION_INDEX_NAME + add_concurrent_index :snippet_repositories, :verification_retry_at, where: "(verification_state = 3)", order: { verification_retry_at: 'ASC NULLS FIRST' }, name: FAILED_VERIFICATION_INDEX_NAME + add_concurrent_index :snippet_repositories, :verification_state, where: "(verification_state = 0 OR verification_state = 3)", name: NEEDS_VERIFICATION_INDEX_NAME + end + + def down + remove_concurrent_index_by_name :snippet_repositories, VERIFICATION_STATE_INDEX_NAME + remove_concurrent_index_by_name :snippet_repositories, PENDING_VERIFICATION_INDEX_NAME + remove_concurrent_index_by_name :snippet_repositories, FAILED_VERIFICATION_INDEX_NAME + remove_concurrent_index_by_name :snippet_repositories, NEEDS_VERIFICATION_INDEX_NAME + end +end diff --git a/db/schema_migrations/20210313045617 b/db/schema_migrations/20210313045617 new file mode 100644 index 00000000000000..d422d0ab5d2106 --- /dev/null +++ b/db/schema_migrations/20210313045617 @@ -0,0 +1 @@ +d1e6596e9c6825e29c50523dce60fd3d0b3c067c10e210f74640ba94f7938871 \ No newline at end of file diff --git a/db/schema_migrations/20210313045845 b/db/schema_migrations/20210313045845 new file mode 100644 index 00000000000000..8e2b5605f8bf98 --- /dev/null +++ b/db/schema_migrations/20210313045845 @@ -0,0 +1 @@ +bc6302444f7a0a858c821d971fc45a4ececd7b877020f8e920a244866c60b7a2 \ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 2bdf4873acf1f7..b37ebb6c49a084 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -17579,6 +17579,8 @@ CREATE TABLE snippet_repositories ( verified_at timestamp with time zone, verification_checksum bytea, verification_failure text, + verification_state smallint DEFAULT 0 NOT NULL, + verification_started_at timestamp with time zone, CONSTRAINT snippet_repositories_verification_failure_text_limit CHECK ((char_length(verification_failure) <= 255)) ); @@ -23874,10 +23876,18 @@ CREATE INDEX index_smartcard_identities_on_user_id ON smartcard_identities USING CREATE INDEX index_snippet_on_id_and_project_id ON snippets USING btree (id, project_id); +CREATE INDEX index_snippet_repositories_failed_verification ON snippet_repositories USING btree (verification_retry_at NULLS FIRST) WHERE (verification_state = 3); + +CREATE INDEX index_snippet_repositories_needs_verification ON snippet_repositories USING btree (verification_state) WHERE ((verification_state = 0) OR (verification_state = 3)); + CREATE UNIQUE INDEX index_snippet_repositories_on_disk_path ON snippet_repositories USING btree (disk_path); CREATE INDEX index_snippet_repositories_on_shard_id ON snippet_repositories USING btree (shard_id); +CREATE INDEX index_snippet_repositories_pending_verification ON snippet_repositories USING btree (verified_at NULLS FIRST) WHERE (verification_state = 0); + +CREATE INDEX index_snippet_repositories_verification_state ON snippet_repositories USING btree (verification_state); + CREATE INDEX index_snippet_repository_storage_moves_on_snippet_id ON snippet_repository_storage_moves USING btree (snippet_id); CREATE UNIQUE INDEX index_snippet_user_mentions_on_note_id ON snippet_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL); diff --git a/ee/app/models/concerns/geo/repository_replicator_strategy.rb b/ee/app/models/concerns/geo/repository_replicator_strategy.rb index 3caddd139551d4..62b02a8028a0d6 100644 --- a/ee/app/models/concerns/geo/repository_replicator_strategy.rb +++ b/ee/app/models/concerns/geo/repository_replicator_strategy.rb @@ -61,5 +61,30 @@ def deleted_params full_path: model_record.repository.full_path ) end + + # Returns a checksum of the repository refs as defined by Gitaly + # + # @return [String] checksum of the repository refs + def calculate_checksum + repository.checksum + rescue Gitlab::Git::Repository::NoRepository => e + log_error('Repository cannot be checksummed because it does not exist', e, self.replicable_params) + + raise + end + + # Return whether it's capable of generating a checksum of itself + # + # @return [Boolean] whether it can generate a checksum + def checksummable? + repository.exists? + end + + # Return whether it's immutable + # + # @return [Boolean] whether the replicable is immutable + def immutable? + false + end end end diff --git a/ee/app/models/ee/snippet_repository.rb b/ee/app/models/ee/snippet_repository.rb index 2d49bea402dfe2..54591fa1dbc743 100644 --- a/ee/app/models/ee/snippet_repository.rb +++ b/ee/app/models/ee/snippet_repository.rb @@ -6,6 +6,7 @@ module SnippetRepository prepended do include ::Gitlab::Geo::ReplicableModel + include ::Gitlab::Geo::VerificationState include FromUnion with_replicator Geo::SnippetRepositoryReplicator diff --git a/ee/app/models/geo/snippet_repository_registry.rb b/ee/app/models/geo/snippet_repository_registry.rb index 4b375713e37d34..81fa91e44eed04 100644 --- a/ee/app/models/geo/snippet_repository_registry.rb +++ b/ee/app/models/geo/snippet_repository_registry.rb @@ -2,6 +2,7 @@ class Geo::SnippetRepositoryRegistry < Geo::BaseRegistry include Geo::ReplicableRegistry + include ::Geo::VerifiableRegistry MODEL_CLASS = ::SnippetRepository MODEL_FOREIGN_KEY = :snippet_repository_id diff --git a/ee/app/replicators/geo/snippet_repository_replicator.rb b/ee/app/replicators/geo/snippet_repository_replicator.rb index 455339e59eab0b..546d6dbf5045cf 100644 --- a/ee/app/replicators/geo/snippet_repository_replicator.rb +++ b/ee/app/replicators/geo/snippet_repository_replicator.rb @@ -3,6 +3,7 @@ module Geo class SnippetRepositoryReplicator < Gitlab::Geo::Replicator include ::Geo::RepositoryReplicatorStrategy + extend ::Gitlab::Utils::Override def self.model ::SnippetRepository @@ -12,6 +13,11 @@ def self.git_access_class ::Gitlab::GitAccessSnippet end + override :verification_feature_flag_enabled? + def self.verification_feature_flag_enabled? + Feature.enabled?(:geo_snippet_repository_verification, default_enabled: :yaml) + end + def repository model_record.repository end diff --git a/ee/config/feature_flags/development/geo_snippet_repository_verification.yml b/ee/config/feature_flags/development/geo_snippet_repository_verification.yml new file mode 100644 index 00000000000000..5f92923baff58f --- /dev/null +++ b/ee/config/feature_flags/development/geo_snippet_repository_verification.yml @@ -0,0 +1,8 @@ +--- +name: geo_snippet_repository_verification +introduced_by_url: +rollout_issue_url: +milestone: '13.10' +type: development +group: group::geo +default_enabled: false diff --git a/ee/db/geo/migrate/20210313050709_add_verification_to_snippet_repository_registry.rb b/ee/db/geo/migrate/20210313050709_add_verification_to_snippet_repository_registry.rb new file mode 100644 index 00000000000000..7d0c4124f620fc --- /dev/null +++ b/ee/db/geo/migrate/20210313050709_add_verification_to_snippet_repository_registry.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddVerificationToSnippetRepositoryRegistry < ActiveRecord::Migration[6.0] + DOWNTIME = false + + def change + add_column :snippet_repository_registry, :verification_started_at, :datetime_with_timezone + add_column :snippet_repository_registry, :verified_at, :datetime_with_timezone + add_column :snippet_repository_registry, :verification_retry_at, :datetime_with_timezone + add_column :snippet_repository_registry, :verification_retry_count, :integer + add_column :snippet_repository_registry, :verification_state, :integer, limit: 2, default: 0, null: false + add_column :snippet_repository_registry, :checksum_mismatch, :boolean + add_column :snippet_repository_registry, :verification_checksum, :binary + add_column :snippet_repository_registry, :verification_checksum_mismatched, :binary + add_column :snippet_repository_registry, :verification_failure, :string, limit: 255 # rubocop:disable Migration/PreventStrings because https://gitlab.com/gitlab-org/gitlab/-/issues/323806 + end +end diff --git a/ee/db/geo/migrate/20210313051642_add_verification_indexes_to_snippet_repository_registry.rb b/ee/db/geo/migrate/20210313051642_add_verification_indexes_to_snippet_repository_registry.rb new file mode 100644 index 00000000000000..363b3f83080578 --- /dev/null +++ b/ee/db/geo/migrate/20210313051642_add_verification_indexes_to_snippet_repository_registry.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class AddVerificationIndexesToSnippetRepositoryRegistry < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + PENDING_VERIFICATION_INDEX_NAME = "snippet_repository_registry_pending_verification" + FAILED_VERIFICATION_INDEX_NAME = "snippet_repository_registry_failed_verification" + NEEDS_VERIFICATION_INDEX_NAME = "snippet_repository_registry_needs_verification" + + disable_ddl_transaction! + + def up + add_concurrent_index :snippet_repository_registry, :verified_at, where: "(state = 2 AND verification_state = 0)", order: { verified_at: 'ASC NULLS FIRST' }, name: PENDING_VERIFICATION_INDEX_NAME + add_concurrent_index :snippet_repository_registry, :verification_retry_at, where: "(state = 2 AND verification_state = 3)", order: { verification_retry_at: 'ASC NULLS FIRST' }, name: FAILED_VERIFICATION_INDEX_NAME + add_concurrent_index :snippet_repository_registry, :verification_state, where: "(state = 2 AND (verification_state IN (0, 3)))", name: NEEDS_VERIFICATION_INDEX_NAME + end + + def down + remove_concurrent_index_by_name :snippet_repository_registry, PENDING_VERIFICATION_INDEX_NAME + remove_concurrent_index_by_name :snippet_repository_registry, FAILED_VERIFICATION_INDEX_NAME + remove_concurrent_index_by_name :snippet_repository_registry, NEEDS_VERIFICATION_INDEX_NAME + end +end diff --git a/ee/db/geo/schema.rb b/ee/db/geo/schema.rb index 81a1037b24f130..2781d2340170a4 100644 --- a/ee/db/geo/schema.rb +++ b/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: 2021_02_25_200858) do +ActiveRecord::Schema.define(version: 2021_03_13_051642) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -70,9 +70,9 @@ t.bigint "group_wiki_repository_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.boolean "force_to_redownload" t.boolean "missing_on_primary" + t.text "last_sync_failure" t.index ["group_wiki_repository_id"], name: "index_g_wiki_repository_registry_on_group_wiki_repository_id", unique: true t.index ["retry_at"], name: "index_group_wiki_repository_registry_on_retry_at" t.index ["state"], name: "index_group_wiki_repository_registry_on_state" @@ -148,6 +148,31 @@ t.index ["verified_at"], name: "package_file_registry_pending_verification", order: "NULLS FIRST", where: "((state = 2) AND (verification_state = 0))" end + create_table "pipeline_artifact_registry", force: :cascade do |t| + t.bigint "pipeline_artifact_id", null: false + t.datetime_with_timezone "created_at", null: false + t.datetime_with_timezone "last_synced_at" + t.datetime_with_timezone "retry_at" + t.datetime_with_timezone "verified_at" + t.datetime_with_timezone "verification_started_at" + t.datetime_with_timezone "verification_retry_at" + t.integer "state", limit: 2, default: 0, null: false + t.integer "verification_state", limit: 2, default: 0, null: false + t.integer "retry_count", limit: 2, default: 0 + t.integer "verification_retry_count", limit: 2, default: 0 + t.boolean "checksum_mismatch" + t.binary "verification_checksum" + t.binary "verification_checksum_mismatched" + t.string "verification_failure", limit: 255 + t.string "last_sync_failure", limit: 255 + t.index ["pipeline_artifact_id"], name: "index_pipeline_artifact_registry_on_pipeline_artifact_id", unique: true + t.index ["retry_at"], name: "index_pipeline_artifact_registry_on_retry_at" + t.index ["state"], name: "index_pipeline_artifact_registry_on_state" + t.index ["verification_retry_at"], name: "pipeline_artifact_registry_failed_verification", order: "NULLS FIRST", where: "((state = 2) AND (verification_state = 3))" + t.index ["verification_state"], name: "pipeline_artifact_registry_needs_verification", where: "((state = 2) AND (verification_state = ANY (ARRAY[0, 3])))" + t.index ["verified_at"], name: "pipeline_artifact_registry_pending_verification", order: "NULLS FIRST", where: "((state = 2) AND (verification_state = 0))" + end + create_table "project_registry", id: :serial, force: :cascade do |t| t.integer "project_id", null: false t.datetime "last_repository_synced_at" @@ -221,6 +246,15 @@ t.text "last_sync_failure" t.boolean "force_to_redownload" t.boolean "missing_on_primary" + t.datetime_with_timezone "verification_started_at" + t.datetime_with_timezone "verified_at" + t.datetime_with_timezone "verification_retry_at" + t.integer "verification_retry_count" + t.integer "verification_state", limit: 2, default: 0, null: false + t.boolean "checksum_mismatch" + t.binary "verification_checksum" + t.binary "verification_checksum_mismatched" + t.string "verification_failure", limit: 255 t.index ["retry_at"], name: "index_snippet_repository_registry_on_retry_at" t.index ["snippet_repository_id"], name: "index_snippet_repository_registry_on_snippet_repository_id", unique: true t.index ["state"], name: "index_snippet_repository_registry_on_state" diff --git a/ee/lib/gitlab/geo/replicator.rb b/ee/lib/gitlab/geo/replicator.rb index 57bcd07d14126a..99998aed271370 100644 --- a/ee/lib/gitlab/geo/replicator.rb +++ b/ee/lib/gitlab/geo/replicator.rb @@ -278,6 +278,8 @@ def handle_after_update return unless self.class.enabled? publish(:updated, **updated_params) + + after_verifiable_update end def created_params diff --git a/ee/spec/lib/gitlab/geo/replicable_model_spec.rb b/ee/spec/lib/gitlab/geo/replicable_model_spec.rb index 1c8e5288c4dee1..3f8c7ef04431f7 100644 --- a/ee/spec/lib/gitlab/geo/replicable_model_spec.rb +++ b/ee/spec/lib/gitlab/geo/replicable_model_spec.rb @@ -31,6 +31,7 @@ it_behaves_like 'a replicable model' do let(:model_record) { subject } + let(:replicator_class) { Geo::DummyReplicator } end describe '#replicator' do diff --git a/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb b/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb index abf052f9a2f005..a1073471347327 100644 --- a/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb +++ b/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb @@ -3,7 +3,9 @@ require 'spec_helper' RSpec.describe Geo::SnippetRepositoryReplicator do - let(:model_record) { build(:snippet_repository, snippet: create(:snippet)) } + let(:snippet) { create(:snippet, :repository) } + let(:model_record) { snippet.snippet_repository } include_examples 'a repository replicator' + it_behaves_like 'a verifiable replicator' end diff --git a/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb index bb278a13a1d19e..5d3ca964713df1 100644 --- a/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb @@ -3,8 +3,11 @@ # Include these shared examples in specs of Replicators that include # BlobReplicatorStrategy. # -# A let variable called model_record should be defined in the spec. It should be -# a valid, unpersisted instance of the model class. +# Required let variables: +# +# - model_record: A valid, unpersisted instance of the model class. Or a valid, +# persisted instance of the model class in a not-yet loaded let +# variable (so we can trigger creation). # RSpec.shared_examples 'a blob replicator' do include EE::GeoHelpers @@ -21,7 +24,9 @@ it_behaves_like 'a replicator' # This could be included in each model's spec, but including it here is DRYer. - include_examples 'a replicable model' + include_examples 'a replicable model' do + let(:replicator_class) { described_class } + end describe '#handle_after_create_commit' do it 'creates a Geo::Event' do diff --git a/ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb index 503fa92de274ec..33f0b8a9c0d5cc 100644 --- a/ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/replicable_model_shared_examples.rb @@ -2,10 +2,10 @@ # Required let variables: # -# - model_record: A valid, unpersisted instance of the model class -# -# We do not use `described_class` here, so we can include this in replicator -# strategy shared examples instead of in *every* model spec. +# - model_record: A valid, unpersisted instance of the model class. Or a valid, +# persisted instance of the model class in a not-yet loaded let +# variable (so we can trigger creation). +# - replicator_class # # Also see ee/spec/lib/gitlab/geo/replicable_model_spec.rb: # @@ -23,7 +23,9 @@ end it 'invokes replicator.handle_after_create_commit on create' do - expect(model_record.replicator).to receive(:handle_after_create_commit) + expect_next_instance_of(replicator_class) do |replicator| + expect(replicator).to receive(:handle_after_create_commit) + end model_record.save! end diff --git a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb index 71227b812db89e..c9b940a257c316 100644 --- a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb @@ -3,8 +3,11 @@ # Include these shared examples in specs of Replicators that include # BlobReplicatorStrategy. # -# A let variable called model_record should be defined in the spec. It should be -# a valid, unpersisted instance of the model class. +# Required let variables: +# +# - model_record: A valid, unpersisted instance of the model class. Or a valid, +# persisted instance of the model class in a not-yet loaded let +# variable (so we can trigger creation). # RSpec.shared_examples 'a repository replicator' do include EE::GeoHelpers @@ -21,7 +24,9 @@ it_behaves_like 'a replicator' # This could be included in each model's spec, but including it here is DRYer. - include_examples 'a replicable model' + include_examples 'a replicable model' do + let(:replicator_class) { described_class } + end describe '#handle_after_update' do it 'creates a Geo::Event' do diff --git a/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb index 801ea4698f34cf..4c9fa2c8073130 100644 --- a/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb @@ -563,6 +563,9 @@ context 'on a secondary' do before do + # Set the primary checksum + replicator.verify + stub_secondary_node end diff --git a/spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb b/spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb index 50e799908c6864..dbf74bd9333160 100644 --- a/spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb +++ b/spec/lib/gitlab/background_migration/backfill_snippet_repositories_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, schema: 2020_04_20_094444 do +RSpec.describe Gitlab::BackgroundMigration::BackfillSnippetRepositories, :migration, schema: 2021_03_13_045845 do let(:gitlab_shell) { Gitlab::Shell.new } let(:users) { table(:users) } let(:snippets) { table(:snippets) } -- GitLab From 30e25e3338b15876cdf848734c1191dd48261cb8 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:36:39 -0700 Subject: [PATCH 5/6] Update Geo API documentation --- doc/api/geo_nodes.md | 212 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 173 insertions(+), 39 deletions(-) diff --git a/doc/api/geo_nodes.md b/doc/api/geo_nodes.md index 0401680b016f8c..9b2919bd66313b 100644 --- a/doc/api/geo_nodes.md +++ b/doc/api/geo_nodes.md @@ -372,23 +372,66 @@ Example response: "last_successful_status_check_timestamp": 1510125024, "version": "10.3.0", "revision": "33d33a096a", - "package_files_count": 10, - "package_files_checksummed_count": 10, + "merge_request_diffs_count": 5, + "merge_request_diffs_checksum_total_count": 5, + "merge_request_diffs_checksummed_count": 5, + "merge_request_diffs_checksum_failed_count": 0, + "merge_request_diffs_synced_count": null, + "merge_request_diffs_failed_count": null, + "merge_request_diffs_registry_count": null, + "merge_request_diffs_verification_total_count": null, + "merge_request_diffs_verified_count": null, + "merge_request_diffs_verification_failed_count": null, + "merge_request_diffs_synced_in_percentage": "0.00%", + "merge_request_diffs_verified_in_percentage": "0.00%", + "package_files_count": 5, + "package_files_checksum_total_count": 5, + "package_files_checksummed_count": 5, "package_files_checksum_failed_count": 0, - "package_files_registry_count": 10, - "package_files_synced_count": 6, - "package_files_failed_count": 3, - "snippet_repositories_count": 10, - "snippet_repositories_checksummed_count": 10, + "package_files_synced_count": null, + "package_files_failed_count": null, + "package_files_registry_count": null, + "package_files_verification_total_count": null, + "package_files_verified_count": null, + "package_files_verification_failed_count": null, + "package_files_synced_in_percentage": "0.00%", + "package_files_verified_in_percentage": "0.00%", + "terraform_state_versions_count": 5, + "terraform_state_versions_checksum_total_count": 5, + "terraform_state_versions_checksummed_count": 5, + "terraform_state_versions_checksum_failed_count": 0, + "terraform_state_versions_synced_count": null, + "terraform_state_versions_failed_count": null, + "terraform_state_versions_registry_count": null, + "terraform_state_versions_verification_total_count": null, + "terraform_state_versions_verified_count": null, + "terraform_state_versions_verification_failed_count": null, + "terraform_state_versions_synced_in_percentage": "0.00%", + "terraform_state_versions_verified_in_percentage": "0.00%", + "snippet_repositories_count": 5, + "snippet_repositories_checksum_total_count": 5, + "snippet_repositories_checksummed_count": 5, "snippet_repositories_checksum_failed_count": 0, - "snippet_repositories_registry_count": 10, - "snippet_repositories_synced_count": 6, - "snippet_repositories_failed_count": 3, - "group_wiki_repositories_checksummed_count": 10, + "snippet_repositories_synced_count": null, + "snippet_repositories_failed_count": null, + "snippet_repositories_registry_count": null, + "snippet_repositories_verification_total_count": null, + "snippet_repositories_verified_count": null, + "snippet_repositories_verification_failed_count": null, + "snippet_repositories_synced_in_percentage": "0.00%", + "snippet_repositories_verified_in_percentage": "0.00%", + "group_wiki_repositories_count": 5, + "group_wiki_repositories_checksum_total_count": 5, + "group_wiki_repositories_checksummed_count": 5, "group_wiki_repositories_checksum_failed_count": 0, - "group_wiki_repositories_registry_count": 10, - "group_wiki_repositories_synced_count": 6, - "group_wiki_repositories_failed_count": 3 + "group_wiki_repositories_synced_count": null, + "group_wiki_repositories_failed_count": null, + "group_wiki_repositories_registry_count": null, + "group_wiki_repositories_verification_total_count": null, + "group_wiki_repositories_verified_count": null, + "group_wiki_repositories_verification_failed_count": null, + "group_wiki_repositories_synced_in_percentage": "0.00%", + "group_wiki_repositories_verified_in_percentage": "0.00%", }, { "geo_node_id": 2, @@ -459,35 +502,66 @@ 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_count": 5, + "merge_request_diffs_checksum_total_count": 5, + "merge_request_diffs_checksummed_count": 5, "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, + "merge_request_diffs_synced_count": 5, + "merge_request_diffs_failed_count": 0, + "merge_request_diffs_registry_count": 5, + "merge_request_diffs_verification_total_count": 5, + "merge_request_diffs_verified_count": 5, + "merge_request_diffs_verification_failed_count": 0, + "merge_request_diffs_synced_in_percentage": "100.00%", + "merge_request_diffs_verified_in_percentage": "100.00%", + "package_files_count": 5, + "package_files_checksum_total_count": 5, + "package_files_checksummed_count": 5, "package_files_checksum_failed_count": 0, - "package_files_registry_count": 10, - "package_files_synced_count": 6, - "package_files_failed_count": 3, - "terraform_state_versions_count": 10, - "terraform_state_versions_checksummed_count": 10, + "package_files_synced_count": 5, + "package_files_failed_count": 0, + "package_files_registry_count": 5, + "package_files_verification_total_count": 5, + "package_files_verified_count": 5, + "package_files_verification_failed_count": 0, + "package_files_synced_in_percentage": "100.00%", + "package_files_verified_in_percentage": "100.00%", + "terraform_state_versions_count": 5, + "terraform_state_versions_checksum_total_count": 5, + "terraform_state_versions_checksummed_count": 5, "terraform_state_versions_checksum_failed_count": 0, - "terraform_state_versions_registry_count": 10, - "terraform_state_versions_synced_count": 6, - "terraform_state_versions_failed_count": 3, - "snippet_repositories_count": 10, - "snippet_repositories_checksummed_count": 10, + "terraform_state_versions_synced_count": 5, + "terraform_state_versions_failed_count": 0, + "terraform_state_versions_registry_count": 5, + "terraform_state_versions_verification_total_count": 5, + "terraform_state_versions_verified_count": 5, + "terraform_state_versions_verification_failed_count": 0, + "terraform_state_versions_synced_in_percentage": "100.00%", + "terraform_state_versions_verified_in_percentage": "100.00%", + "snippet_repositories_count": 5, + "snippet_repositories_checksum_total_count": 5, + "snippet_repositories_checksummed_count": 5, "snippet_repositories_checksum_failed_count": 0, - "snippet_repositories_registry_count": 10, - "snippet_repositories_synced_count": 6, - "snippet_repositories_failed_count": 3, - "group_wiki_repositories_checksummed_count": 10, + "snippet_repositories_synced_count": 5, + "snippet_repositories_failed_count": 0, + "snippet_repositories_registry_count": 5, + "snippet_repositories_verification_total_count": 5, + "snippet_repositories_verified_count": 5, + "snippet_repositories_verification_failed_count": 0, + "snippet_repositories_synced_in_percentage": "100.00%", + "snippet_repositories_verified_in_percentage": "100.00%", + "group_wiki_repositories_count": 5, + "group_wiki_repositories_checksum_total_count": 5, + "group_wiki_repositories_checksummed_count": 5, "group_wiki_repositories_checksum_failed_count": 0, - "group_wiki_repositories_registry_count": 10, - "group_wiki_repositories_synced_count": 6, - "group_wiki_repositories_failed_count": 3 + "group_wiki_repositories_synced_count": 5, + "group_wiki_repositories_failed_count": 0, + "group_wiki_repositories_registry_count": 5, + "group_wiki_repositories_verification_total_count": 5, + "group_wiki_repositories_verified_count": 5, + "group_wiki_repositories_verification_failed_count": 0, + "group_wiki_repositories_synced_in_percentage": "100.00%", + "group_wiki_repositories_verified_in_percentage": "100.00%", } ] ``` @@ -554,7 +628,67 @@ Example response: "cursor_last_event_timestamp": 1509681166, "last_successful_status_check_timestamp": 1510125268, "version": "10.3.0", - "revision": "33d33a096a" + "revision": "33d33a096a", + "merge_request_diffs_count": 5, + "merge_request_diffs_checksum_total_count": 5, + "merge_request_diffs_checksummed_count": 5, + "merge_request_diffs_checksum_failed_count": 0, + "merge_request_diffs_synced_count": 5, + "merge_request_diffs_failed_count": 0, + "merge_request_diffs_registry_count": 5, + "merge_request_diffs_verification_total_count": 5, + "merge_request_diffs_verified_count": 5, + "merge_request_diffs_verification_failed_count": 0, + "merge_request_diffs_synced_in_percentage": "100.00%", + "merge_request_diffs_verified_in_percentage": "100.00%", + "package_files_count": 5, + "package_files_checksum_total_count": 5, + "package_files_checksummed_count": 5, + "package_files_checksum_failed_count": 0, + "package_files_synced_count": 5, + "package_files_failed_count": 0, + "package_files_registry_count": 5, + "package_files_verification_total_count": 5, + "package_files_verified_count": 5, + "package_files_verification_failed_count": 0, + "package_files_synced_in_percentage": "100.00%", + "package_files_verified_in_percentage": "100.00%", + "terraform_state_versions_count": 5, + "terraform_state_versions_checksum_total_count": 5, + "terraform_state_versions_checksummed_count": 5, + "terraform_state_versions_checksum_failed_count": 0, + "terraform_state_versions_synced_count": 5, + "terraform_state_versions_failed_count": 0, + "terraform_state_versions_registry_count": 5, + "terraform_state_versions_verification_total_count": 5, + "terraform_state_versions_verified_count": 5, + "terraform_state_versions_verification_failed_count": 0, + "terraform_state_versions_synced_in_percentage": "100.00%", + "terraform_state_versions_verified_in_percentage": "100.00%", + "snippet_repositories_count": 5, + "snippet_repositories_checksum_total_count": 5, + "snippet_repositories_checksummed_count": 5, + "snippet_repositories_checksum_failed_count": 0, + "snippet_repositories_synced_count": 5, + "snippet_repositories_failed_count": 0, + "snippet_repositories_registry_count": 5, + "snippet_repositories_verification_total_count": 5, + "snippet_repositories_verified_count": 5, + "snippet_repositories_verification_failed_count": 0, + "snippet_repositories_synced_in_percentage": "100.00%", + "snippet_repositories_verified_in_percentage": "100.00%", + "group_wiki_repositories_count": 5, + "group_wiki_repositories_checksum_total_count": 5, + "group_wiki_repositories_checksummed_count": 5, + "group_wiki_repositories_checksum_failed_count": 0, + "group_wiki_repositories_synced_count": 5, + "group_wiki_repositories_failed_count": 0, + "group_wiki_repositories_registry_count": 5, + "group_wiki_repositories_verification_total_count": 5, + "group_wiki_repositories_verified_count": 5, + "group_wiki_repositories_verification_failed_count": 0, + "group_wiki_repositories_synced_in_percentage": "100.00%", + "group_wiki_repositories_verified_in_percentage": "100.00%", } ``` -- GitLab From a46c5f90abed7716225cad36eacaadab443eb214 Mon Sep 17 00:00:00 2001 From: Mike Kozono Date: Tue, 23 Mar 2021 00:44:08 -0700 Subject: [PATCH 6/6] Add changelog entry --- changelogs/unreleased/mk-verify-snippets.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/mk-verify-snippets.yml diff --git a/changelogs/unreleased/mk-verify-snippets.yml b/changelogs/unreleased/mk-verify-snippets.yml new file mode 100644 index 00000000000000..17354f4cdd008a --- /dev/null +++ b/changelogs/unreleased/mk-verify-snippets.yml @@ -0,0 +1,6 @@ +--- +title: 'Geo: Prepare snippet_repositories and snippet_repository_registry tables for + adding verification' +merge_request: 56596 +author: +type: added -- GitLab