Merge branch 'fj-move-gitgarbagecollectworker-to-projects-namespace' into 'master'

Move GitGarbageCollectWorker to Projects namespace

See merge request gitlab-org/gitlab!52064

.rubocop_manual_todo.yml

@@ -1435,7 +1435,6 @@ RSpec/AnyInstanceOf:
     - 'spec/workers/emails_on_push_worker_spec.rb'
     - 'spec/workers/error_tracking_issue_link_worker_spec.rb'
     - 'spec/workers/expire_pipeline_cache_worker_spec.rb'
-    - 'spec/workers/git_garbage_collect_worker_spec.rb'
     - 'spec/workers/group_export_worker_spec.rb'
     - 'spec/workers/group_import_worker_spec.rb'
     - 'spec/workers/namespaceless_project_destroy_worker_spec.rb'

app/services/projects/cleanup_service.rb

@@ -40,7 +40,7 @@ module Projects
       apply_bfg_object_map!
 
       # Remove older objects that are no longer referenced
-      GitGarbageCollectWorker.new.perform(project.id, :prune, "project_cleanup:gc:#{project.id}")
+      Projects::GitGarbageCollectWorker.new.perform(project.id, :prune, "project_cleanup:gc:#{project.id}")
 
       # The cache may now be inaccurate, and holding onto it could prevent
       # bugs assuming the presence of some object from manifesting for some

app/services/repositories/housekeeping_service.rb

@@ -45,7 +45,7 @@ module Repositories
     private
 
     def execute_gitlab_shell_gc(lease_uuid)
-      GitGarbageCollectWorker.perform_async(@resource.id, task, lease_key, lease_uuid)
+      Projects::GitGarbageCollectWorker.perform_async(@resource.id, task, lease_key, lease_uuid)
     ensure
       if pushes_since_gc >= gc_period
         Gitlab::Metrics.measure(:reset_pushes_since_gc) do

app/workers/all_queues.yml

@@ -1999,6 +1999,14 @@
   :weight: 1
   :idempotent: true
   :tags: []
+- :name: projects_git_garbage_collect
+  :feature_category: :gitaly
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: prometheus_create_default_alerts
   :feature_category: :incident_management
   :has_external_dependencies:

app/workers/git_garbage_collect_worker.rb

 # frozen_string_literal: true
 
+# According to our docs, we can only remove workers on major releases
+# https://docs.gitlab.com/ee/development/sidekiq_style_guide.html#removing-workers.
+#
+# We need to still maintain this until 14.0 but with the current functionality.
+#
+# In https://gitlab.com/gitlab-org/gitlab/-/issues/299290 we track that removal.
 class GitGarbageCollectWorker # rubocop:disable Scalability/IdempotentWorker
   include ApplicationWorker
 
@@ -7,117 +13,7 @@ class GitGarbageCollectWorker # rubocop:disable Scalability/IdempotentWorker
   feature_category :gitaly
   loggable_arguments 1, 2, 3
 
-  # Timeout set to 24h
-  LEASE_TIMEOUT = 86400
-
   def perform(project_id, task = :gc, lease_key = nil, lease_uuid = nil)
