upstream/vagrant
1
0
Fork 0
mirror of https://github.com/hashicorp/vagrant.git synced 2026-05-28 04:36:05 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Refactor: isolate "storage bus" logic

Browse source 
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.
This commit is contained in:
Jeff Bonhag 2020-07-01 10:19:19 -04:00
parent 33ef2ca017
commit 883e45cc49
No known key found for this signature in database
GPG key ID: 32966F3FB5AC1129
8 changed files with 82 additions and 105 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
plugins/providers/virtualbox/cap/cleanup_disks.rb
View file

@ -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

12
plugins/providers/virtualbox/cap/configure_disks.rb
View file

@ -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

51
plugins/providers/virtualbox/model/storage_controller.rb
View file

@ -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

61
plugins/providers/virtualbox/model/storage_controller_array.rb
View file

@ -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<String>] types
def types
map { |c| c.storage_bus }
end
end
end
end

4
test/unit/plugins/providers/virtualbox/cap/cleanup_disks_test.rb
View file

@ -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)

14
test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb
View file

@ -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)

27
test/unit/plugins/providers/virtualbox/model/storage_controller_array_test.rb
View file

@ -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

14
test/unit/plugins/providers/virtualbox/model/storage_controller_test.rb
View file

@ -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