Always use state machine for deployments

This refactors Deployment, Deployments::CreateService, and
Deployments::UpdateService so that the appopriate state machine events
are triggered when creating or updating a deployment. Without this we
would not trigger any events when creating deployments using the API,
such as the event used for setting `finished_at`.

As part of this change, Deployments::CreateService no longer calls
Deployments::AfterCreateService directly; instead it uses Sidekiq which
is triggered using a state machine event.

This ensures that deployments created using the API and CI both trigger
the same workers, hooks, etc.

To make this work some specs had to be adjusted, which included
adjusting some Gitaly request count limits that were racy.

This fixes https://gitlab.com/gitlab-org/gitlab/issues/36648 and fixes
https://gitlab.com/gitlab-org/gitlab/issues/35763.
Co-Authored-By: Alessio Caiazza <acaiazza@gitlab.com>

app/models/deployment.rb

@@ -217,6 +217,23 @@ class Deployment < ApplicationRecord
     SQL
   end
 
+  # Changes the status of a deployment and triggers the correspinding state
+  # machine events.
+  def update_status(status)
+    case status
+    when 'running'
+      run
+    when 'success'
+      succeed
+    when 'failed'
+      drop
+    when 'canceled'
+      cancel
+    else
+      raise ArgumentError, "The status #{status.inspect} is invalid"
+    end
+  end
+
   private
 
   def ref_path

app/services/deployments/create_service.rb

@@ -11,15 +11,17 @@ module Deployments
     end
 
     def execute
-      create_deployment.tap do |deployment|
-        AfterCreateService.new(deployment).execute if deployment.persisted?
+      environment.deployments.build(deployment_attributes).tap do |deployment|
+        # Deployment#change_status already saves the model, so we only need to
+        # call #save ourselves if no status is provided.
+        if (status = params[:status])
+          deployment.update_status(status)
+        else
+          deployment.save
+        end
       end
     end
 
-    def create_deployment
-      environment.deployments.create(deployment_attributes)
-    end
-
     def deployment_attributes
       # We use explicit parameters here so we never by accident allow parameters
       # to be set that one should not be able to set (e.g. the row ID).
@@ -31,8 +33,7 @@ module Deployments
         tag: params[:tag],
         sha: params[:sha],
         user: current_user,
-        on_stop: params[:on_stop],
-        status: params[:status]
+        on_stop: params[:on_stop]
       }
     end
   end

app/services/deployments/update_service.rb

@@ -10,22 +10,7 @@ module Deployments
     end
 
     def execute
-      # A regular update() does not trigger the state machine transitions, which
-      # we need to ensure merge requests are linked when changing the status to
-      # success. To work around this we use this case statment, using the right
-      # event methods to trigger the transition hooks.
-      case params[:status]
-      when 'running'
-        deployment.run
-      when 'success'
-        deployment.succeed
-      when 'failed'
-        deployment.drop
-      when 'canceled'
-        deployment.cancel
-      else
-        false
-      end
+      deployment.update_status(params[:status])
     end
   end
 end

changelogs/unreleased/deployment-finished-at.yml

+---
+title: Refactor the Deployment model so state machine events are used by both CI and the API
+merge_request: 20474
+author:
+type: fixed

spec/controllers/projects/jobs_controller_spec.rb

@@ -5,7 +5,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
   include ApiHelpers
   include HttpIOHelpers
 
-  let(:project) { create(:project, :public) }
+  let(:project) { create(:project, :public, :repository) }
   let(:pipeline) { create(:ci_pipeline, project: project) }
   let(:user) { create(:user) }
 
@@ -511,7 +511,7 @@ describe Projects::JobsController, :clean_gitlab_redis_shared_state do
 
     def get_show_json
       expect { get_show(id: job.id, format: :json) }
-        .to change { Gitlab::GitalyClient.get_request_count }.by(1) # ListCommitsByOid
+        .to change { Gitlab::GitalyClient.get_request_count }.by_at_most(2)
     end
 
     def get_show(**extra_params)

spec/models/deployment_spec.rb

@@ -474,4 +474,29 @@ describe Deployment do
       end
     end
   end
+
+  context '#update_status' do
+    let(:deploy) { create(:deployment, status: :running) }
+
+    it 'changes the status' do
+      deploy.update_status('success')
+
+      expect(deploy).to be_success
+    end
+
+    it 'schedules SuccessWorker and FinishedWorker when finishing a deploy' do
+      expect(Deployments::SuccessWorker).to receive(:perform_async)
+      expect(Deployments::FinishedWorker).to receive(:perform_async)
+
+      deploy.update_status('success')
+    end
+
+    it 'updates finished_at when transitioning to a finished status' do
+      Timecop.freeze do
+        deploy.update_status('success')
+
+        expect(deploy.read_attribute(:finished_at)).to eq(Time.now)
+      end
+    end
+  end
 end

