From f51192e80b3a9d671da7948c8d2d92b4c89d29f2 Mon Sep 17 00:00:00 2001 From: Darragh Bailey Date: Sat, 5 Dec 2020 15:49:25 +0000 Subject: [PATCH] Provide uid/gid for additional volumes qemu:///session (#1170) When using qemu:///session, it's necessary to ensure the correct user/group is passed in when creating additional volume storage as otherwise the default is to attempt to chown/chgrp it to 0:0 which will fail. With this in place and recent changes around uri/qemu_use_session, remove the checks guarding retrieving the storage pool as it is also possible for it to be created as expected for the session. Update create domain tests to check for the correct settings such as storage path and user/group id's passed to the volume create call for the additional disks. Fixes: #986 --- lib/vagrant-libvirt/action/create_domain.rb | 13 +- spec/unit/action/create_domain_spec.rb | 144 +++++++++++++----- ...ol.xml => default_system_storage_pool.xml} | 0 .../default_user_storage_pool.xml | 17 +++ 4 files changed, 133 insertions(+), 41 deletions(-) rename spec/unit/action/create_domain_spec/{default_storage_pool.xml => default_system_storage_pool.xml} (100%) create mode 100644 spec/unit/action/create_domain_spec/default_user_storage_pool.xml diff --git a/lib/vagrant-libvirt/action/create_domain.rb b/lib/vagrant-libvirt/action/create_domain.rb index be9cb0e..9ed0766 100644 --- a/lib/vagrant-libvirt/action/create_domain.rb +++ b/lib/vagrant-libvirt/action/create_domain.rb @@ -5,6 +5,7 @@ module VagrantPlugins module Action class CreateDomain include VagrantPlugins::ProviderLibvirt::Util::ErbTemplate + include VagrantPlugins::ProviderLibvirt::Util::StorageUtil def initialize(app, _env) @logger = Log4r::Logger.new('vagrant_libvirt::action::create_domain') @@ -147,12 +148,10 @@ module VagrantPlugins # If we have a box, take the path from the domain volume and set our storage_prefix. # If not, we dump the storage pool xml to get its defined path. # the default storage prefix is typically: /var/lib/libvirt/images/ - if !config.qemu_use_session - if env[:machine].config.vm.box - storage_prefix = File.dirname(@domain_volume_path) + '/' # steal - else - storage_prefix = get_disk_storage_prefix(env, @storage_pool_name) - end + if env[:machine].config.vm.box + storage_prefix = File.dirname(@domain_volume_path) + '/' # steal + else + storage_prefix = get_disk_storage_prefix(env, @storage_pool_name) end @disks.each do |disk| @@ -184,6 +183,8 @@ module VagrantPlugins format_type: disk[:type], path: disk[:absolute_path], capacity: disk[:size], + owner: storage_uid(env), + group: storage_uid(env), #:allocation => ?, pool_name: disk_pool_name ) diff --git a/spec/unit/action/create_domain_spec.rb b/spec/unit/action/create_domain_spec.rb index 56c389f..d85d676 100644 --- a/spec/unit/action/create_domain_spec.rb +++ b/spec/unit/action/create_domain_spec.rb @@ -26,60 +26,134 @@ describe VagrantPlugins::ProviderLibvirt::Action::CreateDomain do allow(connection).to receive(:servers).and_return(servers) allow(connection).to receive(:volumes).and_return(volumes) + allow(logger).to receive(:info) + + env[:domain_name] = "vagrant-test_default" + + # should be ignored for system session and used for user session + allow(Process).to receive(:uid).and_return(9999) + allow(Process).to receive(:gid).and_return(9999) end - context 'default pool' do - let(:test_file) { 'default_storage_pool.xml' } + context 'connection => qemu:///system' do + context 'default pool' do + let(:test_file) { 'default_system_storage_pool.xml' } - it 'should execute correctly' do - expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) - expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) - expect(servers).to receive(:create).and_return(machine) + it 'should execute correctly' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) + expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) + expect(servers).to receive(:create).and_return(machine) + expect(volumes).to_not receive(:create) # additional disks only - expect(subject.call(env)).to be_nil - end + expect(subject.call(env)).to be_nil + end - context 'additional disks' do - let(:vagrantfile) do - <<-EOF - Vagrant.configure('2') do |config| - config.vm.define :test - config.vm.provider :libvirt do |libvirt| - libvirt.storage :file, :size => '20G' + context 'additional disks' do + let(:vagrantfile) do + <<-EOF + Vagrant.configure('2') do |config| + config.vm.define :test + config.vm.provider :libvirt do |libvirt| + libvirt.storage :file, :size => '20G' + end + end + EOF + end + + context 'volume create failed' do + it 'should raise an exception' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) + expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) + expect(volumes).to receive(:create).and_raise(Libvirt::Error) + + expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::FogCreateDomainVolumeError) end end - EOF - end - context 'volume create failed' do - it 'should raise an exception' do - expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) - expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) - expect(volumes).to receive(:create).and_raise(Libvirt::Error) + context 'volume create succeeded' do + it 'should complete' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) + expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) + expect(volumes).to receive(:create).with( + hash_including( + :path => "/var/lib/libvirt/images/vagrant-test_default-vdb.qcow2", + :owner => 0, + :group => 0, + :pool_name => "default", + ) + ) + expect(servers).to receive(:create).and_return(machine) - expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::FogCreateDomainVolumeError) + expect(subject.call(env)).to be_nil + end end end + end - context 'volume create succeeded' do - it 'should complete' do - expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) - expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) - expect(volumes).to receive(:create) - expect(servers).to receive(:create).and_return(machine) + context 'no default pool' do + it 'should raise an exception' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(nil) - expect(subject.call(env)).to be_nil - end + expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::NoStoragePool) end end end - context 'no default pool' do - it 'should raise an exception' do - expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(nil) + context 'connection => qemu:///session' do + let(:vagrantfile) do + <<-EOF + Vagrant.configure('2') do |config| + config.vm.define :test + config.vm.provider :libvirt do |libvirt| + libvirt.qemu_use_session = true + end + end + EOF + end - expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::NoStoragePool) + context 'default pool' do + let(:test_file) { 'default_user_storage_pool.xml' } + + it 'should execute correctly' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) + expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) + expect(servers).to receive(:create).and_return(machine) + + expect(subject.call(env)).to be_nil + end + + context 'additional disks' do + let(:vagrantfile) do + <<-EOF + Vagrant.configure('2') do |config| + config.vm.define :test + config.vm.provider :libvirt do |libvirt| + libvirt.qemu_use_session = true + libvirt.storage :file, :size => '20G' + end + end + EOF + end + + context 'volume create succeeded' do + it 'should complete' do + expect(libvirt_client).to receive(:lookup_storage_pool_by_name).and_return(libvirt_storage_pool) + expect(libvirt_storage_pool).to receive(:xml_desc).and_return(storage_pool_xml) + expect(volumes).to receive(:create).with( + hash_including( + :path => "/var/lib/libvirt/images/vagrant-test_default-vdb.qcow2", + :owner => 9999, + :group => 9999, + :pool_name => "default", + ) + ) + expect(servers).to receive(:create).and_return(machine) + + expect(subject.call(env)).to be_nil + end + end + end end end end diff --git a/spec/unit/action/create_domain_spec/default_storage_pool.xml b/spec/unit/action/create_domain_spec/default_system_storage_pool.xml similarity index 100% rename from spec/unit/action/create_domain_spec/default_storage_pool.xml rename to spec/unit/action/create_domain_spec/default_system_storage_pool.xml diff --git a/spec/unit/action/create_domain_spec/default_user_storage_pool.xml b/spec/unit/action/create_domain_spec/default_user_storage_pool.xml new file mode 100644 index 0000000..751f896 --- /dev/null +++ b/spec/unit/action/create_domain_spec/default_user_storage_pool.xml @@ -0,0 +1,17 @@ + + default + 434e1b75-4a72-45d7-8a98-ebd90c125d22 + 10737418240 + 10737418240 + 10737418240 + + + + /var/lib/libvirt/images + + 0755 + 0 + 0 + + +