Merge branch 'feature/gb/use-pending-builds-table' into 'master'

Enable pending builds table parity by default [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!62195

app/models/ci/build.rb

@@ -1076,6 +1076,10 @@ module Ci
       ::Ci::PendingBuild.where(build_id: self.id)
     end
 
+    def create_queuing_entry!
+      ::Ci::PendingBuild.upsert_from_build!(self)
+    end
+
     protected
 
     def run_status_commit_hooks!

app/services/ci/register_job_service.rb

@@ -287,15 +287,11 @@ module Ci
           .order(Arel.sql('COALESCE(project_builds.running_builds, 0) ASC'), 'ci_builds.id ASC')
       end
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def builds_for_project_runner
       new_builds.where(project: runner.projects.without_deleted.with_builds_enabled).order('id ASC')
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def builds_for_group_runner
       # Workaround for weird Rails bug, that makes `runner.groups.to_sql` to return `runner_id = NULL`
       groups = ::Group.joins(:runner_namespaces).merge(runner.runner_namespaces)
@@ -307,17 +303,23 @@ module Ci
         .without_deleted
       new_builds.where(project: projects).order('id ASC')
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
-    # rubocop: disable CodeReuse/ActiveRecord
     def running_builds_for_shared_runners
       Ci::Build.running.where(runner: Ci::Runner.instance_type)
         .group(:project_id).select(:project_id, 'count(*) AS running_builds')
     end
+
+    def all_builds
+      if Feature.enabled?(:ci_pending_builds_queue_join, runner, default_enabled: :yaml)
+        Ci::Build.joins(:queuing_entry)
+      else
+        Ci::Build.all
+      end
+    end
     # rubocop: enable CodeReuse/ActiveRecord
 
     def new_builds
-      builds = Ci::Build.pending.unstarted
+      builds = all_builds.pending.unstarted
       builds = builds.ref_protected if runner.ref_protected?
       builds
     end

app/services/ci/update_build_queue_service.rb

@@ -19,7 +19,7 @@ module Ci
       raise InvalidQueueTransition unless transition.to == 'pending'
 
       transition.within_transaction do
-        result = ::Ci::PendingBuild.upsert_from_build!(build)
+        result = build.create_queuing_entry!
 
         unless result.empty?
           metrics.increment_queue_operation(:build_queue_push)

config/feature_flags/development/ci_pending_builds_queue_join.yml

+---
+name: ci_pending_builds_queue_join
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/62195
+rollout_issue_url:
+milestone: '13.12'
+type: development
+group: group::pipeline execution
+default_enabled: false

config/feature_flags/development/ci_pending_builds_queue_maintain.yml

 ---
 name: ci_pending_builds_queue_maintain
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61581
-rollout_issue_url: 
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/331496
 milestone: '13.12'
 type: development
 group: group::continuous integration

db/post_migrate/20210525075724_clean_up_pending_builds_table.rb

+# frozen_string_literal: true
+
+class CleanUpPendingBuildsTable < ActiveRecord::Migration[6.0]
+  BATCH_SIZE = 1000
+
+  disable_ddl_transaction!
+
+  def up
+    return unless Gitlab.dev_or_test_env? || Gitlab.com?
+
+    each_batch('ci_pending_builds', of: BATCH_SIZE) do |min, max|
+      execute <<~SQL
+        DELETE FROM ci_pending_builds
+          USING ci_builds
+          WHERE ci_builds.id = ci_pending_builds.build_id
+            AND ci_builds.status != 'pending'
+            AND ci_builds.type = 'Ci::Build'
+            AND ci_pending_builds.id BETWEEN #{min} AND #{max}
+      SQL
+    end
+  end
+
+  def down
+    # noop
+  end
+
+  private
+
+  def each_batch(table_name, scope: ->(table) { table.all }, of: 1000)
+    table = Class.new(ActiveRecord::Base) do
+      include EachBatch
+
+      self.table_name = table_name
+      self.inheritance_column = :_type_disabled
+    end
+
+    scope.call(table).each_batch(of: of) do |batch|
+      yield batch.pluck('MIN(id), MAX(id)').first
+    end
+  end
+end

db/schema_migrations/20210525075724

+5dc1119c5efe28225bb7ac8a9ed2c4c5cfaeaff202194ed4419cfd54eaf7483d
\ No newline at end of file

ee/spec/requests/api/ci/runner_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe API::Ci::Runner do
           }
         end
 
-        let!(:ci_build) { create(:ci_build, pipeline: pipeline, secrets: secrets) }
+        let!(:ci_build) { create(:ci_build, :pending, :queued, pipeline: pipeline, secrets: secrets) }
 
         context 'when secrets management feature is available' do
           before do

