Merge branch 'specify-job-level-timeout' into 'master'

Allow timeout property to be provided per build

See merge request gitlab-org/gitlab!16777

app/models/ci/build_metadata.rb

@@ -4,9 +4,12 @@ module Ci
   # The purpose of this class is to store Build related data that can be disposed.
   # Data that should be persisted forever, should be stored with Ci::Build model.
   class BuildMetadata < ApplicationRecord
+    BuildTimeout = Struct.new(:value, :source)
+
     extend Gitlab::Ci::Model
     include Presentable
     include ChronicDurationAttribute
+    include Gitlab::Utils::StrongMemoize
 
     self.table_name = 'ci_builds_metadata'
 
@@ -25,17 +28,16 @@ module Ci
     enum timeout_source: {
         unknown_timeout_source: 1,
         project_timeout_source: 2,
-        runner_timeout_source: 3
+        runner_timeout_source: 3,
+        job_timeout_source: 4
     }
 
     def update_timeout_state
-      return unless build.runner.present?
+      timeout = timeout_with_highest_precedence
 
-      project_timeout = project&.build_timeout
-      timeout = [project_timeout, build.runner.maximum_timeout].compact.min
-      timeout_source = timeout < project_timeout ? :runner_timeout_source : :project_timeout_source
+      return unless timeout
 
-      update(timeout: timeout, timeout_source: timeout_source)
+      update(timeout: timeout.value, timeout_source: timeout.source)
     end
 
     private
@@ -43,5 +45,37 @@ module Ci
     def set_build_project
       self.project_id ||= self.build.project_id
     end
+
+    def timeout_with_highest_precedence
+      [(job_timeout || project_timeout), runner_timeout].compact.min_by { |timeout| timeout.value }
+    end
+
+    def project_timeout
+      strong_memoize(:project_timeout) do
+        BuildTimeout.new(project&.build_timeout, :project_timeout_source)
+      end
+    end
+
+    def job_timeout
+      return unless build.options
+
+      strong_memoize(:job_timeout) do
+        if timeout_from_options = build.options[:job_timeout]
+          BuildTimeout.new(timeout_from_options, :job_timeout_source)
+        end
+      end
+    end
+
+    def runner_timeout
+      return unless runner_timeout_set?
+
+      strong_memoize(:runner_timeout) do
+        BuildTimeout.new(build.runner.maximum_timeout, :runner_timeout_source)
+      end
+    end
+
+    def runner_timeout_set?
+      build.runner&.maximum_timeout.to_i > 0
+    end
   end
 end

app/models/project.rb

@@ -58,6 +58,7 @@ class Project < ApplicationRecord
   ACCESS_REQUEST_APPROVERS_TO_BE_NOTIFIED_LIMIT = 10
 
   SORTING_PREFERENCE_FIELD = :projects_sort
+  MAX_BUILD_TIMEOUT = 1.month
 
   cache_markdown_field :description, pipeline: :description
 
@@ -430,7 +431,7 @@ class Project < ApplicationRecord
 
   validates :build_timeout, allow_nil: true,
                             numericality: { greater_than_or_equal_to: 10.minutes,
-                                            less_than: 1.month,
+                                            less_than: MAX_BUILD_TIMEOUT,
                                             only_integer: true,
                                             message: _('needs to be between 10 minutes and 1 month') }
 

app/presenters/ci/build_metadata_presenter.rb

@@ -5,7 +5,8 @@ module Ci
     TIMEOUT_SOURCES = {
         unknown_timeout_source: nil,
         project_timeout_source: 'project',
-        runner_timeout_source: 'runner'
+        runner_timeout_source: 'runner',
+        job_timeout_source: 'job'
     }.freeze
 
     presents :metadata

changelogs/unreleased/ms-implement-build-specific-timeout.yml

+---
+title: Allow specifying timeout per-job in .gitlab-ci.yml
+merge_request: 16777
+author: Michał Siwek
+type: added

doc/ci/yaml/README.md

