Remove machine ssh_info and state from using actions

Remove the ReadSSHInfo and ReadState actions and corresponding calls to
dispatch queries by vagrant on the provider for current `ssh_info` and
`state` to be handled by actions. Change the corresponding methods added
to the Driver and Provider classes to avoid modifying `machine.id`
directly and allow vagrant to take care of resetting it whenever `state`
returns :not_created.

This ensures that both `ssh_info` and `state` may be called by other
threads, such as the ansible provisioner building the inventory file,
on machines without causing exceptions due to machine locks preventing
modification (setting `machine.id` to nil) and Batch locking preventing
multiple sets of actions being executed on the same machine by
different threads/processes.

Follows the design of the in-tree docker provider for vagrant.

lib/vagrant-libvirt/action.rb

@@ -259,25 +259,6 @@ module VagrantPlugins
         end
       end
 
-      # This action is called to read the state of the machine. The resulting
-      # state is expected to be put into the `:machine_state_id` key.
-      def self.action_read_state
-        Vagrant::Action::Builder.new.tap do |b|
-          b.use ConfigValidate
-          b.use ReadState
-        end
-      end
-
-      # This action is called to read the SSH info of the machine. The
-      # resulting state is expected to be put into the `:machine_ssh_info`
-      # key.
-      def self.action_read_ssh_info
-        Vagrant::Action::Builder.new.tap do |b|
-          b.use ConfigValidate
-          b.use ReadSSHInfo
-        end
-      end
-
       def self.action_read_mac_addresses
         Vagrant::Action::Builder.new.tap do |b|
           b.use ConfigValidate
@@ -336,9 +317,7 @@ module VagrantPlugins
       autoload :PrepareNFSValidIds, action_root.join('prepare_nfs_valid_ids')
       autoload :PruneNFSExports, action_root.join('prune_nfs_exports')
 
-      autoload :ReadSSHInfo, action_root.join('read_ssh_info')
       autoload :ReadMacAddresses, action_root.join('read_mac_addresses')
-      autoload :ReadState, action_root.join('read_state')
       autoload :ResumeDomain, action_root.join('resume_domain')
       autoload :SetNameOfDomain, action_root.join('set_name_of_domain')
 

lib/vagrant-libvirt/action/read_ssh_info.rb

@@ -1,68 +0,0 @@
-require "log4r"
-
-module VagrantPlugins
-  module ProviderLibvirt
-    module Action
-      # This action reads the SSH info for the machine and puts it into the
-      # `:machine_ssh_info` key in the environment.
-      class ReadSSHInfo
-        def initialize(app, env)
-          @app    = app
-          @logger = Log4r::Logger.new("vagrant_libvirt::action::read_ssh_info")
-        end
-
-        def call(env)
-          env[:machine_ssh_info] = read_ssh_info(env[:machine].provider.driver.connection,
-                                                 env[:machine])
-
-          @app.call(env)
-        end
-
-        def read_ssh_info(libvirt, machine)
-          return nil if machine.id.nil?
-          return nil if machine.state.id != :running
-
-          # Find the machine
-          domain = libvirt.servers.get(machine.id)
-          if domain.nil?
-            # The machine can't be found
-            @logger.info("Machine couldn't be found, assuming it got destroyed.")
-            machine.id = nil
-            return nil
-          end
-
-          # Get IP address from dnsmasq lease file.
-          ip_address = nil
-          begin
-            domain.wait_for(2) do
-              addresses.each_pair do |type, ip|
-                # Multiple leases are separated with a newline, return only
-                # the most recent address
-                ip_address = ip[0].split("\n").first if ip[0] != nil
-              end
-              ip_address != nil
-            end
-          rescue Fog::Errors::TimeoutError
-            @logger.info("Timeout at waiting for an ip address for machine %s" % machine.name)
-          end
-
-          if not ip_address
-            @logger.info("No lease found for machine %s" % machine.name)
-            return nil
-          end
-
-          ssh_info = {
-            :host          => ip_address,
-            :port          => machine.config.ssh.guest_port,
-            :forward_agent => machine.config.ssh.forward_agent,
-            :forward_x11   => machine.config.ssh.forward_x11,
-          }
-
-          ssh_info[:proxy_command] = "ssh '#{machine.provider_config.host}' -l '#{machine.provider_config.username}' -i '#{machine.provider_config.id_ssh_key_file}' nc %h %p" if machine.provider_config.connect_via_ssh
-
-          ssh_info
-        end
-      end
-    end
-  end
-end

