Refactor instance state handling to use constants

neddp · aramprice · commit 88e21e705f6f · 2026-02-04T17:42:17.000-08:00
- 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
diff --git a/src/bosh-director/lib/bosh/director.rb b/src/bosh-director/lib/bosh/director.rb
@@ -1,5 +1,11 @@
 module Bosh
   module Director
+    INSTANCE_STATE_STARTED = 'started'.freeze
+    INSTANCE_STATE_STOPPED = 'stopped'.freeze
+    INSTANCE_STATE_DETACHED = 'detached'.freeze
+
+    INSTANCE_VIRTUAL_STATE_RESTART = 'restart'.freeze
+    INSTANCE_VIRTUAL_STATE_RECREATE = 'recreate'.freeze
   end
 end
 
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/compilation_instance_pool.rb b/src/bosh-director/lib/bosh/director/deployment_plan/compilation_instance_pool.rb
@@ -168,7 +168,7 @@ def create_instance_plan(stemcell)
         instance = Instance.create_from_instance_group(
           compile_instance_group,
           0,
-          'started',
+          Bosh::Director::INSTANCE_STATE_STARTED,
           @deployment_plan.model,
           {},
           availability_zone,
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/instance.rb b/src/bosh-director/lib/bosh/director/deployment_plan/instance.rb
@@ -5,13 +5,9 @@ module Bosh::Director
   module DeploymentPlan
     # Represents a single Instance Group instance.
     class Instance
-      RECREATE = 'recreate'.freeze
-      RESTART = 'restart'.freeze
-      STARTED = 'started'.freeze
-
       VIRTUAL_STATE_TO_STATE_MAPPING = {
-        RECREATE => STARTED,
-        RESTART => STARTED
+        Bosh::Director::INSTANCE_VIRTUAL_STATE_RESTART => Bosh::Director::INSTANCE_STATE_STARTED,
+        Bosh::Director::INSTANCE_VIRTUAL_STATE_RECREATE => Bosh::Director::INSTANCE_STATE_STARTED
       }
 
       # @return [Integer] Instance index
@@ -311,6 +307,18 @@ def virtual_state
         @virtual_state.to_s.inquiry
       end
 
+      def started?
+        state == Bosh::Director::INSTANCE_STATE_STARTED
+      end
+
+      def stopped?
+        state == Bosh::Director::INSTANCE_STATE_STOPPED
+      end
+
+      def detached?
+        state == Bosh::Director::INSTANCE_STATE_DETACHED
+      end
+
       ##
       # Checks if the target VM already has the same set of trusted SSL certificates
       # as the director currently wants to install on all managed VMs. This will
@@ -385,7 +393,7 @@ def find_or_create_model
         }
 
         Models::Instance.find_or_create(conditions) do |model|
-          model.state = 'started'
+          model.state = Bosh::Director::INSTANCE_STATE_STARTED
           model.compilation = @compilation
           model.uuid = SecureRandom.uuid
           model.variable_set_id = @deployment_model.current_variable_set.id
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/instance_group.rb b/src/bosh-director/lib/bosh/director/deployment_plan/instance_group.rb
@@ -14,7 +14,13 @@ class InstanceGroup
       # recreate and restart are two virtual states
       # (both set  target instance state to "started" and set
       # appropriate instance spec modifiers)
-      VALID_STATES = %w[started stopped detached recreate restart].freeze
+      VALID_STATES = [
+        Bosh::Director::INSTANCE_STATE_STARTED,
+        Bosh::Director::INSTANCE_STATE_STOPPED,
+        Bosh::Director::INSTANCE_STATE_DETACHED,
+        Bosh::Director::INSTANCE_VIRTUAL_STATE_RESTART,
+        Bosh::Director::INSTANCE_VIRTUAL_STATE_RECREATE,
+      ].freeze
 
       # @return [String] Instance group name
       attr_reader :name
@@ -159,7 +165,7 @@ def unignored_instance_plans_needing_duplicate_vm
                                                      .select(&:needs_duplicate_vm?)
                                                      .reject(&:new?)
                                                      .reject(&:should_be_ignored?)
-                                                     .reject { |plan| plan.instance.state == 'detached' }
+                                                     .reject { |plan| plan.instance.detached? }
       end
 
       def add_job(job_to_add)
@@ -370,7 +376,7 @@ def errand?
 
       def instance_plans_with_missing_vms
         needed_instance_plans.reject do |instance_plan|
