From ff9f9c40e8e7948e0a13fa60564ec32904389f0e Mon Sep 17 00:00:00 2001 From: Jeff Bonhag Date: Tue, 30 Jun 2020 13:58:54 -0400 Subject: [PATCH] Differentiate between controller "not found" errors This commit adds a new error message to be raised if a VM has no supported storage controllers. This lets us differentiate between two different "controller not found" scenarios: 1. If we are looking for a controller that we're expecting to find (i.e. one that was recorded in the disk metadata file) 2. If we are poking around for the *best* controller to use in a configuration task --- lib/vagrant/errors.rb | 4 ++ .../virtualbox/cap/configure_disks.rb | 2 +- .../model/storage_controller_array.rb | 55 +++++++++++++++---- templates/locales/en.yml | 17 ++++-- .../virtualbox/cap/configure_disks_test.rb | 5 +- .../model/storage_controller_array_test.rb | 16 ++++-- 6 files changed, 74 insertions(+), 25 deletions(-) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 52fc1a897..c3fa8595e 100644 --- a/lib/vagrant/errors.rb +++ b/lib/vagrant/errors.rb @@ -936,6 +936,10 @@ module Vagrant error_key(:virtualbox_disks_controller_not_found) end + class VirtualBoxDisksNoSupportedControllers < VagrantError + error_key(:virtualbox_disks_no_supported_controllers) + end + class VirtualBoxDisksPrimaryNotFound < VagrantError error_key(:virtualbox_disks_primary_not_found) end diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index a7c7d16ba..575c0d54a 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -55,7 +55,7 @@ module VagrantPlugins dvds_defined = defined_disks.select { |d| d.type == :dvd } if dvds_defined.any? - dvd_controller = storage_controllers.get_controller!(storage_bus: "IDE") + dvd_controller = storage_controllers.get_dvd_controller if dvds_defined.size > dvd_controller.limit raise Vagrant::Errors::VirtualBoxDisksDefinedExceedLimit, diff --git a/plugins/providers/virtualbox/model/storage_controller_array.rb b/plugins/providers/virtualbox/model/storage_controller_array.rb index bf2388b20..bcc93978e 100644 --- a/plugins/providers/virtualbox/model/storage_controller_array.rb +++ b/plugins/providers/virtualbox/model/storage_controller_array.rb @@ -4,6 +4,13 @@ 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. @@ -23,7 +30,7 @@ module VagrantPlugins # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] def get_controller!(opts = {}) controller = get_controller(opts) - if controller.nil? + if !controller && opts[:name] raise Vagrant::Errors::VirtualBoxDisksControllerNotFound, name: opts[:name] end controller @@ -33,19 +40,21 @@ module VagrantPlugins # disk). This is used to determine which controller virtual disks # should be attached to. # + # Raises an exception if no supported controllers are found. + # # @return [VagrantPlugins::ProviderVirtualBox::Model::StorageController] def get_primary_controller controller = nil - if length == 1 - controller = first + 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) + if ide_controller && ide_controller.attachments.any? { |a| hdd?(a) } + controller = ide_controller else - ide_controller = get_controller(storage_bus: "IDE") - if ide_controller && ide_controller.attachments.any? { |a| hdd?(a) } - controller = ide_controller - else - controller = get_controller!(storage_bus: "SATA") - end + controller = get_controller(storage_bus: SATA_TYPE) end controller @@ -68,6 +77,24 @@ module VagrantPlugins attachment end + # Find a suitable 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 = nil + + 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 + end + + controller + end + private # Determine whether the given attachment is a hard disk. @@ -76,8 +103,14 @@ module VagrantPlugins # @return [Boolean] def hdd?(attachment) ext = File.extname(attachment[:location].to_s).downcase - # TODO: hook into ValidateDiskExt capability - [".vdi", ".vmdk", ".vhd"].include?(ext) + DEFAULT_DISK_EXT.include?(ext) + end + + # List of storage controller types. + # + # @return [Array] types + def types + map { |c| c.storage_bus } end end end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 87f6690ff..64be91abc 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -1679,17 +1679,26 @@ en: Vagrant. VirtualBox 4.2.16+ fixes this problem. Please upgrade VirtualBox. virtualbox_disks_controller_not_found: |- - Vagrant was not able to find a storage controller called '%{name}'. + Vagrant expected to find a storage controller called '%{name}', + but there is no controller with this name attached to the current VM. If you have changed or removed any storage controllers, please restore - them to their prior configuration. + them to their previous configuration. + virtualbox_disks_no_supported_controllers: |- + Vagrant was unable to detect a disk controller with any of the + following supported types: + + {supported_types} + + Please add one of the supported controller types in order to use the + disk configuration feature. virtualbox_disks_defined_exceed_limit: |- VirtualBox only allows up to %{limit} disks to be attached to the storage controller '%{name}'. Please remove some disks from your disk configuration, or attach a new storage controller. virtualbox_disks_primary_not_found: |- - Vagrant was not able to detect a primary disk attached to the SATA + Vagrant was not able to detect a primary disk attached to the main controller. Vagrant Disks requires that the primary disk be attached - to the first port of the SATA controller in order to manage it. + to the first port of the main controller in order to manage it. virtualbox_disks_unsupported_controller: |- An disk operation was attempted on the controller '%{controller_name}', but Vagrant doesn't support this type of disk controller. 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 429653b86..da52c3e15 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -129,10 +129,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do and_return(controller1) allow(storage_controllers).to receive(:get_controller!).with(name: controller2.name). and_return(controller2) - allow(storage_controllers).to receive(:get_controller!).with(storage_bus: controller1.storage_bus). - and_return(controller1) - allow(storage_controllers).to receive(:get_controller!).with(storage_bus: controller2.storage_bus). - 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) end 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 f67a07eac..354f50767 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 @@ -6,11 +6,10 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do 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(:primary_disk) { double("attachment", location: "/tmp/primary.vdi") } + let(:primary_disk) { {location: "/tmp/primary.vdi"} } before do - subject << ide_controller - subject << sata_controller + subject.replace([ide_controller, sata_controller]) end describe "#get_controller" do @@ -35,9 +34,10 @@ describe VagrantPlugins::ProviderVirtualBox::Model::StorageControllerArray do end describe "#get_primary_controller" do - context "with one 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]) end it "returns the controller" do @@ -65,7 +65,7 @@ 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::VirtualBoxDisksControllerNotFound) + expect { subject.get_primary_controller }.to raise_error(Vagrant::Errors::VirtualBoxDisksNoSupportedControllers) end end end @@ -104,4 +104,10 @@ 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