diff options
| -rw-r--r-- | app/services/ci/create_pipeline_service.rb | 10 | ||||
| -rw-r--r-- | app/workers/pipeline_schedule_worker.rb | 32 | ||||
| -rw-r--r-- | changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml | 5 | ||||
| -rw-r--r-- | doc/user/project/pipelines/schedules.md | 12 | ||||
| -rw-r--r-- | spec/services/ci/create_pipeline_service_spec.rb | 33 | ||||
| -rw-r--r-- | spec/workers/pipeline_schedule_worker_spec.rb | 78 |
6 files changed, 154 insertions, 16 deletions
diff --git a/app/services/ci/create_pipeline_service.rb b/app/services/ci/create_pipeline_service.rb index 92a8438ab2f..46a82377c10 100644 --- a/app/services/ci/create_pipeline_service.rb +++ b/app/services/ci/create_pipeline_service.rb @@ -4,6 +4,8 @@ module Ci class CreatePipelineService < BaseService attr_reader :pipeline + CreateError = Class.new(StandardError) + SEQUENCE = [Gitlab::Ci::Pipeline::Chain::Build, Gitlab::Ci::Pipeline::Chain::Validate::Abilities, Gitlab::Ci::Pipeline::Chain::Validate::Repository, @@ -47,6 +49,14 @@ module Ci pipeline end + def execute!(*args, &block) + execute(*args, &block).tap do |pipeline| + unless pipeline.persisted? + raise CreateError, pipeline.errors.full_messages.join(',') + end + end + end + private def commit diff --git a/app/workers/pipeline_schedule_worker.rb b/app/workers/pipeline_schedule_worker.rb index 85d1ffe0fa9..ac4e9710f33 100644 --- a/app/workers/pipeline_schedule_worker.rb +++ b/app/workers/pipeline_schedule_worker.rb @@ -9,18 +9,36 @@ class PipelineScheduleWorker Ci::PipelineSchedule.active.where("next_run_at < ?", Time.now) .preload(:owner, :project).find_each do |schedule| begin - pipeline = Ci::CreatePipelineService.new(schedule.project, - schedule.owner, - ref: schedule.ref) - .execute(:schedule, ignore_skip_ci: true, save_on_errors: false, schedule: schedule) - - schedule.deactivate! unless pipeline.persisted? + Ci::CreatePipelineService.new(schedule.project, + schedule.owner, + ref: schedule.ref) + .execute!(:schedule, ignore_skip_ci: true, save_on_errors: true, schedule: schedule) rescue => e - Rails.logger.error "#{schedule.id}: Failed to create a scheduled pipeline: #{e.message}" + error(schedule, e) ensure schedule.schedule_next_run! end end end # rubocop: enable CodeReuse/ActiveRecord + + private + + def error(schedule, error) + failed_creation_counter.increment + + Rails.logger.error "Failed to create a scheduled pipeline. " \ + "schedule_id: #{schedule.id} message: #{error.message}" + + Gitlab::Sentry + .track_exception(error, + issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/41231', + extra: { schedule_id: schedule.id }) + end + + def failed_creation_counter + @failed_creation_counter ||= + Gitlab::Metrics.counter(:pipeline_schedule_creation_failed_total, + "Counter of failed attempts of pipeline schedule creation") + end end diff --git a/changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml b/changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml new file mode 100644 index 00000000000..90f47aa12db --- /dev/null +++ b/changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml @@ -0,0 +1,5 @@ +--- +title: Remove auto deactivation when failed to create a pipeline via pipeline schedules +merge_request: 22243 +author: +type: changed diff --git a/doc/user/project/pipelines/schedules.md b/doc/user/project/pipelines/schedules.md index 9daacc37994..051277dfe02 100644 --- a/doc/user/project/pipelines/schedules.md +++ b/doc/user/project/pipelines/schedules.md @@ -83,12 +83,12 @@ The next time a pipeline is scheduled, your credentials will be used.  -> **Note:** -When the owner of the schedule doesn't have the ability to create pipelines -anymore, due to e.g., being blocked or removed from the project, or lacking -the permission to run on protected branches or tags. When this happened, the -schedule is deactivated. Another user can take ownership and activate it, so -the schedule can be run again. +NOTE: **Note:** +If the owner of a pipeline schedule doesn't have the ability to create pipelines +on the target branch, the schedule will stop creating new pipelines. This can +happen if, for example, the owner is blocked or removed from the project, or +the target branch or tag is protected. In this case, someone with sufficient +privileges must take ownership of the schedule. ## Advanced admin configuration diff --git a/spec/services/ci/create_pipeline_service_spec.rb b/spec/services/ci/create_pipeline_service_spec.rb index 4d9c5aabbda..a4582d1bc64 100644 --- a/spec/services/ci/create_pipeline_service_spec.rb +++ b/spec/services/ci/create_pipeline_service_spec.rb @@ -657,4 +657,37 @@ describe Ci::CreatePipelineService do end end end + + describe '#execute!' do + subject { service.execute!(*args) } + + let(:service) { described_class.new(project, user, ref: ref_name) } + let(:args) { [:push] } + + context 'when user has a permission to create a pipeline' do + let(:user) { create(:user) } + + before do + project.add_developer(user) + end + + it 'does not raise an error' do + expect { subject }.not_to raise_error + end + + it 'creates a pipeline' do + expect { subject }.to change { Ci::Pipeline.count }.by(1) + end + end + + context 'when user does not have a permission to create a pipeline' do + let(:user) { create(:user) } + + it 'raises an error' do + expect { subject } + .to raise_error(described_class::CreateError) + .with_message('Insufficient permissions to create a new pipeline') + end + end + end end diff --git a/spec/workers/pipeline_schedule_worker_spec.rb b/spec/workers/pipeline_schedule_worker_spec.rb index a2fe4734d47..c5a60e9855b 100644 --- a/spec/workers/pipeline_schedule_worker_spec.rb +++ b/spec/workers/pipeline_schedule_worker_spec.rb @@ -56,17 +56,89 @@ describe PipelineScheduleWorker do expect { subject }.not_to change { project.pipelines.count } end end + + context 'when gitlab-ci.yml is corrupted' do + before do + stub_ci_pipeline_yaml_file(YAML.dump(rspec: { variables: 'rspec' } )) + end + + it 'creates a failed pipeline with the reason' do + expect { subject }.to change { project.pipelines.count }.by(1) + expect(Ci::Pipeline.last).to be_config_error + expect(Ci::Pipeline.last.yaml_errors).not_to be_nil + end + end end context 'when the schedule is not runnable by the user' do - it 'deactivates the schedule' do + before do + expect(Gitlab::Sentry) + .to receive(:track_exception) + .with(Ci::CreatePipelineService::CreateError, + issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/41231', + extra: { schedule_id: pipeline_schedule.id } ).once + end + + it 'does not deactivate the schedule' do + subject + + expect(pipeline_schedule.reload.active).to be_truthy + end + + it 'increments Prometheus counter' do + expect(Gitlab::Metrics) + .to receive(:counter) + .with(:pipeline_schedule_creation_failed_total, "Counter of failed attempts of pipeline schedule creation") + .and_call_original + + subject + end + + it 'logging a pipeline error' do + expect(Rails.logger) + .to receive(:error) + .with(a_string_matching("Insufficient permissions to create a new pipeline")) + .and_call_original + subject + end + + it 'does not create a pipeline' do + expect { subject }.not_to change { project.pipelines.count } + end - expect(pipeline_schedule.reload.active).to be_falsy + it 'does not raise an exception' do + expect { subject }.not_to raise_error end + end + + context 'when .gitlab-ci.yml is missing in the project' do + before do + stub_ci_pipeline_yaml_file(nil) + project.add_maintainer(user) - it 'does not schedule a pipeline' do + expect(Gitlab::Sentry) + .to receive(:track_exception) + .with(Ci::CreatePipelineService::CreateError, + issue_url: 'https://gitlab.com/gitlab-org/gitlab-ce/issues/41231', + extra: { schedule_id: pipeline_schedule.id } ).once + end + + it 'logging a pipeline error' do + expect(Rails.logger) + .to receive(:error) + .with(a_string_matching("Missing .gitlab-ci.yml file")) + .and_call_original + + subject + end + + it 'does not create a pipeline' do expect { subject }.not_to change { project.pipelines.count } end + + it 'does not raise an exception' do + expect { subject }.not_to raise_error + end end end |