Refactor finalize out of scale action

* also start separating total instance count from current scale target

Author: sethboyles

lib/cloud_controller/deployment_updater/actions/finalize.rb

@@ -15,6 +15,7 @@ def call
 
           update_non_web_processes
           restart_non_web_processes
+          
           deployment.update(
             state: DeploymentModel::DEPLOYED_STATE,
             status_value: DeploymentModel::FINALIZED_STATUS_VALUE,

lib/cloud_controller/deployment_updater/actions/scale.rb

@@ -1,18 +1,21 @@
 require 'cloud_controller/deployment_updater/actions/scale_down_canceled_processes'
 require 'cloud_controller/deployment_updater/actions/scale_down_old_process'
 require 'cloud_controller/deployment_updater/actions/finalize'
+require 'cloud_controller/diego/constants'
 
 module VCAP::CloudController
   module DeploymentUpdater
     module Actions
       class Scale
         HEALTHY_STATES = [VCAP::CloudController::Diego::LRP_RUNNING, VCAP::CloudController::Diego::LRP_STARTING].freeze
-        attr_reader :deployment, :logger, :app
+        attr_reader :deployment, :logger, :app, :target_total_instance_count, :interim_desired_instance_count
 
-        def initialize(deployment, logger)
+        def initialize(deployment, logger, target_total_instance_count, interim_desired_instance_count = nil)
           @deployment = deployment
           @logger = logger
           @app = deployment.app
+          @target_total_instance_count = target_total_instance_count
+          @interim_desired_instance_count = interim_desired_instance_count || target_total_instance_count
         end
 
         def call
@@ -31,21 +34,19 @@ def call
               status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
               status_reason: DeploymentModel::DEPLOYING_STATUS_REASON
             )
-
-            if deploying_web_process.instances >= deployment.original_web_process_instance_count
-              Finalize.new(deployment).call
-              return
-            end
-
+            
             ScaleDownCanceledProcesses.new(deployment).call
 
             scale_down_old_processes if can_downscale?
 
+            return true if deploying_web_process.instances >= interim_desired_instance_count
+
             if can_scale?
               deploying_web_process.update(instances: desired_new_instances)
               deployment.update(last_healthy_at: Time.now)
             end
           end
+          false
         end
 
         private
@@ -83,11 +84,11 @@ def can_downscale?
         end
 
         def desired_non_deploying_instances
-          [deployment.original_web_process_instance_count - routable_instances.count, 0].max
+          [target_total_instance_count - routable_instances.count, 0].max
         end
 
         def desired_new_instances
-          [routable_instances.count + deployment.max_in_flight, deployment.original_web_process_instance_count].min
+          [routable_instances.count + deployment.max_in_flight, interim_desired_instance_count].min
         end
 
         def oldest_web_process_with_instances

lib/cloud_controller/deployment_updater/updater.rb

@@ -1,6 +1,7 @@
 require 'cloud_controller/deployment_updater/actions/scale'
 require 'cloud_controller/deployment_updater/actions/cancel'
 require 'cloud_controller/deployment_updater/actions/canary'
+require 'cloud_controller/deployment_updater/actions/finalize'
 module VCAP::CloudController
   module DeploymentUpdater
     class Updater
@@ -13,7 +14,8 @@ def initialize(deployment, logger)
 
       def scale
         with_error_logging('error-scaling-deployment') do
-          Actions::Scale.new(deployment, logger).call
+          finished = Actions::Scale.new(deployment, logger, deployment.original_web_process_instance_count).call
+          Actions::Finalize.new(deployment).call if finished
           logger.info("ran-deployment-update-for-#{deployment.guid}")
         end
       end

spec/unit/lib/cloud_controller/deployment_updater/actions/scale_spec.rb

@@ -3,7 +3,8 @@
 
 module VCAP::CloudController
   RSpec.describe DeploymentUpdater::Actions::Scale do
-    subject(:scale_action) { DeploymentUpdater::Actions::Scale.new(deployment, logger) }
+    subject(:scale_action) { DeploymentUpdater::Actions::Scale.new(deployment, logger, target_total_instance_count) }
+    let(:target_total_instance_count) { 6 }
     let(:a_day_ago) { Time.now - 1.day }
     let(:an_hour_ago) { Time.now - 1.hour }
     let(:app) { AppModel.make(droplet: droplet, revisions_enabled: true) }
@@ -30,7 +31,6 @@ module VCAP::CloudController
     let(:revision) { RevisionModel.make(app: app, droplet: droplet, version: 300) }
     let!(:deploying_route_mapping) { RouteMappingModel.make(app: web_process.app, process_type: deploying_web_process.type) }
     let(:space) { web_process.space }
