Merge branch 'issue-32741' into 'master'

Interruptible builds for redundant pipelines See merge request gitlab-org/gitlab-ce!23464

Merge branch 'issue-32741' into 'master'
Interruptible builds for redundant pipelines See merge request gitlab-org/gitlab-ce!23464
0b43cf89 · Kamil Trzciński · be920a60 · e195e486 · 0b43cf89 · 0b43cf89
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -88,6 +88,7 @@ def persisted_environment
    validates :coverage, numericality: true, allow_blank: true
    validates :ref, presence: true

+    scope :not_interruptible, -> { joins(:metadata).where(ci_builds_metadata: { interruptible: false }) }
    scope :unstarted, ->() { where(runner_id: nil) }
    scope :ignore_failures, ->() { where(allow_failure: false) }
    scope :with_artifacts_archive, ->() do

--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -225,6 +225,14 @@ class Pipeline < ApplicationRecord
      where('EXISTS (?)', ::Ci::Build.latest.with_reports(reports_scope).where('ci_pipelines.id=ci_builds.commit_id').select(1))
    end

+    scope :without_interruptible_builds, -> do
+      where('NOT EXISTS (?)',
+        Ci::Build.where('ci_builds.commit_id = ci_pipelines.id')
+                 .with_status(:running, :success, :failed)
+                 .not_interruptible
+      )
+    end
+
    # Returns the pipelines in descending order (= newest first), optionally
    # limited to a number of references.
    #

--- a/app/models/concerns/ci/metadatable.rb
+++ b/app/models/concerns/ci/metadatable.rb
@@ -15,6 +15,7 @@ module Metadatable
                         autosave: true

      delegate :timeout, to: :metadata, prefix: true, allow_nil: true
+      delegate :interruptible, to: :metadata, prefix: false, allow_nil: true
      before_create :ensure_metadata
    end

@@ -50,6 +51,14 @@ def yaml_variables=(value)
      write_metadata_attribute(:yaml_variables, :config_variables, value)
    end

+    def interruptible
+      metadata&.interruptible
+    end
+
+    def interruptible=(value)
+      ensure_metadata.interruptible = value
+    end
+
    private

    def read_metadata_attribute(legacy_key, metadata_key, default_value = nil)

--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -102,6 +102,7 @@ def completed_statuses
    scope :manual, -> { with_status(:manual) }
    scope :scheduled, -> { with_status(:scheduled) }
    scope :alive, -> { with_status(:created, :preparing, :pending, :running) }
+    scope :alive_or_scheduled, -> { with_status(:created, :preparing, :pending, :running, :scheduled) }
    scope :created_or_pending, -> { with_status(:created, :pending) }
    scope :running_or_pending, -> { with_status(:running, :pending) }
    scope :finished, -> { with_status(:success, :failed, :canceled) }

--- a/app/services/ci/create_pipeline_service.rb
+++ b/app/services/ci/create_pipeline_service.rb
@@ -91,11 +91,21 @@ def cancel_pending_pipelines

    # rubocop: disable CodeReuse/ActiveRecord
    def auto_cancelable_pipelines
-      project.ci_pipelines
-        .where(ref: pipeline.ref)
-        .where.not(id: pipeline.id)
-        .where.not(sha: project.commit(pipeline.ref).try(:id))
-        .created_or_pending
+      # TODO: Introduced by https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/23464
+      if Feature.enabled?(:ci_support_interruptible_pipelines, project, default_enabled: true)
+        project.ci_pipelines
+          .where(ref: pipeline.ref)
+          .where.not(id: pipeline.id)
+          .where.not(sha: project.commit(pipeline.ref).try(:id))
+          .alive_or_scheduled
+          .without_interruptible_builds
+      else
+        project.ci_pipelines
+          .where(ref: pipeline.ref)
+          .where.not(id: pipeline.id)
+          .where.not(sha: project.commit(pipeline.ref).try(:id))
+          .created_or_pending
+      end
    end
    # rubocop: enable CodeReuse/ActiveRecord


--- a/changelogs/unreleased/issue-32741.yml
+++ b/changelogs/unreleased/issue-32741.yml
+---
+title: New interruptible attribute for CI/CD jobs
+merge_request: 23464
+author: Cédric Tabin
+type: added
--- a/db/migrate/20190905223800_add_interruptible_to_builds_metadata.rb
+++ b/db/migrate/20190905223800_add_interruptible_to_builds_metadata.rb
+# frozen_string_literal: true
+
+class AddInterruptibleToBuildsMetadata < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    add_column :ci_builds_metadata, :interruptible, :boolean
+  end
+end
--- a/db/migrate/20190905223900_add_concurrent_index_to_builds_metadata.rb
+++ b/db/migrate/20190905223900_add_concurrent_index_to_builds_metadata.rb
+# frozen_string_literal: true
+
+class AddConcurrentIndexToBuildsMetadata < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :ci_builds_metadata, [:build_id],
+                         where: "interruptible = false",
+                         name: "index_ci_builds_metadata_on_build_id_and_interruptible_false"
+  end
+
+  def down
+    remove_concurrent_index_by_name(:ci_builds_metadata, 'index_ci_builds_metadata_on_build_id_and_interruptible_false')
+  end
+end
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.

