Merge branch 'mk/verification-refactors' into 'master'

Geo: Verification-related refactors

See merge request gitlab-org/gitlab!47361

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

@@ -65,9 +65,5 @@ module Geo
     def deleted_params
       { model_record_id: model_record.id, blob_path: blob_path }
     end
-
-    def schedule_checksum_calculation
-      Geo::VerificationWorker.perform_async(replicable_name, model_record.id)
-    end
   end
 end

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

@@ -25,10 +25,6 @@ module Geo
         Feature.enabled?(:geo_framework_verification)
       end
 
-      def checksummed
-        model.available_replicables.checksummed
-      end
-
       def checksummed_count
         # When verification is disabled, this returns nil.
         # Bonus: This causes the progress bar to be hidden.
@@ -47,11 +43,15 @@ module Geo
     end
 
     def after_verifiable_update
-      schedule_checksum_calculation if needs_checksum?
+      verify_async if needs_checksum?
+    end
+
+    def verify_async
+      Geo::VerificationWorker.perform_async(replicable_name, model_record.id)
     end
 
-    def calculate_checksum!
-      checksum = model_record.calculate_checksum!
+    def verify
+      checksum = model_record.calculate_checksum
       update_verification_state!(checksum: checksum)
     rescue => e
       log_error('Error calculating the checksum', e)
@@ -106,9 +106,5 @@ module Geo
       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/app/services/geo/blob_verification_secondary_service.rb

@@ -36,7 +36,7 @@ module Geo
     end
 
     def verify_checksum
-      checksum = model_record.calculate_checksum!
+      checksum = model_record.calculate_checksum
 
       if mismatch?(checksum)
         update_registry!(mismatch: checksum, failure: 'checksum mismatch')

ee/app/workers/geo/verification_worker.rb

@@ -14,7 +14,7 @@ module Geo
     def perform(replicable_name, replicable_id)
       replicator = ::Gitlab::Geo::Replicator.for_replicable_params(replicable_name: replicable_name, replicable_id: replicable_id)
 
-      replicator.calculate_checksum!
+      replicator.verify
     rescue ActiveRecord::RecordNotFound
       log_error("Couldn't find the record, skipping", replicable_name: replicable_name, replicable_id: replicable_id)
     end

ee/lib/gitlab/geo/replicable_model.rb

@@ -46,13 +46,13 @@ module Gitlab
         raise NotImplementedError, 'There is no Replicator defined for this model'
       end
 
-      # Clear model verification checksum and force recalculation
-      def calculate_checksum!
-        self.verification_checksum = nil
-
-        return unless needs_checksum?
+      # Returns a checksum of the file (assumed to be a "blob" type)
+      #
+      # @return [String] SHA256 hash of the carrierwave file
+      def calculate_checksum
+        return unless checksummable?
 
-        self.verification_checksum = self.class.hexdigest(replicator.carrierwave_uploader.path)
+        self.class.hexdigest(replicator.carrierwave_uploader.path)
       end
 
       # Checks whether model needs checksum to be performed

ee/spec/models/packages/package_file_spec.rb

@@ -4,24 +4,19 @@ require 'spec_helper'
 RSpec.describe Packages::PackageFile, type: :model do
   include ::EE::GeoHelpers
 
-  describe '#calculate_checksum!' do
+  describe '#calculate_checksum' do
     let(:package_file) { create(:conan_package_file, :conan_recipe_file) }
 
-    it 'sets `verification_checksum` to SHA256 sum of the file' do
+    it 'returns SHA256 sum of the file' do
       expected = Digest::SHA256.file(package_file.file.path).hexdigest
 
-      expect { package_file.calculate_checksum! }
-        .to change { package_file.verification_checksum }.from(nil).to(expected)
+      expect(package_file.calculate_checksum).to eq(expected)
     end
 