-    lease_key ||= "git_gc:#{task}:#{project_id}"
-    project = Project.find(project_id)
-    active_uuid = get_lease_uuid(lease_key)
-
-    if active_uuid
-      return unless active_uuid == lease_uuid
-
-      renew_lease(lease_key, active_uuid)
-    else
-      lease_uuid = try_obtain_lease(lease_key)
-
-      return unless lease_uuid
-    end
-
-    task = task.to_sym
-
-    if gc?(task)
-      ::Projects::GitDeduplicationService.new(project).execute
-      cleanup_orphan_lfs_file_references(project)
-    end
-
-    gitaly_call(task, project)
-
-    # Refresh the branch cache in case garbage collection caused a ref lookup to fail
-    flush_ref_caches(project) if gc?(task)
-
-    update_repository_statistics(project) if task != :pack_refs
-
-    # In case pack files are deleted, release libgit2 cache and open file
-    # descriptors ASAP instead of waiting for Ruby garbage collection
-    project.cleanup
-  ensure
-    cancel_lease(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present?
-  end
-
-  private
-
-  def gc?(task)
-    task == :gc || task == :prune
-  end
-
-  def try_obtain_lease(key)
-    ::Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain
-  end
-
-  def renew_lease(key, uuid)
-    ::Gitlab::ExclusiveLease.new(key, uuid: uuid, timeout: LEASE_TIMEOUT).renew
-  end
-
-  def cancel_lease(key, uuid)
-    ::Gitlab::ExclusiveLease.cancel(key, uuid)
-  end
-
-  def get_lease_uuid(key)
-    ::Gitlab::ExclusiveLease.get_uuid(key)
-  end
-
-  def gitaly_call(task, project)
-    repository = project.repository.raw_repository
-
-    client = if task == :pack_refs
-               Gitlab::GitalyClient::RefService.new(repository)
-             else
-               Gitlab::GitalyClient::RepositoryService.new(repository)
-             end
-
-    case task
-    when :prune, :gc
-      client.garbage_collect(bitmaps_enabled?, prune: task == :prune)
-    when :full_repack
-      client.repack_full(bitmaps_enabled?)
-    when :incremental_repack
-      client.repack_incremental
-    when :pack_refs
-      client.pack_refs
-    end
-  rescue GRPC::NotFound => e
-    Gitlab::GitLogger.error("#{__method__} failed:\nRepository not found")
-    raise Gitlab::Git::Repository::NoRepository.new(e)
-  rescue GRPC::BadStatus => e
-    Gitlab::GitLogger.error("#{__method__} failed:\n#{e}")
-    raise Gitlab::Git::CommandError.new(e)
-  end
-
-  def cleanup_orphan_lfs_file_references(project)
-    return if Gitlab::Database.read_only? # GitGarbageCollectWorker may be run on a Geo secondary
-
-    ::Gitlab::Cleanup::OrphanLfsFileReferences.new(project, dry_run: false, logger: logger).run!
-  rescue => err
-    Gitlab::GitLogger.warn(message: "Cleaning up orphan LFS objects files failed", error: err.message)
-    Gitlab::ErrorTracking.track_and_raise_for_dev_exception(err)
-  end
-
-  def flush_ref_caches(project)
-    project.repository.expire_branches_cache
-    project.repository.branch_names
-    project.repository.has_visible_content?
-  end
-
-  def update_repository_statistics(project)
-    project.repository.expire_statistics_caches
-    return if Gitlab::Database.read_only? # GitGarbageCollectWorker may be run on a Geo secondary
-
-    Projects::UpdateStatisticsService.new(project, nil, statistics: [:repository_size, :lfs_objects_size]).execute
-  end
-
-  def bitmaps_enabled?
-    Gitlab::CurrentSettings.housekeeping_bitmaps_enabled
+    ::Projects::GitGarbageCollectWorker.new.perform(project_id, task, lease_key, lease_uuid)
   end
 end

app/workers/projects/git_garbage_collect_worker.rb

+# frozen_string_literal: true
+
+module Projects
+  class GitGarbageCollectWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    sidekiq_options retry: false
+    feature_category :gitaly
+    loggable_arguments 1, 2, 3
+
+    # Timeout set to 24h
+    LEASE_TIMEOUT = 86400
+
+    def perform(project_id, task = :gc, lease_key = nil, lease_uuid = nil)
+      lease_key ||= "git_gc:#{task}:#{project_id}"
+      project = find_project(project_id)
+      active_uuid = get_lease_uuid(lease_key)
+
+      if active_uuid
+        return unless active_uuid == lease_uuid
+
+        renew_lease(lease_key, active_uuid)
+      else
+        lease_uuid = try_obtain_lease(lease_key)
+
+        return unless lease_uuid
+      end
+
+      task = task.to_sym
+
+      if gc?(task)
+        ::Projects::GitDeduplicationService.new(project).execute
+        cleanup_orphan_lfs_file_references(project)
+      end
+
+      gitaly_call(task, project)
+
+      # Refresh the branch cache in case garbage collection caused a ref lookup to fail
+      flush_ref_caches(project) if gc?(task)
+
+      update_repository_statistics(project) if task != :pack_refs
+
+      # In case pack files are deleted, release libgit2 cache and open file
+      # descriptors ASAP instead of waiting for Ruby garbage collection
+      project.cleanup
+    ensure
+      cancel_lease(lease_key, lease_uuid) if lease_key.present? && lease_uuid.present?
+    end
+
+    private
+
+    def find_project(project_id)
+      Project.find(project_id)
+    end
+
+    def gc?(task)
+      task == :gc || task == :prune
+    end
+
+    def try_obtain_lease(key)
+      ::Gitlab::ExclusiveLease.new(key, timeout: LEASE_TIMEOUT).try_obtain
+    end
+
+    def renew_lease(key, uuid)
+      ::Gitlab::ExclusiveLease.new(key, uuid: uuid, timeout: LEASE_TIMEOUT).renew
+    end
+
+    def cancel_lease(key, uuid)
+      ::Gitlab::ExclusiveLease.cancel(key, uuid)
+    end
+
+    def get_lease_uuid(key)
+      ::Gitlab::ExclusiveLease.get_uuid(key)
+    end
+
+    def gitaly_call(task, project)
+      repository = project.repository.raw_repository
+      client = get_gitaly_client(task, repository)
+
+      case task
+      when :prune, :gc
+        client.garbage_collect(bitmaps_enabled?, prune: task == :prune)
+      when :full_repack
+        client.repack_full(bitmaps_enabled?)
+      when :incremental_repack
+        client.repack_incremental
+      when :pack_refs
+        client.pack_refs
+      end
+    rescue GRPC::NotFound => e
+      Gitlab::GitLogger.error("#{__method__} failed:\nRepository not found")
+      raise Gitlab::Git::Repository::NoRepository.new(e)
+    rescue GRPC::BadStatus => e
+      Gitlab::GitLogger.error("#{__method__} failed:\n#{e}")
+      raise Gitlab::Git::CommandError.new(e)
+    end
+
+    def get_gitaly_client(task, repository)
+      if task == :pack_refs
+        Gitlab::GitalyClient::RefService
+      else
+        Gitlab::GitalyClient::RepositoryService
+      end.new(repository)
+    end
+
+    def cleanup_orphan_lfs_file_references(project)
+      return if Gitlab::Database.read_only? # GitGarbageCollectWorker may be run on a Geo secondary
+
+      ::Gitlab::Cleanup::OrphanLfsFileReferences.new(project, dry_run: false, logger: logger).run!
+    rescue => err
+      Gitlab::GitLogger.warn(message: "Cleaning up orphan LFS objects files failed", error: err.message)
+      Gitlab::ErrorTracking.track_and_raise_for_dev_exception(err)
+    end
+
+    def flush_ref_caches(project)
+      project.repository.expire_branches_cache
+      project.repository.branch_names
+      project.repository.has_visible_content?
+    end
+
+    def update_repository_statistics(project)
+      project.repository.expire_statistics_caches
+      return if Gitlab::Database.read_only? # GitGarbageCollectWorker may be run on a Geo secondary
+
+      Projects::UpdateStatisticsService.new(project, nil, statistics: [:repository_size, :lfs_objects_size]).execute
+    end
+
+    def bitmaps_enabled?
+      Gitlab::CurrentSettings.housekeeping_bitmaps_enabled
+    end
+  end
+end

config/sidekiq_queues.yml

@@ -270,6 +270,8 @@
   - 1
 - - project_update_repository_storage
   - 1
+- - projects_git_garbage_collect
+  - 1
 - - prometheus_create_default_alerts
   - 1
 - - propagate_integration

doc/administration/housekeeping.md

@@ -49,7 +49,7 @@ It is ultimately performed by the Gitaly RPC `FetchIntoObjectPool`.
 This is the current call stack by which it is invoked:
 
 1. `Repositories::HousekeepingService#execute_gitlab_shell_gc`
-1. `GitGarbageCollectWorker#perform`
+1. `Projects::GitGarbageCollectWorker#perform`
 1. `Projects::GitDeduplicationService#fetch_from_source`
 1. `ObjectPool#fetch`
 1. `ObjectPoolService#fetch`

ee/app/services/geo/project_housekeeping_service.rb

@@ -52,7 +52,7 @@ module Geo
     end
 
     def execute_gitlab_shell_gc(lease_uuid)
-      GitGarbageCollectWorker.perform_async(project.id, task, lease_key, lease_uuid)
+      ::Projects::GitGarbageCollectWorker.perform_async(project.id, task, lease_key, lease_uuid)
     ensure
       if should_reset?
         Gitlab::Metrics.measure(:geo_reset_syncs_since_gc) do

ee/spec/services/geo/project_housekeeping_service_spec.rb

@@ -52,13 +52,13 @@ RSpec.describe Geo::ProjectHousekeepingService do
         stub_exclusive_lease(:the_lease_key, :the_uuid)
 
         # At fetch 200
-        expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid)
+        expect(::Projects::GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid)
           .once
         # At fetch 50, 100, 150
