Commit f2a4420d authored by Kamil Trzcinski's avatar Kamil Trzcinski

Store retried in database for CI builds

parent 6ad3814e
...@@ -124,8 +124,8 @@ module Ci ...@@ -124,8 +124,8 @@ module Ci
success? || failed? || canceled? success? || failed? || canceled?
end end
def retried? def latest?
!self.pipeline.statuses.latest.include?(self) !retried?
end end
def expanded_environment_name def expanded_environment_name
......
...@@ -18,13 +18,7 @@ class CommitStatus < ActiveRecord::Base ...@@ -18,13 +18,7 @@ class CommitStatus < ActiveRecord::Base
validates :name, presence: true validates :name, presence: true
alias_attribute :author, :user 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 scope :failed_but_allowed, -> do
where(allow_failure: true, status: [:failed, :canceled]) where(allow_failure: true, status: [:failed, :canceled])
end end
...@@ -37,7 +31,8 @@ class CommitStatus < ActiveRecord::Base ...@@ -37,7 +31,8 @@ class CommitStatus < ActiveRecord::Base
false, all_state_names - [:failed, :canceled, :manual]) false, all_state_names - [:failed, :canceled, :manual])
end end
scope :retried, -> { where.not(id: latest) } scope :latest, -> { where(retried: false) }
scope :retried, -> { where(retried: true) }
scope :ordered, -> { order(:name) } scope :ordered, -> { order(:name) }
scope :latest_ordered, -> { latest.ordered.includes(project: :namespace) } scope :latest_ordered, -> { latest.ordered.includes(project: :namespace) }
scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) }
......
...@@ -5,6 +5,8 @@ module Ci ...@@ -5,6 +5,8 @@ module Ci
def execute(pipeline) def execute(pipeline)
@pipeline = pipeline @pipeline = pipeline
update_retried
new_builds = new_builds =
stage_indexes_of_created_builds.map do |index| stage_indexes_of_created_builds.map do |index|
process_stage(index) process_stage(index)
...@@ -71,5 +73,23 @@ module Ci ...@@ -71,5 +73,23 @@ module Ci
def created_builds def created_builds
pipeline.builds.created pipeline.builds.created
end 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
end end
...@@ -6,7 +6,7 @@ module Ci ...@@ -6,7 +6,7 @@ module Ci
description tag_list].freeze description tag_list].freeze
def execute(build) 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) build.pipeline.mark_as_processable_after_stage(build.stage_idx)
new_build.enqueue! new_build.enqueue!
...@@ -17,7 +17,7 @@ module Ci ...@@ -17,7 +17,7 @@ module Ci
end end
end end
def reprocess(build) def reprocess!(build)
unless can?(current_user, :update_build, build) unless can?(current_user, :update_build, build)
raise Gitlab::Access::AccessDeniedError raise Gitlab::Access::AccessDeniedError
end end
...@@ -28,7 +28,14 @@ module Ci ...@@ -28,7 +28,14 @@ module Ci
attributes.push([:user, current_user]) 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 end
end end
...@@ -11,7 +11,7 @@ module Ci ...@@ -11,7 +11,7 @@ module Ci
next unless can?(current_user, :update_build, build) next unless can?(current_user, :update_build, build)
Ci::RetryBuildService.new(project, current_user) Ci::RetryBuildService.new(project, current_user)
.reprocess(build) .reprocess!(build)
end end
pipeline.builds.latest.skipped.find_each do |skipped| pipeline.builds.latest.skipped.find_each do |skipped|
......
---
title: Store retried in database for CI Builds
merge_request:
author:
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
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
...@@ -232,6 +232,7 @@ ActiveRecord::Schema.define(version: 20170504102911) do ...@@ -232,6 +232,7 @@ ActiveRecord::Schema.define(version: 20170504102911) do
t.integer "lock_version" t.integer "lock_version"
t.string "coverage_regex" t.string "coverage_regex"
t.integer "auto_canceled_by_id" t.integer "auto_canceled_by_id"
t.boolean "retried", default: false, null: false
end 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 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
......
...@@ -229,6 +229,7 @@ CommitStatus: ...@@ -229,6 +229,7 @@ CommitStatus:
- lock_version - lock_version
- coverage_regex - coverage_regex
- auto_canceled_by_id - auto_canceled_by_id
- retried
Ci::Variable: Ci::Variable:
- id - id
- project_id - project_id
......
...@@ -59,8 +59,8 @@ describe Ci::Pipeline, models: true do ...@@ -59,8 +59,8 @@ describe Ci::Pipeline, models: true do
subject { pipeline.retried } subject { pipeline.retried }
before do before do
@build1 = FactoryGirl.create :ci_build, pipeline: pipeline, name: 'deploy' @build1 = create(:ci_build, pipeline: pipeline, name: 'deploy', retried: true)
@build2 = FactoryGirl.create :ci_build, pipeline: pipeline, name: 'deploy' @build2 = create(:ci_build, pipeline: pipeline, name: 'deploy')
end end
it 'returns old builds' do it 'returns old builds' do
...@@ -69,31 +69,31 @@ describe Ci::Pipeline, models: true do ...@@ -69,31 +69,31 @@ describe Ci::Pipeline, models: true do
end end
describe "coverage" do describe "coverage" do
let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" } let(:project) { create(:empty_project, build_coverage_regex: "/.*/") }
let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project } let(:pipeline) { create(:ci_empty_pipeline, project: project) }
it "calculates average when there are two builds with coverage" do it "calculates average when there are two builds with coverage" do
FactoryGirl.create :ci_build, name: "rspec", coverage: 30, pipeline: pipeline create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
expect(pipeline.coverage).to eq("35.00") expect(pipeline.coverage).to eq("35.00")
end end
it "calculates average when there are two builds with coverage and one with nil" do 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 create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
FactoryGirl.create :ci_build, pipeline: pipeline create(:ci_build, pipeline: pipeline)
expect(pipeline.coverage).to eq("35.00") expect(pipeline.coverage).to eq("35.00")
end end
it "calculates average when there are two builds with coverage and one is retried" do 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 create(:ci_build, name: "rspec", coverage: 30, pipeline: pipeline)
FactoryGirl.create :ci_build, name: "rubocop", coverage: 30, pipeline: pipeline create(:ci_build, name: "rubocop", coverage: 30, pipeline: pipeline, retried: true)
FactoryGirl.create :ci_build, name: "rubocop", coverage: 40, pipeline: pipeline create(:ci_build, name: "rubocop", coverage: 40, pipeline: pipeline)
expect(pipeline.coverage).to eq("35.00") expect(pipeline.coverage).to eq("35.00")
end end
it "calculates average when there is one build without coverage" do 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 expect(pipeline.coverage).to be_nil
end end
end end
...@@ -221,13 +221,15 @@ describe Ci::Pipeline, models: true do ...@@ -221,13 +221,15 @@ describe Ci::Pipeline, models: true do
%w(deploy running)]) %w(deploy running)])
end end
context 'when commit status is retried' do context 'when commit status is retried' do
before do before do
create(:commit_status, pipeline: pipeline, create(:commit_status, pipeline: pipeline,
stage: 'build', stage: 'build',
name: 'mac', name: 'mac',
stage_idx: 0, stage_idx: 0,
status: 'success') status: 'success')
pipeline.process!
end end
it 'ignores the previous state' do it 'ignores the previous state' do
...@@ -488,6 +490,10 @@ describe Ci::Pipeline, models: true do ...@@ -488,6 +490,10 @@ describe Ci::Pipeline, models: true do
context 'there are multiple of the same name' do context 'there are multiple of the same name' do
let!(:manual2) { create(:ci_build, :manual, pipeline: pipeline, name: 'deploy') } let!(:manual2) { create(:ci_build, :manual, pipeline: pipeline, name: 'deploy') }
before do
manual.update(retried: true)
end
it 'returns latest one' do it 'returns latest one' do
is_expected.to contain_exactly(manual2) is_expected.to contain_exactly(manual2)
end end
......
...@@ -102,6 +102,10 @@ describe Ci::Stage, models: true do ...@@ -102,6 +102,10 @@ describe Ci::Stage, models: true do
context 'and builds are retried' do context 'and builds are retried' do
let!(:new_build) { create_job(:ci_build, status: :success) } let!(:new_build) { create_job(:ci_build, status: :success) }
before do
stage_build.update(retried: true)
end
it "returns status of latest build" do it "returns status of latest build" do
is_expected.to eq('success') is_expected.to eq('success')
end end
......
...@@ -157,9 +157,9 @@ describe CommitStatus, :models do ...@@ -157,9 +157,9 @@ describe CommitStatus, :models do
subject { described_class.latest.order(:id) } subject { described_class.latest.order(:id) }
let(:statuses) do let(:statuses) do
[create_status(name: 'aa', ref: 'bb', status: 'running'), [create_status(name: 'aa', ref: 'bb', status: 'running', retried: true),
create_status(name: 'cc', ref: 'cc', status: 'pending'), create_status(name: 'cc', ref: 'cc', status: 'pending', retried: true),
create_status(name: 'aa', ref: 'cc', status: 'success'), create_status(name: 'aa', ref: 'cc', status: 'success', retried: true),
create_status(name: 'cc', ref: 'bb', status: 'success'), create_status(name: 'cc', ref: 'bb', status: 'success'),
create_status(name: 'aa', ref: 'bb', status: 'success')] create_status(name: 'aa', ref: 'bb', status: 'success')]
end end
...@@ -169,6 +169,22 @@ describe CommitStatus, :models do ...@@ -169,6 +169,22 @@ describe CommitStatus, :models do
end end
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 describe '.running_or_pending' do
subject { described_class.running_or_pending.order(:id) } subject { described_class.running_or_pending.order(:id) }
...@@ -181,7 +197,7 @@ describe CommitStatus, :models do ...@@ -181,7 +197,7 @@ describe CommitStatus, :models do
end end
it 'returns statuses that are running or pending' do 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
end end
......
...@@ -26,8 +26,8 @@ describe API::CommitStatuses do ...@@ -26,8 +26,8 @@ describe API::CommitStatuses do
create(:commit_status, { pipeline: commit, ref: commit.ref }.merge(opts)) create(:commit_status, { pipeline: commit, ref: commit.ref }.merge(opts))
end end
let!(:status1) { create_status(master, status: 'running') } let!(:status1) { create_status(master, status: 'running', retried: true) }
let!(:status2) { create_status(master, name: 'coverage', status: 'pending') } let!(:status2) { create_status(master, name: 'coverage', status: 'pending', retried: true) }
let!(:status3) { create_status(develop, status: 'running', allow_failure: true) } let!(:status3) { create_status(develop, status: 'running', allow_failure: true) }
let!(:status4) { create_status(master, name: 'coverage', status: 'success') } let!(:status4) { create_status(master, name: 'coverage', status: 'success') }
let!(:status5) { create_status(develop, name: 'coverage', status: 'success') } let!(:status5) { create_status(develop, name: 'coverage', status: 'success') }
......
...@@ -425,6 +425,21 @@ describe Ci::ProcessPipelineService, '#execute', :services do ...@@ -425,6 +425,21 @@ describe Ci::ProcessPipelineService, '#execute', :services do
end end
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 def process_pipeline
described_class.new(pipeline.project, user).execute(pipeline) described_class.new(pipeline.project, user).execute(pipeline)
end end
......
...@@ -22,7 +22,7 @@ describe Ci::RetryBuildService, :services do ...@@ -22,7 +22,7 @@ describe Ci::RetryBuildService, :services do
%i[type lock_version target_url base_tags %i[type lock_version target_url base_tags
commit_id deployments erased_by_id last_deployment project_id commit_id deployments erased_by_id last_deployment project_id
runner_id tag_taggings taggings tags trigger_request_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 shared_examples 'build duplication' do
let(:build) do let(:build) do
...@@ -115,7 +115,7 @@ describe Ci::RetryBuildService, :services do ...@@ -115,7 +115,7 @@ describe Ci::RetryBuildService, :services do
end end
describe '#reprocess' do describe '#reprocess' do
let(:new_build) { service.reprocess(build) } let(:new_build) { service.reprocess!(build) }
context 'when user has ability to execute build' do context 'when user has ability to execute build' do
before do before do
...@@ -131,11 +131,16 @@ describe Ci::RetryBuildService, :services do ...@@ -131,11 +131,16 @@ describe Ci::RetryBuildService, :services do
it 'does not enqueue the new build' do it 'does not enqueue the new build' do
expect(new_build).to be_created expect(new_build).to be_created
end end
it 'does mark old build as retried' do
expect(new_build).to be_latest
expect(build.reload).to be_retried
end
end end
context 'when user does not have ability to execute build' do context 'when user does not have ability to execute build' do
it 'raises an error' do it 'raises an error' do
expect { service.reprocess(build) } expect { service.reprocess!(build) }
.to raise_error Gitlab::Access::AccessDeniedError .to raise_error Gitlab::Access::AccessDeniedError
end end
end end
......
...@@ -13,7 +13,7 @@ describe Ci::RetryPipelineService, '#execute', :services do ...@@ -13,7 +13,7 @@ describe Ci::RetryPipelineService, '#execute', :services do
context 'when there are already retried jobs present' do context 'when there are already retried jobs present' do
before do before do
create_build('rspec', :canceled, 0) create_build('rspec', :canceled, 0, retried: true)
create_build('rspec', :failed, 0) create_build('rspec', :failed, 0)
end end
......
...@@ -39,9 +39,8 @@ describe 'projects/pipelines/_stage', :view do ...@@ -39,9 +39,8 @@ describe 'projects/pipelines/_stage', :view do
context 'when there are retried builds present' do context 'when there are retried builds present' do
before do before do
create_list(:ci_build, 2, name: 'test:build', create(:ci_build, name: 'test:build', stage: stage.name, pipeline: pipeline, retried: true)
stage: stage.name, create(:ci_build, name: 'test:build', stage: stage.name, pipeline: pipeline)
pipeline: pipeline)
end end
it 'shows only latest builds' do it 'shows only latest builds' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment