Scale action takes total instance count from model rather than relying on original web process count

Author: sethboyles

app/models/runtime/deployment_model.rb

@@ -118,6 +118,10 @@ def current_canary_instance_target
       canary_step[:canary]
     end
 
+    def canary_total_instances
+      canary_step[:canary] + canary_step[:original]
+    end
+
     def canary_step
       raise 'canary_step is only valid for canary deloyments' unless strategy == CANARY_STRATEGY
 
@@ -135,6 +139,7 @@ def canary_step_plan
         target_canary = (original_web_process_instance_count * (weight.to_f / 100)).round.to_i
         target_canary = 1 if target_canary.zero?
         target_original = original_web_process_instance_count - target_canary + 1
+        target_original = 0 if weight == 100
         { canary: target_canary, original: target_original }
       end
     end

lib/cloud_controller/deployment_updater/actions/up_scaler.rb

@@ -31,7 +31,7 @@ def can_scale?
         end
 
         def finished_scaling?
-          deploying_web_process.instances >= interim_desired_instance_count
+          deploying_web_process.instances >= interim_desired_instance_count && @routable_instances_count >= interim_desired_instance_count
         end
 
         private

lib/cloud_controller/deployment_updater/updater.rb

@@ -23,7 +23,7 @@ def scale
       def canary
         with_error_logging('error-canarying-deployment') do
           # TODO: do we need to pass in deployment.original_web_process_instance_count + 1 if there is a single canary instance?
-          finished = Actions::Scale.new(deployment, logger, deployment.original_web_process_instance_count, deployment.current_canary_instance_target).call
+          finished = Actions::Scale.new(deployment, logger, deployment.canary_total_instances, deployment.current_canary_instance_target).call
           if finished
             deployment.update(
               last_healthy_at: Time.now,

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

@@ -101,30 +101,22 @@ module VCAP::CloudController
         expect(up_scaler.finished_scaling?).to be false
       end
 
-      # TODO: Currently, deployments transition to COMPLETED even if there are some instances that have not started and become routable
-      # Since they can still fail at this stage, it might be better to wait for them to all become healthy and routable
-      # before transitioning the deployment to completed
-
       it 'returns false if number of routable instances does not match desired instances' do
-        skip('Might need to restructure the scale action if we do this')
         summary = double(starting_instances_count: 0, routable_instances_count: 4, healthy_instances_count: 5, unhealthy_instances_count: 0)
         up_scaler = DeploymentUpdater::Actions::UpScaler.new(deployment, logger, 5, summary)
         expect(up_scaler.finished_scaling?).to be false
       end
 
-      it 'returns false if there are any unhealthy instances' do
-        skip('Might need to restructure the scale action if we do this')
+      it 'returns true if there are any unhealthy instances but we have already reached the target' do
         summary = double(starting_instances_count: 0, routable_instances_count: 5, healthy_instances_count: 5, unhealthy_instances_count: 1)
-        up_scaler = DeploymentUpdater::Actions::UpScaler.new(deployment, logger, 10, summary)
-        expect(up_scaler.finished_scaling?).to be false
+        up_scaler = DeploymentUpdater::Actions::UpScaler.new(deployment, logger, 5, summary)
+        expect(up_scaler.finished_scaling?).to be true
       end
 
-      it 'returns false if there are any starting instances' do
-        skip('Might need to restructure the scale action if we do this')
-        # TODO: perhaps this should return true?  We have reached the target, after all...
+      it 'returns true if there are any starting instances but we have already reached the target' do
         summary = double(starting_instances_count: 1, routable_instances_count: 5, healthy_instances_count: 5, unhealthy_instances_count: 0)
-        up_scaler = DeploymentUpdater::Actions::UpScaler.new(deployment, logger, 10, summary)
-        expect(up_scaler.finished_scaling?).to be false
+        up_scaler = DeploymentUpdater::Actions::UpScaler.new(deployment, logger, 5, summary)
+        expect(up_scaler.finished_scaling?).to be true
       end
     end
 

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

@@ -192,6 +192,7 @@ module VCAP::CloudController
 
         context 'when the current step instance count has been reached' do
           let(:current_deploying_instances) { 5 }
+          let(:current_web_instances) { 8 }
           let(:all_instances_results) do
             {
               0 => { state: 'RUNNING', uptime: 50, since: 2, routable: true },
@@ -205,6 +206,8 @@ module VCAP::CloudController
           it 'transitions state to paused' do
             subject.canary
             expect(deployment.state).to eq(DeploymentModel::PAUSED_STATE)
+            expect(web_process.reload.instances).to eq(6)
+            expect(deploying_web_process.instances).to eq(5)
           end
         end
 

spec/unit/lib/cloud_controller/deployment_updater/updater_spec.rb

@@ -317,14 +317,14 @@ module VCAP::CloudController
             { canary: 1, original: 1 },
             { canary: 1, original: 1 },
             { canary: 1, original: 1 },
-            { canary: 1, original: 1 }
+            { canary: 1, original: 0 }
           ]
         }
       ]
 
       tests.each do |test|
         context "with #{test[:existing_instances]} existing instances and weights #{test[:weights]}" do
