Skip to content

Commit 5befa06

Browse files
sethboylessethboyles
committed
Canary steps WIP
1 parent 2be3e87 commit 5befa06

File tree

5 files changed

+418
-9
lines changed

5 files changed

+418
-9
lines changed

‎app/messages/deployment_create_message.rb‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,15 @@ class DeploymentCreateMessage < MetadataBaseMessage
2020
allow_nil: true,
2121
hash: true
2222

23-
validate :validate_max_in_flight
23+
validates :canary_steps,
24+
allow_nil: true,
25+
array: true
26+
27+
validates :canary_options,
28+
allow_nil: true,
29+
hash: true
30+
31+
validate :validate_canary_options
2432

2533
def app_guid
2634
relationships&.dig(:app, :data, :guid)
@@ -38,6 +46,18 @@ def max_in_flight
3846
options&.dig(:max_in_flight) || 1
3947
end
4048

49+
def canary_steps
50+
return [] unless canary_options.present? && canary_options.is_a?(Hash)
51+
52+
canary_options&.dig(:steps) || []
53+
end
54+
55+
def canary_options
56+
return {} unless options.present? && options.is_a?(Hash)
57+
58+
options&.dig(:canary) || {}
59+
end
60+
4161
private
4262

4363
def mutually_exclusive_droplet_sources
@@ -46,14 +66,10 @@ def mutually_exclusive_droplet_sources
4666
errors.add(:droplet, "Cannot set both fields 'droplet' and 'revision'")
4767
end
4868

49-
def validate_max_in_flight
50-
return unless options.present? && options.is_a?(Hash) && options[:max_in_flight]
51-
52-
max_in_flight = options[:max_in_flight]
53-
54-
return unless !max_in_flight.is_a?(Integer) || max_in_flight < 1
69+
def validate_canary_options
70+
return if canary_options.blank?
5571

56-
errors.add(:max_in_flight, 'must be an integer greater than 0')
72+
errors.add(:canary_options, 'are only valid for Canary deployments') unless strategy == 'canary'
5773
end
5874
end
5975
end

‎app/messages/validators.rb‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,48 @@ def validate(record)
257257
end
258258
end
259259

260+
class DeploymentOptionsValidator < ActiveModel::Validator
261+
def validate(record)
262+
options = record.options
263+
return unless options.present?
264+
265+
if !options.is_a?(Hash)
266+
record.errors.add(:options, 'must be an object')
267+
return
268+
end
269+
270+
max_in_flight = options[:max_in_flight]
271+
272+
# TODO: figure out if we should allow max_in_flight set to nil explicitly
273+
if options.key?(:max_in_flight) && (!max_in_flight.is_a?(Integer) || max_in_flight < 1)
274+
record.errors.add(:max_in_flight, 'must be an integer greater than 0')
275+
end
276+
277+
canary_options = options[:canary]
278+
return if canary_options.nil?
279+
280+
if !canary_options.is_a?(Hash)
281+
record.errors.add(:"options.canary", 'must be an object')
282+
return
283+
end
284+
285+
# binding.pry
286+
287+
strategy = record.strategy
288+
289+
if canary_options && strategy != "canary"
290+
record.errors.add(:"options.canary", 'are only valid for Canary deployments')
291+
return
292+
end
293+
294+
steps = canary_options[:steps]
295+
296+
if steps && (!steps.is_a?(Array) || steps.any? { |step| !step.is_a?(Hash) })
297+
record.errors.add(:"options.canary.steps", 'must be an array of objects')
298+
end
299+
end
300+
end
301+
260302
class ToOneRelationshipValidator < ActiveModel::EachValidator
261303
def validate_each(record, attribute, relationship)
262304
if has_correct_structure?(relationship)

‎spec/request/deployments_spec.rb‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,111 @@
10281028
end
10291029
end
10301030

