Make disks attribute path required check for name collisions (#1293)

Ensure the path attribute for disks in the new format is required to
ensure there is always one parameter required for the hash element in
the array of disks. This avoids a certain amount of magical behaviour.

Identify that the name attribute is optional, however add checks for
collisions in volume names generated, as well as ensuring the volumes
are still prefixed in a way to prevent accidental collisions between
boxes utilizing the same names.

Add notes to the README identifying the format as experimental, with
notes on how the format should appear.

README.md

@@ -63,7 +63,9 @@ can help a lot :-)
 * [Memory balloon](#memory-balloon)
 * [Libvirt communication channels](#libvirt-communication-channels)
 * [Custom command line arguments and environment variables](#custom-command-line-arguments-and-environment-variables)
-* [Box Format](#box-format)
+* [Box Formats](#box-formats)
+  * [Version 1](#version-1)
+  * [Version 2 (Experimental)](#version-2-experimental)
 * [Create Box](#create-box)
 * [Package Box from VM](#package-box-from-vm)
 * [Troubleshooting VMs](#troubleshooting-vms)
@@ -1774,7 +1776,11 @@ Vagrant.configure("2") do |config|
 end
 ```
 
-## Box Format
+## Box Formats
+
+### Version 1
+
+This is the original format that most boxes currently use.
 
 You can view an example box in the
 [`example_box/directory`](https://github.com/vagrant-libvirt/vagrant-libvirt/tree/master/example_box).
@@ -1788,6 +1794,45 @@ The box is a tarball containing:
 * `Vagrantfile` that does default settings for the provider-specific
   configuration for this provider
 
+
+### Version 2 (Experimental)
+
+Due to the limitation of only being able to handle a single disk with the version 1 format, a new
+format was added to support boxes that need to specify multiple disks. This is still currently
+experimental and as such support for packaging has yet to be added. There is a script in the tools
+folder (tools/create_box_with_two_disks.sh) that should provide a guideline on how to create such
+a box for those that wish to experiment and provide early feedback.
+
+At it's most basic, it expects an array of disks to allow a specific order to be presented. Disks
+will be attached in this order and as such assume device names base on this within the VM. The
+'path' attribute is required, and is expected to be relative to the base of the box. This should
+allow placing the disk images within a nested directory within the box if it useful for those
+with a larger number of disks. The name allows overriding the target volume name that will be
+used in the libvirt storage pool. Note that vagrant-libvirt will still prefix the volume name
+with `#{box_name}_vagrant_box_image_#{box_version}_` to avoid accidental clashes with other boxes.
+
+Format and virtual size need no longer be specified as they are now retrieved directly from the
+provided image using `qemu-img info ...`.
+
+Example format:
+```json
+{
+  'disks': [
+      {
+          'path': 'disk1.img'
+      },
+      {
+          'path': 'disk2.img',
+          'name': 'secondary_disk'
+      },
+      {
+          'path': 'disk3.img'
+      }
+  ],
+  'provider': 'libvirt'
+}
+```
+
 ## Create Box
 
 If creating a box from a modified vagrant-libvirt machine, ensure that

lib/vagrant-libvirt/action/handle_box_image.rb

@@ -30,30 +30,40 @@ module VagrantPlugins
 
             env[:box_volume_number] = 1
             env[:box_volumes] = [{
-              :path => HandleBoxImage.get_box_image_path(env, HandleBoxImage.get_default_box_image_path(0)),
+              :path => HandleBoxImage.get_box_image_path(env[:machine].box, 'box.img'),
-              :name => HandleBoxImage.get_volume_name(env, 0),
+              :name => HandleBoxImage.get_volume_name(env[:machine].box, 'box'),
               :virtual_size => HandleBoxImage.get_virtual_size(env),
               :format => box_format,
             }]
           else
             # Handle box v2 format
             # {
-            #   'path': '<path-of-file-box>', # can be inferred
+            #   'path': '<path-of-file-box>',
-            #   'name': '<name-to-use-in-storage>'
+            #   'name': '<name-to-use-in-storage>' # optional, will use index
             # }
             #
-
             env[:box_volume_number] = disks.length()
+            target_volumes = Hash[]
             env[:box_volumes] = Array.new(env[:box_volume_number]) { |i|
-              image_path = HandleBoxImage.get_box_image_path(
+              raise Errors::BoxFormatMissingAttribute, attribute: "disks[#{i}]['path']" if disks[i]['path'].nil?
-                env,
+
-                disks[i].fetch('path', HandleBoxImage.get_default_box_image_path(i))
+              image_path = HandleBoxImage.get_box_image_path(env[:machine].box, disks[i]['path'])
-              )
               format, virtual_size = HandleBoxImage.get_box_disk_settings(image_path)
+              volume_name = HandleBoxImage.get_volume_name(
+                env[:machine].box,
+                disks[i].fetch('name', disks[i]['path'].sub(/#{File.extname(disks[i]['path'])}$/, '')),
+              )
+
+              # allowing name means needing to check that it doesn't cause a clash
+              existing = target_volumes[volume_name]
+              if !existing.nil?
+                raise Errors::BoxFormatDuplicateVolume, volume: volume_name, new_disk: "disks[#{i}]", orig_disk: "disks[#{existing}]"
+              end
+              target_volumes[volume_name] = i
 
               {
                 :path => image_path,
-                :name => disks[i].fetch('name', HandleBoxImage.get_volume_name(env, i)),
+                :name => volume_name,
                 :virtual_size => virtual_size.to_i,
                 :format => HandleBoxImage.verify_box_format(format)
               }
@@ -105,15 +115,15 @@ module VagrantPlugins
 
         protected
 
-        def self.get_volume_name(env, index)
+        def self.get_volume_name(box, name)
-          name = env[:machine].box.name.to_s.dup.gsub('/', '-VAGRANTSLASH-')
+          vol_name = box.name.to_s.dup.gsub('/', '-VAGRANTSLASH-')
-          name << "_vagrant_box_image_#{
+          vol_name << "_vagrant_box_image_#{
             begin
-            env[:machine].box.version.to_s
+              box.version.to_s
             rescue
               ''
-          end}_#{index}.img"
+            end
-          return name
+          }_#{name.dup.gsub('/', '-SLASH-')}.img"
         end
 
         def self.get_virtual_size(env)
@@ -123,12 +133,8 @@ module VagrantPlugins
           return box_virtual_size
         end
 
-        def self.get_default_box_image_path(index)
+        def self.get_box_image_path(box, box_name)
-          return index <= 0 ? 'box.img' : "box_#{index}.img"
+          return box.directory.join(box_name).to_s
-        end
-
-        def self.get_box_image_path(env, box_name)
-          return env[:machine].box.directory.join(box_name).to_s
         end
 
         def self.verify_box_format(box_format, disk_index=nil)

lib/vagrant-libvirt/errors.rb

@@ -37,7 +37,18 @@ module VagrantPlugins
         error_key(:image_download_error)
       end
 
-      # Box exceptions
+      # Box exceptions, capture all under one
+      class BoxError < VagrantLibvirtError
+      end
+
+      class BoxFormatMissingAttribute < BoxError
+        error_key(:box_format_missing_attribute)
+      end
+
+      class BoxFormatDuplicateVolume < BoxError
+        error_key(:box_format_duplicate_volume)
+      end
+
       class BadBoxImage < VagrantLibvirtError
         error_key(:bad_box_image)
       end

locales/en.yml

@@ -72,6 +72,10 @@ en:
       no_storage_pool: |-
         No usable storage pool found! Please check if storage pool is
         created and available.
+      box_format_duplicate_volume: |-
+        Encountered a duplicate volume name '%{volume}' generated for disk '%{new_disk}', due to already allocated for disk '%{orig_disk}'.
+      box_format_missing_attribute: |-
+        Invalid box metadata, missing expected attribute: '%{attribute}'
       bad_box_image: |-
         Received error when query the box image details from '%{image}'.
         Stdout: %{out}

spec/unit/action/handle_box_image_spec.rb

@@ -50,7 +50,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
           [
             {
               :path=>"/test/box.img",
-              :name=>"test_vagrant_box_image_1.1.1_0.img", 
+              :name=>"test_vagrant_box_image_1.1.1_box.img",
               :virtual_size=>5,
               :format=>"qcow2"
             }
@@ -69,10 +69,10 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
 
         it 'should upload disk' do
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_0.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_box.img in storage pool default.')
           expect(volumes).to receive(:create).with(
             hash_including(
-              :name => "test_vagrant_box_image_1.1.1_0.img",
+              :name => "test_vagrant_box_image_1.1.1_box.img",
               :allocation => "5120M",
               :capacity => "5G",
             )
@@ -107,12 +107,15 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
         allow(env[:machine]).to receive_message_chain("box.metadata") { Hash[
           'disks' => [
             {
-              'name'=>'send_box_name.img',
+              'path' => 'box.img',
+              'name' => 'send_box_name',
             },
             {
               'path' => 'disk.qcow2',
             },
-            { },
+            {
+              'path' => 'box_2.img',
+            },
           ],
         ]}
         allow(env[:machine]).to receive_message_chain("box.directory.join") do |arg|
@@ -137,19 +140,19 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
           [
             {
               :path=>"/test/box.img",
-              :name=>"send_box_name.img",
+              :name=>"test_vagrant_box_image_1.1.1_send_box_name.img",
               :virtual_size=>5,
               :format=>"qcow2"
             },
             {
               :path=>"/test/disk.qcow2",
-              :name=>"test_vagrant_box_image_1.1.1_1.img", 
+              :name=>"test_vagrant_box_image_1.1.1_disk.img",
               :virtual_size=>10,
               :format=>"qcow2"
             },
             {
               :path=>"/test/box_2.img",
-              :name=>"test_vagrant_box_image_1.1.1_2.img", 
+              :name=>"test_vagrant_box_image_1.1.1_box_2.img",
               :virtual_size=>20,
               :format=>"qcow2"
             }
@@ -168,30 +171,30 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
 
         it 'should upload all 3 disks' do
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume send_box_name.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_send_box_name.img in storage pool default.')
           expect(volumes).to receive(:create).with(
             hash_including(
-              :name => "send_box_name.img",
+              :name => "test_vagrant_box_image_1.1.1_send_box_name.img",
               :allocation => "5120M",
               :capacity => "5G",
             )
           )
           expect(subject).to receive(:upload_image)
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_1.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_disk.img in storage pool default.')
           expect(volumes).to receive(:create).with(
             hash_including(
-              :name => "test_vagrant_box_image_1.1.1_1.img",
+              :name => "test_vagrant_box_image_1.1.1_disk.img",
               :allocation => "10240M",
               :capacity => "10G",
             )
           )
           expect(subject).to receive(:upload_image)
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_2.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_box_2.img in storage pool default.')
           expect(volumes).to receive(:create).with(
             hash_including(
-              :name => "test_vagrant_box_image_1.1.1_2.img",
+              :name => "test_vagrant_box_image_1.1.1_box_2.img",
               :allocation => "20480M",
               :capacity => "20G",
             )
@@ -214,12 +217,12 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
 
         it 'upload disks 1 and 2 only' do
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_1.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_disk.img in storage pool default.')
-          expect(volumes).to receive(:create).with(hash_including(:name => "test_vagrant_box_image_1.1.1_1.img"))
+          expect(volumes).to receive(:create).with(hash_including(:name => "test_vagrant_box_image_1.1.1_disk.img"))
           expect(subject).to receive(:upload_image)
           expect(ui).to receive(:info).with('Uploading base box image as volume into Libvirt storage...')
-          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_2.img in storage pool default.')
+          expect(logger).to receive(:info).with('Creating volume test_vagrant_box_image_1.1.1_box_2.img in storage pool default.')
-          expect(volumes).to receive(:create).with(hash_including(:name => "test_vagrant_box_image_1.1.1_2.img"))
+          expect(volumes).to receive(:create).with(hash_including(:name => "test_vagrant_box_image_1.1.1_box_2.img"))
           expect(subject).to receive(:upload_image)
 
           expect(subject.call(env)).to be_nil
@@ -263,7 +266,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
 
     end
 
-    context 'when one of a multi disk definition has wrong disk format in metadata.json' do
+    context 'when invalid format in metadata.json' do
       let(:status) { double }
 
       before do
@@ -271,20 +274,7 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
         allow(box_volume).to receive(:id).and_return(1)
         allow(env[:machine]).to receive_message_chain("box.name") { 'test' }
         allow(env[:machine]).to receive_message_chain("box.version") { '1.1.1' }
-        allow(env[:machine]).to receive_message_chain("box.metadata") {
+        allow(env[:machine]).to receive_message_chain("box.metadata") { box_metadata }
-          Hash[
-            'disks' => [
-              {
-                'name'=>'send_box_name.img',
-                'format'=> 'wrongFormat'
-              },
-              {
-                'path' => 'disk.qcow2',
-              },
-              { },
-            ],
-          ]
-        }
         allow(env[:machine]).to receive_message_chain("box.directory.join") do |arg|
           '/test/'.concat(arg.to_s)
         end
@@ -300,10 +290,74 @@ describe VagrantPlugins::ProviderLibvirt::Action::HandleBoxImage do
         ])
       end
 
+      context 'with one disk having wrong disk format' do
+        let(:box_metadata) {
+          Hash[
+            'disks' => [
+              {
+                'path'   => 'box.img',
+                'name'   =>'send_box_name.img',
+                'format' => 'wrongFormat'
+              },
+              {
+                'path' => 'disk.qcow2',
+              },
+              {
+                'path' => 'box_2.img',
+              },
+            ],
+          ]
+        }
+
         it 'should be ignored' do
           expect(subject.call(env)).to be_nil
         end
       end
 
+      context 'with one disk missing path' do
+        let(:box_metadata) {
+          Hash[
+            'disks' => [
+              {
+                'path' => 'box.img',
+              },
+              {
+                'name' => 'send_box_name',
+              },
+              {
+                'path' => 'box_2.img',
+              },
+            ],
+          ]
+        }
+
+        it 'should raise an exception' do
+          expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::BoxFormatMissingAttribute, /: 'disks\[1\]\['path'\]'/)
+        end
+      end
+
+      context 'with one disk name duplicating a path of another' do
+        let(:box_metadata) {
+          Hash[
+            'disks' => [
+              {
+                'path' => 'box.img',
+                'name' => 'box_2',
+              },
+              {
+                'path' => 'disk.qcow2',
+              },
+              {
+                'path' => 'box_2.img',
+              },
+            ],
+          ]
+        }
+
+        it 'should raise an exception' do
+          expect{ subject.call(env) }.to raise_error(VagrantPlugins::ProviderLibvirt::Errors::BoxFormatDuplicateVolume, /test_vagrant_box_image_1.1.1_box_2.img.*'disks\[2\]'.*'disks\[0\]'/)
+        end
+      end
+    end
   end
 end