-          let(:canary_steps) { test[:weights].map { |weight| { :instance_weight => weight } } }
+          let(:canary_steps) { test[:weights].map { |weight| { instance_weight: weight } } }
 
           let(:deployment) do
             DeploymentModel.make(
@@ -361,6 +361,25 @@ module VCAP::CloudController
         end
       end
 
+      context 'when deployment is has a 100% step' do
+        let(:deployment) do
+          DeploymentModel.make(
+            app: app,
+            strategy: 'canary',
+            droplet: droplet,
+            deploying_web_process: deploying_web_process,
+            canary_steps: [{ instance_weight: 1 }, { instance_weight: 25 }, { instance_weight: 50 }, { instance_weight: 99 }, { instance_weight: 100 }],
+            original_web_process_instance_count: 10,
+            canary_current_step: 1
+          )
+        end
+
+        it 'provides an extra canary instance for every step except at 100%' do
+          expect(deployment.canary_step_plan).to eq([{ canary: 1, original: 10 }, { canary: 3, original: 8 }, { canary: 5, original: 6 }, { canary: 10, original: 1 },
+                                                     { canary: 10, original: 0 }])
+        end
+      end
+
       context 'when deployment is not canary' do
         let(:deployment) do
           DeploymentModel.make(
@@ -378,6 +397,44 @@ module VCAP::CloudController
       end
     end
 
+    describe '#current_canary_instance_target' do
+      let(:deployment) do
+        DeploymentModel.make(
+          app: app,
+          strategy: 'canary',
+          droplet: droplet,
+          deploying_web_process: deploying_web_process,
+          canary_steps: [{ instance_weight: 1 }, { instance_weight: 25 }, { instance_weight: 50 }, { instance_weight: 99 }, { instance_weight: 100 }],
+          original_web_process_instance_count: 10
+        )
+      end
+
+      it 'returns the canary target of the current step' do
+        deployment.update(canary_current_step: 3)
+
+        expect(deployment.current_canary_instance_target).to eq(5)
+      end
+    end
+
+    describe '#canary_total_instances' do
+      let(:deployment) do
+        DeploymentModel.make(
+          app: app,
+          strategy: 'canary',
+          droplet: droplet,
+          deploying_web_process: deploying_web_process,
+          canary_steps: [{ instance_weight: 1 }, { instance_weight: 25 }, { instance_weight: 50 }, { instance_weight: 99 }, { instance_weight: 100 }],
+          original_web_process_instance_count: 10
+        )
+      end
+
+      it 'returns the total instances of the canary and original processes' do
+        deployment.update(canary_current_step: 3)
+
+        expect(deployment.canary_total_instances).to eq(11)
+      end
+    end
+
     describe '#canary_step' do
       let(:deployment) do
         DeploymentModel.make(
@@ -386,7 +443,7 @@ module VCAP::CloudController
           droplet: droplet,
           deploying_web_process: deploying_web_process,
           original_web_process_instance_count: 10,
-          canary_steps: [{ :instance_weight => 20 }, { :instance_weight => 40 }],
+          canary_steps: [{ instance_weight: 20 }, { instance_weight: 40 }],
           canary_current_step: 1
         )
       end