Merge branch 'mk/make-repo-sync-retries-more-resilient' into 'master'

Geo: Reduce frequency of redownload attempts

See merge request gitlab-org/gitlab!70329

ee/app/models/geo/design_registry.rb

@@ -6,7 +6,7 @@ class Geo::DesignRegistry < Geo::BaseRegistry
   MODEL_CLASS = ::Project
   MODEL_FOREIGN_KEY = :project_id
 
-  RETRIES_BEFORE_REDOWNLOAD = 5
+  RETRIES_BEFORE_REDOWNLOAD = 10
 
   belongs_to :project
 
@@ -120,6 +120,6 @@ class Geo::DesignRegistry < Geo::BaseRegistry
   def should_be_redownloaded?
     return true if force_to_redownload
 
-    retry_count > RETRIES_BEFORE_REDOWNLOAD
+    retry_count.present? && retry_count > RETRIES_BEFORE_REDOWNLOAD && retry_count.odd?
   end
 end

ee/app/models/geo/project_registry.rb

@@ -10,7 +10,7 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
   MODEL_FOREIGN_KEY = :project_id
 
   REGISTRY_TYPES = %i{repository wiki}.freeze
-  RETRIES_BEFORE_REDOWNLOAD = 5
+  RETRIES_BEFORE_REDOWNLOAD = 10
 
   sha_attribute :repository_verification_checksum_sha
   sha_attribute :repository_verification_checksum_mismatched
@@ -409,7 +409,9 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
     ensure_valid_type!(type)
     return true if public_send("force_to_redownload_#{type}") # rubocop:disable GitlabSecurity/PublicSend
 
-    retry_count(type) > RETRIES_BEFORE_REDOWNLOAD
+    retries = retry_count(type)
+
+    retries > RETRIES_BEFORE_REDOWNLOAD && retries.odd?
   end
 
   def verification_retry_count(type)

ee/app/services/geo/framework_repository_sync_service.rb

@@ -17,7 +17,7 @@ module Geo
 
     LEASE_TIMEOUT    = 8.hours
     LEASE_KEY_PREFIX = 'geo_sync_ssf_service'
-    RETRIES_BEFORE_REDOWNLOAD = 5
+    RETRIES_BEFORE_REDOWNLOAD = 10
 
     def initialize(replicator)
       @replicator = replicator
@@ -245,7 +245,9 @@ module Geo
     def should_be_redownloaded?
       return true if registry.force_to_redownload
 
-      registry.retry_count > RETRIES_BEFORE_REDOWNLOAD
+      retries = registry.retry_count
+
+      retries.present? && retries > RETRIES_BEFORE_REDOWNLOAD && retries.odd?
     end
 
     def reschedule_sync

ee/spec/models/geo/design_registry_spec.rb

@@ -4,6 +4,7 @@ require 'spec_helper'
 
 RSpec.describe Geo::DesignRegistry, :geo do
   include ::EE::GeoHelpers
+  using RSpec::Parameterized::TableSyntax
 
   it_behaves_like 'a BulkInsertSafe model', Geo::DesignRegistry do
     let(:valid_items_for_bulk_insertion) do
@@ -266,25 +267,27 @@ RSpec.describe Geo::DesignRegistry, :geo do
   end
 
   describe '#should_be_redownloaded?' do
