wip canary step model and actions

Samze · sethboyles · commit 886a824fa53d · 2025-02-18T10:20:57.000-08:00
diff --git a/app/actions/deployment_continue.rb b/app/actions/deployment_continue.rb
@@ -12,11 +12,34 @@ def continue(deployment:, user_audit_info:)
           reject_invalid_state!(deployment) unless deployment.continuable?
 
           record_audit_event(deployment, user_audit_info)
-          deployment.update(
-            state: DeploymentModel::DEPLOYING_STATE,
-            status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
-            status_reason: DeploymentModel::DEPLOYING_STATUS_REASON
-          )
+
+          # state = if deployment.canary_steps && deployment.canary_current_step != deployment.canary_steps.length
+          #           DeploymentModel::PREPAUSED_STATE
+          #         else
+          #           DeploymentModel::DEPLOYING_STATE
+          #         end
+
+          # deployment.update(
+          #   state: state,
+          #   status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
+          #   status_reason: DeploymentModel::DEPLOYING_STATUS_REASON
+          # )
+
+          # TODO: Should we increment step here or in the PAUSE action?
+          if deployment.canary_steps && deployment.canary_current_step < deployment.canary_steps.length
+            deployment.update(
+              state: DeploymentModel::PREPAUSED_STATE,
+              status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
+              status_reason: DeploymentModel::DEPLOYING_STATUS_REASON,
+              canary_current_step: deployment.canary_current_step + 1
+            )
+          else
+            deployment.update(
+              state: DeploymentModel::DEPLOYING_STATE,
+              status_value: DeploymentModel::ACTIVE_STATUS_VALUE,
+              status_reason: DeploymentModel::DEPLOYING_STATUS_REASON
+            )
+          end
         end
       end
 
diff --git a/app/actions/deployment_create.rb b/app/actions/deployment_create.rb
@@ -56,11 +56,21 @@ def create(app:, user_audit_info:, message:)
             strategy: message.strategy,
             max_in_flight: message.max_in_flight
           )
+
+          # TODO: should this dig go into a model func
+          canary_steps = message.options&.dig(:canary, :steps)
+          if canary_steps && canary_steps.any?
+            deployment.update(
+              canary_steps: canary_steps.map(&:stringify_keys),
+              canary_current_step: 1
+            )
+          end
+
           MetadataUpdate.update(deployment, message)
 
           supersede_deployment(previous_deployment)
 
-          process_instances = starting_process_instances(message, desired_instances)
+          process_instances = starting_process_instances(deployment, desired_instances)
 
           process = create_deployment_process(app, deployment.guid, revision, process_instances)
           # Need to transition from STOPPED to STARTED to engage the ProcessObserver to desire the LRP.