-          instance_plan.instance.vm_created? || instance_plan.instance.state == 'detached'
+          instance_plan.instance.vm_created? || instance_plan.instance.detached?
         end
       end
 
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/instance_plan.rb b/src/bosh-director/lib/bosh/director/deployment_plan/instance_plan.rb
@@ -186,15 +186,15 @@ def network_settings_changed?
         end
 
         def state_changed?
-          if instance.state.detached? && existing_instance.state != instance.state
+          if instance.detached? && existing_instance.state != instance.state
             @logger.debug("Instance '#{instance}' needs to be detached")
             return true
           end
 
           return true if unresponsive_agent?
 
-          if instance.state.stopped? && instance.current_job_state.running? ||
-              instance.state.started? && !instance.current_job_state.running?
+          if (instance.stopped? && instance.current_job_state.running?) ||
+             (instance.started? && !instance.current_job_state.running?)
             @logger.debug("Instance state is '#{instance.state}' and agent reports '#{instance.current_job_state}'")
             return true
           end
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/instance_repository.rb b/src/bosh-director/lib/bosh/director/deployment_plan/instance_repository.rb
@@ -120,7 +120,7 @@ def create(desired_instance, index)
       instance = Instance.create_from_instance_group(
         desired_instance.instance_group,
         index,
-        'started',
+        Bosh::Director::INSTANCE_STATE_STARTED,
         desired_instance.deployment.model,
         nil,
         desired_instance.az,
diff --git a/src/bosh-director/lib/bosh/director/deployment_plan/planner.rb b/src/bosh-director/lib/bosh/director/deployment_plan/planner.rb
@@ -260,7 +260,7 @@ def instance_groups_starting_on_deploy
           if instance_group.service?
             instance_groups << instance_group
           elsif instance_group.errand?
-            if instance_group.instances.any?(&:vm_created?) || instance_group.instances.any? { |i| i.model.state == 'detached' }
+            if instance_group.instances.any?(&:vm_created?) || instance_group.instances.any? { |i| i.model.detached? }
               instance_groups << instance_group
             end
           end
diff --git a/src/bosh-director/lib/bosh/director/instance_deleter.rb b/src/bosh-director/lib/bosh/director/instance_deleter.rb
@@ -78,7 +78,7 @@ def add_event(deployment_name, instance_name, parent_id = nil, error = nil)
     end
 
     def stop(instance_plan)
-      Stopper.stop(intent: @stop_intent, instance_plan: instance_plan, target_state: 'stopped', logger: @logger)
+      Stopper.stop(intent: @stop_intent, instance_plan: instance_plan, target_state: Bosh::Director::INSTANCE_STATE_STOPPED, logger: @logger)
     end
 
     # FIXME: why do we hate dependency injection?
diff --git a/src/bosh-director/lib/bosh/director/instance_updater/state_applier.rb b/src/bosh-director/lib/bosh/director/instance_updater/state_applier.rb
@@ -17,7 +17,7 @@ def apply(update_config, wait_for_running = true)
       @instance.update_templates(@instance_plan.templates)
       @rendered_job_templates_cleaner.clean
 
-      start(update_config, wait_for_running) if @instance.state == 'started'
+      start(update_config, wait_for_running) if @instance.started?
     end
 
     private
diff --git a/src/bosh-director/lib/bosh/director/instance_updater/update_procedure.rb b/src/bosh-director/lib/bosh/director/instance_updater/update_procedure.rb
@@ -1,20 +1,16 @@
 module Bosh::Director
   class InstanceUpdater
     class UpdateProcedure
-      STARTED = 'started'.freeze
-      STOPPED = 'stopped'.freeze
-      DETACHED = 'detached'.freeze
-
       RECREATE = 'recreate'.freeze
       CREATE = 'create'.freeze
       START = 'start'.freeze
       STOP = 'stop'.freeze
       UPDATE = 'update'.freeze
 
       VIRTUAL_STATE_TO_ACTION_MAPPING = {
-        STARTED => START,
-        STOPPED => STOP,
-        DETACHED => STOP,
+        Bosh::Director::INSTANCE_STATE_STARTED => START,
+        Bosh::Director::INSTANCE_STATE_STOPPED => STOP,
+        Bosh::Director::INSTANCE_STATE_DETACHED => STOP,
       }
 
       attr_reader :instance, :instance_plan, :options, :instance_report, :action, :context
@@ -78,7 +74,7 @@ def perform
           handle_not_detached_instance_plan
 
           # desired state
-          if instance.state == 'stopped'
+          if instance.stopped?
             # Command issued: `bosh stop`
             update_instance
             return
@@ -87,12 +83,12 @@ def perform
           handle_detached_instance_if_detached
         end
 
-        converge_vm if instance.state != 'detached'
+        converge_vm unless instance.detached?
         update_instance
         update_dns_if_changed
         update_vm_disk_metadata
 
-        return if instance.state == 'detached'
+        return if instance.detached?
 
         @rendered_templates_persister.persist(instance_plan)
         apply_state
@@ -119,19 +115,19 @@ def handle_not_detached_instance_plan
           @rendered_templates_persister.persist(instance_plan)
         end
 
-        unless instance_plan.needs_shutting_down? || instance.state == 'detached'
+        unless instance_plan.needs_shutting_down? || instance.detached?
           DeploymentPlan::Steps::PrepareInstanceStep.new(instance_plan).perform(instance_report)
         end
 
         # current state
-        return unless instance.model.state != 'stopped'
+        return if instance.model.stopped?
 
         stop
         take_snapshot
       end
 
       def handle_detached_instance_if_detached
-        return unless instance.state == 'detached'
+        return unless instance.detached?
 
         # Command issued: `bosh stop --hard`
         @logger.info("Detaching instance #{instance}")
@@ -149,7 +145,7 @@ def update_instance
       def update_vm_disk_metadata
         return unless instance_plan.changes.include?(:tags)
         return if instance_plan.new? || @needs_recreate
-        return if instance.state == 'detached' # disks will get a metadata update when attaching again
+        return if instance.detached? # disks will get a metadata update when attaching again
 
         @logger.debug("Updating instance #{instance} VM and disk metadata with tags")
         tags = instance_plan.tags
@@ -188,7 +184,7 @@ def stop
       end
 
       def deleting_vm?
-        @needs_recreate || instance_plan.needs_shutting_down? || instance.state == 'detached' ||
+        @needs_recreate || instance_plan.needs_shutting_down? || instance.detached? ||
           (instance_plan.should_create_swap_delete? && instance_plan.instance.model.vms.count > 1)
       end
 
diff --git a/src/bosh-director/lib/bosh/director/jobs/attach_disk.rb b/src/bosh-director/lib/bosh/director/jobs/attach_disk.rb
@@ -53,7 +53,7 @@ def validate_instance(instance)
               'Attaching disks to ignored instances is not allowed.'
         end
 
-        if instance.state != 'detached' && instance.state != 'stopped'
+        unless instance.detached? || instance.stopped?
           raise AttachDiskInvalidInstanceState, "Instance '#{@job_name}/#{@instance_id}' in deployment '#{@deployment_name}' must be in 'bosh stopped' state"
         end
       end
@@ -66,7 +66,7 @@ def handle_previous_disk(instance)
           @cloud_properties = previous_persistent_disk.cloud_properties
         end
 
-        if instance.state == 'stopped'
+        if instance.stopped?
           @disk_manager.detach_disk(previous_persistent_disk)
         end
 
@@ -88,7 +88,7 @@ def handle_new_disk(instance)
           )
         end
 
