ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job per build to cleanup23096-expire-artifacts-per-job

We use Sidekiq::Client.push_bulk to avoid Redis round trips

5 files changed, 100 insertions, 43 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 8bfc5c97820..fec8d5fba29 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -6,6 +6,7 @@ v 8.13.0 (unreleased)
   - Improve issue load time performance by avoiding ORDER BY in find_by call
   - Use gitlab-shell v3.6.2 (GIT TRACE logging)
   - Fix centering of custom header logos (Ashley Dumaine)
+  - ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job per build to cleanup
   - AbstractReferenceFilter caches project_refs on RequestStore when active
   - Replaced the check sign to arrow in the show build view. !6501
   - Add a /wip slash command to toggle the Work In Progress status of a merge request. !6259 (tbalthazar)
diff --git a/app/workers/expire_build_artifacts_worker.rb b/app/workers/expire_build_artifacts_worker.rb
index c64ea108d52..174eabff9fd 100644
--- a/app/workers/expire_build_artifacts_worker.rb
+++ b/app/workers/expire_build_artifacts_worker.rb
@@ -2,12 +2,11 @@ class ExpireBuildArtifactsWorker
   include Sidekiq::Worker
 
   def perform
-    Rails.logger.info 'Cleaning old build artifacts'
+    Rails.logger.info 'Scheduling removal of build artifacts'
 
-    builds = Ci::Build.with_expired_artifacts
-    builds.find_each(batch_size: 50).each do |build|
-      Rails.logger.debug "Removing artifacts build #{build.id}..."
-      build.erase_artifacts!
-    end
+    build_ids = Ci::Build.with_expired_artifacts.pluck(:id)
+    build_ids = build_ids.map { |build_id| [build_id] }
+
+    Sidekiq::Client.push_bulk('class' => ExpireBuildInstanceArtifactsWorker, 'args' => build_ids )
   end
 end
diff --git a/app/workers/expire_build_instance_artifacts_worker.rb b/app/workers/expire_build_instance_artifacts_worker.rb
new file mode 100644
index 00000000000..916c2e633c1
--- /dev/null
+++ b/app/workers/expire_build_instance_artifacts_worker.rb
@@ -0,0 +1,11 @@
+class ExpireBuildInstanceArtifactsWorker
+  include Sidekiq::Worker
+
+  def perform(build_id)
+    build = Ci::Build.with_expired_artifacts.reorder(nil).find_by(id: build_id)
+    return unless build
+
+    Rails.logger.info "Removing artifacts build #{build.id}..."
+    build.erase_artifacts!
+  end
+end
diff --git a/spec/workers/expire_build_artifacts_worker_spec.rb b/spec/workers/expire_build_artifacts_worker_spec.rb
index 7d6668920c0..73cbadc13d9 100644
--- a/spec/workers/expire_build_artifacts_worker_spec.rb
+++ b/spec/workers/expire_build_artifacts_worker_spec.rb
@@ -5,65 +5,42 @@ describe ExpireBuildArtifactsWorker do
 
   let(:worker) { described_class.new }
 
+  before { Sidekiq::Worker.clear_all }
+
   describe '#perform' do
     before { build }
 
-    subject! { worker.perform }
+    subject! do
+      Sidekiq::Testing.fake! { worker.perform }
+    end
 
     context 'with expired artifacts' do
       let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now - 7.days) }
 
-      it 'does expire' do
-        expect(build.reload.artifacts_expired?).to be_truthy
-      end
-
-      it 'does remove files' do
-        expect(build.reload.artifacts_file.exists?).to be_falsey
-      end
-
-      it 'does nullify artifacts_file column' do
-        expect(build.reload.artifacts_file_identifier).to be_nil
+      it 'enqueues that build' do
+        expect(jobs_enqueued.size).to eq(1)
+        expect(jobs_enqueued[0]["args"]).to eq([build.id])
       end
     end
 
     context 'with not yet expired artifacts' do
       let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) }
 
-      it 'does not expire' do
-        expect(build.reload.artifacts_expired?).to be_falsey
-      end
-
-      it 'does not remove files' do
-        expect(build.reload.artifacts_file.exists?).to be_truthy
-      end
-
-      it 'does not nullify artifacts_file column' do
-        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      it 'does not enqueue that build' do
+        expect(jobs_enqueued.size).to eq(0)
       end
     end
 
     context 'without expire date' do
       let(:build) { create(:ci_build, :artifacts) }
 
-      it 'does not expire' do
-        expect(build.reload.artifacts_expired?).to be_falsey
-      end
-
-      it 'does not remove files' do
-        expect(build.reload.artifacts_file.exists?).to be_truthy
-      end
-
-      it 'does not nullify artifacts_file column' do
-        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      it 'does not enqueue that build' do
+        expect(jobs_enqueued.size).to eq(0)
       end
     end
 
-    context 'for expired artifacts' do
-      let(:build) { create(:ci_build, artifacts_expire_at: Time.now - 7.days) }
-
-      it 'is still expired' do
-        expect(build.reload.artifacts_expired?).to be_truthy
-      end
+    def jobs_enqueued
+      Sidekiq::Queues.jobs_by_worker['ExpireBuildInstanceArtifactsWorker']
     end
   end
 end
diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb
new file mode 100644
index 00000000000..2b140f2ba28
--- /dev/null
+++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe ExpireBuildInstanceArtifactsWorker do
+  include RepoHelpers
+
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    before { build }
+
+    subject! { worker.perform(build.id) }
+
+    context 'with expired artifacts' do
+      let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now - 7.days) }
+
+      it 'does expire' do
+        expect(build.reload.artifacts_expired?).to be_truthy
+      end
+
+      it 'does remove files' do
+        expect(build.reload.artifacts_file.exists?).to be_falsey
+      end
+
+      it 'does nullify artifacts_file column' do
+        expect(build.reload.artifacts_file_identifier).to be_nil
+      end
+    end
+
+    context 'with not yet expired artifacts' do
+      let(:build) { create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days) }
+
+      it 'does not expire' do
+        expect(build.reload.artifacts_expired?).to be_falsey
+      end
+
+      it 'does not remove files' do
+        expect(build.reload.artifacts_file.exists?).to be_truthy
+      end
+
+      it 'does not nullify artifacts_file column' do
+        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      end
+    end
+
+    context 'without expire date' do
+      let(:build) { create(:ci_build, :artifacts) }
+
+      it 'does not expire' do
+        expect(build.reload.artifacts_expired?).to be_falsey
+      end
+
+      it 'does not remove files' do
+        expect(build.reload.artifacts_file.exists?).to be_truthy
+      end
+
+      it 'does not nullify artifacts_file column' do
+        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      end
+    end
+
+    context 'for expired artifacts' do
+      let(:build) { create(:ci_build, artifacts_expire_at: Time.now - 7.days) }
+
+      it 'is still expired' do
+        expect(build.reload.artifacts_expired?).to be_truthy
+      end
+    end
+  end
+end