Merge branch 'mk/refactor-extract-verification' into 'master'

Geo: Refactor: Extract verification concern

See merge request gitlab-org/gitlab!45695

ee/app/models/concerns/geo/blob_replicator_strategy.rb

@@ -4,7 +4,7 @@ module Geo
   module BlobReplicatorStrategy
     extend ActiveSupport::Concern
 
-    include Delay
+    include ::Geo::VerifiableReplicator
     include Gitlab::Geo::LogHelpers
 
     included do
@@ -17,7 +17,8 @@ module Geo
       return unless self.class.enabled?
 
       publish(:created, **created_params)
-      schedule_checksum_calculation if needs_checksum?
+
+      after_verifiable_update
     end
 
     # Called by Gitlab::Geo::Replicator#consume
@@ -47,45 +48,8 @@ module Geo
       carrierwave_uploader.path
     end
 
-    def calculate_checksum!
-      checksum = model_record.calculate_checksum!
-      update_verification_state!(checksum: checksum)
-    rescue => e
-      log_error('Error calculating the checksum', e)
-      update_verification_state!(failure: e.message)
-    end
-
-    # Check if given checksum matches known one
-    #
-    # @param [String] checksum
-    # @return [Boolean] whether checksum matches
-    def matches_checksum?(checksum)
-      model_record.verification_checksum == checksum
-    end
-
     private
 
-    # Update checksum on Geo primary database
-    #
-    # @param [String] checksum value generated by the checksum routine
-    # @param [String] failure (optional) stacktrace from failed execution
-    def update_verification_state!(checksum: nil, failure: nil)
-      retry_at, retry_count = calculate_next_retry_attempt if failure.present?
-
-      model_record.update!(
-        verification_checksum: checksum,
-        verified_at: Time.current,
-        verification_failure: failure,
-        verification_retry_at: retry_at,
-        verification_retry_count: retry_count
-      )
-    end
-
-    def calculate_next_retry_attempt
-      retry_count = model_record.verification_retry_count.to_i + 1
-      [next_retry_time(retry_count), retry_count]
-    end
-
     def download
       ::Geo::BlobDownloadService.new(replicator: self).execute
     end

ee/app/models/concerns/geo/repository_replicator_strategy.rb

@@ -4,7 +4,7 @@ module Geo
   module RepositoryReplicatorStrategy
     extend ActiveSupport::Concern
 
-    include Delay
+    include ::Geo::VerifiableReplicator
     include Gitlab::Geo::LogHelpers
 
     included do

ee/app/models/concerns/geo/verifiable_replicator.rb

+# frozen_string_literal: true
+
+module Geo
+  module VerifiableReplicator
+    extend ActiveSupport::Concern
+
+    include Delay
+
+    class_methods do
+      def checksummed
+        model.available_replicables.checksummed
+      end
+
+      def checksummed_count
+        model.available_replicables.checksummed.count
+      end
+
+      def checksum_failed_count
+        model.available_replicables.checksum_failed.count
+      end
+    end
+
+    def after_verifiable_update
+      schedule_checksum_calculation if needs_checksum?
+    end
+
+    def calculate_checksum!
+      checksum = model_record.calculate_checksum!
+      update_verification_state!(checksum: checksum)
+    rescue => e
+      log_error('Error calculating the checksum', e)
+      update_verification_state!(failure: e.message)
+    end
+
+    # Check if given checksum matches known one
+    #
+    # @param [String] checksum
+    # @return [Boolean] whether checksum matches
+    def matches_checksum?(checksum)
+      model_record.verification_checksum == checksum
+    end
+
+    def needs_checksum?
+      return true unless model_record.respond_to?(:needs_checksum?)
+
+      model_record.needs_checksum?
+    end
+
+    # Checksum value from the main database
+    #
+    # @abstract
+    def primary_checksum
+      model_record.verification_checksum
+    end
+
+    def secondary_checksum
+      registry.verification_checksum
+    end
+
+    private
+
+    # Update checksum on Geo primary database
+    #
+    # @param [String] checksum value generated by the checksum routine
+    # @param [String] failure (optional) stacktrace from failed execution
+    def update_verification_state!(checksum: nil, failure: nil)
+      retry_at, retry_count = calculate_next_retry_attempt if failure.present?
+
+      model_record.update!(
+        verification_checksum: checksum,
+        verified_at: Time.current,
+        verification_failure: failure,
+        verification_retry_at: retry_at,
+        verification_retry_count: retry_count
+      )
+    end
+
+    def calculate_next_retry_attempt
+      retry_count = model_record.verification_retry_count.to_i + 1
+      [next_retry_time(retry_count), retry_count]
+    end
+
+    def schedule_checksum_calculation
+      raise NotImplementedError
+    end
+  end
+end

ee/lib/gitlab/geo/replicator.rb

@@ -134,18 +134,6 @@ module Gitlab
         replicator_class.new(model_record_id: replicable_id)
       end
 