1031+
context 'canary steps' do
1032+
let(:user) { make_developer_for_space(space) }
1033+
let(:create_request) do
1034+
{
1035+
strategy: strategy,
1036+
options: {
1037+
canary: {
1038+
steps: [
1039+
{ instance_weight: 20 },
1040+
{ instance_weight: 40 },
1041+
{ instance_weight: 60 },
1042+
{ instance_weight: 80 }
1043+
]
1044+
}
1045+
},
1046+
relationships: {
1047+
app: {
1048+
data: {
1049+
guid: app_model.guid
1050+
}
1051+
}
1052+
}
1053+
}
1054+
end
1055+
1056+
context 'when deployment type is "rolling"' do
1057+
let(:strategy) { 'rolling' }
1058+
1059+
it 'returns a 422' do
1060+
post '/v3/deployments', create_request.to_json, user_header
1061+
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')
1063+
end
1064+
end
1065+
1066+
context 'when deployment type is "canary"' do
1067+
let(:strategy) { 'canary' }
1068+
1069+
it 'succeeds and returns the canary steps in the response' do
1070+
post '/v3/deployments', create_request.to_json, user_header
1071+
expect(last_response.status).to eq(201), last_response.body
1072+
1073+
deployment = VCAP::CloudController::DeploymentModel.last
1074+
1075+
expect(parsed_response).to be_a_response_like({
1076+
'guid' => deployment.guid,
1077+
'status' => {
1078+
'value' => VCAP::CloudController::DeploymentModel::ACTIVE_STATUS_VALUE,
1079+
'reason' => VCAP::CloudController::DeploymentModel::DEPLOYING_STATUS_REASON,
1080+
'details' => {
1081+
'last_successful_healthcheck' => iso8601,
1082+
'last_status_change' => iso8601
1083+
}
1084+
},
1085+
'strategy' => 'canary',
1086+
'options' => {
1087+
'max_in_flight' => 1,
1088+
'steps' => [
1089+
{ 'instance_weight' => 20 },
1090+
{ 'instance_weight' => 40 },
1091+
{ 'instance_weight' => 60 },
1092+
{ 'instance_weight' => 80 }
1093+
]
1094+
},
1095+
'droplet' => {
1096+
'guid' => droplet.guid
1097+
},
1098+
'revision' => {
1099+
'guid' => app_model.latest_revision.guid,
1100+
'version' => app_model.latest_revision.version
1101+
},
1102+
'previous_droplet' => {
1103+
'guid' => droplet.guid
1104+
},
1105+
'new_processes' => [{
1106+
'guid' => deployment.deploying_web_process.guid,
1107+
'type' => deployment.deploying_web_process.type
1108+
}],
1109+
'created_at' => iso8601,
1110+
'updated_at' => iso8601,
1111+
'metadata' => metadata,
1112+
'relationships' => {
1113+
'app' => {
1114+
'data' => {
1115+
'guid' => app_model.guid
1116+
}
1117+
}
1118+
},
1119+
'links' => {
1120+
'self' => {
1121+
'href' => "#{link_prefix}/v3/deployments/#{deployment.guid}"
1122+
},
1123+
'app' => {
1124+
'href' => "#{link_prefix}/v3/apps/#{app_model.guid}"
1125+
},
1126+
'cancel' => {
1127+
'href' => "#{link_prefix}/v3/deployments/#{deployment.guid}/actions/cancel",
1128+
'method' => 'POST'
1129+
}
1130+
}
1131+
})
1132+
end
1133+
end
1134+
end
1135+
10311136
context 'validation failures' do
10321137
let(:user) { make_developer_for_space(space) }
10331138
let(:smol_quota) { VCAP::CloudController::QuotaDefinition.make(memory_limit: 1) }

‎spec/unit/messages/deployment_create_message_spec.rb‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,5 +213,113 @@ module VCAP::CloudController
213213
end
214214
end
215215
end
216+
217+
describe 'canary options' do
218+
before do
219+
body['strategy'] = 'canary'
220+
end
221+
222+
context 'when options.canary is a hash"' do
223+
before do
224+
body['options'] = { canary: { my_option: 'foo' } }
225+
end
226+
227+
it 'is valid' do
228+
message = DeploymentCreateMessage.new(body)
229+
expect(message).to be_valid
230+
expect(message.canary_options).to eq({ my_option: 'foo' })
231+
end
232+
end
233+
234+
context 'when options.canary is not a hash"' do
235+
before do
236+
body['options'] = { canary: 'I should be a hash' }
237+
end
238+
239+
it 'is invalid' do
240+
message = DeploymentCreateMessage.new(body)
241+
expect(message).not_to be_valid
242+
expect(message.errors_on(:canary_options)).to include('must be an object')
243+
end
244+
end
245+
246+
context 'when options.canary is specified but strategy is "rolling"' do
247+
before do
248+
body['strategy'] = 'rolling'
249+
body['options'] = { canary: { my_option: 'foo' } }
250+
end
251+
252+
it 'errors' do
253+
message = DeploymentCreateMessage.new(body)
254+
expect(message).not_to be_valid
255+
expect(message.errors.full_messages).to include('Canary options are only valid for Canary deployments')
256+
end
257+
end
258+
end
259+
260+
describe 'canary steps' do
261+
context 'when objects is not specified' do
262+
before do
263+
body['options'] = nil
264+
end
265+
266+
it 'returns the default value of empty array' do
267+
message = DeploymentCreateMessage.new(body)
268+
expect(message).to be_valid
269+
expect(message.canary_steps).to eq []
270+
end
271+
end
272+
273+
context 'when options.canary_steps is specified' do
274+
before do
275+
body['strategy'] = 'canary'
276+
body['options'] = { canary: {
277+
steps: [
278+
{ instance_weight: 20 },
279+
{ instance_weight: 40 },
280+
{ instance_weight: 60 },
281+
{ instance_weight: 80 }
282+
]
283+
} }
284+
end
285+
286+
# TODO: Perhaps this should be converted to an `instance_weights` property?
287+
it 'returns the array of weights' do
288+
message = DeploymentCreateMessage.new(body)
289+
expect(message).to be_valid
290+
expect(message.canary_steps).to eq [
291+
{ instance_weight: 20 },
292+
{ instance_weight: 40 },
293+
{ instance_weight: 60 },
294+
{ instance_weight: 80 }
295+
]
296+
end
297+
end
298+
299+
context 'when options.canary_steps is not an array' do
300+
before do
301+
body['strategy'] = 'canary'
302+
body['options'] = { canary: {
303+
steps: { instance_weight: 80 }
304+
} }
305+
end
306+
307+
# TODO: Perhaps this should be converted to an `instance_weights` property?
308+
it 'errors' do
309+
message = DeploymentCreateMessage.new(body)
310+
expect(message).not_to be_valid
311+
expect(message.errors.full_messages).to include('Canary steps must be an array')
312+
end
313+
end
314+
315+
context 'validations' do
316+
context 'when one object in the array is missing "instance_weight"'
317+
context 'when one object in the array has a bad key"'
318+
context 'when one instance_weight is below 0'
319+
context 'when one instance_weight is above 100'
320+
context 'when an instance weight is a decimal or non-integer'
321+
context 'when canary steps is an empty array'
322+
end
323+
end
216324
end
217325
end

0 commit comments

Comments
 (0)