Merge branch 'separate-transaction-for-auto-job-retry' into 'master'

Separate transaction for Deployment in Auto Job Retry

See merge request gitlab-org/gitlab!76193

app/services/ci/retry_build_service.rb

@@ -42,16 +42,19 @@ module Ci
       check_access!(build)
 
       new_build = clone_build(build)
+
+      if create_deployment_in_separate_transaction?
+        new_build.run_after_commit do |new_build|
+          ::Deployments::CreateForBuildService.new.execute(new_build)
+        end
+      end
+
       ::Ci::Pipelines::AddJobService.new(build.pipeline).execute!(new_build) do |job|
         BulkInsertableAssociations.with_bulk_insert do
           job.save!
         end
       end
 
-      if create_deployment_in_separate_transaction?
-        clone_deployment!(new_build, build)
-      end
-
       build.reset # refresh the data to get new values of `retried` and `processed`.
 
       new_build
@@ -95,20 +98,6 @@ module Ci
         .deployment_attributes_for(new_build, old_build.persisted_environment)
     end
 
-    def clone_deployment!(new_build, old_build)
-      return unless old_build.deployment.present?
-
-      # We should clone the previous deployment attributes instead of initializing
-      # new object with `Seed::Deployment`.
-      # See https://gitlab.com/gitlab-org/gitlab/-/issues/347206
-      deployment = ::Gitlab::Ci::Pipeline::Seed::Deployment
-        .new(new_build, old_build.persisted_environment).to_resource
-
-      return unless deployment
-
-      new_build.create_deployment!(deployment.attributes)
-    end
-
     def create_deployment_in_separate_transaction?
       strong_memoize(:create_deployment_in_separate_transaction) do
         ::Feature.enabled?(:create_deployment_in_separate_transaction, project, default_enabled: :yaml)

app/services/deployments/create_for_build_service.rb

+# frozen_string_literal: true
+
+module Deployments
+  # This class creates a deployment record for a build (a pipeline job).
+  class CreateForBuildService
+    DeploymentCreationError = Class.new(StandardError)
+
+    def execute(build)
+      return unless build.instance_of?(::Ci::Build) && build.persisted_environment.present?
+
+      # TODO: Move all buisness logic in `Seed::Deployment` to this class after
+      # `create_deployment_in_separate_transaction` feature flag has been removed.
+      # See https://gitlab.com/gitlab-org/gitlab/-/issues/348778
+      deployment = ::Gitlab::Ci::Pipeline::Seed::Deployment
+        .new(build, build.persisted_environment).to_resource
+
+      return unless deployment
+
+      build.create_deployment!(deployment.attributes)
+    rescue ActiveRecord::RecordInvalid => e
+      Gitlab::ErrorTracking.track_and_raise_for_dev_exception(
+        DeploymentCreationError.new(e.message), build_id: build.id)
+    end
+  end
+end

lib/gitlab/ci/pipeline/chain/create_deployments.rb

@@ -5,8 +5,6 @@ module Gitlab
     module Pipeline
       module Chain
         class CreateDeployments < Chain::Base
-          DeploymentCreationError = Class.new(StandardError)
-
           def perform!
             return unless pipeline.create_deployment_in_separate_transaction?
 
@@ -24,18 +22,7 @@ module Gitlab
           end
 
           def create_deployment(build)
-            return unless build.instance_of?(::Ci::Build) && build.persisted_environment.present?
-
-            deployment = ::Gitlab::Ci::Pipeline::Seed::Deployment
-              .new(build, build.persisted_environment).to_resource
-
-            return unless deployment
-
-            deployment.deployable = build
-            deployment.save!
-          rescue ActiveRecord::RecordInvalid => e
-            Gitlab::ErrorTracking.track_and_raise_for_dev_exception(
-              DeploymentCreationError.new(e.message), build_id: build.id)
+            ::Deployments::CreateForBuildService.new.execute(build)
           end
         end
       end

spec/lib/gitlab/ci/pipeline/chain/create_deployments_spec.rb

@@ -38,20 +38,6 @@ RSpec.describe Gitlab::Ci::Pipeline::Chain::CreateDeployments do
         expect(job.deployment.environment).to eq(job.persisted_environment)
       end
 
