Merge branch '267546-limited-worker-nonloopless-inner-class' into 'master'

Add an inner class in the cleanup container repository worker [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!56989

app/workers/container_expiration_policies/cleanup_container_repository_worker.rb

@@ -21,82 +21,34 @@ module ContainerExpirationPolicies
       cleanup_tags_service_deleted_size
     ].freeze
 
-    def perform_work
-      return unless throttling_enabled?
-      return unless container_repository
-
-      log_extra_metadata_on_done(:container_repository_id, container_repository.id)
-      log_extra_metadata_on_done(:project_id, project.id)
-
-      unless allowed_to_run?(container_repository)
-        container_repository.cleanup_unscheduled!
-        log_extra_metadata_on_done(:cleanup_status, :skipped)
-        return
+    delegate :perform_work, :remaining_work_count, to: :inner_instance
+
+    def inner_instance
+      strong_memoize(:inner_instance) do
+        if loopless_enabled?
+          Loopless.new(self)
+        else
+          Looping.new(self)
+        end
       end
-
-      result = ContainerExpirationPolicies::CleanupService.new(container_repository)
-                                                          .execute
-      log_on_done(result)
-    end
-
-    def remaining_work_count
-      cleanup_scheduled_count = ContainerRepository.cleanup_scheduled.count
-      cleanup_unfinished_count = ContainerRepository.cleanup_unfinished.count
-      total_count = cleanup_scheduled_count + cleanup_unfinished_count
-
-      log_info(
-        cleanup_scheduled_count: cleanup_scheduled_count,
-        cleanup_unfinished_count: cleanup_unfinished_count,
-        cleanup_total_count: total_count
-      )
-
-      total_count
     end
 
     def max_running_jobs
       return 0 unless throttling_enabled?
 
-      ::Gitlab::CurrentSettings.current_application_settings.container_registry_expiration_policies_worker_capacity
-    end
-
-    private
-
-    def allowed_to_run?(container_repository)
-      return false unless policy&.enabled && policy&.next_run_at
-
-      Time.zone.now + max_cleanup_execution_time.seconds < policy.next_run_at
+      ::Gitlab::CurrentSettings.container_registry_expiration_policies_worker_capacity
     end
 
     def throttling_enabled?
       Feature.enabled?(:container_registry_expiration_policies_throttling)
     end
 
-    def max_cleanup_execution_time
-      ::Gitlab::CurrentSettings.current_application_settings.container_registry_delete_tags_service_timeout
-    end
-
-    def policy
-      project.container_expiration_policy
+    def loopless_enabled?
+      Feature.enabled?(:container_registry_expiration_policies_loopless)
     end
 
-    def project
-      container_repository.project
-    end
-
-    def container_repository
-      strong_memoize(:container_repository) do
-        ContainerRepository.transaction do
-          # rubocop: disable CodeReuse/ActiveRecord
-          # We need a lock to prevent two workers from picking up the same row
-          container_repository = ContainerRepository.waiting_for_cleanup
-                                                    .order(:expiration_policy_cleanup_status, :expiration_policy_started_at)
-                                                    .limit(1)
-                                                    .lock('FOR UPDATE SKIP LOCKED')
-                                                    .first
-          # rubocop: enable CodeReuse/ActiveRecord
-          container_repository&.tap(&:cleanup_ongoing!)
-        end
-      end
+    def max_cleanup_execution_time
+      ::Gitlab::CurrentSettings.container_registry_delete_tags_service_timeout
     end
 
     def log_info(extra_structure)
@@ -120,5 +72,100 @@ module ContainerExpirationPolicies
       log_extra_metadata_on_done(:cleanup_tags_service_truncated, !!truncated)
       log_extra_metadata_on_done(:running_jobs_count, running_jobs_count)
     end
