Geo: (SSF) State machine refactoring

Implement state machine on a state model instead
of the main model.

Changelog: other

.gitlab/issue_templates/Geo Replicate a new Git repository type.md

@@ -193,6 +193,8 @@ That's all of the required database changes.
     include ::Geo::ReplicableModel
     include ::Geo::VerifiableModel
 
+    delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :cool_widget_state)
+
     with_replicator Geo::CoolWidgetReplicator
 
     mount_uploader :file, CoolWidgetUploader
@@ -201,16 +203,6 @@ That's all of the required database changes.
 
     after_save :save_verification_details
 
-    delegate :verification_retry_at, :verification_retry_at=,
-             :verified_at, :verified_at=,
-             :verification_checksum, :verification_checksum=,
-             :verification_failure, :verification_failure=,
-             :verification_retry_count, :verification_retry_count=,
-             :verification_state=, :verification_state,
-             :verification_started_at=, :verification_started_at,
-             to: :cool_widget_state
-    ...
-
     scope :with_verification_state, ->(state) { joins(:cool_widget_state).where(cool_widget_states: { verification_state: verification_state_value(state) }) }
     scope :checksummed, -> { joins(:cool_widget_state).where.not(cool_widget_states: { verification_checksum: nil } ) }
     scope :not_checksummed, -> { joins(:cool_widget_state).where(cool_widget_states: { verification_checksum: nil } ) }
@@ -487,6 +479,7 @@ That's all of the required database changes.
   module Geo
     class CoolWidgetState < ApplicationRecord
       include EachBatch
+      include ::Geo::VerificationStateDefinition
 
       self.primary_key = :cool_widget_id
 

.gitlab/issue_templates/Geo Replicate a new blob type.md

@@ -195,6 +195,8 @@ That's all of the required database changes.
     include ::Geo::ReplicableModel
     include ::Geo::VerifiableModel
 
+    delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :cool_widget_state)
+
     with_replicator Geo::CoolWidgetReplicator
 
     mount_uploader :file, CoolWidgetUploader
@@ -203,16 +205,6 @@ That's all of the required database changes.
 
     after_save :save_verification_details
 
-    delegate :verification_retry_at, :verification_retry_at=,
-             :verified_at, :verified_at=,
-             :verification_checksum, :verification_checksum=,
-             :verification_failure, :verification_failure=,
-             :verification_retry_count, :verification_retry_count=,
-             :verification_state=, :verification_state,
-             :verification_started_at=, :verification_started_at,
-             to: :cool_widget_state
-    ...
-
     scope :with_verification_state, ->(state) { joins(:cool_widget_state).where(cool_widget_states: { verification_state: verification_state_value(state) }) }
     scope :checksummed, -> { joins(:cool_widget_state).where.not(cool_widget_states: { verification_checksum: nil } ) }
     scope :not_checksummed, -> { joins(:cool_widget_state).where(cool_widget_states: { verification_checksum: nil } ) }
@@ -451,6 +443,7 @@ That's all of the required database changes.
   module Geo
     class CoolWidgetState < ApplicationRecord
       include EachBatch
+      include ::Geo::VerificationStateDefinition
 
       self.primary_key = :cool_widget_id
 

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

@@ -5,6 +5,7 @@ module Geo
     extend ActiveSupport::Concern
     extend ::Gitlab::Utils::Override
     include ::Geo::VerificationState
+    include ::Geo::VerificationStateDefinition
 
     class_methods do
       extend ::Gitlab::Utils::Override

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

@@ -19,6 +19,19 @@ module Geo
     }.freeze
 
     VERIFICATION_TIMEOUT = 8.hours
+    VERIFICATION_METHODS = [:verification_retry_at, :verification_retry_at=,
+                            :verified_at, :verified_at=, :verification_failed?,
+                            :verification_checksum, :verification_checksum=,
+                            :verification_failure, :verification_failure=,
+                            :verification_retry_count, :verification_retry_count=,
+                            :verification_state=, :verification_state,
+                            :verification_started_at=, :verification_started_at,
+                            :verification_started!, :verification_pending!,
+                            :verification_succeeded!, :verification_failed!,
+                            :verification_started?, :verification_succeeded,
+                            :with_verification_state, :verification_started,
+                            :verification_succeeded?, :verification_failed,
+                            :verification_pending].freeze
 
     included do
       sha_attribute :verification_checksum
