diff --git a/plugins/providers/virtualbox/cap/cleanup_disks.rb b/plugins/providers/virtualbox/cap/cleanup_disks.rb index aff49a54f..b01a2b5ba 100644 --- a/plugins/providers/virtualbox/cap/cleanup_disks.rb +++ b/plugins/providers/virtualbox/cap/cleanup_disks.rb @@ -40,7 +40,7 @@ module VagrantPlugins LOGGER.warn("Found disk not in Vagrantfile config: '#{d["name"]}'. Removing disk from guest #{machine.name}") machine.ui.warn(I18n.t("vagrant.cap.cleanup_disks.disk_cleanup", name: d["name"]), prefix: true) - controller = storage_controllers.get_controller!(name: d["controller"]) + controller = storage_controllers.get_controller(d["controller"]) attachment = controller.get_attachment(uuid: d["uuid"]) if !attachment @@ -68,7 +68,7 @@ module VagrantPlugins machine.ui.warn("DVD '#{d["name"]}' no longer exists in Vagrant config. Removing medium from guest...", prefix: true) storage_controllers = machine.provider.driver.read_storage_controllers - controller = storage_controllers.get_controller!(name: d["controller"]) + controller = storage_controllers.get_controller(d["controller"]) attachment = controller.get_attachment(uuid: d["uuid"]) if !attachment diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index 575c0d54a..aac4a3824 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -115,7 +115,7 @@ module VagrantPlugins # @return [Hash] - disk_metadata def self.handle_configure_disk(machine, disk, all_disks, controller_name) storage_controllers = machine.provider.driver.read_storage_controllers - controller = storage_controllers.get_controller!(name: controller_name) + controller = storage_controllers.get_controller(controller_name) disk_metadata = {} @@ -164,7 +164,7 @@ module VagrantPlugins # @return [Hash] - dvd_metadata def self.handle_configure_dvd(machine, dvd, controller_name) storage_controllers = machine.provider.driver.read_storage_controllers - controller = storage_controllers.get_controller!(name: controller_name) + controller = storage_controllers.get_controller(controller_name) dvd_metadata = {} @@ -189,7 +189,7 @@ module VagrantPlugins # Refresh the controller information storage_controllers = machine.provider.driver.read_storage_controllers - controller = storage_controllers.get_controller!(name: controller_name) + controller = storage_controllers.get_controller(controller_name) attachment = controller.attachments.detect { |a| a[:port] == dsk_info[:port] && a[:device] == dsk_info[:device] } @@ -277,19 +277,19 @@ module VagrantPlugins def self.get_next_port(machine, controller) dsk_info = {} - if controller.storage_bus == "SATA" + if controller.sata? used_ports = controller.attachments.map { |a| a[:port].to_i } next_available_port = ((0..(controller.maxportcount - 1)).to_a - used_ports).first dsk_info[:port] = next_available_port.to_s dsk_info[:device] = "0" - elsif controller.storage_bus == "IDE" + elsif controller.ide? # IDE Controllers have primary/secondary devices, so find the first port # with an empty device (0..(controller.maxportcount - 1)).each do |port| # Skip this port if it's full port_attachments = controller.attachments.select { |a| a[:port] == port.to_s } - next if port_attachments.count == 2 + next if port_attachments.count == controller.devices_per_port dsk_info[:port] = port.to_s diff --git a/plugins/providers/virtualbox/model/storage_controller.rb b/plugins/providers/virtualbox/model/storage_controller.rb index 777caa0c5..e378ea081 100644 --- a/plugins/providers/virtualbox/model/storage_controller.rb +++ b/plugins/providers/virtualbox/model/storage_controller.rb @@ -7,6 +7,9 @@ module VagrantPlugins SATA_CONTROLLER_TYPES = ["IntelAhci"].map(&:freeze).freeze IDE_CONTROLLER_TYPES = ["PIIX4", "PIIX3", "ICH6"].map(&:freeze).freeze + SATA_DEVICES_PER_PORT = 1.freeze + IDE_DEVICES_PER_PORT = 2.freeze + # The name of the storage controller. # # @return [String] @@ -17,17 +20,17 @@ 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. # # @return [Integer] attr_reader :maxportcount + # The number of devices that can be attached to each port. For SATA + # controllers, this will usually be 1, and for IDE controllers this + # will usually be 2. + # @return [Integer] + attr_reader :devices_per_port + # The maximum number of individual disks that can be attached to the # storage controller. For SATA controllers, this equals the maximum # number of ports. For IDE controllers, this will be twice the max @@ -45,20 +48,20 @@ module VagrantPlugins @name = name @type = type - if SATA_CONTROLLER_TYPES.include?(@type) - @storage_bus = "SATA" - elsif IDE_CONTROLLER_TYPES.include?(@type) - @storage_bus = "IDE" + @maxportcount = maxportcount.to_i + + if IDE_CONTROLLER_TYPES.include?(@type) + @storage_bus = :ide + @devices_per_port = IDE_DEVICES_PER_PORT + elsif SATA_CONTROLLER_TYPES.include?(@type) + @storage_bus = :sata + @devices_per_port = SATA_DEVICES_PER_PORT else - @storage_bus = "Unknown" + @storage_bus = :unknown + @devices_per_port = 1 end - @maxportcount = maxportcount.to_i - if @storage_bus == "IDE" - @limit = @maxportcount * 2 - else - @limit = @maxportcount - end + @limit = @maxportcount * @devices_per_port attachments ||= [] @attachments = attachments @@ -77,6 +80,20 @@ module VagrantPlugins @attachments.detect { |a| a[:uuid] == opts[:uuid] } end end + + # Returns true if the storage controller is a IDE type controller. + # + # @return [Boolean] + def ide? + @storage_bus == :ide + end + + # Returns true if the storage controller is a SATA type controller. + # + # @return [Boolean] + def sata? + @storage_bus == :sata + end end end end diff --git a/plugins/providers/virtualbox/model/storage_controller_array.rb b/plugins/providers/virtualbox/model/storage_controller_array.rb index bcc93978e..63a8eabd1 100644 --- a/plugins/providers/virtualbox/model/storage_controller_array.rb +++ b/plugins/providers/virtualbox/model/storage_controller_array.rb @@ -4,34 +4,18 @@ module VagrantPlugins # A collection of storage controllers. Includes finder methods to look # up a storage controller by given attributes. class StorageControllerArray < Array - SATA_TYPE = "SATA".freeze - IDE_TYPE = "IDE".freeze - SUPPORTED_TYPES = [SATA_TYPE, IDE_TYPE].freeze - # TODO: hook into ValidateDiskExt capability DEFAULT_DISK_EXT = [".vdi", ".vmdk", ".vhd"].map(&:freeze).freeze - # Get a single controller matching the given options. - # - # @param [Hash] opts - A hash of attributes to match. - # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] - def get_controller(opts = {}) - if opts[:name] - detect { |c| c.name == opts[:name] } - elsif opts[:storage_bus] - detect { |c| c.storage_bus == opts[:storage_bus] } - end - end - - # Get a single controller matching the given options. Raise an + # Returns a storage controller with the given name. Raises an # exception if a matching controller can't be found. # - # @param [Hash] opts - A hash of attributes to match. + # @param [String] name - The name of the storage controller # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] - def get_controller!(opts = {}) - controller = get_controller(opts) - if !controller && opts[:name] - raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, name: opts[:name] + def get_controller(name) + controller = detect { |c| c.name == name } + if !controller + raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, name: name end controller end @@ -46,15 +30,17 @@ module VagrantPlugins def get_primary_controller controller = nil - if !types.any? { |t| SUPPORTED_TYPES.include?(t) } - raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, supported_types: SUPPORTED_TYPES - end - - ide_controller = get_controller(storage_bus: IDE_TYPE) + ide_controller = detect { |c| c.ide? } if ide_controller && ide_controller.attachments.any? { |a| hdd?(a) } controller = ide_controller else - controller = get_controller(storage_bus: SATA_TYPE) + controller = detect { |c| c.sata? } + end + + if !controller + supported_types = StorageController::SATA_CONTROLLER_TYPES + StorageController::IDE_CONTROLLER_TYPES + raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, + supported_types: supported_types.join(" ,") end controller @@ -82,14 +68,12 @@ module VagrantPlugins # # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] def get_dvd_controller - controller = nil + controller = detect { |c| c.ide? } || detect { |c| c.sata? } - if types.include?(IDE_TYPE) - controller = get_controller(storage_bus: IDE_TYPE) - elsif types.include?(SATA_TYPE) - controller = get_controller(storage_bus: SATA_TYPE) - else - raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, supported_types: SUPPORTED_TYPES + if !controller + supported_types = StorageController::SATA_CONTROLLER_TYPES + StorageController::IDE_CONTROLLER_TYPES + raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, + supported_types: supported_types.join(" ,") end controller @@ -105,13 +89,6 @@ module VagrantPlugins ext = File.extname(attachment[:location].to_s).downcase DEFAULT_DISK_EXT.include?(ext) end - - # List of storage controller types. - # - # @return [Array] types - def types - map { |c| c.storage_bus } - end end end end diff --git a/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb b/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb index 1deff8c20..6dc9072cc 100644 --- a/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb @@ -33,7 +33,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::CleanupDisks do let(:attachments) { [{port: "0", device: "0", uuid: "12345"}, {port: "1", device: "0", uuid: "67890"}]} - let(:controller) { double("controller", name: "controller", limit: 30, storage_bus: "SATA", maxportcount: 30) } + let(:controller) { double("controller", name: "controller", limit: 30, maxportcount: 30) } let(:storage_controllers) { double("storage controllers") } @@ -42,7 +42,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::CleanupDisks do allow(controller).to receive(:get_attachment).with(port: "0", device: "0").and_return(attachments[0]) allow(controller).to receive(:get_attachment).with(uuid: "12345").and_return(attachments[0]) allow(controller).to receive(:get_attachment).with(uuid: "67890").and_return(attachments[1]) - allow(storage_controllers).to receive(:get_controller!).and_return(controller) + allow(storage_controllers).to receive(:get_controller).and_return(controller) allow(storage_controllers).to receive(:get_primary_controller).and_return(controller) allow(storage_controllers).to receive(:get_primary_attachment).and_return(attachments[0]) allow(driver).to receive(:read_storage_controllers).and_return(storage_controllers) 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 da52c3e15..93870baf7 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -27,7 +27,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do let(:storage_controllers) { double("storage controllers") } - let(:controller) { double("controller", name: "controller", limit: 30, storage_bus: "SATA", maxportcount: 30) } + let(:controller) { double("controller", name: "controller", limit: 30, sata?: true, maxportcount: 30) } let(:attachments) { [{port: "0", device: "0", uuid: "12345"}, {port: "1", device: "0", uuid: "67890"}]} @@ -70,7 +70,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do before do allow(Vagrant::Util::Experimental).to receive(:feature_enabled?).and_return(true) allow(controller).to receive(:attachments).and_return(attachments) - allow(storage_controllers).to receive(:get_controller!).with(name: controller.name).and_return(controller) + allow(storage_controllers).to receive(:get_controller).with(controller.name).and_return(controller) allow(storage_controllers).to receive(:first).and_return(controller) allow(storage_controllers).to receive(:size).and_return(1) allow(driver).to receive(:read_storage_controllers).and_return(storage_controllers) @@ -120,14 +120,14 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do # hashicorp/bionic64 context "with more than one storage controller" do - let(:controller1) { double("controller1", name: "IDE Controller", storage_bus: "IDE", limit: 4) } - let(:controller2) { double("controller2", name: "SATA Controller", storage_bus: "SATA", limit: 30) } + let(:controller1) { double("controller1", name: "IDE Controller", limit: 4) } + let(:controller2) { double("controller2", name: "SATA Controller", limit: 30) } before do allow(storage_controllers).to receive(:size).and_return(2) - allow(storage_controllers).to receive(:get_controller!).with(name: controller1.name). + allow(storage_controllers).to receive(:get_controller).with(controller1.name). and_return(controller1) - allow(storage_controllers).to receive(:get_controller!).with(name: controller2.name). + allow(storage_controllers).to receive(:get_controller).with(controller2.name). and_return(controller2) allow(storage_controllers).to receive(:get_dvd_controller).and_return(controller1) allow(storage_controllers).to receive(:get_primary_controller).and_return(controller2) @@ -373,7 +373,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end context "with empty IDE controller" do - let(:empty_controller) { double("controller", storage_bus: "IDE", attachments: [], maxportcount: 2) } + let(:empty_controller) { double("controller", ide?: true, sata?: false, attachments: [], maxportcount: 2, devices_per_port: 2) } it "attaches to port 0, device 0" do dsk_info = subject.get_next_port(machine, empty_controller) diff --git a/test/unit/plugins/providers/virtualbox/model/storage_controller_array_test.rb b/test/unit/plugins/providers/virtualbox/model/storage_controller_array_test.rb index 354f50767..7db7ab97d 100644 --- a/test/unit/plugins/providers/virtualbox/model/storage_controller_array_test.rb +++ b/test/unit/plugins/providers/virtualbox/model/storage_controller_array_test.rb @@ -3,8 +3,8 @@ require File.expand_path("../../base", __FILE__) describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do include_context "unit" - let(:ide_controller) { double("ide_controller", name: "IDE Controller", storage_bus: "IDE") } - let(:sata_controller) { double("sata_controller", name: "SATA Controller", storage_bus: "SATA") } + let(:ide_controller) { double("ide_controller", name: "IDE Controller", ide?: true, sata?: false) } + let(:sata_controller) { double("sata_controller", name: "SATA Controller", sata?: true) } let(:primary_disk) { {location: "/tmp/primary.vdi"} } @@ -14,21 +14,11 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do describe "#get_controller" do it "gets a controller by name" do - expect(subject.get_controller(name: "IDE Controller")).to eq(ide_controller) - end - - it "gets a controller by storage bus" do - expect(subject.get_controller(storage_bus: "SATA")).to eq(sata_controller) - end - end - - describe "#get_controller!" do - it "gets a controller if it exists" do - expect(subject.get_controller!(name: "IDE Controller")).to eq(ide_controller) + expect(subject.get_controller("IDE Controller")).to eq(ide_controller) end it "raises an exception if a matching storage controller can't be found" do - expect { subject.get_controller!(name: "Foo Controller") }. + expect { subject.get_controller(name: "Foo Controller") }. to raise_error(Vagrant::Errors::VirtualBoxDisksControllerNotFound) end end @@ -65,7 +55,8 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do it "raises an error if the machine doesn't have a SATA or an IDE controller" do subject.replace([]) - expect { subject.get_primary_controller }.to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) + expect { subject.get_primary_controller }. + to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) end end end @@ -104,10 +95,4 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do expect { subject.get_primary_attachment }.to raise_error(Vagrant::Errors::VirtualBoxDisksPrimaryNotFound) end end - - describe "#types" do - it "returns a list of storage controller types" do - expect(subject.send(:types)).to eq(["IDE", "SATA"]) - end - end end diff --git a/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb b/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb index d25298449..8032687e7 100644 --- a/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb +++ b/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb @@ -4,19 +4,16 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do include_context "unit" let(:name) {} - let(:type) {} - let(:maxportcount) {} + let(:type) { "IntelAhci" } + let(:maxportcount) { 30 } let(:attachments) {} subject { described_class.new(name, type, maxportcount, attachments) } describe "#initialize" do context "with SATA controller type" do - let(:type) { "IntelAhci" } - let(:maxportcount) { 30 } - it "recognizes a SATA controller" do - expect(subject.storage_bus).to eq('SATA') + expect(subject.sata?).to be(true) end it "calculates the maximum number of attachments" do @@ -29,7 +26,7 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do let(:maxportcount) { 2 } it "recognizes an IDE controller" do - expect(subject.storage_bus).to eq('IDE') + expect(subject.ide?).to be(true) end it "calculates the maximum number of attachments" do @@ -41,7 +38,8 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do let(:type) { "foo" } it "is unknown" do - expect(subject.storage_bus).to eq('Unknown') + expect(subject.ide?).to be(false) + expect(subject.sata?).to be(false) end end end