From ece5449dd282dbc729c1e87a58574c19abb4f6b3 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Tue, 14 Jul 2020 09:22:55 -0700 Subject: [PATCH] Update virtualbox configure_disks to use storage controller attachments This commit updates how Vagrant manages disk state with the virtualbox provider. Instead of using the raw structure from `list_hdds` for all_disks, it instead uses that data that now lives inside the controllers attachment structure. --- .../virtualbox/cap/configure_disks.rb | 57 +++--- .../virtualbox/cap/configure_disks_test.rb | 182 ++++++++++-------- 2 files changed, 124 insertions(+), 115 deletions(-) diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index e2f683260..ba9307ee9 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -93,12 +93,9 @@ module VagrantPlugins current_disk = nil if disk.primary storage_controllers = machine.provider.driver.read_storage_controllers - primary = storage_controllers.get_primary_attachment - primary_uuid = primary[:uuid] - - current_disk = all_disks.select { |d| d["UUID"] == primary_uuid }.first + current_disk = storage_controllers.get_primary_attachment else - current_disk = all_disks.detect { |d| d["Disk Name"] == disk.name } + current_disk = all_disks.detect { |d| d[:disk_name] == disk.name } end current_disk @@ -113,18 +110,7 @@ module VagrantPlugins def self.handle_configure_disk(machine, disk, controller_name) storage_controllers = machine.provider.driver.read_storage_controllers controller = storage_controllers.get_controller(controller_name) - - # Filter current guest disks from all disks - # - # NOTE: This is required beyond just using the entries in - # `controller.attachments` because `list_hdds` gives us more information - # about a disk such as its capacity and disk type, however `list_hdds` - # lists every single harddisk in VirtualBox, so we need to filter on - # our known UUIDs in controller attachments to obtain this info - all_disks = [] - machine.provider.driver.list_hdds.each do |hdd| - all_disks << hdd if controller.attachments.detect { |v| v[:uuid] == hdd["UUID"] } - end + all_disks = controller.attachments disk_metadata = {} @@ -140,7 +126,7 @@ module VagrantPlugins else # TODO: What if it needs to be resized? - disk_info = machine.provider.driver.get_port_and_device(current_disk["UUID"]) + disk_info = machine.provider.driver.get_port_and_device(current_disk[:uuid]) if disk_info.empty? LOGGER.warn("Disk '#{disk.name}' is not connected to guest '#{machine.name}', Vagrant will attempt to connect disk to guest") dsk_info = get_next_port(machine, controller) @@ -148,7 +134,7 @@ module VagrantPlugins dsk_info[:port], dsk_info[:device], "hdd", - current_disk["Location"]) + current_disk[:location]) disk_metadata[:port] = dsk_info[:port] disk_metadata[:device] = dsk_info[:device] else @@ -157,7 +143,7 @@ module VagrantPlugins disk_metadata[:device] = disk_info[:device] end - disk_metadata[:uuid] = current_disk["UUID"] + disk_metadata[:uuid] = current_disk[:uuid] disk_metadata[:name] = disk.name disk_metadata[:controller] = controller.name end @@ -220,7 +206,7 @@ module VagrantPlugins # @return [Boolean] def self.compare_disk_size(machine, disk_config, defined_disk) requested_disk_size = Vagrant::Util::Numeric.bytes_to_megabytes(disk_config.size) - defined_disk_size = defined_disk["Capacity"].split(" ").first.to_f + defined_disk_size = defined_disk[:capacity].split(" ").first.to_f if defined_disk_size > requested_disk_size machine.ui.warn(I18n.t("vagrant.cap.configure_disks.shrink_size_not_supported", name: disk_config.name)) @@ -336,17 +322,17 @@ module VagrantPlugins machine.ui.detail(I18n.t("vagrant.cap.configure_disks.resize_disk", name: disk_config.name), prefix: true) # grab disk to be resized port and device number - disk_info = machine.provider.driver.get_port_and_device(defined_disk["UUID"]) + disk_info = machine.provider.driver.get_port_and_device(defined_disk[:uuid]) - if defined_disk["Storage format"] == "VMDK" + if defined_disk[:storage_format] == "VMDK" LOGGER.warn("Disk type VMDK cannot be resized in VirtualBox. Vagrant will convert disk to VDI format to resize first, and then convert resized disk back to VMDK format") # original disk information in case anything goes wrong during clone/resize original_disk = defined_disk - backup_disk_location = "#{original_disk["Location"]}.backup" + backup_disk_location = "#{original_disk[:location]}.backup" # clone disk to vdi formatted disk - vdi_disk_file = machine.provider.driver.vmdk_to_vdi(defined_disk["Location"]) + vdi_disk_file = machine.provider.driver.vmdk_to_vdi(defined_disk[:location]) # resize vdi machine.provider.driver.resize_disk(vdi_disk_file, disk_config.size.to_i) @@ -355,12 +341,12 @@ module VagrantPlugins # remove and close original volume machine.provider.driver.remove_disk(controller.name, disk_info[:port], disk_info[:device]) # Create a backup of the original disk if something goes wrong - LOGGER.warn("Making a backup of the original disk at #{defined_disk["Location"]}") - FileUtils.mv(defined_disk["Location"], backup_disk_location) + LOGGER.warn("Making a backup of the original disk at #{defined_disk[:location]}") + FileUtils.mv(defined_disk[:location], backup_disk_location) # we have to close here, otherwise we can't re-clone after # resizing the vdi disk - machine.provider.driver.close_medium(defined_disk["UUID"]) + machine.provider.driver.close_medium(defined_disk[:uuid]) # clone back to original vmdk format and attach resized disk vmdk_disk_file = machine.provider.driver.vdi_to_vmdk(vdi_disk_file) @@ -372,7 +358,7 @@ module VagrantPlugins rescue ScriptError, SignalException, StandardError LOGGER.warn("Vagrant encountered an error while trying to resize a disk. Vagrant will now attempt to reattach and preserve the original disk...") machine.ui.error(I18n.t("vagrant.cap.configure_disks.recovery_from_resize", - location: original_disk["Location"], + location: original_disk[:location], name: machine.name)) recover_from_resize(machine, disk_info, backup_disk_location, original_disk, vdi_disk_file, controller) raise @@ -385,13 +371,16 @@ module VagrantPlugins machine.provider.driver.close_medium(vdi_disk_file) # Get new updated disk UUID for vagrant disk_meta file - new_disk_info = machine.provider.driver.list_hdds.select { |h| h["Location"] == defined_disk["Location"] }.first + storage_controllers = machine.provider.driver.read_storage_controllers + updated_controller = storage_controllers.get_controller(controller.name) + new_disk_info = updated_controller.attachments.detect { |h| h[:location] == defined_disk[:location] } + defined_disk = new_disk_info else - machine.provider.driver.resize_disk(defined_disk["Location"], disk_config.size.to_i) + machine.provider.driver.resize_disk(defined_disk[:location], disk_config.size.to_i) end - disk_metadata = { uuid: defined_disk["UUID"], name: disk_config.name, controller: controller.name, + disk_metadata = { uuid: defined_disk[:uuid], name: disk_config.name, controller: controller.name, port: disk_info[:port], device: disk_info[:device] } disk_metadata @@ -411,13 +400,13 @@ module VagrantPlugins def self.recover_from_resize(machine, disk_info, backup_disk_location, original_disk, vdi_disk_file, controller) begin # move backup to original name - FileUtils.mv(backup_disk_location, original_disk["Location"], force: true) + FileUtils.mv(backup_disk_location, original_disk[:location], force: true) # Attach disk machine.provider.driver.attach_disk(controller.name, disk_info[:port], disk_info[:device], "hdd", - original_disk["Location"]) + original_disk[:location]) # Remove cloned disk if still hanging around if vdi_disk_file 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 39dd5113f..5373d17a8 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -31,49 +31,75 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do let(:attachments) { [{:port=>"0", :device=>"0", :uuid=>"12345", + :storage_format=>"VMDK", + :capacity=>"65536 MBytes", :disk_name=>"ubuntu-18.04-amd64-disk001", :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}, {:port=>"1", :device=>"0", :uuid=>"67890", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", :disk_name=>"disk-0", :location=>"/home/vagrant/VirtualBox VMs/disk-0.vdi"}, {:port=>"2", :device=>"0", :uuid=>"10111", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", :disk_name=>"disk-1", :location=>"/home/vagrant/VirtualBox VMs/disk-1.vdi"}] } - let(:defined_disks) { [double("disk", name: "vagrant_primary", size: "5GB", primary: true, type: :disk), - double("disk", name: "disk-0", size: "5GB", primary: false, type: :disk), - double("disk", name: "disk-1", size: "5GB", primary: false, type: :disk), + let(:defined_disks) { [double("disk", name: "vagrant_primary", size: Vagrant::Util::Numeric::string_to_bytes("65GB"), primary: true, type: :disk), + double("disk", name: "disk-0", size: Vagrant::Util::Numeric::string_to_bytes("10GB"), primary: false, type: :disk), + double("disk", name: "disk-1", size: "10GB", primary: false, type: :disk), double("disk", name: "disk-2", size: "5GB", primary: false, type: :disk)] } - let(:all_disks) { [{"UUID"=>"12345", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk", - "Disk Name"=>"ubuntu-18.04-amd64-disk001", - "Storage format"=>"VMDK", - "Capacity"=>"65536 MBytes", - "Encryption"=>"disabled"}, - {"UUID"=>"67890", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/disk-0.vdi", - "Disk Name"=>"disk-0", - "Storage format"=>"VDI", - "Capacity"=>"10240 MBytes", - "Encryption"=>"disabled"}, - {"UUID"=>"324bbb53-d5ad-45f8-9bfa-1f2468b199a8", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/disk-1.vdi", - "Disk Name"=>"disk-1", - "Storage format"=>"VDI", - "Capacity"=>"5120 MBytes", - "Encryption"=>"disabled"}] } + + let(:all_disks) { [{:port=>"0", :device=>"0", + :uuid=>"12345", + :storage_format=>"VMDK", + :capacity=>"65536 MBytes", + :disk_name=>"ubuntu-18.04-amd64-disk001", + :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}, + {:port=>"1", :device=>"0", + :uuid=>"67890", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", + :disk_name=>"disk-0", + :location=>"/home/vagrant/VirtualBox VMs/disk-0.vdi"}, + {:port=>"2", :device=>"0", + :uuid=>"10111", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", + :disk_name=>"disk-1", + :location=>"/home/vagrant/VirtualBox VMs/disk-1.vdi"}] } + + let(:list_hdds_result) { [{"UUID"=>"12345", + "Parent UUID"=>"base", + "State"=>"created", + "Type"=>"normal (base)", + "Location"=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk", + "Disk Name"=>"ubuntu-18.04-amd64-disk001", + "Storage format"=>"VMDK", + "Capacity"=>"65536 MBytes", + "Encryption"=>"disabled"}, + {"UUID"=>"67890", + "Parent UUID"=>"base", + "State"=>"created", + "Type"=>"normal (base)", + "Location"=>"/home/vagrant/VirtualBox VMs/disk-0.vdi", + "Disk Name"=>"disk-0", + "Storage format"=>"VDI", + "Capacity"=>"10240 MBytes", + "Encryption"=>"disabled"}, + {"UUID"=>"324bbb53-d5ad-45f8-9bfa-1f2468b199a8", + "Parent UUID"=>"base", + "State"=>"created", + "Type"=>"normal (base)", + "Location"=>"/home/vagrant/VirtualBox VMs/disk-1.vdi", + "Disk Name"=>"disk-1", + "Storage format"=>"VDI", + "Capacity"=>"5120 MBytes", + "Encryption"=>"disabled"}] } let(:subject) { described_class } @@ -212,21 +238,19 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do describe "#handle_configure_disk" do context "when creating a new disk" do - let(:all_disks) { [{"UUID"=>"12345", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk", - "Disk Name"=>"ubuntu-18.04-amd64-disk001", - "Storage format"=>"VMDK", - "Capacity"=>"65536 MBytes", - "Encryption"=>"disabled"}] } + let(:all_disks) { [{:port=>"0", :device=>"0", + :uuid=>"12345", + :storage_format=>"VMDK", + :capacity=>"65536 MBytes", + :disk_name=>"ubuntu-18.04-amd64-disk001", + :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}] } let(:disk_meta) { {uuid: "67890", name: "disk-0", controller: "controller", port: "1", device: "1"} } it "creates a new disk if it doesn't yet exist" do expect(subject).to receive(:create_disk).with(machine, defined_disks[1], controller) .and_return(disk_meta) + expect(controller).to receive(:attachments).and_return(all_disks) subject.handle_configure_disk(machine, defined_disks[1], controller.name) end @@ -234,6 +258,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do it "includes disk attachment info in metadata" do expect(subject).to receive(:create_disk).with(machine, defined_disks[1], controller) .and_return(disk_meta) + expect(controller).to receive(:attachments).and_return(all_disks) disk_metadata = subject.handle_configure_disk(machine, defined_disks[1], controller.name) expect(disk_metadata).to have_key(:controller) @@ -244,26 +269,22 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end context "when a disk needs to be resized" do - let(:all_disks) { [{"UUID"=>"12345", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk", - "Disk Name"=>"ubuntu-18.04-amd64-disk001", - "Storage format"=>"VMDK", - "Capacity"=>"65536 MBytes", - "Encryption"=>"disabled"}, - {"UUID"=>"67890", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/disk-0.vdi", - "Disk Name"=>"disk-0", - "Storage format"=>"VDI", - "Capacity"=>"10240 MBytes", - "Encryption"=>"disabled"}] } + let(:all_disks) { [{:port=>"0", :device=>"0", + :uuid=>"12345", + :storage_format=>"VMDK", + :capacity=>"65536 MBytes", + :disk_name=>"ubuntu-18.04-amd64-disk001", + :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}, + {:port=>"1", :device=>"0", + :uuid=>"67890", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", + :disk_name=>"disk-0", + :location=>"/home/vagrant/VirtualBox VMs/disk-0.vdi"}] } it "resizes a disk" do + expect(controller).to receive(:attachments).and_return(all_disks) + expect(subject).to receive(:get_current_disk). with(machine, defined_disks[1], all_disks).and_return(all_disks[1]) @@ -278,24 +299,18 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do end context "if no additional disk configuration is required" do - let(:all_disks) { [{"UUID"=>"12345", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk", - "Disk Name"=>"ubuntu-18.04-amd64-disk001", - "Storage format"=>"VMDK", - "Capacity"=>"65536 MBytes", - "Encryption"=>"disabled"}, - {"UUID"=>"67890", - "Parent UUID"=>"base", - "State"=>"created", - "Type"=>"normal (base)", - "Location"=>"/home/vagrant/VirtualBox VMs/disk-0.vdi", - "Disk Name"=>"disk-0", - "Storage format"=>"VDI", - "Capacity"=>"10240 MBytes", - "Encryption"=>"disabled"}] } + let(:all_disks) { [{:port=>"0", :device=>"0", + :uuid=>"12345", + :storage_format=>"VMDK", + :capacity=>"65536 MBytes", + :disk_name=>"ubuntu-18.04-amd64-disk001", + :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}, + {:port=>"1", :device=>"0", + :uuid=>"67890", + :storage_format=>"VDI", + :capacity=>"10240 MBytes", + :disk_name=>"disk-0", + :location=>"/home/vagrant/VirtualBox VMs/disk-0.vdi"}] } let(:disk_info) { {port: "1", device: "0"} } @@ -309,6 +324,8 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do :location=>"/home/vagrant/VirtualBox VMs/disk-0.vdi"}] } it "reattaches disk if vagrant defined disk exists but is not attached to guest" do + expect(controller).to receive(:attachments).and_return(all_disks) + expect(subject).to receive(:get_current_disk). with(machine, defined_disks[1], all_disks).and_return(all_disks[1]) @@ -322,12 +339,14 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do (disk_info[:port].to_i + 1).to_s, disk_info[:device], "hdd", - all_disks[1]["Location"]) + all_disks[1][:location]) subject.handle_configure_disk(machine, defined_disks[1], controller.name) end it "does nothing if all disks are properly configured" do + expect(controller).to receive(:attachments).and_return(all_disks) + expect(subject).to receive(:get_current_disk). with(machine, defined_disks[1], all_disks).and_return(all_disks[1]) @@ -459,7 +478,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(driver).to receive(:get_port_and_device).with("12345"). and_return(attach_info) - expect(driver).to receive(:vmdk_to_vdi).with(all_disks[0]["Location"]). + expect(driver).to receive(:vmdk_to_vdi).with(all_disks[0][:location]). and_return(vdi_disk_file) expect(driver).to receive(:resize_disk).with(vdi_disk_file, disk_config.size.to_i).and_return(true) @@ -475,7 +494,8 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do with(controller.name, attach_info[:port], attach_info[:device], "hdd", vmdk_disk_file).and_return(true) expect(driver).to receive(:close_medium).with(vdi_disk_file).and_return(true) - expect(driver).to receive(:list_hdds).and_return(all_disks) + expect(driver).to receive(:read_storage_controllers) + expect(storage_controllers).to receive(:get_controller) expect(FileUtils).to receive(:remove).with("#{vmdk_disk_file}.backup", force: true). and_return(true) @@ -490,7 +510,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(driver).to receive(:get_port_and_device).with("12345"). and_return(attach_info) - expect(driver).to receive(:vmdk_to_vdi).with(all_disks[0]["Location"]). + expect(driver).to receive(:vmdk_to_vdi).with(all_disks[0][:location]). and_return(vdi_disk_file) expect(driver).to receive(:resize_disk).with(vdi_disk_file, disk_config.size.to_i).and_return(true) @@ -512,9 +532,9 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do primary: false, type: :disk, disk_ext: "vdi", provider_config: nil) } it "resizes the disk" do - expect(driver).to receive(:resize_disk).with(all_disks[1]["Location"], disk_config.size.to_i) + expect(driver).to receive(:resize_disk).with(all_disks[1][:location], disk_config.size.to_i) - expect(driver).to receive(:get_port_and_device).with(all_disks[1]["UUID"]). + expect(driver).to receive(:get_port_and_device).with(all_disks[1][:uuid]). and_return({port: "1", device: "0"}) subject.resize_disk(machine, disk_config, all_disks[1], controller)