@@ -34,57 +47,6 @@ module Geo
       scope :needs_verification, -> { available_verifiables.merge(with_verification_state(:verification_pending).or(with_verification_state(:verification_failed).verification_retry_due)) }
       scope :needs_reverification, -> { verification_succeeded.where("verified_at < ?", ::Gitlab::Geo.current_node.minimum_reverification_interval.days.ago) }
 
-      state_machine :verification_state, initial: :verification_pending do
-        state :verification_pending, value: VERIFICATION_STATE_VALUES[:verification_pending]
-        state :verification_started, value: VERIFICATION_STATE_VALUES[:verification_started]
-        state :verification_succeeded, value: VERIFICATION_STATE_VALUES[:verification_succeeded] do
-          validates :verification_checksum, presence: true
-        end
-        state :verification_failed, value: VERIFICATION_STATE_VALUES[:verification_failed] do
-          validates :verification_failure, presence: true
-        end
-
-        before_transition any => :verification_started do |instance, _|
-          instance.verification_started_at = Time.current
-        end
-
-        before_transition [:verification_pending, :verification_started, :verification_succeeded] => :verification_pending do |instance, _|
-          instance.clear_verification_failure_fields!
-        end
-
-        before_transition verification_failed: :verification_pending do |instance, _|
-          # If transitioning from verification_failed, then don't clear
-          # verification_retry_count and verification_retry_at to ensure
-          # progressive backoff of syncs-due-to-verification-failures
-          instance.verification_failure = nil
-        end
-
-        before_transition any => :verification_failed do |instance, _|
-          instance.before_verification_failed
-        end
-
-        before_transition any => :verification_succeeded do |instance, _|
-          instance.verified_at = Time.current
-          instance.clear_verification_failure_fields!
-        end
-
-        event :verification_started do
-          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_started
-        end
-
-        event :verification_succeeded do
-          transition verification_started: :verification_succeeded
-        end
-
-        event :verification_failed do
-          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_failed
-        end
-
-        event :verification_pending do
-          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_pending
-        end
-      end
-
       private_class_method :start_verification_batch
       private_class_method :start_verification_batch_query
       private_class_method :start_verification_batch_subselect
@@ -275,21 +237,6 @@ module Geo
       end
     end
 
-    # Overridden by Geo::VerifiableRegistry
-    def clear_verification_failure_fields!
-      self.verification_retry_count = 0
-      self.verification_retry_at = nil
-      self.verification_failure = nil
-    end
-
-    # Overridden by Geo::VerifiableRegistry
-    def before_verification_failed
-      self.verification_retry_count ||= 0
-      self.verification_retry_count += 1
-      self.verification_retry_at = self.next_retry_time(self.verification_retry_count)
-      self.verified_at = Time.current
-    end
-
     # Provides a safe and easy way to manage the verification state for a
     # synchronous checksum calculation.
     #

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

+# frozen_string_literal: true
+
+module Geo
+  module VerificationStateDefinition
+    extend ActiveSupport::Concern
+    include Delay
+
+    included do
+      state_machine :verification_state, initial: :verification_pending do
+        state :verification_pending, value: VerificationState::VERIFICATION_STATE_VALUES[:verification_pending]
+        state :verification_started, value: VerificationState::VERIFICATION_STATE_VALUES[:verification_started]
+        state :verification_succeeded, value: VerificationState::VERIFICATION_STATE_VALUES[:verification_succeeded] do
+          validates :verification_checksum, presence: true
+        end
+        state :verification_failed, value: VerificationState::VERIFICATION_STATE_VALUES[:verification_failed] do
+          validates :verification_failure, presence: true
+        end
+
+        before_transition any => :verification_started do |instance, _|
+          instance.verification_started_at = Time.current
+        end
+
+        before_transition [:verification_pending, :verification_started, :verification_succeeded] => :verification_pending do |instance, _|
+          instance.clear_verification_failure_fields!
+        end
+
+        before_transition verification_failed: :verification_pending do |instance, _|
+          # If transitioning from verification_failed, then don't clear
+          # verification_retry_count and verification_retry_at to ensure
+          # progressive backoff of syncs-due-to-verification-failures
+          instance.verification_failure = nil
+        end
+
+        before_transition any => :verification_failed do |instance, _|
+          instance.before_verification_failed
+        end
+
+        before_transition any => :verification_succeeded do |instance, _|
+          instance.verified_at = Time.current
+          instance.clear_verification_failure_fields!
+        end
+
+        event :verification_started do
+          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_started
+        end
+
+        event :verification_succeeded do
+          transition verification_started: :verification_succeeded
+        end
+
+        event :verification_failed do
+          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_failed
+        end
+
+        event :verification_pending do
+          transition [:verification_pending, :verification_started, :verification_succeeded, :verification_failed] => :verification_pending
+        end
+      end
+    end
+
+    # Overridden by Geo::VerifiableRegistry
+    def clear_verification_failure_fields!
+      self.verification_retry_count = 0
+      self.verification_retry_at = nil
+      self.verification_failure = nil
+    end
+
+    # Overridden by Geo::VerifiableRegistry
+    def before_verification_failed
+      self.verification_retry_count ||= 0
+      self.verification_retry_count += 1
+      self.verification_retry_at = self.next_retry_time(self.verification_retry_count)
+      self.verified_at = Time.current
+    end
+  end
+end

