Merge branch 'expired-artifact-fast-removal' into 'master'

Restore optimized newly expired artifacts removal

See merge request gitlab-org/gitlab!76504

app/models/ci/job_artifact.rb

@@ -271,10 +271,6 @@ module Ci
       self.where(project: project).sum(:size)
     end
 
-    def self.distinct_job_ids
-      distinct.pluck(:job_id)
-    end
-
     ##
     # FastDestroyAll concerns
     # rubocop: disable CodeReuse/ServiceClass

app/services/ci/job_artifacts/destroy_all_expired_service.rb

@@ -8,13 +8,15 @@ module Ci
 
       BATCH_SIZE = 100
       LOOP_TIMEOUT = 5.minutes
-      LOOP_LIMIT = 1000
+      SMALL_LOOP_LIMIT = 10
+      LARGE_LOOP_LIMIT = 100
       EXCLUSIVE_LOCK_KEY = 'expired_job_artifacts:destroy:lock'
       LOCK_TIMEOUT = 6.minutes
 
       def initialize
         @removed_artifacts_count = 0
         @start_at = Time.current
+        @loop_limit = ::Feature.enabled?(:ci_artifact_fast_removal_large_loop_limit, default_enabled: :yaml) ? LARGE_LOOP_LIMIT : SMALL_LOOP_LIMIT
       end
 
       ##
@@ -24,6 +26,8 @@ module Ci
       # preventing multiple `ExpireBuildArtifactsWorker` CRON jobs run concurrently,
       # which is scheduled every 7 minutes.
       def execute
+        return 0 unless ::Feature.enabled?(:ci_destroy_all_expired_service, default_enabled: :yaml)
+
         in_lock(EXCLUSIVE_LOCK_KEY, ttl: LOCK_TIMEOUT, retries: 1) do
           if ::Feature.enabled?(:ci_destroy_unlocked_job_artifacts)
             destroy_unlocked_job_artifacts
@@ -38,34 +42,13 @@ module Ci
       private
 
       def destroy_unlocked_job_artifacts
-        loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do
+        loop_until(timeout: LOOP_TIMEOUT, limit: @loop_limit) do
           artifacts = Ci::JobArtifact.expired_before(@start_at).artifact_unlocked.limit(BATCH_SIZE)
           service_response = destroy_batch(artifacts)
           @removed_artifacts_count += service_response[:destroyed_artifacts_count]
-
-          update_locked_status_on_unknown_artifacts if service_response[:destroyed_artifacts_count] == 0
-
-          # Return a truthy value here to prevent exiting #loop_until
-          @removed_artifacts_count
         end
       end
 
-      def update_locked_status_on_unknown_artifacts
-        build_ids = Ci::JobArtifact.expired_before(@start_at).artifact_unknown.limit(BATCH_SIZE).distinct_job_ids
-
-        return unless build_ids.present?
-
-        locked_pipeline_build_ids   = ::Ci::Build.with_pipeline_locked_artifacts.id_in(build_ids).pluck_primary_key
-        unlocked_pipeline_build_ids = build_ids - locked_pipeline_build_ids
-
-        update_unknown_artifacts(locked_pipeline_build_ids,   Ci::JobArtifact.lockeds[:artifacts_locked])
-        update_unknown_artifacts(unlocked_pipeline_build_ids, Ci::JobArtifact.lockeds[:unlocked])
-      end
-
-      def update_unknown_artifacts(build_ids, locked_value)
-        Ci::JobArtifact.for_job_ids(build_ids).update_all(locked: locked_value) if build_ids.any?
-      end
-
       def destroy_job_artifacts_with_slow_iteration
         Ci::JobArtifact.expired_before(@start_at).each_batch(of: BATCH_SIZE, column: :expire_at, order: :desc) do |relation, index|
           # For performance reasons, join with ci_pipelines after the batch is queried.
@@ -76,7 +59,7 @@ module Ci
           @removed_artifacts_count += service_response[:destroyed_artifacts_count]
 
           break if loop_timeout?
-          break if index >= LOOP_LIMIT
+          break if index >= @loop_limit
         end
       end
 

