Merge branch 'fix-cross-db-transaction-on-job-state-machine' into 'master'

Fix Cross-DB transaction on Deployment Status Sync

See merge request gitlab-org/gitlab!71450

app/models/ci/build.rb

@@ -309,9 +309,11 @@ module Ci
       end
 
       after_transition pending: :running do |build|
+        unless build.update_deployment_after_transaction_commit?
           Gitlab::Database.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338867') do
             build.deployment&.run
           end
+        end
 
         build.run_after_commit do
           build.pipeline.persistent_ref.create
@@ -333,9 +335,11 @@ module Ci
       end
 
       after_transition any => [:success] do |build|
+        unless build.update_deployment_after_transaction_commit?
           Gitlab::Database.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338867') do
             build.deployment&.succeed
           end
+        end
 
         build.run_after_commit do
           BuildSuccessWorker.perform_async(id)
@@ -347,6 +351,7 @@ module Ci
         next unless build.project
         next unless build.deployment
 
+        unless build.update_deployment_after_transaction_commit?
           begin
             Gitlab::Database.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338867') do
               build.deployment.drop!
@@ -354,6 +359,7 @@ module Ci
           rescue StandardError => e
             Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e, build_id: build.id)
           end
+        end
 
         true
       end
@@ -371,6 +377,7 @@ module Ci
       end
 
       after_transition any => [:skipped, :canceled] do |build, transition|
