Store retried in database for CI buildsretried-in-database-mysql

18 files changed, 158 insertions, 41 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 971ab7cb0ee..3c4a4d93349 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -124,8 +124,8 @@ module Ci
       success? || failed? || canceled?
     end
 
-    def retried?
-      !self.pipeline.statuses.latest.include?(self)
+    def latest?
+      !retried?
     end
 
     def expanded_environment_name
diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 2c4033146bf..528bf4d87ac 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -18,13 +18,7 @@ class CommitStatus < ActiveRecord::Base
   validates :name, presence: true
 
   alias_attribute :author, :user
-
-  scope :latest, -> do
-    max_id = unscope(:select).select("max(#{quoted_table_name}.id)")
-
-    where(id: max_id.group(:name, :commit_id))
-  end
-
+  
   scope :failed_but_allowed, -> do
     where(allow_failure: true, status: [:failed, :canceled])
   end
@@ -37,7 +31,8 @@ class CommitStatus < ActiveRecord::Base
       false, all_state_names - [:failed, :canceled, :manual])
   end
 
-  scope :retried, -> { where.not(id: latest) }
+  scope :latest, -> { where(retried: false) }
+  scope :retried, -> { where(retried: true) }
   scope :ordered, -> { order(:name) }
   scope :latest_ordered, -> { latest.ordered.includes(project: :namespace) }
   scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) }
diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb
index 33edcd60944..647836de384 100644
--- a/app/services/ci/process_pipeline_service.rb
+++ b/app/services/ci/process_pipeline_service.rb
@@ -5,6 +5,8 @@ module Ci
     def execute(pipeline)
       @pipeline = pipeline
 
+      update_retried
+
       new_builds =
         stage_indexes_of_created_builds.map do |index|
           process_stage(index)
@@ -71,5 +73,23 @@ module Ci
     def created_builds
       pipeline.builds.created
     end
+
+    # This method is for compatibility and data consistency and should be removed with 9.3 version of GitLab
+    # This replicates what is db/post_migrate/20170416103934_upate_retried_for_ci_build.rb
+    # and ensures that functionality will not be broken before migration is run
+    # this updates only when there are data that needs to be updated, there are two groups with no retried flag
+    def update_retried
+      # find the latest builds for each name
+      latest_statuses = pipeline.statuses.latest
+        .group(:name)
+        .having('count(*) > 1')
+        .pluck('max(id)', 'name')
+
+      # mark builds that are retried
+      pipeline.statuses.latest
+        .where(name: latest_statuses.map(&:second))
+        .where.not(id: latest_statuses.map(&:first))
+        .update_all(retried: true) if latest_statuses.any?
+    end
   end
 end
diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb
index 89da05b72bb..9c532b585a1 100644
--- a/app/services/ci/retry_build_service.rb
+++ b/app/services/ci/retry_build_service.rb
@@ -6,7 +6,7 @@ module Ci
                          description tag_list].freeze
 
     def execute(build)
-      reprocess(build).tap do |new_build|
+      reprocess!(build).tap do |new_build|
         build.pipeline.mark_as_processable_after_stage(build.stage_idx)
 
         new_build.enqueue!
@@ -17,7 +17,7 @@ module Ci
       end
     end
 
-    def reprocess(build)
+    def reprocess!(build)
       unless can?(current_user, :update_build, build)
         raise Gitlab::Access::AccessDeniedError
       end
@@ -28,7 +28,14 @@ module Ci
 
       attributes.push([:user, current_user])
 
-      project.builds.create(Hash[attributes])
+      Ci::Build.transaction do
+        # mark all other builds of that name as retried
+        build.pipeline.builds.where(name: build.name)
+          .where.not(retried: true)
+          .update_all(retried: true)
+
+        project.builds.create!(Hash[attributes])
+      end
     end
   end
 end
diff --git a/app/services/ci/retry_pipeline_service.rb b/app/services/ci/retry_pipeline_service.rb
index 5b207157345..c5a43869990 100644
--- a/app/services/ci/retry_pipeline_service.rb
+++ b/app/services/ci/retry_pipeline_service.rb
@@ -11,7 +11,7 @@ module Ci
         next unless can?(current_user, :update_build, build)
 
         Ci::RetryBuildService.new(project, current_user)
-          .reprocess(build)
+          .reprocess!(build)
       end
 
       pipeline.builds.latest.skipped.find_each do |skipped|