-        if instance.state == 'stopped'
+        if instance.stopped?
           @disk_manager.attach_disk(disk, instance.deployment.tags)
         end
       end
diff --git a/src/bosh-director/lib/bosh/director/jobs/update_instance.rb b/src/bosh-director/lib/bosh/director/jobs/update_instance.rb
@@ -106,7 +106,7 @@ def start_instance(instance_model, deployment_plan)
         instance_plan = DeploymentPlan::InstancePlanFromDB.create_from_instance_model(
           instance_model,
           deployment_plan,
-          'started',
+          Bosh::Director::INSTANCE_STATE_STARTED,
           @logger,
         )
 
@@ -145,14 +145,14 @@ def detach_instance(instance_model, instance_plan)
 
         DeploymentPlan::Steps::DeleteVmStep.new(true, false, Config.enable_virtual_delete_vms).perform(instance_report)
         @logger.debug("Setting instance #{@instance_id} state to detached")
-        instance_model.update(state: 'detached')
+        instance_model.update(state: Bosh::Director::INSTANCE_STATE_DETACHED)
       end
 
       def stop(instance_model, instance_plan)
         task_checkpoint
 
         intent = @options['hard'] ? :delete_vm : :keep_vm
-        target_state = @options['hard'] ? 'detached' : 'stopped'
+        target_state = @options['hard'] ? Bosh::Director::INSTANCE_STATE_DETACHED : Bosh::Director::INSTANCE_STATE_STOPPED
         parent_event = add_event('stop', instance_model)
 
         Stopper.stop(intent: intent, instance_plan: instance_plan, target_state: target_state, logger: @logger)
