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