-    let_it_be(:design_registry) { create(:geo_design_registry) }
-
-    context 'when force_to_redownload is false' do
-      it 'returns false' do
-        expect(design_registry.should_be_redownloaded?).to be false
-      end
-
-      it 'returns true when limit is exceeded' do
-        design_registry.retry_count = Geo::DesignRegistry::RETRIES_BEFORE_REDOWNLOAD + 1
-
-        expect(design_registry.should_be_redownloaded?).to be true
-      end
-    end
-
-    context 'when force_to_redownload is true' do
-      it 'resets the state of the sync' do
-        design_registry.force_to_redownload = true
-
-        expect(design_registry.should_be_redownloaded?).to be true
+    where(:force_to_redownload, :retry_count, :expected) do
+      false | nil | false
+      false | 0   | false
+      false | 1   | false
+      false | 10  | false
+      false | 11  | true
+      false | 12  | false
+      false | 13  | true
+      false | 14  | false
+      false | 101 | true
+      false | 102 | false
+      true  | nil | true
+      true  | 0   | true
+      true  | 11  | true
+    end
+
+    with_them do
+      it "returns the expected boolean" do
+        registry = build(:geo_design_registry, retry_count: retry_count, force_to_redownload: force_to_redownload)
+
+        expect(registry.should_be_redownloaded?).to eq(expected)
       end
     end
   end

ee/spec/models/geo/project_registry_spec.rb

@@ -1259,4 +1259,68 @@ RSpec.describe Geo::ProjectRegistry, :geo do
       end
     end
   end
+
+  describe 'should_be_redownloaded?' do
+    context 'when type is invalid' do
+      it 'raises ArgumentError' do
+        registry = build(:geo_project_registry)
+
+        expect do
+          registry.should_be_redownloaded?(:invalid_type)
+        end.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'when type is repository' do
+      where(:force_to_redownload_repository, :repository_retry_count, :expected) do
+        false | nil | false
+        false | 0   | false
+        false | 1   | false
+        false | 10  | false
+        false | 11  | true
+        false | 12  | false
+        false | 13  | true
+        false | 14  | false
+        false | 101 | true
+        false | 102 | false
+        true  | nil | true
+        true  | 0   | true
+        true  | 11  | true
+      end
+
+      with_them do
+        it "returns the expected boolean" do
+          registry = build(:geo_project_registry, repository_retry_count: repository_retry_count, force_to_redownload_repository: force_to_redownload_repository)
+
+          expect(registry.should_be_redownloaded?(:repository)).to eq(expected)
+        end
+      end
+    end
+
+    context 'when type is wiki' do
+      where(:force_to_redownload_wiki, :wiki_retry_count, :expected) do
+        false | nil | false
+        false | 0   | false
+        false | 1   | false
+        false | 10  | false
+        false | 11  | true
+        false | 12  | false
+        false | 13  | true
+        false | 14  | false
+        false | 101 | true
+        false | 102 | false
+        true  | nil | true
+        true  | 0   | true
+        true  | 11  | true
+      end
+
+      with_them do
+        it "returns the expected boolean" do
+          registry = build(:geo_project_registry, wiki_retry_count: wiki_retry_count, force_to_redownload_wiki: force_to_redownload_wiki)
+
+          expect(registry.should_be_redownloaded?(:wiki)).to eq(expected)
+        end
+      end
+    end
+  end
 end

ee/spec/services/geo/framework_repository_sync_service_spec.rb

@@ -5,6 +5,7 @@ require 'spec_helper'
 RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
   include ::EE::GeoHelpers
   include ExclusiveLeaseHelpers
+  using RSpec::Parameterized::TableSyntax
 
   let_it_be(:primary) { create(:geo_node, :primary) }
   let_it_be(:secondary) { create(:geo_node) }
@@ -327,4 +328,30 @@ RSpec.describe Geo::FrameworkRepositorySyncService, :geo do
       end
     end
   end
+
+  describe '#should_be_redownloaded?' do
+    where(:force_to_redownload, :retry_count, :expected) do
+      false | nil | false
+      false | 0   | false
+      false | 1   | false
+      false | 10  | false
+      false | 11  | true
+      false | 12  | false
+      false | 13  | true
+      false | 14  | false
+      false | 101 | true
+      false | 102 | false
+      true  | nil | true
+      true  | 0   | true
+      true  | 11  | true
+    end
+
+    with_them do
+      it "returns the expected boolean" do
+        registry.update!(retry_count: retry_count, force_to_redownload: force_to_redownload)
+
+        expect(subject.send(:should_be_redownloaded?)).to eq(expected)
+      end
+    end
+  end
 end