separate logic out into calculators

Author: sethboyles

lib/cloud_controller/deployment_updater/actions/canary.rb

@@ -16,18 +16,17 @@ def call
           deployment.db.transaction do
             deployment.lock!
             return unless deployment.state == DeploymentModel::PREPAUSED_STATE
+            return unless Calculators::AllInstancesRoutable.new(deployment, logger).call
 
             ScaleDownSuperseded.new(deployment).call
 
-            if Calculators::AllInstancesRoutable.new(deployment, logger).call
-              deployment.update(
-                last_healthy_at: Time.now,
-                state: DeploymentModel::PAUSED_STATE,
-                status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
-                status_reason: DeploymentModel::PAUSED_STATUS_REASON
-              )
-              logger.info("paused-canary-deployment-for-#{deployment.guid}")
-            end
+            deployment.update(
+              last_healthy_at: Time.now,
+              state: DeploymentModel::PAUSED_STATE,
+              status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
+              status_reason: DeploymentModel::PAUSED_STATUS_REASON
+            )
+            logger.info("paused-canary-deployment-for-#{deployment.guid}")
           end
         end
 

lib/cloud_controller/deployment_updater/actions/cancel.rb

@@ -1,3 +1,6 @@
+require 'cloud_controller/deployment_updater/calculators/find_interim_web_process'
+require 'cloud_controller/deployment_updater/actions/cleanup_web_processes'
+
 module VCAP::CloudController
   module DeploymentUpdater
     module Actions
@@ -18,12 +21,12 @@ def call
 
             deploying_web_process.lock!
 
-            prior_web_process = interim_web_process || app.oldest_web_process
+            prior_web_process = Calculators::FindInterimWebProcess.new(deployment).call || app.oldest_web_process
             prior_web_process.lock!
 
             prior_web_process.update(instances: deployment.original_web_process_instance_count, type: ProcessTypes::WEB)
 
-            cleanup_web_processes_except(prior_web_process)
+            CleanupWebProcesses.new(deployment, prior_web_process).call
 
             deployment.update(
               state: DeploymentModel::CANCELED_STATE,
@@ -32,37 +35,6 @@ def call
             )
           end
         end
-
-        private
-
-        def cleanup_web_processes_except(protected_process)
-          app.web_processes.
-            reject { |p| p.guid == protected_process.guid }.
-            map(&:destroy)
-        end
-
-        def interim_web_process
-          # Find newest interim web process that (a) belongs to a SUPERSEDED (DEPLOYED) deployment and (b) has at least
-          # one running instance.
-          app.web_processes_dataset.
-            qualify.
-            join(:deployments, deploying_web_process_guid: :guid).
-            where(deployments__state: DeploymentModel::DEPLOYED_STATE).
-            where(deployments__status_reason: DeploymentModel::SUPERSEDED_STATUS_REASON).
-            order(Sequel.desc(:created_at), Sequel.desc(:id)).
-            find { |p| running_instance?(p) }
-        end
-
-        def running_instance?(process)
-          instances = instance_reporters.all_instances_for_app(process)
-          instances.any? { |_, val| val[:state] == VCAP::CloudController::Diego::LRP_RUNNING }
-        rescue CloudController::Errors::ApiError # the instances_reporter re-raises InstancesUnavailable as ApiError
-          false
-        end
-
-        def instance_reporters
-          CloudController::DependencyLocator.instance.instances_reporters
-        end
       end
     end
   end

lib/cloud_controller/deployment_updater/actions/cleanup_web_processes.rb

@@ -0,0 +1,21 @@
+module VCAP::CloudController
+  module DeploymentUpdater
+    module Actions
+      class CleanupWebProcesses
+        attr_reader :deployment, :app, :protected_process
+
+        def initialize(deployment, process)
+          @deployment = deployment
+          @app = deployment.app
+          @protected_process = process
+        end
+
+        def call
+          app.web_processes.
+            reject { |p| p.guid == protected_process.guid }.
+            map(&:destroy)
+        end
+      end
+    end
+  end
+end

lib/cloud_controller/deployment_updater/actions/finalize.rb

@@ -1,3 +1,4 @@
+require 'cloud_controller/deployment_updater/actions/cleanup_web_processes'
 module VCAP::CloudController
   module DeploymentUpdater
     module Actions
@@ -13,7 +14,7 @@ def initialize(deployment)
         def call
           promote_deploying_web_process
 
-          cleanup_web_processes_except(deploying_web_process)
+          CleanupWebProcesses.new(deployment, deploying_web_process).call
 
           update_non_web_processes
           restart_non_web_processes
@@ -30,12 +31,6 @@ def promote_deploying_web_process
           deploying_web_process.update(type: ProcessTypes::WEB)
         end
 
-        def cleanup_web_processes_except(protected_process)
-          app.web_processes.
-            reject { |p| p.guid == protected_process.guid }.
-            map(&:destroy)
-        end
-
         def update_non_web_processes
           return if deploying_web_process.revision.nil?
 

lib/cloud_controller/deployment_updater/actions/scale.rb

@@ -2,6 +2,8 @@
 require 'cloud_controller/deployment_updater/actions/scale_down_old_process'
 require 'cloud_controller/deployment_updater/actions/finalize'
 require 'cloud_controller/deployment_updater/calculators/all_instances_routable'
+require 'cloud_controller/deployment_updater/calculators/instances_to_scale_up'
+require 'cloud_controller/deployment_updater/calculators/instances_to_scale_down'
 
 module VCAP::CloudController
   module DeploymentUpdater
