Merge branch '330322-unstuck-stuck-cleanup-policies' into 'master'

Properly process stale container repository cleanups

See merge request gitlab-org/gitlab!62005

app/models/container_repository.rb

@@ -33,6 +33,7 @@ class ContainerRepository < ApplicationRecord
   scope :search_by_name, ->(query) { fuzzy_search(query, [:name], use_minimum_char_limit: false) }
   scope :waiting_for_cleanup, -> { where(expiration_policy_cleanup_status: WAITING_CLEANUP_STATUSES) }
   scope :expiration_policy_started_at_nil_or_before, ->(timestamp) { where('expiration_policy_started_at < ? OR expiration_policy_started_at IS NULL', timestamp) }
+  scope :with_stale_ongoing_cleanup, ->(threshold) { cleanup_ongoing.where('expiration_policy_started_at < ?', threshold) }
 
   def self.exists_by_path?(path)
     where(

app/workers/container_expiration_policy_worker.rb

@@ -14,11 +14,18 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
   BATCH_SIZE = 1000
 
   def perform
+    process_stale_ongoing_cleanups
     throttling_enabled? ? perform_throttled : perform_unthrottled
   end
 
   private
 
+  def process_stale_ongoing_cleanups
+    threshold = delete_tags_service_timeout.seconds + 30.minutes
+    ContainerRepository.with_stale_ongoing_cleanup(threshold.ago)
+                       .update_all(expiration_policy_cleanup_status: :cleanup_unfinished)
+  end
+
   def perform_unthrottled
     with_runnable_policy(preloaded: true) do |policy|
       with_context(project: policy.project,
@@ -86,4 +93,8 @@ class ContainerExpirationPolicyWorker # rubocop:disable Scalability/IdempotentWo
   def lease_timeout
     5.hours
   end
+
+  def delete_tags_service_timeout
+    ::Gitlab::CurrentSettings.current_application_settings.container_registry_delete_tags_service_timeout || 0
+  end
 end

changelogs/unreleased/330322-unstuck-stuck-cleanup-policies.yml

+---
+title: Properly process stale ongoing container repository cleanups
+merge_request: 62005
+author:
+type: fixed

spec/models/container_repository_spec.rb

@@ -360,6 +360,17 @@ RSpec.describe ContainerRepository do
     it { is_expected.to contain_exactly(repository1, repository2, repository4) }
   end
 
+  describe '.with_stale_ongoing_cleanup' do
+    let_it_be(:repository1) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 1.day.ago) }
+    let_it_be(:repository2) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 25.minutes.ago) }
+    let_it_be(:repository3) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 1.week.ago) }
+    let_it_be(:repository4) { create(:container_repository, :cleanup_unscheduled, expiration_policy_started_at: 25.minutes.ago) }
+
+    subject { described_class.with_stale_ongoing_cleanup(27.minutes.ago) }
+
+    it { is_expected.to contain_exactly(repository1, repository3) }
+  end
+
   describe '.waiting_for_cleanup' do
     let_it_be(:repository_cleanup_scheduled) { create(:container_repository, :cleanup_scheduled) }
     let_it_be(:repository_cleanup_unfinished) { create(:container_repository, :cleanup_unfinished) }

spec/workers/container_expiration_policy_worker_spec.rb

@@ -193,5 +193,18 @@ RSpec.describe ContainerExpirationPolicyWorker do
         end
       end
     end
+
+    context 'process stale ongoing cleanups' do
+      let_it_be(:stuck_cleanup) { create(:container_repository, :cleanup_ongoing, expiration_policy_started_at: 1.day.ago) }
+      let_it_be(:container_repository) { create(:container_repository, :cleanup_scheduled) }
+      let_it_be(:container_repository) { create(:container_repository, :cleanup_unfinished) }
+
+      it 'set them as unfinished' do
+        expect { subject }
+          .to change { ContainerRepository.cleanup_ongoing.count }.from(1).to(0)
+          .and change { ContainerRepository.cleanup_unfinished.count }.from(1).to(2)
+        expect(stuck_cleanup.reload).to be_cleanup_unfinished
+      end
+    end
   end
 end