From a26e25ea757f475afa1b6fd667c58fe15e306022 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 21 Nov 2017 18:44:52 +0100 Subject: [PATCH 1/4] Optimise StuckCiJobsWorker --- app/workers/stuck_ci_jobs_worker.rb | 11 ++++++++--- .../unreleased/optimise-stuck-ci-jobs-worker.yml | 5 +++++ 2 files changed, 13 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/optimise-stuck-ci-jobs-worker.yml diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index fdbc049c2df..566b4507965 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -45,9 +45,13 @@ class StuckCiJobsWorker end def search(status, timeout) - builds = Ci::Build.where(status: status).where('ci_builds.updated_at < ?', timeout.ago) - builds.joins(:project).merge(Project.without_deleted).includes(:tags, :runner, project: :namespace).find_each(batch_size: 50).each do |build| - yield(build) + Ci::Build.where(status: status).in_batches(of: 1000) do |batch| + batch = batch.where('ci_builds.updated_at < ?', timeout.ago) + .joins(:project).merge(Project.without_deleted) + .includes(:tags, :runner, project: :namespace) + batch.each do |build| + yield(build) + end end end @@ -58,3 +62,4 @@ class StuckCiJobsWorker end end end + diff --git a/changelogs/unreleased/optimise-stuck-ci-jobs-worker.yml b/changelogs/unreleased/optimise-stuck-ci-jobs-worker.yml new file mode 100644 index 00000000000..7f6adfb4fd8 --- /dev/null +++ b/changelogs/unreleased/optimise-stuck-ci-jobs-worker.yml @@ -0,0 +1,5 @@ +--- +title: Optimise StuckCiJobsWorker using cheap SQL query outside, and expensive inside +merge_request: +author: +type: performance From d58bab4aa535a6a82416b9bf664e99468ad18e4b Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 21 Nov 2017 19:49:10 +0100 Subject: [PATCH 2/4] Use not-ordered search --- app/workers/stuck_ci_jobs_worker.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 566b4507965..365a77c5be4 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -45,12 +45,17 @@ class StuckCiJobsWorker end def search(status, timeout) - Ci::Build.where(status: status).in_batches(of: 1000) do |batch| - batch = batch.where('ci_builds.updated_at < ?', timeout.ago) + loop do + jobs = Ci::Build.where(status: status) + .where('ci_builds.updated_at < ?', timeout.ago) .joins(:project).merge(Project.without_deleted) .includes(:tags, :runner, project: :namespace) - batch.each do |build| - yield(build) + .limit(100) + .to_a + break if jobs.empty? + + jobs.each do |job| + yield(job) end end end From 07c7ba1bf4a1d0092d07a23e23caf698512d46e0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 24 Nov 2017 16:30:08 +0100 Subject: [PATCH 3/4] Allow to drop jobs for deleted projects --- app/models/ci/build.rb | 1 + app/models/commit_status.rb | 16 ++++++++++------ app/workers/stuck_ci_jobs_worker.rb | 2 -- spec/workers/stuck_ci_jobs_worker_spec.rb | 4 ++-- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1e9a3920667..4ea040dfad5 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -104,6 +104,7 @@ module Ci end before_transition any => [:failed] do |build| + next unless build.project next if build.retries_max.zero? if build.retries_count < build.retries_max diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6b07dbdf3ea..ee21ed8e420 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -17,6 +17,7 @@ class CommitStatus < ActiveRecord::Base validates :name, presence: true, unless: :importing? alias_attribute :author, :user + alias_attribute :pipeline_id, :commit_id scope :failed_but_allowed, -> do where(allow_failure: true, status: [:failed, :canceled]) @@ -103,26 +104,29 @@ class CommitStatus < ActiveRecord::Base end after_transition do |commit_status, transition| + next unless commit_status.project next if transition.loopback? commit_status.run_after_commit do - if pipeline + if pipeline_id if complete? || manual? - PipelineProcessWorker.perform_async(pipeline.id) + PipelineProcessWorker.perform_async(pipeline_id) else - PipelineUpdateWorker.perform_async(pipeline.id) + PipelineUpdateWorker.perform_async(pipeline_id) end end - StageUpdateWorker.perform_async(commit_status.stage_id) - ExpireJobCacheWorker.perform_async(commit_status.id) + StageUpdateWorker.perform_async(stage_id) + ExpireJobCacheWorker.perform_async(id) end end after_transition any => :failed do |commit_status| + next unless commit_status.project + commit_status.run_after_commit do MergeRequests::AddTodoWhenBuildFailsService - .new(pipeline.project, nil).execute(self) + .new(project, nil).execute(self) end end end diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 365a77c5be4..367e227f680 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -48,7 +48,6 @@ class StuckCiJobsWorker loop do jobs = Ci::Build.where(status: status) .where('ci_builds.updated_at < ?', timeout.ago) - .joins(:project).merge(Project.without_deleted) .includes(:tags, :runner, project: :namespace) .limit(100) .to_a @@ -67,4 +66,3 @@ class StuckCiJobsWorker end end end - diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index ac6f4fefb4e..7f9545177bc 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -105,8 +105,8 @@ describe StuckCiJobsWorker do job.project.update(pending_delete: true) end - it 'does not drop job' do - expect_any_instance_of(Ci::Build).not_to receive(:drop) + it 'does drop job' do + expect_any_instance_of(Ci::Build).to receive(:drop) worker.perform end end From 6c1a4294209157b43db82678d34e3cba7d2cba3a Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 27 Nov 2017 11:56:06 +0100 Subject: [PATCH 4/4] Fix stuck jobs tests --- spec/workers/stuck_ci_jobs_worker_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index 7f9545177bc..bdc64c6785b 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -106,7 +106,7 @@ describe StuckCiJobsWorker do end it 'does drop job' do - expect_any_instance_of(Ci::Build).to receive(:drop) + expect_any_instance_of(Ci::Build).to receive(:drop).and_call_original worker.perform end end @@ -117,7 +117,7 @@ describe StuckCiJobsWorker do let(:worker2) { described_class.new } it 'is guard by exclusive lease when executed concurrently' do - expect(worker).to receive(:drop).at_least(:once) + expect(worker).to receive(:drop).at_least(:once).and_call_original expect(worker2).not_to receive(:drop) worker.perform allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(false) @@ -125,8 +125,8 @@ describe StuckCiJobsWorker do end it 'can be executed in sequence' do - expect(worker).to receive(:drop).at_least(:once) - expect(worker2).to receive(:drop).at_least(:once) + expect(worker).to receive(:drop).at_least(:once).and_call_original + expect(worker2).to receive(:drop).at_least(:once).and_call_original worker.perform worker2.perform end