diff --git a/src/bosh-director/lib/bosh/director/models/instance.rb b/src/bosh-director/lib/bosh/director/models/instance.rb
@@ -11,12 +11,15 @@ class Instance < Sequel::Model(Bosh::Director::Config.db)
     many_to_one :variable_set, class: 'Bosh::Director::Models::VariableSet'
     many_to_many :links, :class => 'Bosh::Director::Models::Links::Link', :join_table => :instances_links
 
+    VALID_INSTANCE_STATES = [Bosh::Director::INSTANCE_STATE_STARTED,
+                             Bosh::Director::INSTANCE_STATE_STOPPED,
+                             Bosh::Director::INSTANCE_STATE_DETACHED]
     def validate
       validates_presence [:deployment_id, :job, :index, :state]
       validates_unique [:deployment_id, :job, :index]
       validates_unique [:vms].sort.first
       validates_integer :index
-      validates_includes %w(started stopped detached), :state
+      validates_includes VALID_INSTANCE_STATES, :state
     end
 
     def managed_persistent_disk
@@ -154,11 +157,11 @@ def lifecycle
     end
 
     def expects_vm?
-      lifecycle == 'service' && ['started', 'stopped'].include?(self.state)
+      lifecycle == 'service' && (started? || stopped?)
     end
 
     def has_important_vm?
-      active_vm != nil && state != 'stopped' && !ignore
+      active_vm != nil && !stopped? && !ignore
     end
 
     def most_recent_inactive_vm
@@ -181,12 +184,16 @@ def nats_config_sha1
       active_vm&.nats_config_sha1
     end
 
+    def started?
+      state == Bosh::Director::INSTANCE_STATE_STARTED
+    end
+
     def stopped?
-      state == 'stopped'
+      state == Bosh::Director::INSTANCE_STATE_STOPPED
     end
 
     def detached?
-      state == 'detached'
+      state == Bosh::Director::INSTANCE_STATE_DETACHED
     end
 
     private
diff --git a/src/bosh-director/lib/bosh/director/stopper.rb b/src/bosh-director/lib/bosh/director/stopper.rb
@@ -77,8 +77,8 @@ def perform_drain(instance_plan, target_state, instance_model, agent_client, log
       end
 
       def needs_shutdown?(instance_plan, target_state)
-        target_state == 'stopped' ||
-          target_state == 'detached' ||
+        target_state == Bosh::Director::INSTANCE_STATE_STOPPED ||
+          target_state == Bosh::Director::INSTANCE_STATE_DETACHED ||
           instance_plan.needs_shutting_down? ||
           instance_plan.persistent_disk_changed?
       end
diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/instance_group_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/instance_group_spec.rb
@@ -1111,7 +1111,15 @@
   end
 
   describe '#unignored_instance_plans_needing_duplicate_vm' do
-    let(:instance_plan_instance) { instance_double(Bosh::Director::DeploymentPlan::Instance, vm_created?: true, state: 'started') }
+    let(:instance_state) { 'started' }
+    let(:instance_plan_instance) do
+      instance_double(Bosh::Director::DeploymentPlan::Instance,
+                      vm_created?: true,
+                      state: instance_state,
+                      detached?: instance_state == 'detached',
+                      started?: instance_state == 'started',
+      )
+    end
     let(:instance_plan) do
       instance_double(Bosh::Director::DeploymentPlan::InstancePlan, instance: instance_plan_instance, new?: false, needs_duplicate_vm?: true, should_be_ignored?: false)
     end
@@ -1128,9 +1136,7 @@
     end
 
     context 'when instance group contains detached instance plan' do
-      before do
-        allow(instance_plan_instance).to receive(:state).and_return('detached')
-      end
+      let(:instance_state) { 'detached' }
 
       it 'should filter detached instance plans' do
         expect(instance_group.unignored_instance_plans_needing_duplicate_vm).to be_empty
diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/instance_plan_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/instance_plan_spec.rb
@@ -1358,7 +1358,7 @@ module Bosh::Director::DeploymentPlan
       end
 
       context 'when instance is not being recreated' do
-        let(:instance_state) { 'stopped' }
+        let(:instance_state) { Bosh::Director::INSTANCE_STATE_STOPPED }
 
         it 'should return false when desired instance is in any another state' do
           expect(instance_plan.recreation_requested?).to be_falsey
diff --git a/src/bosh-director/spec/unit/bosh/director/deployment_plan/planner_spec.rb b/src/bosh-director/spec/unit/bosh/director/deployment_plan/planner_spec.rb
diff --git a/src/bosh-director/spec/unit/bosh/director/instance_updater/update_procedure_spec.rb b/src/bosh-director/spec/unit/bosh/director/instance_updater/update_procedure_spec.rb
