Restore handling of disk_device domain setting (#1365)

Re-enable handling of the disk_device domain volume setting to ensure it
can be overridden from the default of vda to a value chosen.

Provide a disk resolver to resolve devices after the box has been downloaded
so that initial devices can be correctly allocated and avoid conflicts with
additional disks added that would otherwise get assigned the same device.

Removes hack for destroy domain when more than one disk, as now devices
in the config are only present if provided by the configuration.

Fixes: #1353

spec/unit/action/create_domain_spec.rb

@@ -33,7 +33,6 @@ describe VagrantPlugins::ProviderLibvirt::Action::CreateDomain do
       allow(connection).to receive(:volumes).and_return(volumes)
       allow(volumes).to receive(:all).and_return([domain_volume])
       allow(domain_volume).to receive(:pool_name).and_return('default')
-      allow(domain_volume).to receive(:[]).with('name').and_return('vagrant-test_default.img')
       allow(domain_volume).to receive(:path).and_return('/var/lib/libvirt/images/vagrant-test_default.img')
       allow(machine).to receive_message_chain("box.name") { 'vagrant-libvirt/test' }
 
@@ -117,6 +116,48 @@ describe VagrantPlugins::ProviderLibvirt::Action::CreateDomain do
             end
           end
         end
+
+        context 'with custom disk device setting' do
+          let(:domain_xml_file) { 'custom_disk_settings.xml' }
+          let(:vagrantfile_providerconfig) {
+            <<-EOF
+              libvirt.disk_device = 'sda'
+            EOF
+          }
+
+          it 'should set the domain device' do
+            expect(ui).to receive(:info).with(/ -- Image\(sda\):.*/)
+            expect(servers).to receive(:create).with(xml: domain_xml).and_return(machine)
+
+            expect(subject.call(env)).to be_nil
+          end
+        end
+
+        context 'with two domain disks' do
+          let(:domain_xml_file) { 'two_disk_settings.xml' }
+          let(:domain_volume_2) { double('domain_volume 2') }
+
+          before do
+            expect(volumes).to receive(:all).and_return([domain_volume])
+            expect(volumes).to receive(:all).and_return([domain_volume_2])
+            expect(domain_volume_2).to receive(:pool_name).and_return('default')
+            expect(domain_volume_2).to receive(:path).and_return('/var/lib/libvirt/images/vagrant-test_default_1.img')
+
+            env[:box_volumes].push({
+              :path=>"/test/box_1.img",
+              :name=>"test_vagrant_box_image_1.1.1_1.img",
+              :virtual_size=> ByteNumber.new(5),
+            })
+          end
+
+          it 'should correctly assign device entries' do
+            expect(ui).to receive(:info).with(/ -- Image\(vda\):.*/)
+            expect(ui).to receive(:info).with(/ -- Image\(vdb\):.*/)
+            expect(servers).to receive(:create).with(xml: domain_xml).and_return(machine)
+
+            expect(subject.call(env)).to be_nil
+          end
+        end
       end
 
       context 'no default pool' do

spec/unit/action/create_domain_spec/custom_disk_settings.xml

@@ -0,0 +1,55 @@
+<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
+  <name>vagrant-test_default</name>
+  <title></title>
+  <description>Source: /rootpath/Vagrantfile</description>
+  <uuid></uuid>
+  <memory>524288</memory>
+  <vcpu>1</vcpu>
+
+
+  <cpu mode='host-model'>
+      <model fallback='allow'></model>
+  </cpu>
+
+
+  <os>
+        <type>hvm</type>
+    <kernel></kernel>
+    <initrd></initrd>
+    <cmdline></cmdline>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'>
+  </clock>
+  <devices>
+    <disk type='file' device='disk'>
+      <alias name='ua-box-volume-0'/>
+      <driver name='qemu' type='qcow2' cache='default'/>
+      <source file='/var/lib/libvirt/images/vagrant-test_default.img'/>
+      <target dev='sda' bus='virtio'/>
+    </disk>
+
+
+    <serial type='pty'>
+      <target port='0'/>
+    </serial>
+    <console type='pty'>
+      <target port='0'/>
+    </console>
+
+
+    <input type='mouse' bus='ps2'/>
+
+      <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' keymap='en-us'  />
+      <video>
+        <model type='cirrus' vram='9216' heads='1'/>
+      </video>
+
+
+  </devices>
+
+</domain>

spec/unit/action/create_domain_spec/two_disk_settings.xml

@@ -0,0 +1,61 @@
+<domain type='kvm' xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'>
+  <name>vagrant-test_default</name>
+  <title></title>
+  <description>Source: /rootpath/Vagrantfile</description>
+  <uuid></uuid>
+  <memory>524288</memory>
+  <vcpu>1</vcpu>
+
+
+  <cpu mode='host-model'>
+      <model fallback='allow'></model>
+  </cpu>
+
+
+  <os>
+        <type>hvm</type>
+    <kernel></kernel>
+    <initrd></initrd>
+    <cmdline></cmdline>
+  </os>
+  <features>
+    <acpi/>
+    <apic/>
+    <pae/>
+  </features>
+  <clock offset='utc'>
+  </clock>
+  <devices>
+    <disk type='file' device='disk'>
+      <alias name='ua-box-volume-0'/>
+      <driver name='qemu' type='qcow2' cache='default'/>
+      <source file='/var/lib/libvirt/images/vagrant-test_default.img'/>
+      <target dev='vda' bus='virtio'/>
+    </disk>
+    <disk type='file' device='disk'>
+      <alias name='ua-box-volume-1'/>
+      <driver name='qemu' type='qcow2' cache='default'/>
+      <source file='/var/lib/libvirt/images/vagrant-test_default_1.img'/>
+      <target dev='vdb' bus='virtio'/>
+    </disk>
+
+
+    <serial type='pty'>
+      <target port='0'/>
+    </serial>
+    <console type='pty'>
+      <target port='0'/>
+    </console>
+
+
+    <input type='mouse' bus='ps2'/>
+
+      <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1' keymap='en-us'  />
+      <video>
+        <model type='cirrus' vram='9216' heads='1'/>
+      </video>
+
+
+  </devices>
+
+</domain>

spec/unit/config_spec.rb

@@ -606,11 +606,11 @@ describe VagrantPlugins::ProviderLibvirt::Config do
         end
 
         context 'without devices given' do
-          it 'should merge disks with different devices assigned automatically' do
+          it 'should merge disks without assigning devices automatically' do
             one.storage(:file)
             two.storage(:file)
             subject.finalize!
-            expect(subject.disks).to include(include(device: 'vdb'),
+            expect(subject.disks).to_not include(include(device: 'vdb'),
                                              include(device: 'vdc'))
           end
         end

spec/unit/templates/domain_all_settings.xml

@@ -59,13 +59,13 @@
       <alias name='ua-disk-volume-0'/>
       <driver name='qemu' type='qcow2' cache='default'/>
       <source file='/var/lib/libvirt/images/test-disk1.qcow2'/>
-      <target dev='vdb' bus='virtio'/>
+      <target dev='vdc' bus='virtio'/>
     </disk>
     <disk type='file' device='disk'>
       <alias name='ua-disk-volume-1'/>
       <driver name='qemu' type='qcow2' cache='default' io='threads' copy_on_read='on' discard='unmap' detect_zeroes='on'/>
       <source file='/var/lib/libvirt/images/test-disk2.qcow2'/>
-      <target dev='vdc' bus='virtio'/>
+      <target dev='vdd' bus='virtio'/>
     </disk>
 
     <disk type='file' device='cdrom'>

spec/unit/templates/domain_spec.rb

@@ -59,13 +59,13 @@ describe 'templates/domain' do
       domain.disk_device = 'vda'
       domain.disk_driver(:cache => 'unsafe', :io => 'threads', :copy_on_read => 'on', :discard => 'unmap', :detect_zeroes => 'on')
       domain.domain_volumes.push({
-        :dev => 1.vdev.to_s,
+        :dev => 'vda',
         :cache => 'unsafe',
         :bus => domain.disk_bus,
         :path => '/var/lib/libvirt/images/test.qcow2'
       })
       domain.domain_volumes.push({
-        :dev => 2.vdev.to_s,
+        :dev => 'vdb',
         :cache => 'unsafe',
         :bus => domain.disk_bus,
         :path => '/var/lib/libvirt/images/test2.qcow2'
@@ -118,6 +118,13 @@ describe 'templates/domain' do
     let(:test_file) { 'domain_all_settings.xml' }
     it 'renders template' do
       domain.finalize!
+      # resolving is now done during create domain, so need to recreate
+      # the same behaviour before calling the template until that
+      # is separated out from create domain.
+      resolver = ::VagrantPlugins::ProviderLibvirt::Util::DiskDeviceResolver.new(prefix=domain.disk_device[0..1])
+      resolver.resolve!(domain.domain_volumes.dup.each { |volume| volume[:device] = volume[:dev] })
+      resolver.resolve!(domain.disks)
+
       expect(domain.to_xml('domain')).to eq xml_expected
     end
   end

spec/unit/util/resolvers_spec.rb

@@ -0,0 +1,116 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+require 'vagrant-libvirt/util/resolvers'
+
+describe VagrantPlugins::ProviderLibvirt::Util::DiskDeviceResolver do
+  subject { described_class.new }
+
+  def deep_clone_disks(disk_array)
+    new_array = []
+    disk_array.each do |disk|
+      new_array.push disk.dup
+    end
+
+    new_array
+  end
+
+  describe '#resolve!' do
+    context 'when using default prefix' do
+      [
+        [
+          [{:name => 'single-disk'}],
+          [{:name => 'single-disk', :device => 'vda'}],
+        ],
+        [
+          [{:name => 'disk1'}, {:name => 'disk2'}],
+          [{:name => 'disk1', :device => 'vda'}, {:name => 'disk2', :device => 'vdb'}],
+        ],
+        [
+          [{:name => 'disk1'}, {:name => 'disk2', :device => 'vdc'}],
+          [{:name => 'disk1', :device => 'vda'}, {:name => 'disk2', :device => 'vdc'}],
+        ],
+        [
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2', :device => 'vda'}],
+        ],
+      ].each do |input_disks, output_disks, options={}|
+        opts = {}.merge!(options)
+        it "should handle inputs: #{input_disks}" do
+          disks = deep_clone_disks(input_disks)
+          expect(subject.resolve!(disks)).to eq(output_disks)
+          expect(disks).to_not eq(input_disks)
+        end
+      end
+    end
+
+    context 'when using different default prefix' do
+      let(:subject) { described_class.new('sd') }
+      [
+        [
+          [{:name => 'single-disk'}],
+          [{:name => 'single-disk', :device => 'sda'}],
+        ],
+        [
+          [{:name => 'disk1'}, {:name => 'disk2'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2', :device => 'sdb'}],
+        ],
+        [
+          [{:name => 'disk1'}, {:name => 'disk2', :device => 'vdc'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2', :device => 'vdc'}],
+        ],
+        [
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2', :device => 'sdb'}],
+        ],
+        [
+          [{:name => 'disk1'}, {:name => 'disk2', :device => 'sda'}],
+          [{:name => 'disk1', :device => 'sdb'}, {:name => 'disk2', :device => 'sda'}],
+        ],
+      ].each do |input_disks, output_disks, options={}|
+        opts = {}.merge!(options)
+        it "should handle inputs: #{input_disks}" do
+          disks = deep_clone_disks(input_disks)
+          expect(subject.resolve!(disks)).to eq(output_disks)
+        end
+      end
+    end
+
+    context 'when using custom prefix' do
+      [
+        [
+          [{:name => 'existing-disk', :device => 'vda'}],
+          [{:name => 'single-disk'}],
+          [{:name => 'single-disk', :device => 'sda'}],
+          {:prefix => 'sd'},
+        ],
+        [
+          [{:name => 'existing-disk', :device => 'vda'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2'}],
+          [{:name => 'disk1', :device => 'sda'}, {:name => 'disk2', :device => 'sdb'}],
+          {:prefix => 'sd'},
+        ],
+      ].each do |existing, input_disks, output_disks, options={}|
+        opts = {}.merge!(options)
+        it "should handle inputs: #{input_disks} with opts: #{opts}" do
+          disks = deep_clone_disks(input_disks)
+          subject.resolve(existing)
+          expect(subject.resolve!(disks, opts)).to eq(output_disks)
+        end
+      end
+    end
+  end
+
+  describe '#resolve' do
+    let(:input_disks) { [{:name => 'single-disk'}] }
+    let(:output_disks) { [{:name => 'single-disk', :device => 'vda'}] }
+
+    it "should resolve without modifying" do
+      disks = deep_clone_disks(input_disks)
+      expect(subject.resolve(disks)).to eq(output_disks)
+      expect(disks).to_not eq(output_disks)
+      expect(disks).to eq(input_disks)
+    end
+  end
+end