+
+    # rubocop: disable Scalability/IdempotentWorker
+    # TODO: move the logic from this class to the parent one when container_registry_expiration_policies_loopless is removed
+    # Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/325273
+    class Loopless
+      # TODO fill the logic here with the approach documented in
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/267546#limited-worker
+      def initialize(parent)
+        @parent = parent
+      end
+    end
+    # rubocop: enable Scalability/IdempotentWorker
+
+    # rubocop: disable Scalability/IdempotentWorker
+    # TODO remove this class when `container_registry_expiration_policies_loopless` is removed
+    # Tracking issue: https://gitlab.com/gitlab-org/gitlab/-/issues/325273
+    class Looping
+      include Gitlab::Utils::StrongMemoize
+
+      delegate :throttling_enabled?,
+               :log_extra_metadata_on_done,
+               :log_info,
+               :log_on_done,
+               :max_cleanup_execution_time,
+               to: :@parent
+
+      def initialize(parent)
+        @parent = parent
+      end
+
+      def perform_work
+        return unless throttling_enabled?
+        return unless container_repository
+
+        log_extra_metadata_on_done(:container_repository_id, container_repository.id)
+        log_extra_metadata_on_done(:project_id, project.id)
+
+        unless allowed_to_run?(container_repository)
+          container_repository.cleanup_unscheduled!
+          log_extra_metadata_on_done(:cleanup_status, :skipped)
+          return
+        end
+
+        result = ContainerExpirationPolicies::CleanupService.new(container_repository)
+                                                            .execute
+        log_on_done(result)
+      end
+
+      def remaining_work_count
+        cleanup_scheduled_count = ContainerRepository.cleanup_scheduled.count
+        cleanup_unfinished_count = ContainerRepository.cleanup_unfinished.count
+        total_count = cleanup_scheduled_count + cleanup_unfinished_count
+
+        log_info(
+          cleanup_scheduled_count: cleanup_scheduled_count,
+          cleanup_unfinished_count: cleanup_unfinished_count,
+          cleanup_total_count: total_count
+        )
+
+        total_count
+      end
+
+      private
+
+      def allowed_to_run?(container_repository)
+        return false unless policy&.enabled && policy&.next_run_at
+
+        Time.zone.now + max_cleanup_execution_time.seconds < policy.next_run_at
+      end
+
+      def policy
+        project.container_expiration_policy
+      end
+
+      def project
+        container_repository.project
+      end
+
+      def container_repository
+        strong_memoize(:container_repository) do
+          ContainerRepository.transaction do
+            # rubocop: disable CodeReuse/ActiveRecord
+            # We need a lock to prevent two workers from picking up the same row
+            container_repository = ContainerRepository.waiting_for_cleanup
+                                                      .order(:expiration_policy_cleanup_status, :expiration_policy_started_at)
+                                                      .limit(1)
+                                                      .lock('FOR UPDATE SKIP LOCKED')
+                                                      .first
+            # rubocop: enable CodeReuse/ActiveRecord
+            container_repository&.tap(&:cleanup_ongoing!)
+          end
+        end
+      end
+    end
+    # rubocop: enable Scalability/IdempotentWorker
   end
 end

spec/workers/container_expiration_policies/cleanup_container_repository_worker_spec.rb

@@ -106,96 +106,102 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       end
     end
 
-    context 'with repository in cleanup scheduled state' do
-      it_behaves_like 'handling all repository conditions'
-    end
-
-    context 'with repository in cleanup unfinished state' do
+    context 'with loopless disabled' do
       before do
-        repository.cleanup_unfinished!
+        stub_feature_flags(container_registry_expiration_policies_loopless: false)
       end
 
-      it_behaves_like 'handling all repository conditions'
-    end
-
-    context 'with another repository in cleanup unfinished state' do
-      let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) }
+      context 'with repository in cleanup scheduled state' do
+        it_behaves_like 'handling all repository conditions'
+      end
 
