Squashed commit of the following:

commit 0c00e52d339f8471a6ea425d5a4a59751a3f4a35 Author: Shinya Maeda <shinya@gitlab.com> Date: Fri Nov 30 15:41:46 2018 +0900 Update schedules.md commit 0ae56bf5a0ba9254d2ebd4c846395113ae72d686 Merge: c143777c9f2 9ce28bf08b Author: Shinya Maeda <shinya@gitlab.com> Date: Fri Nov 30 15:38:01 2018 +0900 Merge branch 'master-ce' into ignore-failed-pipeline-creation-on-pipeline-schedule commit c143777c9f250c8075355ac07e9bae7b074665c3 Author: Shinya Maeda <shinya@gitlab.com> Date: Thu Nov 29 17:18:07 2018 +0900 Fix coding offence commit 7c816dfa634b5911310c67c285fc3c37d5f03517 Author: Shinya Maeda <shinya@gitlab.com> Date: Thu Nov 29 16:12:06 2018 +0900 Improve spec quality commit f78eed45e991123f8af4a7b24f041529bbb35e91 Merge: 96d20ce9144 a5f4627857 Author: Shinya Maeda <shinya@gitlab.com> Date: Thu Nov 29 15:20:16 2018 +0900 Merge branch 'master-ce' into ignore-failed-pipeline-creation-on-pipeline-schedule commit 96d20ce914458f86e68b57bc1bb88ab8d27f010b Author: Shinya Maeda <shinya@gitlab.com> Date: Tue Nov 27 16:25:42 2018 +0900 Print pipeline error commit 97842068b6cf1432cd400ead749843946b4f51ee Merge: c2b015949af 2ee8c40fc1 Author: Shinya Maeda <shinya@gitlab.com> Date: Tue Nov 27 15:51:49 2018 +0900 Merge branch 'master-ce' into ignore-failed-pipeline-creation-on-pipeline-schedule commit c2b015949afb3ecc70cb057e2d13672f378c0d03 Merge: 3435137c17b fbbe5ccd1b Author: Shinya Maeda <shinya@gitlab.com> Date: Mon Nov 26 15:26:17 2018 +0900 Merge branch 'master-ce' into ignore-failed-pipeline-creation-on-pipeline-schedule commit 3435137c17b0ef03003e39dd08c7370fe916c626 Author: Shinya Maeda <shinya@gitlab.com> Date: Tue Nov 20 17:45:38 2018 +0900 Track exception with Sentry commit 3f01f10d3b7380f0e8ceb3a379d8b6c602e9d6ca Merge: 5749c62355f 8a581d531b Author: Shinya Maeda <shinya@gitlab.com> Date: Tue Nov 20 17:12:41 2018 +0900 Merge branch 'master-ce' into ignore-failed-pipeline-creation-on-pipeline-schedule commit 5749c62355f8de62bb4e36ba1e351a78350607c1 Author: Shinya Maeda <shinya@gitlab.com> Date: Thu Nov 1 11:14:26 2018 +0900 Create a pipeline even if it is corrupted commit e01789890b6949b346d40fadef41aa133191cc43 Author: Shinya Maeda <shinya@gitlab.com> Date: Wed Oct 31 14:26:09 2018 +0900 Improve production log message commit f20d698a535f1dc70d5437c20b629fd1d956fb27 Author: Shinya Maeda <shinya@gitlab.com> Date: Fri Oct 19 17:11:20 2018 +0900 Fix typo commit 01323b02ac41ec50bcf237409f2e3c5c214bbfc1 Author: Shinya Maeda <shinya@gitlab.com> Date: Thu Oct 18 14:46:44 2018 +0900 Update documents commit 460337bf4a7e67a35d6c342678b4cfe66710ad56 Author: Shinya Maeda <shinya@gitlab.com> Date: Wed Oct 10 13:21:26 2018 +0900 Add changelog commit a3c4711752fedebfacbdf52da94058524af3c9f4 Author: Shinya Maeda <shinya@gitlab.com> Date: Wed Oct 10 09:20:06 2018 +0900 Ignore failed pipeline creation in pipeline schedule worker. Instead, logging the event.
2018-11-30 16:32:30 +09:00 · 2018-11-30 16:32:30 +09:00 · d56d419a79
parent 9ce28bf08b
commit d56d419a79
6 changed files with 156 additions and 18 deletions
--- 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
--- 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
--- a/changelogs/unreleased/ignore-failed-pipeline-creation-on-pipeline-schedule.yml
+++ 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
--- 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

--- 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
--- 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
-      subject
-
-      expect(pipeline_schedule.reload.active).to be_falsy
+    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 schedule a pipeline' do
+    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
+
+    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)
+
+      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