Merge branch 'expire-job-artifacts-worker' into 'master'

Efficiently remove expired artifacts in `ExpireBuildArtifactsWorker`

Closes #41057

See merge request gitlab-org/gitlab-ce!24450

8 files changed, 245 insertions, 1 deletions

diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
index 11c88200c37..789bb293811 100644
--- a/app/models/ci/job_artifact.rb
+++ b/app/models/ci/job_artifact.rb
@@ -73,6 +73,8 @@ module Ci
       where(file_type: types)
     end
 
+    scope :expired, -> (limit) { where('expire_at < ?', Time.now).limit(limit) }
+
     delegate :filename, :exists?, :open, to: :file
 
     enum file_type: {
diff --git a/app/services/ci/destroy_expired_job_artifacts_service.rb b/app/services/ci/destroy_expired_job_artifacts_service.rb
new file mode 100644
index 00000000000..7d2f5d33fed
--- /dev/null
+++ b/app/services/ci/destroy_expired_job_artifacts_service.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+module Ci
+  class DestroyExpiredJobArtifactsService
+    include ::Gitlab::ExclusiveLeaseHelpers
+    include ::Gitlab::LoopHelpers
+
+    BATCH_SIZE = 100
+    LOOP_TIMEOUT = 45.minutes
+    LOOP_LIMIT = 1000
+    EXCLUSIVE_LOCK_KEY = 'expired_job_artifacts:destroy:lock'
+    LOCK_TIMEOUT = 50.minutes
+
+    ##
+    # Destroy expired job artifacts on GitLab instance
+    #
+    # This destroy process cannot run for more than 45 minutes. This is for
+    # preventing multiple `ExpireBuildArtifactsWorker` CRON jobs run concurrently,
+    # which is scheduled at every hour.
+    def execute
+      in_lock(EXCLUSIVE_LOCK_KEY, ttl: LOCK_TIMEOUT, retries: 1) do
+        loop_until(timeout: LOOP_TIMEOUT, limit: LOOP_LIMIT) do
+          destroy_batch
+        end
+      end
+    end
+
+    private
+
+    def destroy_batch
+      artifacts = Ci::JobArtifact.expired(BATCH_SIZE).to_a
+
+      return false if artifacts.empty?
+
+      artifacts.each(&:destroy!)
+    end
+  end
+end
diff --git a/app/workers/expire_build_artifacts_worker.rb b/app/workers/expire_build_artifacts_worker.rb
index dce812d1ae2..251e95c68d5 100644
--- a/app/workers/expire_build_artifacts_worker.rb
+++ b/app/workers/expire_build_artifacts_worker.rb
@@ -4,8 +4,20 @@ class ExpireBuildArtifactsWorker
   include ApplicationWorker
   include CronjobQueue
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def perform
+    if Feature.enabled?(:ci_new_expire_job_artifacts_service, default_enabled: true)
+      perform_efficient_artifacts_removal
+    else
+      perform_legacy_artifacts_removal
+    end
+  end
+
+  def perform_efficient_artifacts_removal
+    Ci::DestroyExpiredJobArtifactsService.new.execute
+  end
+
+  # rubocop: disable CodeReuse/ActiveRecord
+  def perform_legacy_artifacts_removal
     Rails.logger.info 'Scheduling removal of build artifacts'
 
     build_ids = Ci::Build.with_expired_artifacts.pluck(:id)
diff --git a/changelogs/unreleased/expire-job-artifacts-worker.yml b/changelogs/unreleased/expire-job-artifacts-worker.yml
new file mode 100644
index 00000000000..cda6e9ff497
--- /dev/null
+++ b/changelogs/unreleased/expire-job-artifacts-worker.yml
@@ -0,0 +1,5 @@
+---
+title: Efficiently remove expired artifacts in `ExpireBuildArtifactsWorker`
+merge_request: 24450
+author:
+type: performance
diff --git a/lib/gitlab/loop_helpers.rb b/lib/gitlab/loop_helpers.rb
new file mode 100644
index 00000000000..3873156a3b0
--- /dev/null
+++ b/lib/gitlab/loop_helpers.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module LoopHelpers
+    ##
+    # This helper method repeats the same task until it's expired.
+    #
+    # Note: ExpiredLoopError does not happen until the given block finished.
+    #       Please do not use this method for heavy or asynchronous operations.
+    def loop_until(timeout: nil, limit: 1_000_000)
+      raise ArgumentError unless limit
+
+      start = Time.now
+
+      limit.times do
+        return true unless yield
+
+        return false if timeout && (Time.now - start) > timeout
+      end
+
+      false
+    end
+  end
+end
diff --git a/spec/lib/gitlab/loop_helpers_spec.rb b/spec/lib/gitlab/loop_helpers_spec.rb
new file mode 100644
index 00000000000..e17a0342d64
--- /dev/null
+++ b/spec/lib/gitlab/loop_helpers_spec.rb
@@ -0,0 +1,45 @@
+require 'spec_helper'
+
+describe Gitlab::LoopHelpers do
+  let(:class_instance) { (Class.new { include ::Gitlab::LoopHelpers }).new }
+
+  describe '#loop_until' do
+    subject do
+      class_instance.loop_until(**params) { true }
+    end
+
+    context 'when limit is not given' do
+      let(:params) { { limit: nil } }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(ArgumentError)
+      end
+    end
+
+    context 'when timeout is specified' do
+      let(:params) { { timeout: 1.second } }
+
+      it "returns false after it's expired" do
+        is_expected.to be_falsy
+      end
+
+      it 'executes the block at least once' do
+        expect { |b| class_instance.loop_until(**params, &b) }
+          .to yield_control.at_least(1)
+      end
+    end
+
+    context 'when iteration limit is specified' do
+      let(:params) { { limit: 1 } }
+
+      it "returns false after it's expired" do
+        is_expected.to be_falsy
+      end
+
+      it 'executes the block once' do
+        expect { |b| class_instance.loop_until(**params, &b) }
+          .to yield_control.once
+      end
+    end
+  end
+end
diff --git a/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
new file mode 100644
index 00000000000..80d82ba3ac9
--- /dev/null
+++ b/spec/services/ci/destroy_expired_job_artifacts_service_spec.rb
@@ -0,0 +1,104 @@
+require 'spec_helper'
+
+describe Ci::DestroyExpiredJobArtifactsService, :clean_gitlab_redis_shared_state do
+  include ExclusiveLeaseHelpers
+
+  describe '.execute' do
+    subject { service.execute }
+
+    let(:service) { described_class.new }
+    let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+
+    it 'destroys expired job artifacts' do
+      expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+    end
+
+    context 'when artifact is not expired' do
+      let!(:artifact) { create(:ci_job_artifact, expire_at: 1.day.since) }
+
+      it 'does not destroy expired job artifacts' do
+        expect { subject }.not_to change { Ci::JobArtifact.count }
+      end
+    end
+
+    context 'when artifact is permanent' do
+      let!(:artifact) { create(:ci_job_artifact, expire_at: nil) }
+
+      it 'does not destroy expired job artifacts' do
+        expect { subject }.not_to change { Ci::JobArtifact.count }
+      end
+    end
+
+    context 'when failed to destroy artifact' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 10)
+
+        allow_any_instance_of(Ci::JobArtifact)
+          .to receive(:destroy!)
+          .and_raise(ActiveRecord::RecordNotDestroyed)
+      end
+
+      it 'raises an exception and stop destroying' do
+        expect { subject }.to raise_error(ActiveRecord::RecordNotDestroyed)
+      end
+    end
+
+    context 'when exclusive lease has already been taken by the other instance' do
+      before do
+        stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY, timeout: described_class::LOCK_TIMEOUT)
+      end
+
+      it 'raises an error and does not start destroying' do
+        expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+      end
+    end
+
+    context 'when timeout happens' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_TIMEOUT', 1.second)
+        allow_any_instance_of(described_class).to receive(:destroy_batch) { true }
+      end
+
+      it 'returns false and does not continue destroying' do
+        is_expected.to be_falsy
+      end
+    end
+
+    context 'when loop reached loop limit' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::LOOP_LIMIT', 1)
+        stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+      end
+
+      let!(:artifact) { create_list(:ci_job_artifact, 2, expire_at: 1.day.ago) }
+
+      it 'raises an error and does not continue destroying' do
+        is_expected.to be_falsy
+      end
+
+      it 'destroys one artifact' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+      end
+    end
+
+    context 'when there are no artifacts' do
+      let!(:artifact) { }
+
+      it 'does not raise error' do
+        expect { subject }.not_to raise_error
+      end
+    end
+
+    context 'when there are artifacts more than batch sizes' do
+      before do
+        stub_const('Ci::DestroyExpiredJobArtifactsService::BATCH_SIZE', 1)
+      end
+
+      let!(:artifact) { create_list(:ci_job_artifact, 2, expire_at: 1.day.ago) }
+
+      it 'destroys all expired artifacts' do
+        expect { subject }.to change { Ci::JobArtifact.count }.by(-2)
+      end
+    end
+  end
+end
diff --git a/spec/workers/expire_build_artifacts_worker_spec.rb b/spec/workers/expire_build_artifacts_worker_spec.rb
index b47b4a02a68..27995cf1611 100644
--- a/spec/workers/expire_build_artifacts_worker_spec.rb
+++ b/spec/workers/expire_build_artifacts_worker_spec.rb
@@ -11,6 +11,7 @@ describe ExpireBuildArtifactsWorker do
 
   describe '#perform' do
     before do
+      stub_feature_flags(ci_new_expire_job_artifacts_service: false)
       build
     end
 
@@ -47,4 +48,17 @@ describe ExpireBuildArtifactsWorker do
       Sidekiq::Queues.jobs_by_worker['ExpireBuildInstanceArtifactsWorker']
     end
   end
+
+  describe '#perform with ci_new_expire_job_artifacts_service feature flag' do
+    before do
+      stub_feature_flags(ci_new_expire_job_artifacts_service: true)
+    end
+
+    it 'executes a service' do
+      expect_any_instance_of(Ci::DestroyExpiredJobArtifactsService).to receive(:execute)
+      expect(ExpireBuildInstanceArtifactsWorker).not_to receive(:bulk_perform_async)
+
+      worker.perform
+    end
+  end
 end