diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 0c9f69b6714..9a1c2a4c9e1 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -115,6 +115,7 @@ module ApplicationSettingsHelper :akismet_api_key, :akismet_enabled, :allow_local_requests_from_hooks_and_services, + :archive_builds_in_human_readable, :authorized_keys_enabled, :auto_devops_enabled, :auto_devops_domain, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index b66ec0ffab6..704310f53f0 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -5,6 +5,7 @@ class ApplicationSetting < ActiveRecord::Base include CacheMarkdownField include TokenAuthenticatable include IgnorableColumn + include ChronicDurationAttribute add_authentication_token_field :runners_registration_token add_authentication_token_field :health_check_access_token @@ -45,6 +46,8 @@ class ApplicationSetting < ActiveRecord::Base default_value_for :id, 1 + chronic_duration_attr_writer :archive_builds_in_human_readable, :archive_builds_in_seconds + validates :uuid, presence: true validates :session_expire_delay, @@ -184,6 +187,10 @@ class ApplicationSetting < ActiveRecord::Base validates :user_default_internal_regex, js_regex: true, allow_nil: true + validates :archive_builds_in_seconds, + allow_nil: true, + numericality: { only_integer: true, greater_than_or_equal_to: 1.day.seconds } + SUPPORTED_KEY_TYPES.each do |type| validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type } end @@ -441,6 +448,10 @@ class ApplicationSetting < ActiveRecord::Base latest_terms end + def archive_builds_older_than + archive_builds_in_seconds.seconds.ago if archive_builds_in_seconds + end + private def ensure_uuid! diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 600c562d05a..d7eab57763e 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -258,8 +258,24 @@ module Ci self.name == 'pages' end + # degenerated build is one that cannot be run by Runner + def degenerated? + self.options.nil? + end + + def degenerate! + self.update!(options: nil, yaml_variables: nil, commands: nil) + end + + def archived? + return true if degenerated? + + archive_builds_older_than = Gitlab::CurrentSettings.current_application_settings.archive_builds_older_than + archive_builds_older_than.present? && created_at < archive_builds_older_than + end + def playable? - action? && (manual? || scheduled? || retryable?) + action? && !archived? && (manual? || scheduled? || retryable?) end def schedulable? @@ -287,7 +303,7 @@ module Ci end def retryable? - success? || failed? || canceled? + !archived? && (success? || failed? || canceled?) end def retries_count @@ -295,7 +311,7 @@ module Ci end def retries_max - self.options.fetch(:retry, 0).to_i + self.options.to_h.fetch(:retry, 0).to_i end def latest? diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 95c88e11a6e..755f8bd4d06 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -51,7 +51,8 @@ class CommitStatus < ActiveRecord::Base missing_dependency_failure: 5, runner_unsupported: 6, stale_schedule: 7, - job_execution_timeout: 8 + job_execution_timeout: 8, + archived_failure: 9 } ## @@ -167,16 +168,18 @@ class CommitStatus < ActiveRecord::Base false end - # To be overridden when inherrited from def retryable? false end - # To be overridden when inherrited from def cancelable? false end + def archived? + false + end + def stuck? false end diff --git a/app/policies/ci/build_policy.rb b/app/policies/ci/build_policy.rb index 3858b29c82c..0ca3e696f46 100644 --- a/app/policies/ci/build_policy.rb +++ b/app/policies/ci/build_policy.rb @@ -20,12 +20,17 @@ module Ci @subject.project.branch_allows_collaboration?(@user, @subject.ref) end + condition(:archived, scope: :subject) do + @subject.archived? + end + condition(:terminal, scope: :subject) do @subject.has_terminal? end - rule { protected_ref }.policy do + rule { protected_ref | archived }.policy do prevent :update_build + prevent :update_commit_status prevent :erase_build end diff --git a/app/policies/deployment_policy.rb b/app/policies/deployment_policy.rb index 56ac898b6ab..d4f2f3c52b1 100644 --- a/app/policies/deployment_policy.rb +++ b/app/policies/deployment_policy.rb @@ -2,4 +2,13 @@ class DeploymentPolicy < BasePolicy delegate { @subject.project } + + condition(:can_retry_deployable) do + can?(:update_build, @subject.deployable) + end + + rule { ~can_retry_deployable }.policy do + prevent :create_deployment + prevent :update_deployment + end end diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb index a866e76df5a..0cd77da6303 100644 --- a/app/presenters/commit_status_presenter.rb +++ b/app/presenters/commit_status_presenter.rb @@ -10,7 +10,8 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated missing_dependency_failure: 'There has been a missing dependency failure', runner_unsupported: 'Your runner is outdated, please upgrade your runner', stale_schedule: 'Delayed job could not be executed by some reason, please try again', - job_execution_timeout: 'The script exceeded the maximum execution time set for the job' + job_execution_timeout: 'The script exceeded the maximum execution time set for the job', + archived_failure: 'The job is archived and cannot be run' }.freeze private_constant :CALLOUT_FAILURE_MESSAGES @@ -30,6 +31,6 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated end def unrecoverable? - script_failure? || missing_dependency_failure? + script_failure? || missing_dependency_failure? || archived_failure? end end diff --git a/app/serializers/job_entity.rb b/app/serializers/job_entity.rb index aebbc18e32f..d0099ae77f2 100644 --- a/app/serializers/job_entity.rb +++ b/app/serializers/job_entity.rb @@ -7,6 +7,7 @@ class JobEntity < Grape::Entity expose :name expose :started?, as: :started + expose :archived?, as: :archived expose :build_path do |build| build_path(build) diff --git a/app/services/ci/register_job_service.rb b/app/services/ci/register_job_service.rb index 5a7be921389..e06f1c05843 100644 --- a/app/services/ci/register_job_service.rb +++ b/app/services/ci/register_job_service.rb @@ -82,6 +82,11 @@ module Ci return false end + if build.archived? + build.drop!(:archived_failure) + return false + end + build.run! true end diff --git a/app/views/admin/application_settings/_ci_cd.html.haml b/app/views/admin/application_settings/_ci_cd.html.haml index 97be658cd34..adb496495d1 100644 --- a/app/views/admin/application_settings/_ci_cd.html.haml +++ b/app/views/admin/application_settings/_ci_cd.html.haml @@ -41,5 +41,13 @@ The default unit is in seconds, but you can define an alternative. For example: 4 mins 2 sec, 2h42min. = link_to icon('question-circle'), help_page_path('user/admin_area/settings/continuous_integration', anchor: 'default-artifacts-expiration') + .form-group + = f.label :archive_builds_in_human_readable, 'Archive builds in', class: 'label-bold' + = f.text_field :archive_builds_in_human_readable, class: 'form-control', placeholder: 'never' + .form-text.text-muted + Set the duration when build gonna be considered old. Archived builds cannot be retried. + Make it empty to never expire builds. It has to be larger than 1 day. + The default unit is in seconds, but you can define an alternative. For example: + 4 mins 2 sec, 2h42min. = f.submit 'Save changes', class: "btn btn-success" diff --git a/app/views/projects/deployments/_rollback.haml b/app/views/projects/deployments/_rollback.haml index 281e042c915..1bd538a08ff 100644 --- a/app/views/projects/deployments/_rollback.haml +++ b/app/views/projects/deployments/_rollback.haml @@ -1,4 +1,4 @@ -- if can?(current_user, :create_deployment, deployment) && deployment.deployable +- if can?(current_user, :create_deployment, deployment) - tooltip = deployment.last? ? s_('Environments|Re-deploy to environment') : s_('Environments|Rollback environment') = link_to [:retry, @project.namespace.becomes(Namespace), @project, deployment.deployable], method: :post, class: 'btn btn-build has-tooltip', title: tooltip do - if deployment.last? diff --git a/app/views/projects/pipelines/_with_tabs.html.haml b/app/views/projects/pipelines/_with_tabs.html.haml index 95bba47802c..66e202103a9 100644 --- a/app/views/projects/pipelines/_with_tabs.html.haml +++ b/app/views/projects/pipelines/_with_tabs.html.haml @@ -61,12 +61,14 @@ %td.responsive-table-cell.build-failure{ data: { column: _("Failure")} } = build.present.callout_failure_message %td.responsive-table-cell.build-actions - = link_to retry_project_job_path(build.project, build, return_to: request.original_url), method: :post, title: _('Retry'), class: 'btn btn-build' do - = icon('repeat') - %tr.build-trace-row.responsive-table-border-end - %td - %td.responsive-table-cell.build-trace-container{ colspan: 4 } - %pre.build-trace.build-trace-rounded - %code.bash.js-build-output - = build_summary(build) + - if can?(current_user, :update_build, job) + = link_to retry_project_job_path(build.project, build, return_to: request.original_url), method: :post, title: _('Retry'), class: 'btn btn-build' do + = icon('repeat') + - if can?(current_user, :read_build, job) + %tr.build-trace-row.responsive-table-border-end + %td + %td.responsive-table-cell.build-trace-container{ colspan: 4 } + %pre.build-trace.build-trace-rounded + %code.bash.js-build-output + = build_summary(build) = render_if_exists "projects/pipelines/tabs_content", pipeline: @pipeline, project: @project diff --git a/changelogs/unreleased/disallow-retry-of-old-builds.yml b/changelogs/unreleased/disallow-retry-of-old-builds.yml new file mode 100644 index 00000000000..03992fc0213 --- /dev/null +++ b/changelogs/unreleased/disallow-retry-of-old-builds.yml @@ -0,0 +1,5 @@ +--- +title: Soft-archive old jobs +merge_request: +author: +type: added diff --git a/db/migrate/20181023104858_add_archive_builds_duration_to_application_settings.rb b/db/migrate/20181023104858_add_archive_builds_duration_to_application_settings.rb new file mode 100644 index 00000000000..744748b3fad --- /dev/null +++ b/db/migrate/20181023104858_add_archive_builds_duration_to_application_settings.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddArchiveBuildsDurationToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def change + add_column(:application_settings, :archive_builds_in_seconds, :integer, allow_null: true) + end +end diff --git a/db/schema.rb b/db/schema.rb index d1815687968..5ad8fb7c5a4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -165,6 +165,7 @@ ActiveRecord::Schema.define(version: 20181101144347) do t.integer "usage_stats_set_by_user_id" t.integer "receive_max_input_size" t.integer "diff_max_patch_bytes", default: 102400, null: false + t.integer "archive_builds_in_seconds" end create_table "audit_events", force: :cascade do |t| diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb index 7cc1cc6b8e3..d40454df737 100644 --- a/lib/gitlab/ci/status/build/failed.rb +++ b/lib/gitlab/ci/status/build/failed.rb @@ -14,7 +14,8 @@ module Gitlab missing_dependency_failure: 'missing dependency failure', runner_unsupported: 'unsupported runner', stale_schedule: 'stale schedule', - job_execution_timeout: 'job execution timeout' + job_execution_timeout: 'job execution timeout', + archived_failure: 'archived failure' }.freeze private_constant :REASONS diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 85ba7d4097d..0cacdf7931f 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -27,6 +27,12 @@ FactoryBot.define do pipeline factory: :ci_pipeline + trait :degenerated do + commands nil + options nil + yaml_variables nil + end + trait :started do started_at 'Di 29. Okt 09:51:28 CET 2013' end diff --git a/spec/features/projects/environments/environment_spec.rb b/spec/features/projects/environments/environment_spec.rb index 70e0879dd81..4f8f67aab22 100644 --- a/spec/features/projects/environments/environment_spec.rb +++ b/spec/features/projects/environments/environment_spec.rb @@ -53,10 +53,21 @@ describe 'Environment' do it 'does show build name' do expect(page).to have_link("#{build.name} (##{build.id})") - expect(page).to have_link('Re-deploy') + expect(page).not_to have_link('Re-deploy') expect(page).not_to have_terminal_button end + context 'when user has ability to re-deploy' do + let(:permissions) do + create(:protected_branch, :developers_can_merge, + name: build.ref, project: project) + end + + it 'does show re-deploy' do + expect(page).to have_link('Re-deploy') + end + end + context 'with manual action' do let(:action) do create(:ci_build, :manual, pipeline: pipeline, diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index cd6c37bf54d..049bbca958f 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -388,54 +388,83 @@ describe 'Pipeline', :js do let(:pipeline_failures_page) { failures_project_pipeline_path(project, pipeline) } let!(:failed_build) { create(:ci_build, :failed, pipeline: pipeline) } + subject { visit pipeline_failures_page } + context 'with failed build' do before do failed_build.trace.set('4 examples, 1 failure') - - visit pipeline_failures_page end it 'shows jobs tab pane as active' do + subject + expect(page).to have_content('Failed Jobs') expect(page).to have_css('#js-tab-failures.active') end it 'lists failed builds' do + subject + expect(page).to have_content(failed_build.name) expect(page).to have_content(failed_build.stage) end it 'shows build failure logs' do + subject + expect(page).to have_content('4 examples, 1 failure') end it 'shows the failure reason' do + subject + expect(page).to have_content('There is an unknown failure, please try again') end - it 'shows retry button for failed build' do - page.within(find('.build-failures', match: :first)) do - expect(page).to have_link('Retry') + context 'when user does not have permission to retry build' do + it 'shows retry button for failed build' do + subject + + page.within(find('.build-failures', match: :first)) do + expect(page).not_to have_link('Retry') + end + end + end + + context 'when user does have permission to retry build' do + before do + create(:protected_branch, :developers_can_merge, + name: pipeline.ref, project: project) + end + + it 'shows retry button for failed build' do + subject + + page.within(find('.build-failures', match: :first)) do + expect(page).to have_link('Retry') + end end end end context 'when missing build logs' do - before do - visit pipeline_failures_page - end - it 'shows jobs tab pane as active' do + subject + expect(page).to have_content('Failed Jobs') expect(page).to have_css('#js-tab-failures.active') end it 'lists failed builds' do + subject + expect(page).to have_content(failed_build.name) expect(page).to have_content(failed_build.stage) end it 'does not show trace' do + subject + expect(page).to have_content('No job trace') end end @@ -448,11 +477,9 @@ describe 'Pipeline', :js do end context 'when accessing failed jobs page' do - before do - visit pipeline_failures_page - end - it 'fails to access the page' do + subject + expect(page).to have_title('Access Denied') end end @@ -461,11 +488,11 @@ describe 'Pipeline', :js do context 'without failures' do before do failed_build.update!(status: :success) - - visit pipeline_failures_page end it 'displays the pipeline graph' do + subject + expect(current_path).to eq(pipeline_path(pipeline)) expect(page).not_to have_content('Failed Jobs') expect(page).to have_selector('.pipeline-visualization') diff --git a/spec/fixtures/api/schemas/job/job.json b/spec/fixtures/api/schemas/job/job.json index 734c535ef70..f3d5e9b038a 100644 --- a/spec/fixtures/api/schemas/job/job.json +++ b/spec/fixtures/api/schemas/job/job.json @@ -9,7 +9,8 @@ "playable", "created_at", "updated_at", - "status" + "status", + "archived" ], "properties": { "id": { "type": "integer" }, @@ -27,7 +28,8 @@ "updated_at": { "type": "string" }, "status": { "$ref": "../status/ci_detailed_status.json" }, "callout_message": { "type": "string" }, - "recoverable": { "type": "boolean" } + "recoverable": { "type": "boolean" }, + "archived": { "type": "boolean" } }, "additionalProperties": true } diff --git a/spec/models/application_setting_spec.rb b/spec/models/application_setting_spec.rb index 87b91286168..95ae7bd21ab 100644 --- a/spec/models/application_setting_spec.rb +++ b/spec/models/application_setting_spec.rb @@ -594,4 +594,24 @@ describe ApplicationSetting do end end end + + describe '#archive_builds_older_than' do + subject { setting.archive_builds_older_than } + + context 'when the archive_builds_in_seconds is set' do + before do + setting.archive_builds_in_seconds = 3600 + end + + it { is_expected.to be_within(1.minute).of(1.hour.ago) } + end + + context 'when the archive_builds_in_seconds is set' do + before do + setting.archive_builds_in_seconds = nil + end + + it { is_expected.to be_nil } + end + end end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b07f8bc98b5..4089f099fdf 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -1314,6 +1314,14 @@ describe Ci::Build do it { is_expected.not_to be_retryable } end + + context 'when build is degenerated' do + before do + build.degenerate! + end + + it { is_expected.not_to be_retryable } + end end end @@ -1396,6 +1404,14 @@ describe Ci::Build do expect(subject.retries_max).to eq 0 end end + + context 'when build is degenerated' do + subject { create(:ci_build, :degenerated) } + + it 'returns zero' do + expect(subject.retries_max).to eq 0 + end + end end end @@ -1659,6 +1675,12 @@ describe Ci::Build do it { is_expected.to be_playable } end + + context 'when build is a manual and degenerated' do + subject { build_stubbed(:ci_build, :manual, :degenerated, status: :manual) } + + it { is_expected.not_to be_playable } + end end context 'when build is scheduled' do @@ -3227,4 +3249,54 @@ describe Ci::Build do it { expect(build.deployment_status).to eq(:creating) } end end + + describe '#degenerated?' do + context 'when build is degenerated' do + subject { create(:ci_build, :degenerated) } + + it { is_expected.to be_degenerated } + end + + context 'when build is valid' do + subject { create(:ci_build) } + + it { is_expected.not_to be_degenerated } + + context 'and becomes degenerated' do + before do + subject.degenerate! + end + + it { is_expected.to be_degenerated } + end + end + end + + describe '#archived?' do + context 'when build is degenerated' do + subject { create(:ci_build, :degenerated) } + + it { is_expected.to be_archived } + end + + context 'for old build' do + subject { create(:ci_build, created_at: 1.day.ago) } + + context 'when archive_builds_in is set' do + before do + stub_application_setting(archive_builds_in_seconds: 3600) + end + + it { is_expected.to be_archived } + end + + context 'when archive_builds_in is not set' do + before do + stub_application_setting(archive_builds_in_seconds: nil) + end + + it { is_expected.not_to be_archived } + end + end + end end diff --git a/spec/presenters/ci/build_presenter_spec.rb b/spec/presenters/ci/build_presenter_spec.rb index d7992f0a4a9..676835b3880 100644 --- a/spec/presenters/ci/build_presenter_spec.rb +++ b/spec/presenters/ci/build_presenter_spec.rb @@ -267,7 +267,7 @@ describe Ci::BuildPresenter do let(:build) { create(:ci_build, :failed, :script_failure) } context 'when is a script or missing dependency failure' do - let(:failure_reasons) { %w(script_failure missing_dependency_failure) } + let(:failure_reasons) { %w(script_failure missing_dependency_failure archived_failure) } it 'should return false' do failure_reasons.each do |failure_reason| diff --git a/spec/services/ci/register_job_service_spec.rb b/spec/services/ci/register_job_service_spec.rb index a6565709641..56e2a405bcd 100644 --- a/spec/services/ci/register_job_service_spec.rb +++ b/spec/services/ci/register_job_service_spec.rb @@ -478,6 +478,20 @@ module Ci it_behaves_like 'validation is not active' end end + + context 'when build is degenerated' do + let!(:pending_job) { create(:ci_build, :pending, :degenerated, pipeline: pipeline) } + + subject { execute(specific_runner, {}) } + + it 'does not pick the build and drops the build' do + expect(subject).to be_nil + + pending_job.reload + expect(pending_job).to be_failed + expect(pending_job).to be_archived_failure + end + end end describe '#register_success' do