From 84d2c38cc275de57852c0315f1b6320c7702cd2a Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Thu, 25 Jun 2020 11:08:51 -0400 Subject: [PATCH] Disallow multiple copies of the same ISO This makes disk/dvd behavior more consistent, and makes it easier to determine whether a dvd is already attached. --- plugins/kernel_v2/config/vm.rb | 4 +- .../virtualbox/cap/configure_disks.rb | 44 +++++++++++++------ .../virtualbox/driver/version_5_0.rb | 7 ++- test/unit/plugins/kernel_v2/config/vm_test.rb | 8 ---- .../virtualbox/cap/configure_disks_test.rb | 2 + .../virtualbox/driver/version_5_0_test.rb | 6 ++- 6 files changed, 45 insertions(+), 26 deletions(-) diff --git a/plugins/kernel_v2/config/vm.rb b/plugins/kernel_v2/config/vm.rb index cd9e2e4d9..2f5a46fd8 100644 --- a/plugins/kernel_v2/config/vm.rb +++ b/plugins/kernel_v2/config/vm.rb @@ -900,7 +900,7 @@ module VagrantPlugins end # Validate disks - # Check if there is more than one primrary disk defined and throw an error + # Check if there is more than one primary disk defined and throw an error primary_disks = @disks.select { |d| d.primary && d.type == :disk } if primary_disks.size > 1 errors << I18n.t("vagrant.config.vm.multiple_primary_disks_error", @@ -914,7 +914,7 @@ module VagrantPlugins name: duplicate_names) end - disk_files = @disks.select { |d| d.type == :disk }.map { |d| d.file } + disk_files = @disks.map { |d| d.file } duplicate_files = disk_files.detect { |d| disk_files.count(d) > 1 } if duplicate_files && duplicate_files.size errors << I18n.t("vagrant.config.vm.multiple_disk_files_error", diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index f2fec894f..f6ae4b0f5 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -83,7 +83,6 @@ module VagrantPlugins # TODO: Write me machine.ui.info(I18n.t("vagrant.cap.configure_disks.floppy_not_supported", name: disk.name)) elsif disk.type == :dvd - # refresh controller state here dvd_data = handle_configure_dvd(machine, disk, dvd_controller.name) configured_disks[:dvd] << dvd_data unless dvd_data.empty? end @@ -176,23 +175,42 @@ module VagrantPlugins # @param [String] controller_name - the name of the storage controller to use # @return [Hash] - dvd_metadata def self.handle_configure_dvd(machine, dvd, controller_name) + dvd_metadata = {} + controller = machine.provider.driver.get_storage_controller(controller_name) - disk_info = get_next_port(machine, controller) - port = disk_info[:port] - device = disk_info[:device] + dvd_location = File.expand_path(dvd.file) + dvd_attached = controller.attachments.detect { |a| a[:location] == dvd_location } - machine.provider.driver.attach_disk(controller.name, port, device, "dvddrive", dvd.file) - - # Refresh the controller information - controller = machine.provider.driver.get_storage_controller(controller.name) - attachment = controller.attachments.detect { |a| a[:port] == port && a[:device] == device } - - if attachment - { uuid: attachment[:uuid], name: dvd.name, controller: controller.name, port: port, device: device } + if dvd_attached + LOGGER.info("No further configuration required for dvd '#{dvd.name}'") + dvd_metadata[:name] = dvd.name + dvd_metadata[:port] = dvd_attached[:port] + dvd_metadata[:device] = dvd_attached[:device] + dvd_metadata[:uuid] = dvd_attached[:uuid] + dvd_metadata[:controller] = controller.name else - {} + LOGGER.warn("DVD '#{dvd.name}' is not connected to guest '#{machine.name}', Vagrant will attempt to connect dvd to guest") + dsk_info = get_next_port(machine, controller) + machine.provider.driver.attach_disk(controller.name, + dsk_info[:port], + dsk_info[:device], + "dvddrive", + dvd.file) + + # Refresh the controller information + controller = machine.provider.driver.get_storage_controller(controller.name) + attachment = controller.attachments.detect { |a| a[:port] == dsk_info[:port] && + a[:device] == dsk_info[:device] } + + dvd_metadata[:name] = dvd.name + dvd_metadata[:port] = dsk_info[:port] + dvd_metadata[:device] = dsk_info[:device] + dvd_metadata[:uuid] = attachment[:uuid] + dvd_metadata[:controller] = controller.name end + + dvd_metadata end # Check to see if current disk is configured based on defined_disks diff --git a/plugins/providers/virtualbox/driver/version_5_0.rb b/plugins/providers/virtualbox/driver/version_5_0.rb index a37af4c49..ec075329c 100644 --- a/plugins/providers/virtualbox/driver/version_5_0.rb +++ b/plugins/providers/virtualbox/driver/version_5_0.rb @@ -947,7 +947,12 @@ module VagrantPlugins attachments = [] vm_info.each do |k, v| if /^#{name}-ImageUUID-(\d+)-(\d+)$/ =~ k - attachments << {port: $1.to_s, device: $2.to_s, uuid: v} + port = $1.to_s + device = $2.to_s + uuid = v + location = vm_info["#{name}-#{port}-#{device}"] + + attachments << {port: port, device: device, uuid: uuid, location: location} end end diff --git a/test/unit/plugins/kernel_v2/config/vm_test.rb b/test/unit/plugins/kernel_v2/config/vm_test.rb index 952fc2d61..cf41addcb 100644 --- a/test/unit/plugins/kernel_v2/config/vm_test.rb +++ b/test/unit/plugins/kernel_v2/config/vm_test.rb @@ -630,14 +630,6 @@ describe VagrantPlugins::Kernel_V2::VMConfig do assert_invalid end - it "does not raise an error with duplicate dvd files" do - allow(File).to receive(:file?).with("bar.iso").and_return(true) - subject.disk(:dvd, name: "foo1", file: "bar.iso") - subject.disk(:dvd, name: "foo2", file: "bar.iso") - subject.finalize! - assert_valid - end - it "does not merge duplicate disks" do subject.disk(:disk, size: 1000, primary: false, name: "storage") subject.disk(:disk, size: 1000, primary: false, name: "backup") diff --git a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb index b50356a71..f20e44da0 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -217,6 +217,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(disk_metadata).to have_key(:controller) expect(disk_metadata).to have_key(:port) expect(disk_metadata).to have_key(:device) + expect(disk_metadata).to have_key(:name) end end @@ -502,6 +503,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(dvd_metadata).to have_key(:controller) expect(dvd_metadata).to have_key(:port) expect(dvd_metadata).to have_key(:device) + expect(dvd_metadata).to have_key(:name) end end end diff --git a/test/unit/plugins/providers/virtualbox/driver/version_5_0_test.rb b/test/unit/plugins/providers/virtualbox/driver/version_5_0_test.rb index e44ecca10..573e08f1e 100644 --- a/test/unit/plugins/providers/virtualbox/driver/version_5_0_test.rb +++ b/test/unit/plugins/providers/virtualbox/driver/version_5_0_test.rb @@ -118,7 +118,9 @@ OUTPUT { "storagecontrollername0" => "SATA Controller", "storagecontrollertype0" => "IntelAhci", "storagecontrollermaxportcount0" => "30", + "SATA Controller-0-0" => "/tmp/primary.vdi", "SATA Controller-ImageUUID-0-0" => "12345", + "SATA Controller-1-0" => "/tmp/secondary.vdi", "SATA Controller-ImageUUID-1-0" => "67890" } ) end @@ -134,8 +136,8 @@ OUTPUT it "includes attachments for each storage controller" do storage_controllers = subject.read_storage_controllers - expect(storage_controllers.first.attachments).to include(port: "0", device: "0", uuid: "12345") - expect(storage_controllers.first.attachments).to include(port: "1", device: "0", uuid: "67890") + expect(storage_controllers.first.attachments).to include(port: "0", device: "0", uuid: "12345", location: "/tmp/primary.vdi") + expect(storage_controllers.first.attachments).to include(port: "1", device: "0", uuid: "67890", location: "/tmp/secondary.vdi") end end