-    let(:original_web_process_instance_count) { 6 }
     let(:current_web_instances) { 6 }
     let(:current_deploying_instances) { 0 }
 
@@ -41,7 +41,6 @@ module VCAP::CloudController
         app: web_process.app,
         deploying_web_process: deploying_web_process,
         state: state,
-        original_web_process_instance_count: original_web_process_instance_count,
         max_in_flight: max_in_flight
       )
     end
@@ -74,7 +73,7 @@ module VCAP::CloudController
       let(:current_deploying_instances) { 1 }
 
       it 'scales the old web process down by one' do
-        expect(original_web_process_instance_count).to be > current_deploying_instances
+        expect(target_total_instance_count).to be > current_deploying_instances
         expect do
           subject.call
         end.to change {
@@ -97,7 +96,6 @@ module VCAP::CloudController
           app: web_process.app,
           deploying_web_process: deploying_web_process,
           state: 'DEPLOYING',
-          original_web_process_instance_count: original_web_process_instance_count,
           max_in_flight: 2
         )
       end
@@ -126,7 +124,6 @@ module VCAP::CloudController
           app: web_process.app,
           deploying_web_process: deploying_web_process,
           state: 'DEPLOYING',
-          original_web_process_instance_count: 6,
           max_in_flight: 5
         )
       end
@@ -156,7 +153,6 @@ module VCAP::CloudController
           app: web_process.app,
           deploying_web_process: deploying_web_process,
           state: 'DEPLOYING',
-          original_web_process_instance_count: original_web_process_instance_count,
           max_in_flight: 100
         )
       end
@@ -166,7 +162,7 @@ module VCAP::CloudController
           subject.call
         end.to change {
           deploying_web_process.reload.instances
-        }.by(original_web_process_instance_count)
+        }.by(target_total_instance_count)
       end
 
       it 'doesnt scale down the old web process (there is no new routable instance yet)' do
@@ -178,7 +174,10 @@ module VCAP::CloudController
       end
     end
 
-    context 'when the deployment process has reached original_web_process_instance_count' do
+    context 'when the deployment process has reached interim_desired_instance_count' do
+      let(:interim_desired_instance_count) { 3 }
+      let(:target_total_instance_count) { 6 }
+
       let(:droplet) do
         DropletModel.make(
           process_types: {
@@ -188,7 +187,9 @@ module VCAP::CloudController
         )
       end
 
-      let(:current_deploying_instances) { original_web_process_instance_count }
+      subject(:scale_action) { DeploymentUpdater::Actions::Scale.new(deployment, logger, target_total_instance_count, interim_desired_instance_count) }
+
+      let(:current_deploying_instances) { interim_desired_instance_count }
 
       let!(:interim_deploying_web_process) do
         ProcessModel.make(
@@ -200,31 +201,24 @@ module VCAP::CloudController
         )
       end
 
-      before do
-        allow(ProcessRestart).to receive(:restart)
-        RevisionProcessCommandModel.where(
-          process_type: 'worker',
-          revision_guid: revision.guid
-        ).update(process_command: 'revision-non-web-1-command')
-      end
-
-      it 'finalizes the deployment' do
-        subject.call
+      it 'returns true and leaves the deployment in a deploying state' do
+        expect(subject.call).to be true
         deployment.reload
-        expect(deployment.state).to eq(DeploymentModel::DEPLOYED_STATE)
-        expect(deployment.status_value).to eq(DeploymentModel::FINALIZED_STATUS_VALUE)
-        expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYED_STATUS_REASON)
+        expect(deployment.state).to eq(DeploymentModel::DEPLOYING_STATE)
+
+        earliest_web_process = deployment.app.web_processes.first
+        expect(earliest_web_process.guid).to eq(web_process.guid)
+        expect(earliest_web_process.instances).to eq(2)
+        expect(interim_deploying_web_process.instances).to eq(1)
 
-        after_web_process = deployment.app.web_processes.first
-        expect(after_web_process.guid).to eq(deploying_web_process.guid)
-        expect(after_web_process.instances).to eq(6)
+        expect(deploying_web_process.instances).to eq(3)
       end
     end
 
     context 'when the (oldest) web process will be at zero instances and is type web' do
       let(:current_web_instances) { 1 }
       let(:current_deploying_instances) { 3 }
