Refactor some code and address review feedback

ee/app/models/geo/project_registry.rb

@@ -61,21 +61,21 @@ class Geo::ProjectRegistry < Geo::BaseRegistry
   end
 
   def syncs_since_gc
-    Gitlab::Redis::SharedState.with { |redis| redis.get(syncs_since_gc_redis_shared_state_key).to_i }
+    Gitlab::Redis::SharedState.with { |redis| redis.get(fetches_since_gc_redis_key).to_i }
   end
 
   def increment_syncs_since_gc
-    Gitlab::Redis::SharedState.with { |redis| redis.incr(syncs_since_gc_redis_shared_state_key) }
+    Gitlab::Redis::SharedState.with { |redis| redis.incr(fetches_since_gc_redis_key) }
   end
 
   def reset_syncs_since_gc
-    Gitlab::Redis::SharedState.with { |redis| redis.del(syncs_since_gc_redis_shared_state_key) }
+    Gitlab::Redis::SharedState.with { |redis| redis.del(fetches_since_gc_redis_key) }
   end
 
   private
 
-  def syncs_since_gc_redis_shared_state_key
-    "projects/#{id}/syncs_since_gc"
+  def fetches_since_gc_redis_key
+    "projects/#{project.id}/fetches_since_gc"
   end
 
   def never_synced_repository?

ee/app/services/geo/project_housekeeping_service.rb

@@ -7,9 +7,7 @@
 #
 module Geo
   class ProjectHousekeepingService < BaseService
-    # Timeout set to 24h
-    LEASE_TIMEOUT = 86400
-
+    LEASE_TIMEOUT = 24.hours
     attr_reader :project
 
     def initialize(project)
@@ -17,34 +15,37 @@ module Geo
     end
 
     def execute
-      lease_uuid = try_obtain_lease
-      return false unless lease_uuid.present?
-
-      yield if block_given?
-
-      execute_gitlab_shell_gc(lease_uuid)
+      increment!
+      do_housekeeping if needed?
     end
 
     def needed?
       syncs_since_gc > 0 && period_match? && housekeeping_enabled?
     end
 
+    def registry
+      @registry ||= Geo::ProjectRegistry.find_or_initialize_by(project_id: project.id)
+    end
+
+    private
+
     def increment!
       Gitlab::Metrics.measure(:geo_increment_syncs_since_gc) do
         registry.increment_syncs_since_gc
       end
     end
 
-    def registry
-      @registry ||= Geo::ProjectRegistry.find_or_initialize_by(project_id: project.id)
-    end
+    def do_housekeeping
+      lease_uuid = try_obtain_lease
+      return false unless lease_uuid.present?
 
-    private
+      execute_gitlab_shell_gc(lease_uuid)
+    end
 
     def execute_gitlab_shell_gc(lease_uuid)
       GitGarbageCollectWorker.perform_async(project.id, task, lease_key, lease_uuid)
     ensure
-      if syncs_since_gc >= gc_period
+      if should_reset?
         Gitlab::Metrics.measure(:geo_reset_syncs_since_gc) do
           registry.reset_syncs_since_gc
         end
@@ -58,6 +59,10 @@ module Geo
       end
     end
 
+    def should_reset?
+      syncs_since_gc >= gc_period
+    end
+
     def lease_key
       "geo_project_housekeeping:#{project.id}"
     end

ee/app/services/geo/repository_sync_service.rb

@@ -29,6 +29,7 @@ module Geo
     ensure
       clean_up_temporary_repository if redownload
       expire_repository_caches
+      execute_housekeeping
     end
 
     def mark_sync_as_successful
@@ -66,5 +67,9 @@ module Geo
     def schedule_repack
       GitGarbageCollectWorker.perform_async(@project.id, :full_repack, lease_key)
     end
+
+    def execute_housekeeping
+      Geo::ProjectHousekeepingService.new(project).execute
+    end
   end
 end

ee/app/workers/geo/project_sync_worker.rb

@@ -31,8 +31,6 @@ module Geo
 
       Geo::RepositorySyncService.new(project).execute if registry.repository_sync_due?(scheduled_time)
       Geo::WikiSyncService.new(project).execute if registry.wiki_sync_due?(scheduled_time)
-
-      execute_housekeeping(project)
     end
 
     private
@@ -55,11 +53,5 @@ module Geo
         log_info("#{success ? 'Successfully marked' : 'Failed to mark'} disabled wiki as synced", registry_id: registry.id, project_id: registry.project_id)
       end
     end
-
-    def execute_housekeeping(project)
-      housekeeping = Geo::ProjectHousekeepingService.new(project)
-      housekeeping.increment!
-      housekeeping.execute if housekeeping.needed?
-    end
   end
 end

ee/spec/services/geo/project_housekeeping_service_spec.rb

 require 'spec_helper'
 
 describe Geo::ProjectHousekeepingService do
-  subject { described_class.new(project) }
+  subject(:service) { described_class.new(project) }
   set(:project) { create(:project, :repository) }
-  let(:registry) { subject.registry }
+  let(:registry) { service.registry }
 
   before do
     registry.reset_syncs_since_gc