lib/vagrant-libvirt/action/read_state.rb

@@ -1,60 +0,0 @@
-require 'log4r'
-
-module VagrantPlugins
-  module ProviderLibvirt
-    module Action
-      # This action reads the state of the machine and puts it in the
-      # `:machine_state_id` key in the environment.
-      class ReadState
-        def initialize(app, env)
-          @app    = app
-          @logger = Log4r::Logger.new('vagrant_libvirt::action::read_state')
-        end
-
-        def call(env)
-          env[:machine_state_id] = read_state(env[:machine].provider.driver.connection, env[:machine])
-          @app.call(env)
-        end
-
-        def read_state(libvirt, machine)
-          return :not_created if machine.id.nil?
-
-          begin
-            server = libvirt.servers.get(machine.id)
-          rescue Libvirt::RetrieveError => e
-            server = nil
-            @logger.debug('Machine not found #{e}.')
-          end
-          # Find the machine
-          begin
-            # Wait for libvirt to shutdown the domain
-            while libvirt.servers.get(machine.id).state.to_sym == :'shutting-down' do
-              @logger.info('Waiting on the machine to shut down...')
-              sleep 1
-            end
-
-            server = libvirt.servers.get(machine.id)
-
-            if server.nil? || server.state.to_sym == :terminated
-              # The machine can't be found
-              @logger.info('Machine terminated, assuming it got destroyed.')
-              machine.id = nil
-              return :not_created
-            end
-          rescue Libvirt::RetrieveError => e
-            if e.libvirt_code == ProviderLibvirt::Util::ErrorCodes::VIR_ERR_NO_DOMAIN
-              @logger.info("Machine #{machine.id} not found.")
-              machine.id = nil
-              return :not_created
-            else
-              raise e
-            end
-          end
-
-          # Return the state
-          return server.state.to_sym
-        end
-      end
-    end
-  end
-end

lib/vagrant-libvirt/driver.rb

@@ -47,6 +47,88 @@ module VagrantPlugins
 
         @@connection
       end
+
+      def get_domain(mid)
+        begin
+          domain = connection.servers.get(mid)
+        rescue Libvirt::RetrieveError => e
+          if e.libvirt_code == ProviderLibvirt::Util::ErrorCodes::VIR_ERR_NO_DOMAIN
+            @logger.debug("machine #{mid} not found #{e}.")
+            return nil
+          else
+            raise e
+          end
+        end
+
+        domain
+      end
+
+      def created?(mid)
+        domain = get_domain(mid)
+        !domain.nil?
+      end
+
+      def get_ipaddress(machine)
+        # Find the machine
+        domain = get_domain(machine.id)
+
+        if domain.nil?
+          # The machine can't be found
+          return nil
+        end
+
+        # Get IP address from arp table
+        ip_address = nil
+        begin
+          domain.wait_for(2) do
+            addresses.each_pair do |type, ip|
+              # Multiple leases are separated with a newline, return only
+              # the most recent address
+              ip_address = ip[0].split("\n").first if ip[0] != nil
+            end
+            ip_address != nil
+          end
+        rescue Fog::Errors::TimeoutError
+          @logger.info("Timeout at waiting for an ip address for machine %s" % machine.name)
+        end
+
+        if not ip_address
+          @logger.info("No arp table entry found for machine %s" % machine.name)
+          return nil
+        end
+
+        ip_address
+      end
+
+      def state(machine)
+        # TODO: while this currently matches the previous behaviour in actions
+        # read_state, it shouldn't be necessary to loop and wait for the
+        # machine to reach a state other than shutting-down, before returning
+
+        # may be other error states with initial retreival we can't handle
+        begin
+          domain = get_domain(machine.id)
+        rescue Libvirt::RetrieveError => e
+          @logger.debug("Machine #{machine.id} not found #{e}.")
+          return :not_created
+        end
+
+        # need to wait for the shutting-down state to stablize to a another
+        loop do
+          if domain.nil? || domain.state.to_sym == :terminated
+            return :not_created
+          end
+
+          if domain.state.to_sym != :'shutting-down'
+            # Return the state
+            return domain.state.to_sym
+          end
+
+          @logger.info('Waiting on the machine %s to shut down...' % machine.name)
+          sleep 1
+          domain = get_domain(machine.id)
+        end
+      end
     end
   end
 end