-    it 'sets `checksum` to nil for a non-existent file' do
-      checksum = Digest::SHA256.file(package_file.file.path).hexdigest
-      package_file.verification_checksum = checksum
-
+    it 'returns nil for a non-existent file' do
       allow(package_file).to receive(:file_exist?).and_return(false)
 
-      expect { package_file.calculate_checksum! }
-        .to change { package_file.verification_checksum }.from(checksum).to(nil)
+      expect(package_file.calculate_checksum).to eq(nil)
     end
   end
 

ee/spec/replicators/geo/package_file_replicator_spec.rb

@@ -6,4 +6,8 @@ RSpec.describe Geo::PackageFileReplicator do
   let(:model_record) { build(:package_file, :npm) }
 
   include_examples 'a blob replicator'
+  # TODO: Move these examples to the blob and repo strategy shared examples so
+  # these get run for all Replicators.
+  # https://gitlab.com/gitlab-org/gitlab/-/issues/280768
+  it_behaves_like 'a verifiable replicator'
 end

ee/spec/services/geo/blob_verification_secondary_service_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
     it 'does not calculate the checksum when not running on a secondary' do
       stub_primary_node
 
-      expect(package_file).not_to receive(:calculate_checksum!)
+      expect(package_file).not_to receive(:calculate_checksum)
 
       service.execute
     end
@@ -29,7 +29,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
     it 'does not verify the checksum if resync is needed' do
       registry.resync
 
-      expect(package_file).not_to receive(:calculate_checksum!)
+      expect(package_file).not_to receive(:calculate_checksum)
 
       service.execute
     end
@@ -37,7 +37,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
     it 'does not verify the checksum if sync is started' do
       registry.start!
 
-      expect(package_file).not_to receive(:calculate_checksum!)
+      expect(package_file).not_to receive(:calculate_checksum)
 
       service.execute
     end
@@ -45,7 +45,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
     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!)
+      expect(package_file).not_to receive(:calculate_checksum)
 
       service.execute
     end
@@ -54,13 +54,13 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
       package_file.assign_attributes(verification_checksum: '62fc1ec4ce60')
       registry.update(verification_checksum: '62fc1ec4ce60')
 
-      expect(package_file).not_to receive(:calculate_checksum!)
+      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')
+      allow(package_file).to receive(:calculate_checksum).and_return('62fc1ec4ce60')
 
       service.execute
 
@@ -77,7 +77,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
 
     context 'when the checksum mismatch' do
       before do
-        allow(package_file).to receive(:calculate_checksum!).and_return('99fc1ec4ce60')
+        allow(package_file).to receive(:calculate_checksum).and_return('99fc1ec4ce60')
       end
 
       it 'keeps track of failures' do
@@ -109,7 +109,7 @@ RSpec.describe Geo::BlobVerificationSecondaryService, :geo do
 
     context 'when checksum calculation fails' do
       before do
-        allow(package_file).to receive(:calculate_checksum!).and_raise('Error calculating checksum')
+        allow(package_file).to receive(:calculate_checksum).and_raise('Error calculating checksum')
       end
 
       it 'keeps track of failures' do

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

@@ -19,7 +19,6 @@ 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'

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

@@ -19,7 +19,6 @@ RSpec.shared_examples 'a repository 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'

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

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

ee/spec/workers/geo/verification_worker_spec.rb

@@ -9,11 +9,11 @@ RSpec.describe Geo::VerificationWorker, :geo do
   let(:job_args) { ['package_file', package_file.id] }
 
   describe '#perform' do
-    it 'calls calculate_checksum!' do
+    it 'calls verify' do
       replicator = double(:replicator)
       allow(::Gitlab::Geo::Replicator).to receive(:for_replicable_params).with(replicable_name: 'package_file', replicable_id: package_file.id).and_return(replicator)
 
-      expect(replicator).to receive(:calculate_checksum!)
+      expect(replicator).to receive(:verify)
 
       described_class.new.perform(*job_args)
     end