-        expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :full_repack, :the_lease_key, :the_uuid)
+        expect(::Projects::GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :full_repack, :the_lease_key, :the_uuid)
           .exactly(3).times
         # At fetch 10, 20, ... (except those above)
-        expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :incremental_repack, :the_lease_key, :the_uuid)
+        expect(::Projects::GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :incremental_repack, :the_lease_key, :the_uuid)
           .exactly(16).times
 
         201.times do
@@ -76,7 +76,7 @@ RSpec.describe Geo::ProjectHousekeepingService do
         allow(service).to receive(:lease_key).and_return(:the_lease_key)
         stub_exclusive_lease(:the_lease_key, :the_uuid)
 
-        expect(GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid).once
+        expect(::Projects::GitGarbageCollectWorker).to receive(:perform_async).with(project.id, :gc, :the_lease_key, :the_uuid).once
 
         service.execute
       end
@@ -88,7 +88,7 @@ RSpec.describe Geo::ProjectHousekeepingService do
       it 'does not run gc for a non-new repository' do
         stub_exclusive_lease(:the_lease_key, :the_uuid)
 
-        expect(GitGarbageCollectWorker).not_to receive(:perform_async)
+        expect(::Projects::GitGarbageCollectWorker).not_to receive(:perform_async)
 
         service.execute
       end