ee/spec/services/ci/register_job_service_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 RSpec.describe Ci::RegisterJobService do
   let_it_be_with_refind(:shared_runner) { create(:ci_runner, :instance) }
 
-  let!(:project) { create :project, shared_runners_enabled: true }
-  let!(:pipeline) { create :ci_empty_pipeline, project: project }
-  let!(:pending_build) { create :ci_build, pipeline: pipeline }
+  let!(:project) { create(:project, shared_runners_enabled: true) }
+  let!(:pipeline) { create(:ci_empty_pipeline, project: project) }
+  let!(:pending_build) { create(:ci_build, :pending, :queued, pipeline: pipeline) }
 
   describe '#execute' do
     context 'shared runners minutes limit' do

spec/factories/ci/builds.rb

@@ -79,6 +79,7 @@ FactoryBot.define do
 
     trait :pending do
       queued_at { 'Di 29. Okt 09:50:59 CET 2013' }
+
       status { 'pending' }
     end
 
@@ -286,6 +287,15 @@ FactoryBot.define do
 
     trait :queued do
       queued_at { Time.now }
+
+      after(:create) do |build|
+        build.create_queuing_entry!
+      end
+    end
+
+    trait :picked do
+      running
+
       runner factory: :ci_runner
     end
 

spec/migrations/clean_up_pending_builds_table_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20210525075724_clean_up_pending_builds_table.rb')
+
+RSpec.describe CleanUpPendingBuildsTable do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:queue) { table(:ci_pending_builds) }
+  let(:builds) { table(:ci_builds) }
+
+  before do
+    namespaces.create!(id: 123, name: 'sample', path: 'sample')
+    projects.create!(id: 123, name: 'sample', path: 'sample', namespace_id: 123)
+
+    builds.create!(id: 1, project_id: 123, status: 'pending', type: 'Ci::Build')
+    builds.create!(id: 2, project_id: 123, status: 'pending', type: 'GenericCommitStatus')
+    builds.create!(id: 3, project_id: 123, status: 'success', type: 'Ci::Bridge')
+    builds.create!(id: 4, project_id: 123, status: 'success', type: 'Ci::Build')
+    builds.create!(id: 5, project_id: 123, status: 'running', type: 'Ci::Build')
+    builds.create!(id: 6, project_id: 123, status: 'created', type: 'Ci::Build')
+
+    queue.create!(id: 1, project_id: 123, build_id: 1)
+    queue.create!(id: 2, project_id: 123, build_id: 4)
+    queue.create!(id: 3, project_id: 123, build_id: 5)
+  end
+
+  it 'removes duplicated data from pending builds table' do
+    migrate!
+
+    expect(queue.all.count).to eq 1
+    expect(queue.first.id).to eq 1
+    expect(builds.all.count).to eq 6
+  end
+
+  context 'when there are multiple batches' do
+    before do
+      stub_const("#{described_class}::BATCH_SIZE", 1)
+    end
+
+    it 'iterates the data correctly' do
+      migrate!
+
+      expect(queue.all.count).to eq 1
+    end
+  end
+end

spec/models/ci/build_spec.rb

@@ -354,7 +354,7 @@ RSpec.describe Ci::Build do
       it 'does not push build to the queue' do
         build.enqueue
 
-        expect(::Ci::PendingBuild.all.count).to be_zero
+        expect(build.queuing_entry).not_to be_present
       end
     end
 

spec/requests/api/ci/runner/jobs_request_post_spec.rb

@@ -23,7 +23,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
     let(:runner) { create(:ci_runner, :project, projects: [project]) }
     let(:user) { create(:user) }
     let(:job) do
-      create(:ci_build, :artifacts, :extended_options,
+      create(:ci_build, :pending, :queued, :artifacts, :extended_options,
              pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0)
     end
 
@@ -129,7 +129,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         context 'when other projects have pending jobs' do
           before do
             job.success
-            create(:ci_build, :pending)
+            create(:ci_build, :pending, :queued)
           end
 
           it_behaves_like 'no jobs available'
@@ -239,7 +239,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when job is made for tag' do
-            let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
 
             it 'sets branch as ref_type' do
               request_job
@@ -297,7 +297,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when job filtered by job_age' do
-            let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, queued_at: 60.seconds.ago) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, queued_at: 60.seconds.ago) }
 
             context 'job is queued less than job_age parameter' do
               let(:job_age) { 120 }
@@ -359,7 +359,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when job is for a release' do
-            let!(:job) { create(:ci_build, :release_options, pipeline: pipeline) }
+            let!(:job) { create(:ci_build, :pending, :queued, :release_options, pipeline: pipeline) }
 
             context 'when `multi_build_steps` is passed by the runner' do
               it 'exposes release info' do