ee/app/models/ee/ci/job_artifact.rb

@@ -15,6 +15,8 @@ module EE
       include ::Geo::ReplicableModel
       include ::Geo::VerifiableModel
 
+      delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :job_artifact_state)
+
       with_replicator ::Geo::JobArtifactReplicator
 
       has_one :job_artifact_state, autosave: false, inverse_of: :job_artifact, class_name: '::Geo::JobArtifactState'
@@ -83,15 +85,6 @@ module EE
 
       delegate :validate_schema?, to: :job
 
-      delegate :verification_retry_at, :verification_retry_at=,
-               :verified_at, :verified_at=,
-               :verification_checksum, :verification_checksum=,
-               :verification_failure, :verification_failure=,
-               :verification_retry_count, :verification_retry_count=,
-               :verification_state=, :verification_state,
-               :verification_started_at=, :verification_started_at,
-               to: :job_artifact_state
-
       after_save :save_verification_details
     end
 

ee/app/models/ee/ci/pipeline_artifact.rb

@@ -8,6 +8,7 @@ module EE
       prepended do
         include ::Geo::ReplicableModel
         include ::Geo::VerifiableModel
+        include ::Geo::VerificationStateDefinition
 
         with_replicator ::Geo::PipelineArtifactReplicator
       end

ee/app/models/ee/lfs_object.rb

@@ -14,6 +14,8 @@ module EE
       include ::Geo::ReplicableModel
       include ::Geo::VerifiableModel
 
+      delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :lfs_object_state)
+
       with_replicator ::Geo::LfsObjectReplicator
 
       scope :project_id_in, ->(ids) { joins(:projects).merge(::Project.id_in(ids)) }
@@ -22,15 +24,6 @@ module EE
 
       after_save :save_verification_details
 
-      delegate :verification_retry_at, :verification_retry_at=,
-             :verified_at, :verified_at=,
-             :verification_checksum, :verification_checksum=,
-             :verification_failure, :verification_failure=,
-             :verification_retry_count, :verification_retry_count=,
-             :verification_state=, :verification_state,
-             :verification_started_at=, :verification_started_at,
-             to: :lfs_object_state, allow_nil: true
-
       scope :with_verification_state, ->(state) { joins(:lfs_object_state).where(lfs_object_states: { verification_state: verification_state_value(state) }) }
       scope :checksummed, -> { joins(:lfs_object_state).where.not(lfs_object_states: { verification_checksum: nil } ) }
       scope :not_checksummed, -> { joins(:lfs_object_state).where(lfs_object_states: { verification_checksum: nil } ) }

ee/app/models/ee/merge_request_diff.rb

@@ -9,6 +9,8 @@ module EE
       include ObjectStorable
       include ::Geo::VerifiableModel
 
+      delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :merge_request_diff_detail)
+
       STORE_COLUMN = :external_diff_store
 
       with_replicator ::Geo::MergeRequestDiffReplicator
@@ -17,15 +19,6 @@ module EE
 
       after_save :save_verification_details
 
-      delegate :verification_retry_at, :verification_retry_at=,
-               :verified_at, :verified_at=,
-               :verification_checksum, :verification_checksum=,
-               :verification_failure, :verification_failure=,
-               :verification_retry_count, :verification_retry_count=,
-               :verification_state=, :verification_state,
-               :verification_started_at=, :verification_started_at,
-        to: :merge_request_diff_detail, allow_nil: true
-
       scope :has_external_diffs, -> { with_files.where(stored_externally: true) }
       scope :project_id_in, ->(ids) { where(merge_request_id: ::MergeRequest.where(target_project_id: ids)) }
       scope :available_replicables, -> { has_external_diffs }

