summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--app/services/ci/create_pipeline_service.rb10
-rw-r--r--app/workers/pipeline_schedule_worker.rb32
-rw-r--r--changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml5
-rw-r--r--doc/user/project/pipelines/schedules.md12
-rw-r--r--spec/services/ci/create_pipeline_service_spec.rb33
-rw-r--r--spec/workers/pipeline_schedule_worker_spec.rb78
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.
![Schedules list](img/pipeline_schedules_ownership.png)
-> **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