Merge branch 'mk/fix-reverify-object-storage' into 'master'

Geo: Fix reverify object stored files

See merge request gitlab-org/gitlab!79469

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

@@ -55,6 +55,29 @@ module Geo
         self.verification_failed!
       end
 
+      override :track_checksum_attempt!
+      def track_checksum_attempt!(&block)
+        # If this resource will never become checksummed on the primary (because
+        # e.g. it is a remote stored file), then as a bandaid, mark it as
+        # verification succeeded. This will stop the cycle of:
+        # Sync succeeded => Verification failed => Sync failed => Sync succeeded
+        #
+        # A better fix is proposed in
+        # https://gitlab.com/gitlab-org/gitlab/-/issues/299819
+        if will_never_be_checksummed_on_the_primary?
+          # To ensure we avoid transition errors
+          self.verification_started unless self.verification_started?
+
+          # A checksum value is required by a state machine validation rule, so
+          # set it to zeroes
+          self.verification_checksum = '0000000000000000000000000000000000000000'
+          self.verification_succeeded!
+          return
+        end
+
+        super
+      end
+
       private
 
       override :track_checksum_result!
@@ -90,10 +113,8 @@ module Geo
       self.verification_pending!
     end
 
-    # For example, remote stored files are filtered from available_verifiables
-    # because we don't support verification of remote stored files.
     def will_never_be_checksummed_on_the_primary?
-      !replicator.model_record.in_available_verifiables?
+      replicator.will_never_be_checksummed_on_the_primary?
     end
 
     override :before_verification_failed

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

@@ -262,6 +262,12 @@ module Geo
       Gitlab::Geo.secondary? ? registry : model_record
     end
 
+    # For example, remote stored files are filtered from available_verifiables
+    # because we don't support verification of remote stored files.
+    def will_never_be_checksummed_on_the_primary?
+      !model_record.in_available_verifiables?
+    end
+
     # @abstract
     # @return [String] a checksum representing the data
     def calculate_checksum

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

@@ -565,6 +565,24 @@ RSpec.shared_examples 'a verifiable replicator' do
     end
   end
 
+  describe '#will_never_be_checksummed_on_the_primary?' do
+    context 'when the model record is not in available_verifiables' do
+      it 'returns true' do
+        allow(model_record).to receive(:in_available_verifiables?).and_return(false)
+
+        expect(replicator.will_never_be_checksummed_on_the_primary?).to be_truthy
+      end
+    end
+
+    context 'when the model record is in available_verifiables' do
+      it 'returns false' do
+        allow(model_record).to receive(:in_available_verifiables?).and_return(true)
+
+        expect(replicator.will_never_be_checksummed_on_the_primary?).to be_falsey
+      end
+    end
+  end
+
   context 'integration tests' do
     before do
       model_record.save!

ee/spec/support/shared_examples/models/geo_verifiable_registry_shared_examples.rb

@@ -204,6 +204,8 @@ RSpec.shared_examples 'a Geo verifiable registry' do
   describe '#track_checksum_attempt!', :aggregate_failures do
     context 'when verification was not yet started' do
       it 'starts verification' do
+        allow(subject).to receive(:will_never_be_checksummed_on_the_primary?).and_return(false)
+
         expect do
           subject.track_checksum_attempt! do
             'a_checksum_value'
@@ -211,45 +213,85 @@ RSpec.shared_examples 'a Geo verifiable registry' do
         end.to change { subject.verification_started_at }.from(nil)
       end
 
-      context 'comparison with primary checksum' do
-        let(:replicator) { double('replicator') }
-        let(:calculated_checksum) { 'abc123' }
-
+      context 'when the model record will never be checksummed on the primary' do
         before do
-          allow(subject).to receive(:replicator).and_return(replicator)
-          allow(replicator).to receive(:matches_checksum?).with(calculated_checksum).and_return(matches_checksum)
+          allow(registry).to receive(:will_never_be_checksummed_on_the_primary?).and_return(true)
         end
 
-        context 'when the calculated checksum matches the primary checksum' do
-          let(:matches_checksum) { true }
+        context 'when the registry is already verification_succeeded' do
+          let(:registry) { create(registry_class_factory, :started, :verification_succeeded) }
 
-          it 'transitions to verification_succeeded and updates the checksum' do
+          it 'leaves verification as succeeded' do
             expect do
-              subject.track_checksum_attempt! do
-                calculated_checksum
+              registry.track_checksum_attempt! do
+                ''
               end
-            end.to change { subject.verification_succeeded? }.from(false).to(true)
+            end.not_to change { registry.verification_succeeded? }
 
-            expect(subject.verification_checksum).to eq(calculated_checksum)
+            expect(registry.verification_checksum).to eq('0000000000000000000000000000000000000000')
           end
         end
 
