minor refactors + move updater tests to seperate actions

Author: sethboyles

lib/cloud_controller/deployment_updater/actions/canary.rb

@@ -29,10 +29,6 @@ def call
             logger.info("paused-canary-deployment-for-#{deployment.guid}")
           end
         end
-
-        def instance_reporters
-          CloudController::DependencyLocator.instance.instances_reporters
-        end
       end
     end
   end

lib/cloud_controller/deployment_updater/actions/cancel.rb

@@ -24,7 +24,7 @@ def call
             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)
+            prior_web_process.update(instances: deployment.original_web_process_instance_count)
 
             CleanupWebProcesses.new(deployment, prior_web_process).call
 

lib/cloud_controller/deployment_updater/actions/finalize.rb

@@ -12,8 +12,6 @@ def initialize(deployment)
         end
 
         def call
-          promote_deploying_web_process
-
           CleanupWebProcesses.new(deployment, deploying_web_process).call
 
           update_non_web_processes
@@ -27,10 +25,6 @@ def call
 
         private
 
-        def promote_deploying_web_process
-          deploying_web_process.update(type: ProcessTypes::WEB)
-        end
-
         def update_non_web_processes
           return if deploying_web_process.revision.nil?
 

lib/cloud_controller/deployment_updater/actions/scale.rb

@@ -2,8 +2,6 @@
 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
@@ -26,9 +24,6 @@ def call
             oldest_web_process_with_instances.lock!
             deploying_web_process.lock!
 
-            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,
               state: DeploymentModel::DEPLOYING_STATE,
@@ -50,6 +45,14 @@ def call
 
         private
 
+        def instances_to_scale_down
+          [(oldest_web_process_with_instances.instances - deployment.max_in_flight), 0].max
+        end
+
+        def instances_to_scale_up
+          [deploying_web_process.instances + deployment.max_in_flight, deployment.original_web_process_instance_count].min
+        end
+
         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

lib/cloud_controller/deployment_updater/calculators/instances_to_scale_down.rb

