diff options
author | Rémy Coutable <remy@rymai.me> | 2016-08-12 18:04:08 +0300 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2016-08-12 18:04:08 +0300 |
commit | 283b2c0b15bbd39136f601f595157033850dd5a8 (patch) | |
tree | d70dde807ea3af22790b670f5666da3fb1f650bd | |
parent | b2828d4145ac01468a59b821ced29dd248526089 (diff) | |
parent | 7cfc47432170be14f9449a77f893c4662634019d (diff) |
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
-rw-r--r-- | app/models/ci/build.rb | 39 | ||||
-rw-r--r-- | app/models/ci/pipeline.rb | 73 | ||||
-rw-r--r-- | app/models/commit_status.rb | 20 | ||||
-rw-r--r-- | app/services/ci/create_pipeline_service.rb | 5 | ||||
-rw-r--r-- | app/services/ci/process_pipeline_service.rb | 2 | ||||
-rw-r--r-- | features/steps/shared/builds.rb | 4 | ||||
-rw-r--r-- | spec/features/projects/pipelines_spec.rb | 17 | ||||
-rw-r--r-- | spec/lib/ci/charts_spec.rb | 17 | ||||
-rw-r--r-- | spec/models/build_spec.rb | 6 | ||||
-rw-r--r-- | spec/models/ci/pipeline_spec.rb | 130 | ||||
-rw-r--r-- | spec/requests/api/builds_spec.rb | 12 | ||||
-rw-r--r-- | spec/services/ci/image_for_build_service_spec.rb | 2 |
12 files changed, 214 insertions, 113 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 88a340379b8..3d6c6ea3209 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -42,24 +42,25 @@ module Ci end def retry(build, user = nil) - new_build = Ci::Build.new(status: 'pending') - new_build.ref = build.ref - new_build.tag = build.tag - new_build.options = build.options - new_build.commands = build.commands - new_build.tag_list = build.tag_list - new_build.project = build.project - new_build.pipeline = build.pipeline - new_build.name = build.name - new_build.allow_failure = build.allow_failure - new_build.stage = build.stage - new_build.stage_idx = build.stage_idx - new_build.trigger_request = build.trigger_request - new_build.yaml_variables = build.yaml_variables - new_build.when = build.when - new_build.user = user - new_build.environment = build.environment - new_build.save + new_build = Ci::Build.create( + ref: build.ref, + tag: build.tag, + options: build.options, + commands: build.commands, + tag_list: build.tag_list, + project: build.project, + pipeline: build.pipeline, + name: build.name, + allow_failure: build.allow_failure, + stage: build.stage, + stage_idx: build.stage_idx, + trigger_request: build.trigger_request, + yaml_variables: build.yaml_variables, + when: build.when, + user: user, + environment: build.environment, + status_event: 'enqueue' + ) MergeRequests::AddTodoWhenBuildFailsService.new(build.project, nil).close(new_build) new_build end @@ -101,7 +102,7 @@ module Ci def play(current_user = nil) # Try to queue a current build - if self.queue + if self.enqueue self.update(user: current_user) self else diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 718fe3290c1..8cfba92ae9b 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -19,6 +19,45 @@ module Ci 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 scope :latest_successful_for, ->(ref = default_branch) do where(ref: ref).success.order(id: :desc).limit(1) @@ -89,16 +128,12 @@ module Ci def cancel_running builds.running_or_pending.each(&:cancel) - - reload_status! end def retry_failed(user) builds.latest.failed.select(&:retryable?).each do |build| Ci::Build.retry(build, user) end - - reload_status! end def latest? @@ -185,7 +220,17 @@ module Ci def process! 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 def predefined_variables @@ -194,22 +239,18 @@ module Ci ] end - def reload_status! - 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 + def update_duration self.duration = statuses.latest.duration - save end private + def latest_builds_status + return 'failed' unless yaml_errors.blank? + + statuses.latest.status || 'skipped' + end + def keep_around_commits return unless project diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 20713314a25..703ca90edb6 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -26,7 +26,7 @@ class CommitStatus < ActiveRecord::Base scope :ignored, -> { where(allow_failure: true, status: [:failed, :canceled]) } state_machine :status do - event :queue do + event :enqueue do transition [:created, :skipped] => :pending end @@ -62,6 +62,17 @@ class CommitStatus < ActiveRecord::Base commit_status.update_attributes finished_at: Time.now 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| MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status) end @@ -69,13 +80,6 @@ class CommitStatus < ActiveRecord::Base after_transition any => :failed do |commit_status| MergeRequests::AddTodoWhenBuildFailsService.new(commit_status.pipeline.project, nil).execute(commit_status) 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 delegate :sha, :short_sha, to: :pipeline diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 7398fd8e10a..cde856b0186 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -37,7 +37,8 @@ module Ci end 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 unless pipeline.config_builds_attributes.present? @@ -93,7 +94,7 @@ module Ci def error(message, save: false) pipeline.errors.add(:base, message) - pipeline.reload_status! if save + pipeline.drop if save pipeline end end diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb index 86c4823d18a..6f7610d42ba 100644 --- a/app/services/ci/process_pipeline_service.rb +++ b/app/services/ci/process_pipeline_service.rb @@ -37,7 +37,7 @@ module Ci return false unless Statuseable::COMPLETED_STATUSES.include?(current_status) if valid_statuses_for_when(build.when).include?(current_status) - build.queue + build.enqueue true else build.skip diff --git a/features/steps/shared/builds.rb b/features/steps/shared/builds.rb index c7f61da05fa..70e6d4836b2 100644 --- a/features/steps/shared/builds.rb +++ b/features/steps/shared/builds.rb @@ -12,7 +12,6 @@ module SharedBuilds step 'project has a recent build' do @pipeline = create(:ci_empty_pipeline, project: @project, sha: @project.commit.sha, ref: 'master') @build = create(:ci_build_with_coverage, pipeline: @pipeline) - @pipeline.reload_status! end step 'recent build is successful' do @@ -24,8 +23,7 @@ module SharedBuilds end step 'project has another build that is running' do - create(:ci_build, pipeline: @pipeline, name: 'second build', status: 'running') - @pipeline.reload_status! + create(:ci_build, pipeline: @pipeline, name: 'second build', status_event: 'run') end step 'I visit recent build details page' do diff --git a/spec/features/projects/pipelines_spec.rb b/spec/features/projects/pipelines_spec.rb index c39bb7ffa37..29d150bc597 100644 --- a/spec/features/projects/pipelines_spec.rb +++ b/spec/features/projects/pipelines_spec.rb @@ -12,7 +12,7 @@ describe "Pipelines" do end 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| context "displaying #{scope}" do @@ -31,10 +31,10 @@ describe "Pipelines" do end 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 - pipeline.reload_status! + build.run visit namespace_project_pipelines_path(project.namespace, project) end @@ -50,10 +50,10 @@ describe "Pipelines" do end 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 - pipeline.reload_status! + build.drop visit namespace_project_pipelines_path(project.namespace, project) end @@ -64,7 +64,7 @@ describe "Pipelines" do before { click_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 @@ -87,7 +87,6 @@ describe "Pipelines" do let!(:running) { create(:generic_commit_status, status: 'running', pipeline: pipeline, stage: 'test') } before do - pipeline.reload_status! visit namespace_project_pipelines_path(project.namespace, project) end @@ -101,10 +100,10 @@ describe "Pipelines" do end 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 - pipeline.reload_status! + status.drop visit namespace_project_pipelines_path(project.namespace, project) end diff --git a/spec/lib/ci/charts_spec.rb b/spec/lib/ci/charts_spec.rb index 2cd6b00dad6..fb6cc398307 100644 --- a/spec/lib/ci/charts_spec.rb +++ b/spec/lib/ci/charts_spec.rb @@ -2,22 +2,23 @@ require 'spec_helper' describe Ci::Charts, lib: true do context "build_times" do + let(:project) { create(:empty_project) } + let(:chart) { Ci::Charts::BuildTime.new(project) } + + subject { chart.build_times } + before do - @pipeline = FactoryGirl.create(:ci_pipeline) - FactoryGirl.create(:ci_build, pipeline: @pipeline) - @pipeline.reload_status! + create(:ci_empty_pipeline, project: project, duration: 120) end it 'returns build times in minutes' do - chart = Ci::Charts::BuildTime.new(@pipeline.project) - expect(chart.build_times).to eq([2]) + is_expected.to contain_exactly(2) end 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) - expect(chart.build_times).to eq([2, 0]) + is_expected.to contain_exactly(2, 0) end end end diff --git a/spec/models/build_spec.rb b/spec/models/build_spec.rb index 60a221eba50..5980f6ddc32 100644 --- a/spec/models/build_spec.rb +++ b/spec/models/build_spec.rb @@ -886,8 +886,10 @@ describe Ci::Build, models: true do is_expected.to eq(build) end - context 'for success build' do - before { build.queue } + context 'for successful build' do + before do + build.update(status: 'success') + end it 'creates a new build' do is_expected.to be_pending diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index fdb579ab45c..950833cb219 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Ci::Pipeline, models: true do 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(:user) } @@ -51,25 +51,6 @@ describe Ci::Pipeline, models: true do 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 let(:project) { FactoryGirl.create :empty_project, build_coverage_regex: "/.*/" } let(:pipeline) { FactoryGirl.create :ci_empty_pipeline, project: project } @@ -139,32 +120,47 @@ describe Ci::Pipeline, models: true do end end - describe '#reload_status!' do - let(:pipeline) { create :ci_empty_pipeline, project: project } + describe 'state machine' do + let(:current) { Time.now.change(usec: 0) } + let(:build) { create :ci_build, name: 'build1', pipeline: pipeline, started_at: current - 60, finished_at: current } + let(:build2) { create :ci_build, name: 'build2', pipeline: pipeline, started_at: current - 60, finished_at: current } - context 'dependent objects' do - let(:commit_status) { create :commit_status, :pending, pipeline: pipeline } + describe '#duration' do + before do + build.skip + build2.skip + end - it 'executes reload_status! after succeeding dependent object' do - expect(pipeline).to receive(:reload_status!).and_return(true) + it 'matches sum of builds duration' do + expect(pipeline.reload.duration).to eq(build.duration + build2.duration) + end + end + + describe '#started_at' do + it 'updates on transitioning to running' do + build.run - commit_status.success + expect(pipeline.reload.started_at).not_to be_nil + end + + it 'does not update on transitioning to success' do + build.success + + expect(pipeline.reload.started_at).to be_nil end end - context 'updates' do - let(:current) { Time.now.change(usec: 0) } - let(:build) { FactoryGirl.create :ci_build, pipeline: pipeline, started_at: current - 120, finished_at: current - 60 } + describe '#finished_at' do + it 'updates on transitioning to success' do + build.success - before do - build - pipeline.reload_status! + expect(pipeline.reload.finished_at).not_to be_nil end - [:status, :started_at, :finished_at, :duration].each do |param| - it "#{param}" do - expect(pipeline.send(param)).to eq(build.send(param)) - end + it 'does not update on transitioning to running' do + build.run + + expect(pipeline.reload.finished_at).to be_nil end end end @@ -254,4 +250,64 @@ describe Ci::Pipeline, models: true do 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 diff --git a/spec/requests/api/builds_spec.rb b/spec/requests/api/builds_spec.rb index a4cdd8f3140..41503885dd9 100644 --- a/spec/requests/api/builds_spec.rb +++ b/spec/requests/api/builds_spec.rb @@ -9,7 +9,7 @@ describe API::API, api: true do let!(:developer) { create(:project_member, :developer, user: user, project: project) } let(:reporter) { create(:project_member, :reporter, 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) } describe 'GET /projects/:id/builds ' do @@ -174,7 +174,11 @@ describe API::API, api: true do describe 'GET /projects/:id/artifacts/:ref_name/download?job=name' do 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) api("/projects/#{project.id}/builds/artifacts/#{ref}/download?job=#{job}", api_user) @@ -238,10 +242,6 @@ describe API::API, api: true do it { expect(response.headers).to include(download_headers) } end - before do - pipeline.reload_status! - end - context 'with regular branch' do before do pipeline.update(ref: 'master', diff --git a/spec/services/ci/image_for_build_service_spec.rb b/spec/services/ci/image_for_build_service_spec.rb index 259062406c7..c931c3e4829 100644 --- a/spec/services/ci/image_for_build_service_spec.rb +++ b/spec/services/ci/image_for_build_service_spec.rb @@ -14,7 +14,6 @@ module Ci context 'branch name' do before { allow(project).to receive(:commit).and_return(OpenStruct.new(sha: commit_sha)) } before { build.run! } - before { pipeline.reload_status! } let(:image) { service.execute(project, ref: 'master') } it { expect(image).to be_kind_of(OpenStruct) } @@ -32,7 +31,6 @@ module Ci context 'commit sha' do before { build.run! } - before { pipeline.reload_status! } let(:image) { service.execute(project, sha: build.sha) } it { expect(image).to be_kind_of(OpenStruct) } |