ee/app/models/ee/packages/package_file.rb

@@ -8,6 +8,7 @@ module EE
       prepended do
         include ::Geo::ReplicableModel
         include ::Geo::VerifiableModel
+        include ::Geo::VerificationStateDefinition
 
         with_replicator ::Geo::PackageFileReplicator
       end

ee/app/models/ee/pages_deployment.rb

@@ -8,21 +8,14 @@ module EE
       include ::Geo::ReplicableModel
       include ::Geo::VerifiableModel
 
+      delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :pages_deployment_state)
+
       with_replicator ::Geo::PagesDeploymentReplicator
 
       has_one :pages_deployment_state, autosave: false, inverse_of: :pages_deployment, class_name: '::Geo::PagesDeploymentState'
 
       after_save :save_verification_details
 
-      delegate :verification_retry_at, :verification_retry_at=,
-         :verified_at, :verified_at=,
-         :verification_checksum, :verification_checksum=,
-         :verification_failure, :verification_failure=,
-         :verification_retry_count, :verification_retry_count=,
-         :verification_state=, :verification_state,
-         :verification_started_at=, :verification_started_at,
-         to: :pages_deployment_state
-
       scope :with_verification_state, ->(state) { joins(:pages_deployment_state).where(pages_deployment_states: { verification_state: verification_state_value(state) }) }
       scope :checksummed, -> { joins(:pages_deployment_state).where.not(pages_deployment_states: { verification_checksum: nil } ) }
       scope :not_checksummed, -> { joins(:pages_deployment_state).where(pages_deployment_states: { verification_checksum: nil } ) }

ee/app/models/ee/snippet_repository.rb

@@ -7,6 +7,7 @@ module EE
     prepended do
       include ::Geo::ReplicableModel
       include ::Geo::VerifiableModel
+      include ::Geo::VerificationStateDefinition
       include FromUnion
 
       with_replicator ::Geo::SnippetRepositoryReplicator

ee/app/models/ee/terraform/state_version.rb

@@ -8,6 +8,7 @@ module EE
       prepended do
         include ::Geo::ReplicableModel
         include ::Geo::VerifiableModel
+        include ::Geo::VerificationStateDefinition
 
         with_replicator ::Geo::TerraformStateVersionReplicator
 

ee/app/models/ee/upload.rb

@@ -13,6 +13,8 @@ module EE
       include ::Geo::ReplicableModel
       include ::Geo::VerifiableModel
 
+      delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :upload_state)
+
       with_replicator ::Geo::UploadReplicator
 
       scope :for_model, ->(model) { where(model_id: model.id, model_type: model.class.name) }
@@ -28,15 +30,6 @@ module EE
               inverse_of: :upload,
               class_name: '::Geo::UploadState'
 
-      delegate :verification_retry_at, :verification_retry_at=,
-               :verified_at, :verified_at=,
-               :verification_checksum, :verification_checksum=,
-               :verification_failure, :verification_failure=,
-               :verification_retry_count, :verification_retry_count=,
-               :verification_state=, :verification_state,
-               :verification_started_at=, :verification_started_at,
-               to: :upload_state
-
       after_save :save_verification_details
 
       def verification_state_object

ee/app/models/geo/job_artifact_state.rb

@@ -3,6 +3,7 @@
 module Geo
   class JobArtifactState < Ci::ApplicationRecord
     include EachBatch
+    include ::Geo::VerificationStateDefinition
 
     self.primary_key = :job_artifact_id
 

ee/app/models/geo/lfs_object_state.rb

@@ -2,6 +2,7 @@
 
 module Geo
   class LfsObjectState < ApplicationRecord
+    include ::Geo::VerificationStateDefinition
     include EachBatch
 
     self.primary_key = :lfs_object_id

ee/app/models/geo/pages_deployment_state.rb

@@ -2,6 +2,7 @@
 
 module Geo
   class PagesDeploymentState < ApplicationRecord
+    include ::Geo::VerificationStateDefinition
     include EachBatch
 
     self.primary_key = :pages_deployment_id

ee/app/models/geo/upload_state.rb

@@ -2,6 +2,7 @@
 
 module Geo
   class UploadState < ApplicationRecord
+    include ::Geo::VerificationStateDefinition
     include EachBatch
 
     self.primary_key = :upload_id

