From 42ca9c6f0de34dfa7ae09cc0e9672ea5857afd38 Mon Sep 17 00:00:00 2001 From: Tiger Date: Wed, 27 Feb 2019 13:13:06 +1100 Subject: [PATCH 1/8] Add :preparing status to HasStatus Introduces a new status for builds between :created and :pending that will be used when builds require one or more prerequisite actions to be completed before being picked up by a runner (such as creating Kubernetes resources before deploying). The existing :created > :pending transition is unchanged, so only builds that require preparation will use the :preparing status. --- app/assets/stylesheets/framework/icons.scss | 1 + .../stylesheets/pages/merge_requests.scss | 1 + app/assets/stylesheets/pages/pipelines.scss | 1 + app/assets/stylesheets/pages/status.scss | 1 + app/models/ci/pipeline.rb | 13 ++-- app/models/ci/stage.rb | 7 +- app/models/commit_status.rb | 14 ++-- app/models/concerns/has_status.rb | 17 +++-- lib/gitlab/badge/pipeline/template.rb | 1 + lib/gitlab/ci/status/build/factory.rb | 1 + lib/gitlab/ci/status/build/preparing.rb | 28 ++++++++ lib/gitlab/ci/status/preparing.rb | 33 ++++++++++ locale/gitlab.pot | 12 ++++ spec/factories/ci/builds.rb | 4 ++ spec/factories/ci/pipelines.rb | 8 +++ spec/factories/commit_statuses.rb | 4 ++ .../projects/badges/pipeline_badge_spec.rb | 18 ++++++ .../projects/pipelines/pipeline_spec.rb | 23 +++++++ .../projects/pipelines/pipelines_spec.rb | 24 +++++++ .../gitlab/badge/pipeline/template_spec.rb | 10 +++ .../gitlab/ci/status/build/preparing_spec.rb | 33 ++++++++++ spec/lib/gitlab/ci/status/preparing_spec.rb | 29 +++++++++ spec/models/ci/pipeline_spec.rb | 64 +++++++++++++++---- spec/models/commit_status_spec.rb | 17 +++++ spec/models/concerns/has_status_spec.rb | 22 ++++++- 25 files changed, 354 insertions(+), 32 deletions(-) create mode 100644 lib/gitlab/ci/status/build/preparing.rb create mode 100644 lib/gitlab/ci/status/preparing.rb create mode 100644 spec/lib/gitlab/ci/status/build/preparing_spec.rb create mode 100644 spec/lib/gitlab/ci/status/preparing_spec.rb diff --git a/app/assets/stylesheets/framework/icons.scss b/app/assets/stylesheets/framework/icons.scss index 49b9b7014ae..3ab61cc5c47 100644 --- a/app/assets/stylesheets/framework/icons.scss +++ b/app/assets/stylesheets/framework/icons.scss @@ -31,6 +31,7 @@ } } +.ci-status-icon-preparing, .ci-status-icon-running { svg { fill: $blue-400; diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 44556060c65..203f695d885 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -166,6 +166,7 @@ float: left; .accept-merge-request { + &.ci-preparing, &.ci-pending, &.ci-running { @include btn-blue; diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 2b6319ddd4f..13c518a1ba8 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -793,6 +793,7 @@ @include mini-pipeline-graph-color($white, $orange-100, $orange-200, $orange-500, $orange-600, $orange-700); } + &.ci-status-icon-preparing, &.ci-status-icon-running { @include mini-pipeline-graph-color($white, $blue-100, $blue-200, $blue-500, $blue-600, $blue-700); } diff --git a/app/assets/stylesheets/pages/status.scss b/app/assets/stylesheets/pages/status.scss index f4d568d02ac..a59bb31bdcb 100644 --- a/app/assets/stylesheets/pages/status.scss +++ b/app/assets/stylesheets/pages/status.scss @@ -44,6 +44,7 @@ } &.ci-info, + &.ci-preparing, &.ci-running { @include status-color($blue-100, $blue-500, $blue-600); } diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index adffdc0355e..ae74f569415 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -82,10 +82,14 @@ module Ci state_machine :status, initial: :created do event :enqueue do - transition [:created, :skipped, :scheduled] => :pending + transition [:created, :preparing, :skipped, :scheduled] => :pending transition [:success, :failed, :canceled] => :running end + event :prepare do + transition any - [:preparing] => :preparing + end + event :run do transition any - [:running] => :running end @@ -118,7 +122,7 @@ module Ci # Do not add any operations to this state_machine # Create a separate worker for each new operation - before_transition [:created, :pending] => :running do |pipeline| + before_transition [:created, :preparing, :pending] => :running do |pipeline| pipeline.started_at = Time.now end @@ -141,7 +145,7 @@ module Ci end end - after_transition [:created, :pending] => :running do |pipeline| + after_transition [:created, :preparing, :pending] => :running do |pipeline| pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end @@ -149,7 +153,7 @@ module Ci pipeline.run_after_commit { PipelineMetricsWorker.perform_async(pipeline.id) } end - after_transition [:created, :pending, :running] => :success do |pipeline| + after_transition [:created, :preparing, :pending, :running] => :success do |pipeline| pipeline.run_after_commit { PipelineSuccessWorker.perform_async(pipeline.id) } end @@ -597,6 +601,7 @@ module Ci retry_optimistic_lock(self) do case latest_builds_status.to_s when 'created' then nil + when 'preparing' then prepare when 'pending' then enqueue when 'running' then run when 'success' then succeed diff --git a/app/models/ci/stage.rb b/app/models/ci/stage.rb index 0389945191e..098f5189517 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -39,10 +39,14 @@ module Ci state_machine :status, initial: :created do event :enqueue do - transition created: :pending + transition [:created, :preparing] => :pending transition [:success, :failed, :canceled, :skipped] => :running end + event :prepare do + transition any - [:preparing] => :preparing + end + event :run do transition any - [:running] => :running end @@ -76,6 +80,7 @@ module Ci retry_optimistic_lock(self) do case statuses.latest.status when 'created' then nil + when 'preparing' then prepare when 'pending' then enqueue when 'running' then run when 'success' then succeed diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 7f6562b63e5..bfb13fa0ce8 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -66,7 +66,7 @@ class CommitStatus < ActiveRecord::Base end event :enqueue do - transition [:created, :skipped, :manual, :scheduled] => :pending + transition [:created, :preparing, :skipped, :manual, :scheduled] => :pending end event :run do @@ -74,26 +74,26 @@ class CommitStatus < ActiveRecord::Base end event :skip do - transition [:created, :pending] => :skipped + transition [:created, :preparing, :pending] => :skipped end event :drop do - transition [:created, :pending, :running, :scheduled] => :failed + transition [:created, :preparing, :pending, :running, :scheduled] => :failed end event :success do - transition [:created, :pending, :running] => :success + transition [:created, :preparing, :pending, :running] => :success end event :cancel do - transition [:created, :pending, :running, :manual, :scheduled] => :canceled + transition [:created, :preparing, :pending, :running, :manual, :scheduled] => :canceled end - before_transition [:created, :skipped, :manual, :scheduled] => :pending do |commit_status| + before_transition [:created, :preparing, :skipped, :manual, :scheduled] => :pending do |commit_status| commit_status.queued_at = Time.now end - before_transition [:created, :pending] => :running do |commit_status| + before_transition [:created, :preparing, :pending] => :running do |commit_status| commit_status.started_at = Time.now end diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb index 0d2be4c61ab..8882f48c281 100644 --- a/app/models/concerns/has_status.rb +++ b/app/models/concerns/has_status.rb @@ -5,14 +5,14 @@ module HasStatus DEFAULT_STATUS = 'created'.freeze BLOCKED_STATUS = %w[manual scheduled].freeze - AVAILABLE_STATUSES = %w[created pending running success failed canceled skipped manual scheduled].freeze + AVAILABLE_STATUSES = %w[created preparing pending running success failed canceled skipped manual scheduled].freeze STARTED_STATUSES = %w[running success failed skipped manual scheduled].freeze - ACTIVE_STATUSES = %w[pending running].freeze + ACTIVE_STATUSES = %w[preparing pending running].freeze COMPLETED_STATUSES = %w[success failed canceled skipped].freeze - ORDERED_STATUSES = %w[failed pending running manual scheduled canceled success skipped created].freeze + ORDERED_STATUSES = %w[failed preparing pending running manual scheduled canceled success skipped created].freeze STATUSES_ENUM = { created: 0, pending: 1, running: 2, success: 3, failed: 4, canceled: 5, skipped: 6, manual: 7, - scheduled: 8 }.freeze + scheduled: 8, preparing: 9 }.freeze UnknownStatusError = Class.new(StandardError) @@ -26,6 +26,7 @@ module HasStatus success = scope_relevant.success.select('count(*)').to_sql manual = scope_relevant.manual.select('count(*)').to_sql scheduled = scope_relevant.scheduled.select('count(*)').to_sql + preparing = scope_relevant.preparing.select('count(*)').to_sql pending = scope_relevant.pending.select('count(*)').to_sql running = scope_relevant.running.select('count(*)').to_sql skipped = scope_relevant.skipped.select('count(*)').to_sql @@ -37,12 +38,14 @@ module HasStatus WHEN (#{builds})=(#{skipped}) THEN 'skipped' WHEN (#{builds})=(#{success}) THEN 'success' WHEN (#{builds})=(#{created}) THEN 'created' + WHEN (#{builds})=(#{preparing}) THEN 'preparing' WHEN (#{builds})=(#{success})+(#{skipped}) THEN 'success' WHEN (#{builds})=(#{success})+(#{skipped})+(#{canceled}) THEN 'canceled' WHEN (#{builds})=(#{created})+(#{skipped})+(#{pending}) THEN 'pending' WHEN (#{running})+(#{pending})>0 THEN 'running' WHEN (#{manual})>0 THEN 'manual' WHEN (#{scheduled})>0 THEN 'scheduled' + WHEN (#{preparing})>0 THEN 'preparing' WHEN (#{created})>0 THEN 'running' ELSE 'failed' END)" @@ -70,6 +73,7 @@ module HasStatus state_machine :status, initial: :created do state :created, value: 'created' + state :preparing, value: 'preparing' state :pending, value: 'pending' state :running, value: 'running' state :failed, value: 'failed' @@ -81,6 +85,7 @@ module HasStatus end scope :created, -> { where(status: 'created') } + scope :preparing, -> { where(status: 'preparing') } scope :relevant, -> { where(status: AVAILABLE_STATUSES - ['created']) } scope :running, -> { where(status: 'running') } scope :pending, -> { where(status: 'pending') } @@ -90,14 +95,14 @@ module HasStatus scope :skipped, -> { where(status: 'skipped') } scope :manual, -> { where(status: 'manual') } scope :scheduled, -> { where(status: 'scheduled') } - scope :alive, -> { where(status: [:created, :pending, :running]) } + scope :alive, -> { where(status: [:created, :preparing, :pending, :running]) } scope :created_or_pending, -> { where(status: [:created, :pending]) } scope :running_or_pending, -> { where(status: [:running, :pending]) } scope :finished, -> { where(status: [:success, :failed, :canceled]) } scope :failed_or_canceled, -> { where(status: [:failed, :canceled]) } scope :cancelable, -> do - where(status: [:running, :pending, :created, :scheduled]) + where(status: [:running, :preparing, :pending, :created, :scheduled]) end end diff --git a/lib/gitlab/badge/pipeline/template.rb b/lib/gitlab/badge/pipeline/template.rb index 64c3dfcd10b..2c5f9654496 100644 --- a/lib/gitlab/badge/pipeline/template.rb +++ b/lib/gitlab/badge/pipeline/template.rb @@ -15,6 +15,7 @@ module Gitlab failed: '#e05d44', running: '#dfb317', pending: '#dfb317', + preparing: '#dfb317', canceled: '#9f9f9f', skipped: '#9f9f9f', unknown: '#9f9f9f' diff --git a/lib/gitlab/ci/status/build/factory.rb b/lib/gitlab/ci/status/build/factory.rb index 6e4bfe23f2b..f7d0715e617 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -11,6 +11,7 @@ module Gitlab Status::Build::Manual, Status::Build::Canceled, Status::Build::Created, + Status::Build::Preparing, Status::Build::Pending, Status::Build::Skipped], [Status::Build::Cancelable, diff --git a/lib/gitlab/ci/status/build/preparing.rb b/lib/gitlab/ci/status/build/preparing.rb new file mode 100644 index 00000000000..1fddcb05f79 --- /dev/null +++ b/lib/gitlab/ci/status/build/preparing.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Status + module Build + class Preparing < Status::Extended + ## + # TODO: image is shared with 'pending' + # until we get a dedicated one + # + def illustration + { + image: 'illustrations/job_not_triggered.svg', + size: 'svg-306', + title: _('This job is preparing to start'), + content: _('This job is performing tasks that must complete before it can start') + } + end + + def self.matches?(build, _) + build.preparing? + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/preparing.rb b/lib/gitlab/ci/status/preparing.rb new file mode 100644 index 00000000000..62985d0a9f9 --- /dev/null +++ b/lib/gitlab/ci/status/preparing.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Status + class Preparing < Status::Core + def text + s_('CiStatusText|preparing') + end + + def label + s_('CiStatusLabel|preparing') + end + + ## + # TODO: shared with 'created' + # until we get one for 'preparing' + # + def icon + 'status_created' + end + + ## + # TODO: shared with 'created' + # until we get one for 'preparing' + # + def favicon + 'favicon_status_created' + end + end + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 31b9af92805..471e5995ce9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1512,6 +1512,9 @@ msgstr "" msgid "CiStatusLabel|pending" msgstr "" +msgid "CiStatusLabel|preparing" +msgstr "" + msgid "CiStatusLabel|skipped" msgstr "" @@ -1545,6 +1548,9 @@ msgstr "" msgid "CiStatusText|pending" msgstr "" +msgid "CiStatusText|preparing" +msgstr "" + msgid "CiStatusText|skipped" msgstr "" @@ -7864,6 +7870,12 @@ msgstr "" msgid "This job is in pending state and is waiting to be picked by a runner" msgstr "" +msgid "This job is performing tasks that must complete before it can start" +msgstr "" + +msgid "This job is preparing to start" +msgstr "" + msgid "This job is stuck because you don't have any active runners online with any of these tags assigned to them:" msgstr "" diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 0b3e67b4987..067391c1179 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -75,6 +75,10 @@ FactoryBot.define do status 'created' end + trait :preparing do + status 'preparing' + end + trait :scheduled do schedulable status 'scheduled' diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index ee5d27355f1..aa5ccbda6cd 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -50,6 +50,14 @@ FactoryBot.define do failure_reason :config_error end + trait :created do + status :created + end + + trait :preparing do + status :preparing + end + trait :blocked do status :manual end diff --git a/spec/factories/commit_statuses.rb b/spec/factories/commit_statuses.rb index 381bf07f6a0..848a31e96c1 100644 --- a/spec/factories/commit_statuses.rb +++ b/spec/factories/commit_statuses.rb @@ -33,6 +33,10 @@ FactoryBot.define do status 'pending' end + trait :preparing do + status 'preparing' + end + trait :created do status 'created' end diff --git a/spec/features/projects/badges/pipeline_badge_spec.rb b/spec/features/projects/badges/pipeline_badge_spec.rb index dee81898928..96efa06d843 100644 --- a/spec/features/projects/badges/pipeline_badge_spec.rb +++ b/spec/features/projects/badges/pipeline_badge_spec.rb @@ -41,6 +41,24 @@ describe 'Pipeline Badge' do end end + context 'when the pipeline is preparing' do + let!(:job) { create(:ci_build, status: 'created', pipeline: pipeline) } + + before do + # Prevent skipping directly to 'pending' + allow(Ci::BuildPrepareWorker).to receive(:perform_async) + end + + it 'displays the preparing badge' do + job.prepare + + visit pipeline_project_badges_path(project, ref: ref, format: :svg) + + expect(page.status_code).to eq(200) + expect_badge('preparing') + end + end + context 'when the pipeline is running' do it 'shows displays so on the badge' do create(:ci_build, pipeline: pipeline, name: 'second build', status_event: 'run') diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 36b8c15b8b6..84d4312010f 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -21,6 +21,11 @@ describe 'Pipeline', :js do pipeline: pipeline, stage: 'test', name: 'test') end + let!(:build_preparing) do + create(:ci_build, :preparing, + pipeline: pipeline, stage: 'deploy', name: 'prepare') + end + let!(:build_running) do create(:ci_build, :running, pipeline: pipeline, stage: 'deploy', name: 'deploy') @@ -97,6 +102,24 @@ describe 'Pipeline', :js do end end + context 'when pipeline has preparing builds' do + it 'shows a preparing icon and a cancel action' do + page.within('#ci-badge-prepare') do + expect(page).to have_selector('.js-ci-status-icon-preparing') + expect(page).to have_selector('.js-icon-cancel') + expect(page).to have_content('prepare') + end + end + + it 'cancels the preparing build and shows retry button' do + find('#ci-badge-deploy .ci-action-icon-container').click + + page.within('#ci-badge-deploy') do + expect(page).to have_css('.js-icon-retry') + end + end + end + context 'when pipeline has successful builds' do it 'shows the success icon and a retry action for the successful build' do page.within('#ci-badge-build') do diff --git a/spec/features/projects/pipelines/pipelines_spec.rb b/spec/features/projects/pipelines/pipelines_spec.rb index 88d7c9ef8bd..9dc505573d4 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -282,6 +282,30 @@ describe 'Pipelines', :js do end context 'for generic statuses' do + context 'when preparing' do + let!(:pipeline) do + create(:ci_empty_pipeline, + status: 'preparing', project: project) + end + + let!(:status) do + create(:generic_commit_status, + :preparing, pipeline: pipeline) + end + + before do + visit_project_pipelines + end + + it 'is cancelable' do + expect(page).to have_selector('.js-pipelines-cancel-button') + end + + it 'shows the pipeline as preparing' do + expect(page).to have_selector('.ci-preparing') + end + end + context 'when running' do let!(:running) do create(:generic_commit_status, diff --git a/spec/lib/gitlab/badge/pipeline/template_spec.rb b/spec/lib/gitlab/badge/pipeline/template_spec.rb index 20fa4f879c3..bcef0b7e120 100644 --- a/spec/lib/gitlab/badge/pipeline/template_spec.rb +++ b/spec/lib/gitlab/badge/pipeline/template_spec.rb @@ -59,6 +59,16 @@ describe Gitlab::Badge::Pipeline::Template do end end + context 'when status is preparing' do + before do + allow(badge).to receive(:status).and_return('preparing') + end + + it 'has expected color' do + expect(template.value_color).to eq '#dfb317' + end + end + context 'when status is unknown' do before do allow(badge).to receive(:status).and_return('unknown') diff --git a/spec/lib/gitlab/ci/status/build/preparing_spec.rb b/spec/lib/gitlab/ci/status/build/preparing_spec.rb new file mode 100644 index 00000000000..4d8945845ba --- /dev/null +++ b/spec/lib/gitlab/ci/status/build/preparing_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Status::Build::Preparing do + subject do + described_class.new(double('subject')) + end + + describe '#illustration' do + it { expect(subject.illustration).to include(:image, :size, :title, :content) } + end + + describe '.matches?' do + subject { described_class.matches?(build, nil) } + + context 'when build is preparing' do + let(:build) { create(:ci_build, :preparing) } + + it 'is a correct match' do + expect(subject).to be true + end + end + + context 'when build is not preparing' do + let(:build) { create(:ci_build, :success) } + + it 'does not match' do + expect(subject).to be false + end + end + end +end diff --git a/spec/lib/gitlab/ci/status/preparing_spec.rb b/spec/lib/gitlab/ci/status/preparing_spec.rb new file mode 100644 index 00000000000..7211c0e506d --- /dev/null +++ b/spec/lib/gitlab/ci/status/preparing_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Status::Preparing do + subject do + described_class.new(double('subject'), nil) + end + + describe '#text' do + it { expect(subject.text).to eq 'preparing' } + end + + describe '#label' do + it { expect(subject.label).to eq 'preparing' } + end + + describe '#icon' do + it { expect(subject.icon).to eq 'status_created' } + end + + describe '#favicon' do + it { expect(subject.favicon).to eq 'favicon_status_created' } + end + + describe '#group' do + it { expect(subject.group).to eq 'preparing' } + end +end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 7eeaa7a18ef..d35caac33dc 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1201,16 +1201,28 @@ describe Ci::Pipeline, :mailer do end describe '#started_at' do - it 'updates on transitioning to running' do - build.run + let(:pipeline) { create(:ci_empty_pipeline, status: from_status) } + + %i[created preparing pending].each do |status| + context "from #{status}" do + let(:from_status) { status } - expect(pipeline.reload.started_at).not_to be_nil + it 'updates on transitioning to running' do + pipeline.run + + expect(pipeline.started_at).not_to be_nil + end + end end - it 'does not update on transitioning to success' do - build.success + context 'from created' do + let(:from_status) { :created } + + it 'does not update on transitioning to success' do + pipeline.succeed - expect(pipeline.reload.started_at).to be_nil + expect(pipeline.started_at).to be_nil + end end end @@ -1229,27 +1241,49 @@ describe Ci::Pipeline, :mailer do end describe 'merge request metrics' do - let(:project) { create(:project, :repository) } - let(:pipeline) { FactoryBot.create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) } - let!(:merge_request) { create(:merge_request, source_project: project, source_branch: pipeline.ref) } + let(:pipeline) { create(:ci_empty_pipeline, status: from_status) } before do expect(PipelineMetricsWorker).to receive(:perform_async).with(pipeline.id) end context 'when transitioning to running' do - it 'schedules metrics workers' do - pipeline.run + %i[created preparing pending].each do |status| + context "from #{status}" do + let(:from_status) { status } + + it 'schedules metrics workers' do + pipeline.run + end + end end end context 'when transitioning to success' do + let(:from_status) { 'created' } + it 'schedules metrics workers' do pipeline.succeed end end end + describe 'merge on success' do + let(:pipeline) { create(:ci_empty_pipeline, status: from_status) } + + %i[created preparing pending running].each do |status| + context "from #{status}" do + let(:from_status) { status } + + it 'schedules pipeline success worker' do + expect(PipelineSuccessWorker).to receive(:perform_async).with(pipeline.id) + + pipeline.succeed + end + end + end + end + describe 'pipeline caching' do it 'performs ExpirePipelinesCacheWorker' do expect(ExpirePipelineCacheWorker).to receive(:perform_async).with(pipeline.id) @@ -1768,6 +1802,14 @@ describe Ci::Pipeline, :mailer do subject { pipeline.reload.status } + context 'on prepare' do + before do + build.prepare + end + + it { is_expected.to eq('preparing') } + end + context 'on queuing' do before do build.enqueue diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 8b7c88805c1..1d241bf6000 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -49,6 +49,16 @@ describe CommitStatus do commit_status.success! end + + describe 'transitioning to running' do + let(:commit_status) { create(:commit_status, :pending, started_at: nil) } + + it 'records the started at time' do + commit_status.run! + + expect(commit_status.started_at).to be_present + end + end end describe '#started?' do @@ -555,6 +565,7 @@ describe CommitStatus do before do allow(Time).to receive(:now).and_return(current_time) + expect(commit_status.any_unmet_prerequisites?).to eq false end shared_examples 'commit status enqueued' do @@ -569,6 +580,12 @@ describe CommitStatus do it_behaves_like 'commit status enqueued' end + context 'when initial state is :preparing' do + let(:commit_status) { create(:commit_status, :preparing) } + + it_behaves_like 'commit status enqueued' + end + context 'when initial state is :skipped' do let(:commit_status) { create(:commit_status, :skipped) } diff --git a/spec/models/concerns/has_status_spec.rb b/spec/models/concerns/has_status_spec.rb index 6b1038cb8fd..e8b1eba67cc 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -34,6 +34,22 @@ describe HasStatus do it { is_expected.to eq 'running' } end + context 'all preparing' do + let!(:statuses) do + [create(type, status: :preparing), create(type, status: :preparing)] + end + + it { is_expected.to eq 'preparing' } + end + + context 'at least one preparing' do + let!(:statuses) do + [create(type, status: :success), create(type, status: :preparing)] + end + + it { is_expected.to eq 'preparing' } + end + context 'success and failed but allowed to fail' do let!(:statuses) do [create(type, status: :success), @@ -188,7 +204,7 @@ describe HasStatus do end end - %i[created running pending success + %i[created preparing running pending success failed canceled skipped].each do |status| it_behaves_like 'having a job', status end @@ -234,7 +250,7 @@ describe HasStatus do describe '.alive' do subject { CommitStatus.alive } - %i[running pending created].each do |status| + %i[running pending preparing created].each do |status| it_behaves_like 'containing the job', status end @@ -270,7 +286,7 @@ describe HasStatus do describe '.cancelable' do subject { CommitStatus.cancelable } - %i[running pending created scheduled].each do |status| + %i[running pending preparing created scheduled].each do |status| it_behaves_like 'containing the job', status end -- GitLab From 00f0d356b71fa87f8190810b01add0cc4586e90a Mon Sep 17 00:00:00 2001 From: Tiger Date: Mon, 4 Mar 2019 10:56:20 +1100 Subject: [PATCH 2/8] Create framework for build prerequisites Introduces the concept of Prerequisites for a CI build. If a build has unmet prerequisites it will go through the :preparing state before being made available to a runner. There are no actual prerequisites yet, so current behaviour is unchanged. --- app/models/ci/build.rb | 24 +++++++- app/models/commit_status.rb | 9 ++- app/models/commit_status_enums.rb | 3 +- app/presenters/commit_status_presenter.rb | 3 +- app/services/ci/prepare_build_service.rb | 25 ++++++++ app/workers/all_queues.yml | 1 + app/workers/ci/build_prepare_worker.rb | 16 ++++++ lib/gitlab/ci/build/prerequisite/base.rb | 27 +++++++++ lib/gitlab/ci/build/prerequisite/factory.rb | 33 +++++++++++ lib/gitlab/ci/status/build/failed.rb | 3 +- .../projects/badges/pipeline_badge_spec.rb | 3 +- .../ci/build/prerequisite/factory_spec.rb | 34 +++++++++++ spec/models/ci/build_spec.rb | 57 +++++++++++++++++++ spec/models/ci/pipeline_spec.rb | 6 +- spec/models/commit_status_spec.rb | 6 ++ spec/requests/api/runner_spec.rb | 9 +++ .../services/ci/prepare_build_service_spec.rb | 54 ++++++++++++++++++ spec/workers/ci/build_prepare_worker_spec.rb | 30 ++++++++++ 18 files changed, 336 insertions(+), 7 deletions(-) create mode 100644 app/services/ci/prepare_build_service.rb create mode 100644 app/workers/ci/build_prepare_worker.rb create mode 100644 lib/gitlab/ci/build/prerequisite/base.rb create mode 100644 lib/gitlab/ci/build/prerequisite/factory.rb create mode 100644 spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb create mode 100644 spec/services/ci/prepare_build_service_spec.rb create mode 100644 spec/workers/ci/build_prepare_worker_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index a629db82c19..a293afaed08 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -172,6 +172,10 @@ module Ci end state_machine :status do + event :enqueue do + transition [:created, :skipped, :manual, :scheduled] => :preparing, if: :any_unmet_prerequisites? + end + event :actionize do transition created: :manual end @@ -185,8 +189,12 @@ module Ci end event :enqueue_scheduled do + transition scheduled: :preparing, if: ->(build) do + build.scheduled_at&.past? && build.any_unmet_prerequisites? + end + transition scheduled: :pending, if: ->(build) do - build.scheduled_at && build.scheduled_at < Time.now + build.scheduled_at&.past? && !build.any_unmet_prerequisites? end end @@ -204,6 +212,12 @@ module Ci end end + after_transition any => [:preparing] do |build| + build.run_after_commit do + Ci::BuildPrepareWorker.perform_async(id) + end + end + after_transition any => [:pending] do |build| build.run_after_commit do BuildQueueWorker.perform_async(id) @@ -355,6 +369,14 @@ module Ci !retried? end + def any_unmet_prerequisites? + prerequisites.present? + end + + def prerequisites + Gitlab::Ci::Build::Prerequisite::Factory.new(self).unmet + end + def expanded_environment_name return unless has_environment? diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index bfb13fa0ce8..5f66a661324 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -66,7 +66,10 @@ class CommitStatus < ActiveRecord::Base end event :enqueue do - transition [:created, :preparing, :skipped, :manual, :scheduled] => :pending + # A CommitStatus will never have prerequisites, but this event + # is shared by Ci::Build, which cannot progress unless prerequisites + # are satisfied. + transition [:created, :preparing, :skipped, :manual, :scheduled] => :pending, unless: :any_unmet_prerequisites? end event :run do @@ -180,6 +183,10 @@ class CommitStatus < ActiveRecord::Base false end + def any_unmet_prerequisites? + false + end + def auto_canceled? canceled? && auto_canceled_by_id? end diff --git a/app/models/commit_status_enums.rb b/app/models/commit_status_enums.rb index 152105d9429..45e08fa18fe 100644 --- a/app/models/commit_status_enums.rb +++ b/app/models/commit_status_enums.rb @@ -14,7 +14,8 @@ module CommitStatusEnums runner_unsupported: 6, stale_schedule: 7, job_execution_timeout: 8, - archived_failure: 9 + archived_failure: 9, + unmet_prerequisites: 10 } end end diff --git a/app/presenters/commit_status_presenter.rb b/app/presenters/commit_status_presenter.rb index 0cd77da6303..28a25c8b7a3 100644 --- a/app/presenters/commit_status_presenter.rb +++ b/app/presenters/commit_status_presenter.rb @@ -11,7 +11,8 @@ class CommitStatusPresenter < Gitlab::View::Presenter::Delegated 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', - archived_failure: 'The job is archived and cannot be run' + archived_failure: 'The job is archived and cannot be run', + unmet_prerequisites: 'The job failed to complete prerequisite tasks' }.freeze private_constant :CALLOUT_FAILURE_MESSAGES diff --git a/app/services/ci/prepare_build_service.rb b/app/services/ci/prepare_build_service.rb new file mode 100644 index 00000000000..32f11438b79 --- /dev/null +++ b/app/services/ci/prepare_build_service.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +module Ci + class PrepareBuildService + attr_reader :build + + def initialize(build) + @build = build + end + + def execute + prerequisites.each(&:complete!) + + unless build.enqueue + build.drop!(:unmet_prerequisites) + end + end + + private + + def prerequisites + build.prerequisites + end + end +end diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml index b2d88567e0e..6ebd756d3da 100644 --- a/app/workers/all_queues.yml +++ b/app/workers/all_queues.yml @@ -71,6 +71,7 @@ - pipeline_hooks:build_hooks - pipeline_hooks:pipeline_hooks - pipeline_processing:build_finished +- pipeline_processing:ci_build_prepare - pipeline_processing:build_queue - pipeline_processing:build_success - pipeline_processing:pipeline_process diff --git a/app/workers/ci/build_prepare_worker.rb b/app/workers/ci/build_prepare_worker.rb new file mode 100644 index 00000000000..1a35a74ae53 --- /dev/null +++ b/app/workers/ci/build_prepare_worker.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Ci + class BuildPrepareWorker + include ApplicationWorker + include PipelineQueue + + queue_namespace :pipeline_processing + + def perform(build_id) + Ci::Build.find_by_id(build_id).try do |build| + Ci::PrepareBuildService.new(build).execute + end + end + end +end diff --git a/lib/gitlab/ci/build/prerequisite/base.rb b/lib/gitlab/ci/build/prerequisite/base.rb new file mode 100644 index 00000000000..156aa22d95b --- /dev/null +++ b/lib/gitlab/ci/build/prerequisite/base.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Build + module Prerequisite + class Base + include Utils::StrongMemoize + + attr_reader :build + + def initialize(build) + @build = build + end + + def unmet? + raise NotImplementedError + end + + def complete! + raise NotImplementedError + end + end + end + end + end +end diff --git a/lib/gitlab/ci/build/prerequisite/factory.rb b/lib/gitlab/ci/build/prerequisite/factory.rb new file mode 100644 index 00000000000..6b9c17bba07 --- /dev/null +++ b/lib/gitlab/ci/build/prerequisite/factory.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Build + module Prerequisite + class Factory + attr_reader :build + + def self.prerequisites + [] + end + + def initialize(build) + @build = build + end + + def unmet + build_prerequisites.select(&:unmet?) + end + + private + + def build_prerequisites + self.class.prerequisites.map do |prerequisite| + prerequisite.new(build) + end + end + end + end + end + end +end diff --git a/lib/gitlab/ci/status/build/failed.rb b/lib/gitlab/ci/status/build/failed.rb index d40454df737..76dfe7b7639 100644 --- a/lib/gitlab/ci/status/build/failed.rb +++ b/lib/gitlab/ci/status/build/failed.rb @@ -15,7 +15,8 @@ module Gitlab runner_unsupported: 'unsupported runner', stale_schedule: 'stale schedule', job_execution_timeout: 'job execution timeout', - archived_failure: 'archived failure' + archived_failure: 'archived failure', + unmet_prerequisites: 'unmet prerequisites' }.freeze private_constant :REASONS diff --git a/spec/features/projects/badges/pipeline_badge_spec.rb b/spec/features/projects/badges/pipeline_badge_spec.rb index 96efa06d843..4ac4e8f0fcb 100644 --- a/spec/features/projects/badges/pipeline_badge_spec.rb +++ b/spec/features/projects/badges/pipeline_badge_spec.rb @@ -47,10 +47,11 @@ describe 'Pipeline Badge' do before do # Prevent skipping directly to 'pending' allow(Ci::BuildPrepareWorker).to receive(:perform_async) + allow(job).to receive(:prerequisites).and_return([double]) end it 'displays the preparing badge' do - job.prepare + job.enqueue visit pipeline_project_badges_path(project, ref: ref, format: :svg) diff --git a/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb new file mode 100644 index 00000000000..5187f99a441 --- /dev/null +++ b/spec/lib/gitlab/ci/build/prerequisite/factory_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Build::Prerequisite::Factory do + let(:build) { create(:ci_build) } + + describe '.for_build' do + let(:kubernetes_namespace) do + instance_double( + Gitlab::Ci::Build::Prerequisite::KubernetesNamespace, + unmet?: unmet) + end + + subject { described_class.new(build).unmet } + + before do + expect(Gitlab::Ci::Build::Prerequisite::KubernetesNamespace) + .to receive(:new).with(build).and_return(kubernetes_namespace) + end + + context 'prerequisite is unmet' do + let(:unmet) { true } + + it { is_expected.to eq [kubernetes_namespace] } + end + + context 'prerequisite is met' do + let(:unmet) { false } + + it { is_expected.to be_empty } + end + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index 9ca4241d7d8..b31c4fcceb3 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -186,6 +186,37 @@ describe Ci::Build do end end + describe '#enqueue' do + let(:build) { create(:ci_build, :created) } + + subject { build.enqueue } + + before do + allow(build).to receive(:any_unmet_prerequisites?).and_return(has_prerequisites) + allow(Ci::PrepareBuildService).to receive(:perform_async) + end + + context 'build has unmet prerequisites' do + let(:has_prerequisites) { true } + + it 'transitions to preparing' do + subject + + expect(build).to be_preparing + end + end + + context 'build has no prerequisites' do + let(:has_prerequisites) { false } + + it 'transitions to pending' do + subject + + expect(build).to be_pending + end + end + end + describe '#actionize' do context 'when build is a created' do before do @@ -344,6 +375,18 @@ describe Ci::Build do expect(build).to be_pending end + + context 'build has unmet prerequisites' do + before do + allow(build).to receive(:prerequisites).and_return([double]) + end + + it 'transits to preparing' do + subject + + expect(build).to be_preparing + end + end end end @@ -2928,6 +2971,20 @@ describe Ci::Build do end end + describe 'state transition: any => [:preparing]' do + let(:build) { create(:ci_build, :created) } + + before do + allow(build).to receive(:prerequisites).and_return([double]) + end + + it 'queues BuildPrepareWorker' do + expect(Ci::BuildPrepareWorker).to receive(:perform_async).with(build.id) + + build.enqueue + end + end + describe 'state transition: any => [:pending]' do let(:build) { create(:ci_build, :created) } diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index d35caac33dc..2ac056f63b2 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1804,7 +1804,11 @@ describe Ci::Pipeline, :mailer do context 'on prepare' do before do - build.prepare + # Prevent skipping directly to 'pending' + allow(build).to receive(:prerequisites).and_return([double]) + allow(Ci::BuildPrepareWorker).to receive(:perform_async) + + build.enqueue end it { is_expected.to eq('preparing') } diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb index 1d241bf6000..e2b7f5c6ee2 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -489,6 +489,12 @@ describe CommitStatus do it { is_expected.to be_script_failure } end + + context 'when failure_reason is unmet_prerequisites' do + let(:reason) { :unmet_prerequisites } + + it { is_expected.to be_unmet_prerequisites } + end end describe 'ensure stage assignment' do diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb index 9087cccb759..3ccedd8dd06 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -918,6 +918,15 @@ describe API::Runner, :clean_gitlab_redis_shared_state do it { expect(job).to be_job_execution_timeout } end + + context 'when failure_reason is unmet_prerequisites' do + before do + update_job(state: 'failed', failure_reason: 'unmet_prerequisites') + job.reload + end + + it { expect(job).to be_unmet_prerequisites } + end end context 'when trace is given' do diff --git a/spec/services/ci/prepare_build_service_spec.rb b/spec/services/ci/prepare_build_service_spec.rb new file mode 100644 index 00000000000..1797f8f964f --- /dev/null +++ b/spec/services/ci/prepare_build_service_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::PrepareBuildService do + describe '#execute' do + let(:build) { create(:ci_build, :preparing) } + + subject { described_class.new(build).execute } + + before do + allow(build).to receive(:prerequisites).and_return(prerequisites) + end + + shared_examples 'build enqueueing' do + it 'enqueues the build' do + expect(build).to receive(:enqueue).once + + subject + end + end + + context 'build has unmet prerequisites' do + let(:prerequisite) { double(complete!: true) } + let(:prerequisites) { [prerequisite] } + + it 'completes each prerequisite' do + expect(prerequisites).to all(receive(:complete!)) + + subject + end + + include_examples 'build enqueueing' + + context 'prerequisites fail to complete' do + before do + allow(build).to receive(:enqueue).and_return(false) + end + + it 'drops the build' do + expect(build).to receive(:drop!).with(:unmet_prerequisites).once + + subject + end + end + end + + context 'build has no prerequisites' do + let(:prerequisites) { [] } + + include_examples 'build enqueueing' + end + end +end diff --git a/spec/workers/ci/build_prepare_worker_spec.rb b/spec/workers/ci/build_prepare_worker_spec.rb new file mode 100644 index 00000000000..9f76696ee66 --- /dev/null +++ b/spec/workers/ci/build_prepare_worker_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Ci::BuildPrepareWorker do + subject { described_class.new.perform(build_id) } + + context 'build exists' do + let(:build) { create(:ci_build) } + let(:build_id) { build.id } + let(:service) { double(execute: true) } + + it 'calls the prepare build service' do + expect(Ci::PrepareBuildService).to receive(:new).with(build).and_return(service) + expect(service).to receive(:execute).once + + subject + end + end + + context 'build does not exist' do + let(:build_id) { -1 } + + it 'does not attempt to prepare the build' do + expect(Ci::PrepareBuildService).not_to receive(:new) + + subject + end + end +end -- GitLab From 98a14a498dc3ffe6ea8bcd7db62e8bada5d2eb45 Mon Sep 17 00:00:00 2001 From: Tiger Date: Mon, 4 Mar 2019 11:00:40 +1100 Subject: [PATCH 3/8] Add build prerequisite for Kubernetes namespaces Builds that have deployments require Kubernetes resources to be created before the build can be deployed. These resources are no longer created when the cluster is created, which allows us to only create the resources required by each specific build. --- ...115-just-in-time-k8s-resource-creation.yml | 5 ++ lib/gitlab/ci/build/prerequisite/factory.rb | 2 +- .../prerequisite/kubernetes_namespace.rb | 48 +++++++++++++ .../prerequisite/kubernetes_namespace_spec.rb | 72 +++++++++++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/57115-just-in-time-k8s-resource-creation.yml create mode 100644 lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb create mode 100644 spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb diff --git a/changelogs/unreleased/57115-just-in-time-k8s-resource-creation.yml b/changelogs/unreleased/57115-just-in-time-k8s-resource-creation.yml new file mode 100644 index 00000000000..2141c75ec72 --- /dev/null +++ b/changelogs/unreleased/57115-just-in-time-k8s-resource-creation.yml @@ -0,0 +1,5 @@ +--- +title: Create Kubernetes resources for projects when their deployment jobs run. +merge_request: 25586 +author: +type: changed diff --git a/lib/gitlab/ci/build/prerequisite/factory.rb b/lib/gitlab/ci/build/prerequisite/factory.rb index 6b9c17bba07..60cdf7af418 100644 --- a/lib/gitlab/ci/build/prerequisite/factory.rb +++ b/lib/gitlab/ci/build/prerequisite/factory.rb @@ -8,7 +8,7 @@ module Gitlab attr_reader :build def self.prerequisites - [] + [KubernetesNamespace] end def initialize(build) diff --git a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb new file mode 100644 index 00000000000..d1b59d0a64c --- /dev/null +++ b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Build + module Prerequisite + class KubernetesNamespace < Base + ## + # Cluster settings may have changed since the last deploy, + # so we must always ensure the namespace is up to date. + # + def unmet? + build.has_deployment? && clusters_missing_namespaces.present? + end + + def complete! + return unless unmet? + + clusters_missing_namespaces.each do |cluster| + create_or_update_namespace(cluster) + end + end + + private + + def project + build.project + end + + def create_or_update_namespace(cluster) + kubernetes_namespace = cluster.find_or_initialize_kubernetes_namespace_for_project(project) + + Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService.new( + cluster: cluster, + kubernetes_namespace: kubernetes_namespace + ).execute + end + + def clusters_missing_namespaces + strong_memoize(:clusters_missing_namespaces) do + project.all_clusters.missing_kubernetes_namespace(project.kubernetes_namespaces).to_a + end + end + end + end + end + end +end diff --git a/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb new file mode 100644 index 00000000000..6f6e4abc0c8 --- /dev/null +++ b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb @@ -0,0 +1,72 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Build::Prerequisite::KubernetesNamespace do + let(:build) { create(:ci_build) } + + describe '#unmet?' do + subject { described_class.new(build).unmet? } + + context 'build has no deployment' do + before do + expect(build.deployment).to be_nil + end + + it { is_expected.to be_falsey } + end + + context 'build has a deployment, and no existing kubernetes namespace' do + let!(:deployment) { create(:deployment, deployable: build) } + let!(:cluster) { create(:cluster, projects: [build.project]) } + + before do + expect(build.project.kubernetes_namespaces).to be_empty + end + + it { is_expected.to be_truthy } + end + + context 'build has a deployment and kubernetes namespaces' do + let!(:deployment) { create(:deployment, deployable: build) } + let!(:cluster) { create(:cluster, projects: [build.project]) } + let!(:kubernetes_namespace) { create(:cluster_kubernetes_namespace, cluster: cluster) } + + it { is_expected.to be_falsey } + end + end + + describe '#complete!' do + let(:cluster) { create(:cluster, projects: [build.project]) } + let(:service) { double(execute: true) } + + subject { described_class.new(build).complete! } + + context 'completion is required' do + let!(:deployment) { create(:deployment, deployable: build) } + + it 'creates a kubernetes namespace' do + expect(Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService) + .to receive(:new) + .with(cluster: cluster, kubernetes_namespace: instance_of(Clusters::KubernetesNamespace)) + .and_return(service) + + expect(service).to receive(:execute).once + + subject + end + end + + context 'completion is not required' do + before do + expect(build.deployment).to be_nil + end + + it 'does not create a namespace' do + expect(Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService).not_to receive(:new) + + subject + end + end + end +end -- GitLab From 42c6ccd2098ec98e5244e743a0c39634f076f66f Mon Sep 17 00:00:00 2001 From: Tiger Date: Mon, 4 Mar 2019 12:14:47 +1100 Subject: [PATCH 4/8] Tweak FactoryBot definition for clusters Only create an associated project or group if there were none already specified. --- spec/factories/clusters/clusters.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/factories/clusters/clusters.rb b/spec/factories/clusters/clusters.rb index a2e5f4862db..1cc3c0e03d8 100644 --- a/spec/factories/clusters/clusters.rb +++ b/spec/factories/clusters/clusters.rb @@ -12,7 +12,7 @@ FactoryBot.define do cluster_type { Clusters::Cluster.cluster_types[:project_type] } before(:create) do |cluster, evaluator| - cluster.projects << create(:project, :repository) + cluster.projects << create(:project, :repository) unless cluster.projects.present? end end @@ -20,7 +20,7 @@ FactoryBot.define do cluster_type { Clusters::Cluster.cluster_types[:group_type] } before(:create) do |cluster, evalutor| - cluster.groups << create(:group) + cluster.groups << create(:group) unless cluster.groups.present? end end -- GitLab From 759dab5b69f53a861045ebbc84836f83c7502af2 Mon Sep 17 00:00:00 2001 From: Tiger Date: Tue, 12 Mar 2019 17:37:37 +1100 Subject: [PATCH 5/8] Add feature flag for build preparing state The flag is on by default, but allows us to revert back to the old behaviour if we encounter any problems. --- app/models/ci/build.rb | 2 ++ app/workers/cluster_configure_worker.rb | 2 ++ .../cluster_project_configure_worker.rb | 2 ++ spec/models/ci/build_spec.rb | 30 +++++++++++++++++++ spec/requests/api/project_clusters_spec.rb | 1 - spec/services/projects/create_service_spec.rb | 1 + .../projects/transfer_service_spec.rb | 1 + spec/workers/cluster_configure_worker_spec.rb | 16 ++++++++++ .../cluster_project_configure_worker_spec.rb | 21 +++++++++++++ 9 files changed, 75 insertions(+), 1 deletion(-) create mode 100644 spec/workers/cluster_project_configure_worker_spec.rb diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index a293afaed08..59f47effff7 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -370,6 +370,8 @@ module Ci end def any_unmet_prerequisites? + return false unless Feature.enabled?(:ci_preparing_state, default_enabled: true) + prerequisites.present? end diff --git a/app/workers/cluster_configure_worker.rb b/app/workers/cluster_configure_worker.rb index 63e6cc147be..b984dee5b21 100644 --- a/app/workers/cluster_configure_worker.rb +++ b/app/workers/cluster_configure_worker.rb @@ -5,6 +5,8 @@ class ClusterConfigureWorker include ClusterQueue def perform(cluster_id) + return if Feature.enabled?(:ci_preparing_state, default_enabled: true) + Clusters::Cluster.find_by_id(cluster_id).try do |cluster| Clusters::RefreshService.create_or_update_namespaces_for_cluster(cluster) end diff --git a/app/workers/cluster_project_configure_worker.rb b/app/workers/cluster_project_configure_worker.rb index 497e57c0d0b..d7bea69a01c 100644 --- a/app/workers/cluster_project_configure_worker.rb +++ b/app/workers/cluster_project_configure_worker.rb @@ -5,6 +5,8 @@ class ClusterProjectConfigureWorker include ClusterQueue def perform(project_id) + return if Feature.enabled?(:ci_preparing_state, default_enabled: true) + project = Project.find(project_id) ::Clusters::RefreshService.create_or_update_namespaces_for_project(project) diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index b31c4fcceb3..7500e6ae5b1 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2919,6 +2919,36 @@ describe Ci::Build do end end + describe '#any_unmet_prerequisites?' do + let(:build) { create(:ci_build, :created) } + + subject { build.any_unmet_prerequisites? } + + context 'build has prerequisites' do + before do + allow(build).to receive(:prerequisites).and_return([double]) + end + + it { is_expected.to be_truthy } + + context 'and the ci_preparing_state feature is disabled' do + before do + stub_feature_flags(ci_preparing_state: false) + end + + it { is_expected.to be_falsey } + end + end + + context 'build does not have prerequisites' do + before do + allow(build).to receive(:prerequisites).and_return([]) + end + + it { is_expected.to be_falsey } + end + end + describe '#yaml_variables' do let(:build) { create(:ci_build, pipeline: pipeline, yaml_variables: variables) } diff --git a/spec/requests/api/project_clusters_spec.rb b/spec/requests/api/project_clusters_spec.rb index 9bab1f95150..4e42e233b4c 100644 --- a/spec/requests/api/project_clusters_spec.rb +++ b/spec/requests/api/project_clusters_spec.rb @@ -331,7 +331,6 @@ describe API::ProjectClusters do it 'should update cluster attributes' do expect(cluster.platform_kubernetes.namespace).to eq('new-namespace') - expect(cluster.kubernetes_namespace.namespace).to eq('new-namespace') end end diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index d1b110b9806..e8418b09dc2 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -276,6 +276,7 @@ describe Projects::CreateService, '#execute' do before do group.add_owner(user) + stub_feature_flags(ci_preparing_state: false) expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).to receive(:namespace_creator).and_return(service_account_creator) expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).to receive(:new).and_return(secrets_fetcher) end diff --git a/spec/services/projects/transfer_service_spec.rb b/spec/services/projects/transfer_service_spec.rb index aae50d5307f..4efd360cb30 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -83,6 +83,7 @@ describe Projects::TransferService do subject { transfer_project(project, user, group) } before do + stub_feature_flags(ci_preparing_state: false) expect(Clusters::Gcp::Kubernetes::CreateOrUpdateServiceAccountService).to receive(:namespace_creator).and_return(service_account_creator) expect(Clusters::Gcp::Kubernetes::FetchKubernetesTokenService).to receive(:new).and_return(secrets_fetcher) end diff --git a/spec/workers/cluster_configure_worker_spec.rb b/spec/workers/cluster_configure_worker_spec.rb index 6918ee3d7d8..83f76809435 100644 --- a/spec/workers/cluster_configure_worker_spec.rb +++ b/spec/workers/cluster_configure_worker_spec.rb @@ -4,6 +4,11 @@ require 'spec_helper' describe ClusterConfigureWorker, '#perform' do let(:worker) { described_class.new } + let(:ci_preparing_state_enabled) { false } + + before do + stub_feature_flags(ci_preparing_state: ci_preparing_state_enabled) + end context 'when group cluster' do let(:cluster) { create(:cluster, :group, :provided_by_gcp) } @@ -66,4 +71,15 @@ describe ClusterConfigureWorker, '#perform' do described_class.new.perform(123) end end + + context 'ci_preparing_state feature is enabled' do + let(:cluster) { create(:cluster) } + let(:ci_preparing_state_enabled) { true } + + it 'does not configure the cluster' do + expect(Clusters::RefreshService).not_to receive(:create_or_update_namespaces_for_cluster) + + described_class.new.perform(cluster.id) + end + end end diff --git a/spec/workers/cluster_project_configure_worker_spec.rb b/spec/workers/cluster_project_configure_worker_spec.rb new file mode 100644 index 00000000000..afdea55adf4 --- /dev/null +++ b/spec/workers/cluster_project_configure_worker_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe ClusterProjectConfigureWorker, '#perform' do + let(:worker) { described_class.new } + + context 'ci_preparing_state feature is enabled' do + let(:cluster) { create(:cluster) } + + before do + stub_feature_flags(ci_preparing_state: true) + end + + it 'does not configure the cluster' do + expect(Clusters::RefreshService).not_to receive(:create_or_update_namespaces_for_project) + + described_class.new.perform(cluster.id) + end + end +end -- GitLab From 89b0bc04b9927abc85ce5fc3735438f956a8d5a2 Mon Sep 17 00:00:00 2001 From: Tiger Date: Wed, 13 Mar 2019 14:06:54 +1100 Subject: [PATCH 6/8] Create one Kubernetes namespace for a deployment Instead of creating a Kubernetes namespace on every cluster related to a project, only create one on the cluster the project is about to be deployed to. --- app/models/deployment.rb | 4 ++ lib/gitlab/ci/build/prerequisite/base.rb | 2 - .../prerequisite/kubernetes_namespace.rb | 22 ++++------- .../prerequisite/kubernetes_namespace_spec.rb | 37 +++++++++++-------- spec/models/deployment_spec.rb | 28 ++++++++++++++ 5 files changed, 61 insertions(+), 32 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 811e623b7f7..428edfd88de 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -78,6 +78,10 @@ class Deployment < ActiveRecord::Base Commit.truncate_sha(sha) end + def cluster + project.deployment_platform(environment: environment.name)&.cluster + end + def last? self == environment.last_deployment end diff --git a/lib/gitlab/ci/build/prerequisite/base.rb b/lib/gitlab/ci/build/prerequisite/base.rb index 156aa22d95b..d3c37a3e02e 100644 --- a/lib/gitlab/ci/build/prerequisite/base.rb +++ b/lib/gitlab/ci/build/prerequisite/base.rb @@ -5,8 +5,6 @@ module Gitlab module Build module Prerequisite class Base - include Utils::StrongMemoize - attr_reader :build def initialize(build) diff --git a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb index d1b59d0a64c..3d66b13caa6 100644 --- a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb +++ b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb @@ -10,37 +10,29 @@ module Gitlab # so we must always ensure the namespace is up to date. # def unmet? - build.has_deployment? && clusters_missing_namespaces.present? + deployment_cluster.present? end def complete! return unless unmet? - clusters_missing_namespaces.each do |cluster| - create_or_update_namespace(cluster) - end + create_or_update_namespace end private - def project - build.project + def deployment_cluster + build.deployment&.cluster end - def create_or_update_namespace(cluster) - kubernetes_namespace = cluster.find_or_initialize_kubernetes_namespace_for_project(project) + def create_or_update_namespace + kubernetes_namespace = deployment_cluster.find_or_initialize_kubernetes_namespace_for_project(build.project) Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService.new( - cluster: cluster, + cluster: deployment_cluster, kubernetes_namespace: kubernetes_namespace ).execute end - - def clusters_missing_namespaces - strong_memoize(:clusters_missing_namespaces) do - project.all_clusters.missing_kubernetes_namespace(project.kubernetes_namespaces).to_a - end - end end end end diff --git a/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb index 6f6e4abc0c8..ba87863c978 100644 --- a/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb +++ b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb @@ -16,34 +16,41 @@ describe Gitlab::Ci::Build::Prerequisite::KubernetesNamespace do it { is_expected.to be_falsey } end - context 'build has a deployment, and no existing kubernetes namespace' do + context 'build has a deployment' do let!(:deployment) { create(:deployment, deployable: build) } - let!(:cluster) { create(:cluster, projects: [build.project]) } - before do - expect(build.project.kubernetes_namespaces).to be_empty - end + context 'and a cluster to deploy to' do + let(:cluster) { create(:cluster, projects: [build.project]) } - it { is_expected.to be_truthy } - end + before do + allow(build.deployment).to receive(:cluster).and_return(cluster) + end - context 'build has a deployment and kubernetes namespaces' do - let!(:deployment) { create(:deployment, deployable: build) } - let!(:cluster) { create(:cluster, projects: [build.project]) } - let!(:kubernetes_namespace) { create(:cluster_kubernetes_namespace, cluster: cluster) } + it { is_expected.to be_truthy } + end - it { is_expected.to be_falsey } + context 'and no cluster to deploy to' do + before do + expect(deployment.cluster).to be_nil + end + + it { is_expected.to be_falsey } + end end end describe '#complete!' do - let(:cluster) { create(:cluster, projects: [build.project]) } + let!(:deployment) { create(:deployment, deployable: build) } let(:service) { double(execute: true) } subject { described_class.new(build).complete! } context 'completion is required' do - let!(:deployment) { create(:deployment, deployable: build) } + let(:cluster) { create(:cluster, projects: [build.project]) } + + before do + allow(build.deployment).to receive(:cluster).and_return(cluster) + end it 'creates a kubernetes namespace' do expect(Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService) @@ -59,7 +66,7 @@ describe Gitlab::Ci::Build::Prerequisite::KubernetesNamespace do context 'completion is not required' do before do - expect(build.deployment).to be_nil + expect(deployment.cluster).to be_nil end it 'does not create a namespace' do diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index a8d53cfcd7d..5fce9504334 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -356,4 +356,32 @@ describe Deployment do end end end + + describe '#cluster' do + let(:deployment) { create(:deployment) } + let(:project) { deployment.project } + let(:environment) { deployment.environment } + + subject { deployment.cluster } + + before do + expect(project).to receive(:deployment_platform) + .with(environment: environment.name).and_call_original + end + + context 'project has no deployment platform' do + before do + expect(project.clusters).to be_empty + end + + it { is_expected.to be_nil } + end + + context 'project has a deployment platform' do + let!(:cluster) { create(:cluster, projects: [project]) } + let!(:platform) { create(:cluster_platform_kubernetes, cluster: cluster) } + + it { is_expected.to eq cluster } + end + end end -- GitLab From 325d504c3c9697c73130aef67e8b32c99544b453 Mon Sep 17 00:00:00 2001 From: Tiger Date: Mon, 18 Mar 2019 13:02:39 +1100 Subject: [PATCH 7/8] Don't recreate Kubernetes namespaces if they exist Instead of attempting to create or update a Kubernetes namespace on every deploy, only do so when we know it doesn't exist yet. --- lib/gitlab/ci/build/prerequisite/base.rb | 2 ++ .../ci/build/prerequisite/kubernetes_namespace.rb | 14 +++++++------- .../prerequisite/kubernetes_namespace_spec.rb | 6 ++++++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/gitlab/ci/build/prerequisite/base.rb b/lib/gitlab/ci/build/prerequisite/base.rb index d3c37a3e02e..156aa22d95b 100644 --- a/lib/gitlab/ci/build/prerequisite/base.rb +++ b/lib/gitlab/ci/build/prerequisite/base.rb @@ -5,6 +5,8 @@ module Gitlab module Build module Prerequisite class Base + include Utils::StrongMemoize + attr_reader :build def initialize(build) diff --git a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb index 3d66b13caa6..41135ae62bb 100644 --- a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb +++ b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb @@ -5,12 +5,8 @@ module Gitlab module Build module Prerequisite class KubernetesNamespace < Base - ## - # Cluster settings may have changed since the last deploy, - # so we must always ensure the namespace is up to date. - # def unmet? - deployment_cluster.present? + deployment_cluster.present? && kubernetes_namespace.new_record? end def complete! @@ -25,9 +21,13 @@ module Gitlab build.deployment&.cluster end - def create_or_update_namespace - kubernetes_namespace = deployment_cluster.find_or_initialize_kubernetes_namespace_for_project(build.project) + def kubernetes_namespace + strong_memoize(:kubernetes_namespace) do + deployment_cluster.find_or_initialize_kubernetes_namespace_for_project(build.project) + end + end + def create_or_update_namespace Clusters::Gcp::Kubernetes::CreateOrUpdateNamespaceService.new( cluster: deployment_cluster, kubernetes_namespace: kubernetes_namespace diff --git a/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb index ba87863c978..62dcd80fad7 100644 --- a/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb +++ b/spec/lib/gitlab/ci/build/prerequisite/kubernetes_namespace_spec.rb @@ -27,6 +27,12 @@ describe Gitlab::Ci::Build::Prerequisite::KubernetesNamespace do end it { is_expected.to be_truthy } + + context 'and a namespace is already created for this project' do + let!(:kubernetes_namespace) { create(:cluster_kubernetes_namespace, cluster: cluster, project: build.project) } + + it { is_expected.to be_falsey } + end end context 'and no cluster to deploy to' do -- GitLab From 0c3df3b56973d78345c6791cc3882a50d916cbc8 Mon Sep 17 00:00:00 2001 From: Tiger Date: Wed, 20 Mar 2019 16:07:12 +1100 Subject: [PATCH 8/8] Amend cluster and auto devops troubleshooting docs Update these sections to reflect Kubernetes resources now being created as a build prerequisite. Remove section about deploys not being triggered as it is no longer accurate. --- doc/topics/autodevops/index.md | 4 ++++ doc/user/project/clusters/index.md | 25 +++++++++++++------------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/doc/topics/autodevops/index.md b/doc/topics/autodevops/index.md index c73e810545e..301c8224d2e 100644 --- a/doc/topics/autodevops/index.md +++ b/doc/topics/autodevops/index.md @@ -1005,6 +1005,10 @@ planned for a subsequent release. buildpack](#custom-buildpacks). - Auto Test may fail because of a mismatch between testing frameworks. In this case, you may need to customize your `.gitlab-ci.yml` with your test commands. +- Auto Deploy may fail if it is unable to create a Kubernetes namespace and + service account for your project. See the + [troubleshooting failed deployments](../../user/project/clusters/index.md#troubleshooting-failed-deployment-jobs) + section to debug why these resources were not created. ### Disable the banner instance wide diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md index d1206b0c80a..ab8b314f862 100644 --- a/doc/user/project/clusters/index.md +++ b/doc/user/project/clusters/index.md @@ -556,26 +556,27 @@ NOTE: **NOTE:** Prior to GitLab 11.5, `KUBE_TOKEN` was the Kubernetes token of the main service account of the cluster integration. -### Troubleshooting missing `KUBECONFIG` or `KUBE_TOKEN` +### Troubleshooting failed deployment jobs -GitLab will create a new service account specifically for your CI builds. The -new service account is created when the cluster is added to the project. -Sometimes there may be errors that cause the service account creation to fail. +GitLab will create a namespace and service account specifically for your +deployment jobs. These resources are created just before the deployment +job starts. Sometimes there may be errors that cause their creation to fail. -In such instances, your build will not be passed the `KUBECONFIG` or -`KUBE_TOKEN` variables and, if you are using Auto DevOps, your Auto DevOps -pipelines will no longer trigger a `production` deploy build. You will need to -check the [logs](../../../administration/logs.md) to debug why the service -account creation failed. +In such instances, your job will fail with the message: + +```The job failed to complete prerequisite tasks``` + +You will need to check the [logs](../../../administration/logs.md) to debug +why the namespace and service account creation failed. A common reason for failure is that the token you gave GitLab did not have [`cluster-admin`](https://kubernetes.io/docs/reference/access-authn-authz/rbac/#user-facing-roles) privileges as GitLab expects. -Another common problem for why these variables are not being passed to your -builds is that they must have a matching +Another common problem is caused by a missing `KUBECONFIG` or `KUBE_TOKEN`. +To be passed to your job, it must have a matching [`environment:name`](../../../ci/environments.md#defining-environments). If -your build has no `environment:name` set, it will not be passed the Kubernetes +your job has no `environment:name` set, it will not be passed the Kubernetes credentials. ## Monitoring your Kubernetes cluster **[ULTIMATE]** -- GitLab