Commit 283b2c0b authored by Rémy Coutable's avatar Rémy Coutable

Merge branch 'improve-pipeline-processing' into 'master'

Improve pipeline processing

## What does this MR do?
This works on top of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295 trying to solve some edge cases introduced by that Merge Request. The fix switches to a state machine which is already a part of `Ci::Pipeline` and uses events with conditional transitions to switch between pipeline states.

This is approach is much more bullet proof and much easier to understand than a previous one where we were calling a `reload_status!` which manually updated `status`. Previous approach become confusing and prone to number of errors.

## Why was this MR needed?
This improves changes introduced by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295

## What are the relevant issue numbers?
None, yet.

## Screenshots (if relevant)
Not needed.

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added (not needed since changelog for https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5295 is already introduced)
- [ ] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added (not needed)
- Tests
  - [x] Added for this feature/bug (most of tests do cover the triggering of Pipeline)
  - [ ] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5782
parents b2828d41 7cfc4743
...@@ -42,24 +42,25 @@ module Ci ...@@ -42,24 +42,25 @@ module Ci
end end
def retry(build, user = nil) def retry(build, user = nil)
new_build = Ci::Build.new(status: 'pending') new_build = Ci::Build.create(
new_build.ref = build.ref ref: build.ref,
new_build.tag = build.tag tag: build.tag,
new_build.options = build.options options: build.options,
new_build.commands = build.commands commands: build.commands,
new_build.tag_list = build.tag_list tag_list: build.tag_list,
new_build.project = build.project project: build.project,
new_build.pipeline = build.pipeline pipeline: build.pipeline,
new_build.name = build.name name: build.name,
new_build.allow_failure = build.allow_failure allow_failure: build.allow_failure,
new_build.stage = build.stage stage: build.stage,
new_build.stage_idx = build.stage_idx stage_idx: build.stage_idx,
new_build.trigger_request = build.trigger_request trigger_request: build.trigger_request,
new_build.yaml_variables = build.yaml_variables yaml_variables: build.yaml_variables,
new_build.when = build.when when: build.when,
new_build.user = user user: user,
new_build.environment = build.environment environment: build.environment,
new_build.save status_event: 'enqueue'
)
MergeRequests::AddTodoWhenBuildFailsService.new(build.project, nil).close(new_build) MergeRequests::AddTodoWhenBuildFailsService.new(build.project, nil).close(new_build)
new_build new_build
end end
...@@ -101,7 +102,7 @@ module Ci ...@@ -101,7 +102,7 @@ module Ci
def play(current_user = nil) def play(current_user = nil)
# Try to queue a current build # Try to queue a current build
if self.queue if self.enqueue
self.update(user: current_user) self.update(user: current_user)
self self
else else
......
...@@ -19,6 +19,45 @@ module Ci ...@@ -19,6 +19,45 @@ module Ci
after_save :keep_around_commits after_save :keep_around_commits
state_machine :status, initial: :created do
event :enqueue do
transition created: :pending
transition [:success, :failed, :canceled, :skipped] => :running
end
event :run do
transition any => :running
end
event :skip do
transition any => :skipped
end
event :drop do
transition any => :failed
end
event :succeed do
transition any => :success
end
event :cancel do
transition any => :canceled
end
before_transition [:created, :pending] => :running do |pipeline|
pipeline.started_at = Time.now
end
before_transition any => [:success, :failed, :canceled] do |pipeline|
pipeline.finished_at = Time.now
end
before_transition do |pipeline|
pipeline.update_duration
end
end
# ref can't be HEAD or SHA, can only be branch/tag name # ref can't be HEAD or SHA, can only be branch/tag name
scope :latest_successful_for, ->(ref = default_branch) do scope :latest_successful_for, ->(ref = default_branch) do
where(ref: ref).success.order(id: :desc).limit(1) where(ref: ref).success.order(id: :desc).limit(1)
...@@ -89,16 +128,12 @@ module Ci ...@@ -89,16 +128,12 @@ module Ci
def cancel_running def cancel_running
builds.running_or_pending.each(&:cancel) builds.running_or_pending.each(&:cancel)
reload_status!
end end
def retry_failed(user) def retry_failed(user)
builds.latest.failed.select(&:retryable?).each do |build| builds.latest.failed.select(&:retryable?).each do |build|
Ci::Build.retry(build, user) Ci::Build.retry(build, user)
end end
reload_status!
end end
def latest? def latest?
...@@ -185,7 +220,17 @@ module Ci ...@@ -185,7 +220,17 @@ module Ci
def process! def process!
Ci::ProcessPipelineService.new(project, user).execute(self) Ci::ProcessPipelineService.new(project, user).execute(self)
reload_status! end
def build_updated
case latest_builds_status
when 'pending' then enqueue
when 'running' then run
when 'success' then succeed
when 'failed' then drop
when 'canceled' then cancel
when 'skipped' then skip
end
end end
def predefined_variables def predefined_variables
...@@ -194,22 +239,18 @@ module Ci ...@@ -194,22 +239,18 @@ module Ci
] ]
end end
def reload_status! def update_duration
statuses.reload
self.status =
if yaml_errors.blank?
statuses.latest.status || 'skipped'
else
'failed'
end
self.started_at = statuses.started_at
self.finished_at = statuses.finished_at
self.duration = statuses.latest.duration self.duration = statuses.latest.duration
save
end end
private private
def latest_builds_status
return 'failed' unless yaml_errors.blank?
statuses.latest.status || 'skipped'
end
def keep_around_commits def keep_around_commits
return unless project return unless project
......
...@@ -26,7 +26,7 @@ class CommitStatus < ActiveRecord::Base ...@@ -26,7 +26,7 @@ class CommitStatus < ActiveRecord::Base
scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) } scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) }
state_machine :status do state_machine :status do
event :queue do event :enqueue do
transition [:created, :skipped] => :pending transition [:created, :skipped] => :pending
end end
...@@ -62,6 +62,17 @@ class CommitStatus < ActiveRecord::Base ...@@ -62,6 +62,17 @@ class CommitStatus < ActiveRecord::Base
commit_status.update_attributes finished_at: Time.now commit_status.update_attributes finished_at: Time.now
end end
# We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
around_transition any => [:success, :failed, :canceled] do |commit_status, block|
block.call
commit_status.pipeline.try(:process!)
end
after_transition do |commit_status, transition|
commit_status.pipeline.try(:build_updated) unless transition.loopback?
end
after_transition [:created, :pending, :running] => :success do |commit_status| after_transition [:created, :pending, :running] => :success do |commit_status|
MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status) MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
end end
...@@ -69,13 +80,6 @@ class CommitStatus < ActiveRecord::Base ...@@ -69,13 +80,6 @@ class CommitStatus < ActiveRecord::Base
after_transition any => :failed do |commit_status| after_transition any => :failed do |commit_status|
MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status) MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status)
end end
# We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
around_transition any => [:success, :failed, :canceled] do |commit_status, block|
block.call
commit_status.pipeline.process! if commit_status.pipeline
end
end end
delegate :sha, :short_sha, to: :pipeline delegate :sha, :short_sha, to: :pipeline
......
...@@ -37,7 +37,8 @@ module Ci ...@@ -37,7 +37,8 @@ module Ci
end end
if !ignore_skip_ci && skip_ci? if !ignore_skip_ci && skip_ci?
return error('Creation of pipeline is skipped', save: save_on_errors) pipeline.skip if save_on_errors
return pipeline
end end
unless pipeline.config_builds_attributes.present? unless pipeline.config_builds_attributes.present?
...@@ -93,7 +94,7 @@ module Ci ...@@ -93,7 +94,7 @@ module Ci
def error(message, save: false) def error(message, save: false)
pipeline.errors.add(:base, message) pipeline.errors.add(:base, message)
pipeline.reload_status! if save pipeline.drop if save
pipeline pipeline
end end
end end
......
...@@ -37,7 +37,7 @@ module Ci ...@@ -37,7 +37,7 @@ module Ci
return false unless Statuseable::COMPLETED_STATUSES.include?(current_status) return false unless Statuseable::COMPLETED_STATUSES.include?(current_status)
if valid_statuses_for_when(build.when).include?(current_status) if valid_statuses_for_when(build.when).include?(current_status)
build.queue build.enqueue
true true
else else
build.skip build.skip
......
...@@ -12,7 +12,6 @@ module SharedBuilds ...@@ -12,7 +12,6 @@ module SharedBuilds
step 'project has a recent build' do step 'project has a recent build' do
@pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master') @pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master')
@build = create(:ci_build_with_coverage, pipeline: @pipeline) @build = create(:ci_build_with_coverage, pipeline: @pipeline)
@pipeline.reload_status!
end end
step 'recent build is successful' do step 'recent build is successful' do
...@@ -24,8 +23,7 @@ module SharedBuilds ...@@ -24,8 +23,7 @@ module SharedBuilds
end end
step 'project has another build that is running' do step 'project has another build that is running' do
create(:ci_build, pipeline: @pipeline, name: 'second build', status: 'running') create(:ci_build, pipeline: @pipeline, name: 'second build', status_event: 'run')
@pipeline.reload_status!
end end
step 'I visit recent build details page' do step 'I visit recent build details page' do
......
...@@ -12,7 +12,7 @@ describe "Pipelines" do ...@@ -12,7 +12,7 @@ describe "Pipelines" do
end end
describe 'GET /:project/pipelines' do describe 'GET /:project/pipelines' do
let!(:pipeline) { create(:ci_pipeline, project: project, ref: 'master', status: 'running') } let!(:pipeline) { create(:ci_empty_pipeline, project: project, ref: 'master', status: 'running') }
[:all, :running, :branches].each do |scope| [:all, :running, :branches].each do |scope|
context "displaying #{scope}" do context "displaying #{scope}" do
...@@ -31,10 +31,10 @@ describe "Pipelines" do ...@@ -31,10 +31,10 @@ describe "Pipelines" do
end end
context 'cancelable pipeline' do context 'cancelable pipeline' do
let!(:running) { create(:ci_build, :running, pipeline: pipeline, stage: 'test', commands: 'test') } let!(:build) { create(:ci_build, pipeline: pipeline, stage: 'test', commands: 'test') }
before do before do
pipeline.reload_status! build.run
visit namespace_project_pipelines_path(project.namespace, project) visit namespace_project_pipelines_path(project.namespace, project)
end end
...@@ -50,10 +50,10 @@ describe "Pipelines" do ...@@ -50,10 +50,10 @@ describe "Pipelines" do
end end
context 'retryable pipelines' do context 'retryable pipelines' do
let!(:failed) { create(:ci_build, :failed, pipeline: pipeline, stage: 'test', commands: 'test') } let!(:build) { create(:ci_build, pipeline: pipeline, stage: 'test', commands: 'test') }
before do before do
pipeline.reload_status! build.drop
visit namespace_project_pipelines_path(project.namespace, project) visit namespace_project_pipelines_path(project.namespace, project)
end end
...@@ -64,7 +64,7 @@ describe "Pipelines" do ...@@ -64,7 +64,7 @@ describe "Pipelines" do
before { click_link('Retry') } before { click_link('Retry') }
it { expect(page).not_to have_link('Retry') } it { expect(page).not_to have_link('Retry') }
it { expect(page).to have_selector('.ci-pending') } it { expect(page).to have_selector('.ci-running') }
end end
end end
...@@ -87,7 +87,6 @@ describe "Pipelines" do ...@@ -87,7 +87,6 @@ describe "Pipelines" do
let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') } let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') }
before do before do
pipeline.reload_status!
visit namespace_project_pipelines_path(project.namespace, project) visit namespace_project_pipelines_path(project.namespace, project)
end end
...@@ -101,10 +100,10 @@ describe "Pipelines" do ...@@ -101,10 +100,10 @@ describe "Pipelines" do
end end
context 'when failed' do context 'when failed' do
let!(:failed) { create(:generic_commit_status, status: 'failed', pipeline: pipeline, stage: 'test') } let!(:status) { create(:generic_commit_status, :pending, pipeline: pipeline, stage: 'test') }
before do before do
pipeline.reload_status! status.drop
visit namespace_project_pipelines_path(project.namespace, project) visit namespace_project_pipelines_path(project.namespace, project)
end end
......
...@@ -2,22 +2,23 @@ require 'spec_helper' ...@@ -2,22 +2,23 @@ require 'spec_helper'
describe Ci::Charts, lib: true do describe Ci::Charts, lib: true do
context "build_times" do context "build_times" do
let(:project) { create(:empty_project) }
let(:chart) { Ci::Charts::BuildTime.new(project) }
subject { chart.build_times }
before do before do
@pipeline = FactoryGirl.create(:ci_pipeline) create(:ci_empty_pipeline, project: project, duration: 120)
FactoryGirl.create(:ci_build, pipeline: @pipeline)
@pipeline.reload_status!
end end
it 'returns build times in minutes' do it 'returns build times in minutes' do
chart = Ci::Charts::BuildTime.new(@pipeline.project) is_expected.to contain_exactly(2)
expect(chart.build_times).to eq([2])
end end
it 'handles nil build times' do it 'handles nil build times' do
create(:ci_pipeline, duration: nil, project: @pipeline.project) create(:ci_empty_pipeline, project: project, duration: nil)
chart = Ci::Charts::BuildTime.new(@pipeline.project) is_expected.to contain_exactly(2, 0)
expect(chart.build_times).to eq([2, 0])
end end
end end
end end
...@@ -886,8 +886,10 @@ describe Ci::Build, models: true do ...@@ -886,8 +886,10 @@ describe Ci::Build, models: true do
is_expected.to eq(build) is_expected.to eq(build)
end end
context 'for success build' do context 'for successful build' do
before { build.queue } before do
build.update(status: 'success')
end
it 'creates a new build' do it 'creates a new build' do
is_expected.to be_pending is_expected.to be_pending
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Ci::Pipeline, models: true do describe Ci::Pipeline, models: true do
let(:project) { FactoryGirl.create :empty_project } let(:project) { FactoryGirl.create :empty_project }
let(:pipeline) { FactoryGirl.create :ci_pipeline, project: project } let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:project) }
it { is_expected.to belong_to(:user) } it { is_expected.to belong_to(:user) }
...@@ -51,25 +51,6 @@ describe Ci::Pipeline, models: true do ...@@ -51,25 +51,6 @@ describe Ci::Pipeline, models: true do
end end
end end
describe "#finished_at" do
let(:pipeline) { FactoryGirl.create :ci_pipeline }
it "returns finished_at of latest build" do
build = FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 60
FactoryGirl.create :ci_build, pipeline: pipeline, finished_at: Time.now - 120
pipeline.reload_status!
expect(pipeline.finished_at.to_i).to eq(build.finished_at.to_i)
end
it "returns nil if there is no finished build" do
FactoryGirl.create :ci_not_started_build, pipeline: pipeline
pipeline.reload_status!
expect(pipeline.finished_at).to be_nil
end
end
describe "coverage" do describe "coverage" do
let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" } let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" }
let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project } let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project }
...@@ -139,33 +120,48 @@ describe Ci::Pipeline, models: true do ...@@ -139,33 +120,48 @@ describe Ci::Pipeline, models: true do
end end
end end
describe '#reload_status!' do describe 'state machine' do
let(:pipeline) { create :ci_empty_pipeline, project: project } let(:current) { Time.now.change(usec: 0) }
let(:build) { create :ci_build, name: 'build1', pipeline: pipeline, started_at: current - 60, finished_at: current }
context 'dependent objects' do let(:build2) { create :ci_build, name: 'build2', pipeline: pipeline, started_at: current - 60, finished_at: current }
let(:commit_status) { create :commit_status, :pending, pipeline: pipeline }
it 'executes reload_status! after succeeding dependent object' do describe '#duration' do
expect(pipeline).to receive(:reload_status!).and_return(true) before do
build.skip
build2.skip
end
commit_status.success it 'matches sum of builds duration' do
expect(pipeline.reload.duration).to eq(build.duration + build2.duration)
end end
end end
context 'updates' do describe '#started_at' do
let(:current) { Time.now.change(usec: 0) } it 'updates on transitioning to running' do
let(:build) { FactoryGirl.create :ci_build, pipeline: pipeline, started_at: current - 120, finished_at: current - 60 } build.run
before do expect(pipeline.reload.started_at).not_to be_nil
build
pipeline.reload_status!
end end
[:status, :started_at, :finished_at, :duration].each do |param| it 'does not update on transitioning to success' do
it "#{param}" do build.success
expect(pipeline.send(param)).to eq(build.send(param))
expect(pipeline.reload.started_at).to be_nil
end end
end end
describe '#finished_at' do
it 'updates on transitioning to success' do
build.success
expect(pipeline.reload.finished_at).not_to be_nil
end
it 'does not update on transitioning to running' do
build.run
expect(pipeline.reload.finished_at).to be_nil
end
end end
end end
...@@ -254,4 +250,64 @@ describe Ci::Pipeline, models: true do ...@@ -254,4 +250,64 @@ describe Ci::Pipeline, models: true do
end end
end end
end end
describe '#status' do
let!(:build) { create(:ci_build, :created, pipeline: pipeline, name: 'test') }
subject { pipeline.reload.status }
context 'on queuing' do
before do
build.enqueue
end
it { is_expected.to eq('pending') }
end
context 'on run' do
before do
build.enqueue
build.run
end
it { is_expected.to eq('running') }
end
context 'on drop' do
before do
build.drop
end
it { is_expected.to eq('failed') }
end
context 'on success' do
before do
build.success
end
it { is_expected.to eq('success') }
end
context 'on cancel' do
before do
build.cancel
end
it { is_expected.to eq('canceled') }
end
context 'on failure and build retry' do
before do
build.drop
Ci::Build.retry(build)
end
# We are changing a state: created > failed > running
# Instead of: created > failed > pending
# Since the pipeline already run, so it should not be pending anymore
it { is_expected.to eq('running') }
end
end
end end
...@@ -9,7 +9,7 @@ describe API::API, api: true do ...@@ -9,7 +9,7 @@ describe API::API, api: true do
let!(:developer) { create(:project_member, :developer, user: user, project: project) } let!(:developer) { create(:project_member, :developer, user: user, project: project) }
let(:reporter) { create(:project_member, :reporter, project: project) } let(:reporter) { create(:project_member, :reporter, project: project) }
let(:guest) { create(:project_member, :guest, project: project) } let(:guest) { create(:project_member, :guest, project: project) }
let!(:pipeline) { create(:ci_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) } let!(:pipeline) { create(:ci_empty_pipeline, project: project, sha: project.commit.id, ref: project.default_branch) }
let!(:build) { create(:ci_build, pipeline: pipeline) } let!(:build) { create(:ci_build, pipeline: pipeline) }
describe 'GET /projects/:id/builds ' do describe 'GET /projects/:id/builds ' do
...@@ -174,7 +174,11 @@ describe API::API, api: true do ...@@ -174,7 +174,11 @@ describe API::API, api: true do
describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do
let(:api_user) { reporter.user } let(:api_user) { reporter.user }
let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) } let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
before do
build.success
end
def path_for_ref(ref = pipeline.ref, job = build.name) def path_for_ref(ref = pipeline.ref, job = build.name)
api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user) api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user)
...@@ -238,10 +242,6 @@ describe API::API, api: true do ...@@ -238,10 +242,6 @@ describe API::API, api: true do
it { expect(response.headers).to include(download_headers) } it { expect(response.headers).to include(download_headers) }
end end
before do
pipeline.reload_status!
end
context 'with regular branch' do context 'with regular branch' do
before do before do
pipeline.update(ref: 'master', pipeline.update(ref: 'master',
......
...@@ -14,7 +14,6 @@ module Ci ...@@ -14,7 +14,6 @@ module Ci
context 'branch name' do context 'branch name' do
before { allow(project).to receive(:commit).and_return(OpenStruct.new(sha: commit_sha)) } before { allow(project).to receive(:commit).and_return(OpenStruct.new(sha: commit_sha)) }
before { build.run! } before { build.run! }
before { pipeline.reload_status! }
let(:image) { service.execute(project, ref: 'master') } let(:image) { service.execute(project, ref: 'master') }
it { expect(image).to be_kind_of(OpenStruct) } it { expect(image).to be_kind_of(OpenStruct) }
...@@ -32,7 +31,6 @@ module Ci ...@@ -32,7 +31,6 @@ module Ci
context 'commit sha' do context 'commit sha' do
before { build.run! } before { build.run! }
before { pipeline.reload_status! }
let(:image) { service.execute(project, sha: build.sha) } let(:image) { service.execute(project, sha: build.sha) }
it { expect(image).to be_kind_of(OpenStruct) } it { expect(image).to be_kind_of(OpenStruct) }
......
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