@@ -17,16 +19,15 @@ def initialize(deployment, logger)
 
         def call
           deployment.db.transaction do
-            app.lock!
-
             return unless deployment.lock!.state == DeploymentModel::DEPLOYING_STATE
+            return unless Calculators::AllInstancesRoutable.new(deployment, logger).call
 
-            ScaleDownSuperseded.new(deployment).call
-
+            app.lock!
             oldest_web_process_with_instances.lock!
             deploying_web_process.lock!
 
-            return unless Calculators::AllInstancesRoutable.new(deployment, logger).call
+            instances_to_scale_up = Calculators::InstancesToScaleUp.new(deployment).call
+            instances_to_scale_down = Calculators::InstancesToScaleDown.new(deployment, oldest_web_process_with_instances).call
 
             deployment.update(
               last_healthy_at: Time.now,
@@ -40,10 +41,10 @@ def call
               return
             end
 
-            ScaleDownOldProcess.new(deployment).call
+            ScaleDownSuperseded.new(deployment).call
+            ScaleDownOldProcess.new(deployment, oldest_web_process_with_instances, instances_to_scale_down).call
 
-            instances_to_scale = [deploying_web_process.instances + deployment.max_in_flight, deployment.original_web_process_instance_count].min
-            deploying_web_process.update(instances: instances_to_scale)
+            deploying_web_process.update(instances: instances_to_scale_up)
           end
         end
 

lib/cloud_controller/deployment_updater/actions/scale_down_old_process.rb

@@ -2,30 +2,26 @@ module VCAP::CloudController
   module DeploymentUpdater
     module Actions
       class ScaleDownOldProcess
-        attr_reader :deployment, :app
+        attr_reader :deployment, :process, :app, :instances_to_scale_down
 
-        def initialize(deployment)
+        def initialize(deployment, process, instances_to_scale_down)
           @deployment = deployment
           @app = deployment.app
+          @process = process
+          @instances_to_scale_down = instances_to_scale_down
         end
 
         def call
-          process = oldest_web_process_with_instances
-
           if process.instances <= deployment.max_in_flight && is_interim_process?(process)
             process.destroy
             return
           end
 
-          process.update(instances: [(process.instances - deployment.max_in_flight), 0].max)
+          process.update(instances: instances_to_scale_down)
         end
 
         private
 
-        def oldest_web_process_with_instances
-          @oldest_web_process_with_instances ||= app.web_processes.select { |process| process.instances > 0 }.min_by { |p| [p.created_at, p.id] }
-        end
-
         def is_original_web_process?(process)
           process == app.oldest_web_process
         end

lib/cloud_controller/deployment_updater/calculators/find_interim_web_process.rb

@@ -0,0 +1,37 @@
+module VCAP::CloudController
+  module DeploymentUpdater
+    module Calculators
+      class FindInterimWebProcess
+        attr_reader :deployment, :app
+
+        def initialize(deployment)
+          @deployment = deployment
+          @app = deployment.app
+        end
+
+        def call
+          # Find newest interim web process that (a) belongs to a SUPERSEDED (DEPLOYED) deployment and (b) has at least
+          # one running instance.
+          app.web_processes_dataset.
+            qualify.
+            join(:deployments, deploying_web_process_guid: :guid).
+            where(deployments__state: DeploymentModel::DEPLOYED_STATE).
+            where(deployments__status_reason: DeploymentModel::SUPERSEDED_STATUS_REASON).
+            order(Sequel.desc(:created_at), Sequel.desc(:id)).
+            find { |p| running_instance?(p) }
+        end
+
+        def running_instance?(process)
+          instances = instance_reporters.all_instances_for_app(process)
+          instances.any? { |_, val| val[:state] == VCAP::CloudController::Diego::LRP_RUNNING }
+        rescue CloudController::Errors::ApiError # the instances_reporter re-raises InstancesUnavailable as ApiError
+          false
+        end
+
+        def instance_reporters
+          CloudController::DependencyLocator.instance.instances_reporters
+        end
+      end
+    end
+  end
+end

lib/cloud_controller/deployment_updater/calculators/instances_to_scale_down.rb

@@ -0,0 +1,18 @@
+module VCAP::CloudController
+  module DeploymentUpdater
+    module Calculators
+      class InstancesToScaleDown
+        attr_reader :deployment, :process
+
+        def initialize(deployment, process)
+          @deployment = deployment
+          @process = process
+        end
+
+        def call
+          [(process.instances - deployment.max_in_flight), 0].max
+        end
+      end
+    end
+  end
+end

lib/cloud_controller/deployment_updater/calculators/instances_to_scale_up.rb

@@ -0,0 +1,17 @@
+module VCAP::CloudController
+  module DeploymentUpdater
+    module Calculators
+      class InstancesToScaleUp
+        attr_reader :deployment
+
+        def initialize(deployment)
+          @deployment = deployment
+        end
+
+        def call
+          [deployment.deploying_web_process.instances + deployment.max_in_flight, deployment.original_web_process_instance_count].min
+        end
+      end
+    end
+  end
+end

lib/cloud_controller/deployment_updater/updater.rb

@@ -37,7 +37,6 @@ def cancel
       def with_error_logging(error_message)
         yield
       rescue StandardError => e
-        # raise e
         error_name = e.is_a?(CloudController::Errors::ApiError) ? e.name : e.class.name
         logger.error(
           error_message,