lib/vagrant-libvirt/provider.rb

@@ -40,9 +40,8 @@ module VagrantPlugins
       # SSH into the machine. If the machine is not at a point where
       # SSH is even possible, then `nil` should be returned.
       def ssh_info
-        # Run a custom action called "read_ssh_info" which does what it says
+        # Return the ssh_info if already retrieved otherwise call the driver
-        # and puts the resulting SSH info into the `:machine_ssh_info` key in
+        # and save the result.
-        # the environment.
         #
         # Ssh info has following format..
         #
@@ -52,8 +51,32 @@ module VagrantPlugins
         #  :username => "mitchellh",
         #  :private_key_path => "/path/to/my/key"
         #}
-        env = @machine.action('read_ssh_info', :lock => false)
+        # note that modifing @machine.id or accessing @machine.state is not
-        env[:machine_ssh_info]
+        # thread safe, so be careful to avoid these here as this method may
+        # be called from other threads of execution.
+        return nil if state.id != :running
+
+        ip = driver.get_ipaddress(@machine)
+
+        # if can't determine the IP, just return nil and let the core
+        # deal with it, similar to the docker provider
+        return nil if !ip
+
+        ssh_info = {
+          :host          => ip,
+          :port          => @machine.config.ssh.guest_port,
+          :forward_agent => @machine.config.ssh.forward_agent,
+          :forward_x11   => @machine.config.ssh.forward_x11,
+        }
+
+        ssh_info[:proxy_command] = (
+          "ssh '#{@machine.provider_config.host}' " +
+          "-l '#{@machine.provider_config.username}' " +
+          "-i '#{@machine.provider_config.id_ssh_key_file}' " +
+          "nc %h %p"
+        ) if @machine.provider_config.connect_via_ssh
+
+        ssh_info
       end
 
       def mac_addresses
@@ -71,17 +94,29 @@ module VagrantPlugins
       # This should return the state of the machine within this provider.
       # The state must be an instance of {MachineState}.
       def state
-        # Run a custom action we define called "read_state" which does
-        # what it says. It puts the state in the `:machine_state_id`
-        # key in the environment.
-        env = @machine.action('read_state', :lock => false)
 
-        state_id = env[:machine_state_id]
+        state_id = nil
+        state_id = :not_created if !@machine.id
+        state_id = :not_created if (
+          !state_id && (!@machine.id || !driver.created?(@machine.id)))
+        # Query the driver for the current state of the machine
+        state_id = driver.state(@machine) if @machine.id && !state_id
+        state_id = :unknown if !state_id
+
+        # This is a special pseudo-state so that we don't set the
+        # NOT_CREATED_ID while we're setting up the machine. This avoids
+        # clearing the data dir.
+        state_id = :preparing if @machine.id == "preparing"
 
         # Get the short and long description
         short = I18n.t("vagrant_libvirt.states.short_#{state_id}")
         long  = I18n.t("vagrant_libvirt.states.long_#{state_id}")
 
+        # If we're not created, then specify the special ID flag
+        if state_id == :not_created
+          state_id = Vagrant::MachineState::NOT_CREATED_ID
+        end
+
         # Return the MachineState object
         Vagrant::MachineState.new(state_id, short, long)
       end

locales/en.yml

@@ -155,3 +155,8 @@ en:
       long_running: |-
         The Libvirt domain is running. To stop this machine, you can run
         `vagrant halt`. To destroy the machine, you can run `vagrant destroy`.
+      short_preparing: |-
+        preparing
+      long_preparing: |-
+        The vagrant machine is being prepared for creation, please wait for
+        it to reach a steady state before issuing commands on it.