@@ -110,6 +110,7 @@ The following table lists available parameters for jobs:
 | [`dependencies`](#dependencies)                    | Other jobs that a job depends on so that you can pass artifacts between them.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
 | [`coverage`](#coverage)                            | Code coverage settings for a given job.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 |
 | [`retry`](#retry)                                  | When and how many times a job can be auto-retried in case of a failure.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 |
+| [`timeout`](#timeout)                              | Define a custom timeout that would take precedence over the project-wide one.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
 | [`parallel`](#parallel)                            | How many instances of a job should be run in parallel.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  |
 | [`trigger`](#trigger-premium)                      | Defines a downstream pipeline trigger.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  |
 | [`include`](#include)                              | Allows this job to include external YAML files. Also available: `include:local`, `include:file`, `include:template`, and `include:remote`.                                                                                                                                                                                                                                                                                                                                                                                                                                              |
@@ -1995,6 +1996,20 @@ Possible values for `when` are:
 - `missing_dependency_failure`: Retry if a dependency was missing.
 - `runner_unsupported`: Retry if the runner was unsupported.
 
+### timeout
+
+`timeout` allows you to configure a timeout for a specific job:
+
+```yaml
+build:
+  script: build.sh
+  timeout: 3 hours 30 minutes
+
+test:
+  script: rspec
+  timeout: 3h 30m
+```
+
 ### `parallel`
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/22631) in GitLab 11.5.

lib/gitlab/ci/config/entry/job.rb

@@ -14,8 +14,8 @@ module Gitlab
           ALLOWED_WHEN = %w[on_success on_failure always manual delayed].freeze
           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 interruptible].freeze
+                            dependencies before_script needs after_script variables
+                            environment coverage retry parallel extends interruptible timeout].freeze
 
           REQUIRED_BY_NEEDS = %i[stage].freeze
 
@@ -46,6 +46,8 @@ module Gitlab
                 message: "should be one of: #{ALLOWED_WHEN.join(', ')}"
               }
 
+              validates :timeout, duration: { limit: ChronicDuration.output(Project::MAX_BUILD_TIMEOUT) }
+
               validates :dependencies, array_of_strings: true
               validates :needs, array_of_strings: true
               validates :extends, array_of_strings_or_string: true
@@ -127,7 +129,7 @@ module Gitlab
 
           attributes :script, :tags, :allow_failure, :when, :dependencies,
                      :needs, :retry, :parallel, :extends, :start_in, :rules,
-                     :interruptible
+                     :interruptible, :timeout
 
           def self.matching?(name, config)
             !name.to_s.start_with?('.') &&
@@ -218,6 +220,7 @@ module Gitlab
               retry: retry_defined? ? retry_value : nil,
               parallel: parallel_defined? ? parallel_value.to_i : nil,
               interruptible: interruptible_defined? ? interruptible_value : nil,
+              timeout: has_timeout? ? ChronicDuration.parse(timeout.to_s) : nil,
               artifacts: artifacts_value,
               after_script: after_script_value,
               ignore: ignored?,

lib/gitlab/ci/yaml_processor.rb

@@ -49,6 +49,7 @@ module Gitlab
             artifacts: job[:artifacts],
             cache: job[:cache],
             dependencies: job[:dependencies],
+            job_timeout: job[:timeout],
             before_script: job[:before_script],
             script: job[:script],
             after_script: job[:after_script],

spec/lib/gitlab/ci/build/step_spec.rb

@@ -4,31 +4,29 @@ require 'spec_helper'
 
 describe Gitlab::Ci::Build::Step do
   describe '#from_commands' do
-    shared_examples 'has correct script' do
     subject { described_class.from_commands(job) }
 
     before do
       job.run!
     end
 
+    shared_examples 'has correct script' do
       it 'fabricates an object' do
         expect(subject.name).to eq(:script)
         expect(subject.script).to eq(script)
-        expect(subject.timeout).to eq(job.metadata_timeout)
         expect(subject.when).to eq('on_success')
         expect(subject.allow_failure).to be_falsey
       end
     end
 
     context 'when script option is specified' do
-      it_behaves_like 'has correct script' do
       let(:job) { create(:ci_build, :no_options, options: { script: ["ls -la\necho aaa", "date"] }) }
       let(:script) { ["ls -la\necho aaa", 'date'] }
-      end
+
+      it_behaves_like 'has correct script'
     end
 
     context 'when before and script option is specified' do
-      it_behaves_like 'has correct script' do
       let(:job) do
         create(:ci_build, options: {
           before_script: ["ls -la\necho aaa"],
@@ -37,6 +35,18 @@ describe Gitlab::Ci::Build::Step do
       end
 
       let(:script) { ["ls -la\necho aaa", 'date'] }
+
+      it_behaves_like 'has correct script'
+    end
+
+    context 'when timeout option is specified in seconds' do
+      let(:job) { create(:ci_build, options: { job_timeout: 3, script: ["ls -la\necho aaa", 'date'] }) }
+      let(:script) { ["ls -la\necho aaa", 'date'] }
+
+      it_behaves_like 'has correct script'
+
+      it 'has job level timeout' do
+        expect(subject.timeout).to eq(3)
       end
     end
   end
@@ -57,12 +67,12 @@ describe Gitlab::Ci::Build::Step do
     end
 
     context 'when after_script is not empty' do
-      let(:job) { create(:ci_build, options: { script: ['bash'], after_script: ['ls -la', 'date'] }) }
+      let(:job) { create(:ci_build, options: { job_timeout: 60, script: ['bash'], after_script: ['ls -la', 'date'] }) }
 
       it 'fabricates an object' do
         expect(subject.name).to eq(:after_script)
         expect(subject.script).to eq(['ls -la', 'date'])
-        expect(subject.timeout).to eq(job.metadata_timeout)
+        expect(subject.timeout).to eq(60)
         expect(subject.when).to eq('always')
         expect(subject.allow_failure).to be_truthy
       end

spec/lib/gitlab/ci/config/entry/job_spec.rb

@@ -417,6 +417,37 @@ describe Gitlab::Ci::Config::Entry::Job do
           end
         end
       end
+
+      context 'when timeout value is not correct' do
+        context 'when it is higher than instance wide timeout' do
+          let(:config) { { timeout: '3 months' } }
+
+          it 'returns error about value too high' do
+            expect(entry).not_to be_valid
+            expect(entry.errors)
+              .to include "job timeout should not exceed the limit"
+          end
+        end
+
+        context 'when it is not a duration' do
+          let(:config) { { timeout: 100 } }
+
+          it 'returns error about wrong value' do
+            expect(entry).not_to be_valid
+            expect(entry.errors).to include 'job timeout should be a duration'
+          end
+        end
+      end
+
+      context 'when timeout value is correct' do
+        let(:config) { { script: 'echo', timeout: '1m 1s' } }
+
+        it 'returns correct timeout' do
+          expect(entry).to be_valid
+          expect(entry.errors).to be_empty
+          expect(entry.timeout).to eq('1m 1s')
+        end
+      end
     end
   end
 

spec/lib/gitlab/ci/yaml_processor_spec.rb

@@ -1134,6 +1134,48 @@ module Gitlab
         end
       end
 
+      describe "Timeout" do
+        let(:config) do
+          {
+            deploy_to_production: {
+              stage:  'deploy',
+              script: 'test'
+            }
+          }
+        end
+
+        let(:processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
+        let(:builds) { processor.stage_builds_attributes('deploy') }
+
+        context 'when no timeout was provided' do
+          it 'does not include job_timeout' do
+            expect(builds.size).to eq(1)
+            expect(builds.first[:options]).not_to include(:job_timeout)
+          end
+        end
+
+        context 'when an invalid timeout was provided' do
+          before do
+            config[:deploy_to_production][:timeout] = 'not-a-number'
+          end
+
+          it 'raises an error for invalid number' do
+            expect { builds }.to raise_error('jobs:deploy_to_production timeout should be a duration')
+          end
+        end
+
+        context 'when some valid timeout was provided' do
+          before do
+            config[:deploy_to_production][:timeout] = '1m 3s'
+          end
+
+          it 'returns provided timeout value' do
+            expect(builds.size).to eq(1)
+            expect(builds.first[:options]).to include(job_timeout: 63)
+          end
+        end
+      end
+
       describe "Dependencies" do
         let(:config) do
           {

spec/models/ci/build_metadata_spec.rb

@@ -22,42 +22,72 @@ describe Ci::BuildMetadata do
   describe '#update_timeout_state' do
     subject { metadata }
 
-    context 'when runner is not assigned to the job' do
-      it "doesn't change timeout value" do
-        expect { subject.update_timeout_state }.not_to change { subject.reload.timeout }
+    shared_examples 'sets timeout' do |source, timeout|
+      it 'sets project_timeout_source' do
+        expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to(source)
       end
 
-      it "doesn't change timeout_source value" do
-        expect { subject.update_timeout_state }.not_to change { subject.reload.timeout_source }
+      it 'sets project timeout' do
+        expect { subject.update_timeout_state }.to change { subject.reload.timeout }.to(timeout)
       end
     end
 
+    context 'when project timeout is set' do
       context 'when runner is assigned to the job' do
         before do
-        build.update(runner: runner)
+          build.update!(runner: runner)
+        end
+
+        context 'when runner timeout is not set' do
+          let(:runner) { create(:ci_runner, maximum_timeout: nil) }
+
+          it_behaves_like 'sets timeout', 'project_timeout_source', 2000
         end
 
         context 'when runner timeout is lower than project timeout' do
           let(:runner) { create(:ci_runner, maximum_timeout: 1900) }
 
-        it 'sets runner timeout' do
-          expect { subject.update_timeout_state }.to change { subject.reload.timeout }.to(1900)
+          it_behaves_like 'sets timeout', 'runner_timeout_source', 1900
         end
 
-        it 'sets runner_timeout_source' do
-          expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('runner_timeout_source')
+        context 'when runner timeout is higher than project timeout' do
+          let(:runner) { create(:ci_runner, maximum_timeout: 2100) }
+
+          it_behaves_like 'sets timeout', 'project_timeout_source', 2000
         end
       end
 
-      context 'when runner timeout is higher than project timeout' do
+      context 'when job timeout is set' do
+        context 'when job timeout is higher than project timeout' do
+          let(:build) { create(:ci_build, pipeline: pipeline, options: { job_timeout: 3000 }) }
+
+          it_behaves_like 'sets timeout', 'job_timeout_source', 3000
+        end
+
+        context 'when job timeout is lower than project timeout' do
+          let(:build) { create(:ci_build, pipeline: pipeline, options: { job_timeout: 1000 }) }
+
+          it_behaves_like 'sets timeout', 'job_timeout_source', 1000
+        end
+      end
+
+      context 'when both runner and job timeouts are set' do
+        before do
+          build.update(runner: runner)
+        end
+
+        context 'when job timeout is higher than runner timeout' do
+          let(:build) { create(:ci_build, pipeline: pipeline, options: { job_timeout: 3000 }) }
           let(:runner) { create(:ci_runner, maximum_timeout: 2100) }
 
-        it 'sets project timeout' do
-          expect { subject.update_timeout_state }.to change { subject.reload.timeout }.to(2000)
+          it_behaves_like 'sets timeout', 'runner_timeout_source', 2100
         end
 
-        it 'sets project_timeout_source' do
-          expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('project_timeout_source')
+        context 'when job timeout is lower than runner timeout' do
+          let(:build) { create(:ci_build, pipeline: pipeline, options: { job_timeout: 1900 }) }
+          let(:runner) { create(:ci_runner, maximum_timeout: 2100) }
+
+          it_behaves_like 'sets timeout', 'job_timeout_source', 1900
         end
       end
     end

spec/services/ci/create_pipeline_service_spec.rb

@@ -795,6 +795,22 @@ describe Ci::CreatePipelineService do
       end
     end
 
+    context 'with timeout' do
+      context 'when builds with custom timeouts are configured' do
+        before do
+          config = YAML.dump(rspec: { script: 'rspec', timeout: '2m 3s' })
+          stub_ci_pipeline_yaml_file(config)
+        end
+
+        it 'correctly creates builds with custom timeout value configured' do
+          pipeline = execute_service
+
+          expect(pipeline).to be_persisted
+          expect(pipeline.builds.find_by(name: 'rspec').options[:job_timeout]).to eq 123
+        end
+      end
+    end
+
     shared_examples 'when ref is protected' do
       let(:user) { create(:user) }