@@ -165,6 +175,8 @@ def deployment_for_stopped_app(app, message, previous_deployment, previous_dropl
         # Do not create a revision here because AppStart will not handle the rollback case
         AppStart.start(app: app, user_audit_info: user_audit_info, create_revision: false)
 
+        # TODO, do we update this for canary steps?
+
         deployment = DeploymentModel.create(
           app: app,
           state: DeploymentModel::DEPLOYED_STATE,
@@ -217,14 +229,14 @@ def starting_state(message)
         end
       end
 
-      def starting_process_instances(message, desired_instances)
-        if message.strategy == DeploymentModel::CANARY_STRATEGY
-          # TODO: replace this with [message.max_in_flight, deployment.next_canary_step].min
-          # or remove this 'if' and make `desired_instances` do this work
-          1
-        else
-          [message.max_in_flight, desired_instances].min
-        end
+      def starting_process_instances(deployment, desired_instances)
+        starting_process_count = if deployment.strategy == DeploymentModel::CANARY_STRATEGY
+                                   deployment.canary_step[:canary]
+                                 else
+                                   desired_instances
+                                 end
+
+        [deployment.max_in_flight, starting_process_count].min
       end
 
       def log_rollback_event(app_guid, user_id, revision_id, strategy)
diff --git a/app/models/runtime/deployment_model.rb b/app/models/runtime/deployment_model.rb
@@ -1,5 +1,7 @@
 module VCAP::CloudController
   class DeploymentModel < Sequel::Model(:deployments)
+    plugin :serialization
+
     DEPLOYMENT_STATES = [
       DEPLOYING_STATE = 'DEPLOYING'.freeze,
       PREPAUSED_STATE = 'PREPAUSED'.freeze,
@@ -76,6 +78,8 @@ class DeploymentModel < Sequel::Model(:deployments)
     add_association_dependencies labels: :destroy
     add_association_dependencies annotations: :destroy
 
+    serialize_attributes :json, :canary_steps
+
     dataset_module do
       def deploying_count
         where(state: DeploymentModel::PROGRESSING_STATES).count
@@ -99,6 +103,40 @@ def continuable?
       state == DeploymentModel::PAUSED_STATE
     end
 
+    # TODO: index 0 for visibility??
+    def canary_step
+      # TODO: when strategy is not canary
+      plan = canary_step_plan
+
+      current_step = canary_current_step || 1
+      plan[current_step - 1]
+    end
+
+    # TODO: to be called by continue deployment. Should this also set the state to pre-paused conditionally (if not last step)
+    # def set_canary_step_complete
+    #   self.canary_current_step = canary_current_step + 1
+    #   # TODO: do we need to do something for model to save here or is it automatic?
+
+    #   # if (more steps)
+    #   # set state to prepaused
+    #   # else
+    #   # set state to deploying
+    # end
+
+    # TODO: would be used to populate any API.
+    def canary_step_plan
+      # TODO: when strategy is not canary
+      return [{ canary: 1, original: original_web_process_instance_count }] if canary_steps.nil?
+
+      canary_steps.map do |step|
+        weight = step['instance_weight']
+        target_canary = (original_web_process_instance_count * (weight.to_f / 100)).round.to_i # TODO: this rounds up at mid point 5.5 (is this an issue(?))
+        target_canary = 1 if target_canary.zero?
+        target_original = original_web_process_instance_count - target_canary + 1
+        { canary: target_canary, original: target_original }
+      end
+    end
+
     private
 
     def set_status_updated_at
diff --git a/db/migrations/20250130172009_add_canary_steps_to_deployments.rb b/db/migrations/20250130172009_add_canary_steps_to_deployments.rb
@@ -0,0 +1,15 @@
+Sequel.migration do
+  up do
+    alter_table(:deployments) do
+      # TODO: what size?
+      add_column :canary_steps, String, size: 4096
+      add_column :canary_current_step, :integer
+    end
+  end
+  down do
+    alter_table(:deployments) do
+      drop_column :canary_steps
+      drop_column :canary_current_step
+    end
+  end
+end
diff --git a/spec/unit/actions/deployment_continue_spec.rb b/spec/unit/actions/deployment_continue_spec.rb
@@ -30,36 +30,140 @@ module VCAP::CloudController
         let(:status_value) { DeploymentModel::ACTIVE_STATUS_VALUE }
         let(:status_reason) { DeploymentModel::DEPLOYING_STATUS_REASON }
 
-        it 'sets the deployments status to DEPLOYING' do
-          expect(deployment.state).not_to eq(DeploymentModel::DEPLOYING_STATE)
+        context 'there are no steps defined' do
+          it 'sets the deployments status to DEPLOYING' do
+            expect(deployment.state).not_to eq(DeploymentModel::DEPLOYING_STATE)
 
-          DeploymentContinue.continue(deployment:, user_audit_info:)
-          deployment.reload
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+            deployment.reload
+
+            expect(deployment.state).to eq(DeploymentModel::DEPLOYING_STATE)
+            expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+            expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+          end
+
+          it 'records an audit event for the continue deployment' do
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+
+            event = VCAP::CloudController::Event.find(type: 'audit.app.deployment.continue')
+            expect(event).not_to be_nil
+            expect(event.actor).to eq('1234')
+            expect(event.actor_type).to eq('user')
+            expect(event.actor_name).to eq('eric@example.com')
+            expect(event.actor_username).to eq('eric')
+            expect(event.actee).to eq(app.guid)
+            expect(event.actee_type).to eq('app')
+            expect(event.actee_name).to eq(app.name)
+            expect(event.timestamp).to be
+            expect(event.space_guid).to eq(app.space_guid)
+            expect(event.organization_guid).to eq(app.space.organization.guid)
+            expect(event.metadata).to eq({
+                                           'droplet_guid' => droplet.guid,
+                                           'deployment_guid' => deployment.guid
+                                         })
+          end
+        end
+
+        context 'and there are no remaining steps' do
+          let!(:deployment) do
+            VCAP::CloudController::DeploymentModel.make(
+              state: state,
+              status_value: status_value,
+              status_reason: status_reason,
+              droplet: droplet,
+              app: original_web_process.app,
+              deploying_web_process: deploying_web_process,
+              canary_current_step: 2
+            )
+          end
+
+          it 'sets the deployments status to DEPLOYING' do
+            expect(deployment.state).not_to eq(DeploymentModel::DEPLOYING_STATE)
+
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+            deployment.reload
+
+            expect(deployment.state).to eq(DeploymentModel::DEPLOYING_STATE)
+            expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
+            expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+          end
+
+          it 'records an audit event for the continue deployment' do
+            DeploymentContinue.continue(deployment:, user_audit_info:)
 
-          expect(deployment.state).to eq(DeploymentModel::DEPLOYING_STATE)
-          expect(deployment.status_value).to eq(DeploymentModel::ACTIVE_STATUS_VALUE)
-          expect(deployment.status_reason).to eq(DeploymentModel::DEPLOYING_STATUS_REASON)
+            event = VCAP::CloudController::Event.find(type: 'audit.app.deployment.continue')
+            expect(event).not_to be_nil
+            expect(event.actor).to eq('1234')
+            expect(event.actor_type).to eq('user')
+            expect(event.actor_name).to eq('eric@example.com')
+            expect(event.actor_username).to eq('eric')
+            expect(event.actee).to eq(app.guid)
+            expect(event.actee_type).to eq('app')
+            expect(event.actee_name).to eq(app.name)
+            expect(event.timestamp).to be
+            expect(event.space_guid).to eq(app.space_guid)
+            expect(event.organization_guid).to eq(app.space.organization.guid)
+            expect(event.metadata).to eq({
+                                           'droplet_guid' => droplet.guid,
+                                           'deployment_guid' => deployment.guid
+                                         })
+          end
         end
 
-        it 'records an audit event for the continue deployment' do
-          DeploymentContinue.continue(deployment:, user_audit_info:)
-
-          event = VCAP::CloudController::Event.find(type: 'audit.app.deployment.continue')
-          expect(event).not_to be_nil
-          expect(event.actor).to eq('1234')
-          expect(event.actor_type).to eq('user')
-          expect(event.actor_name).to eq('eric@example.com')
-          expect(event.actor_username).to eq('eric')
-          expect(event.actee).to eq(app.guid)
-          expect(event.actee_type).to eq('app')
-          expect(event.actee_name).to eq(app.name)
-          expect(event.timestamp).to be
-          expect(event.space_guid).to eq(app.space_guid)
-          expect(event.organization_guid).to eq(app.space.organization.guid)
-          expect(event.metadata).to eq({
-                                         'droplet_guid' => droplet.guid,
-                                         'deployment_guid' => deployment.guid
-                                       })
+        context 'and there are remaining steps' do
+          let!(:deployment) do
+            VCAP::CloudController::DeploymentModel.make(
+              state: state,
+              status_value: status_value,
+              status_reason: status_reason,
+              droplet: droplet,
+              app: original_web_process.app,
+              deploying_web_process: deploying_web_process,
+              canary_current_step: 1,
+              canary_steps: [{ 'instance_weight' => 10 }, { 'instance_weight' => 40 }]
+            )
+          end
+
+          it 'sets the deployments status to PREPAUSED' do
+            expect(deployment.state).not_to eq(DeploymentModel::DEPLOYING_STATE)
+
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+            deployment.reload
+
+            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
+
+          it 'increments the current canary step' do
+            expect(deployment.state).not_to eq(DeploymentModel::DEPLOYING_STATE)
+
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+            deployment.reload
+
+            expect(deployment.canary_current_step).to eq(2)
+          end
+
+          it 'records an audit event for the continue deployment' do
+            DeploymentContinue.continue(deployment:, user_audit_info:)
+
+            event = VCAP::CloudController::Event.find(type: 'audit.app.deployment.continue')
+            expect(event).not_to be_nil
+            expect(event.actor).to eq('1234')
+            expect(event.actor_type).to eq('user')
+            expect(event.actor_name).to eq('eric@example.com')
+            expect(event.actor_username).to eq('eric')
+            expect(event.actee).to eq(app.guid)
+            expect(event.actee_type).to eq('app')
+            expect(event.actee_name).to eq(app.name)
+            expect(event.timestamp).to be
+            expect(event.space_guid).to eq(app.space_guid)
+            expect(event.organization_guid).to eq(app.space.organization.guid)
+            expect(event.metadata).to eq({
+                                           'droplet_guid' => droplet.guid,
+                                           'deployment_guid' => deployment.guid
+                                         })
+          end
         end
       end
 
diff --git a/spec/unit/actions/deployment_create_spec.rb b/spec/unit/actions/deployment_create_spec.rb
@@ -1124,6 +1124,58 @@ module VCAP::CloudController
 
             expect(deployment.state).to eq(DeploymentModel::PREPAUSED_STATE)
           end
+
+          context 'and there are canary options with steps' do
+            let(:message) do
+              DeploymentCreateMessage.new({
+                                            relationships: { app: { data: { guid: app.guid } } },
+                                            droplet: { guid: next_droplet.guid },
+                                            strategy: strategy,
+                                            options: {
+                                              max_in_flight: max_in_flight,
+                                              canary: { steps: [{ 'instance_weight' => 66 }, { 'instance_weight' => 99 }] }
+                                            }
+                                          })
+            end
+
+            it 'sets the deployment canary steps' do
+              deployment = nil
+
+              expect do
+                deployment = DeploymentCreate.create(app:, message:, user_audit_info:)
+              end.to change(DeploymentModel, :count).by(1)
+
+              expect(deployment.canary_steps).to eq([{ instance_weight: 66 }, { instance_weight: 99 }])
+            end
+
+            it 'sets the deployment canary current step' do
+              deployment = nil
+
+              expect do
+                deployment = DeploymentCreate.create(app:, message:, user_audit_info:)
+              end.to change(DeploymentModel, :count).by(1)
+
+              expect(deployment.canary_current_step).to eq(1)
+            end
+
+            it 'sets starting instances to max in flight' do
+              DeploymentCreate.create(app:, message:, user_audit_info:)
+
+              deploying_web_process = app.reload.newest_web_process
+              expect(deploying_web_process.instances).to eq(1)
+            end
+
+            context 'when max_in_flight is more than first canary step' do
+              let!(:max_in_flight) { 100 }
+
+              it 'creates a process with first canary step' do
+                DeploymentCreate.create(app:, message:, user_audit_info:)
+
+                deploying_web_process = app.reload.newest_web_process
+                expect(deploying_web_process.instances).to eq(2)
+              end
+            end
+          end
         end
 
         context 'when the strategy is nil' do
diff --git a/spec/unit/models/runtime/deployment_model_spec.rb b/spec/unit/models/runtime/deployment_model_spec.rb