+        unless build.update_deployment_after_transaction_commit?
           Gitlab::Database.allow_cross_database_modification_within_transaction(url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/338867') do
             if transition.to_name == :skipped
               build.deployment&.skip
@@ -381,6 +388,20 @@ module Ci
         end
       end
 
+      # Synchronize Deployment Status
+      # Please note that the data integirty is not assured because we can't use
+      # a database transaction due to DB decomposition.
+      after_transition do |build, transition|
+        next if transition.loopback?
+        next unless build.project
+        next unless build.update_deployment_after_transaction_commit?
+
+        build.run_after_commit do
+          build.deployment&.sync_status_with(build)
+        end
+      end
+    end
+
     def self.build_matchers(project)
       unique_params = [
         :protected,
@@ -1095,6 +1116,12 @@ module Ci
       runner&.instance_type?
     end
 
+    def update_deployment_after_transaction_commit?
+      strong_memoize(:update_deployment_after_transaction_commit) do
+        Feature.enabled?(:update_deployment_after_transaction_commit, project, default_enabled: :yaml)
+      end
+    end
+
     protected
 
     def run_status_commit_hooks!

app/models/deployment.rb

@@ -10,6 +10,9 @@ class Deployment < ApplicationRecord
   include FastDestroyAll
   include IgnorableColumns
 
+  StatusUpdateError = Class.new(StandardError)
+  StatusSyncError = Class.new(StandardError)
+
   belongs_to :project, required: true
   belongs_to :environment, required: true
   belongs_to :cluster, class_name: 'Clusters::Cluster', optional: true
@@ -312,20 +315,23 @@ class Deployment < ApplicationRecord
   # Changes the status of a deployment and triggers the corresponding state
   # machine events.
   def update_status(status)
-    case status
-    when 'running'
-      run
-    when 'success'
-      succeed
-    when 'failed'
-      drop
-    when 'canceled'
-      cancel
-    when 'skipped'
-      skip
-    else
-      raise ArgumentError, "The status #{status.inspect} is invalid"
+    update_status!(status)
+  rescue StandardError => e
+    Gitlab::ErrorTracking.track_exception(
+      StatusUpdateError.new(e.message), deployment_id: self.id)
+
+    false
   end
+
+  def sync_status_with(build)
+    return false unless ::Deployment.statuses.include?(build.status)
+
+    update_status!(build.status)
+  rescue StandardError => e
+    Gitlab::ErrorTracking.track_exception(
+      StatusSyncError.new(e.message), deployment_id: self.id, build_id: build.id)
+
+    false
   end
 
   def valid_sha
@@ -353,6 +359,23 @@ class Deployment < ApplicationRecord
 
   private
 
+  def update_status!(status)
+    case status
+    when 'running'
+      run!
+    when 'success'
+      succeed!
+    when 'failed'
+      drop!
+    when 'canceled'
+      cancel!
+    when 'skipped'
+      skip!
+    else
+      raise ArgumentError, "The status #{status.inspect} is invalid"
+    end
+  end
+
   def legacy_finished_at
     self.created_at if success? && !read_attribute(:finished_at)
   end

config/feature_flags/development/update_deployment_after_transaction_commit.yml

+---
+name: update_deployment_after_transaction_commit
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71450
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/342021
+milestone: '14.4'
+type: development
+group: group::release
+default_enabled: false

spec/models/ci/build_spec.rb

@@ -1290,7 +1290,7 @@ RSpec.describe Ci::Build do
     end
   end
 
-  describe 'state transition as a deployable' do
+  shared_examples_for 'state transition as a deployable' do
     subject { build.send(event) }
 
     let!(:build) { create(:ci_build, :with_deployment, :start_review_app, project: project, pipeline: pipeline) }
@@ -1399,6 +1399,36 @@ RSpec.describe Ci::Build do
     end
   end
 
+  it_behaves_like 'state transition as a deployable' do
+    context 'when transits to running' do
+      let(:event) { :run! }
+
+      context 'when deployment is already running state' do
+        before do
+          build.deployment.success!
+        end
+
+        it 'does not change deployment status and tracks an error' do
+          expect(Gitlab::ErrorTracking)
+            .to receive(:track_exception).with(
+              instance_of(Deployment::StatusSyncError), deployment_id: deployment.id, build_id: build.id)
+
+          with_cross_database_modification_prevented do
+            expect { subject }.not_to change { deployment.reload.status }
+          end
+        end
+      end
+    end
+  end
+
+  context 'when update_deployment_after_transaction_commit feature flag is disabled' do
+    before do
+      stub_feature_flags(update_deployment_after_transaction_commit: false)
+    end
+
+    it_behaves_like 'state transition as a deployable'
+  end
+
   describe '#on_stop' do
     subject { build.on_stop }
 
@@ -3948,7 +3978,7 @@ RSpec.describe Ci::Build do
       end
 
       it 'can drop the build' do
-        expect(Gitlab::ErrorTracking).to receive(:track_and_raise_for_dev_exception)
+        expect(Gitlab::ErrorTracking).to receive(:track_exception)
 
         expect { build.drop! }.not_to raise_error
 

spec/models/deployment_spec.rb

@@ -765,7 +765,7 @@ RSpec.describe Deployment do
       expect(Deployments::LinkMergeRequestWorker).to receive(:perform_async)
       expect(Deployments::HooksWorker).to receive(:perform_async)
 
-      deploy.update_status('success')
+      expect(deploy.update_status('success')).to eq(true)
     end
 
     it 'updates finished_at when transitioning to a finished status' do
@@ -775,6 +775,139 @@ RSpec.describe Deployment do
         expect(deploy.read_attribute(:finished_at)).to eq(Time.current)
       end
     end
+
+    it 'tracks an exception if an invalid status transition is detected' do
+      expect(Gitlab::ErrorTracking)
+        .to receive(:track_exception)
+        .with(instance_of(described_class::StatusUpdateError), deployment_id: deploy.id)
+
+      expect(deploy.update_status('running')).to eq(false)
+    end
+
+    it 'tracks an exception if an invalid argument' do
+      expect(Gitlab::ErrorTracking)
+        .to receive(:track_exception)
+        .with(instance_of(described_class::StatusUpdateError), deployment_id: deploy.id)
+
+      expect(deploy.update_status('created')).to eq(false)
+    end
+  end
+
+  describe '#sync_status_with' do
+    subject { deployment.sync_status_with(ci_build) }
+
+    let_it_be(:project) { create(:project, :repository) }
+
+    let(:deployment) { create(:deployment, project: project, status: deployment_status) }
+    let(:ci_build) { create(:ci_build, project: project, status: build_status) }
+
+    shared_examples_for 'synchronizing deployment' do
+      it 'changes deployment status' do
+        expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
+
+        is_expected.to eq(true)
+
+        expect(deployment.status).to eq(build_status.to_s)
+        expect(deployment.errors).to be_empty
+      end
+    end
+
+    shared_examples_for 'gracefully handling error' do
+      it 'tracks an exception' do
+        expect(Gitlab::ErrorTracking).to receive(:track_exception).with(
+          instance_of(described_class::StatusSyncError),
+          deployment_id: deployment.id,
+          build_id: ci_build.id)
+
+        is_expected.to eq(false)
+
+        expect(deployment.status).to eq(deployment_status.to_s)
+        expect(deployment.errors.full_messages).to include(error_message)
+      end
+    end
+
+    shared_examples_for 'ignoring build' do
+      it 'does not change deployment status' do
+        expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
+
+        is_expected.to eq(false)
+
+        expect(deployment.status).to eq(deployment_status.to_s)
+        expect(deployment.errors).to be_empty
+      end
+    end
+
+    context 'with created deployment' do
+      let(:deployment_status) { :created }
+
+      context 'with running build' do
+        let(:build_status) { :running }
+
+        it_behaves_like 'synchronizing deployment'
+      end
+
+      context 'with finished build' do
+        let(:build_status) { :success }
+
+        it_behaves_like 'synchronizing deployment'
+      end
+
+      context 'with unrelated build' do
+        let(:build_status) { :waiting_for_resource }
+
+        it_behaves_like 'ignoring build'
+      end
+    end
+
+    context 'with running deployment' do
+      let(:deployment_status) { :running }
+
+      context 'with running build' do
+        let(:build_status) { :running }
+
+        it_behaves_like 'gracefully handling error' do
+          let(:error_message) { %Q{Status cannot transition via \"run\"} }
+        end
+      end
+
+      context 'with finished build' do
+        let(:build_status) { :success }
+
+        it_behaves_like 'synchronizing deployment'
+      end
+
+      context 'with unrelated build' do
+        let(:build_status) { :waiting_for_resource }
+
+        it_behaves_like 'ignoring build'
+      end
+    end
+
+    context 'with finished deployment' do
+      let(:deployment_status) { :success }
+
+      context 'with running build' do
+        let(:build_status) { :running }
+
+        it_behaves_like 'gracefully handling error' do
+          let(:error_message) { %Q{Status cannot transition via \"run\"} }
+        end
+      end
+
+      context 'with finished build' do
+        let(:build_status) { :success }
+
+        it_behaves_like 'gracefully handling error' do
+          let(:error_message) { %Q{Status cannot transition via \"succeed\"} }
+        end
+      end
+
+      context 'with unrelated build' do
+        let(:build_status) { :waiting_for_resource }
+
+        it_behaves_like 'ignoring build'
+      end
+    end
   end
 
   describe '#valid_sha' do

spec/requests/api/deployments_spec.rb

@@ -376,6 +376,16 @@ RSpec.describe API::Deployments do
         expect(json_response['status']).to eq('success')
       end
 
+      it 'returns an error when an invalid status transition is detected' do
+        put(
+          api("/projects/#{project.id}/deployments/#{deploy.id}", user),
+          params: { status: 'running' }
+        )
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']['status']).to include(%Q{cannot transition via \"run\"})
+      end
+
       it 'links merge requests when the deployment status changes to success', :sidekiq_inline do
         mr = create(
           :merge_request,

spec/services/deployments/update_service_spec.rb

@@ -34,9 +34,11 @@ RSpec.describe Deployments::UpdateService do
       expect(deploy).to be_canceled
     end
 
-    it 'raises ArgumentError if the status is invalid' do
-      expect { described_class.new(deploy, status: 'kittens').execute }
-        .to raise_error(ArgumentError)
+    it 'does not change the state if the status is invalid' do
+      expect(described_class.new(deploy, status: 'kittens').execute)
+        .to be_falsy
+
+      expect(deploy).to be_created
     end
 
     it 'links merge requests when changing the status to success', :sidekiq_inline do