Assign deployments.iid before pipeline transaction

This commit reduces the lock contention of internal iid

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

app/models/concerns/deployable.rb

-# 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

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

changelogs/unreleased/deployment-iid-transaction-improvement.yml

+---
+title: Reduce lock contention of deployment creation by allocating IID outside
+  of the pipeline transaction
+merge_request: 17696
+author:
+type: performance

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)

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

lib/gitlab/ci/pipeline/seed/deployment.rb

+# 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

lib/gitlab/ci/pipeline/seed/environment.rb

+# 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

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

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

spec/lib/gitlab/ci/pipeline/seed/deployment_spec.rb

+# 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

spec/lib/gitlab/ci/pipeline/seed/environment_spec.rb

+# 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

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)

spec/models/concerns/deployable_spec.rb

-# 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

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 }

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)

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

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!

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',

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,