diff --git a/changelogs/unreleased/store-retried-in-database-for-ci-builds.yml b/changelogs/unreleased/store-retried-in-database-for-ci-builds.yml
new file mode 100644
index 00000000000..9185113f51c
--- /dev/null
+++ b/changelogs/unreleased/store-retried-in-database-for-ci-builds.yml
@@ -0,0 +1,4 @@
+---
+title: Store retried in database for CI Builds
+merge_request:
+author:
diff --git a/db/migrate/20170503004426_add_retried_to_ci_build.rb b/db/migrate/20170503004426_add_retried_to_ci_build.rb
new file mode 100644
index 00000000000..9f509f97f14
--- /dev/null
+++ b/db/migrate/20170503004426_add_retried_to_ci_build.rb
@@ -0,0 +1,15 @@
+class AddRetriedToCiBuild < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default(:ci_builds, :retried, :boolean, default: false)
+  end
+
+  def down
+    remove_column(:ci_builds, :retried)
+  end
+end
diff --git a/db/post_migrate/20170503004427_upate_retried_for_ci_build.rb b/db/post_migrate/20170503004427_upate_retried_for_ci_build.rb
new file mode 100644
index 00000000000..e12180930a7
--- /dev/null
+++ b/db/post_migrate/20170503004427_upate_retried_for_ci_build.rb
@@ -0,0 +1,29 @@
+class UpateRetriedForCiBuild < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  
+  disable_ddl_transaction!
+
+  def up
+    disable_statement_timeout
+
+    latest_id = <<-SQL.strip_heredoc
+      SELECT MAX(ci_builds2.id)
+        FROM ci_builds ci_builds2
+        WHERE ci_builds.commit_id=ci_builds2.commit_id
+          AND ci_builds.name=ci_builds2.name
+    SQL
+    
+    # This is slow update as it does single-row query
+    # This is designed to be run as idle, or a post deployment migration
+    is_retried = Arel.sql("((#{latest_id}) != ci_builds.id)")
+
+    update_column_in_batches(:ci_builds, :retried, is_retried) do |table, query|
+      query.where(table[:retried].eq(false))
+    end
+  end
+
+  def down
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 61af066ed3b..8046e9b48a7 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -232,6 +232,7 @@ ActiveRecord::Schema.define(version: 20170504102911) do
     t.integer "lock_version"
     t.string "coverage_regex"
     t.integer "auto_canceled_by_id"
+    t.boolean "retried", default: false, null: false
   end
 
   add_index "ci_builds", ["commit_id", "stage_idx", "created_at"], name: "index_ci_builds_on_commit_id_and_stage_idx_and_created_at", using: :btree
diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml
index a66086f8b47..56b9729a870 100644
--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -229,6 +229,7 @@ CommitStatus:
 - lock_version
 - coverage_regex
 - auto_canceled_by_id
+- retried
 Ci::Variable:
 - id
 - project_id
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 3b222ea1c3d..d79f1e22f32 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -59,8 +59,8 @@ describe Ci::Pipeline, models: true do
     subject { pipeline.retried }
 
     before do
-      @build1 = FactoryGirl.create :ci_build, pipeline: pipeline, name: 'deploy'
-      @build2 = FactoryGirl.create :ci_build, pipeline: pipeline, name: 'deploy'
+      @build1 = create(:ci_build, pipeline: pipeline, name: 'deploy', retried: true)
+      @build2 = create(:ci_build, pipeline: pipeline, name: 'deploy')
     end
 
     it 'returns old builds' do
@@ -69,31 +69,31 @@ describe Ci::Pipeline, models: true do
   end
 
   describe "coverage" do
-    let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" }
-    let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
+    let(:project) { create(:empty_project, build_coverage_regex: "/.*/") }
+    let(:pipeline) { create(:ci_empty_pipeline, project: project) }
 
     it "calculates average when there are two builds with coverage" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, pipeline: pipeline
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline
+      create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
+      create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
       expect(pipeline.coverage).to eq("35.00")
     end
 
     it "calculates average when there are two builds with coverage and one with nil" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, pipeline: pipeline
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline
-      FactoryGirl.create :ci_build, pipeline: pipeline
+      create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
+      create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
+      create(:ci_build, pipeline: pipeline)
       expect(pipeline.coverage).to eq("35.00")
     end
 
     it "calculates average when there are two builds with coverage and one is retried" do