ee/app/models/merge_request_diff_detail.rb

 # frozen_string_literal: true
 
 class MergeRequestDiffDetail < ApplicationRecord
+  include ::Geo::VerificationStateDefinition
   include EachBatch
 
   self.primary_key = :merge_request_diff_id

ee/spec/models/concerns/geo/verification_state_spec.rb

@@ -423,6 +423,12 @@ RSpec.describe Geo::VerificationState do
           expect(subject.verification_checksum).to be_nil
         end
       end
+
+      describe '#verification_started!' do
+        it 'flips the state to started state' do
+          expect { subject.verification_started! }.to change { subject.verification_state }.from(0).to(1)
+        end
+      end
     end
 
     context 'when verification state is stored in a separate table' do
@@ -451,17 +457,34 @@ RSpec.describe Geo::VerificationState do
           expect(subject.reload.verification_failed?).to be_truthy
         end
       end
+
+      describe '#verification_started!' do
+        it 'flips the state to started state without reseting/reloading the original object (only state record)' do
+          subject.verification_failure = 'draft changes'
+
+          expect { subject.verification_started! }.to change { subject.verification_state }.from(0).to(1)
+          expect(subject.verification_failure).to eq('draft changes')
+        end
+      end
     end
   end
 
   context 'for registry classes' do
     describe '.fail_verification_timeouts' do
       it 'sets verification state to failed' do
-        state = create(:geo_package_file_registry, :synced, verification_started_at: (described_class::VERIFICATION_TIMEOUT + 1.minute).ago, verification_state: 1)
+        registry = create(:geo_package_file_registry, :synced, verification_started_at: (described_class::VERIFICATION_TIMEOUT + 1.minute).ago, verification_state: 1)
+
+        registry.class.fail_verification_timeouts
+
+        expect(registry.reload.verification_failed?).to be_truthy
+      end
+    end
 
-        state.class.fail_verification_timeouts
+    describe '#verification_started!' do
+      it 'flips the state to started state' do
+        registry = create(:geo_package_file_registry)
 
-        expect(state.reload.verification_failed?).to be_truthy
+        expect { registry.verification_started! }.to change { registry.verification_state }.from(0).to(1)
       end
     end
   end

ee/spec/replicators/geo/merge_request_diff_replicator_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Geo::MergeRequestDiffReplicator do
-  let(:model_record) { build(:merge_request_diff, :external) }
+  let(:model_record) { create(:merge_request_diff, :external) }
 
   include_examples 'a blob replicator'
   include_examples 'a verifiable replicator'

ee/spec/replicators/geo/pages_deployment_replicator_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Geo::PagesDeploymentReplicator do
-  let(:model_record) { build(:pages_deployment) }
+  let(:model_record) { create(:pages_deployment) }
 
   include_examples 'a blob replicator'
   include_examples 'a verifiable replicator'

ee/spec/support/helpers/ee/geo_helpers.rb

@@ -85,6 +85,7 @@ module EE
       DummyModel.class_eval do
         include ::Geo::ReplicableModel
         include ::Geo::VerifiableModel
+        include ::Geo::VerificationStateDefinition
 
         with_replicator Geo::DummyReplicator
 
@@ -177,6 +178,8 @@ module EE
         include ::Geo::ReplicableModel
         include ::Geo::VerifiableModel
 
+        delegate(*::Geo::VerificationState::VERIFICATION_METHODS, to: :_test_dummy_model_state)
+
         with_replicator Geo::DummyReplicator
 
         has_one :_test_dummy_model_state,
@@ -186,15 +189,6 @@ module EE
 
         after_save :save_verification_details
 
-        delegate :verification_retry_at, :verification_retry_at=,
-                 :verified_at, :verified_at=,
-                 :verification_checksum, :verification_checksum=,
-                 :verification_failure, :verification_failure=,
-                 :verification_retry_count, :verification_retry_count=,
-                 :verification_state=, :verification_state,
-                 :verification_started_at=, :verification_started_at,
-          to: :_test_dummy_model_state, allow_nil: true
-
         scope :available_verifiables, -> { joins(:_test_dummy_model_state) }
 
         def verification_state_object
@@ -222,6 +216,7 @@ module EE
 
       TestDummyModelState.class_eval do
         include EachBatch
+        include ::Geo::VerificationStateDefinition
 
         self.table_name = '_test_dummy_model_states'
         self.primary_key = '_test_dummy_model_with_separate_state_id'