-      let(:original_web_process_instance_count) { 6 }
+      let(:target_total_instance_count) { 6 }
 
       let!(:interim_deploying_web_process) do
         ProcessModel.make(
@@ -253,7 +247,6 @@ module VCAP::CloudController
             app: web_process.app,
             deploying_web_process: deploying_web_process,
             state: 'DEPLOYING',
-            original_web_process_instance_count: original_web_process_instance_count,
             max_in_flight: 10
           )
         end
@@ -304,7 +297,7 @@ module VCAP::CloudController
 
     context 'when there are more instances in interim processes than there should be' do
       let(:current_deploying_instances) { 10 }
-      let(:original_web_process_instance_count) { 20 }
+      let(:target_total_instance_count) { 20 }
       let(:max_in_flight) { 4 }
 
       let!(:web_process) do
@@ -385,7 +378,7 @@ module VCAP::CloudController
 
     context 'when there are fewer instances in interim processes than there should be' do
       let(:current_deploying_instances) { 10 }
-      let(:original_web_process_instance_count) { 20 }
+      let(:target_total_instance_count) { 20 }
       let(:max_in_flight) { 4 }
 
       let!(:web_process) do
@@ -430,7 +423,7 @@ module VCAP::CloudController
 
     context 'when some instances are missing' do
       let(:current_web_instances) { 10 }
-      let(:original_web_process_instance_count) { 10 }
+      let(:target_total_instance_count) { 10 }
 
       let(:current_deploying_instances) { 9 }
       let(:max_in_flight) { 5 }
@@ -454,7 +447,7 @@ module VCAP::CloudController
 
     context 'when some, but not all, instances have finished' do
       let(:current_web_instances) { 10 }
-      let(:original_web_process_instance_count) { 10 }
+      let(:target_total_instance_count) { 10 }
 
       let(:current_deploying_instances) { 5 }
       let(:max_in_flight) { 5 }
@@ -677,7 +670,6 @@ module VCAP::CloudController
             app: web_process.app,
             deploying_web_process: deploying_web_process,
             state: 'DEPLOYING',
-            original_web_process_instance_count: original_web_process_instance_count,
             max_in_flight: 10
           )
         end
@@ -687,7 +679,7 @@ module VCAP::CloudController
             subject.call
           end.to change {
             deploying_web_process.reload.instances
-          }.by(original_web_process_instance_count)
+          }.by(target_total_instance_count)
         end
       end
     end
@@ -728,5 +720,84 @@ module VCAP::CloudController
         expect(deployment).not_to have_received(:update)
       end
     end
+
+    describe 'interim_desired_instance_count' do 
+      let(:deployment) do
+        DeploymentModel.make(
+          app: web_process.app,
+          deploying_web_process: deploying_web_process,
+          state: 'DEPLOYING',
+          max_in_flight: 100
+        )
+      end
+
+      context 'when not passed in' do 
+        subject(:scale_action) { DeploymentUpdater::Actions::Scale.new(deployment, logger, target_total_instance_count) }
+        let(:target_total_instance_count) { 6 }
+
+        it 'scales up the new web process to the target_total_instance_count' do
+          expect do
+            subject.call
+          end.to change {
+            deploying_web_process.reload.instances
+          }.by(target_total_instance_count)
+        end
+
+        it 'doesnt scale down the old web process (there is no new routable instance yet)' do
+          expect do
+            subject.call
+          end.not_to(change do
+            web_process.reload.instances
+          end)
+        end
+
+        context 'when there are routable instances' do 
+          let(:current_deploying_instances) { 6 }
+
+          it 'does scale down the old web process' do
+            expect do
+              subject.call
+            end.to(change do
+              web_process.reload.instances
+            end.from(6).to(0))
+          end
+
+        end
+      end
+
+      context 'when passed in' do 
+        let(:interim_desired_instance_count) { 3 }
+        let(:target_total_instance_count) { 6 }
+        subject(:scale_action) { DeploymentUpdater::Actions::Scale.new(deployment, logger, target_total_instance_count, interim_desired_instance_count) }
+
+        it 'scales up the new web process to the interim_desired_instance_count' do
+          expect do
+            subject.call
+          end.to change {
+            deploying_web_process.reload.instances
+          }.by(interim_desired_instance_count)
+        end
+
+        it 'doesnt scale down the old web process (there is no new routable instance yet)' do
+          expect do
+            subject.call
+          end.not_to(change do
+            web_process.reload.instances
+          end)
+        end
+
+        context 'when there are routable instances' do 
+          let(:current_deploying_instances) { 3 }
+
+          it 'does scale down the old web process' do
+            expect do
+              subject.call
+            end.to(change do
+              web_process.reload.instances
+            end.from(6).to(3))
+          end
+        end
+      end
+    end
   end
 end