-      FactoryGirl.create :ci_build, name: "rspec", coverage: 30, pipeline: pipeline
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 30, pipeline: pipeline
-      FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline
+      create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
+      create(:ci_build, name: "rubocop", coverage: 30, pipeline: pipeline, retried: true)
+      create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
       expect(pipeline.coverage).to eq("35.00")
     end
 
     it "calculates average when there is one build without coverage" do
-      FactoryGirl.create :ci_build, pipeline: pipeline
+      FactoryGirl.create(:ci_build, pipeline: pipeline)
       expect(pipeline.coverage).to be_nil
     end
   end
@@ -221,13 +221,15 @@ describe Ci::Pipeline, models: true do
                                   %w(deploy running)])
         end
 
-        context 'when commit status  is retried' do
+        context 'when commit status is retried' do
           before do
             create(:commit_status, pipeline: pipeline,
                                    stage: 'build',
                                    name: 'mac',
                                    stage_idx: 0,
                                    status: 'success')
+
+            pipeline.process!
           end
 
           it 'ignores the previous state' do
@@ -488,6 +490,10 @@ describe Ci::Pipeline, models: true do
       context 'there are multiple of the same name' do
         let!(:manual2) { create(:ci_build, :manual, pipeline: pipeline, name: 'deploy') }
 
+        before do
+          manual.update(retried: true)
+        end
+
         it 'returns latest one' do
           is_expected.to contain_exactly(manual2)
         end
diff --git a/spec/models/ci/stage_spec.rb b/spec/models/ci/stage_spec.rb
index 372b662fab2..8f6ab908987 100644
--- a/spec/models/ci/stage_spec.rb
+++ b/spec/models/ci/stage_spec.rb
@@ -102,6 +102,10 @@ describe Ci::Stage, models: true do
       context 'and builds are retried' do
         let!(:new_build) { create_job(:ci_build, status: :success) }
 
+        before do
+          stage_build.update(retried: true)
+        end
+
         it "returns status of latest build" do
           is_expected.to eq('success')
         end
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index 0ee85489574..6947affcc1e 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -157,9 +157,9 @@ describe CommitStatus, :models do
     subject { described_class.latest.order(:id) }
 
     let(:statuses) do
-      [create_status(name: 'aa', ref: 'bb', status: 'running'),
-       create_status(name: 'cc', ref: 'cc', status: 'pending'),
-       create_status(name: 'aa', ref: 'cc', status: 'success'),
+      [create_status(name: 'aa', ref: 'bb', status: 'running', retried: true),
+       create_status(name: 'cc', ref: 'cc', status: 'pending', retried: true),
+       create_status(name: 'aa', ref: 'cc', status: 'success', retried: true),
        create_status(name: 'cc', ref: 'bb', status: 'success'),
        create_status(name: 'aa', ref: 'bb', status: 'success')]
     end
@@ -169,6 +169,22 @@ describe CommitStatus, :models do
     end
   end
 
+  describe '.retried' do
+    subject { described_class.retried.order(:id) }
+
+    let(:statuses) do
+      [create_status(name: 'aa', ref: 'bb', status: 'running', retried: true),
+       create_status(name: 'cc', ref: 'cc', status: 'pending', retried: true),
+       create_status(name: 'aa', ref: 'cc', status: 'success', retried: true),
+       create_status(name: 'cc', ref: 'bb', status: 'success'),
+       create_status(name: 'aa', ref: 'bb', status: 'success')]
+    end
+
+    it 'returns unique statuses' do
+      is_expected.to contain_exactly(*statuses.values_at(0, 1, 2))
+    end
+  end
+
   describe '.running_or_pending' do
     subject { described_class.running_or_pending.order(:id) }
 
@@ -181,7 +197,7 @@ describe CommitStatus, :models do
     end
 
     it 'returns statuses that are running or pending' do
-      is_expected.to eq(statuses.values_at(0, 1))
+      is_expected.to contain_exactly(*statuses.values_at(0, 1))
     end
   end
 
diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb
index 1233cdc64c4..1c163cee152 100644
--- a/spec/requests/api/commit_statuses_spec.rb
+++ b/spec/requests/api/commit_statuses_spec.rb
@@ -26,8 +26,8 @@ describe API::CommitStatuses do
           create(:commit_status, { pipeline: commit, ref: commit.ref }.merge(opts))
         end
 
-        let!(:status1) { create_status(master, status: 'running') }
-        let!(:status2) { create_status(master, name: 'coverage', status: 'pending') }
+        let!(:status1) { create_status(master, status: 'running', retried: true) }
+        let!(:status2) { create_status(master, name: 'coverage', status: 'pending', retried: true) }
         let!(:status3) { create_status(develop, status: 'running', allow_failure: true) }
         let!(:status4) { create_status(master, name: 'coverage', status: 'success') }
         let!(:status5) { create_status(develop, name: 'coverage', status: 'success') }
diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb
index cf773866a6f..f1e1e1b6067 100644
--- a/spec/services/ci/process_pipeline_service_spec.rb
+++ b/spec/services/ci/process_pipeline_service_spec.rb
@@ -425,6 +425,21 @@ describe Ci::ProcessPipelineService, '#execute', :services do
     end
   end
 
+  context 'updates a list of retried builds' do
+    subject { described_class.retried.order(:id) }
+
+    let!(:build_retried) { create_build('build') }
+    let!(:build) { create_build('build') }
+    let!(:test) { create_build('test') }
+
+    it 'returns unique statuses' do
+      process_pipeline
+
+      expect(all_builds.latest).to contain_exactly(build, test)
+      expect(all_builds.retried).to contain_exactly(build_retried)
+    end
+  end
+
   def process_pipeline
     described_class.new(pipeline.project, user).execute(pipeline)
   end
diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb
index b2d37657770..7254e6b357a 100644
--- a/spec/services/ci/retry_build_service_spec.rb
+++ b/spec/services/ci/retry_build_service_spec.rb
@@ -22,7 +22,7 @@ describe Ci::RetryBuildService, :services do
     %i[type lock_version target_url base_tags
        commit_id deployments erased_by_id last_deployment project_id
        runner_id tag_taggings taggings tags trigger_request_id
-       user_id auto_canceled_by_id].freeze
+       user_id auto_canceled_by_id retried].freeze
 
   shared_examples 'build duplication' do
     let(:build) do