-      it 'process the cleanup scheduled repository first' do
-        service_response = cleanup_service_response(repository: repository)
-        expect(ContainerExpirationPolicies::CleanupService)
-          .to receive(:new).with(repository).and_return(double(execute: service_response))
-        expect_log_extra_metadata(service_response: service_response)
+      context 'with repository in cleanup unfinished state' do
+        before do
+          repository.cleanup_unfinished!
+        end
 
-        subject
+        it_behaves_like 'handling all repository conditions'
       end
-    end
 
-    context 'with multiple repositories in cleanup unfinished state' do
-      let_it_be(:repository2) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 20.minutes.ago) }
-      let_it_be(:repository3) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 10.minutes.ago) }
+      context 'with another repository in cleanup unfinished state' do
+        let_it_be(:another_repository) { create(:container_repository, :cleanup_unfinished) }
 
-      before do
-        repository.update!(expiration_policy_cleanup_status: :cleanup_unfinished, expiration_policy_started_at: 30.minutes.ago)
+        it 'process the cleanup scheduled repository first' do
+          service_response = cleanup_service_response(repository: repository)
+          expect(ContainerExpirationPolicies::CleanupService)
+            .to receive(:new).with(repository).and_return(double(execute: service_response))
+          expect_log_extra_metadata(service_response: service_response)
+
+          subject
+        end
       end
 
-      it 'process the repository with the oldest expiration_policy_started_at' do
-        service_response = cleanup_service_response(repository: repository)
-        expect(ContainerExpirationPolicies::CleanupService)
-          .to receive(:new).with(repository).and_return(double(execute: service_response))
-        expect_log_extra_metadata(service_response: service_response)
+      context 'with multiple repositories in cleanup unfinished state' do
+        let_it_be(:repository2) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 20.minutes.ago) }
+        let_it_be(:repository3) { create(:container_repository, :cleanup_unfinished, expiration_policy_started_at: 10.minutes.ago) }
 
-        subject
-      end
-    end
+        before do
+          repository.update!(expiration_policy_cleanup_status: :cleanup_unfinished, expiration_policy_started_at: 30.minutes.ago)
+        end
 
-    context 'with repository in cleanup ongoing state' do
-      before do
-        repository.cleanup_ongoing!
+        it 'process the repository with the oldest expiration_policy_started_at' do
+          service_response = cleanup_service_response(repository: repository)
+          expect(ContainerExpirationPolicies::CleanupService)
+            .to receive(:new).with(repository).and_return(double(execute: service_response))
+          expect_log_extra_metadata(service_response: service_response)
+
+          subject
+        end
       end
 
-      it 'does not process it' do
-        expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
+      context 'with repository in cleanup ongoing state' do
+        before do
+          repository.cleanup_ongoing!
+        end
 
-        expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
-        expect(repository.cleanup_ongoing?).to be_truthy
-      end
-    end
+        it 'does not process it' do
+          expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
 
-    context 'with no repository in any cleanup state' do
-      before do
-        repository.cleanup_unscheduled!
+          expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
+          expect(repository.cleanup_ongoing?).to be_truthy
+        end
       end
 
-      it 'does not process it' do
-        expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
+      context 'with no repository in any cleanup state' do
+        before do
+          repository.cleanup_unscheduled!
+        end
 
-        expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
-        expect(repository.cleanup_unscheduled?).to be_truthy
-      end
-    end
+        it 'does not process it' do
+          expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
 
-    context 'with no container repository waiting' do
-      before do
-        repository.destroy!
+          expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
+          expect(repository.cleanup_unscheduled?).to be_truthy
+        end
       end
 
-      it 'does not execute the cleanup tags service' do
-        expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
+      context 'with no container repository waiting' do
+        before do
+          repository.destroy!
+        end
 
-        expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
-      end
-    end
+        it 'does not execute the cleanup tags service' do
+          expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
 
-    context 'with feature flag disabled' do
-      before do
-        stub_feature_flags(container_registry_expiration_policies_throttling: false)
+          expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
+        end
       end
 
