Merge branch 'ci-pipeline-artifacts-removal-cleanup' into 'master'

Remove feature flags from artifacts destroy services

See merge request gitlab-org/gitlab!51323

app/services/ci/destroy_expired_job_artifacts_service.rb

@@ -12,6 +12,10 @@ module Ci
     EXCLUSIVE_LOCK_KEY = 'expired_job_artifacts:destroy:lock'
     LOCK_TIMEOUT = 6.minutes
 
+    def initialize
+      @removed_artifacts_count = 0
+    end
+
     ##
     # Destroy expired job artifacts on GitLab instance
     #
@@ -20,47 +24,13 @@ module Ci
     # which is scheduled every 7 minutes.
     def execute
       in_lock(EXCLUSIVE_LOCK_KEY, ttl: LOCK_TIMEOUT, retries: 1) do
-        if ::Feature.enabled?(:ci_slow_artifacts_removal)
-          destroy_job_and_pipeline_artifacts
-        else
-          loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do
-            destroy_artifacts_batch
-          end
-        end
+        destroy_job_artifacts_with_slow_iteration(Time.current)
       end
-    end
-
-    private
-
-    def destroy_job_and_pipeline_artifacts
-      start_at = Time.current
-      destroy_job_artifacts_with_slow_iteration(start_at)
 
-      timeout = LOOP_TIMEOUT - (Time.current - start_at)
-      return false if timeout < 0
-
-      loop_until(timeout: timeout, limit: LOOP_LIMIT) do
-        destroy_pipeline_artifacts_batch
-      end
+      @removed_artifacts_count
     end
 
-    def destroy_artifacts_batch
-      destroy_job_artifacts_batch || destroy_pipeline_artifacts_batch
-    end
-
-    def destroy_job_artifacts_batch
-      artifacts = Ci::JobArtifact
-        .expired(BATCH_SIZE)
-        .unlocked
-        .order_expired_desc
-        .with_destroy_preloads
-        .to_a
-
-      return false if artifacts.empty?
-
-      parallel_destroy_batch(artifacts)
-      true
-    end
+    private
 
     def destroy_job_artifacts_with_slow_iteration(start_at)
       Ci::JobArtifact.expired_before(start_at).each_batch(of: BATCH_SIZE, column: :expire_at, order: :desc) do |relation, index|
@@ -72,19 +42,6 @@ module Ci
       end
     end
 
-    # TODO: Make sure this can also be parallelized
-    # https://gitlab.com/gitlab-org/gitlab/-/issues/270973
-    def destroy_pipeline_artifacts_batch
-      return false if ::Feature.enabled?(:ci_split_pipeline_artifacts_removal)
-
-      artifacts = Ci::PipelineArtifact.expired(BATCH_SIZE).to_a
-      return false if artifacts.empty?
-
-      artifacts.each(&:destroy!)
-
-      true
-    end
-
     def parallel_destroy_batch(job_artifacts)
       Ci::DeletedObject.transaction do
         Ci::DeletedObject.bulk_import(job_artifacts)
@@ -93,14 +50,14 @@ module Ci
       end
 
       # This is executed outside of the transaction because it depends on Redis
-      update_statistics_for(job_artifacts)
-      destroyed_artifacts_counter.increment({}, job_artifacts.size)
+      update_project_statistics_for(job_artifacts)
+      increment_monitoring_statistics(job_artifacts.size)
     end
 
     # This method is implemented in EE and it must do only database work
     def destroy_related_records_for(job_artifacts); end
 
-    def update_statistics_for(job_artifacts)
+    def update_project_statistics_for(job_artifacts)
       artifacts_by_project = job_artifacts.group_by(&:project)
       artifacts_by_project.each do |project, artifacts|
         delta = -artifacts.sum { |artifact| artifact.size.to_i }
@@ -109,6 +66,11 @@ module Ci
       end
     end
 
+    def increment_monitoring_statistics(size)
+      destroyed_artifacts_counter.increment({}, size)
+      @removed_artifacts_count += size
+    end
+
     def destroyed_artifacts_counter
       strong_memoize(:destroyed_artifacts_counter) do
         name = :destroyed_job_artifacts_count_total

app/workers/ci/pipeline_artifacts/expire_artifacts_worker.rb

@@ -14,8 +14,6 @@ module Ci
       feature_category :continuous_integration
 
       def perform
-        return unless ::Feature.enabled?(:ci_split_pipeline_artifacts_removal)
-
         service = ::Ci::PipelineArtifacts::DestroyExpiredArtifactsService.new
         artifacts_count = service.execute
         log_extra_metadata_on_done(:destroyed_pipeline_artifacts_count, artifacts_count)

app/workers/expire_build_artifacts_worker.rb

@@ -10,6 +10,8 @@ class ExpireBuildArtifactsWorker # rubocop:disable Scalability/IdempotentWorker
   feature_category :continuous_integration
 
   def perform