@@ -14,57 +14,36 @@ describe Geo::ProjectHousekeepingService do
   end
 
   describe '#execute' do
-    it 'enqueues a sidekiq job' do
-      expect(subject).to receive(:try_obtain_lease).and_return(:the_uuid)
-      expect(subject).to receive(:lease_key).and_return(:the_lease_key)
-      expect(subject).to receive(:task).and_return(:incremental_repack)
-      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :incremental_repack, :the_lease_key, :the_uuid).and_call_original
+    it 'executes housekeeping when conditions are fulfilled' do
+      allow(service).to receive(:needed?) { true }
 
-      Sidekiq::Testing.fake! do
-        expect { subject.execute }.to change(GitGarbageCollectWorker.jobs, :size).by(1)
-      end
+      expect(service).to receive(:do_housekeeping)
+
+      service.execute
     end
 
-    it 'yields the block if given' do
-      expect do |block|
-        subject.execute(&block)
-      end.to yield_with_no_args
+    it 'does not execute housekeeping when conditions are not fulfilled' do
+      allow(service).to receive(:needed?) { false }
+
+      expect(service).not_to receive(:do_housekeeping)
+
+      service.execute
     end
 
     it 'resets counter when syncs_since_gc > gc_period' do
-      expect(subject).to receive(:try_obtain_lease).and_return(:the_uuid)
-      allow(subject).to receive(:gc_period).and_return(1)
+      allow(service).to receive(:gc_period).and_return(1)
+      allow(service).to receive(:try_obtain_lease).and_return(:the_uuid)
       registry.increment_syncs_since_gc
 
       Sidekiq::Testing.inline! do
-        expect { subject.execute }.to change { registry.syncs_since_gc }.to(0)
-      end
-    end
-
-    context 'when no lease can be obtained' do
-      before do
-        expect(subject).to receive(:try_obtain_lease).and_return(false)
-      end
-
-      it 'does not enqueue a job' do
-        expect(GitGarbageCollectWorker).not_to receive(:perform_async)
-
-        expect(subject.execute).to be_falsey
-      end
-
-      it 'does not reset syncs_since_gc' do
-        expect { subject.execute }.not_to change { registry.syncs_since_gc }
-      end
-
-      it 'does not yield' do
-        expect { |block| subject.execute(&block) }.not_to yield_with_no_args
+        expect { service.execute }.to change { registry.syncs_since_gc }.to(0)
       end
     end
 
     context 'task type' do
       it 'goes through all three housekeeping tasks, executing only the highest task when there is overlap' do
-        allow(subject).to receive(:try_obtain_lease).and_return(:the_uuid)
-        allow(subject).to receive(:lease_key).and_return(:the_lease_key)
+        allow(service).to receive(:lease_key).and_return(:the_lease_key)
+        allow(service).to receive(:try_obtain_lease).and_return(:the_uuid)
 
         # At push 200
         expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid)
@@ -77,8 +56,7 @@ describe Geo::ProjectHousekeepingService do
           .exactly(16).times
 
         201.times do
-          subject.increment!
-          subject.execute if subject.needed?
+          service.execute
         end
 
         expect(registry.syncs_since_gc).to eq(1)
@@ -86,20 +64,52 @@ describe Geo::ProjectHousekeepingService do
     end
   end
 
+  describe 'do_housekeeping' do
+    context 'when no lease can be obtained' do
+      before do
+        expect(service).to receive(:try_obtain_lease).and_return(false)
+      end
+
+      it 'does not enqueue a job' do
+        expect(GitGarbageCollectWorker).not_to receive(:perform_async)
+
+        expect(service.send(:do_housekeeping)).to be_falsey
+      end
+
+      it 'does not reset syncs_since_gc' do
+        allow(service).to receive(:try_obtain_lease).and_return(false)
+        allow(service).to receive(:increment!)
+
+        expect { service.send(:do_housekeeping) }.not_to change { registry.syncs_since_gc }
+      end
+    end
+
+    it 'enqueues a sidekiq job' do
+      expect(service).to receive(:try_obtain_lease).and_return(:the_uuid)
+      expect(service).to receive(:lease_key).and_return(:the_lease_key)
+      expect(service).to receive(:task).and_return(:incremental_repack)
+      expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :incremental_repack, :the_lease_key, :the_uuid).and_call_original
+
+      Sidekiq::Testing.fake! do
+        expect { service.send(:do_housekeeping) }.to change(GitGarbageCollectWorker.jobs, :size).by(1)
+      end
+    end
+  end
+
   describe '#needed?' do
     it 'when the count is low enough' do
-      expect(subject.needed?).to eq(false)
+      expect(service.needed?).to eq(false)
     end
 
     it 'when the count is high enough' do
       allow(registry).to receive(:syncs_since_gc).and_return(10)
-      expect(subject.needed?).to eq(true)
+      expect(service.needed?).to eq(true)
     end
   end
 
   describe '#increment!' do
     it 'increments the syncs_since_gc counter' do
-      expect { subject.increment! }.to change { registry.syncs_since_gc }.by(1)
+      expect { service.send(:increment!) }.to change { registry.syncs_since_gc }.by(1)
     end
   end