From 724687a6012cd210fbe5c038031c14f1886c536b Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Wed, 24 Jun 2020 16:52:55 -0400 Subject: [PATCH] Fix stale controller state This was causing multiple dvds to be attached to the same port/device. --- .../virtualbox/cap/configure_disks.rb | 24 +++++---- plugins/providers/virtualbox/driver/meta.rb | 2 +- .../virtualbox/driver/version_5_0.rb | 13 +++++ templates/locales/en.yml | 7 ++- .../virtualbox/cap/configure_disks_test.rb | 52 ++++++++++++++----- .../virtualbox/driver/version_5_0_test.rb | 25 +++++++++ 6 files changed, 95 insertions(+), 28 deletions(-) diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index d354423dc..f2fec894f 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -63,7 +63,7 @@ module VagrantPlugins raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "IDE" end - if disks_defined.size > dvd_controller.limit + if dvds_defined.size > dvd_controller.limit raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit, limit: dvd_controller.limit, name: dvd_controller.name @@ -77,13 +77,14 @@ module VagrantPlugins defined_disks.each do |disk| if disk.type == :disk - disk_data = handle_configure_disk(machine, disk, current_disks, disk_controller) + disk_data = handle_configure_disk(machine, disk, current_disks, disk_controller.name) configured_disks[:disk] << disk_data unless disk_data.empty? elsif disk.type == :floppy # TODO: Write me machine.ui.info(I18n.t("vagrant.cap.configure_disks.floppy_not_supported", name: disk.name)) elsif disk.type == :dvd - dvd_data = handle_configure_dvd(machine, disk, dvd_controller) + # refresh controller state here + dvd_data = handle_configure_dvd(machine, disk, dvd_controller.name) configured_disks[:dvd] << dvd_data unless dvd_data.empty? end end @@ -124,10 +125,11 @@ module VagrantPlugins # @param [Vagrant::Machine] machine - the current machine # @param [Config::Disk] disk - the current disk to configure # @param [Array] all_disks - A list of all currently defined disks in VirtualBox - # @param [VagrantPlugins::ProviderVirtualBox::Model::StorageController] controller - - # the storage controller to use + # @param [String] controller_name - the name of the storage controller to use # @return [Hash] - disk_metadata - def self.handle_configure_disk(machine, disk, all_disks, controller) + def self.handle_configure_disk(machine, disk, all_disks, controller_name) + controller = machine.provider.driver.get_storage_controller(controller_name) + disk_metadata = {} # Grab the existing configured disk, if it exists @@ -171,10 +173,11 @@ module VagrantPlugins # # @param [Vagrant::Machine] machine - the current machine # @param [Config::Disk] dvd - the current disk to configure - # @param [VagrantPlugins::ProviderVirtualBox::Model::StorageController] controller - - # the storage controller to use + # @param [String] controller_name - the name of the storage controller to use # @return [Hash] - dvd_metadata - def self.handle_configure_dvd(machine, dvd, controller) + def self.handle_configure_dvd(machine, dvd, controller_name) + controller = machine.provider.driver.get_storage_controller(controller_name) + disk_info = get_next_port(machine, controller) port = disk_info[:port] device = disk_info[:device] @@ -182,8 +185,7 @@ module VagrantPlugins machine.provider.driver.attach_disk(controller.name, port, device, "dvddrive", dvd.file) # Refresh the controller information - storage_controllers = machine.provider.driver.read_storage_controllers - controller = storage_controllers.detect { |c| c.name == controller.name } + controller = machine.provider.driver.get_storage_controller(controller.name) attachment = controller.attachments.detect { |a| a[:port] == port && a[:device] == device } if attachment diff --git a/plugins/providers/virtualbox/driver/meta.rb b/plugins/providers/virtualbox/driver/meta.rb index c5f0104d5..54e817a17 100644 --- a/plugins/providers/virtualbox/driver/meta.rb +++ b/plugins/providers/virtualbox/driver/meta.rb @@ -116,8 +116,8 @@ module VagrantPlugins :execute_command, :export, :forward_ports, - :get_controller, :get_port_and_device, + :get_storage_controller, :halt, :import, :list_snapshots, diff --git a/plugins/providers/virtualbox/driver/version_5_0.rb b/plugins/providers/virtualbox/driver/version_5_0.rb index 1e01014c0..a37af4c49 100644 --- a/plugins/providers/virtualbox/driver/version_5_0.rb +++ b/plugins/providers/virtualbox/driver/version_5_0.rb @@ -955,6 +955,19 @@ module VagrantPlugins end end + # Gets the storage controller matching the specified name. Raises an + # error if the storage controller can't be found. + # + # @param [String] name - storage controller name + # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] + def get_storage_controller(name) + controller = read_storage_controllers.detect { |c| c.name == name } + if !controller + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, name: name + end + controller + end + protected def valid_ip_address?(ip) diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 902b54337..87f6690ff 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1679,10 +1679,9 @@ en: Vagrant. VirtualBox 4.2.16+ fixes this problem. Please upgrade VirtualBox. virtualbox_disks_controller_not_found: |- - The current disk configuration requires a controller with storage bus - '%{storage_bus}', but no such controller was found. Please add the - proper controller to your guest using provider-specific - customizations. + Vagrant was not able to find a storage controller called '%{name}'. + If you have changed or removed any storage controllers, please restore + them to their prior configuration. virtualbox_disks_defined_exceed_limit: |- VirtualBox only allows up to %{limit} disks to be attached to the storage controller '%{name}'. Please remove some disks from your disk 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 b99c4bc4f..b50356a71 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -69,6 +69,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do allow(Vagrant::Util::Experimental).to receive(:feature_enabled?).and_return(true) allow(controller).to receive(:attachments).and_return(attachments) allow(driver).to receive(:read_storage_controllers).and_return([controller]) + allow(driver).to receive(:get_storage_controller).with(controller.name).and_return(controller) end describe "#configure_disks" do @@ -80,7 +81,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end it "configures disks and returns the disks defined" do - expect(subject).to receive(:handle_configure_disk).with(machine, anything, [], controller). + expect(subject).to receive(:handle_configure_disk).with(machine, anything, [], controller.name). exactly(4).and_return(dsk_data) subject.configure_disks(machine, defined_disks) end @@ -88,7 +89,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do it "configures dvd and returns the disks defined" do defined_disks = [ dvd ] - expect(subject).to receive(:handle_configure_dvd).with(machine, dvd, controller). + expect(subject).to receive(:handle_configure_dvd).with(machine, dvd, controller.name). and_return({}) subject.configure_disks(machine, defined_disks) end @@ -113,16 +114,17 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end end + # hashicorp/bionic64 context "with more than one storage controller" do - let(:controller1) { double("controller1", storage_bus: "IDE", limit: 4) } - let(:controller2) { double("controller2", storage_bus: "SATA", limit: 30) } + let(:controller1) { double("controller1", name: "IDE Controller", storage_bus: "IDE", limit: 4) } + let(:controller2) { double("controller2", name: "SATA Controller", storage_bus: "SATA", limit: 30) } before do allow(driver).to receive(:read_storage_controllers).and_return([controller1, controller2]) end it "attaches disks to the SATA controller" do - expect(subject).to receive(:handle_configure_disk).with(machine, anything, [], controller2). + expect(subject).to receive(:handle_configure_disk).with(machine, anything, [], controller2.name). exactly(4).and_return(dsk_data) subject.configure_disks(machine, defined_disks) end @@ -130,10 +132,36 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do it "attaches dvds to the IDE controller" do defined_disks = [ dvd ] - expect(subject).to receive(:handle_configure_dvd).with(machine, dvd, controller1). + expect(subject).to receive(:handle_configure_dvd).with(machine, dvd, controller1.name). and_return({}) subject.configure_disks(machine, defined_disks) end + + it "raises an error if there are more than 4 dvds configured" do + defined_disks = [ + double("dvd", name: "installer1", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer2", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer3", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer4", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer5", type: :dvd, file: "installer.iso") + ] + + expect { subject.configure_disks(machine, defined_disks) }. + to raise_error(Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit) + end + + it "attaches multiple dvds" do + defined_disks = [ + double("dvd", name: "installer1", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer2", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer3", type: :dvd, file: "installer.iso"), + double("dvd", name: "installer4", type: :dvd, file: "installer.iso"), + ] + + expect(subject).to receive(:handle_configure_dvd).exactly(4).times.and_return({}) + + subject.configure_disks(machine, defined_disks) + end end end @@ -178,14 +206,14 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(subject).to receive(:create_disk).with(machine, defined_disks[1], controller) .and_return(disk_meta) - subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller) + subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller.name) end it "includes disk attachment info in metadata" do expect(subject).to receive(:create_disk).with(machine, defined_disks[1], controller) .and_return(disk_meta) - disk_metadata = subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller) + disk_metadata = subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller.name) expect(disk_metadata).to have_key(:controller) expect(disk_metadata).to have_key(:port) expect(disk_metadata).to have_key(:device) @@ -222,7 +250,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(subject).to receive(:resize_disk). with(machine, defined_disks[1], all_disks[1], controller).and_return({}) - subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller) + subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller.name) end end @@ -264,7 +292,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do "hdd", all_disks[1]["Location"]) - subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller) + subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller.name) end it "does nothing if all disks are properly configured" do @@ -277,7 +305,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(driver).to receive(:get_port_and_device).with("67890"). and_return(disk_info) - subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller) + subject.handle_configure_disk(machine, defined_disks[1], all_disks, controller.name) end end end @@ -469,7 +497,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do it "includes disk attachment info in metadata" do expect(driver).to receive(:attach_disk).with(controller.name, "0", "0", "dvddrive", "/tmp/untitled.iso") - dvd_metadata = subject.handle_configure_dvd(machine, dvd_config, controller) + dvd_metadata = subject.handle_configure_dvd(machine, dvd_config, controller.name) expect(dvd_metadata[:uuid]).to eq("12345") expect(dvd_metadata).to have_key(:controller) expect(dvd_metadata).to have_key(:port) 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 5a44fc423..e44ecca10 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 @@ -138,4 +138,29 @@ OUTPUT expect(storage_controllers.first.attachments).to include(port: "1", device: "0", uuid: "67890") end end + + describe "#get_storage_controller" do + let(:storage_controllers) { [double("controller", name: "IDE Controller")] } + + before do + allow(subject).to receive(:read_storage_controllers).and_return(storage_controllers) + end + + it "refreshes the storage controller state" do + expect(subject).to receive(:read_storage_controllers) + + subject.get_storage_controller("IDE Controller") + end + + it "returns the storage controller matching the specified name" do + controller = subject.get_storage_controller("IDE Controller") + + expect(controller.name).to eq("IDE Controller") + end + + it "raises an exception if the storage controller can't be found" do + expect { subject.get_storage_controller("SCSI Controller") }. + to raise_error(Vagrant::Errors::VirtualBoxDisksControllerNotFound) + end + end end