@@ -398,7 +398,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
 
           context 'when job is made for merge request' do
             let(:pipeline) { create(:ci_pipeline, source: :merge_request_event, project: project, ref: 'feature', merge_request: merge_request) }
-            let!(:job) { create(:ci_build, pipeline: pipeline, name: 'spinach', ref: 'feature', stage: 'test', stage_idx: 0) }
+            let!(:job) { create(:ci_build, :pending, :queued, pipeline: pipeline, name: 'spinach', ref: 'feature', stage: 'test', stage_idx: 0) }
             let(:merge_request) { create(:merge_request) }
 
             it 'sets branch as ref_type' do
@@ -479,9 +479,9 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when project and pipeline have multiple jobs' do
-            let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
-            let!(:job2) { create(:ci_build, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
-            let!(:test_job) { create(:ci_build, pipeline: pipeline, name: 'deploy', stage: 'deploy', stage_idx: 1) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
+            let!(:job2) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
+            let!(:test_job) { create(:ci_build, :pending, :queued, pipeline: pipeline, name: 'deploy', stage: 'deploy', stage_idx: 1) }
 
             before do
               job.success
@@ -531,8 +531,8 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when pipeline have jobs with artifacts' do
-            let!(:job) { create(:ci_build, :tag, :artifacts, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
-            let!(:test_job) { create(:ci_build, pipeline: pipeline, name: 'deploy', stage: 'deploy', stage_idx: 1) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, :artifacts, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
+            let!(:test_job) { create(:ci_build, :pending, :queued, pipeline: pipeline, name: 'deploy', stage: 'deploy', stage_idx: 1) }
 
             before do
               job.success
@@ -551,10 +551,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when explicit dependencies are defined' do
-            let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
-            let!(:job2) { create(:ci_build, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
+            let!(:job2) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
             let!(:test_job) do
-              create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'deploy',
+              create(:ci_build, :pending, :queued, pipeline: pipeline, token: 'test-job-token', name: 'deploy',
                                 stage: 'deploy', stage_idx: 1,
                                 options: { script: ['bash'], dependencies: [job2.name] })
             end
@@ -575,10 +575,10 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           end
 
           context 'when dependencies is an empty array' do
-            let!(:job) { create(:ci_build, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
-            let!(:job2) { create(:ci_build, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
+            let!(:job) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0) }
+            let!(:job2) { create(:ci_build, :pending, :queued, :tag, pipeline: pipeline, name: 'rubocop', stage: 'test', stage_idx: 0) }
             let!(:empty_dependencies_job) do
-              create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'empty_dependencies_job',
+              create(:ci_build, :pending, :queued, pipeline: pipeline, token: 'test-job-token', name: 'empty_dependencies_job',
                                 stage: 'deploy', stage_idx: 1,
                                 options: { script: ['bash'], dependencies: [] })
             end
@@ -739,7 +739,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         end
 
         describe 'port support' do
-          let(:job) { create(:ci_build, pipeline: pipeline, options: options) }
+          let(:job) { create(:ci_build, :pending, :queued, pipeline: pipeline, options: options) }
 
           context 'when job image has ports' do
             let(:options) do
@@ -791,7 +791,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
         describe 'a job with excluded artifacts' do
           context 'when excluded paths are defined' do
             let(:job) do
-              create(:ci_build, pipeline: pipeline, token: 'test-job-token', name: 'test',
+              create(:ci_build, :pending, :queued, pipeline: pipeline, token: 'test-job-token', name: 'test',
                                 stage: 'deploy', stage_idx: 1,
                                 options: { artifacts: { paths: ['abc'], exclude: ['cde'] } })
             end
@@ -839,7 +839,7 @@ RSpec.describe API::Ci::Runner, :clean_gitlab_redis_shared_state do
           subject { request_job }
 
           context 'when triggered by a user' do
-            let(:job) { create(:ci_build, user: user, project: project) }
+            let(:job) { create(:ci_build, :pending, :queued, user: user, project: project) }
 
             subject { request_job(id: job.id) }
 

spec/services/ci/retry_build_service_spec.rb

@@ -66,7 +66,7 @@ RSpec.describe Ci::RetryBuildService do
     let_it_be(:another_pipeline) { create(:ci_empty_pipeline, project: project) }
 
     let_it_be(:build) do
-      create(:ci_build, :failed, :expired, :erased, :queued, :coverage, :tags,
+      create(:ci_build, :failed, :picked, :expired, :erased, :queued, :coverage, :tags,
              :allowed_to_fail, :on_tag, :triggered, :teardown_environment, :resource_group,
              description: 'my-job', stage: 'test', stage_id: stage.id,
              pipeline: pipeline, auto_canceled_by: another_pipeline,