Merge branch 'backstage/gb/migrate-pipeline-stages-index' into 'master'

Migrate pipeline stages indices

Closes #43009

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

17 files changed, 234 insertions, 8 deletions

diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb
index 75b8ea2a371..5a1eeb966aa 100644
--- a/app/models/ci/stage.rb
+++ b/app/models/ci/stage.rb
@@ -13,14 +13,27 @@ module Ci
     has_many :statuses, class_name: 'CommitStatus', foreign_key: :stage_id
     has_many :builds, foreign_key: :stage_id
 
-    validates :project, presence: true, unless: :importing?
-    validates :pipeline, presence: true, unless: :importing?
-    validates :name, presence: true, unless: :importing?
+    with_options unless: :importing? do
+      validates :project, presence: true
+      validates :pipeline, presence: true
+      validates :name, presence: true
+      validates :position, presence: true
+    end
 
-    after_initialize do |stage|
+    after_initialize do
       self.status = DEFAULT_STATUS if self.status.nil?
     end
 
+    before_validation unless: :importing? do
+      next if position.present?
+
+      self.position = statuses.select(:stage_idx)
+        .where('stage_idx IS NOT NULL')
+        .group(:stage_idx)
+        .order('COUNT(*) DESC')
+        .first&.stage_idx.to_i
+    end
+
     state_machine :status, initial: :created do
       event :enqueue do
         transition created: :pending
diff --git a/app/services/ci/ensure_stage_service.rb b/app/services/ci/ensure_stage_service.rb
index 87f19b333de..b8c7be2d350 100644
--- a/app/services/ci/ensure_stage_service.rb
+++ b/app/services/ci/ensure_stage_service.rb
@@ -42,6 +42,7 @@ module Ci
 
     def create_stage
       Ci::Stage.create!(name: @build.stage,
+                        position: @build.stage_idx,
                         pipeline: @build.pipeline,
                         project: @build.project)
     end
diff --git a/db/migrate/20180417101040_add_tmp_stage_priority_index_to_ci_builds.rb b/db/migrate/20180417101040_add_tmp_stage_priority_index_to_ci_builds.rb
new file mode 100644
index 00000000000..ee82c70ecf8
--- /dev/null
+++ b/db/migrate/20180417101040_add_tmp_stage_priority_index_to_ci_builds.rb
@@ -0,0 +1,16 @@
+class AddTmpStagePriorityIndexToCiBuilds < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index(:ci_builds, [:stage_id, :stage_idx],
+                         where: 'stage_idx IS NOT NULL', name: 'tmp_build_stage_position_index')
+  end
+
+  def down
+    remove_concurrent_index_by_name(:ci_builds, 'tmp_build_stage_position_index')
+  end
+end
diff --git a/db/migrate/20180417101940_add_index_to_ci_stage.rb b/db/migrate/20180417101940_add_index_to_ci_stage.rb
new file mode 100644
index 00000000000..9dac78db774
--- /dev/null
+++ b/db/migrate/20180417101940_add_index_to_ci_stage.rb
@@ -0,0 +1,9 @@
+class AddIndexToCiStage < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :ci_stages, :position, :integer
+  end
+end
diff --git a/db/post_migrate/20180420080616_schedule_stages_index_migration.rb b/db/post_migrate/20180420080616_schedule_stages_index_migration.rb
new file mode 100644
index 00000000000..1d0daad002f
--- /dev/null
+++ b/db/post_migrate/20180420080616_schedule_stages_index_migration.rb
@@ -0,0 +1,29 @@
+class ScheduleStagesIndexMigration < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  MIGRATION = 'MigrateStageIndex'.freeze
+  BATCH_SIZE = 10000
+
+  disable_ddl_transaction!
+
+  class Stage < ActiveRecord::Base
+    include EachBatch
+    self.table_name = 'ci_stages'
+  end
+
+  def up
+    disable_statement_timeout
+
+    Stage.all.tap do |relation|
+      queue_background_migration_jobs_by_range_at_intervals(relation,
+                                                            MIGRATION,
+                                                            5.minutes,
+                                                            batch_size: BATCH_SIZE)
+    end
+  end
+
+  def down
+    # noop
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 5853b428430..10cd1bff125 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -322,6 +322,7 @@ ActiveRecord::Schema.define(version: 20180425131009) do
   add_index "ci_builds", ["project_id", "id"], name: "index_ci_builds_on_project_id_and_id", using: :btree
   add_index "ci_builds", ["protected"], name: "index_ci_builds_on_protected", using: :btree
   add_index "ci_builds", ["runner_id"], name: "index_ci_builds_on_runner_id", using: :btree
+  add_index "ci_builds", ["stage_id", "stage_idx"], name: "tmp_build_stage_position_index", where: "(stage_idx IS NOT NULL)", using: :btree
   add_index "ci_builds", ["stage_id"], name: "index_ci_builds_on_stage_id", using: :btree
   add_index "ci_builds", ["status", "type", "runner_id"], name: "index_ci_builds_on_status_and_type_and_runner_id", using: :btree
   add_index "ci_builds", ["status"], name: "index_ci_builds_on_status", using: :btree
