Skip to content

Commit 4432b38

Browse files
Samzesethboyles
Samze
authored and
sethboyles
committed
wip more canary pieces
1 parent 622df2c commit 4432b38

File tree

7 files changed

+220
-83
lines changed

7 files changed

+220
-83
lines changed

‎app/actions/deployment_create.rb‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,18 +54,10 @@ def create(app:, user_audit_info:, message:)
5454
revision_guid: revision&.guid,
5555
revision_version: revision&.version,
5656
strategy: message.strategy,
57-
max_in_flight: message.max_in_flight
57+
max_in_flight: message.max_in_flight,
58+
canary_steps: message.options&.dig(:canary, :steps)
5859
)
5960

60-
# TODO: should this dig go into a model func
61-
canary_steps = message.options&.dig(:canary, :steps)
62-
if canary_steps && canary_steps.any?
63-
deployment.update(
64-
canary_steps: canary_steps.map(&:stringify_keys),
65-
canary_current_step: 1
66-
)
67-
end
68-
6961
MetadataUpdate.update(deployment, message)
7062

7163
supersede_deployment(previous_deployment)
@@ -175,8 +167,6 @@ def deployment_for_stopped_app(app, message, previous_deployment, previous_dropl
175167
# Do not create a revision here because AppStart will not handle the rollback case
176168
AppStart.start(app: app, user_audit_info: user_audit_info, create_revision: false)
177169

178-
# TODO, do we update this for canary steps?
179-
180170
deployment = DeploymentModel.create(
181171
app: app,
182172
state: DeploymentModel::DEPLOYED_STATE,
@@ -188,7 +178,8 @@ def deployment_for_stopped_app(app, message, previous_deployment, previous_dropl
188178
revision_guid: revision&.guid,
189179
revision_version: revision&.version,
190180
strategy: message.strategy,
191-
max_in_flight: message.max_in_flight
181+
max_in_flight: message.max_in_flight,
182+
canary_steps: message.options&.dig(:canary, :steps)
192183
)
193184

194185
MetadataUpdate.update(deployment, message)

‎app/models/runtime/deployment_model.rb‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,17 @@ def before_update
9090
super
9191
set_status_updated_at
9292
end
93+
# TODO: documentation
94+
95+
def before_create
96+
self.canary_current_step = 1 if strategy == DeploymentModel::CANARY_STRATEGY
97+
98+
unless canary_steps.nil?
99+
# ensure that canary steps are in the correct format for serialization
100+
self.canary_steps = canary_steps.map(&:stringify_keys)
101+
end
102+
super
103+
end
93104

94105
def deploying?
95106
DeploymentModel::PROGRESSING_STATES.include?(state)
@@ -103,34 +114,21 @@ def continuable?
103114
state == DeploymentModel::PAUSED_STATE
104115
end
105116

106-
# TODO: index 0 for visibility??
107117
def canary_step
108-
# TODO: when strategy is not canary
109-
plan = canary_step_plan
118+
raise 'canary_step is only valid for canary deloyments' unless strategy == CANARY_STRATEGY
110119

111120
current_step = canary_current_step || 1
112-
plan[current_step - 1]
121+
canary_step_plan[current_step - 1]
113122
end
114123

115-
# TODO: to be called by continue deployment. Should this also set the state to pre-paused conditionally (if not last step)
116-
# def set_canary_step_complete
117-
# self.canary_current_step = canary_current_step + 1
118-
# # TODO: do we need to do something for model to save here or is it automatic?
119-
120-
# # if (more steps)
121-
# # set state to prepaused
122-
# # else
123-
# # set state to deploying
124-
# end
125-
126-
# TODO: would be used to populate any API.
127124
def canary_step_plan
128-
# TODO: when strategy is not canary
125+
raise 'canary_step_plan is only valid for canary deloyments' unless strategy == CANARY_STRATEGY
126+
129127
return [{ canary: 1, original: original_web_process_instance_count }] if canary_steps.nil?
130128

131129
canary_steps.map do |step|
132130
weight = step['instance_weight']
133-
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(?))
131+
target_canary = (original_web_process_instance_count * (weight.to_f / 100)).round.to_i
134132
target_canary = 1 if target_canary.zero?
135133
target_original = original_web_process_instance_count - target_canary + 1
136134
{ canary: target_canary, original: target_original }

‎app/presenters/v3/deployment_presenter.rb‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,9 @@ def to_hash
1010
guid: deployment.guid,
1111
created_at: deployment.created_at,
1212
updated_at: deployment.updated_at,
13-
status: build_status(deployment),
13+
status: status(deployment),
1414
strategy: deployment.strategy,
15-
options: {
16-
max_in_flight: deployment.max_in_flight
17-
},
15+
options: options(deployment),
1816
droplet: {
1917
guid: deployment.droplet_guid
2018
},
@@ -57,7 +55,21 @@ def new_processes
5755
end
5856
end
5957

60-
def build_status(deployment)
58+
def options(deployment)
59+
options = {
60+
max_in_flight: deployment.max_in_flight
61+
}
62+
63+
if deployment.strategy == VCAP::CloudController::DeploymentModel::CANARY_STRATEGY && deployment.canary_steps
64+
options[:canary] = {
65+
steps: deployment.canary_steps
66+
}
67+
end
68+
69+
options
70+
end
71+
72+
def status(deployment)
6173
status = {
6274
value: deployment.status_value,
6375
reason: deployment.status_reason,
@@ -70,7 +82,6 @@ def build_status(deployment)
7082
if deployment.strategy == VCAP::CloudController::DeploymentModel::CANARY_STRATEGY
7183
status[:canary] = {
7284
steps: {
73-
# TODO: delegate to model ?
7485
current: deployment.canary_current_step,
7586
total: deployment.canary_steps&.length || 1
7687
}

‎spec/request/deployments_spec.rb‎

Lines changed: 58 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,14 @@
816816
'details' => {
817817
'last_successful_healthcheck' => iso8601,
818818
'last_status_change' => iso8601
819-
}
819+
},
820+
'canary' =>
821+
{
822+
'steps' => {
823+
'current' => 1,
824+
'total' => 1
825+
}
826+
}
820827
},
821828
'strategy' => 'canary',
822829
'droplet' => {
@@ -1059,7 +1066,7 @@
10591066
it 'returns a 422' do
10601067
post '/v3/deployments', create_request.to_json, user_header
10611068
expect(last_response.status).to eq(422), last_response.body
1062-
expect(parsed_response['errors'][0]['detail']).to match('canary options are only valid for canary strategy')
1069+
expect(parsed_response['errors'][0]['detail']).to match('Options canary are only valid for Canary deployments')
10631070
end
10641071
end
10651072

@@ -1080,17 +1087,26 @@
10801087
'details' => {
10811088
'last_successful_healthcheck' => iso8601,
10821089
'last_status_change' => iso8601
1090+
},
1091+
'canary' =>
1092+
{
1093+
'steps' => {
1094+
'current' => 1,
1095+
'total' => 4
1096+
}
10831097
}
10841098
},
10851099
'strategy' => 'canary',
10861100
'options' => {
10871101
'max_in_flight' => 1,
1088-
'steps' => [
1089-
{ 'instance_weight' => 20 },
1090-
{ 'instance_weight' => 40 },
1091-
{ 'instance_weight' => 60 },
1092-
{ 'instance_weight' => 80 }
1093-
]
1102+
'canary' => {
1103+
'steps' => [
1104+
{ 'instance_weight' => 20 },
1105+
{ 'instance_weight' => 40 },
1106+
{ 'instance_weight' => 60 },
1107+
{ 'instance_weight' => 80 }
1108+
]
1109+
}
10941110
},
10951111
'droplet' => {
10961112
'guid' => droplet.guid
@@ -1452,18 +1468,9 @@
14521468
def json_for_deployment(deployment, app_model, droplet, status_value, status_reason, cancel_link=true)
14531469
{
14541470
guid: deployment.guid,
1455-
status: {
1456-
value: status_value,
1457-
reason: status_reason,
1458-
details: {
1459-
last_successful_healthcheck: iso8601,
1460-
last_status_change: iso8601
1461-
}
1462-
},
1471+
status: json_for_status(deployment, status_value, status_reason),
14631472
strategy: deployment.strategy,
1464-
options: {
1465-
max_in_flight: 1
1466-
},
1473+
options: json_for_options(deployment),
14671474
droplet: {
14681475
guid: droplet.guid
14691476
},
@@ -1507,6 +1514,38 @@ def json_for_deployment(deployment, app_model, droplet, status_value, status_rea
15071514
end
15081515
end
15091516

1517+
def json_for_status(deployment, status_value, status_reason)
1518+
status = {
1519+
value: status_value,
1520+
reason: status_reason,
1521+
details: {
1522+
last_successful_healthcheck: iso8601,
1523+
last_status_change: iso8601
1524+
}
1525+
}
1526+
if deployment.strategy == 'canary'
1527+
status[:canary] = {
1528+
steps: {
1529+
current: deployment.canary_current_step || 1,
1530+
total: deployment.canary_steps&.length || 1
1531+
}
1532+
}
1533+
end
1534+
status
1535+
end
1536+
1537+
def json_for_options(deployment)
1538+
options = {
1539+
max_in_flight: deployment.max_in_flight
1540+
}
1541+
if deployment.canary_steps
1542+
options[:canary] = {
1543+
steps: deployment.canary_steps
1544+
}
1545+
end
1546+
options
1547+
end
1548+
15101549
it 'lists all deployments' do
15111550
get '/v3/deployments?per_page=2', nil, admin_user_header
15121551
expect(last_response.status).to eq(200)

‎spec/unit/actions/deployment_create_spec.rb‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -596,6 +596,24 @@ module VCAP::CloudController
596596
end
597597
end
598598

599+
context 'uses canary steps from the message' do
600+
let(:strategy) { 'canary' }
601+
602+
before do
603+
message.options[:canary] = {
604+
steps: [
605+
{ instance_weight: 40 },
606+
{ instance_weight: 80 }
607+
]
608+
}
609+
end
610+
611+
it 'saves the canary steps' do
612+
deployment = DeploymentCreate.create(app:, message:, user_audit_info:)
613+
expect(deployment.canary_steps).to eq([{ 'instance_weight' => 40 }, { 'instance_weight' => 80 }])
614+
end
615+
end
616+
599617
context 'when the app fails to start' do
600618
before do
601619
allow(VCAP::CloudController::AppStart).to receive(:start).and_raise(VCAP::CloudController::AppStart::InvalidApp.new('memory quota_exceeded'))
@@ -1133,7 +1151,7 @@ module VCAP::CloudController
11331151
strategy: strategy,
11341152
options: {
11351153
max_in_flight: max_in_flight,
1136-
canary: { steps: [{ 'instance_weight' => 66 }, { 'instance_weight' => 99 }] }
1154+
canary: { steps: [{ instance_weight: 66 }, { instance_weight: 99 }] }
11371155
}
11381156
})
11391157
end
@@ -1145,17 +1163,7 @@ module VCAP::CloudController
11451163
deployment = DeploymentCreate.create(app:, message:, user_audit_info:)
11461164
end.to change(DeploymentModel, :count).by(1)
11471165

1148-
expect(deployment.canary_steps).to eq([{ instance_weight: 66 }, { instance_weight: 99 }])
1149-
end
1150-
1151-
it 'sets the deployment canary current step' do
1152-
deployment = nil
1153-
1154-
expect do
1155-
deployment = DeploymentCreate.create(app:, message:, user_audit_info:)
1156-
end.to change(DeploymentModel, :count).by(1)
1157-
1158-
expect(deployment.canary_current_step).to eq(1)
1166+
expect(deployment.canary_steps).to eq([{ 'instance_weight' => 66 }, { 'instance_weight' => 99 }])
11591167
end
11601168

11611169
it 'sets starting instances to max in flight' do

0 commit comments

Comments
 (0)