Skip to content

Commit 742f265

Browse files
neddparamprice
neddp
authored and
aramprice
committed
Refactor instance state handling to use constants
- Remove unnecessary check in already_detached? method - Fix instance state checks to use constants instead of methods - Hoist constants out to Bosh::Director - resurrect attr_accessor's in instance_group.rb - replace magic array's of strings with arrays of constants
1 parent 1d2a233 commit 742f265

12 files changed

Lines changed: 74 additions & 35 deletions

File tree

‎src/bosh-director/lib/bosh/director.rb‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,11 @@
11
module Bosh
22
module Director
3+
INSTANCE_STATE_STARTED = 'started'.freeze
4+
INSTANCE_STATE_STOPPED = 'stopped'.freeze
5+
INSTANCE_STATE_DETACHED = 'detached'.freeze
6+
7+
INSTANCE_VIRTUAL_STATE_RESTART = 'restart'.freeze
8+
INSTANCE_VIRTUAL_STATE_RECREATE = 'recreate'.freeze
39
end
410
end
511

‎src/bosh-director/lib/bosh/director/deployment_plan/instance.rb‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,9 @@ module Bosh::Director
55
module DeploymentPlan
66
# Represents a single Instance Group instance.
77
class Instance
8-
RECREATE = 'recreate'.freeze
9-
RESTART = 'restart'.freeze
10-
STARTED = 'started'.freeze
11-
128
VIRTUAL_STATE_TO_STATE_MAPPING = {
13-
RECREATE => STARTED,
14-
RESTART => STARTED
9+
Bosh::Director::INSTANCE_VIRTUAL_STATE_RESTART => Bosh::Director::INSTANCE_STATE_STARTED,
10+
Bosh::Director::INSTANCE_VIRTUAL_STATE_RECREATE => Bosh::Director::INSTANCE_STATE_STARTED
1511
}
1612

1713
# @return [Integer] Instance index
@@ -311,6 +307,18 @@ def virtual_state
311307
@virtual_state.to_s.inquiry
312308
end
313309

310+
def started?
311+
state == Bosh::Director::INSTANCE_STATE_STARTED
312+
end
313+
314+
def stopped?
315+
state == Bosh::Director::INSTANCE_STATE_STOPPED
316+
end
317+
318+
def detached?
319+
state == Bosh::Director::INSTANCE_STATE_DETACHED
320+
end
321+
314322
##
315323
# Checks if the target VM already has the same set of trusted SSL certificates
316324
# as the director currently wants to install on all managed VMs. This will

‎src/bosh-director/lib/bosh/director/deployment_plan/instance_group.rb‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,13 @@ class InstanceGroup
1414
# recreate and restart are two virtual states
1515
# (both set target instance state to "started" and set
1616
# appropriate instance spec modifiers)
17-
VALID_STATES = %w[started stopped detached recreate restart].freeze
17+
VALID_STATES = [
18+
Bosh::Director::INSTANCE_STATE_STARTED,
19+
Bosh::Director::INSTANCE_STATE_STOPPED,
20+
Bosh::Director::INSTANCE_STATE_DETACHED,
21+
Bosh::Director::INSTANCE_VIRTUAL_STATE_RESTART,
22+
Bosh::Director::INSTANCE_VIRTUAL_STATE_RECREATE,
23+
].freeze
1824

1925
# @return [String] Instance group name
2026
attr_reader :name
@@ -159,7 +165,7 @@ def unignored_instance_plans_needing_duplicate_vm
159165
.select(&:needs_duplicate_vm?)
160166
.reject(&:new?)
161167
.reject(&:should_be_ignored?)
162-
.reject { |plan| plan.instance.state == 'detached' }
168+
.reject { |plan| plan.instance.detached? }
163169
end
164170

165171
def add_job(job_to_add)
@@ -370,7 +376,7 @@ def errand?
370376

371377
def instance_plans_with_missing_vms
372378
needed_instance_plans.reject do |instance_plan|
373-
instance_plan.instance.vm_created? || instance_plan.instance.state == 'detached'
379+
instance_plan.instance.vm_created? || instance_plan.instance.detached?
374380
end
375381
end
376382

‎src/bosh-director/lib/bosh/director/deployment_plan/instance_plan.rb‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,15 +186,15 @@ def network_settings_changed?
186186
end
187187