@@ -102,7 +102,7 @@ RSpec.describe Geo::ProjectHousekeepingService do
       end
 
       it 'does not enqueue a job' do
-        expect(GitGarbageCollectWorker).not_to receive(:perform_async)
+        expect(::Projects::GitGarbageCollectWorker).not_to receive(:perform_async)
 
         expect(service.send(:do_housekeeping)).to be_falsey
       end
@@ -119,10 +119,10 @@ RSpec.describe Geo::ProjectHousekeepingService 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
+      expect(::Projects::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)
+        expect { service.send(:do_housekeeping) }.to change(::Projects::GitGarbageCollectWorker.jobs, :size).by(1)
       end
     end
 

spec/services/projects/cleanup_service_spec.rb

@@ -88,7 +88,7 @@ RSpec.describe Projects::CleanupService do
     end
 
     it 'runs garbage collection on the repository' do
-      expect_next_instance_of(GitGarbageCollectWorker) do |worker|
+      expect_next_instance_of(Projects::GitGarbageCollectWorker) do |worker|
         expect(worker).to receive(:perform).with(project.id, :prune, "project_cleanup:gc:#{project.id}")
       end
 

spec/support/shared_examples/services/projects/update_repository_storage_service_shared_examples.rb

@@ -71,7 +71,7 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
 
       it 'does not enqueue a GC run' do
         expect { subject.execute }