-      it 'is a no-op' do
-        expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
+      context 'with feature flag disabled' do
+        before do
+          stub_feature_flags(container_registry_expiration_policies_throttling: false)
+        end
+
+        it 'is a no-op' do
+          expect(Projects::ContainerRepository::CleanupTagsService).not_to receive(:new)
 
-        expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
+          expect { subject }.not_to change { ContainerRepository.waiting_for_cleanup.count }
+        end
       end
     end
 
@@ -230,37 +236,42 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
   describe '#remaining_work_count' do
     subject { worker.remaining_work_count }
 
-    context 'with container repositoires waiting for cleanup' do
-      let_it_be(:unfinished_repositories) { create_list(:container_repository, 2, :cleanup_unfinished) }
+    context 'with loopless disabled' do
+      before do
+        stub_feature_flags(container_registry_expiration_policies_loopless: false)
+      end
+      context 'with container repositoires waiting for cleanup' do
+        let_it_be(:unfinished_repositories) { create_list(:container_repository, 2, :cleanup_unfinished) }
 
-      it { is_expected.to eq(3) }
+        it { is_expected.to eq(3) }
 
-      it 'logs the work count' do
-        expect_log_info(
-          cleanup_scheduled_count: 1,
-          cleanup_unfinished_count: 2,
-          cleanup_total_count: 3
-        )
+        it 'logs the work count' do
+          expect_log_info(
+            cleanup_scheduled_count: 1,
+            cleanup_unfinished_count: 2,
+            cleanup_total_count: 3
+          )
 
-        subject
+          subject
+        end
       end
-    end
 
-    context 'with no container repositories waiting for cleanup' do
-      before do
-        repository.cleanup_ongoing!
-      end
+      context 'with no container repositories waiting for cleanup' do
+        before do
+          repository.cleanup_ongoing!
+        end
 
-      it { is_expected.to eq(0) }
+        it { is_expected.to eq(0) }
 
-      it 'logs 0 work count' do
-        expect_log_info(
-          cleanup_scheduled_count: 0,
-          cleanup_unfinished_count: 0,
-          cleanup_total_count: 0
-        )
+        it 'logs 0 work count' do
+          expect_log_info(
+            cleanup_scheduled_count: 0,
+            cleanup_unfinished_count: 0,
+            cleanup_total_count: 0
+          )
 
-        subject
+          subject
+        end
       end
     end
   end
@@ -274,14 +285,20 @@ RSpec.describe ContainerExpirationPolicies::CleanupContainerRepositoryWorker do
       stub_application_setting(container_registry_expiration_policies_worker_capacity: capacity)
     end
 
-    it { is_expected.to eq(capacity) }
-
-    context 'with feature flag disabled' do
+    context 'with loopless disabled' do
       before do
-        stub_feature_flags(container_registry_expiration_policies_throttling: false)
+        stub_feature_flags(container_registry_expiration_policies_loopless: false)
       end
 
-      it { is_expected.to eq(0) }
+      it { is_expected.to eq(capacity) }
+
+      context 'with feature flag disabled' do
+        before do
+          stub_feature_flags(container_registry_expiration_policies_throttling: false)
+        end
+
+        it { is_expected.to eq(0) }
+      end
     end
   end
 

spec/workers/container_expiration_policy_worker_spec.rb

@@ -35,10 +35,16 @@ RSpec.describe ContainerExpirationPolicyWorker do
     end
 
     context 'With no container expiration policies' do
-      it 'does not execute any policies' do
-        expect(ContainerRepository).not_to receive(:for_project_id)
+      context 'with loopless disabled' do
+        before do
+          stub_feature_flags(container_registry_expiration_policies_loopless: false)
+        end
 
-        expect { subject }.not_to change { ContainerRepository.cleanup_scheduled.count }
+        it 'does not execute any policies' do
+          expect(ContainerRepository).not_to receive(:for_project_id)
+
+          expect { subject }.not_to change { ContainerRepository.cleanup_scheduled.count }
+        end
       end
     end