@@ -486,6 +487,7 @@ ActiveRecord::Schema.define(version: 20180425131009) do
     t.string "name"
     t.integer "status"
     t.integer "lock_version"
+    t.integer "position"
   end
 
   add_index "ci_stages", ["pipeline_id", "name"], name: "index_ci_stages_on_pipeline_id_and_name", unique: true, using: :btree
diff --git a/lib/gitlab/background_migration/migrate_stage_index.rb b/lib/gitlab/background_migration/migrate_stage_index.rb
new file mode 100644
index 00000000000..f90f35a913d
--- /dev/null
+++ b/lib/gitlab/background_migration/migrate_stage_index.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+
+module Gitlab
+  module BackgroundMigration
+    class MigrateStageIndex
+      def perform(start_id, stop_id)
+        migrate_stage_index_sql(start_id.to_i, stop_id.to_i).tap do |sql|
+          ActiveRecord::Base.connection.execute(sql)
+        end
+      end
+
+      private
+
+      def migrate_stage_index_sql(start_id, stop_id)
+        if Gitlab::Database.postgresql?
+          <<~SQL
+            WITH freqs AS (
+              SELECT stage_id, stage_idx, COUNT(*) AS freq FROM ci_builds
+                WHERE stage_id BETWEEN #{start_id} AND #{stop_id}
+                  AND stage_idx IS NOT NULL
+                GROUP BY stage_id, stage_idx
+            ), indexes AS (
+              SELECT DISTINCT stage_id, last_value(stage_idx)
+                OVER (PARTITION BY stage_id ORDER BY freq ASC) AS index
+                FROM freqs
+            )
+
+            UPDATE ci_stages SET position = indexes.index
+              FROM indexes WHERE indexes.stage_id = ci_stages.id
+                AND ci_stages.position IS NULL;
+          SQL
+        else
+          <<~SQL
+            UPDATE ci_stages
+              SET position =
+                (SELECT stage_idx FROM ci_builds
+                  WHERE ci_builds.stage_id = ci_stages.id
+                  GROUP BY ci_builds.stage_idx ORDER BY COUNT(*) DESC LIMIT 1)
+            WHERE ci_stages.id BETWEEN #{start_id} AND #{stop_id}
+              AND ci_stages.position IS NULL
+          SQL
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/pipeline/seed/stage.rb b/lib/gitlab/ci/pipeline/seed/stage.rb
index c101f30d6e8..2b58d9863a0 100644
--- a/lib/gitlab/ci/pipeline/seed/stage.rb
+++ b/lib/gitlab/ci/pipeline/seed/stage.rb
@@ -19,6 +19,7 @@ module Gitlab
 
           def attributes
             { name: @attributes.fetch(:name),
+              position: @attributes.fetch(:index),
               pipeline: @pipeline,
               project: @pipeline.project }
           end
diff --git a/spec/factories/ci/stages.rb b/spec/factories/ci/stages.rb
index 25309033571..ce61e6bf759 100644
--- a/spec/factories/ci/stages.rb
+++ b/spec/factories/ci/stages.rb
@@ -21,6 +21,7 @@ FactoryBot.define do
     pipeline factory: :ci_empty_pipeline
 
     name 'test'
+    position 1
     status 'pending'
   end
 end
diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb
index ce5fbc343ee..53368c64e10 100644
--- a/spec/factories/commit_statuses.rb
+++ b/spec/factories/commit_statuses.rb
@@ -2,6 +2,7 @@ FactoryBot.define do
   factory :commit_status, class: CommitStatus do
     name 'default'
     stage 'test'
+    stage_idx 0
     status 'success'
     description 'commit status'
     pipeline factory: :ci_pipeline_with_one_job
diff --git a/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb b/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb
new file mode 100644
index 00000000000..f8107dd40b9
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/migrate_stage_index_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::MigrateStageIndex, :migration, schema: 20180420080616 do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:pipelines) { table(:ci_pipelines) }
+  let(:stages) { table(:ci_stages) }
+  let(:jobs) { table(:ci_builds) }
+
+  before do
+    namespaces.create(id: 10, name: 'gitlab-org', path: 'gitlab-org')
+    projects.create!(id: 11, namespace_id: 10, name: 'gitlab', path: 'gitlab')
+    pipelines.create!(id: 12, project_id: 11, ref: 'master', sha: 'adf43c3a')
+
+    stages.create(id: 100, project_id: 11, pipeline_id: 12, name: 'build')
+    stages.create(id: 101, project_id: 11, pipeline_id: 12, name: 'test')
+
+    jobs.create!(id: 121, commit_id: 12, project_id: 11,
+                 stage_idx: 2, stage_id: 100)
+    jobs.create!(id: 122, commit_id: 12, project_id: 11,
+                 stage_idx: 2, stage_id: 100)
+    jobs.create!(id: 123, commit_id: 12, project_id: 11,
+                 stage_idx: 10, stage_id: 100)
+    jobs.create!(id: 124, commit_id: 12, project_id: 11,
+                 stage_idx: 3, stage_id: 101)
+  end
+
+  it 'correctly migrates stages indices' do
+    expect(stages.all.pluck(:position)).to all(be_nil)
+
+    described_class.new.perform(100, 101)
+
+    expect(stages.all.pluck(:position)).to eq [2, 3]
+  end
+end
diff --git a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
index dc12ba076bc..0edc3f315bb 100644
--- a/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/chain/create_spec.rb
@@ -17,7 +17,7 @@ describe Gitlab::Ci::Pipeline::Chain::Create do
 
   context 'when pipeline is ready to be saved' do
     before do
