Merge branch '23096-expire-artifacts-per-job' into 'master'

ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job… See merge request !6732

Merge branch '23096-expire-artifacts-per-job' into 'master'
ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job… See merge request !6732
c901936a · Yorick Peterse · 2cd8b5d2 · 9afb2dac · c901936a · c901936a
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -7,6 +7,7 @@ v 8.13.0 (unreleased)
  - Use gitlab-shell v3.6.2 (GIT TRACE logging)
  - Add `/projects/visible` API endpoint (Ben Boeckel)
  - 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)

--- 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
--- a/app/workers/expire_build_instance_artifacts_worker.rb
+++ b/app/workers/expire_build_instance_artifacts_worker.rb
+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
--- a/spec/workers/expire_build_artifacts_worker_spec.rb
+++ b/spec/workers/expire_build_artifacts_worker_spec.rb
@@ -5,65 +5,42 @@

  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
--- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb
+++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb
+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