Merge branch 'optimise-stuck-ci-jobs-worker' into 'master'

Optimise StuckCiJobsWorker

Closes gitlab-com/infrastructure#3260

See merge request gitlab-org/gitlab-ce!15527

5 files changed, 32 insertions, 14 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 fdbc049c2df..367e227f680 100644
--- a/app/workers/stuck_ci_jobs_worker.rb
+++ b/app/workers/stuck_ci_jobs_worker.rb
@@ -45,9 +45,17 @@ 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)
+    loop do
+      jobs = Ci::Build.where(status: status)
+        .where('ci_builds.updated_at < ?', timeout.ago)
+        .includes(:tags, :runner, project: :namespace)
+        .limit(100)
+        .to_a
+      break if jobs.empty?
+
+      jobs.each do |job|
+        yield(job)
+      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
diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb
index ac6f4fefb4e..bdc64c6785b 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).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