-ActiveRecord::Schema.define(version: 2019_09_04_173203) do
+ActiveRecord::Schema.define(version: 2019_09_05_223900) do

  # These are extensions that must be enabled in order to support this database
  enable_extension "pg_trgm"
@@ -619,9 +619,11 @@
    t.integer "project_id", null: false
    t.integer "timeout"
    t.integer "timeout_source", default: 1, null: false
+    t.boolean "interruptible"
    t.jsonb "config_options"
    t.jsonb "config_variables"
    t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true
+    t.index ["build_id"], name: "index_ci_builds_metadata_on_build_id_and_interruptible_false", where: "(interruptible = false)"
    t.index ["project_id"], name: "index_ci_builds_metadata_on_project_id"
  end


--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -116,6 +116,7 @@ The following table lists available parameters for jobs:
 | [`extends`](#extends)                              | Configuration entries that this job is going to inherit from.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
 | [`pages`](#pages)                                  | Upload the result of a job to use with GitLab Pages.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
 | [`variables`](#variables)                          | Define job variables on a job level.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
+| [interruptible](#interruptible)                    | Defines if a job can be canceled when made redundant by a newer run                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |

 NOTE: **Note:**
 Parameters `types` and `type` are [deprecated](#deprecated-parameters).
@@ -2083,6 +2084,46 @@ staging:
    branch: stable
 ```

+### `interruptible`
+
+`interruptible` is used to indicate that a job should be canceled if made redundant by a newer run of the same job. Defaults to `false` if there is an environment defined and `true` otherwise.
+This value will only be used if the [automatic cancellation of redundant pipelines feature](https://docs.gitlab.com/ee/user/project/pipelines/settings.html#auto-cancel-pending-pipelines)
+is enabled.
+
+When enabled, a pipeline on the same branch will be canceled when:
+
+- It is made redundant by a newer pipeline run.
+- Either all jobs are set as interruptible, or any uninterruptible jobs are not yet pending.
+
+Pending jobs are always considered interruptible.
+
+TIP: **Tip:**
+Set jobs as uninterruptible that should behave atomically and should never be canceled once started.
+
+Here is a simple example:
+
+```yaml
+stages:
+  - stage1
+  - stage2
+
+step-1:
+  stage: stage1
+  script:
+    - echo "Can be canceled"
+  
+step-2:
+  stage: stage2
+  script:
+    - echo "Can not be canceled"
+  interruptible: false
+```
+
+In the example above, a new pipeline run will cause an existing running pipeline to be:
+
+- Canceled, if only `step-1` is running or pending.
+- Not canceled, once `step-2` becomes pending.
+
 ### `include`

 > - Introduced in [GitLab Premium](https://about.gitlab.com/pricing/) 10.5.

--- a/lib/gitlab/ci/config/entry/job.rb
+++ b/lib/gitlab/ci/config/entry/job.rb
@@ -15,7 +15,7 @@ class Job < ::Gitlab::Config::Entry::Node
          ALLOWED_KEYS = %i[tags script only except rules type image services
                            allow_failure type stage when start_in artifacts cache
                            dependencies needs before_script after_script variables
-                            environment coverage retry parallel extends].freeze
+                            environment coverage retry parallel extends interruptible].freeze

          REQUIRED_BY_NEEDS = %i[stage].freeze

@@ -37,6 +37,7 @@ class Job < ::Gitlab::Config::Entry::Node
            with_options allow_nil: true do
              validates :tags, array_of_strings: true
              validates :allow_failure, boolean: true
+              validates :interruptible, boolean: true
              validates :parallel, numericality: { only_integer: true,
                                                   greater_than_or_equal_to: 2,
                                                   less_than_or_equal_to: 50 }
@@ -122,10 +123,11 @@ class Job < ::Gitlab::Config::Entry::Node
          helpers :before_script, :script, :stage, :type, :after_script,
                  :cache, :image, :services, :only, :except, :variables,
                  :artifacts, :environment, :coverage, :retry,
-                  :parallel, :needs
+                  :parallel, :needs, :interruptible

          attributes :script, :tags, :allow_failure, :when, :dependencies,
-                     :needs, :retry, :parallel, :extends, :start_in, :rules
+                     :needs, :retry, :parallel, :extends, :start_in, :rules,
+                     :interruptible

          def self.matching?(name, config)
            !name.to_s.start_with?('.') &&
@@ -207,6 +209,7 @@ def to_hash
              coverage: coverage_defined? ? coverage_value : nil,
              retry: retry_defined? ? retry_value : nil,
              parallel: parallel_defined? ? parallel_value.to_i : nil,
+              interruptible: interruptible_defined? ? interruptible_value : nil,
              artifacts: artifacts_value,
              after_script: after_script_value,
              ignore: ignored?,

--- a/lib/gitlab/ci/yaml_processor.rb
+++ b/lib/gitlab/ci/yaml_processor.rb
@@ -41,6 +41,7 @@ def build_attributes(name)
          coverage_regex: job[:coverage],
          yaml_variables: yaml_variables(name),
          needs_attributes: job[:needs]&.map { |need| { name: need } },
+          interruptible: job[:interruptible],
          options: {
            image: job[:image],
            services: job[:services],

--- a/spec/lib/gitlab/ci/yaml_processor_spec.rb
+++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb
@@ -50,6 +50,32 @@ module Ci
          end
        end

+        describe 'interruptible entry' do
+          describe 'interruptible job' do
+            let(:config) do
+              YAML.dump(rspec: { script: 'rspec', interruptible: true })
+            end
+
+            it { expect(subject[:interruptible]).to be_truthy }
+          end
+
+          describe 'interruptible job with default value' do
+            let(:config) do
+              YAML.dump(rspec: { script: 'rspec' })
+            end
+
+            it { expect(subject).not_to have_key(:interruptible) }
+          end
+
+          describe 'uninterruptible job' do
+            let(:config) do
+              YAML.dump(rspec: { script: 'rspec', interruptible: false })
+            end
+
+            it { expect(subject[:interruptible]).to be_falsy }
+          end
+        end
+
        describe 'retry entry' do
          context 'when retry count is specified' do
            let(:config) do

--- a/spec/lib/gitlab/import_export/safe_model_attributes.yml
+++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml
@@ -329,6 +329,7 @@ CommitStatus:
 - failure_reason
 - scheduled_at
 - upstream_pipeline_id
+- interruptible
 Ci::Variable:
 - id
 - project_id

--- a/spec/models/concerns/has_status_spec.rb
+++ b/spec/models/concerns/has_status_spec.rb
@@ -261,6 +261,18 @@
      end
    end

+    describe '.alive_or_scheduled' do
+      subject { CommitStatus.alive_or_scheduled }
+
+      %i[running pending preparing created scheduled].each do |status|
+        it_behaves_like 'containing the job', status
+      end
+
+      %i[failed success canceled skipped].each do |status|
+        it_behaves_like 'not containing the job', status
+      end
+    end
+
    describe '.created_or_pending' do
      subject { CommitStatus.created_or_pending }


--- a/spec/services/ci/create_pipeline_service_spec.rb
+++ b/spec/services/ci/create_pipeline_service_spec.rb
@@ -220,11 +220,11 @@ def execute_service(
          expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: pipeline.id)
        end

-        it 'does not cancel running outdated pipelines' do
+        it 'cancels running outdated pipelines' do
          pipeline_on_previous_commit.run
-          execute_service
+          head_pipeline = execute_service

-          expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'running', auto_canceled_by_id: nil)
+          expect(pipeline_on_previous_commit.reload).to have_attributes(status: 'canceled', auto_canceled_by_id: head_pipeline.id)
        end

        it 'cancel created outdated pipelines' do
@@ -243,6 +243,202 @@ def execute_service(

          expect(pending_pipeline.reload).to have_attributes(status: 'pending', auto_canceled_by_id: nil)
        end
+
+        context 'when the interruptible attribute is' do
+          context 'not defined' do
+            before do
+              config = YAML.dump(rspec: { script: 'echo' })
+              stub_ci_pipeline_yaml_file(config)
+            end
+
+            it 'is cancelable' do
+              pipeline = execute_service
+
+              expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_nil
+            end
+          end
+
+          context 'set to true' do
+            before do
+              config = YAML.dump(rspec: { script: 'echo', interruptible: true })
+              stub_ci_pipeline_yaml_file(config)
+            end
+
+            it 'is cancelable' do
+              pipeline = execute_service
+
+              expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_truthy
+            end
+          end
+
+          context 'set to false' do
+            before do
+              config = YAML.dump(rspec: { script: 'echo', interruptible: false })
+              stub_ci_pipeline_yaml_file(config)
+            end
+
+            it 'is not cancelable' do
+              pipeline = execute_service
+
+              expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_falsy
+            end
+          end
+
+          context 'not defined, but an environment is' do
+            before do
+              config = YAML.dump(rspec: { script: 'echo', environment: { name: "review/$CI_COMMIT_REF_NAME" } })
+              stub_ci_pipeline_yaml_file(config)
+            end
+
+            it 'is not cancelable' do
+              pipeline = execute_service
+
+              expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_nil
+            end
+          end
+
+          context 'overriding the environment definition' do
+            before do
+              config = YAML.dump(rspec: { script: 'echo', environment: { name: "review/$CI_COMMIT_REF_NAME" }, interruptible: true })
+              stub_ci_pipeline_yaml_file(config)
+            end
+
+            it 'is cancelable' do
+              pipeline = execute_service
+
+              expect(pipeline.builds.find_by(name: 'rspec').interruptible).to be_truthy
+            end
+          end
+        end
+
+        context 'interruptible builds' do
+          before do
+            stub_ci_pipeline_yaml_file(YAML.dump(config))
+          end
+
+          let(:config) do
+            {
+              stages: %w[stage1 stage2 stage3 stage4],
+
+              build_1_1: {
+                stage: 'stage1',
+                script: 'echo'
+              },
+              build_1_2: {
+                stage: 'stage1',
+                script: 'echo',
+                interruptible: true
+              },
+              build_2_1: {
+                stage: 'stage2',
+                script: 'echo',
+                when: 'delayed',
+                start_in: '10 minutes'
+              },
+              build_3_1: {
+                stage: 'stage3',
+                script: 'echo',
+                interruptible: false
+              },
+              build_4_1: {
+                stage: 'stage4',
+                script: 'echo'
+              }
+            }
+          end
+
+          it 'properly configures interruptible status' do
+            interruptible_status =
+              pipeline_on_previous_commit
+                .builds
+                .joins(:metadata)
+                .pluck(:name, 'ci_builds_metadata.interruptible')
+
+            expect(interruptible_status).to contain_exactly(
+              ['build_1_1', nil],
+              ['build_1_2', true],
+              ['build_2_1', nil],
+              ['build_3_1', false],
+              ['build_4_1', nil]
+            )
+          end
+
+          context 'when only interruptible builds are running' do
+            context 'when build marked explicitly by interruptible is running' do
+              it 'cancels running outdated pipelines' do
+                pipeline_on_previous_commit
+                  .builds
+                  .find_by_name('build_1_2')
+                  .run!
+
+                pipeline
+
+                expect(pipeline_on_previous_commit.reload).to have_attributes(
+                  status: 'canceled', auto_canceled_by_id: pipeline.id)
+              end
+            end
+
+            context 'when build that is not marked as interruptible is running' do
+              it 'cancels running outdated pipelines' do
+                pipeline_on_previous_commit
+                  .builds
+                  .find_by_name('build_2_1')
+                  .tap(&:enqueue!)
+                  .run!
+
+                pipeline
+
+                expect(pipeline_on_previous_commit.reload).to have_attributes(
+                  status: 'canceled', auto_canceled_by_id: pipeline.id)
+              end
+            end
+          end
+
+          context 'when an uninterruptible build is running' do
+            it 'does not cancel running outdated pipelines' do
+              pipeline_on_previous_commit
+                .builds
+                .find_by_name('build_3_1')
+                .tap(&:enqueue!)
+                .run!
+
+              pipeline
+
+              expect(pipeline_on_previous_commit.reload).to have_attributes(
+                status: 'running', auto_canceled_by_id: nil)
+            end
+          end
+
+          context 'when an build is waiting on an interruptible scheduled task' do
+            it 'cancels running outdated pipelines' do
+              allow(Ci::BuildScheduleWorker).to receive(:perform_at)
+
+              pipeline_on_previous_commit
+                .builds
+                .find_by_name('build_2_1')
+                .schedule!
+
+              pipeline
+
+              expect(pipeline_on_previous_commit.reload).to have_attributes(
+                status: 'canceled', auto_canceled_by_id: pipeline.id)
+            end
+          end
+
+          context 'when a uninterruptible build has finished' do
+            it 'does not cancel running outdated pipelines' do
+              pipeline_on_previous_commit
+                .builds
+                .find_by_name('build_3_1')
+                .success!
+
+              pipeline
+
+              expect(pipeline_on_previous_commit.reload).to have_attributes(
+                status: 'running', auto_canceled_by_id: nil)
+            end
+          end
+        end
      end

      context 'auto-cancel disabled' do