Commit 8ca53d80 authored by Douglas Barbosa Alexandre's avatar Douglas Barbosa Alexandre

Merge branch 'mk/verify-snippets' into 'master'

Geo: Add snippet repository verification [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56596
parents 5013b34b a46c5f90
---
title: 'Geo: Prepare snippet_repositories and snippet_repository_registry tables for
adding verification'
merge_request: 56596
author:
type: added
# 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
# 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
d1e6596e9c6825e29c50523dce60fd3d0b3c067c10e210f74640ba94f7938871
\ No newline at end of file
bc6302444f7a0a858c821d971fc45a4ececd7b877020f8e920a244866c60b7a2
\ No newline at end of file
......@@ -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);
This diff is collapsed.
......@@ -92,5 +92,14 @@ module Geo
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
......@@ -61,5 +61,30 @@ module Geo
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
......@@ -189,7 +189,7 @@ module Geo
# 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 @@ module Geo
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
......
......@@ -6,6 +6,7 @@ module EE
prepended do
include ::Gitlab::Geo::ReplicableModel
include ::Gitlab::Geo::VerificationState
include FromUnion
with_replicator Geo::SnippetRepositoryReplicator
......
......@@ -2,6 +2,7 @@
class Geo::SnippetRepositoryRegistry < Geo::BaseRegistry
include Geo::ReplicableRegistry
include ::Geo::VerifiableRegistry
MODEL_CLASS = ::SnippetRepository
MODEL_FOREIGN_KEY = :snippet_repository_id
......
......@@ -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 @@ module Geo
::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
......
---
name: geo_snippet_repository_verification
introduced_by_url:
rollout_issue_url:
milestone: '13.10'
type: development
group: group::geo
default_enabled: false
# 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
# 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
......@@ -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 @@ ActiveRecord::Schema.define(version: 2021_02_25_200858) do
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 @@ ActiveRecord::Schema.define(version: 2021_02_25_200858) do
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 @@ ActiveRecord::Schema.define(version: 2021_02_25_200858) do
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"
......
......@@ -278,6 +278,8 @@ module Gitlab
return unless self.class.enabled?
publish(:updated, **updated_params)
after_verifiable_update
end
def created_params
......
......@@ -31,6 +31,7 @@ RSpec.describe Gitlab::Geo::ReplicableModel do
it_behaves_like 'a replicable model' do
let(:model_record) { subject }
let(:replicator_class) { Geo::DummyReplicator }
end
describe '#replicator' do
......
......@@ -406,7 +406,7 @@ RSpec.describe Gitlab::Geo, :geo, :request_store do
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 @@ RSpec.describe Gitlab::Geo, :geo, :request_store do
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 }
......
......@@ -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
......@@ -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 @@ RSpec.shared_examples 'a blob replicator' do
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
......
......@@ -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 @@ RSpec.shared_examples 'a replicable model' do
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
......
......@@ -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 @@ RSpec.shared_examples 'a repository replicator' do
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
......@@ -71,14 +76,16 @@ RSpec.shared_examples 'a repository replicator' do
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 +96,8 @@ RSpec.shared_examples 'a repository replicator' do
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)
......
......@@ -341,14 +341,44 @@ RSpec.shared_examples 'a verifiable replicator' do
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
......@@ -533,6 +563,9 @@ RSpec.shared_examples 'a verifiable replicator' do
context 'on a secondary' do
before do
# Set the primary checksum
replicator.verify
stub_secondary_node
end
......
......@@ -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) }
......
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