diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index fba14f0100cd1527525e753ce40cfa10ad19cfc0..3dacd6a6224b67248e60c29767a000b75b1162c3 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -12,7 +12,6 @@ class Build < CommitStatus
include Presentable
include Importable
include Gitlab::Utils::StrongMemoize
- include Deployable
include HasRef
BuildArchivedError = Class.new(StandardError)
@@ -417,10 +416,6 @@ def environment_action
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 957b72f3721eccbd97f5fb5fd011db3bdb0ba93a..0000000000000000000000000000000000000000
--- 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 338495ba030ee81825a38799530aaebfb2cdb051..7a5e33c61babca3e3a19eb6e3d9612859ec25bba 100644
--- a/app/services/ci/retry_build_service.rb
+++ b/app/services/ci/retry_build_service.rb
@@ -39,9 +39,18 @@ def reprocess!(build)
.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/changelogs/unreleased/deployment-iid-transaction-improvement.yml b/changelogs/unreleased/deployment-iid-transaction-improvement.yml
new file mode 100644
index 0000000000000000000000000000000000000000..aefc3349c350144d3a0f96938f6d9194cd95a0a8
--- /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/ee/spec/policies/ci/build_policy_spec.rb b/ee/spec/policies/ci/build_policy_spec.rb
index 2fc56e2e221dc70bf514198fdc63719d19b69f6d..f71d90d693a1677a651ff753d494d7f4c647f2e6 100644
--- a/ee/spec/policies/ci/build_policy_spec.rb
+++ b/ee/spec/policies/ci/build_policy_spec.rb
@@ -17,7 +17,7 @@
it_behaves_like 'protected environments access'
context 'when a pipeline has manual deployment job' do
- let!(:build) { create(:ee_ci_build, :manual, :deploy_to_production, pipeline: pipeline) }
+ let!(:build) { create(:ee_ci_build, :with_deployment, :manual, :deploy_to_production, pipeline: pipeline) }
before do
project.add_developer(user)
diff --git a/lib/gitlab/ci/pipeline/seed/build.rb b/lib/gitlab/ci/pipeline/seed/build.rb
index 1f6b38530690c7c1a6c963fca12b297ff8636210..fc9c540088b7adabbb9aa446b84571eb9671b97e 100644
--- a/lib/gitlab/ci/pipeline/seed/build.rb
+++ b/lib/gitlab/ci/pipeline/seed/build.rb
@@ -73,7 +73,9 @@ def to_resource
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 0000000000000000000000000000000000000000..238cd845a0e2900cf675b7ae0bc6daad9b13f972
--- /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 0000000000000000000000000000000000000000..813a9e9e399a6b0a492ffed337da24a5cc1f4c51
--- /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 c0f7948f96392a81a59f63eb73e83725dae373ad..0bd39d4cdcfe0a6bd8c98ed85c1c85d062912c75 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -211,6 +211,17 @@
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/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
index 023d7530b4bbe858484dfb3a12690568367fa2f5..945baf47b7bcc0079efa3c42ed9c652cab515229 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 @@
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 0000000000000000000000000000000000000000..4e63f60ea6bc1d6d0cf3e156b88d724f83c62be4
--- /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 0000000000000000000000000000000000000000..92ad20f30a05e74521957f6b46a0984671d8f56e
--- /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 bf09bf7f4b50a159f7bfa4a9c2f1572c291ff4f8..de90e4c2fbaef51023de5a4299579cc6bc60505f 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -957,7 +957,7 @@
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 @@
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 @@ def create_mr(build, pipeline, factory: :merge_request, created_at: Time.now)
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 ad2c0770a2cc19f33f46311acee671c156bcb57e..0000000000000000000000000000000000000000
--- 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 e2836420df9b7f4699803fb173d13b6e492bbd29..01d331f518bbfe0641c7393f0861cb614d0c6d74 100644
--- a/spec/models/environment_status_spec.rb
+++ b/spec/models/environment_status_spec.rb
@@ -95,7 +95,7 @@
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 b142942a0a75b90352960bfe6d856c67a4ede777..b146c767f824b7e3c1871e74263f54eb45f36635 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -2366,7 +2366,7 @@ def set_compare(merge_request)
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 281c7438eeead98fe534f2b3a0e7f2cdd39c7d37..b1368f7776ba968dd1384017591c42f9ffec5960 100644
--- a/spec/services/ci/retry_build_service_spec.rb
+++ b/spec/services/ci/retry_build_service_spec.rb
@@ -204,6 +204,16 @@
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 890fa5bc00943c111c87d26c2c876ab2290969fd..ed92625a2cce42a10c6634fcab108fa36c8f3f1d 100644
--- a/spec/services/ci/stop_environments_service_spec.rb
+++ b/spec/services/ci/stop_environments_service_spec.rb
@@ -121,8 +121,8 @@
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 7dc52f6816a8886d78565994b492fbbb7cae91f7..343dab8a9747b4b614f7b94fa54406c1c617a551 100644
--- a/spec/services/update_deployment_service_spec.rb
+++ b/spec/services/update_deployment_service_spec.rb
@@ -9,6 +9,7 @@
let(:job) do
create(:ci_build,
+ :with_deployment,
ref: 'master',
tag: false,
environment: 'production',
@@ -114,6 +115,7 @@
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 @@
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 @@
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 @@
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 @@
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 b2817f9c14a9a63ae89249d6b546b6612dd3047c..a604359942f46becca876bd80501fc74134fd018 100644
--- a/spec/support/helpers/cycle_analytics_helpers.rb
+++ b/spec/support/helpers/cycle_analytics_helpers.rb
@@ -113,6 +113,7 @@ def dummy_pipeline(project)
def new_dummy_job(user, project, environment)
create(:ci_build,
+ :with_deployment,
project: project,
user: user,
environment: environment,