diff options
23 files changed, 295 insertions, 145 deletions
diff --git a/app/assets/javascripts/blob/template_selector.js b/app/assets/javascripts/blob/template_selector.js index 6cfe354d277..b0de4dc8628 100644 --- a/app/assets/javascripts/blob/template_selector.js +++ b/app/assets/javascripts/blob/template_selector.js @@ -83,6 +83,7 @@ export default class TemplateSelector { if (this.editor instanceof $) { this.editor.get(0).dispatchEvent(this.autosizeUpdateEvent); + this.editor.trigger('input'); } } diff --git a/app/assets/javascripts/templates/issuable_template_selector.js b/app/assets/javascripts/templates/issuable_template_selector.js index 78a1c4fa8a8..10ad4170930 100644 --- a/app/assets/javascripts/templates/issuable_template_selector.js +++ b/app/assets/javascripts/templates/issuable_template_selector.js @@ -31,7 +31,8 @@ export default class IssuableTemplateSelector extends TemplateSelector { }); this.templateWarningEl.find('.js-close-btn').on('click', () => { - if (this.previousSelectedIndex) { + // Explicitly check against 0 value + if (this.previousSelectedIndex !== undefined) { this.dropdown.data('glDropdown').selectRowAtIndex(this.previousSelectedIndex); } else { this.reset(); @@ -109,6 +110,7 @@ export default class IssuableTemplateSelector extends TemplateSelector { } else { this.setEditorContent(this.currentTemplate, { skipFocus: false }); } + return; } } diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fba14f0100c..3dacd6a6224 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -12,7 +12,6 @@ module Ci include Presentable include Importable include Gitlab::Utils::StrongMemoize - include Deployable include HasRef BuildArchivedError = Class.new(StandardError) @@ -417,10 +416,6 @@ module Ci self.options.fetch(:environment, {}).fetch(:action, 'start') if self.options end - def has_deployment? - !!self.deployment - end - def outdated_deployment? success? && !deployment.try(:last?) end diff --git a/app/models/concerns/deployable.rb b/app/models/concerns/deployable.rb deleted file mode 100644 index 957b72f3721..00000000000 --- a/app/models/concerns/deployable.rb +++ /dev/null @@ -1,31 +0,0 @@ -# frozen_string_literal: true - -module Deployable - extend ActiveSupport::Concern - - included do - after_create :create_deployment - - def create_deployment - return unless starts_environment? && !has_deployment? - - environment = project.environments.find_or_create_by( - name: expanded_environment_name - ) - - # If we failed to persist envirionment record by validation error, such as name with invalid character, - # the job will fall back to a non-environment job. - return unless environment.persisted? - - create_deployment!( - cluster_id: environment.deployment_platform&.cluster_id, - project_id: environment.project_id, - environment: environment, - ref: ref, - tag: tag, - sha: sha, - user: user, - on_stop: on_stop) - end - end -end diff --git a/app/services/ci/retry_build_service.rb b/app/services/ci/retry_build_service.rb index 338495ba030..7a5e33c61ba 100644 --- a/app/services/ci/retry_build_service.rb +++ b/app/services/ci/retry_build_service.rb @@ -39,9 +39,18 @@ module Ci .where(name: build.name) .update_all(retried: true) - project.builds.create!(Hash[attributes]) + create_build!(attributes) end end # rubocop: enable CodeReuse/ActiveRecord + + private + + def create_build!(attributes) + build = project.builds.new(Hash[attributes]) + build.deployment = ::Gitlab::Ci::Pipeline::Seed::Deployment.new(build).to_resource + build.save! + build + end end end diff --git a/app/views/shared/form_elements/_apply_template_warning.html.haml b/app/views/shared/form_elements/_apply_template_warning.html.haml index 9027264d221..09ca59a520c 100644 --- a/app/views/shared/form_elements/_apply_template_warning.html.haml +++ b/app/views/shared/form_elements/_apply_template_warning.html.haml @@ -2,7 +2,7 @@ .offset-sm-2.col-sm-10 .warning_message.mb-0{ role: 'alert' } - %btn.js-close-btn.close{ type: "button", "aria-hidden": true, "aria-label": _("Close") } + %btn.js-close-btn.js-dismiss-btn.close{ type: "button", "aria-hidden": true, "aria-label": _("Close") } = sprite_icon("close") %p @@ -10,5 +10,5 @@ %button.js-override-template.btn.btn-warning.mr-2{ type: 'button' } = _("Apply template") - %button.js-cancel-btn.btn.btn-inverted{ type: 'button' } + %button.js-close-btn.js-cancel-btn.btn.btn-inverted{ type: 'button' } = _("Cancel") diff --git a/changelogs/unreleased/deployment-iid-transaction-improvement.yml b/changelogs/unreleased/deployment-iid-transaction-improvement.yml new file mode 100644 index 00000000000..aefc3349c35 --- /dev/null +++ b/changelogs/unreleased/deployment-iid-transaction-improvement.yml @@ -0,0 +1,6 @@ +--- +title: Reduce lock contention of deployment creation by allocating IID outside + of the pipeline transaction +merge_request: 17696 +author: +type: performance diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb index 1f6b3853069..fc9c540088b 100644 --- a/lib/gitlab/ci/pipeline/seed/build.rb +++ b/lib/gitlab/ci/pipeline/seed/build.rb @@ -73,7 +73,9 @@ module Gitlab if bridge? ::Ci::Bridge.new(attributes) else - ::Ci::Build.new(attributes) + ::Ci::Build.new(attributes).tap do |job| + job.deployment = Seed::Deployment.new(job).to_resource + end end end end diff --git a/lib/gitlab/ci/pipeline/seed/deployment.rb b/lib/gitlab/ci/pipeline/seed/deployment.rb new file mode 100644 index 00000000000..238cd845a0e --- /dev/null +++ b/lib/gitlab/ci/pipeline/seed/deployment.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Seed + class Deployment < Seed::Base + attr_reader :job, :environment + + def initialize(job) + @job = job + @environment = Seed::Environment.new(@job) + end + + def to_resource + return job.deployment if job.deployment + return unless job.starts_environment? + + deployment = ::Deployment.new(attributes) + deployment.environment = environment.to_resource + + # If there is a validation error on environment creation, such as + # the name contains invalid character, the job will fall back to a + # non-environment job. + return unless deployment.valid? && deployment.environment.valid? + + deployment.cluster_id = + deployment.environment.deployment_platform&.cluster_id + + # Allocate IID for deployments. + # This operation must be outside of transactions of pipeline creations. + deployment.ensure_project_iid! + + deployment + end + + private + + def attributes + { + project: job.project, + user: job.user, + ref: job.ref, + tag: job.tag, + sha: job.sha, + on_stop: job.on_stop + } + end + end + end + end + end +end diff --git a/lib/gitlab/ci/pipeline/seed/environment.rb b/lib/gitlab/ci/pipeline/seed/environment.rb new file mode 100644 index 00000000000..813a9e9e399 --- /dev/null +++ b/lib/gitlab/ci/pipeline/seed/environment.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Gitlab + module Ci + module Pipeline + module Seed + class Environment < Seed::Base + attr_reader :job + + def initialize(job) + @job = job + end + + def to_resource + find_environment || ::Environment.new(attributes) + end + + private + + def find_environment + job.project.environments.find_by_name(expanded_environment_name) + end + + def expanded_environment_name + job.expanded_environment_name + end + + def attributes + { + project: job.project, + name: expanded_environment_name + } + end + end + end + end + end +end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index c0f7948f963..0bd39d4cdcf 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -211,6 +211,17 @@ FactoryBot.define do build.project ||= build.pipeline.project end + trait :with_deployment do + after(:build) do |build, evaluator| + ## + # Build deployment/environment relations if environment name is set + # to the job. If `build.deployment` has already been set, it doesn't + # build a new instance. + build.deployment = + Gitlab::Ci::Pipeline::Seed::Deployment.new(build).to_resource + end + end + trait :tag do tag { true } end diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb index c06fee92d09..4b6f1672f08 100644 --- a/spec/features/projects/issuable_templates_spec.rb +++ b/spec/features/projects/issuable_templates_spec.rb @@ -131,8 +131,15 @@ describe 'issuable templates', :js do end it 'user selects "bug" template, then updates description, then selects "feature-proposal" template, then cancels template change' do - page.find('.js-template-warning .js-cancel-btn').click + page.find('.js-template-warning .js-close-btn.js-cancel-btn').click expect(find('textarea')['value']).to eq(updated_description) + expect(page).not_to have_content template_override_warning + end + + it 'user selects "bug" template, then updates description, then selects "feature-proposal" template, then dismiss the template warning' do + page.find('.js-template-warning .js-close-btn.js-dismiss-btn').click + expect(find('textarea')['value']).to eq(updated_description) + expect(page).not_to have_content template_override_warning end it 'user selects "bug" template, then updates description, then selects "feature-proposal" template, then applies template change' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb index 023d7530b4b..945baf47b7b 100644 --- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb +++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb @@ -117,6 +117,24 @@ describe Gitlab::Ci::Pipeline::Seed::Build do context 'when job is not a bridge' do it { is_expected.to be_a(::Ci::Build) } it { is_expected.to be_valid } + + context 'when job has environment name' do + let(:attributes) { { name: 'rspec', ref: 'master', environment: 'production' } } + + it 'returns a job with deployment' do + expect(subject.deployment).not_to be_nil + expect(subject.deployment.deployable).to eq(subject) + expect(subject.deployment.environment.name).to eq('production') + end + + context 'when the environment name is invalid' do + let(:attributes) { { name: 'rspec', ref: 'master', environment: '!!!' } } + + it 'returns a job without deployment' do + expect(subject.deployment).to be_nil + end + end + end end context 'when job is a bridge' do diff --git a/spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb new file mode 100644 index 00000000000..4e63f60ea6b --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Seed::Deployment do + let_it_be(:project) { create(:project) } + let(:job) { build(:ci_build, project: project) } + let(:seed) { described_class.new(job) } + let(:attributes) { {} } + + before do + job.assign_attributes(**attributes) + end + + describe '#to_resource' do + subject { seed.to_resource } + + context 'when job has environment attribute' do + let(:attributes) do + { + environment: 'production', + options: { environment: { name: 'production' } } + } + end + + it 'returns a deployment object with environment' do + expect(subject).to be_a(Deployment) + expect(subject.iid).to be_present + expect(subject.environment.name).to eq('production') + expect(subject.cluster).to be_nil + end + + context 'when environment has deployment platform' do + let!(:cluster) { create(:cluster, :provided_by_gcp, projects: [project]) } + + it 'returns a deployment with cluster id' do + expect(subject.cluster).to eq(cluster) + end + end + + context 'when environment has an invalid URL' do + let(:attributes) do + { + environment: '!!!', + options: { environment: { name: '!!!' } } + } + end + + it 'returns nothing' do + is_expected.to be_nil + end + end + + context 'when job has already deployment' do + let(:job) { build(:ci_build, :with_deployment, project: project, environment: 'production') } + + it 'returns the persisted deployment' do + is_expected.to eq(job.deployment) + end + end + end + + context 'when job has environment attribute with stop action' do + let(:attributes) do + { + environment: 'production', + options: { environment: { name: 'production', action: 'stop' } } + } + end + + it 'returns nothing' do + is_expected.to be_nil + end + end + end +end diff --git a/spec/lib/gitlab/ci/pipeline/seed/environment_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/environment_spec.rb new file mode 100644 index 00000000000..92ad20f30a0 --- /dev/null +++ b/spec/lib/gitlab/ci/pipeline/seed/environment_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Ci::Pipeline::Seed::Environment do + let_it_be(:project) { create(:project) } + let(:job) { build(:ci_build, project: project) } + let(:seed) { described_class.new(job) } + let(:attributes) { {} } + + before do + job.assign_attributes(**attributes) + end + + describe '#to_resource' do + subject { seed.to_resource } + + context 'when job has environment attribute' do + let(:attributes) do + { + environment: 'production', + options: { environment: { name: 'production' } } + } + end + + it 'returns an environment object' do + expect(subject).to be_a(Environment) + expect(subject).not_to be_persisted + expect(subject.project).to eq(project) + expect(subject.name).to eq('production') + end + + context 'when environment has already existed' do + let!(:environment) { create(:environment, project: project, name: 'production') } + + it 'returns the existing environment object' do + expect(subject).to be_persisted + expect(subject).to eq(environment) + end + end + end + end +end diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb index bf09bf7f4b5..de90e4c2fba 100644 --- a/spec/models/ci/build_spec.rb +++ b/spec/models/ci/build_spec.rb @@ -957,7 +957,7 @@ describe Ci::Build do end describe 'state transition as a deployable' do - let!(:build) { create(:ci_build, :start_review_app) } + let!(:build) { create(:ci_build, :with_deployment, :start_review_app) } let(:deployment) { build.deployment } let(:environment) { deployment.environment } @@ -1043,20 +1043,6 @@ describe Ci::Build do end describe 'deployment' do - describe '#has_deployment?' do - subject { build.has_deployment? } - - context 'when build has a deployment' do - let!(:deployment) { create(:deployment, deployable: build) } - - it { is_expected.to be_truthy } - end - - context 'when build does not have a deployment' do - it { is_expected.to be_falsy } - end - end - describe '#outdated_deployment?' do subject { build.outdated_deployment? } @@ -1955,7 +1941,7 @@ describe Ci::Build do end context 'when build has a start environment' do - let(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline) } + let(:build) { create(:ci_build, :with_deployment, :deploy_to_production, pipeline: pipeline) } it 'does not expand environment name' do expect(build).not_to receive(:expanded_environment_name) diff --git a/spec/models/concerns/deployable_spec.rb b/spec/models/concerns/deployable_spec.rb deleted file mode 100644 index ad2c0770a2c..00000000000 --- a/spec/models/concerns/deployable_spec.rb +++ /dev/null @@ -1,82 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -describe Deployable do - describe '#create_deployment' do - let(:deployment) { job.deployment } - let(:environment) { deployment&.environment } - - context 'when the deployable object will deploy to production' do - let!(:job) { create(:ci_build, :start_review_app) } - - it 'creates a deployment and environment record' do - expect(deployment.project).to eq(job.project) - expect(deployment.ref).to eq(job.ref) - expect(deployment.tag).to eq(job.tag) - expect(deployment.sha).to eq(job.sha) - expect(deployment.user).to eq(job.user) - expect(deployment.deployable).to eq(job) - expect(deployment.on_stop).to eq('stop_review_app') - expect(environment.name).to eq('review/master') - end - end - - context 'when the deployable object will deploy to a cluster' do - let(:project) { create(:project) } - let!(:cluster) { create(:cluster, :provided_by_user, projects: [project]) } - let!(:job) { create(:ci_build, :start_review_app, project: project) } - - it 'creates a deployment with cluster association' do - expect(deployment.cluster).to eq(cluster) - end - end - - context 'when the deployable object will stop an environment' do - let!(:job) { create(:ci_build, :stop_review_app) } - - it 'does not create a deployment record' do - expect(deployment).to be_nil - end - end - - context 'when the deployable object has already had a deployment' do - let!(:job) { create(:ci_build, :start_review_app, deployment: race_deployment) } - let!(:race_deployment) { create(:deployment, :success) } - - it 'does not create a new deployment' do - expect(deployment).to eq(race_deployment) - end - end - - context 'when the deployable object will not deploy' do - let!(:job) { create(:ci_build) } - - it 'does not create a deployment and environment record' do - expect(deployment).to be_nil - expect(environment).to be_nil - end - end - - context 'when environment scope contains invalid character' do - let(:job) do - create( - :ci_build, - name: 'job:deploy-to-test-site', - environment: '$CI_JOB_NAME', - options: { - environment: { - name: '$CI_JOB_NAME', - url: 'http://staging.example.com/$CI_JOB_NAME', - on_stop: 'stop_review_app' - } - }) - end - - it 'does not create a deployment and environment record' do - expect(deployment).to be_nil - expect(environment).to be_nil - end - end - end -end diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb index e2836420df9..01d331f518b 100644 --- a/spec/models/environment_status_spec.rb +++ b/spec/models/environment_status_spec.rb @@ -95,7 +95,7 @@ describe EnvironmentStatus do describe '.build_environments_status' do subject { described_class.send(:build_environments_status, merge_request, user, pipeline) } - let!(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline) } + let!(:build) { create(:ci_build, :with_deployment, :deploy_to_production, pipeline: pipeline) } let(:environment) { build.deployment.environment } let(:user) { project.owner } diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index b142942a0a7..b146c767f82 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -2366,7 +2366,7 @@ describe MergeRequest do merge_requests_as_head_pipeline: [merge_request]) end - let!(:job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) } + let!(:job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) } it 'returns environments' do is_expected.to eq(pipeline.environments) diff --git a/spec/services/ci/retry_build_service_spec.rb b/spec/services/ci/retry_build_service_spec.rb index 281c7438eee..b1368f7776b 100644 --- a/spec/services/ci/retry_build_service_spec.rb +++ b/spec/services/ci/retry_build_service_spec.rb @@ -204,6 +204,16 @@ describe Ci::RetryBuildService do expect(build).to be_retried expect(build.reload).to be_retried end + + context 'when build with deployment is retried' do + let!(:build) do + create(:ci_build, :with_deployment, :deploy_to_production, pipeline: pipeline, stage_id: stage.id) + end + + it 'creates a new deployment' do + expect { new_build }.to change { Deployment.count }.by(1) + end + end end context 'when user does not have ability to execute build' do diff --git a/spec/services/ci/stop_environments_service_spec.rb b/spec/services/ci/stop_environments_service_spec.rb index 890fa5bc009..ed92625a2cc 100644 --- a/spec/services/ci/stop_environments_service_spec.rb +++ b/spec/services/ci/stop_environments_service_spec.rb @@ -121,8 +121,8 @@ describe Ci::StopEnvironmentsService do merge_requests_as_head_pipeline: [merge_request]) end - let!(:review_job) { create(:ci_build, :start_review_app, pipeline: pipeline, project: project) } - let!(:stop_review_job) { create(:ci_build, :stop_review_app, :manual, pipeline: pipeline, project: project) } + let!(:review_job) { create(:ci_build, :with_deployment, :start_review_app, pipeline: pipeline, project: project) } + let!(:stop_review_job) { create(:ci_build, :with_deployment, :stop_review_app, :manual, pipeline: pipeline, project: project) } before do review_job.deployment.success! diff --git a/spec/services/update_deployment_service_spec.rb b/spec/services/update_deployment_service_spec.rb index 7dc52f6816a..343dab8a974 100644 --- a/spec/services/update_deployment_service_spec.rb +++ b/spec/services/update_deployment_service_spec.rb @@ -9,6 +9,7 @@ describe UpdateDeploymentService do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', tag: false, environment: 'production', @@ -114,6 +115,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses $CI_COMMIT_REF_NAME' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', environment: 'production', project: project, @@ -126,6 +128,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses $CI_ENVIRONMENT_SLUG' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', environment: 'prod-slug', project: project, @@ -138,6 +141,7 @@ describe UpdateDeploymentService do context 'when yaml environment uses yaml_variables containing symbol keys' do let(:job) do create(:ci_build, + :with_deployment, yaml_variables: [{ key: :APP_HOST, value: 'host' }], environment: 'production', project: project, @@ -148,7 +152,7 @@ describe UpdateDeploymentService do end context 'when yaml environment does not have url' do - let(:job) { create(:ci_build, environment: 'staging', project: project) } + let(:job) { create(:ci_build, :with_deployment, environment: 'staging', project: project) } it 'returns the external_url from persisted environment' do is_expected.to be_nil @@ -174,6 +178,7 @@ describe UpdateDeploymentService do context 'when job deploys to staging' do let(:job) do create(:ci_build, + :with_deployment, ref: 'master', tag: false, environment: 'staging', diff --git a/spec/support/helpers/cycle_analytics_helpers.rb b/spec/support/helpers/cycle_analytics_helpers.rb index b2817f9c14a..a604359942f 100644 --- a/spec/support/helpers/cycle_analytics_helpers.rb +++ b/spec/support/helpers/cycle_analytics_helpers.rb @@ -113,6 +113,7 @@ module CycleAnalyticsHelpers def new_dummy_job(user, project, environment) create(:ci_build, + :with_deployment, project: project, user: user, environment: environment, |