-      context 'when creation failure occures' do
-        before do
-          allow_next_instance_of(Deployment) do |deployment|
-            allow(deployment).to receive(:save!) { raise ActiveRecord::RecordInvalid }
-          end
-        end
-
-        it 'trackes the exception' do
-          expect { subject }.to raise_error(described_class::DeploymentCreationError)
-
-          expect(Deployment.count).to eq(0)
-        end
-      end
-
       context 'when the corresponding environment does not exist' do
         let!(:environment) { }
 

spec/models/ci/build_spec.rb

@@ -2064,6 +2064,31 @@ RSpec.describe Ci::Build do
   end
 
   describe 'build auto retry feature' do
+    context 'with deployment job' do
+      let(:build) do
+        create(:ci_build, :deploy_to_production, :with_deployment,
+               user: user, pipeline: pipeline, project: project)
+      end
+
+      before do
+        project.add_developer(user)
+        allow(build).to receive(:auto_retry_allowed?) { true }
+      end
+
+      it 'creates a deployment when a build is dropped' do
+        expect { build.drop!(:script_failure) }.to change { Deployment.count }.by(1)
+
+        retried_deployment = Deployment.last
+        expect(build.deployment.environment).to eq(retried_deployment.environment)
+        expect(build.deployment.ref).to eq(retried_deployment.ref)
+        expect(build.deployment.sha).to eq(retried_deployment.sha)
+        expect(build.deployment.tag).to eq(retried_deployment.tag)
+        expect(build.deployment.user).to eq(retried_deployment.user)
+        expect(build.deployment).to be_failed
+        expect(retried_deployment).to be_created
+      end
+    end
+
     describe '#retries_count' do
       subject { create(:ci_build, name: 'test', pipeline: pipeline) }
 

spec/services/deployments/create_for_build_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Deployments::CreateForBuildService do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+
+  let(:service) { described_class.new }
+
+  describe '#execute' do
+    subject { service.execute(build) }
+
+    context 'with a deployment job' do
+      let!(:build) { create(:ci_build, :start_review_app, project: project) }
+      let!(:environment) { create(:environment, project: project, name: build.expanded_environment_name) }
+
+      it 'creates a deployment record' do
+        expect { subject }.to change { Deployment.count }.by(1)
+
+        build.reset
+        expect(build.deployment.project).to eq(build.project)
+        expect(build.deployment.ref).to eq(build.ref)
+        expect(build.deployment.sha).to eq(build.sha)
+        expect(build.deployment.deployable).to eq(build)
+        expect(build.deployment.deployable_type).to eq('CommitStatus')
+        expect(build.deployment.environment).to eq(build.persisted_environment)
+      end
+
+      context 'when creation failure occures' do
+        before do
+          allow(build).to receive(:create_deployment!) { raise ActiveRecord::RecordInvalid }
+        end
+
+        it 'trackes the exception' do
+          expect { subject }.to raise_error(described_class::DeploymentCreationError)
+
+          expect(Deployment.count).to eq(0)
+        end
+      end
+
+      context 'when the corresponding environment does not exist' do
+        let!(:environment) { }
+
+        it 'does not create a deployment record' do
+          expect { subject }.not_to change { Deployment.count }
+
+          expect(build.deployment).to be_nil
+        end
+      end
+    end
+
+    context 'with a teardown job' do
+      let!(:build) { create(:ci_build, :stop_review_app, project: project) }
+      let!(:environment) { create(:environment, name: build.expanded_environment_name) }
+
+      it 'does not create a deployment record' do
+        expect { subject }.not_to change { Deployment.count }
+
+        expect(build.deployment).to be_nil
+      end
+    end
+
+    context 'with a normal job' do
+      let!(:build) { create(:ci_build, project: project) }
+
+      it 'does not create a deployment record' do
+        expect { subject }.not_to change { Deployment.count }
+
+        expect(build.deployment).to be_nil
+      end
+    end
+
+    context 'with a bridge' do
+      let!(:build) { create(:ci_bridge, project: project) }
+
+      it 'does not create a deployment record' do
+        expect { subject }.not_to change { Deployment.count }
+      end
+    end
+  end
+end