diff options
author | Kamil Trzciński <ayufan@ayufan.eu> | 2018-03-29 10:54:06 +0000 |
---|---|---|
committer | Kamil Trzciński <ayufan@ayufan.eu> | 2018-03-29 10:54:06 +0000 |
commit | 8b37ce6f7f6d29604c42c65f3986d60dce0abd6c (patch) | |
tree | 1bb0959f49edd0980a2336923c6c5399122bf99a | |
parent | b2ccfc084d790d012f43b8f5ffeaaee4c913a08c (diff) | |
parent | 6ecde0076afa83e30608ea9caba924bbab66a123 (diff) | |
download | gitlab-ce-8b37ce6f7f6d29604c42c65f3986d60dce0abd6c.tar.gz |
Merge branch 'add-per-runner-job-timeout' into 'master'
Add per runner job timeout
Closes #43426
See merge request gitlab-org/gitlab-ce!17221
35 files changed, 645 insertions, 30 deletions
diff --git a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue index a6819aaeb12..dfe87d89a39 100644 --- a/app/assets/javascripts/jobs/components/sidebar_detail_row.vue +++ b/app/assets/javascripts/jobs/components/sidebar_detail_row.vue @@ -11,11 +11,19 @@ type: String, required: true, }, + helpUrl: { + type: String, + required: false, + default: '', + }, }, computed: { hasTitle() { return this.title.length > 0; }, + hasHelpURL() { + return this.helpUrl.length > 0; + }, }, }; </script> @@ -28,5 +36,21 @@ {{ title }}: </span> {{ value }} + + <span + v-if="hasHelpURL" + class="help-button pull-right" + > + <a + :href="helpUrl" + target="_blank" + rel="noopener noreferrer nofollow" + > + <i + class="fa fa-question-circle" + aria-hidden="true" + ></i> + </a> + </span> </p> </template> diff --git a/app/assets/javascripts/jobs/components/sidebar_details_block.vue b/app/assets/javascripts/jobs/components/sidebar_details_block.vue index 56814a52525..172de6b3679 100644 --- a/app/assets/javascripts/jobs/components/sidebar_details_block.vue +++ b/app/assets/javascripts/jobs/components/sidebar_details_block.vue @@ -22,6 +22,11 @@ type: Boolean, required: true, }, + runnerHelpUrl: { + type: String, + required: false, + default: '', + }, }, computed: { shouldRenderContent() { @@ -39,6 +44,21 @@ runnerId() { return `#${this.job.runner.id}`; }, + hasTimeout() { + return this.job.metadata != null && this.job.metadata.timeout_human_readable !== ''; + }, + timeout() { + if (this.job.metadata == null) { + return ''; + } + + let t = this.job.metadata.timeout_human_readable; + if (this.job.metadata.timeout_source !== '') { + t += ` (from ${this.job.metadata.timeout_source})`; + } + + return t; + }, renderBlock() { return this.job.merge_request || this.job.duration || @@ -115,6 +135,13 @@ :value="queued" /> <detail-row + class="js-job-timeout" + v-if="hasTimeout" + title="Timeout" + :help-url="runnerHelpUrl" + :value="timeout" + /> + <detail-row class="js-job-runner" v-if="job.runner" title="Runner" diff --git a/app/assets/javascripts/jobs/job_details_bundle.js b/app/assets/javascripts/jobs/job_details_bundle.js index 85a88ae409b..656676ead91 100644 --- a/app/assets/javascripts/jobs/job_details_bundle.js +++ b/app/assets/javascripts/jobs/job_details_bundle.js @@ -51,6 +51,7 @@ export default () => { props: { isLoading: this.mediator.state.isLoading, job: this.mediator.store.state.job, + runnerHelpUrl: dataset.runnerHelpUrl, }, }); }, diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 08bb5915d10..ed02af05e3d 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -24,6 +24,10 @@ module Ci has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id has_one :job_artifacts_trace, -> { where(file_type: Ci::JobArtifact.file_types[:trace]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :metadata, class_name: 'Ci::BuildMetadata' + + delegate :timeout, to: :metadata, prefix: true, allow_nil: true + # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( @@ -153,6 +157,14 @@ module Ci before_transition any => [:running] do |build| build.validates_dependencies! unless Feature.enabled?('ci_disable_validates_dependencies') end + + before_transition pending: :running do |build| + build.ensure_metadata.update_timeout_state + end + end + + def ensure_metadata + metadata || build_metadata(project: project) end def detailed_status(current_user) @@ -231,10 +243,6 @@ module Ci latest_builds.where('stage_idx < ?', stage_idx) end - def timeout - project.build_timeout - end - def triggered_by?(current_user) user == current_user end diff --git a/app/models/ci/build_metadata.rb b/app/models/ci/build_metadata.rb new file mode 100644 index 00000000000..96762f8845c --- /dev/null +++ b/app/models/ci/build_metadata.rb @@ -0,0 +1,35 @@ +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 < ActiveRecord::Base + extend Gitlab::Ci::Model + include Presentable + include ChronicDurationAttribute + + self.table_name = 'ci_builds_metadata' + + belongs_to :build, class_name: 'Ci::Build' + belongs_to :project + + validates :build, presence: true + validates :project, presence: true + + chronic_duration_attr_reader :timeout_human_readable, :timeout + + enum timeout_source: { + unknown_timeout_source: 1, + project_timeout_source: 2, + runner_timeout_source: 3 + } + + def update_timeout_state + return unless build.runner.present? + + 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 + + update(timeout: timeout, timeout_source: timeout_source) + end + end +end diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index 7173f88f1c7..5a4c56ec0dc 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -3,12 +3,13 @@ module Ci extend Gitlab::Ci::Model include Gitlab::SQL::Pattern include RedisCacheable + include ChronicDurationAttribute RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour UPDATE_DB_RUNNER_INFO_EVERY = 40.minutes AVAILABLE_SCOPES = %w[specific shared active paused online].freeze - FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level].freeze + FORM_EDITABLE = %i[description tag_list active run_untagged locked access_level maximum_timeout_human_readable].freeze has_many :builds has_many :runner_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -51,6 +52,12 @@ module Ci cached_attr_reader :version, :revision, :platform, :architecture, :contacted_at, :ip_address + chronic_duration_attr :maximum_timeout_human_readable, :maximum_timeout + + validates :maximum_timeout, allow_nil: true, + numericality: { greater_than_or_equal_to: 600, + message: 'needs to be at least 10 minutes' } + # Searches for runners matching the given query. # # This method uses ILIKE on PostgreSQL and LIKE on MySQL. diff --git a/app/models/concerns/chronic_duration_attribute.rb b/app/models/concerns/chronic_duration_attribute.rb new file mode 100644 index 00000000000..fa1eafb1d7a --- /dev/null +++ b/app/models/concerns/chronic_duration_attribute.rb @@ -0,0 +1,39 @@ +module ChronicDurationAttribute + extend ActiveSupport::Concern + + class_methods do + def chronic_duration_attr_reader(virtual_attribute, source_attribute) + define_method(virtual_attribute) do + chronic_duration_attributes[virtual_attribute] || output_chronic_duration_attribute(source_attribute) + end + end + + def chronic_duration_attr_writer(virtual_attribute, source_attribute) + chronic_duration_attr_reader(virtual_attribute, source_attribute) + + define_method("#{virtual_attribute}=") do |value| + chronic_duration_attributes[virtual_attribute] = value.presence || '' + + begin + new_value = ChronicDuration.parse(value).to_i if value.present? + assign_attributes(source_attribute => new_value) + rescue ChronicDuration::DurationParseError + # ignore error as it will be caught by validation + end + end + + validates virtual_attribute, allow_nil: true, duration: true + end + + alias_method :chronic_duration_attr, :chronic_duration_attr_writer + end + + def chronic_duration_attributes + @chronic_duration_attributes ||= {} + end + + def output_chronic_duration_attribute(source_attribute) + value = attributes[source_attribute.to_s] + ChronicDuration.output(value, format: :short) if value + end +end diff --git a/app/presenters/ci/build_metadata_presenter.rb b/app/presenters/ci/build_metadata_presenter.rb new file mode 100644 index 00000000000..5048f967ea8 --- /dev/null +++ b/app/presenters/ci/build_metadata_presenter.rb @@ -0,0 +1,18 @@ +module Ci + class BuildMetadataPresenter < Gitlab::View::Presenter::Delegated + TIMEOUT_SOURCES = { + unknown_timeout_source: nil, + project_timeout_source: 'project', + runner_timeout_source: 'runner' + }.freeze + + presents :metadata + + def timeout_source + return unless metadata.timeout_source? + + TIMEOUT_SOURCES[metadata.timeout_source.to_sym] || + metadata.timeout_source + end + end +end diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb index 69d46f5ec14..ca4480fe2b1 100644 --- a/app/serializers/build_details_entity.rb +++ b/app/serializers/build_details_entity.rb @@ -5,6 +5,8 @@ class BuildDetailsEntity < JobEntity expose :runner, using: RunnerEntity expose :pipeline, using: PipelineEntity + expose :metadata, using: BuildMetadataEntity + expose :erased_by, if: -> (*) { build.erased? }, using: UserEntity expose :erase_path, if: -> (*) { build.erasable? && can?(current_user, :erase_build, build) } do |build| erase_project_job_path(project, build) diff --git a/app/serializers/build_metadata_entity.rb b/app/serializers/build_metadata_entity.rb new file mode 100644 index 00000000000..39f429aa6c3 --- /dev/null +++ b/app/serializers/build_metadata_entity.rb @@ -0,0 +1,9 @@ +class BuildMetadataEntity < Grape::Entity + expose :timeout_human_readable do |metadata| + metadata.timeout_human_readable unless metadata.timeout.nil? + end + + expose :timeout_source do |metadata| + metadata.present.timeout_source + end +end diff --git a/app/views/projects/jobs/show.html.haml b/app/views/projects/jobs/show.html.haml index 849c273db8c..fa27ded7cc2 100644 --- a/app/views/projects/jobs/show.html.haml +++ b/app/views/projects/jobs/show.html.haml @@ -111,4 +111,4 @@ .js-build-options{ data: javascript_build_options } -#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json) } } +#js-job-details-vue{ data: { endpoint: project_job_path(@project, @build, format: :json), runner_help_url: help_page_path('ci/runners/README.html', anchor: 'setting-maximum-job-timeout-for-a-runner') } } diff --git a/app/views/projects/runners/_form.html.haml b/app/views/projects/runners/_form.html.haml index 49c90869146..6a681736b6f 100644 --- a/app/views/projects/runners/_form.html.haml +++ b/app/views/projects/runners/_form.html.haml @@ -40,6 +40,12 @@ .col-sm-10 = f.text_field :description, class: 'form-control' .form-group + = label_tag :maximum_timeout_human_readable, class: 'control-label' do + Maximum job timeout + .col-sm-10 + = f.text_field :maximum_timeout_human_readable, class: 'form-control' + .help-block This timeout will take precedence when lower than Project-defined timeout + .form-group = label_tag :tag_list, class: 'control-label' do Tags .col-sm-10 diff --git a/app/views/projects/runners/show.html.haml b/app/views/projects/runners/show.html.haml index 4e57f5f844d..f33e7e25b68 100644 --- a/app/views/projects/runners/show.html.haml +++ b/app/views/projects/runners/show.html.haml @@ -56,6 +56,9 @@ %td Description %td= @runner.description %tr + %td Maximum job timeout + %td= @runner.maximum_timeout_human_readable + %tr %td Last contact %td - if @runner.contacted_at diff --git a/changelogs/unreleased/add-per-runner-job-timeout.yml b/changelogs/unreleased/add-per-runner-job-timeout.yml new file mode 100644 index 00000000000..336b4d15ddf --- /dev/null +++ b/changelogs/unreleased/add-per-runner-job-timeout.yml @@ -0,0 +1,5 @@ +--- +title: Add per-runner configured job timeout +merge_request: 17221 +author: +type: added diff --git a/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb b/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb new file mode 100644 index 00000000000..072e696a43e --- /dev/null +++ b/db/migrate/20180219153455_add_maximum_timeout_to_ci_runners.rb @@ -0,0 +1,9 @@ +class AddMaximumTimeoutToCiRunners < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column :ci_runners, :maximum_timeout, :integer + end +end diff --git a/db/migrate/20180301010859_create_ci_builds_metadata_table.rb b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb new file mode 100644 index 00000000000..ce737444092 --- /dev/null +++ b/db/migrate/20180301010859_create_ci_builds_metadata_table.rb @@ -0,0 +1,20 @@ +class CreateCiBuildsMetadataTable < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + create_table :ci_builds_metadata do |t| + t.integer :build_id, null: false + t.integer :project_id, null: false + t.integer :timeout + t.integer :timeout_source, null: false, default: 1 + + t.foreign_key :ci_builds, column: :build_id, on_delete: :cascade + t.foreign_key :projects, column: :project_id, on_delete: :cascade + + t.index :build_id, unique: true + t.index :project_id + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 77b3d836287..9aaefcf1c8d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -329,6 +329,16 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree + create_table "ci_builds_metadata", force: :cascade do |t| + t.integer "build_id", null: false + t.integer "project_id", null: false + t.integer "timeout" + t.integer "timeout_source", default: 1, null: false + end + + add_index "ci_builds_metadata", ["build_id"], name: "index_ci_builds_metadata_on_build_id", unique: true, using: :btree + add_index "ci_builds_metadata", ["project_id"], name: "index_ci_builds_metadata_on_project_id", using: :btree + create_table "ci_group_variables", force: :cascade do |t| t.string "key", null: false t.text "value" @@ -459,6 +469,7 @@ ActiveRecord::Schema.define(version: 20180327101207) do t.boolean "locked", default: false, null: false t.integer "access_level", default: 0, null: false t.string "ip_address" + t.integer "maximum_timeout" end add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree @@ -2027,6 +2038,8 @@ ActiveRecord::Schema.define(version: 20180327101207) do add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade + add_foreign_key "ci_builds_metadata", "ci_builds", column: "build_id", on_delete: :cascade + add_foreign_key "ci_builds_metadata", "projects", on_delete: :cascade add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade add_foreign_key "ci_job_artifacts", "ci_builds", column: "job_id", on_delete: :cascade add_foreign_key "ci_job_artifacts", "projects", on_delete: :cascade diff --git a/doc/api/runners.md b/doc/api/runners.md index 7495c6cdedb..f384ac57bfe 100644 --- a/doc/api/runners.md +++ b/doc/api/runners.md @@ -153,7 +153,8 @@ Example response: "mysql" ], "version": null, - "access_level": "ref_protected" + "access_level": "ref_protected", + "maximum_timeout": 3600 } ``` @@ -174,6 +175,7 @@ PUT /runners/:id | `run_untagged` | boolean | no | Flag indicating the runner can execute untagged jobs | | `locked` | boolean | no | Flag indicating the runner is locked | | `access_level` | string | no | The access_level of the runner; `not_protected` or `ref_protected` | +| `maximum_timeout` | integer | no | Maximum timeout set when this Runner will handle the job | ``` curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/runners/6" --form "description=test-1-20150125-test" --form "tag_list=ruby,mysql,tag1,tag2" @@ -211,7 +213,8 @@ Example response: "tag2" ], "version": null, - "access_level": "ref_protected" + "access_level": "ref_protected", + "maximum_timeout": null } ``` diff --git a/doc/ci/runners/README.md b/doc/ci/runners/README.md index 7a7b50b294d..b91aa334ff3 100644 --- a/doc/ci/runners/README.md +++ b/doc/ci/runners/README.md @@ -231,6 +231,38 @@ To make a Runner pick tagged/untagged jobs: 1. Check the **Run untagged jobs** option 1. Click **Save changes** for the changes to take effect +### Setting maximum job timeout for a Runner + +For each Runner you can specify a _maximum job timeout_. Such timeout, +if smaller than [project defined timeout], will take the precedence. This +feature can be used to prevent Shared Runner from being appropriated +by a project by setting a ridiculous big timeout (e.g. one week). + +When not configured, Runner will not override project timeout. + +How this feature will work: + +**Example 1 - Runner timeout bigger than project timeout** + +1. You set the _maximum job timeout_ for a Runner to 24 hours +1. You set the _CI/CD Timeout_ for a project to **2 hours** +1. You start a job +1. The job, if running longer, will be timeouted after **2 hours** + +**Example 2 - Runner timeout not configured** + +1. You remove the _maximum job timeout_ configuration from a Runner +1. You set the _CI/CD Timeout_ for a project to **2 hours** +1. You start a job +1. The job, if running longer, will be timeouted after **2 hours** + +**Example 3 - Runner timeout smaller than project timeout** + +1. You set the _maximum job timeout_ for a Runner to **30 minutes** +1. You set the _CI/CD Timeout_ for a project to 2 hours +1. You start a job +1. The job, if running longer, will be timeouted after **30 minutes** + ### Be careful with sensitive information With some [Runner Executors](https://docs.gitlab.com/runner/executors/README.html), @@ -259,12 +291,6 @@ Mentioned briefly earlier, but the following things of Runners can be exploited. We're always looking for contributions that can mitigate these [Security Considerations](https://docs.gitlab.com/runner/security/). -[install]: http://docs.gitlab.com/runner/install/ -[fifo]: https://en.wikipedia.org/wiki/FIFO_(computing_and_electronics) -[register]: http://docs.gitlab.com/runner/register/ -[protected branches]: ../../user/project/protected_branches.md -[protected tags]: ../../user/project/protected_tags.md - ## Determining the IP address of a Runner > [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17286) in GitLab 10.6. @@ -297,3 +323,10 @@ You can find the IP address of a Runner for a specific project by: 1. On the details page you should see a row for "IP Address" ![specific Runner IP address](img/specific_runner_ip_address.png) + +[install]: http://docs.gitlab.com/runner/install/ +[fifo]: https://en.wikipedia.org/wiki/FIFO_(computing_and_electronics) +[register]: http://docs.gitlab.com/runner/register/ +[protected branches]: ../../user/project/protected_branches.md +[protected tags]: ../../user/project/protected_tags.md +[project defined timeout]: ../../user/project/pipelines/settings.html#timeout diff --git a/doc/user/project/pipelines/settings.md b/doc/user/project/pipelines/settings.md index 43451844f2d..1052c9efa25 100644 --- a/doc/user/project/pipelines/settings.md +++ b/doc/user/project/pipelines/settings.md @@ -27,6 +27,13 @@ The default value is 60 minutes. Decrease the time limit if you want to impose a hard limit on your jobs' running time or increase it otherwise. In any case, if the job surpasses the threshold, it is marked as failed. +### Timeout overriding on Runner level + +> - [Introduced][ce-17221] in GitLab 10.6. + +Project defined timeout (either specific timeout set by user or the default +60 minutes timeout) may be [overridden on Runner level][timeout overriding]. + ## Custom CI config path > - [Introduced][ce-12509] in GitLab 9.4. @@ -152,5 +159,7 @@ into your `README.md`: [var]: ../../../ci/yaml/README.md#git-strategy [coverage report]: #test-coverage-parsing +[timeout overriding]: ../../../ci/runners/README.html#setting-maximum-job-timeout-for-a-runner [ce-9362]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9362 [ce-12509]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12509 +[ce-17221]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17221 diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 16147ee90c9..38161d1f127 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -951,6 +951,7 @@ module API expose :tag_list expose :run_untagged expose :locked + expose :maximum_timeout expose :access_level expose :version, :revision, :platform, :architecture expose :contacted_at @@ -1119,7 +1120,7 @@ module API end class RunnerInfo < Grape::Entity - expose :timeout + expose :metadata_timeout, as: :timeout end class Step < Grape::Entity diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 8da97a97754..57c0a729535 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -14,9 +14,10 @@ module API optional :locked, type: Boolean, desc: 'Should Runner be locked for current project' optional :run_untagged, type: Boolean, desc: 'Should Runner handle untagged jobs' optional :tag_list, type: Array[String], desc: %q(List of Runner's tags) + optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' end post '/' do - attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list]) + attributes = attributes_for_keys([:description, :locked, :run_untagged, :tag_list, :maximum_timeout]) .merge(get_runner_details_from_request) runner = diff --git a/lib/api/runners.rb b/lib/api/runners.rb index 996457c5dfe..5f2a9567605 100644 --- a/lib/api/runners.rb +++ b/lib/api/runners.rb @@ -57,6 +57,7 @@ module API optional :locked, type: Boolean, desc: 'Flag indicating the runner is locked' optional :access_level, type: String, values: Ci::Runner.access_levels.keys, desc: 'The access_level of the runner' + optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job' at_least_one_of :description, :active, :tag_list, :run_untagged, :locked, :access_level end put ':id' do diff --git a/lib/gitlab/ci/build/step.rb b/lib/gitlab/ci/build/step.rb index 411f67f8ce7..0b1ebe4e048 100644 --- a/lib/gitlab/ci/build/step.rb +++ b/lib/gitlab/ci/build/step.rb @@ -14,7 +14,7 @@ module Gitlab self.new(:script).tap do |step| step.script = job.options[:before_script].to_a + job.options[:script].to_a step.script = job.commands.split("\n") if step.script.empty? - step.timeout = job.timeout + step.timeout = job.metadata_timeout step.when = WHEN_ON_SUCCESS end end @@ -25,7 +25,7 @@ module Gitlab self.new(:after_script).tap do |step| step.script = after_script - step.timeout = job.timeout + step.timeout = job.metadata_timeout step.when = WHEN_ALWAYS step.allow_failure = true end diff --git a/spec/factories/ci/build_metadata.rb b/spec/factories/ci/build_metadata.rb new file mode 100644 index 00000000000..66bbd977b88 --- /dev/null +++ b/spec/factories/ci/build_metadata.rb @@ -0,0 +1,9 @@ +FactoryBot.define do + factory :ci_build_metadata, class: Ci::BuildMetadata do + build factory: :ci_build + + after(:build) do |build_metadata, _| + build_metadata.project ||= build_metadata.build.project + end + end +end diff --git a/spec/javascripts/jobs/mock_data.js b/spec/javascripts/jobs/mock_data.js index 43589d54be4..25ca8eb6c0b 100644 --- a/spec/javascripts/jobs/mock_data.js +++ b/spec/javascripts/jobs/mock_data.js @@ -115,6 +115,10 @@ export default { commit_path: '/root/ci-mock/commit/c58647773a6b5faf066d4ad6ff2c9fbba5f180f6', }, }, + metadata: { + timeout_human_readable: '1m 40s', + timeout_source: 'runner', + }, merge_request: { iid: 2, path: '/root/ci-mock/merge_requests/2', diff --git a/spec/javascripts/jobs/sidebar_detail_row_spec.js b/spec/javascripts/jobs/sidebar_detail_row_spec.js index 3ac65709c4a..e6bfb0c4adc 100644 --- a/spec/javascripts/jobs/sidebar_detail_row_spec.js +++ b/spec/javascripts/jobs/sidebar_detail_row_spec.js @@ -37,4 +37,25 @@ describe('Sidebar detail row', () => { vm.$el.textContent.replace(/\s+/g, ' ').trim(), ).toEqual('this is the title: this is the value'); }); + + describe('when helpUrl not provided', () => { + it('should not render help', () => { + expect(vm.$el.querySelector('.help-button')).toBeNull(); + }); + }); + + describe('when helpUrl provided', () => { + beforeEach(() => { + vm = new SidebarDetailRow({ + propsData: { + helpUrl: 'help url', + value: 'foo', + }, + }).$mount(); + }); + + it('should render help', () => { + expect(vm.$el.querySelector('.help-button a').getAttribute('href')).toEqual('help url'); + }); + }); }); diff --git a/spec/javascripts/jobs/sidebar_details_block_spec.js b/spec/javascripts/jobs/sidebar_details_block_spec.js index 95532ef5382..602dae514b1 100644 --- a/spec/javascripts/jobs/sidebar_details_block_spec.js +++ b/spec/javascripts/jobs/sidebar_details_block_spec.js @@ -96,6 +96,12 @@ describe('Sidebar details block', () => { ).toEqual('Runner: #1'); }); + it('should render timeout information', () => { + expect( + trimWhitespace(vm.$el.querySelector('.js-job-timeout')), + ).toEqual('Timeout: 1m 40s (from runner)'); + }); + it('should render coverage', () => { expect( trimWhitespace(vm.$el.querySelector('.js-job-coverage')), diff --git a/spec/lib/gitlab/ci/build/step_spec.rb b/spec/lib/gitlab/ci/build/step_spec.rb index 5a21282712a..cce4efaa069 100644 --- a/spec/lib/gitlab/ci/build/step_spec.rb +++ b/spec/lib/gitlab/ci/build/step_spec.rb @@ -5,10 +5,14 @@ describe Gitlab::Ci::Build::Step do shared_examples 'has correct script' do subject { described_class.from_commands(job) } + before do + job.run! + end + it 'fabricates an object' do expect(subject.name).to eq(:script) expect(subject.script).to eq(script) - expect(subject.timeout).to eq(job.timeout) + expect(subject.timeout).to eq(job.metadata_timeout) expect(subject.when).to eq('on_success') expect(subject.allow_failure).to be_falsey end @@ -47,6 +51,10 @@ describe Gitlab::Ci::Build::Step do subject { described_class.from_after_script(job) } + before do + job.run! + end + context 'when after_script is empty' do it 'doesn not fabricate an object' do is_expected.to be_nil @@ -59,7 +67,7 @@ describe Gitlab::Ci::Build::Step do 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.timeout) + expect(subject.timeout).to eq(job.metadata_timeout) expect(subject.when).to eq('always') expect(subject.allow_failure).to be_truthy end diff --git a/spec/models/ci/build_metadata_spec.rb b/spec/models/ci/build_metadata_spec.rb new file mode 100644 index 00000000000..268561ee941 --- /dev/null +++ b/spec/models/ci/build_metadata_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' + +describe Ci::BuildMetadata do + set(:user) { create(:user) } + set(:group) { create(:group, :access_requestable) } + set(:project) { create(:project, :repository, group: group, build_timeout: 2000) } + + set(:pipeline) do + create(:ci_pipeline, project: project, + sha: project.commit.id, + ref: project.default_branch, + status: 'success') + end + + let(:build) { create(:ci_build, pipeline: pipeline) } + let(:build_metadata) { create(:ci_build_metadata, build: build) } + + describe '#update_timeout_state' do + subject { build_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 } + end + + it "doesn't change timeout_source value" do + expect { subject.update_timeout_state }.not_to change { subject.reload.timeout_source } + end + end + + context 'when runner is assigned to the job' do + before do + build.update_attributes(runner: runner) + 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) + end + + it 'sets runner_timeout_source' do + expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('runner_timeout_source') + end + end + + context 'when runner timeout is higher than project timeout' do + 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) + end + + it 'sets project_timeout_source' do + expect { subject.update_timeout_state }.to change { subject.reload.timeout_source }.to('project_timeout_source') + end + end + end + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 7d935cf8d76..f5534d22a54 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1271,12 +1271,6 @@ describe Ci::Build do end describe 'project settings' do - describe '#timeout' do - it 'returns project timeout configuration' do - expect(build.timeout).to eq(project.build_timeout) - end - end - describe '#allow_git_fetch' do it 'return project allow_git_fetch configuration' do expect(build.allow_git_fetch).to eq(project.build_allow_git_fetch) @@ -2011,6 +2005,70 @@ describe Ci::Build do end end + describe 'state transition: pending: :running' do + let(:runner) { create(:ci_runner) } + let(:job) { create(:ci_build, :pending, runner: runner) } + + before do + job.project.update_attribute(:build_timeout, 1800) + end + + def run_job_without_exception + job.run! + rescue StateMachines::InvalidTransition + end + + shared_examples 'saves data on transition' do + it 'saves timeout' do + expect { job.run! }.to change { job.reload.ensure_metadata.timeout }.from(nil).to(expected_timeout) + end + + it 'saves timeout_source' do + expect { job.run! }.to change { job.reload.ensure_metadata.timeout_source }.from('unknown_timeout_source').to(expected_timeout_source) + end + + context 'when Ci::BuildMetadata#update_timeout_state fails update' do + before do + allow_any_instance_of(Ci::BuildMetadata).to receive(:update_timeout_state).and_return(false) + end + + it "doesn't save timeout" do + expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout_source } + end + + it "doesn't save timeout_source" do + expect { run_job_without_exception }.not_to change { job.reload.ensure_metadata.timeout_source } + end + + it 'raises an exception' do + expect { job.run! }.to raise_error(StateMachines::InvalidTransition) + end + end + end + + context 'when runner timeout overrides project timeout' do + let(:expected_timeout) { 900 } + let(:expected_timeout_source) { 'runner_timeout_source' } + + before do + runner.update_attribute(:maximum_timeout, 900) + end + + it_behaves_like 'saves data on transition' + end + + context "when runner timeout doesn't override project timeout" do + let(:expected_timeout) { 1800 } + let(:expected_timeout_source) { 'project_timeout_source' } + + before do + runner.update_attribute(:maximum_timeout, 3600) + end + + it_behaves_like 'saves data on transition' + end + end + describe 'state transition: any => [:running]' do shared_examples 'validation is active' do context 'when depended job has not been completed yet' do diff --git a/spec/models/concerns/chronic_duration_attribute_spec.rb b/spec/models/concerns/chronic_duration_attribute_spec.rb new file mode 100644 index 00000000000..27c86e60e60 --- /dev/null +++ b/spec/models/concerns/chronic_duration_attribute_spec.rb @@ -0,0 +1,115 @@ +require 'spec_helper' + +shared_examples 'ChronicDurationAttribute reader' do + it 'contains dynamically created reader method' do + expect(subject.class).to be_public_method_defined(virtual_field) + end + + it 'outputs chronic duration formatted value' do + subject.send("#{source_field}=", 120) + + expect(subject.send(virtual_field)).to eq('2m') + end + + context 'when value is set to nil' do + it 'outputs nil' do + subject.send("#{source_field}=", nil) + + expect(subject.send(virtual_field)).to be_nil + end + end +end + +shared_examples 'ChronicDurationAttribute writer' do + it 'contains dynamically created writer method' do + expect(subject.class).to be_public_method_defined("#{virtual_field}=") + end + + before do + subject.send("#{virtual_field}=", '10m') + end + + it 'parses chronic duration input' do + expect(subject.send(source_field)).to eq(600) + end + + it 'passes validation' do + expect(subject.valid?).to be_truthy + end + + context 'when negative input is used' do + before do + subject.send("#{source_field}=", 3600) + end + + it "doesn't raise exception" do + expect { subject.send("#{virtual_field}=", '-10m') }.not_to raise_error(ChronicDuration::DurationParseError) + end + + it "doesn't change value" do + expect { subject.send("#{virtual_field}=", '-10m') }.not_to change { subject.send(source_field) } + end + + it "doesn't pass validation" do + subject.send("#{virtual_field}=", '-10m') + + expect(subject.valid?).to be_falsey + expect(subject.errors&.messages).to include(virtual_field => ['is not a correct duration']) + end + end + + context 'when empty input is used' do + before do + subject.send("#{virtual_field}=", '') + end + + it 'writes nil' do + expect(subject.send(source_field)).to be_nil + end + + it 'passes validation' do + expect(subject.valid?).to be_truthy + end + end + + context 'when nil input is used' do + before do + subject.send("#{virtual_field}=", nil) + end + + it 'writes nil' do + expect(subject.send(source_field)).to be_nil + end + + it 'passes validation' do + expect(subject.valid?).to be_truthy + end + + it "doesn't raise exception" do + expect { subject.send("#{virtual_field}=", nil) }.not_to raise_error(NoMethodError) + end + end +end + +describe 'ChronicDurationAttribute' do + let(:source_field) {:maximum_timeout} + let(:virtual_field) {:maximum_timeout_human_readable} + + subject { Ci::Runner.new } + + it_behaves_like 'ChronicDurationAttribute reader' + it_behaves_like 'ChronicDurationAttribute writer' +end + +describe 'ChronicDurationAttribute - reader' do + let(:source_field) {:timeout} + let(:virtual_field) {:timeout_human_readable} + + subject {Ci::BuildMetadata.new} + + it "doesn't contain dynamically created writer method" do + expect(subject.class).not_to be_public_method_defined("#{virtual_field}=") + end + + it_behaves_like 'ChronicDurationAttribute reader' +end diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index f3dd121faa9..5084b36c761 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -109,6 +109,26 @@ describe API::Runner do end end + context 'when maximum job timeout is specified' do + it 'creates runner' do + post api('/runners'), token: registration_token, + maximum_timeout: 9000 + + expect(response).to have_gitlab_http_status 201 + expect(Ci::Runner.first.maximum_timeout).to eq(9000) + end + + context 'when maximum job timeout is empty' do + it 'creates runner' do + post api('/runners'), token: registration_token, + maximum_timeout: '' + + expect(response).to have_gitlab_http_status 201 + expect(Ci::Runner.first.maximum_timeout).to be_nil + end + end + end + %w(name version revision platform architecture).each do |param| context "when info parameter '#{param}' info is present" do let(:value) { "#{param}_value" } @@ -340,12 +360,12 @@ describe API::Runner do let(:expected_steps) do [{ 'name' => 'script', 'script' => %w(ls date), - 'timeout' => job.timeout, + 'timeout' => job.metadata_timeout, 'when' => 'on_success', 'allow_failure' => false }, { 'name' => 'after_script', 'script' => %w(ls date), - 'timeout' => job.timeout, + 'timeout' => job.metadata_timeout, 'when' => 'always', 'allow_failure' => true }] end @@ -648,6 +668,41 @@ describe API::Runner do end end end + + describe 'timeout support' do + context 'when project specifies job timeout' do + let(:project) { create(:project, shared_runners_enabled: false, build_timeout: 1234) } + + it 'contains info about timeout taken from project' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1234 }) + end + + context 'when runner specifies lower timeout' do + let(:runner) { create(:ci_runner, maximum_timeout: 1000) } + + it 'contains info about timeout overridden by runner' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1000 }) + end + end + + context 'when runner specifies bigger timeout' do + let(:runner) { create(:ci_runner, maximum_timeout: 2000) } + + it 'contains info about timeout not overridden by runner' do + request_job + + expect(response).to have_gitlab_http_status(201) + expect(json_response['runner_info']).to include({ 'timeout' => 1234 }) + end + end + end + end end def request_job(token = runner.token, **params) diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb index ec5cad4f4fd..d30f0cf36e2 100644 --- a/spec/requests/api/runners_spec.rb +++ b/spec/requests/api/runners_spec.rb @@ -123,6 +123,7 @@ describe API::Runners do expect(response).to have_gitlab_http_status(200) expect(json_response['description']).to eq(shared_runner.description) + expect(json_response['maximum_timeout']).to be_nil end end @@ -192,7 +193,8 @@ describe API::Runners do tag_list: ['ruby2.1', 'pgsql', 'mysql'], run_untagged: 'false', locked: 'true', - access_level: 'ref_protected') + access_level: 'ref_protected', + maximum_timeout: 1234) shared_runner.reload expect(response).to have_gitlab_http_status(200) @@ -204,6 +206,7 @@ describe API::Runners do expect(shared_runner.ref_protected?).to be_truthy expect(shared_runner.ensure_runner_queue_value) .not_to eq(runner_queue_value) + expect(shared_runner.maximum_timeout).to eq(1234) end end diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index b86a3d72bb4..8de0bdf92e2 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -29,7 +29,8 @@ describe Ci::RetryBuildService do commit_id deployments erased_by_id last_deployment project_id runner_id tag_taggings taggings tags trigger_request_id user_id auto_canceled_by_id retried failure_reason - artifacts_file_store artifacts_metadata_store].freeze + artifacts_file_store artifacts_metadata_store + metadata].freeze shared_examples 'build duplication' do let(:another_pipeline) { create(:ci_empty_pipeline, project: project) } |