Merge branch '13837-add-service-to-verify-package-files-on-secondaries' into 'master'

Add service to verify package files on secondaries

Closes #13837

See merge request gitlab-org/gitlab!29131

ee/app/models/geo/package_file_registry.rb

@@ -2,6 +2,7 @@
 
 class Geo::PackageFileRegistry < Geo::BaseRegistry
   include ::Delay
+  include ShaAttribute
 
   def self.declarative_policy_class
     'Geo::RegistryPolicy'
@@ -65,6 +66,9 @@ class Geo::PackageFileRegistry < Geo::BaseRegistry
     end
   end
 
+  sha_attribute :verification_checksum
+  sha_attribute :verification_checksum_mismatched
+
   # @return [Geo::PackageFileRegistry] an instance of this class
   def self.for_model_record_id(id)
     find_or_initialize_by(package_file_id: id)

ee/app/services/geo/blob_verification_secondary_service.rb

+# frozen_string_literal: true
+
+module Geo
+  class BlobVerificationSecondaryService
+    include Delay
+    include Gitlab::Geo::LogHelpers
+
+    def initialize(replicator)
+      @replicator = replicator
+      @registry = replicator.registry
+    end
+
+    def execute
+      return unless Gitlab::Geo.geo_database_configured?
+      return unless Gitlab::Geo.secondary?
+      return unless should_verify_checksum?
+
+      verify_checksum
+    end
+
+    private
+
+    attr_reader :replicator, :registry
+
+    delegate :model_record, :primary_checksum, :secondary_checksum, to: :replicator
+
+    def should_verify_checksum?
+      return false unless registry.synced?
+      return false unless primary_checksum.present?
+
+      mismatch?(secondary_checksum)
+    end
+
+    def mismatch?(checksum)
+      primary_checksum != checksum
+    end
+
+    def verify_checksum
+      checksum = model_record.calculate_checksum!
+
+      if mismatch?(checksum)
+        update_registry!(mismatch: checksum, failure: 'checksum mismatch')
+      else
+        update_registry!(checksum: checksum)
+      end
+    rescue => e
+      update_registry!(failure: 'Error calculating checksum', exception: e)
+    end
+
+    def update_registry!(checksum: nil, mismatch: nil, failure: nil, exception: nil)
+      reverify, verification_retry_count =
+        if mismatch || failure.present?
+          log_error(failure, exception)
+          [true, registry.verification_retry_count.to_i + 1]
+        else
+          [false, nil]
+        end
+
+      resync_retry_at, resync_retry_count =
+        if reverify
+          [*calculate_next_retry_attempt]
+        end
+
+      registry.update!(
+        verification_checksum: checksum,
+        verification_checksum_mismatched: mismatch,
+        checksum_mismatch: mismatch.present?,
+        verified_at: Time.now,
+        verification_failure: failure,
+        verification_retry_count: verification_retry_count,
+        retry_at: resync_retry_at,
+        retry_count: resync_retry_count
+      )
+    end
+
+    def calculate_next_retry_attempt
+      retry_count = registry.retry_count.to_i + 1
+      [next_retry_time(retry_count), retry_count]
+    end
+  end
+end

ee/db/geo/migrate/20200407120740_add_verification_fields_to_package_file_on_secondary.rb

+# frozen_string_literal: true
+
+class AddVerificationFieldsToPackageFileOnSecondary < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_column :package_file_registry, :verification_failure, :string, limit: 255 # rubocop:disable Migration/PreventStrings
+    add_column :package_file_registry, :verification_checksum, :binary
+    add_column :package_file_registry, :checksum_mismatch, :boolean
+    add_column :package_file_registry, :verification_checksum_mismatched, :binary
+    add_column :package_file_registry, :verification_retry_count, :integer
+    add_column :package_file_registry, :verified_at, :datetime_with_timezone
+    add_column :package_file_registry, :verification_retry_at, :datetime_with_timezone
+  end
+end

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: 2020_01_21_194300) do
+ActiveRecord::Schema.define(version: 2020_04_07_120740) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -99,6 +99,13 @@ ActiveRecord::Schema.define(version: 2020_01_21_194300) do
     t.datetime_with_timezone "retry_at"
     t.datetime_with_timezone "last_synced_at"
     t.datetime_with_timezone "created_at", null: false
+    t.string "verification_failure", limit: 255
+    t.binary "verification_checksum"
+    t.boolean "checksum_mismatch"
+    t.binary "verification_checksum_mismatched"
+    t.integer "verification_retry_count"
+    t.datetime_with_timezone "verified_at"
+    t.datetime_with_timezone "verification_retry_at"
     t.index ["package_file_id"], name: "index_package_file_registry_on_repository_id"
     t.index ["retry_at"], name: "index_package_file_registry_on_retry_at"
     t.index ["state"], name: "index_package_file_registry_on_state"

