From 883e45cc49ca4f58c0543963dbbf07daf5fce9a0 Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Wed, 1 Jul 2020 10:19:19 -0400 Subject: [PATCH] Refactor: isolate "storage bus" logic Move all storage bus logic into the storage controller class. Since most of the storage controller interaction only cares about the storage controller name, we can simplify #get_controller and isolate the storage controller detection-type logic in the StorageControllerArray. --- .../providers/virtualbox/cap/cleanup_disks.rb | 4 +- .../virtualbox/cap/configure_disks.rb | 12 ++-- .../virtualbox/model/storage_controller.rb | 51 ++++++++++------ .../model/storage_controller_array.rb | 61 ++++++------------- .../virtualbox/cap/cleanup_disks_test.rb | 4 +- .../virtualbox/cap/configure_disks_test.rb | 14 ++--- .../model/storage_controller_array_test.rb | 27 ++------ .../model/storage_controller_test.rb | 14 ++--- 8 files changed, 82 insertions(+), 105 deletions(-) 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