188188
def state_changed?
189-
if instance.state.detached? && existing_instance.state != instance.state
189+
if instance.detached? && existing_instance.state != instance.state
190190
@logger.debug("Instance '#{instance}' needs to be detached")
191191
return true
192192
end
193193

194194
return true if unresponsive_agent?
195195

196-
if instance.state.stopped? && instance.current_job_state.running? ||
197-
instance.state.started? && !instance.current_job_state.running?
196+
if (instance.stopped? && instance.current_job_state.running?) ||
197+
(instance.started? && !instance.current_job_state.running?)
198198
@logger.debug("Instance state is '#{instance.state}' and agent reports '#{instance.current_job_state}'")
199199
return true
200200
end

‎src/bosh-director/lib/bosh/director/deployment_plan/planner.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ def instance_groups_starting_on_deploy
260260
if instance_group.service?
261261
instance_groups << instance_group
262262
elsif instance_group.errand?
263-
if instance_group.instances.any?(&:vm_created?) || instance_group.instances.any? { |i| i.model.state == 'detached' }
263+
if instance_group.instances.any?(&:vm_created?) || instance_group.instances.any? { |i| i.model.detached? }
264264
instance_groups << instance_group
265265
end
266266
end

‎src/bosh-director/lib/bosh/director/instance_updater/state_applier.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ def apply(update_config, wait_for_running = true)
1717
@instance.update_templates(@instance_plan.templates)
1818
@rendered_job_templates_cleaner.clean
1919

20-
start(update_config, wait_for_running) if @instance.state == 'started'
20+
start(update_config, wait_for_running) if @instance.started?
2121
end
2222

2323
private

‎src/bosh-director/lib/bosh/director/instance_updater/update_procedure.rb‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ def perform
7878
handle_not_detached_instance_plan
7979

8080
# desired state
81-
if instance.state == 'stopped'
81+
if instance.stopped?
8282
# Command issued: `bosh stop`
8383
update_instance
8484
return
@@ -87,12 +87,12 @@ def perform
8787
handle_detached_instance_if_detached
8888
end
8989

90-
converge_vm if instance.state != 'detached'
90+
converge_vm unless instance.detached?
9191
update_instance
9292
update_dns_if_changed
9393
update_vm_disk_metadata
9494

95-
return if instance.state == 'detached'
95+
return if instance.detached?
9696

9797
@rendered_templates_persister.persist(instance_plan)
9898
apply_state
@@ -119,19 +119,19 @@ def handle_not_detached_instance_plan
119119
@rendered_templates_persister.persist(instance_plan)
120120
end
121121

122-
unless instance_plan.needs_shutting_down? || instance.state == 'detached'
122+
unless instance_plan.needs_shutting_down? || instance.detached?
123123
DeploymentPlan::Steps::PrepareInstanceStep.new(instance_plan).perform(instance_report)
124124
end
125125

126126
# current state
127-
return unless instance.model.state != 'stopped'
127+
return if instance.model.stopped?
128128

129129
stop
130130
take_snapshot
131131
end
132132

133133
def handle_detached_instance_if_detached
134-
return unless instance.state == 'detached'
134+
return unless instance.detached?
135135

136136
# Command issued: `bosh stop --hard`
137137
@logger.info("Detaching instance #{instance}")
@@ -149,7 +149,7 @@ def update_instance
149149
def update_vm_disk_metadata
150150
return unless instance_plan.changes.include?(:tags)
151151
return if instance_plan.new? || @needs_recreate
152-
return if instance.state == 'detached' # disks will get a metadata update when attaching again
152+
return if instance.detached? # disks will get a metadata update when attaching again
153153

154154
@logger.debug("Updating instance #{instance} VM and disk metadata with tags")
155155
tags = instance_plan.tags
@@ -188,7 +188,7 @@ def stop
188188
end
189189

190190
def deleting_vm?
191-
@needs_recreate || instance_plan.needs_shutting_down? || instance.state == 'detached' ||
191+
@needs_recreate || instance_plan.needs_shutting_down? || instance.detached? ||
192192
(instance_plan.should_create_swap_delete? && instance_plan.instance.model.vms.count > 1)
193193
end
194194

‎src/bosh-director/lib/bosh/director/jobs/attach_disk.rb‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ def validate_instance(instance)
5353
'Attaching disks to ignored instances is not allowed.'
5454
end
5555

