Ensure acceptance tests are executed (#1680)

Ensure the acceptance tests are actually executed and resolve any issues
that have crept in since they were not running as expected.

Call the ResolveDiskSettings on reload to ensure that the start domain
action will have all of the necessary configuration for any additional
storage disks added.

Tidy up create domain output when disks are attached.

Fixes: #1678

.github/workflows/integration-tests.yml

@@ -52,7 +52,7 @@ jobs:
   run-tests:
     needs: generate-matrix
 
-    runs-on: ubuntu-latest
+    runs-on: ubuntu-22.04
     strategy:
       fail-fast: false
       matrix:
@@ -114,8 +114,6 @@ jobs:
     - name: Run tests
       run: |
         mkdir -p $HOME/.vagrant.d/
-        # tell integration tests to use boxes from the cached location
-        export VAGRANT_LIBVIRT_VAGRANT_HOME=$HOME/.vagrant.d
         # use software emulation due to lack of nested emulation
         cat <<EOF > $HOME/.vagrant.d/Vagrantfile
         Vagrant.configure("2") do |config|
@@ -123,5 +121,20 @@ jobs:
             libvirt.driver = "qemu"
           end
         end
-        # run under libvirt group
+        EOF
-        sg libvirt -c "bundle exec rspec --color --fail-fast --tag acceptance -e '${{ matrix.test_name }}'"
+
+        # tell integration tests to use boxes from the cached location
+        # and also to load the above Vagrantfile. This is done because
+        # sg will not inherit env vars, so it's necessary to set them
+        # in a script that sg runs before calling bundle.
+        cat <<EOF > script.bash
+        #!/bin/bash
+        #
+        export VAGRANT_LIBVIRT_VAGRANT_HOME=$HOME/.vagrant.d
+
+        bundle exec rspec --color --fail-fast --tag acceptance -e '${{ matrix.test_name }}'
+        EOF
+        chmod a+x script.bash
+
+        # run script under libvirt group
+        sg libvirt -c ./script.bash

lib/vagrant-libvirt/action.rb

@@ -226,6 +226,8 @@ module VagrantPlugins
 
             b2.use Provision
             b2.use action_halt
+
+            b2.use ResolveDiskSettings
             b2.use action_start
           end
         end

lib/vagrant-libvirt/action/create_domain.rb

@@ -317,16 +317,12 @@ module VagrantPlugins
             env[:ui].info(" -- Boot device:        #{device}")
           end
 
-          unless @disks.empty?
-            env[:ui].info(" -- Disks:         #{_disks_print(@disks)}")
-          end
-
           if not @launchsecurity_data.nil?
             env[:ui].info(" -- Launch security:   #{@launchsecurity_data.map { |k, v| "#{k.to_s}=#{v}" }.join(", ")}")
           end
 
           @disks.each do |disk|
-            msg = " -- Disk(#{disk[:device]}):     #{disk[:absolute_path]}"
+            msg = " -- Disk(#{disk[:device]}):         #{disk[:absolute_path]}, #{disk[:bus]}, #{disk[:size]}"
             msg += ' Shared' if disk[:shareable]
             msg += ' (Remove only manually)' if disk[:allow_existing]
             msg += ' Not created - using existed.' if disk[:preexisting]
@@ -457,12 +453,6 @@ module VagrantPlugins
         end
 
         private
-        def _disks_print(disks)
-          disks.collect do |x|
-            "#{x[:device]}(#{x[:type]}, #{x[:bus]}, #{x[:size]})"
-          end.join(', ')
-        end
-
         def _cdroms_print(cdroms)
           cdroms.collect { |x| x[:dev] }.join(', ')
         end

lib/vagrant-libvirt/action/snapshot_delete.rb

@@ -12,7 +12,7 @@ module VagrantPlugins
           env[:ui].info(I18n.t(
             "vagrant.actions.vm.snapshot.deleting",
             name: env[:snapshot_name]))
-          env[:machine].provider.driver.delete_snapshot(env[:machine], env[:snapshot_name])
+          env[:machine].provider.driver.delete_snapshot(env[:snapshot_name])
 
           env[:ui].success(I18n.t(
             "vagrant.actions.vm.snapshot.deleted",

lib/vagrant-libvirt/action/snapshot_restore.rb

@@ -12,7 +12,7 @@ module VagrantPlugins
           env[:ui].info(I18n.t(
             "vagrant.actions.vm.snapshot.restoring",
             name: env[:snapshot_name]))
-          env[:machine].provider.driver.restore_snapshot(env[:machine], env[:snapshot_name])
+          env[:machine].provider.driver.restore_snapshot(env[:snapshot_name])
 
           @app.call(env)
         end

lib/vagrant-libvirt/action/snapshot_save.rb

@@ -12,8 +12,7 @@ module VagrantPlugins
           env[:ui].info(I18n.t(
             "vagrant.actions.vm.snapshot.saving",
             name: env[:snapshot_name]))
-          env[:machine].provider.driver.create_snapshot(
+          env[:machine].provider.driver.create_snapshot(env[:snapshot_name])
-            env[:machine], env[:snapshot_name])
 
           env[:ui].success(I18n.t(
             "vagrant.actions.vm.snapshot.saved",

lib/vagrant-libvirt/cap/snapshots.rb

@@ -4,7 +4,7 @@ module VagrantPlugins
       class Snapshots
         def self.snapshot_list(machine)
           return if machine.state.id == :not_created
-          machine.provider.driver.list_snapshots(machine)
+          machine.provider.driver.list_snapshots
         end
       end
     end

lib/vagrant-libvirt/driver.rb

@@ -107,7 +107,7 @@ module VagrantPlugins
 
       def restore_snapshot(snapshot_name)
         domain = get_libvirt_domain()
-        snapshot = get_snapshot_if_exists(@machine, snapshot_name)
+        snapshot = get_snapshot_if_exists(snapshot_name)
         begin
           # 4 is VIR_DOMAIN_SNAPSHOT_REVERT_FORCE
           # needed due to https://bugzilla.redhat.com/show_bug.cgi?id=1006886

spec/acceptance/additional_storage_spec.rb

@@ -25,8 +25,5 @@ describe 'additional storage configured', acceptance: true do
     status('Test: reload handles additional storage correctly')
     result = environment.execute('vagrant', 'reload')
     expect(result).to exit_with(0)
-
-    status('Test: additional storage reported correctly')
-    expect(result.stdout).to match(/\(vdb\).*work_default-vdb\.qcow2/)
   end
 end

spec/acceptance/package_domain_spec.rb

@@ -49,7 +49,7 @@ describe 'package domain', acceptance: true do
       status('Test: machine from packaged box is created successfully')
       result = environment.execute('vagrant', 'up', extra_env: testbox_envvars)
       expect(result).to exit_with(0)
-      expect(result.stdout).to match(/test-package-complex-domain/)
+      expect(result.stdout).to match(/test-package-simple-domain/)
     end
   end
 

spec/acceptance/support-skeletons/adjusted_settings/Vagrantfile

@@ -8,7 +8,7 @@ Vagrant.configure("2") do |config|
   config.ssh.shell = "/bin/sh"
   config.vm.synced_folder ".", "/vagrant", disabled: true
   config.vm.provider :libvirt do |libvirt|
-    libvirt.cpu = 2
+    libvirt.cpus = 2
     libvirt.memory = 1000
     libvirt.default_prefix = "changed_default_prefix"
   end

spec/spec_helper.rb

@@ -48,6 +48,10 @@ RSpec.configure do |config|
     # the global plugin manager from trying to use the default VAGRANT_HOME.
     temp_dir = Dir.mktmpdir("rspec-")
     ENV['VAGRANT_HOME'] = temp_dir
+
+    config.after(:suite) do
+      FileUtils.remove_entry temp_dir
+    end
   else
     ENV['VAGRANT_HOME'] = ENV['VAGRANT_LIBVIRT_VAGRANT_HOME']
   end
@@ -63,10 +67,6 @@ RSpec.configure do |config|
     ENV.delete('LIBVIRT_DEFAULT_URI')
   end
 
-  config.after(:suite) do
-    FileUtils.remove_entry temp_dir
-  end
-
   config.mock_with :rspec do |mocks|
     mocks.verify_partial_doubles = true
   end

spec/support/libvirt_acceptance_context.rb

@@ -21,8 +21,10 @@ shared_context 'libvirt_acceptance' do
   end
 
   before(:each) do
+    vagrant_home = ENV.fetch('VAGRANT_HOME', File.expand_path('~/.vagrant.d'))
     # allow execution environment to cache boxes used
-    symlink_boxes(ENV.fetch('VAGRANT_HOME', File.expand_path('~/.vagrant.d')), environment)
+    symlink_boxes(vagrant_home, environment)
+    copy_vagrantfile(vagrant_home, environment)
   end
 
   after(:each) do
@@ -51,6 +53,7 @@ shared_context 'libvirt_acceptance' do
   def duplicate_environment(env, *args)
     dup_env = new_environment(*args)
     symlink_boxes(env.homedir, dup_env)
+    copy_vagrantfile(vagrant_home, environment)
 
     dup_env
   end
@@ -61,4 +64,15 @@ shared_context 'libvirt_acceptance' do
     # allow use the same boxes location as source environment
     File.symlink File.realpath(File.join(vagrant_home, 'boxes')), File.join(target_env.homedir, 'boxes')
   end
+
+  def copy_vagrantfile(vagrant_home, target_env)
+    return if vagrant_home.nil?
+
+    # allows for a helper Vagrantfile to force specific provider options if testing
+    # environment needs them
+    vagrantfile = File.join(vagrant_home, 'Vagrantfile')
+    if File.exists?(vagrantfile) and !File.exists?(File.join(target_env.homedir, 'Vagrantfile'))
+      FileUtils.cp(vagrantfile, target_env.homedir)
+    end
+  end
 end