From 958023dbb9cf458afbfc2d6619f9e6aa27b8aa4b Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Tue, 2 Jun 2020 14:04:09 -0400 Subject: [PATCH] Create #get_controller method in driver This makes it easier to check if the required controller can be found, and automatically raise an error if it is not. Add a #storage_bus method to the StorageController class as a shorthand way to check the general storage controller type. --- .../virtualbox/cap/configure_disks.rb | 22 +++++------- .../virtualbox/driver/version_5_0.rb | 18 +++++++++- .../virtualbox/model/storage_controller.rb | 23 +++++++----- templates/locales/en.yml | 7 ++-- .../virtualbox/cap/configure_disks_test.rb | 35 +++++++------------ .../models/storage_controller_test.rb | 28 ++++++++++----- 6 files changed, 77 insertions(+), 56 deletions(-) diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index a225ec092..1846a25ef 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -19,20 +19,16 @@ module VagrantPlugins disks_defined = defined_disks.select { |d| d.type == :disk } if disks_defined.any? - controller = machine.provider.driver.storage_controllers.detect { |c| c.sata_controller? } - if controller.nil? - raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, disk_type: ':disk', controller_type: 'SATA' - elsif disks_defined.size > controller.maxportcount + controller = machine.provider.driver.get_controller('SATA') + if disks_defined.size > controller.maxportcount raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit end end dvds_defined = defined_disks.select { |d| d.type == :dvd } if dvds_defined.any? - controller = machine.provider.driver.storage_controllers.detect { |c| c.ide_controller? } - if controller.nil? - raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, disk_type: ':dvd', controller_type: 'IDE' - elsif dvds_defined.size > controller.maxportcount + controller = machine.provider.driver.get_controller('IDE') + if disks_defined.size > controller.maxportcount raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit end end @@ -71,7 +67,7 @@ module VagrantPlugins # Ensure we grab the proper primary disk # We can't rely on the order of `all_disks`, as they will not # always come in port order, but primary is always Port 0 Device 0. - controller = machine.provider.driver.storage_controllers.detect { |c| c.sata_controller? } + controller = machine.provider.driver.get_controller('SATA') primary = controller.attachments.detect { |a| a[:port] == "0" && a[:device] == "0" } primary_uuid = primary[:uuid] @@ -107,7 +103,7 @@ module VagrantPlugins disk_info = machine.provider.driver.get_port_and_device(current_disk["UUID"]) if disk_info.empty? LOGGER.warn("Disk '#{disk.name}' is not connected to guest '#{machine.name}', Vagrant will attempt to connect disk to guest") - controller = machine.provider.driver.storage_controllers.detect { |c| c.sata_controller? } + controller = machine.provider.driver.get_controller('SATA') dsk_info = get_next_port(machine, controller.name) machine.provider.driver.attach_disk(dsk_info[:port], dsk_info[:device], @@ -128,13 +124,13 @@ module VagrantPlugins # @param [Config::Disk] dvd - the current disk to configure # @return [Hash] - dvd_metadata def self.handle_configure_dvd(machine, dvd) - controller = machine.provider.driver.storage_controllers.detect { |c| c.ide_controller? } + controller = machine.provider.driver.get_controller('IDE') disk_info = get_next_port(machine, controller.name) machine.provider.driver.attach_disk(disk_info[:port], disk_info[:device], dvd.file, "dvddrive") # Refresh the controller information - controller = machine.provider.driver.storage_controllers.detect { |c| c.ide_controller? } + controller = machine.provider.driver.get_controller('IDE') attachment = controller.attachments.detect { |a| a[:port] == disk_info[:port] && a[:device] == disk_info[:device] } @@ -211,7 +207,7 @@ module VagrantPlugins # TODO: definitely need an error for this dsk_info = {} - if controller.sata_controller? + if controller.storage_bus == 'SATA' used_ports = controller.attachments.map { |a| a[:port].to_i } next_available_port = ((0..(controller.maxportcount-1)).to_a - used_ports).first diff --git a/plugins/providers/virtualbox/driver/version_5_0.rb b/plugins/providers/virtualbox/driver/version_5_0.rb index 9030b893d..e2a753b4c 100644 --- a/plugins/providers/virtualbox/driver/version_5_0.rb +++ b/plugins/providers/virtualbox/driver/version_5_0.rb @@ -937,7 +937,7 @@ module VagrantPlugins # Helper method to get a list of storage controllers added to the # current VM # - # @return [Array] + # @return [Array] def storage_controllers vm_info = show_vm_info count = vm_info.count { |key, value| key.match(/^storagecontrollername/) } @@ -960,6 +960,22 @@ module VagrantPlugins end end + + # Get the controller that uses the specified storage bus. + # + # A VirtualBoxDisksControllerNotFound error is raised if a compatible + # storage controller cannot be found. + # + # @param [String] storage_bus - for example, 'IDE' or 'SATA' + # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] controller + def get_controller(storage_bus) + controller = storage_controllers.detect { |c| c.storage_bus == storage_bus } + if controller.nil? + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: storage_bus + end + controller + end + protected def valid_ip_address?(ip) diff --git a/plugins/providers/virtualbox/model/storage_controller.rb b/plugins/providers/virtualbox/model/storage_controller.rb index 7343ec9de..600f174eb 100644 --- a/plugins/providers/virtualbox/model/storage_controller.rb +++ b/plugins/providers/virtualbox/model/storage_controller.rb @@ -18,6 +18,12 @@ module VagrantPlugins # @return [String] attr_reader :type + # The storage bus associated with the storage controller, which can be + # inferred from its specific type. + # + # @return [String] + attr_reader :storage_bus + # The maximum number of avilable ports for the storage controller. For # SATA controllers, this indicates the number of disks that can be # attached. For IDE controllers, this indicates that n*2 disks can be @@ -34,19 +40,20 @@ module VagrantPlugins def initialize(name, type, maxportcount, attachments) @name = name @type = type + + if SATA_CONTROLLER_TYPES.include?(@type) + @storage_bus = 'SATA' + elsif IDE_CONTROLLER_TYPES.include?(@type) + @storage_bus = 'IDE' + else + @storage_bus = 'Unknown' + end + @maxportcount = maxportcount.to_i attachments ||= [] @attachments = attachments end - - def sata_controller? - SATA_CONTROLLER_TYPES.include?(@type) - end - - def ide_controller? - IDE_CONTROLLER_TYPES.include?(@type) - end end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index d23c2935f..0d882aae7 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1679,9 +1679,10 @@ en: Vagrant. VirtualBox 4.2.16+ fixes this problem. Please upgrade VirtualBox. virtualbox_disks_controller_not_found: |- - The current disk configuration requires a controller of type '%{controller_type}'. - Please add the proper controller to your guest using provider-specific - customizations, or remove the '%{disk_type}' disk configurations. + 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. virtualbox_disks_defined_exceed_limit: |- VirtualBox only allows up to 30 disks to be attached to a single guest using the SATA Controller, including the primray 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 6ea05d8b3..68ca5be4e 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -31,17 +31,8 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do "SATA Controller-ImageUUID-0-0" => "12345", "SATA Controller-ImageUUID-1-0" => "67890"} } - let(:sata_controller) { double("sata_controller", - name: "SATA Controller", - maxportcount: 30, - sata_controller?: true, - ide_controller?: false) } - - let(:ide_controller) { double("ide_controller", - name: "IDE Controller", - maxportcount: 2, - ide_controller?: true, - sata_controller?: false) } + let(:sata_controller) { double("controller", name: "SATA Controller", storage_bus: "SATA", maxportcount: 30) } + let(:ide_controller) { double("controller", name: "IDE Controller", storage_bus: "IDE", maxportcount: 2) } let(:attachments) { [{port: "0", device: "0", uuid: "12345"}, {port: "1", device: "0", uuid: "67890"}]} @@ -84,8 +75,12 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do before do allow(Vagrant::Util::Experimental).to receive(:feature_enabled?).and_return(true) allow(driver).to receive(:show_vm_info).and_return(vm_info) + allow(sata_controller).to receive(:attachments).and_return(attachments) - allow(driver).to receive(:storage_controllers).and_return([sata_controller]) + + allow(driver).to receive(:get_controller).with("IDE").and_return(ide_controller) + allow(driver).to receive(:get_controller).with("SATA").and_return(sata_controller) + allow(driver).to receive(:storage_controllers).and_return([ide_controller, sata_controller]) end describe "#configure_disks" do @@ -113,9 +108,10 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end end - context "missing SATA controller" do + context "no SATA controller" do before do - allow(driver).to receive(:storage_controllers).and_return([]) + allow(driver).to receive(:get_controller).with("SATA"). + and_raise(Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "SATA") end it "raises an error" do @@ -125,10 +121,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end context "with dvd type" do - before do - allow(driver).to receive(:storage_controllers).and_return([ide_controller]) - end - let(:defined_disks) { [double("dvd", type: :dvd)] } let(:dvd_data) { {uuid: "1234", name: "dvd"} } @@ -138,9 +130,10 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do subject.configure_disks(machine, defined_disks) end - context "missing IDE controller" do + context "no IDE controller" do before do - allow(driver).to receive(:storage_controllers).and_return([]) + allow(driver).to receive(:get_controller).with("IDE"). + and_raise(Vagrant::Errors::VirtualBoxDisksControllerNotFound, storage_bus: "IDE") end it "raises an error" do @@ -332,7 +325,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do before do allow(ide_controller).to receive(:attachments).and_return(attachments) - allow(driver).to receive(:storage_controllers).and_return([ide_controller]) end it "determines the next available port and device to use" do @@ -456,7 +448,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do allow(ide_controller).to receive(:attachments).and_return( [port: "0", device: "0", uuid: "12345"] ) - allow(driver).to receive(:storage_controllers).and_return([ide_controller]) end it "returns the UUID of the newly-attached dvd" do diff --git a/test/unit/plugins/providers/virtualbox/models/storage_controller_test.rb b/test/unit/plugins/providers/virtualbox/models/storage_controller_test.rb index 897722352..015fb2c71 100644 --- a/test/unit/plugins/providers/virtualbox/models/storage_controller_test.rb +++ b/test/unit/plugins/providers/virtualbox/models/storage_controller_test.rb @@ -10,19 +10,29 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do subject { described_class.new(name, type, maxportcount, attachments) } - describe "#sata_controller?" do - let(:type) { "IntelAhci" } + describe "#initialize" do + context "with SATA controller type" do + let(:type) { "IntelAhci" } - it "is true for a SATA type" do - expect(subject.sata_controller?).to be(true) + it "is recognizes a SATA controller" do + expect(subject.storage_bus).to eq('SATA') + end end - end - describe "#ide_controller?" do - let(:type) { "PIIX4" } + context "with IDE controller type" do + let(:type) { "PIIX4" } - it "is true for an IDE type" do - expect(subject.ide_controller?).to be(true) + it "recognizes an IDE controller" do + expect(subject.storage_bus).to eq('IDE') + end + end + + context "with some other type" do + let(:type) { "foo" } + + it "is unknown" do + expect(subject.storage_bus).to eq('Unknown') + end end end end