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 49b9b7014ae2..3ab61cc5c47b 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 44556060c651..203f695d885c 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 2b6319ddd4f9..13c518a1ba80 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 f4d568d02acb..a59bb31bdcb7 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 adffdc0355e3..ae74f569415b 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -82,10 +82,14 @@ class Pipeline < ActiveRecord::Base 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 @@ class Pipeline < ActiveRecord::Base # 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 @@ class Pipeline < ActiveRecord::Base 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 @@ class Pipeline < ActiveRecord::Base 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 @@ def update_status 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 0389945191e0..098f5189517d 100644 --- a/app/models/ci/stage.rb +++ b/app/models/ci/stage.rb @@ -39,10 +39,14 @@ class Stage < ActiveRecord::Base 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 @@ def update_status 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 7f6562b63e5e..bfb13fa0ce8d 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 0d2be4c61abe..8882f48c2819 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 @@ def status_sql 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 @@ def status_sql 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 @@ def all_state_names 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 @@ def all_state_names 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 @@ def all_state_names 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 64c3dfcd10bb..2c5f9654496f 100644 --- a/lib/gitlab/badge/pipeline/template.rb +++ b/lib/gitlab/badge/pipeline/template.rb @@ -15,6 +15,7 @@ class Template < Badge::Template 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 6e4bfe23f2b5..f7d0715e6170 100644 --- a/lib/gitlab/ci/status/build/factory.rb +++ b/lib/gitlab/ci/status/build/factory.rb @@ -11,6 +11,7 @@ def self.extended_statuses 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 000000000000..1fddcb05f791 --- /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 000000000000..62985d0a9f92 --- /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 31b9af928054..471e5995ce98 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 0b3e67b49878..067391c11795 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -75,6 +75,10 @@ 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 ee5d27355f13..aa5ccbda6cd9 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -50,6 +50,14 @@ 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 381bf07f6a0e..848a31e96c19 100644 --- a/spec/factories/commit_statuses.rb +++ b/spec/factories/commit_statuses.rb @@ -33,6 +33,10 @@ 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 dee818989284..96efa06d843a 100644 --- a/spec/features/projects/badges/pipeline_badge_spec.rb +++ b/spec/features/projects/badges/pipeline_badge_spec.rb @@ -41,6 +41,24 @@ 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 36b8c15b8b60..84d4312010fd 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -21,6 +21,11 @@ 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 @@ 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 88d7c9ef8bd6..9dc505573d4a 100644 --- a/spec/features/projects/pipelines/pipelines_spec.rb +++ b/spec/features/projects/pipelines/pipelines_spec.rb @@ -282,6 +282,30 @@ 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 20fa4f879c3e..bcef0b7e120c 100644 --- a/spec/lib/gitlab/badge/pipeline/template_spec.rb +++ b/spec/lib/gitlab/badge/pipeline/template_spec.rb @@ -59,6 +59,16 @@ 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 000000000000..4d8945845ba5 --- /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 000000000000..7211c0e506db --- /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 7eeaa7a18ef4..d35caac33dc8 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1201,16 +1201,28 @@ def create_build(name, status) 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 @@ def create_build(name, status) 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 @@ def create_pipeline(status, ref, sha, project) 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 8b7c88805c19..1d241bf6000c 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -49,6 +49,16 @@ def create_status(**opts) 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 @@ def create_status(**opts) 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 @@ def create_status(**opts) 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 6b1038cb8fd9..e8b1eba67cc0 100644 --- a/spec/models/concerns/has_status_spec.rb +++ b/spec/models/concerns/has_status_spec.rb @@ -34,6 +34,22 @@ 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 @@ 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 '.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 '.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 a629db82c19b..a293afaed088 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -172,6 +172,10 @@ def retry(build, current_user) 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 @@ def retry(build, current_user) 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 @@ def retry(build, current_user) 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 @@ def latest? !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 bfb13fa0ce8d..5f66a6613243 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 @@ def has_trace? 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 152105d94292..45e08fa18fe0 100644 --- a/app/models/commit_status_enums.rb +++ b/app/models/commit_status_enums.rb @@ -14,7 +14,8 @@ def self.failure_reasons 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 0cd77da63037..28a25c8b7a30 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 000000000000..32f11438b799 --- /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 b2d88567e0e8..6ebd756d3da5 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 000000000000..1a35a74ae53e --- /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 000000000000..156aa22d95b2 --- /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 000000000000..6b9c17bba077 --- /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 d40454df7379..76dfe7b76399 100644 --- a/lib/gitlab/ci/status/build/failed.rb +++ b/lib/gitlab/ci/status/build/failed.rb @@ -15,7 +15,8 @@ class Failed < Status::Extended 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 96efa06d843a..4ac4e8f0fcb9 100644 --- a/spec/features/projects/badges/pipeline_badge_spec.rb +++ b/spec/features/projects/badges/pipeline_badge_spec.rb @@ -47,10 +47,11 @@ 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 000000000000..5187f99a441f --- /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 9ca4241d7d83..b31c4fcceb38 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -186,6 +186,37 @@ 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 @@ 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 @@ def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now) 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 d35caac33dc8..2ac056f63b2b 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1804,7 +1804,11 @@ def create_pipeline(status, ref, sha, project) 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 1d241bf6000c..e2b7f5c6ee25 100644 --- a/spec/models/commit_status_spec.rb +++ b/spec/models/commit_status_spec.rb @@ -489,6 +489,12 @@ def create_status(**opts) 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 9087cccb7593..3ccedd8dd065 100644 --- a/spec/requests/api/runner_spec.rb +++ b/spec/requests/api/runner_spec.rb @@ -918,6 +918,15 @@ def request_job(token = runner.token, **params) 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 000000000000..1797f8f964fe --- /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 000000000000..9f76696ee66c --- /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 000000000000..2141c75ec727 --- /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 6b9c17bba077..60cdf7af418b 100644 --- a/lib/gitlab/ci/build/prerequisite/factory.rb +++ b/lib/gitlab/ci/build/prerequisite/factory.rb @@ -8,7 +8,7 @@ class Factory 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 000000000000..d1b59d0a64c7 --- /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 000000000000..6f6e4abc0c8f --- /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 a2e5f4862db5..1cc3c0e03d80 100644 --- a/spec/factories/clusters/clusters.rb +++ b/spec/factories/clusters/clusters.rb @@ -12,7 +12,7 @@ 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 @@ 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 a293afaed088..59f47effff70 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -370,6 +370,8 @@ def latest? 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 63e6cc147be1..b984dee5b218 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 497e57c0d0b6..d7bea69a01ca 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 b31c4fcceb38..7500e6ae5b1c 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -2919,6 +2919,36 @@ def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now) 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 9bab1f95150b..4e42e233b4c6 100644 --- a/spec/requests/api/project_clusters_spec.rb +++ b/spec/requests/api/project_clusters_spec.rb @@ -331,7 +331,6 @@ 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 d1b110b98062..e8418b09dc23 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -276,6 +276,7 @@ def wiki_repo(project) 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 aae50d5307f8..4efd360cb30a 100644 --- a/spec/services/projects/transfer_service_spec.rb +++ b/spec/services/projects/transfer_service_spec.rb @@ -83,6 +83,7 @@ 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 6918ee3d7d87..83f768094358 100644 --- a/spec/workers/cluster_configure_worker_spec.rb +++ b/spec/workers/cluster_configure_worker_spec.rb @@ -4,6 +4,11 @@ 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 @@ 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 000000000000..afdea55adf40 --- /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 811e623b7f7e..428edfd88ded 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -78,6 +78,10 @@ def short_sha 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 156aa22d95b2..d3c37a3e02e3 100644 --- a/lib/gitlab/ci/build/prerequisite/base.rb +++ b/lib/gitlab/ci/build/prerequisite/base.rb @@ -5,8 +5,6 @@ module Ci 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 d1b59d0a64c7..3d66b13caa62 100644 --- a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb +++ b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb @@ -10,37 +10,29 @@ class KubernetesNamespace < Base # 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 6f6e4abc0c8f..ba87863c9788 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 @@ 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 @@ 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 a8d53cfcd7d8..5fce95043342 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -356,4 +356,32 @@ 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 d3c37a3e02e3..156aa22d95b2 100644 --- a/lib/gitlab/ci/build/prerequisite/base.rb +++ b/lib/gitlab/ci/build/prerequisite/base.rb @@ -5,6 +5,8 @@ module Ci 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 3d66b13caa62..41135ae62bbc 100644 --- a/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb +++ b/lib/gitlab/ci/build/prerequisite/kubernetes_namespace.rb @@ -5,12 +5,8 @@ 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? - deployment_cluster.present? + deployment_cluster.present? && kubernetes_namespace.new_record? end def complete! @@ -25,9 +21,13 @@ def deployment_cluster 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 ba87863c9788..62dcd80fad74 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 @@ 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 c73e810545ef..301c8224d2e8 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 d1206b0c80a6..ab8b314f862d 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