-    Ci::DestroyExpiredJobArtifactsService.new.execute
+    service = Ci::DestroyExpiredJobArtifactsService.new
+    artifacts_count = service.execute
+    log_extra_metadata_on_done(:destroyed_job_artifacts_count, artifacts_count)
   end
 end

changelogs/unreleased/ci-pipeline-artifacts-removal-cleanup.yml

+---
+title: Extract expired pipeline artifacts removal service into it's own background
+  worker
+merge_request: 51323
+author:
+type: changed

config/feature_flags/development/ci_slow_artifacts_removal.yml

----
-name: ci_slow_artifacts_removal
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/47496
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/281688
-milestone: '13.8'
-type: development
-group: 'group::continuous integration'
-default_enabled: false

config/feature_flags/development/ci_split_pipeline_artifacts_removal.yml

----
-name: ci_split_pipeline_artifacts_removal
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/50446
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/295300
-milestone: '13.8'
-type: development
-group: group::continuous integration
-default_enabled: false

spec/services/ci/destroy_expired_job_artifacts_service_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
   describe '.execute' do
     subject { service.execute }
 
-    let_it_be(:artifact, reload: true) do
+    let_it_be(:artifact, refind: true) do
       create(:ci_job_artifact, expire_at: 1.day.ago)
     end
 
@@ -164,13 +164,21 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
     end
 
     context 'when timeout happens' do
+      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+
       before do
-        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 1.second)
-        allow_any_instance_of(described_class).to receive(:destroy_pipeline_artifacts_batch) { true }
+        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 0.seconds)
+        stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+
+        second_artifact.job.pipeline.unlocked!
       end
 
-      it 'returns false and does not continue destroying' do
-        is_expected.to be_falsy
+      it 'destroys one artifact' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+      end
+
+      it 'reports the number of destroyed artifacts' do
+        is_expected.to eq(1)
       end
     end
 
@@ -187,6 +195,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       it 'destroys one artifact' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
       end
+
+      it 'reports the number of destroyed artifacts' do
+        is_expected.to eq(1)
+      end
     end
 
     context 'when there are no artifacts' do
@@ -197,6 +209,10 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       it 'does not raise error' do
         expect { subject }.not_to raise_error
       end
+
+      it 'reports the number of destroyed artifacts' do
+        is_expected.to eq(0)
+      end
     end
 
     context 'when there are artifacts more than batch sizes' do
@@ -211,45 +227,9 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       it 'destroys all expired artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(-2)
       end
-    end
-
-    context 'when artifact is a pipeline artifact' do
-      context 'when artifacts are expired' do
-        let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) }
-        let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 1.week.ago) }
 
-        before do
-          [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! }
-
-          stub_feature_flags(ci_split_pipeline_artifacts_removal: false)
-        end
-
-        it 'destroys pipeline artifacts' do
-          expect { subject }.to change { Ci::PipelineArtifact.count }.by(-2)
-        end
-
-        context 'with ci_split_pipeline_artifacts_removal enabled' do
-          before do
-            stub_feature_flags(ci_split_pipeline_artifacts_removal: true)
-          end
-
-          it 'does not destroy pipeline artifacts' do
-            expect { subject }.not_to change { Ci::PipelineArtifact.count }
-          end
-        end
-      end
-
-      context 'when artifacts are not expired' do
-        let!(:pipeline_artifact_1) { create(:ci_pipeline_artifact, expire_at: 2.days.from_now) }
-        let!(:pipeline_artifact_2) { create(:ci_pipeline_artifact, expire_at: 2.days.from_now) }
-
-        before do
-          [pipeline_artifact_1, pipeline_artifact_2].each { |pipeline_artifact| pipeline_artifact.pipeline.unlocked! }
-        end
-
-        it 'does not destroy pipeline artifacts' do
-          expect { subject }.not_to change { Ci::PipelineArtifact.count }
-        end
+      it 'reports the number of destroyed artifacts' do
+        is_expected.to eq(2)
       end
     end
 
@@ -265,16 +245,4 @@ RSpec.describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared
       end
     end
   end
-
-  describe '.destroy_job_artifacts_batch' do
-    it 'returns a falsy value without artifacts' do
-      expect(service.send(:destroy_job_artifacts_batch)).to be_falsy
-    end
-  end
-
-  describe '.destroy_pipeline_artifacts_batch' do
-    it 'returns a falsy value without artifacts' do
-      expect(service.send(:destroy_pipeline_artifacts_batch)).to be_falsy
-    end
-  end
 end

spec/workers/expire_build_artifacts_worker_spec.rb

@@ -8,9 +8,11 @@ RSpec.describe ExpireBuildArtifactsWorker do
   describe '#perform' do
     it 'executes a service' do
       expect_next_instance_of(Ci::DestroyExpiredJobArtifactsService) do |instance|
-        expect(instance).to receive(:execute)
+        expect(instance).to receive(:execute).and_call_original
       end
 
+      expect(worker).to receive(:log_extra_metadata_on_done).with(:destroyed_job_artifacts_count, 0)
+
       worker.perform
     end
   end