@@ -115,7 +115,7 @@ describe Ci::RetryBuildService, :services do
   end
 
   describe '#reprocess' do
-    let(:new_build) { service.reprocess(build) }
+    let(:new_build) { service.reprocess!(build) }
 
     context 'when user has ability to execute build' do
       before do
@@ -131,11 +131,16 @@ describe Ci::RetryBuildService, :services do
       it 'does not enqueue the new build' do
         expect(new_build).to be_created
       end
+
+      it 'does mark old build as retried' do
+        expect(new_build).to be_latest
+        expect(build.reload).to be_retried
+      end
     end
 
     context 'when user does not have ability to execute build' do
       it 'raises an error' do
-        expect { service.reprocess(build) }
+        expect { service.reprocess!(build) }
           .to raise_error Gitlab::Access::AccessDeniedError
       end
     end
diff --git a/spec/services/ci/retry_pipeline_service_spec.rb b/spec/services/ci/retry_pipeline_service_spec.rb
index 40e151545c9..d941d56c0d8 100644
--- a/spec/services/ci/retry_pipeline_service_spec.rb
+++ b/spec/services/ci/retry_pipeline_service_spec.rb
@@ -13,7 +13,7 @@ describe Ci::RetryPipelineService, '#execute', :services do
 
     context 'when there are already retried jobs present' do
       before do
-        create_build('rspec', :canceled, 0)
+        create_build('rspec', :canceled, 0, retried: true)
         create_build('rspec', :failed, 0)
       end
 
diff --git a/spec/views/projects/pipelines/_stage.html.haml_spec.rb b/spec/views/projects/pipelines/_stage.html.haml_spec.rb
index 10095ad7694..9c91c4e0fbd 100644
--- a/spec/views/projects/pipelines/_stage.html.haml_spec.rb
+++ b/spec/views/projects/pipelines/_stage.html.haml_spec.rb
@@ -39,9 +39,8 @@ describe 'projects/pipelines/_stage', :view do
 
   context 'when there are retried builds present' do
     before do
-      create_list(:ci_build, 2, name: 'test:build',
-                                stage: stage.name,
-                                pipeline: pipeline)
+      create(:ci_build, name: 'test:build', stage: stage.name, pipeline: pipeline, retried: true)
+      create(:ci_build, name: 'test:build', stage: stage.name, pipeline: pipeline)
     end
 
     it 'shows only latest builds' do