-          .not_to change(GitGarbageCollectWorker.jobs, :count)
+          .not_to change(Projects::GitGarbageCollectWorker.jobs, :count)
       end
     end
 
@@ -84,12 +84,12 @@ RSpec.shared_examples 'moves repository to another storage' do |repository_type|
         stub_application_setting(housekeeping_enabled: false)
 
         expect { subject.execute }
-          .not_to change(GitGarbageCollectWorker.jobs, :count)
+          .not_to change(Projects::GitGarbageCollectWorker.jobs, :count)
       end
 
       it 'enqueues a GC run' do
         expect { subject.execute }
-          .to change(GitGarbageCollectWorker.jobs, :count).by(1)
+          .to change(Projects::GitGarbageCollectWorker.jobs, :count).by(1)
       end
     end
   end

spec/support/shared_examples/services/repositories/housekeeping_shared_examples.rb

@@ -9,10 +9,10 @@ RSpec.shared_examples 'housekeeps repository' 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(resource.id, :incremental_repack, :the_lease_key, :the_uuid).and_call_original
+        expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :incremental_repack, :the_lease_key, :the_uuid).and_call_original
 
         Sidekiq::Testing.fake! do
-          expect { subject.execute }.to change(GitGarbageCollectWorker.jobs, :size).by(1)
+          expect { subject.execute }.to change(Projects::GitGarbageCollectWorker.jobs, :size).by(1)
         end
       end
 
@@ -38,7 +38,7 @@ RSpec.shared_examples 'housekeeps repository' do
         end
 
         it 'does not enqueue a job' do
-          expect(GitGarbageCollectWorker).not_to receive(:perform_async)
+          expect(Projects::GitGarbageCollectWorker).not_to receive(:perform_async)
 
           expect { subject.execute }.to raise_error(Repositories::HousekeepingService::LeaseTaken)
         end
@@ -63,16 +63,16 @@ RSpec.shared_examples 'housekeeps repository' do
           allow(subject).to receive(:lease_key).and_return(:the_lease_key)
 
           # At push 200
-          expect(GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :gc, :the_lease_key, :the_uuid)
+          expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :gc, :the_lease_key, :the_uuid)
             .once
           # At push 50, 100, 150
-          expect(GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :full_repack, :the_lease_key, :the_uuid)
+          expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :full_repack, :the_lease_key, :the_uuid)
             .exactly(3).times
           # At push 10, 20, ... (except those above)
-          expect(GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :incremental_repack, :the_lease_key, :the_uuid)
+          expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :incremental_repack, :the_lease_key, :the_uuid)
             .exactly(16).times
           # At push 6, 12, 18, ... (except those above)
-          expect(GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :pack_refs, :the_lease_key, :the_uuid)
+          expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :pack_refs, :the_lease_key, :the_uuid)
             .exactly(27).times
 
           201.times do
@@ -90,7 +90,7 @@ RSpec.shared_examples 'housekeeps repository' do
         allow(housekeeping).to receive(:try_obtain_lease).and_return(:gc_uuid)
         allow(housekeeping).to receive(:lease_key).and_return(:gc_lease_key)
 
-        expect(GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :gc, :gc_lease_key, :gc_uuid).twice
+        expect(Projects::GitGarbageCollectWorker).to receive(:perform_async).with(resource.id, :gc, :gc_lease_key, :gc_uuid).twice
 
         2.times do
           housekeeping.execute