From 9afb2dac5c9f49f9f7943053b50a3808a99fdf6b Mon Sep 17 00:00:00 2001 From: Paco Guzman Date: Fri, 7 Oct 2016 11:11:02 +0200 Subject: [PATCH] ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job per build to cleanup We use Sidekiq::Client.push_bulk to avoid Redis round trips --- CHANGELOG | 1 + app/workers/expire_build_artifacts_worker.rb | 11 ++- .../expire_build_instance_artifacts_worker.rb | 11 +++ .../expire_build_artifacts_worker_spec.rb | 51 ++++---------- ...re_build_instance_artifacts_worker_spec.rb | 69 +++++++++++++++++++ 5 files changed, 100 insertions(+), 43 deletions(-) create mode 100644 app/workers/expire_build_instance_artifacts_worker.rb create mode 100644 spec/workers/expire_build_instance_artifacts_worker_spec.rb 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 -- GitLab