56-
if instance.state != 'detached' && instance.state != 'stopped'
56+
unless instance.detached? || instance.stopped?
5757
raise AttachDiskInvalidInstanceState, "Instance '#{@job_name}/#{@instance_id}' in deployment '#{@deployment_name}' must be in 'bosh stopped' state"
5858
end
5959
end
@@ -66,7 +66,7 @@ def handle_previous_disk(instance)
6666
@cloud_properties = previous_persistent_disk.cloud_properties
6767
end
6868

69-
if instance.state == 'stopped'
69+
if instance.stopped?
7070
@disk_manager.detach_disk(previous_persistent_disk)
7171
end
7272

@@ -88,7 +88,7 @@ def handle_new_disk(instance)
8888
)
8989
end
9090

91-
if instance.state == 'stopped'
91+
if instance.stopped?
9292
@disk_manager.attach_disk(disk, instance.deployment.tags)
9393
end
9494
end

‎src/bosh-director/lib/bosh/director/models/instance.rb‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,15 @@ class Instance < Sequel::Model(Bosh::Director::Config.db)
1111
many_to_one :variable_set, class: 'Bosh::Director::Models::VariableSet'
1212
many_to_many :links, :class => 'Bosh::Director::Models::Links::Link', :join_table => :instances_links
1313

14+
VALID_INSTANCE_STATES = [Bosh::Director::INSTANCE_STATE_STARTED,
15+
Bosh::Director::INSTANCE_STATE_STOPPED,
16+
Bosh::Director::INSTANCE_STATE_DETACHED]
1417
def validate
1518
validates_presence [:deployment_id, :job, :index, :state]
1619
validates_unique [:deployment_id, :job, :index]
1720
validates_unique [:vms].sort.first
1821
validates_integer :index
19-
validates_includes %w(started stopped detached), :state
22+
validates_includes VALID_INSTANCE_STATES, :state
2023
end
2124

2225
def managed_persistent_disk
@@ -154,11 +157,11 @@ def lifecycle
154157
end
155158

156159
def expects_vm?
157-
lifecycle == 'service' && ['started', 'stopped'].include?(self.state)
160+
lifecycle == 'service' && (started? || stopped?)
158161
end
159162

160163
def has_important_vm?
161-
active_vm != nil && state != 'stopped' && !ignore
164+
active_vm != nil && !stopped? && !ignore
162165
end
163166

164167
def most_recent_inactive_vm
@@ -181,12 +184,16 @@ def nats_config_sha1
181184
active_vm&.nats_config_sha1
182185
end
183186

187+
def started?
188+
state == Bosh::Director::INSTANCE_STATE_STARTED
189+
end
190+
184191
def stopped?
185-
state == 'stopped'
192+
state == Bosh::Director::INSTANCE_STATE_STOPPED
186193
end
187194

188195
def detached?
189-
state == 'detached'
196+
state == Bosh::Director::INSTANCE_STATE_DETACHED
190197
end
191198

192199
private

‎src/bosh-director/spec/unit/bosh/director/deployment_plan/instance_group_spec.rb‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1111,7 +1111,15 @@
11111111
end
11121112

11131113
describe '#unignored_instance_plans_needing_duplicate_vm' do
1114-
let(:instance_plan_instance) { instance_double(Bosh::Director::DeploymentPlan::Instance, vm_created?: true, state: 'started') }
1114+
let(:instance_state) { 'started' }
1115+
let(:instance_plan_instance) do
1116+
instance_double(Bosh::Director::DeploymentPlan::Instance,
1117+
vm_created?: true,
1118+
state: instance_state,
1119+
detached?: instance_state == 'detached',
1120+
started?: instance_state == 'started',
1121+
)
1122+
end
11151123
let(:instance_plan) do
11161124
instance_double(Bosh::Director::DeploymentPlan::InstancePlan, instance: instance_plan_instance, new?: false, needs_duplicate_vm?: true, should_be_ignored?: false)
11171125
end
@@ -1128,9 +1136,7 @@
11281136
end
11291137

11301138
context 'when instance group contains detached instance plan' do
1131-
before do
1132-
allow(instance_plan_instance).to receive(:state).and_return('detached')
1133-
end
1139+
let(:instance_state) { 'detached' }
11341140

11351141
it 'should filter detached instance plans' do
11361142
expect(instance_group.unignored_instance_plans_needing_duplicate_vm).to be_empty

0 commit comments

Comments
 (0)