@@ -0,0 +1,201 @@
+require 'spec_helper'
+require 'cloud_controller/deployment_updater/actions/canary'
+
+module VCAP::CloudController
+  RSpec.describe DeploymentUpdater::Actions::Canary do
+    subject(:canary_action) { DeploymentUpdater::Actions::Canary.new(deployment, logger) }
+    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) }
+    let(:droplet) { DropletModel.make }
+    let!(:web_process) do
+      ProcessModel.make(
+        instances: current_web_instances,
+        created_at: a_day_ago,
+        guid: 'guid-original',
+        app: app
+      )
+    end
+    let!(:route_mapping) { RouteMappingModel.make(app: web_process.app, process_type: web_process.type) }
+    let!(:deploying_web_process) do
+      ProcessModel.make(
+        app: web_process.app,
+        type: ProcessTypes::WEB,
+        instances: current_deploying_instances,
+        guid: 'guid-final',
+        revision: revision,
+        state: ProcessModel::STOPPED
+      )
+    end
+    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) { 2 }
+
+    let(:state) { DeploymentModel::PREPAUSED_STATE }
+    let(:current_deploying_instances) { 1 }
+
+    let(:deployment) do
+      DeploymentModel.make(
+        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: 1
+      )
+    end
+
+    let(:diego_instances_reporter) { instance_double(Diego::InstancesReporter) }
+    let(:all_instances_results) do
+      {
+        0 => { state: 'RUNNING', uptime: 50, since: 2, routable: true },
+        1 => { state: 'RUNNING', uptime: 50, since: 2, routable: true },
+        2 => { state: 'RUNNING', uptime: 50, since: 2, routable: true }
+      }
+    end
+    let(:instances_reporters) { double(:instance_reporters) }
+    let(:logger) { instance_double(Steno::Logger, info: nil, error: nil) }
+
+    before do
+      allow(CloudController::DependencyLocator.instance).to receive(:instances_reporters).and_return(instances_reporters)
+      allow(instances_reporters).to receive(:all_instances_for_app).and_return(all_instances_results)
+    end
+
+    it 'locks the deployment' do
+      allow(deployment).to receive(:lock!).and_call_original
+      subject.call
+      expect(deployment).to have_received(:lock!)
+    end
+
+    context 'when the canary instance starts succesfully' do
+      let(:all_instances_results) do
+        {
+          0 => { state: 'RUNNING', uptime: 50, since: 2, routable: true }
+        }
+      end
+
+      it 'pauses the deployment' do
+        subject.call
+        expect(deployment.state).to eq(DeploymentModel::PAUSED_STATE)
+        expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+        expect(deployment.status_reason).to eq(DeploymentModel::PAUSED_STATUS_REASON)
+      end
+
+      it 'updates last_healthy_at' do
+        previous_last_healthy_at = deployment.last_healthy_at
+        Timecop.travel(deployment.last_healthy_at + 10.seconds) do
+          subject.call
+          expect(deployment.reload.last_healthy_at).to be > previous_last_healthy_at
+        end
+      end
+
+      it 'does not alter the existing web processes' do
+        expect do
+          subject.call
+        end.not_to(change do
+          web_process.reload.instances
+        end)
+      end
+
+      it 'logs the canary is paused' do
+        subject.call
+        expect(logger).to have_received(:info).with(
+          "paused-canary-deployment-for-#{deployment.guid}"
+        )
+      end
+    end
+
+    context 'while the canary instance is still starting' do
+      let(:all_instances_results) do
+        {
+          0 => { state: 'STARTING', uptime: 50, since: 2, routable: true }
+        }
+      end
+
+      it 'skips the deployment update' do
+        subject.call
+        expect(deployment.state).to eq(DeploymentModel::PREPAUSED_STATE)
+        expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+        expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+      end
+    end
+
+    context 'when the canary is not routable routable' do
+      let(:all_instances_results) do
+        {
+          0 => { state: 'RUNNING', uptime: 50, since: 2, routable: false }
+        }
+      end
+
+      it 'skips the deployment update' do
+        subject.call
+        expect(deployment.state).to eq(DeploymentModel::PREPAUSED_STATE)
+        expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+        expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+      end
+    end
+
+    context 'when the canary instance is failing' do
+      let(:all_instances_results) do
+        {
+          0 => { state: 'FAILING', uptime: 50, since: 2, routable: true }
+        }
+      end
+
+      it 'does not update the deployments last_healthy_at' do
+        Timecop.travel(Time.now + 1.minute) do
+          expect do
+            subject.call
+          end.not_to(change { deployment.reload.last_healthy_at })
+        end
+      end
+
+      it 'changes nothing' do
+        previous_last_healthy_at = deployment.last_healthy_at
+        subject.call
+        expect(deployment.reload.last_healthy_at).to eq previous_last_healthy_at
+        expect(deployment.state).to eq(DeploymentModel::PREPAUSED_STATE)
+        expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+        expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+      end
+    end
+
+    context 'when there is an interim deployment that has been SUPERSEDED (CANCELED)' do
+      let!(:interim_canceling_web_process) do
+        ProcessModel.make(
+          app: app,
+          created_at: an_hour_ago,
+          type: ProcessTypes::WEB,
+          instances: 1,
+          guid: 'guid-canceling'
+        )
+      end
+      let!(:interim_canceled_superseded_deployment) do
+        DeploymentModel.make(
+          deploying_web_process: interim_canceling_web_process,
+          state: 'CANCELED',
+          status_reason: 'SUPERSEDED'
+        )
+      end
+
+      it 'scales the canceled web process to zero' do
+        subject.call
+        expect(interim_canceling_web_process.reload.instances).to eq(0)
+      end
+    end
+
+    context 'when this deployment got superseded' do
+      before do
+        deployment.update(state: 'DEPLOYED', status_reason: 'SUPERSEDED')
+
+        allow(deployment).to receive(:update).and_call_original
+      end
+
+      it 'skips the deployment update' do
+        subject.call
+        expect(deployment).not_to have_received(:update)
+      end
+    end
+  end
+end