-        context 'when the calculated checksum does not match the primary checksum' do
-          let(:matches_checksum) { false }
-
-          it 'transitions to verification_failed and updates mismatch fields' do
-            allow(replicator).to receive(:primary_checksum).and_return(calculated_checksum)
+        context 'when the registry is verification_pending' do
+          let(:registry) { create(registry_class_factory, :started) }
 
+          it 'changes verification to succeeded' do
             expect do
-              subject.track_checksum_attempt! do
-                calculated_checksum
+              registry.track_checksum_attempt! do
+                ''
               end
-            end.to change { subject.verification_failed? }.from(false).to(true)
+            end.to change { registry.verification_succeeded? }.from(false).to(true)
 
-            expect(subject.verification_checksum).to eq(calculated_checksum)
-            expect(subject.verification_checksum_mismatched).to eq(calculated_checksum)
-            expect(subject.checksum_mismatch).to eq(true)
-            expect(subject.verification_failure).to match('Checksum does not match the primary checksum')
+            expect(registry.verification_checksum).to eq('0000000000000000000000000000000000000000')
+          end
+        end
+      end
+
+      context 'when the primary site is expected to checksum the model record' do
+        before do
+          allow(replicator).to receive(:will_never_be_checksummed_on_the_primary?).and_return(false)
+        end
+
+        context 'comparison with primary checksum' do
+          let(:replicator) { double('replicator') }
+          let(:calculated_checksum) { 'abc123' }
+
+          before do
+            allow(subject).to receive(:replicator).and_return(replicator)
+            allow(replicator).to receive(:matches_checksum?).with(calculated_checksum).and_return(matches_checksum)
+          end
+
+          context 'when the calculated checksum matches the primary checksum' do
+            let(:matches_checksum) { true }
+
+            it 'transitions to verification_succeeded and updates the checksum' do
+              expect do
+                subject.track_checksum_attempt! do
+                  calculated_checksum
+                end
+              end.to change { subject.verification_succeeded? }.from(false).to(true)
+
+              expect(subject.verification_checksum).to eq(calculated_checksum)
+            end
+          end
+
+          context 'when the calculated checksum does not match the primary checksum' do
+            let(:matches_checksum) { false }
+
+            it 'transitions to verification_failed and updates mismatch fields' do
+              allow(replicator).to receive(:primary_checksum).and_return(calculated_checksum)
+
+              expect do
+                subject.track_checksum_attempt! do
+                  calculated_checksum
+                end
+              end.to change { subject.verification_failed? }.from(false).to(true)
+
+              expect(subject.verification_checksum).to eq(calculated_checksum)
+              expect(subject.verification_checksum_mismatched).to eq(calculated_checksum)
+              expect(subject.checksum_mismatch).to eq(true)
+              expect(subject.verification_failure).to match('Checksum does not match the primary checksum')
+            end
           end
         end
       end
@@ -257,6 +299,8 @@ RSpec.shared_examples 'a Geo verifiable registry' do
 
     context 'when verification was started' do
       it 'does not update verification_started_at' do
+        allow(subject).to receive(:will_never_be_checksummed_on_the_primary?).and_return(false)
+
         subject.verification_started!
         expected = subject.verification_started_at
 
@@ -269,6 +313,8 @@ RSpec.shared_examples 'a Geo verifiable registry' do
     end
 
     it 'yields to the checksum calculation' do
+      allow(subject).to receive(:will_never_be_checksummed_on_the_primary?).and_return(false)
+
       expect do |probe|
         subject.track_checksum_attempt!(&probe)
       end.to yield_with_no_args
@@ -276,6 +322,8 @@ RSpec.shared_examples 'a Geo verifiable registry' do
 
     context 'when an error occurs while yielding' do
       it 'sets verification_failed' do
+        allow(subject).to receive(:will_never_be_checksummed_on_the_primary?).and_return(false)
+
         subject.track_checksum_attempt! do
           raise 'an error'
         end
@@ -285,28 +333,6 @@ RSpec.shared_examples 'a Geo verifiable registry' do
     end
   end
 
-  describe '#will_never_be_checksummed_on_the_primary?' do
-    context 'when the model record is not in available_verifiables' do
-      it 'returns true' do
-        model_record = double('model_record', in_available_verifiables?: false)
-        replicator = double('replicator', model_record: model_record)
-        allow(subject).to receive(:replicator).and_return(replicator)
-
-        expect(subject.will_never_be_checksummed_on_the_primary?).to be_truthy
-      end
-    end
-
-    context 'when the model record is in available_verifiables' do
-      it 'returns false' do
-        model_record = double('model_record', in_available_verifiables?: true)
-        replicator = double('replicator', model_record: model_record)
-        allow(subject).to receive(:replicator).and_return(replicator)
-
-        expect(subject.will_never_be_checksummed_on_the_primary?).to be_falsey
-      end
-    end
-  end
-
   def verification_state_value(key)
     described_class.verification_state_value(key)
   end