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

@@ -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
               )

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

spec/unit/action/create_domain_spec/default_user_storage_pool.xml

@@ -0,0 +1,17 @@
+<pool type='dir'>
+  <name>default</name>
+  <uuid>434e1b75-4a72-45d7-8a98-ebd90c125d22</uuid>
+  <capacity unit='bytes'>10737418240</capacity>
+  <allocation unit='bytes'>10737418240</allocation>
+  <available unit='bytes'>10737418240</available>
+  <source>
+  </source>
+  <target>
+    <path>/var/lib/libvirt/images</path>
+    <permissions>
+      <mode>0755</mode>
+      <owner>0</owner>
+      <group>0</group>
+    </permissions>
+  </target>
+</pool>