-      pipeline.stages.build(name: 'test', project: project)
+      pipeline.stages.build(name: 'test', position: 0, project: project)
 
       step.perform!
     end
diff --git a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb
index eb1b285c7bd..05ce3412fd8 100644
--- a/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/seed/stage_spec.rb
@@ -24,7 +24,8 @@ describe Gitlab::Ci::Pipeline::Seed::Stage do
   describe '#attributes' do
     it 'returns hash attributes of a stage' do
       expect(subject.attributes).to be_a Hash
-      expect(subject.attributes).to include(:name, :project)
+      expect(subject.attributes)
+        .to include(:name, :position, :pipeline, :project)
     end
   end
 
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index 31141807cb2..62da967cf96 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -232,6 +232,7 @@ Ci::Stage:
 - id
 - name
 - status
+- position
 - lock_version
 - project_id
 - pipeline_id
diff --git a/spec/migrations/schedule_stages_index_migration_spec.rb b/spec/migrations/schedule_stages_index_migration_spec.rb
new file mode 100644
index 00000000000..710264da375
--- /dev/null
+++ b/spec/migrations/schedule_stages_index_migration_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20180420080616_schedule_stages_index_migration')
+
+describe ScheduleStagesIndexMigration, :sidekiq, :migration do
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:pipelines) { table(:ci_pipelines) }
+  let(:stages) { table(:ci_stages) }
+
+  before do
+    stub_const("#{described_class}::BATCH_SIZE", 1)
+
+    namespaces.create(id: 12, name: 'gitlab-org', path: 'gitlab-org')
+    projects.create!(id: 123, namespace_id: 12, name: 'gitlab', path: 'gitlab')
+    pipelines.create!(id: 1, project_id: 123, ref: 'master', sha: 'adf43c3a')
+    stages.create!(id: 121, project_id: 123, pipeline_id: 1, name: 'build')
+    stages.create!(id: 122, project_id: 123, pipeline_id: 1, name: 'test')
+    stages.create!(id: 123, project_id: 123, pipeline_id: 1, name: 'deploy')
+  end
+
+  it 'schedules delayed background migrations in batches' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        expect(stages.all).to all(have_attributes(position: be_nil))
+
+        migrate!
+
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(5.minutes, 121, 121)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(10.minutes, 122, 122)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(15.minutes, 123, 123)
+        expect(BackgroundMigrationWorker.jobs.size).to eq 3
+      end
+    end
+  end
+end
diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb
index 586d073eb5e..a00db1d2bfc 100644
--- a/spec/models/ci/stage_spec.rb
+++ b/spec/models/ci/stage_spec.rb
@@ -51,7 +51,7 @@ describe Ci::Stage, :models do
     end
   end
 
-  describe 'update_status' do
+  describe '#update_status' do
     context 'when stage objects needs to be updated' do
       before do
         create(:ci_build, :success, stage_id: stage.id)
@@ -87,4 +87,36 @@ describe Ci::Stage, :models do
       end
     end
   end
+
+  describe '#index' do
+    context 'when stage has been imported and does not have position index set' do
+      before do
+        stage.update_column(:position, nil)
+      end
+
+      context 'when stage has statuses' do
+        before do
+          create(:ci_build, :running, stage_id: stage.id, stage_idx: 10)
+        end
+
+        it 'recalculates index before updating status' do
+          expect(stage.reload.position).to be_nil
+
+          stage.update_status
+
+          expect(stage.reload.position).to eq 10
+        end
+      end
+
+      context 'when stage does not have statuses' do
+        it 'fallbacks to zero' do
+          expect(stage.reload.position).to be_nil
+
+          stage.update_status
+
+          expect(stage.reload.position).to eq 0
+        end
+      end
+    end
+  end
 end
diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb
index 8de0bdf92e2..5bc6031388e 100644
--- a/spec/services/ci/retry_build_service_spec.rb
+++ b/spec/services/ci/retry_build_service_spec.rb
@@ -6,7 +6,9 @@ describe Ci::RetryBuildService do
   set(:pipeline) { create(:ci_pipeline, project: project) }
 
   let(:stage) do
-    Ci::Stage.create!(project: project, pipeline: pipeline, name: 'test')
+    create(:ci_stage_entity, project: project,
+                             pipeline: pipeline,
+                             name: 'test')
   end
 
   let(:build) { create(:ci_build, pipeline: pipeline, stage_id: stage.id) }