app/workers/expire_build_artifacts_worker.rb

@@ -13,8 +13,8 @@ class ExpireBuildArtifactsWorker # rubocop:disable Scalability/IdempotentWorker
   feature_category :build_artifacts
 
   def perform
-    service = Ci::JobArtifacts::DestroyAllExpiredService.new
-    artifacts_count = service.execute
+    artifacts_count = Ci::JobArtifacts::DestroyAllExpiredService.new.execute
+
     log_extra_metadata_on_done(:destroyed_job_artifacts_count, artifacts_count)
   end
 end

config/feature_flags/development/ci_artifact_fast_removal_large_loop_limit.yml

+---
+name: ci_artifact_fast_removal_large_loop_limit
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76504
+rollout_issue_url:
+milestone: '14.6'
+type: development
+group: group::pipeline execution
+default_enabled: false

config/feature_flags/development/ci_destroy_all_expired_service.yml

+---
+name: ci_destroy_all_expired_service
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/76504
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/348786
+milestone: '14.6'
+type: development
+group: group::pipeline execution
+default_enabled: false

spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb

@@ -20,7 +20,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
 
       context 'with preloaded relationships' do
         before do
-          stub_const("#{described_class}::LOOP_LIMIT", 1)
+          stub_const("#{described_class}::LARGE_LOOP_LIMIT", 1)
         end
 
         context 'with ci_destroy_unlocked_job_artifacts feature flag disabled' do
@@ -53,46 +53,6 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
             log = ActiveRecord::QueryRecorder.new { subject }
             expect(log.count).to be_within(1).of(8)
           end
-
-          context 'with several locked-unknown artifact records' do
-            before do
-              stub_const("#{described_class}::LOOP_LIMIT", 10)
-              stub_const("#{described_class}::BATCH_SIZE", 2)
-            end
-
-            let!(:lockable_artifact_records) do
-              [
-                create(:ci_job_artifact, :metadata,  :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job),
-                create(:ci_job_artifact, :junit,     :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job),
-                create(:ci_job_artifact, :sast,      :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job),
-                create(:ci_job_artifact, :cobertura, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job),
-                create(:ci_job_artifact, :trace,     :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: locked_job)
-              ]
-            end
-
-            let!(:unlockable_artifact_records) do
-              [
-                create(:ci_job_artifact, :metadata,  :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job),
-                create(:ci_job_artifact, :junit,     :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job),
-                create(:ci_job_artifact, :sast,      :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job),
-                create(:ci_job_artifact, :cobertura, :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job),
-                create(:ci_job_artifact, :trace,     :expired, locked: ::Ci::JobArtifact.lockeds[:unknown], job: job),
-                artifact
-              ]
-            end
-
-            it 'updates the locked status of job artifacts from artifacts-locked pipelines' do
-              subject
-
-              expect(lockable_artifact_records).to be_all(&:persisted?)
-              expect(lockable_artifact_records).to be_all { |artifact| artifact.reload.artifact_artifacts_locked? }
-            end
-
-            it 'unlocks and then destroys job artifacts from artifacts-unlocked pipelines' do
-              expect { subject }.to change { Ci::JobArtifact.count }.by(-6)
-              expect(Ci::JobArtifact.where(id: unlockable_artifact_records.map(&:id))).to be_empty
-            end
-          end
         end
       end
 
@@ -159,7 +119,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
       let!(:artifact) { create(:ci_job_artifact, :expired, job: job, locked: job.pipeline.locked) }
 
       before do
-        stub_const("#{described_class}::LOOP_LIMIT", 10)
+        stub_const("#{described_class}::LARGE_LOOP_LIMIT", 10)
       end
 
       context 'when the import fails' do
@@ -229,7 +189,8 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
 
       context 'when loop reached loop limit' do
         before do
-          stub_const("#{described_class}::LOOP_LIMIT", 1)
+          stub_feature_flags(ci_artifact_fast_removal_large_loop_limit: false)
+          stub_const("#{described_class}::SMALL_LOOP_LIMIT", 1)
         end
 
         it 'destroys one artifact' do