-      def self.checksummed
-        model.available_replicables.checksummed
-      end
-
-      def self.checksummed_count
-        model.available_replicables.checksummed.count
-      end
-
-      def self.checksum_failed_count
-        model.available_replicables.checksum_failed.count
-      end
-
       def self.primary_total_count
         model.available_replicables.count
       end
@@ -265,17 +253,6 @@ module Gitlab
         registry_class.for_model_record_id(model_record_id)
       end
 
-      # Checksum value from the main database
-      #
-      # @abstract
-      def primary_checksum
-        model_record.verification_checksum
-      end
-
-      def secondary_checksum
-        registry.verification_checksum
-      end
-
       # Return exactly the data needed by `for_replicable_params` to
       # reinstantiate this Replicator elsewhere.
       #
@@ -298,10 +275,6 @@ module Gitlab
         publish(:updated, **updated_params)
       end
 
-      def schedule_checksum_calculation
-        raise NotImplementedError
-      end
-
       def created_params
         event_params
       end
@@ -318,12 +291,6 @@ module Gitlab
         { model_record_id: model_record.id }
       end
 
-      def needs_checksum?
-        return true unless model_record.respond_to?(:needs_checksum?)
-
-        model_record.needs_checksum?
-      end
-
       protected
 
       # Store an event on the database

ee/spec/support/shared_examples/models/concerns/blob_replicator_strategy_shared_examples.rb

@@ -19,6 +19,7 @@ RSpec.shared_examples 'a blob replicator' do
   end
 
   it_behaves_like 'a replicator'
+  it_behaves_like 'a verifiable replicator'
 
   # This could be included in each model's spec, but including it here is DRYer.
   include_examples 'a replicable model'
@@ -33,9 +34,8 @@ RSpec.shared_examples 'a blob replicator' do
         "replicable_name" => replicator.replicable_name, "event_name" => "created", "payload" => { "model_record_id" => replicator.model_record.id })
     end
 
-    it 'schedules the checksum calculation if needed' do
-      expect(Geo::BlobVerificationPrimaryWorker).to receive(:perform_async)
-      expect(replicator).to receive(:needs_checksum?).and_return(true)
+    it 'calls #after_verifiable_update' do
+      expect(replicator).to receive(:after_verifiable_update)
 
       replicator.handle_after_create_commit
     end
@@ -45,8 +45,8 @@ RSpec.shared_examples 'a blob replicator' do
         stub_feature_flags(replicator.replication_enabled_feature_key => false)
       end
 
-      it 'does not schedule the checksum calculation' do
-        expect(Geo::BlobVerificationPrimaryWorker).not_to receive(:perform_async)
+      it 'does not call #after_verifiable_update' do
+        expect(replicator).not_to receive(:after_verifiable_update)
 
         replicator.handle_after_create_commit
       end
@@ -82,30 +82,6 @@ RSpec.shared_examples 'a blob replicator' do
     end
   end
 
-  describe '#calculate_checksum!' do
-    it 'calculates the checksum' do
-      model_record.save!
-
-      replicator.calculate_checksum!
-
-      expect(model_record.reload.verification_checksum).not_to be_nil
-      expect(model_record.reload.verified_at).not_to be_nil
-    end
-
-    it 'saves the error message and increments retry counter' do
-      model_record.save!
-
-      allow(model_record).to receive(:calculate_checksum!) do
-        raise StandardError.new('Failure to calculate checksum')
-      end
-
-      replicator.calculate_checksum!
-
-      expect(model_record.reload.verification_failure).to eq 'Failure to calculate checksum'
-      expect(model_record.verification_retry_count).to be 1
-    end
-  end
-
   describe 'created event consumption' do
     context "when the blob's project is in replicables for this geo node" do
       it 'invokes Geo::BlobDownloadService' do

ee/spec/support/shared_examples/models/concerns/verifiable_replicator_shared_examples.rb

+# frozen_string_literal: true
+
+# This is included by BlobReplicatorStrategy and RepositoryReplicatorStrategy.
+#
+RSpec.shared_examples 'a verifiable replicator' do
+  include EE::GeoHelpers
+
+  describe '#after_verifiable_update' do
+    it 'schedules the checksum calculation if needed' do
+      expect(replicator).to receive(:schedule_checksum_calculation)
+      expect(replicator).to receive(:needs_checksum?).and_return(true)
+
+      replicator.after_verifiable_update
+    end
+  end
+
+  describe '#calculate_checksum!' do
+    it 'calculates the checksum' do
+      model_record.save!
+
+      replicator.calculate_checksum!
+
+      expect(model_record.reload.verification_checksum).not_to be_nil
+      expect(model_record.reload.verified_at).not_to be_nil
+    end
+
+    it 'saves the error message and increments retry counter' do
+      model_record.save!
+
+      allow(model_record).to receive(:calculate_checksum!) do
+        raise StandardError.new('Failure to calculate checksum')
+      end
+
+      replicator.calculate_checksum!
+
+      expect(model_record.reload.verification_failure).to eq 'Failure to calculate checksum'
+      expect(model_record.verification_retry_count).to be 1
+    end
+  end
+end