From afb53addb146778740ee8e050b61d08556311b99 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Mon, 25 Jan 2016 19:07:14 +0000 Subject: [PATCH 1/3] Add rudimentary spec test to WaitTillUp using vagrant-spec Add some basic spec tests for the WaitTillUp action class to lay some foundations. Utilize vagrant-spec pinned to a known working commit for tests to pass consistently until they provide releases. Requires updating some of the rspec libraries and includes sharedcontext.rb from the jantman/vagrant-r10k project on github. --- Gemfile | 4 +++ spec/spec_helper.rb | 1 + spec/support/libvirt_context.rb | 24 +++++++++++++ spec/support/sharedcontext.rb | 34 +++++++++++++++++++ .../action/wait_till_up_spec.rb | 31 +++++++++++++++++ vagrant-libvirt.gemspec | 6 ++-- 6 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 spec/support/libvirt_context.rb create mode 100644 spec/support/sharedcontext.rb create mode 100644 spec/vagrant-libvirt/action/wait_till_up_spec.rb diff --git a/Gemfile b/Gemfile index 4e4fd9d..4ba77ff 100644 --- a/Gemfile +++ b/Gemfile @@ -13,6 +13,10 @@ group :development do else gem 'vagrant', :git => 'https://github.com/mitchellh/vagrant.git' end + + gem 'vagrant-spec', :github => 'mitchellh/vagrant-spec', + tag: ENV['VAGRANT_SPEC_VERSION'] || "9bba7e1228379c0a249a06ce76ba8ea7d276afbe" + gem 'pry' end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 4842086..4706e48 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -1,5 +1,6 @@ require 'vagrant-libvirt' require 'support/environment_helper' +require 'vagrant-spec/unit' RSpec.configure do |spec| end diff --git a/spec/support/libvirt_context.rb b/spec/support/libvirt_context.rb new file mode 100644 index 0000000..5389047 --- /dev/null +++ b/spec/support/libvirt_context.rb @@ -0,0 +1,24 @@ +require 'fog/libvirt' + +shared_context "libvirt" do + include_context "unit" + + let(:libvirt_context) { true } + let(:id) { "dummy-vagrant_dummy" } + let(:connection) { double("::Fog::Compute") } + + def connection_result(options={}) + result = options.fetch(:result, nil) + double("connection_result" => result) + end + + before do + # we don't want unit tests to ever run commands on the system; so we wire + # in a double to ensure any unexpected messages raise exceptions + stub_const("::Fog::Compute", connection) + + # drivers also call vm_exists? during init; + allow(connection).to receive(:servers).with(kind_of(String)). + and_return(connection_result(result: nil)) + end +end diff --git a/spec/support/sharedcontext.rb b/spec/support/sharedcontext.rb new file mode 100644 index 0000000..474bdb0 --- /dev/null +++ b/spec/support/sharedcontext.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +shared_context "unit" do + include_context 'vagrant-unit' + + let(:test_env) do + vagrantfile ||= <<-EOF + Vagrant.configure('2') do |config| + config.vm.define :test + end + EOF + test_env = isolated_environment + test_env.vagrantfile vagrantfile + test_env + end + let(:env) { { env: iso_env, machine: machine, ui: ui, root_path: '/rootpath' } } + let(:conf) { Vagrant::Config::V2::DummyConfig.new() } + let(:ui) { Vagrant::UI::Basic.new() } + let(:iso_env) { test_env.create_vagrant_env ui_class: Vagrant::UI::Basic } + let(:machine) { iso_env.machine(:test, :libvirt) } + # Mock the communicator to prevent SSH commands for being executed. + let(:communicator) { double('communicator') } + # Mock the guest operating system. + let(:guest) { double('guest') } + let(:app) { lambda { |env| } } + let(:plugin) { register_plugin() } + + before (:each) do + machine.stub(:guest => guest) + machine.stub(:communicator => communicator) + machine.stub(:id => id) + end + +end diff --git a/spec/vagrant-libvirt/action/wait_till_up_spec.rb b/spec/vagrant-libvirt/action/wait_till_up_spec.rb new file mode 100644 index 0000000..fad564f --- /dev/null +++ b/spec/vagrant-libvirt/action/wait_till_up_spec.rb @@ -0,0 +1,31 @@ +require "vagrant-libvirt/action/wait_till_up" +require "vagrant-libvirt/errors" + +require "spec_helper" +require "support/sharedcontext" +require "support/libvirt_context" + +describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do + + subject { described_class.new(app, env) } + + include_context "vagrant-unit" + include_context "libvirt" + include_context "unit" + + describe "#call" do + context "when machine does not exist" do + before do + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:get_domain).and_return(nil) + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state). + and_return(:not_created) + end + + it "raises exception" do + expect(app).to_not receive(:call) + expect{subject.call(env)}.to raise_error + end + end + end + +end diff --git a/vagrant-libvirt.gemspec b/vagrant-libvirt.gemspec index 5b18192..23831b6 100644 --- a/vagrant-libvirt.gemspec +++ b/vagrant-libvirt.gemspec @@ -16,9 +16,9 @@ Gem::Specification.new do |gem| gem.require_paths = ['lib'] gem.version = VagrantPlugins::ProviderLibvirt::VERSION - gem.add_development_dependency "rspec-core", "~> 2.12.2" - gem.add_development_dependency "rspec-expectations", "~> 2.12.1" - gem.add_development_dependency "rspec-mocks", "~> 2.12.1" + gem.add_development_dependency "rspec-core", "~> 2.14.0" + gem.add_development_dependency "rspec-expectations", "~> 2.14.0" + gem.add_development_dependency "rspec-mocks", "~> 2.14.0" gem.add_runtime_dependency 'fog-libvirt', '~> 0.0.1' gem.add_runtime_dependency 'nokogiri', '~> 1.6.0' From dde1b9bd4391f8e6923de9b7827c629aa8880de4 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Mon, 25 Jan 2016 19:11:54 +0000 Subject: [PATCH 2/3] Raise correct exception on domain not found Update spec to check the actually exception raised and fix the code to raise the correct one instead of throwing constant not defined. --- lib/vagrant-libvirt/action/wait_till_up.rb | 8 ++++++-- spec/vagrant-libvirt/action/wait_till_up_spec.rb | 3 ++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/vagrant-libvirt/action/wait_till_up.rb b/lib/vagrant-libvirt/action/wait_till_up.rb index 431a35f..08f5c70 100644 --- a/lib/vagrant-libvirt/action/wait_till_up.rb +++ b/lib/vagrant-libvirt/action/wait_till_up.rb @@ -1,4 +1,5 @@ require 'log4r' +require 'vagrant-libvirt/errors' require 'vagrant-libvirt/util/timer' require 'vagrant/util/retryable' @@ -21,8 +22,11 @@ module VagrantPlugins env[:metrics] ||= {} # Get domain object - domain = env[:machine].provider.driver.connection.servers.get(env[:machine].id.to_s) - raise NoDomainError if domain == nil + domain = env[:machine].provider.driver.get_domain(env[:machine].id.to_s) + if domain == nil + raise Errors::NoDomainError, + :error_message => "Domain #{env[:machine].id} not found" + end # Wait for domain to obtain an ip address. Ip address is searched # from arp table, either localy or remotely via ssh, if libvirt diff --git a/spec/vagrant-libvirt/action/wait_till_up_spec.rb b/spec/vagrant-libvirt/action/wait_till_up_spec.rb index fad564f..a8997ce 100644 --- a/spec/vagrant-libvirt/action/wait_till_up_spec.rb +++ b/spec/vagrant-libvirt/action/wait_till_up_spec.rb @@ -23,7 +23,8 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do it "raises exception" do expect(app).to_not receive(:call) - expect{subject.call(env)}.to raise_error + expect{subject.call(env)}.to raise_error(::VagrantPlugins::ProviderLibvirt::Errors::NoDomainError, + /No domain found. Domain dummy-vagrant_dummy not found/) end end end From 1ecab7a9e90f4375aab5d0a72d377cbf1b1d237b Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Wed, 14 Oct 2015 17:29:07 +0100 Subject: [PATCH 3/3] Support --no-destroy-on-error option Vagrant supports a --no-destroy-on-error option to up to skip destroying of the machine if there was an error on bringing it up. This can be useful where an environment can trigger a bug which would normally result in the domain being torn down preventing additional analysis. Make sure to simply exit the loops by returning terminate, instead of looking to execute the remaining retries. Add spec tests to check that terminate does not call the runner to remove the domain if the user has disabled destroy on error. Define a missing constant for vagrant < 1.6 exposed by the added tests. --- lib/vagrant-libvirt/action/wait_till_up.rb | 27 ++++--- lib/vagrant-libvirt/plugin.rb | 3 + .../action/wait_till_up_spec.rb | 79 +++++++++++++++++++ 3 files changed, 97 insertions(+), 12 deletions(-) diff --git a/lib/vagrant-libvirt/action/wait_till_up.rb b/lib/vagrant-libvirt/action/wait_till_up.rb index 08f5c70..e7bac97 100644 --- a/lib/vagrant-libvirt/action/wait_till_up.rb +++ b/lib/vagrant-libvirt/action/wait_till_up.rb @@ -36,7 +36,7 @@ module VagrantPlugins env[:ui].info(I18n.t("vagrant_libvirt.waiting_for_ip")) retryable(:on => Fog::Errors::TimeoutError, :tries => 300) do # If we're interrupted don't worry about waiting - next if env[:interrupted] + return terminate(env) if env[:interrupted] # Wait for domain to obtain an ip address domain.wait_for(2) { @@ -47,7 +47,6 @@ module VagrantPlugins } end end - terminate(env) if env[:interrupted] @logger.info("Got IP address #{env[:ip_address]}") @logger.info("Time for getting IP: #{env[:metrics]["instance_ip_time"]}") @@ -68,7 +67,8 @@ module VagrantPlugins end end end - terminate(env) if env[:interrupted] + # if interrupted above, just terminate immediately + return terminate(env) if env[:interrupted] @logger.info("Time for SSH ready: #{env[:metrics]["instance_ssh_time"]}") # Booted and ready for use. @@ -80,18 +80,21 @@ module VagrantPlugins def recover(env) return if env["vagrant.error"].is_a?(Vagrant::Errors::VagrantError) - if env[:machine].provider.state.id != :not_created - # Undo the import - terminate(env) - end + # Undo the import + terminate(env) end def terminate(env) - destroy_env = env.dup - destroy_env.delete(:interrupted) - destroy_env[:config_validate] = false - destroy_env[:force_confirm_destroy] = true - env[:action_runner].run(Action.action_destroy, destroy_env) + if env[:machine].provider.state.id != :not_created + # If we're not supposed to destroy on error then just return + return if !env[:destroy_on_error] + + destroy_env = env.dup + destroy_env.delete(:interrupted) + destroy_env[:config_validate] = false + destroy_env[:force_confirm_destroy] = true + env[:action_runner].run(Action.action_destroy, destroy_env) + end end end end diff --git a/lib/vagrant-libvirt/plugin.rb b/lib/vagrant-libvirt/plugin.rb index f624cd5..ae347c7 100644 --- a/lib/vagrant-libvirt/plugin.rb +++ b/lib/vagrant-libvirt/plugin.rb @@ -10,6 +10,9 @@ if Vagrant::VERSION < '1.5.0' raise 'The Vagrant Libvirt plugin is only compatible with Vagrant 1.5+' end +# compatibility fix to define constant not available vagrant <1.6 +::Vagrant::MachineState::NOT_CREATED_ID ||= :not_created + module VagrantPlugins module ProviderLibvirt class Plugin < Vagrant.plugin('2') diff --git a/spec/vagrant-libvirt/action/wait_till_up_spec.rb b/spec/vagrant-libvirt/action/wait_till_up_spec.rb index a8997ce..d76aa56 100644 --- a/spec/vagrant-libvirt/action/wait_till_up_spec.rb +++ b/spec/vagrant-libvirt/action/wait_till_up_spec.rb @@ -27,6 +27,85 @@ describe VagrantPlugins::ProviderLibvirt::Action::WaitTillUp do /No domain found. Domain dummy-vagrant_dummy not found/) end end + + context "when machine is booting" do + before do + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:get_domain).and_return(machine) + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state). + and_return(:running) + end + + context "if interrupted looking for IP" do + before do + env[:interrupted] = true + end + it "should exit" do + expect(app).to_not receive(:call) + expect(ui).to receive(:info).with("Waiting for domain to get an IP address...") + expect(subject.call(env)).to be_nil + end + end + + context "if interrupted waiting for SSH" do + before do + allow(machine).to receive(:wait_for).and_return(true) + allow(env).to receive(:[]).and_call_original + allow(env).to receive(:[]).with(:interrupted).and_return(false, true, true) + allow(env).to receive(:[]).with(:ip_address).and_return("192.168.121.2") + end + it "should exit after getting IP" do + expect(app).to_not receive(:call) + expect(ui).to receive(:info).with("Waiting for domain to get an IP address...") + expect(ui).to receive(:info).with("Waiting for SSH to become available...") + logger = subject.instance_variable_get(:@logger) + expect(logger).to receive(:info).with("Got IP address 192.168.121.2") + expect(logger).to receive(:info).with(/Time for getting IP: .*/) + expect(env[:machine].communicate).to_not receive(:ready?) + expect(subject.call(env)).to be_nil + end + end + end end + describe "#recover" do + before do + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:get_domain).and_return(machine) + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state). + and_return(:not_created) + allow(env).to receive(:[]).and_call_original + end + + it "should do nothing by default" do + expect(env).to_not receive(:[]).with(:action_runner) # cleanup + expect(subject.recover(env)).to be_nil + end + + context "with machine coming up" do + before do + allow_any_instance_of(VagrantPlugins::ProviderLibvirt::Driver).to receive(:state). + and_return(:running) + env[:destroy_on_error] = true + end + + context "and user has disabled destroy on failure" do + before do + env[:destroy_on_error] = false + end + + it "skips terminate on failure" do + expect(env).to_not receive(:[]).with(:action_runner) # cleanup + expect(subject.recover(env)).to be_nil + end + end + + context "and using default settings" do + let(:runner) { double('runner') } + it "deletes VM on failure" do + expect(env).to receive(:[]).with(:action_runner).and_return(runner) # cleanup + expect(runner).to receive(:run) + expect(subject.recover(env)).to be_nil + end + end + end + end end