From d78f0724d9cc3dd7c1f22cef54ad59f8d2b579c4 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Thu, 1 Aug 2019 19:05:02 +0700 Subject: [PATCH] Avoid conflicts between ArchiveTraceWorkers This commits avoiding conflicts between ArchiveTraceWorker and ArchiveTracesCronWorker by changing the target of the latter worker. --- app/models/ci/build.rb | 2 + app/workers/ci/archive_traces_cron_worker.rb | 2 +- ...condition-of-archive-trace-cron-worker.yml | 5 ++ spec/models/ci/build_spec.rb | 50 +++++++++++++++++++ .../ci/archive_traces_cron_worker_spec.rb | 16 ++++-- 5 files changed, 70 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/avoid-race-condition-of-archive-trace-cron-worker.yml diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 3c0efca31db..7930bef5cf2 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -121,6 +121,8 @@ module Ci scope :scheduled_actions, ->() { where(when: :delayed, status: COMPLETED_STATUSES + %i[scheduled]) } scope :ref_protected, -> { where(protected: true) } scope :with_live_trace, -> { where('EXISTS (?)', Ci::BuildTraceChunk.where('ci_builds.id = ci_build_trace_chunks.build_id').select(1)) } + scope :with_stale_live_trace, -> { with_live_trace.finished_before(12.hours.ago) } + scope :finished_before, -> (date) { finished.where('finished_at < ?', date) } scope :matches_tag_ids, -> (tag_ids) do matcher = ::ActsAsTaggableOn::Tagging diff --git a/app/workers/ci/archive_traces_cron_worker.rb b/app/workers/ci/archive_traces_cron_worker.rb index 75e68d0233a..ef2da729705 100644 --- a/app/workers/ci/archive_traces_cron_worker.rb +++ b/app/workers/ci/archive_traces_cron_worker.rb @@ -10,7 +10,7 @@ module Ci # Archive stale live traces which still resides in redis or database # This could happen when ArchiveTraceWorker sidekiq jobs were lost by receiving SIGKILL # More details in https://gitlab.com/gitlab-org/gitlab-ce/issues/36791 - Ci::Build.finished.with_live_trace.find_each(batch_size: 100) do |build| + Ci::Build.with_stale_live_trace.find_each(batch_size: 100) do |build| Ci::ArchiveTraceService.new.execute(build, worker_name: self.class.name) end end diff --git a/changelogs/unreleased/avoid-race-condition-of-archive-trace-cron-worker.yml b/changelogs/unreleased/avoid-race-condition-of-archive-trace-cron-worker.yml new file mode 100644 index 00000000000..b3f84ec32d2 --- /dev/null +++ b/changelogs/unreleased/avoid-race-condition-of-archive-trace-cron-worker.yml @@ -0,0 +1,5 @@ +--- +title: Avoid conflicts between ArchiveTracesCronWorker and ArchiveTraceWorker +merge_request: 31376 +author: +type: fixed diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 4aac4b640f4..bc853d45085 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -149,6 +149,56 @@ describe Ci::Build do end end + describe '.with_stale_live_trace' do + subject { described_class.with_stale_live_trace } + + context 'when build has a stale live trace' do + let!(:build) { create(:ci_build, :success, :trace_live, finished_at: 1.day.ago) } + + it 'selects the build' do + is_expected.to eq([build]) + end + end + + context 'when build does not have a stale live trace' do + let!(:build) { create(:ci_build, :success, :trace_live, finished_at: 1.hour.ago) } + + it 'does not select the build' do + is_expected.to be_empty + end + end + end + + describe '.finished_before' do + subject { described_class.finished_before(date) } + + let(:date) { 1.hour.ago } + + context 'when build has finished one day ago' do + let!(:build) { create(:ci_build, :success, finished_at: 1.day.ago) } + + it 'selects the build' do + is_expected.to eq([build]) + end + end + + context 'when build has finished 30 minutes ago' do + let!(:build) { create(:ci_build, :success, finished_at: 30.minutes.ago) } + + it 'returns an empty array' do + is_expected.to be_empty + end + end + + context 'when build is still running' do + let!(:build) { create(:ci_build, :running) } + + it 'returns an empty array' do + is_expected.to be_empty + end + end + end + describe '.with_reports' do subject { described_class.with_reports(Ci::JobArtifact.test_reports) } diff --git a/spec/workers/ci/archive_traces_cron_worker_spec.rb b/spec/workers/ci/archive_traces_cron_worker_spec.rb index 28381fdc3be..01232e2a58b 100644 --- a/spec/workers/ci/archive_traces_cron_worker_spec.rb +++ b/spec/workers/ci/archive_traces_cron_worker_spec.rb @@ -5,6 +5,8 @@ require 'spec_helper' describe Ci::ArchiveTracesCronWorker do subject { described_class.new.perform } + let(:finished_at) { 1.day.ago } + before do stub_feature_flags(ci_enable_live_trace: true) end @@ -28,7 +30,7 @@ describe Ci::ArchiveTracesCronWorker do end context 'when a job succeeded' do - let!(:build) { create(:ci_build, :success, :trace_live) } + let!(:build) { create(:ci_build, :success, :trace_live, finished_at: finished_at) } it_behaves_like 'archives trace' @@ -39,9 +41,15 @@ describe Ci::ArchiveTracesCronWorker do subject end + context 'when the job finished recently' do + let(:finished_at) { 1.hour.ago } + + it_behaves_like 'does not archive trace' + end + context 'when a trace had already been archived' do let!(:build) { create(:ci_build, :success, :trace_live, :trace_artifact) } - let!(:build2) { create(:ci_build, :success, :trace_live) } + let!(:build2) { create(:ci_build, :success, :trace_live, finished_at: finished_at) } it 'continues to archive live traces' do subject @@ -52,7 +60,7 @@ describe Ci::ArchiveTracesCronWorker do end context 'when an unexpected exception happened during archiving' do - let!(:build) { create(:ci_build, :success, :trace_live) } + let!(:build) { create(:ci_build, :success, :trace_live, finished_at: finished_at) } before do allow(Gitlab::Sentry).to receive(:track_exception) @@ -71,7 +79,7 @@ describe Ci::ArchiveTracesCronWorker do end context 'when a job was cancelled' do - let!(:build) { create(:ci_build, :canceled, :trace_live) } + let!(:build) { create(:ci_build, :canceled, :trace_live, finished_at: finished_at) } it_behaves_like 'archives trace' end