From 099da5db3837febb0350d56a410566ce1eb74580 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Sat, 2 Sep 2017 17:08:14 +0900 Subject: [PATCH] Squashed commit of the following: commit baf6eb8eb7a41b505f98dd179572c21b43fa5598 Author: Shinya Maeda Date: Sat Sep 2 17:06:25 2017 +0900 Improve spec commit fea578d224ff720cd3ee3b457a81e0c8d74ed92d Author: Shinya Maeda Date: Fri Sep 1 22:01:26 2017 +0900 Fix spec commit ebe36fbcad608f90f52dccd703b16ef0d023a4e8 Author: Shinya Maeda Date: Fri Sep 1 16:52:11 2017 +0900 - Allow runner API to pass failure_reason - Fix spec commit 4532913a4d01be8a854bee8c15d2f11a356fb2b6 Author: Shinya Maeda Date: Thu Aug 31 22:03:41 2017 +0900 Fix enum lists commit bf9a8b051c906ce52beea8c91f6c1a29eac3a83b Author: Shinya Maeda Date: Thu Aug 31 03:36:15 2017 +0900 Fix enum wording commit 22077f22e3b9b58a4b2d30c57ead9c0f05f99dd8 Author: Shinya Maeda Date: Thu Aug 31 03:20:54 2017 +0900 Implement `failure_reason` on `ci_builds` --- app/models/commit_status.rb | 12 ++++++++++ app/services/projects/update_pages_service.rb | 2 +- app/workers/stuck_ci_jobs_worker.rb | 2 +- ...-implement-failure_reason-on-ci_builds.yml | 5 +++++ ...0125940_add_failure_reason_to_ci_builds.rb | 9 ++++++++ db/schema.rb | 3 ++- lib/api/commit_statuses.rb | 2 +- lib/api/runner.rb | 8 ++++++- .../import_export/safe_model_attributes.yml | 1 + spec/models/commit_status_spec.rb | 21 ++++++++++++++++++ spec/requests/api/commit_statuses_spec.rb | 3 +++ spec/requests/api/runner_spec.rb | 11 ++++++++++ spec/services/ci/retry_build_service_spec.rb | 4 ++-- .../projects/update_pages_service_spec.rb | 1 + spec/workers/stuck_ci_jobs_worker_spec.rb | 22 +++++++++++-------- 15 files changed, 90 insertions(+), 16 deletions(-) create mode 100644 changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml create mode 100644 db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 842c6e5cb50d79..1e8f28b646636b 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -38,6 +38,13 @@ class CommitStatus < ActiveRecord::Base scope :retried_ordered, -> { retried.ordered.includes(project: :namespace) } scope :after_stage, -> (index) { where('stage_idx > ?', index) } + enum failure_reason: { + unknown_failure: nil, + job_failure: 1, + api_failure: 2, + stuck_or_timeout_failure: 3 + } + state_machine :status do event :process do transition [:skipped, :manual] => :created @@ -79,6 +86,11 @@ class CommitStatus < ActiveRecord::Base commit_status.finished_at = Time.now end + before_transition any => :failed do |commit_status, transition| + failure_reason = transition.args.first + commit_status.failure_reason = failure_reason + end + after_transition do |commit_status, transition| next if transition.loopback? diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 0c131afca315d1..ac7f4df14d73ac 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -51,7 +51,7 @@ def error(message, http_status = nil) log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message - @status.drop + @status.drop(:job_failure) super end diff --git a/app/workers/stuck_ci_jobs_worker.rb b/app/workers/stuck_ci_jobs_worker.rb index 8b0cfcc8af8057..269776a1f626e0 100644 --- a/app/workers/stuck_ci_jobs_worker.rb +++ b/app/workers/stuck_ci_jobs_worker.rb @@ -53,7 +53,7 @@ def search(status, timeout) def drop_build(type, build, status, timeout) Rails.logger.info "#{self.class}: Dropping #{type} build #{build.id} for runner #{build.runner_id} (status: #{status}, timeout: #{timeout})" Gitlab::OptimisticLocking.retry_lock(build, 3) do |b| - b.drop + b.drop(:stuck_or_timeout_failure) end end end diff --git a/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml b/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml new file mode 100644 index 00000000000000..006b0b458446d4 --- /dev/null +++ b/changelogs/unreleased/feature-sm-37239-implement-failure_reason-on-ci_builds.yml @@ -0,0 +1,5 @@ +--- +title: Implement `failure_reason` on `ci_builds` +merge_request: 13937 +author: +type: added diff --git a/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb b/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb new file mode 100644 index 00000000000000..5a7487b9227cd6 --- /dev/null +++ b/db/migrate/20170830125940_add_failure_reason_to_ci_builds.rb @@ -0,0 +1,9 @@ +class AddFailureReasonToCiBuilds < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_builds, :failure_reason, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 26dddd3d74c181..4d3be96031dc45 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170824162758) do +ActiveRecord::Schema.define(version: 20170830125940) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -301,6 +301,7 @@ t.integer "stage_id" t.integer "artifacts_file_store" t.integer "artifacts_metadata_store" + t.integer "failure_reason" end add_index "ci_builds", ["auto_canceled_by_id"], name: "index_ci_builds_on_auto_canceled_by_id", using: :btree diff --git a/lib/api/commit_statuses.rb b/lib/api/commit_statuses.rb index 485b680cd5fddb..892a5e80a1b28d 100644 --- a/lib/api/commit_statuses.rb +++ b/lib/api/commit_statuses.rb @@ -101,7 +101,7 @@ class CommitStatuses < Grape::API when 'success' status.success! when 'failed' - status.drop! + status.drop!(:api_failure) when 'canceled' status.cancel! else diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 119993545947fd..44ca42fef684dd 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -114,6 +114,8 @@ class Runner < Grape::API requires :id, type: Integer, desc: %q(Job's ID) optional :trace, type: String, desc: %q(Job's full trace) optional :state, type: String, desc: %q(Job's status: success, failed) + optional :failure_reason, type: String, values: CommitStatus.failure_reasons.keys, + desc: %q(Job's failure_reason) end put '/:id' do job = authenticate_job! @@ -127,7 +129,11 @@ class Runner < Grape::API when 'success' job.success when 'failed' - job.drop + if params[:failure_reason] + job.drop(params[:failure_reason].to_sym) + else + job.drop(:job_failure) + end end end diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index a4ef0344ce35b1..30a7d9a154f3d9 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -282,6 +282,7 @@ CommitStatus: - coverage_regex - auto_canceled_by_id - retried +- failure_reason Ci::Variable: - id - project_id diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index f7583645e690b2..0a6b0023dea3ab 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -443,4 +443,25 @@ def create_status(**opts) end end end + + describe 'set failure_reason when drop' do + let(:commit_status) { create(:commit_status, :created) } + + subject do + commit_status.drop!(reason) + commit_status + end + + context 'when failure_reason is nil' do + let(:reason) { } + + it { is_expected.to be_unknown_failure } + end + + context 'when failure_reason is job_failure' do + let(:reason) { :job_failure } + + it { is_expected.to be_job_failure } + end + end end diff --git a/spec/requests/api/commit_statuses_spec.rb b/spec/requests/api/commit_statuses_spec.rb index 3c02e6302b4e15..fcdcf461c52d21 100644 --- a/spec/requests/api/commit_statuses_spec.rb +++ b/spec/requests/api/commit_statuses_spec.rb @@ -142,6 +142,9 @@ def create_status(commit, opts = {}) expect(json_response['ref']).not_to be_empty expect(json_response['target_url']).to be_nil expect(json_response['description']).to be_nil + if status == 'failed' + expect(CommitStatus.find(json_response['id'])).to be_api_failure + end end end end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 4ff981330490ef..5aa5f08b1829a9 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -629,12 +629,23 @@ def request_job(token = runner.token, **params) update_job(state: 'success') expect(job.reload.status).to eq 'success' + expect(job).to be_unknown_failure end it 'mark job as failed' do update_job(state: 'failed') expect(job.reload.status).to eq 'failed' + expect(job).to be_job_failure + end + + context 'when failure_reason is given' do + it 'mark job as failed' do + update_job(state: 'failed', failure_reason: 'stuck_or_timeout_failure') + + expect(job.reload.status).to eq 'failed' + expect(job).to be_stuck_or_timeout_failure + end end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 7e7a68c947b214..383bd73c32daa1 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -22,8 +22,8 @@ %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 retried sourced_pipelines - artifacts_file_store artifacts_metadata_store].freeze + user_id auto_canceled_by_id retried failure_reason + sourced_pipelines artifacts_file_store artifacts_metadata_store].freeze # EE shared_examples 'build duplication' do let(:stage) do diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb index aa6ad6340f5c0e..5fc69c7e4e56c9 100644 --- a/spec/services/projects/update_pages_service_spec.rb +++ b/spec/services/projects/update_pages_service_spec.rb @@ -116,6 +116,7 @@ expect(deploy_status.description) .to match(/artifacts for pages are too large/) + expect(deploy_status).to be_job_failure end end diff --git a/spec/workers/stuck_ci_jobs_worker_spec.rb b/spec/workers/stuck_ci_jobs_worker_spec.rb index 549635f7f334db..ac6f4fefb4ef95 100644 --- a/spec/workers/stuck_ci_jobs_worker_spec.rb +++ b/spec/workers/stuck_ci_jobs_worker_spec.rb @@ -6,27 +6,31 @@ let(:worker) { described_class.new } let(:exclusive_lease_uuid) { SecureRandom.uuid } - subject do - job.reload - job.status - end - before do job.update!(status: status, updated_at: updated_at) allow_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).and_return(exclusive_lease_uuid) end shared_examples 'job is dropped' do - it 'changes status' do + before do worker.perform - is_expected.to eq('failed') + job.reload + end + + it "changes status" do + expect(job).to be_failed + expect(job).to be_stuck_or_timeout_failure end end shared_examples 'job is unchanged' do - it "doesn't change status" do + before do worker.perform - is_expected.to eq(status) + job.reload + end + + it "doesn't change status" do + expect(job.status).to eq(status) end end -- GitLab