spec/requests/api/deployments_spec.rb

@@ -147,7 +147,7 @@ describe API::Deployments do
         expect(response).to have_gitlab_http_status(500)
       end
 
-      it 'links any merged merge requests to the deployment' do
+      it 'links any merged merge requests to the deployment', :sidekiq_inline do
         mr = create(
           :merge_request,
           :merged,
@@ -199,7 +199,7 @@ describe API::Deployments do
         expect(json_response['ref']).to eq('master')
       end
 
-      it 'links any merged merge requests to the deployment' do
+      it 'links any merged merge requests to the deployment', :sidekiq_inline do
         mr = create(
           :merge_request,
           :merged,

spec/services/deployments/create_service_spec.rb

@@ -3,67 +3,54 @@
 require 'spec_helper'
 
 describe Deployments::CreateService do
-  let(:environment) do
-    double(
-      :environment,
-      deployment_platform: double(:platform, cluster_id: 1),
-      project_id: 2,
-      id: 3
-    )
-  end
-
-  let(:user) { double(:user) }
+  let(:user) { create(:user) }
 
   describe '#execute' do
-    let(:service) { described_class.new(environment, user, {}) }
-
-    it 'does not run the AfterCreateService service if the deployment is not persisted' do
-      deploy = double(:deployment, persisted?: false)
+    let(:project) { create(:project, :repository) }
+    let(:environment) { create(:environment, project: project) }
 
-      expect(service)
-        .to receive(:create_deployment)
-        .and_return(deploy)
+    it 'creates a deployment' do
+      service = described_class.new(
+        environment,
+        user,
+        sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0',
+        ref: 'master',
+        tag: false,
+        status: 'success'
+      )
 
-      expect(Deployments::AfterCreateService)
-        .not_to receive(:new)
+      expect(Deployments::SuccessWorker).to receive(:perform_async)
+      expect(Deployments::FinishedWorker).to receive(:perform_async)
 
-      expect(service.execute).to eq(deploy)
+      expect(service.execute).to be_persisted
     end
 
-    it 'runs the AfterCreateService service if the deployment is persisted' do
-      deploy = double(:deployment, persisted?: true)
-      after_service = double(:after_create_service)
-
-      expect(service)
-        .to receive(:create_deployment)
-        .and_return(deploy)
-
-      expect(Deployments::AfterCreateService)
-        .to receive(:new)
-        .with(deploy)
-        .and_return(after_service)
+    it 'does not change the status if no status is given' do
+      service = described_class.new(
+        environment,
+        user,
+        sha: 'b83d6e391c22777fca1ed3012fce84f633d7fed0',
+        ref: 'master',
+        tag: false
+      )
 
-      expect(after_service)
-        .to receive(:execute)
+      expect(Deployments::SuccessWorker).not_to receive(:perform_async)
+      expect(Deployments::FinishedWorker).not_to receive(:perform_async)
 
-      expect(service.execute).to eq(deploy)
+      expect(service.execute).to be_persisted
     end
   end
 
-  describe '#create_deployment' do
-    it 'creates a deployment' do
-      environment = build(:environment)
-      service = described_class.new(environment, user, {})
-
-      expect(environment.deployments)
-        .to receive(:create)
-        .with(an_instance_of(Hash))
-
-      service.create_deployment
+  describe '#deployment_attributes' do
+    let(:environment) do
+      double(
+        :environment,
+        deployment_platform: double(:platform, cluster_id: 1),
+        project_id: 2,
+        id: 3
+      )
     end
-  end
 
-  describe '#deployment_attributes' do
     it 'only includes attributes that we want to persist' do
       service = described_class.new(
         environment,
@@ -72,8 +59,7 @@ describe Deployments::CreateService do
         tag: true,
         sha: '123',
         foo: 'bar',
-        on_stop: 'stop',
-        status: 'running'
+        on_stop: 'stop'
       )
 
       expect(service.deployment_attributes).to eq(
@@ -84,8 +70,7 @@ describe Deployments::CreateService do
         tag: true,
         sha: '123',
         user: user,
-        on_stop: 'stop',
-        status: 'running'
+        on_stop: 'stop'
       )
     end
   end

spec/services/deployments/update_service_spec.rb

@@ -34,9 +34,9 @@ describe Deployments::UpdateService do
       expect(deploy).to be_canceled
     end
 
-    it 'returns false when the status is not supported' do
-      expect(described_class.new(deploy, status: 'kittens').execute)
-        .to be_falsey
+    it 'raises ArgumentError if the status is invalid' do
+      expect { described_class.new(deploy, status: 'kittens').execute }
+        .to raise_error(ArgumentError)
     end
 
     it 'links merge requests when changing the status to success', :sidekiq_inline do