ee/lib/gitlab/geo/replicator.rb

@@ -197,7 +197,11 @@ module Gitlab
       #
       # @abstract
       def primary_checksum
-        nil
+        model_record.verification_checksum
+      end
+
+      def secondary_checksum
+        registry.verification_checksum
       end
 
       protected

ee/spec/services/geo/blob_verification_secondary_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Geo::BlobVerificationSecondaryService, :geo do
+  include ::EE::GeoHelpers
+
+  let(:secondary) { create(:geo_node) }
+
+  before do
+    stub_current_geo_node(secondary)
+  end
+
+  describe '#execute' do
+    let_it_be(:package_file) { create(:conan_package_file, :conan_recipe_file, verification_checksum: '62fc1ec4ce60') }
+
+    let_it_be(:registry) { create(:package_file_registry, :synced, package_file: package_file) }
+
+    subject(:service) { described_class.new(package_file.replicator) }
+
+    it 'does not calculate the checksum when not running on a secondary' do
+      stub_primary_node
+
+      expect(package_file).not_to receive(:calculate_checksum!)
+
+      service.execute
+    end
+
+    it 'does not verify the checksum if resync is needed' do
+      registry.resync
+
+      expect(package_file).not_to receive(:calculate_checksum!)
+
+      service.execute
+    end
+
+    it 'does not verify the checksum if sync is started' do
+      registry.start!
+
+      expect(package_file).not_to receive(:calculate_checksum!)
+
+      service.execute
+    end
+
+    it 'does not verify the checksum if primary was never verified' do
+      package_file.assign_attributes(verification_checksum: nil)
+
+      expect(package_file).not_to receive(:calculate_checksum!)
+
+      service.execute
+    end
+
+    it 'does not verify the checksum if the current checksum matches' do
+      package_file.assign_attributes(verification_checksum: '62fc1ec4ce60')
+      registry.update(verification_checksum: '62fc1ec4ce60')
+
+      expect(package_file).not_to receive(:calculate_checksum!)
+
+      service.execute
+    end
+
+    it 'sets checksum when the checksum matches' do
+      allow(package_file).to receive(:calculate_checksum!).and_return('62fc1ec4ce60')
+
+      service.execute
+
+      expect(registry.reload).to have_attributes(
+        verification_checksum: '62fc1ec4ce60',
+        checksum_mismatch: false,
+        verified_at: be_within(1.minute).of(Time.now),
+        verification_failure: nil,
+        verification_retry_count: nil,
+        retry_at: nil,
+        retry_count: nil
+      )
+    end
+
+    context 'when the checksum mismatch' do
+      before do
+        allow(package_file).to receive(:calculate_checksum!).and_return('99fc1ec4ce60')
+      end
+
+      it 'keeps track of failures' do
+        service.execute
+
+        expect(registry.reload).to have_attributes(
+          verification_checksum: nil,
+          verification_checksum_mismatched: '99fc1ec4ce60',
+          checksum_mismatch: true,
+          verified_at: be_within(1.minute).of(Time.now),
+          verification_failure: 'checksum mismatch',
+          verification_retry_count: 1,
+          retry_at: be_present,
+          retry_count: 1
+        )
+      end
+
+      it 'ensures the next retry time is capped properly' do
+        registry.update(retry_count: 30)
+
+        service.execute
+
+        expect(registry.reload).to have_attributes(
+          retry_at: be_within(100.seconds).of(1.hour.from_now),
+          retry_count: 31
+        )
+      end
+    end
+
+    context 'when checksum calculation fails' do
+      before do
+        allow(package_file).to receive(:calculate_checksum!).and_raise('Error calculating checksum')
+      end
+
+      it 'keeps track of failures' do
+        service.execute
+
+        expect(registry.reload).to have_attributes(
+          verification_checksum: nil,
+          verification_checksum_mismatched: nil,
+          checksum_mismatch: false,
+          verified_at: be_within(1.minute).of(Time.now),
+          verification_failure: 'Error calculating checksum',
+          verification_retry_count: 1,
+          retry_at: be_present,
+          retry_count: 1
+        )
+      end
+
+      it 'ensures the next retry time is capped properly' do
+        registry.update(retry_count: 30)
+
+        service.execute
+
+        expect(registry.reload).to have_attributes(
+          retry_at: be_within(100.seconds).of(1.hour.from_now),
+          retry_count: 31
+        )
+      end
+    end
+  end
+end