diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index b3a739dbf..790592651 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -277,13 +277,13 @@ module VagrantPlugins def self.get_next_port(machine, controller) dsk_info = {} - if controller.sata? + if controller.devices_per_port == 1 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.ide? + elsif controller.devices_per_port == 2 # IDE Controllers have primary/secondary devices, so find the first port # with an empty device (0..(controller.maxportcount - 1)).each do |port| diff --git a/plugins/providers/virtualbox/model/storage_controller.rb b/plugins/providers/virtualbox/model/storage_controller.rb index e378ea081..bb9adefcb 100644 --- a/plugins/providers/virtualbox/model/storage_controller.rb +++ b/plugins/providers/virtualbox/model/storage_controller.rb @@ -4,11 +4,17 @@ module VagrantPlugins # Represents a storage controller for VirtualBox. Storage controllers # have a type, a name, and can have hard disks or optical drives attached. class StorageController - SATA_CONTROLLER_TYPES = ["IntelAhci"].map(&:freeze).freeze IDE_CONTROLLER_TYPES = ["PIIX4", "PIIX3", "ICH6"].map(&:freeze).freeze + SATA_CONTROLLER_TYPES = ["IntelAhci"].map(&:freeze).freeze + SCSI_CONTROLLER_TYPES = [ "LsiLogic", "BusLogic"].map(&:freeze).freeze - SATA_DEVICES_PER_PORT = 1.freeze IDE_DEVICES_PER_PORT = 2.freeze + SATA_DEVICES_PER_PORT = 1.freeze + SCSI_DEVICES_PER_PORT = 1.freeze + + IDE_BOOT_PRIORITY = 1.freeze + SATA_BOOT_PRIORITY = 2.freeze + SCSI_BOOT_PRIORITY = 3.freeze # The name of the storage controller. # @@ -39,6 +45,15 @@ module VagrantPlugins # @return [Integer] attr_reader :limit + # The boot priority of the storage controller. This does not seem to + # depend on the controller number returned by `showvminfo`. + # Experimentation has determined that VirtualBox will try to boot from + # the first controller it finds with a hard disk, in this order: + # IDE, SATA, SCSI + # + # @return [Integer] + attr_reader :boot_priority + # The list of disks/ISOs attached to each storage controller. # # @return [Array] @@ -53,9 +68,15 @@ module VagrantPlugins if IDE_CONTROLLER_TYPES.include?(@type) @storage_bus = :ide @devices_per_port = IDE_DEVICES_PER_PORT + @boot_priority = IDE_BOOT_PRIORITY elsif SATA_CONTROLLER_TYPES.include?(@type) @storage_bus = :sata @devices_per_port = SATA_DEVICES_PER_PORT + @boot_priority = SATA_BOOT_PRIORITY + elsif SCSI_CONTROLLER_TYPES.include?(@type) + @storage_bus = :scsi + @devices_per_port = SCSI_DEVICES_PER_PORT + @boot_priority = SCSI_BOOT_PRIORITY else @storage_bus = :unknown @devices_per_port = 1 @@ -81,6 +102,13 @@ module VagrantPlugins end end + # Returns true if the storage controller has a supported type. + # + # @return [Boolean] + def supported? + [:ide, :sata, :scsi].include?(@storage_bus) + end + # Returns true if the storage controller is a IDE type controller. # # @return [Boolean] @@ -94,6 +122,13 @@ module VagrantPlugins def sata? @storage_bus == :sata end + + # Returns true if the storage controller is a SCSI type controller. + # + # @return [Boolean] + def scsi? + @storage_bus == :scsi + 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 0e50aa277..452acf793 100644 --- a/plugins/providers/virtualbox/model/storage_controller_array.rb +++ b/plugins/providers/virtualbox/model/storage_controller_array.rb @@ -27,17 +27,12 @@ module VagrantPlugins # # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] def get_primary_controller - controller = nil - - ide_controller = detect { |c| c.ide? } - if ide_controller && ide_controller.attachments.any? { |a| hdd?(a) } - controller = ide_controller - else - controller = detect { |c| c.sata? } + ordered = sort { |a, b| a.boot_priority <=> b.boot_priority } + controller = ordered.detect do |c| + c.supported? && c.attachments.any? { |a| hdd?(a) } end if !controller - supported_types = StorageController::SATA_CONTROLLER_TYPES + StorageController::IDE_CONTROLLER_TYPES raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, supported_types: supported_types.join(" ,") end @@ -62,15 +57,14 @@ module VagrantPlugins attachment end - # Find a suitable storage controller for attaching dvds. Will raise an - # exception if no suitable controller can be found. + # Returns the first supported storage controller for attaching dvds. + # Will raise an exception if no suitable controller can be found. # # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] def get_dvd_controller - controller = detect { |c| c.ide? } || detect { |c| c.sata? } - + ordered = sort { |a, b| a.boot_priority <=> b.boot_priority } + controller = ordered.detect { |c| c.supported? } if !controller - supported_types = StorageController::SATA_CONTROLLER_TYPES + StorageController::IDE_CONTROLLER_TYPES raise Vagrant::Errors::VirtualBoxDisksNoSupportedControllers, supported_types: supported_types.join(" ,") end @@ -92,6 +86,14 @@ module VagrantPlugins VagrantPlugins::ProviderVirtualBox::Cap::ValidateDiskExt.validate_disk_ext(nil, ext) end end + + # Returns a list of all the supported controller types. + # + # @return [Array] + def supported_types + StorageController::SATA_CONTROLLER_TYPES + StorageController::IDE_CONTROLLER_TYPES + + StorageController::SCSI_CONTROLLER_TYPES + end end end end 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 93870baf7..c06debc3d 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, sata?: true, maxportcount: 30) } + let(:controller) { double("controller", name: "controller", maxportcount: 30, devices_per_port: 1, limit: 30) } let(:attachments) { [{port: "0", device: "0", uuid: "12345"}, {port: "1", device: "0", uuid: "67890"}]} @@ -120,8 +120,8 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do # hashicorp/bionic64 context "with more than one storage controller" do - let(:controller1) { double("controller1", name: "IDE Controller", limit: 4) } - let(:controller2) { double("controller2", name: "SATA Controller", limit: 30) } + let(:controller1) { double("controller1", name: "IDE Controller", maxportcount: 2, devices_per_port: 2, limit: 4) } + let(:controller2) { double("controller2", name: "SATA Controller", maxportcount: 30, devices_per_port: 1, limit: 30) } before do allow(storage_controllers).to receive(:size).and_return(2) @@ -372,16 +372,43 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(dsk_info[:device]).to eq("0") end - context "with empty IDE controller" do - let(:empty_controller) { double("controller", ide?: true, sata?: false, attachments: [], maxportcount: 2, devices_per_port: 2) } + context "with IDE controller" do + let(:controller) { + double("controller", name: "IDE", maxportcount: 2, devices_per_port: 2, limit: 4) + } + + let(:attachments) { [] } it "attaches to port 0, device 0" do - dsk_info = subject.get_next_port(machine, empty_controller) + dsk_info = subject.get_next_port(machine, controller) + expect(dsk_info[:port]).to eq("0") + expect(dsk_info[:device]).to eq("0") + end + + context "with 1 device" do + let(:attachments) { [{port:"0", device: "0"}] } + + it "attaches to the next device on that port" do + dsk_info = subject.get_next_port(machine, controller) + expect(dsk_info[:port]).to eq("0") + expect(dsk_info[:device]).to eq("1") + end + end + end + + context "with SCSI controller" do + let(:controller) { + double("controller", name: "SCSI", maxportcount: 16, devices_per_port: 1, limit: 16) + } + + let(:attachments) { [] } + + it "determines the next available port and device to use" do + dsk_info = subject.get_next_port(machine, controller) expect(dsk_info[:port]).to eq("0") expect(dsk_info[:device]).to eq("0") end end - end describe "#resize_disk" do 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 7db7ab97d..a0b6d66af 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,18 +3,18 @@ require File.expand_path("../../base", __FILE__) describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do include_context "unit" - 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(:controller1) { double("controller1", name: "IDE Controller", supported?: true, boot_priority: 1) } + let(:controller2) { double("controller2", name: "SATA Controller", supported?: true, boot_priority: 2) } let(:primary_disk) { {location: "/tmp/primary.vdi"} } before do - subject.replace([ide_controller, sata_controller]) + subject.replace([controller1, controller2]) end describe "#get_controller" do it "gets a controller by name" do - expect(subject.get_controller("IDE Controller")).to eq(ide_controller) + expect(subject.get_controller("IDE Controller")).to eq(controller1) end it "raises an exception if a matching storage controller can't be found" do @@ -26,34 +26,27 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do describe "#get_primary_controller" do context "with a single supported controller" do before do - subject.replace([ide_controller]) - allow(ide_controller).to receive(:attachments).and_return([primary_disk]) + subject.replace([controller1]) + allow(controller1).to receive(:attachments).and_return([primary_disk]) end it "returns the controller" do - expect(subject.get_primary_controller).to eq(ide_controller) + expect(subject.get_primary_controller).to eq(controller1) end end context "with multiple controllers" do before do - allow(ide_controller).to receive(:attachments).and_return([]) - allow(sata_controller).to receive(:attachments).and_return([]) + allow(controller1).to receive(:attachments).and_return([]) + allow(controller2).to receive(:attachments).and_return([primary_disk]) end - it "returns the SATA controller by default" do - expect(subject.get_primary_controller).to eq(sata_controller) + it "returns the first supported controller with a disk attached" do + expect(subject.get_primary_controller).to eq(controller2) end - it "returns the IDE controller if it has a hdd attached" do - allow(ide_controller).to receive(:attachments).and_return([primary_disk]) - allow(subject).to receive(:hdd?).with(primary_disk).and_return(true) - - expect(subject.get_primary_controller).to eq(ide_controller) - end - - it "raises an error if the machine doesn't have a SATA or an IDE controller" do - subject.replace([]) + it "raises an error if the primary disk is attached to an unsupported controller" do + allow(controller2).to receive(:supported?).and_return(false) expect { subject.get_primary_controller }. to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) @@ -82,17 +75,57 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do let(:attachment) { {location: "/tmp/primary.vdi"} } before do - allow(subject).to receive(:get_primary_controller).and_return(sata_controller) + allow(subject).to receive(:get_primary_controller).and_return(controller2) end it "returns the first attachment on the primary controller" do - allow(sata_controller).to receive(:get_attachment).with(port: "0", device: "0").and_return(attachment) + allow(controller2).to receive(:get_attachment).with(port: "0", device: "0").and_return(attachment) expect(subject.get_primary_attachment).to be(attachment) end it "raises an exception if no attachment exists at port 0, device 0" do - allow(sata_controller).to receive(:get_attachment).with(port: "0", device: "0").and_return(nil) + allow(controller2).to receive(:get_attachment).with(port: "0", device: "0").and_return(nil) expect { subject.get_primary_attachment }.to raise_error(Vagrant::Errors::VirtualBoxDisksPrimaryNotFound) end end + + describe "#get_dvd_controller" do + context "with one controller" do + let(:controller) { double("controller", supported?: true) } + + before do + subject.replace([controller]) + end + + it "returns the controller" do + expect(subject.get_dvd_controller).to be(controller) + end + + it "raises an exception if the controller is unsupported" do + allow(controller).to receive(:supported?).and_return(false) + + expect { subject.get_dvd_controller }.to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) + end + end + + context "with multiple controllers" do + let(:controller1) { double("controller", supported?: true, boot_priority: 2) } + let(:controller2) { double("controller", supported?: true, boot_priority: 1) } + + before do + subject.replace([controller1, controller2]) + end + + it "returns the first supported controller" do + expect(subject.get_dvd_controller).to be(controller2) + end + + it "raises an exception if no controllers are supported" do + allow(controller1).to receive(:supported?).and_return(false) + allow(controller2).to receive(:supported?).and_return(false) + + expect { subject.get_dvd_controller }.to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) + end + 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 8032687e7..918f4815b 100644 --- a/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb +++ b/test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb @@ -19,6 +19,10 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do it "calculates the maximum number of attachments" do expect(subject.limit).to eq(30) end + + it "sets the boot priority" do + expect(subject.boot_priority).to eq(2) + end end context "with IDE controller type" do @@ -32,6 +36,27 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do it "calculates the maximum number of attachments" do expect(subject.limit).to eq(4) end + + it "sets the boot priority" do + expect(subject.boot_priority).to eq(1) + end + end + + context "with SCSI controller type" do + let(:type) { "LsiLogic" } + let(:maxportcount) { 16 } + + it "recognizes an SCSI controller" do + expect(subject.scsi?).to be(true) + end + + it "calculates the maximum number of attachments" do + expect(subject.limit).to eq(16) + end + + it "sets the boot priority" do + expect(subject.boot_priority).to eq(3) + end end context "with some other type" do @@ -43,4 +68,18 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageController do end end end + + describe "#supported?" do + it "returns true if the controller type is supported" do + expect(subject.supported?).to be(true) + end + + context "with unsupported type" do + let(:type) { "foo" } + + it "returns false" do + expect(subject.supported?).to be(false) + end + end + end end