From e2c844db74f78406a23398e883bda048a0e2d3a3 Mon Sep 17 00:00:00 2001 From: Brian Cain Date: Mon, 27 Jul 2020 13:31:28 -0700 Subject: [PATCH] Properly determine disk recovery Because Vagrant no longer determines `all_disks` through `vboxmanage list hdds`, it can't rely on unattached disks existing in `all_disks`. This commit fixes that by including the results from `list hdds` to determine if Vagrant needs to reattach a disk created by Vagrant that failed to be attached previously. --- .../virtualbox/cap/configure_disks.rb | 49 +++++++++++++------ templates/locales/en.yml | 8 +-- .../virtualbox/cap/configure_disks_test.rb | 29 +++++++---- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/plugins/providers/virtualbox/cap/configure_disks.rb b/plugins/providers/virtualbox/cap/configure_disks.rb index ba9307ee9..5eb7e7d60 100644 --- a/plugins/providers/virtualbox/cap/configure_disks.rb +++ b/plugins/providers/virtualbox/cap/configure_disks.rb @@ -114,36 +114,53 @@ module VagrantPlugins disk_metadata = {} - # Grab the existing configured disk, if it exists + # Grab the existing configured disk attached to guest, if it exists current_disk = get_current_disk(machine, disk, all_disks) - # Configure current disk if !current_disk - # create new disk and attach - disk_metadata = create_disk(machine, disk, controller) - elsif compare_disk_size(machine, disk, current_disk) - disk_metadata = resize_disk(machine, disk, current_disk, controller) - else - # TODO: What if it needs to be resized? + # Look for an existing disk that's not been attached but exists + # inside VirtualBox + # + # NOTE: This assumes that if that disk exists and was created by + # Vagrant, it exists in the same location as the primary disk file. + # Otherwise Vagrant has no good way to determining if the disk was + # associated with the guest, since disk names are not unique + # globally to VirtualBox. + primary = storage_controllers.get_primary_attachment + existing_disk = machine.provider.driver.list_hdds.detect do |d| + File.dirname(d["Location"]) == File.dirname(primary[:location]) && + d["Disk Name"] == disk.name + end - disk_info = machine.provider.driver.get_port_and_device(current_disk[:uuid]) - if disk_info.empty? + if !existing_disk + # create new disk and attach to guest + disk_metadata = create_disk(machine, disk, controller) + else + # Disk has been created but failed to be attached to guest, so + # this method recovers that disk from previous failure + # and attaches it onto the guest 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) machine.provider.driver.attach_disk(controller.name, dsk_info[:port], dsk_info[:device], "hdd", - current_disk[:location]) + existing_disk["Location"]) + + disk_metadata[:uuid] = existing_disk["UUID"] disk_metadata[:port] = dsk_info[:port] disk_metadata[:device] = dsk_info[:device] - else - LOGGER.info("No further configuration required for disk '#{disk.name}'") - disk_metadata[:port] = disk_info[:port] - disk_metadata[:device] = disk_info[:device] + disk_metadata[:name] = disk.name + disk_metadata[:controller] = controller.name end - + elsif compare_disk_size(machine, disk, current_disk) + disk_metadata = resize_disk(machine, disk, current_disk, controller) + else + LOGGER.info("No further configuration required for disk '#{disk.name}'") disk_metadata[:uuid] = current_disk[:uuid] + disk_metadata[:port] = current_disk[:port] + disk_metadata[:device] = current_disk[:device] + disk_metadata[:name] = disk.name disk_metadata[:controller] = controller.name end diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 6bdb40967..5afb8c300 100644 --- a/templates/locales/en.yml +++ b/templates/locales/en.yml @@ -946,7 +946,7 @@ en: Failed to build iso image. The following command returned an error: %{cmd} - + Stdout from the command: %{stdout} @@ -2265,12 +2265,11 @@ en: disk_not_found: |- Disk '%{name}' could not be found, and could not be properly removed. Please remove this disk manually if it still exists configure_disks: - start: "Configuring storage mediums..." + create_disk: |- + Disk '%{name}' not found in guest. Creating and attaching disk to guest... floppy_not_supported: "Floppy disk configuration not yet supported. Skipping disk '%{name}'..." shrink_size_not_supported: |- VirtualBox does not support shrinking disk sizes. Cannot shrink '%{name}' disks size. - create_disk: |- - Disk '%{name}' not found in guest. Creating and attaching disk to guest... resize_disk: |- Disk '%{name}' needs to be resized. Resizing disk... recovery_from_resize: |- @@ -2279,6 +2278,7 @@ en: If Vagrant fails to reattach the original disk, it is recommended that you open the VirtualBox GUI and navigate to the current guests settings for '%{name}' and look at the 'storage' section. Here is where you can reattach a missing disk if Vagrant fails to do so... recovery_attached_disks: |- Disk has been reattached. Vagrant will now continue on an raise the exception receieved + start: "Configuring storage mediums..." cloud_init: content_type_not_set: |- 'content_type' must be defined for a cloud_init config. Guest '%{machine}' 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 5373d17a8..81d112ac8 100644 --- a/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb +++ b/test/unit/plugins/providers/virtualbox/cap/configure_disks_test.rb @@ -110,7 +110,7 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do 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) - allow(driver).to receive(:list_hdds).and_return(all_disks) + allow(driver).to receive(:list_hdds).and_return(list_hdds_result) end describe "#configure_disks" do @@ -245,6 +245,16 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do :disk_name=>"ubuntu-18.04-amd64-disk001", :location=>"/home/vagrant/VirtualBox VMs/ubuntu-18.04-amd64-disk001.vmdk"}] } + 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"}] } + 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 @@ -252,6 +262,9 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do .and_return(disk_meta) expect(controller).to receive(:attachments).and_return(all_disks) + expect(storage_controllers).to receive(:get_primary_attachment) + .and_return(all_disks[0]) + subject.handle_configure_disk(machine, defined_disks[1], controller.name) end @@ -259,6 +272,8 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks 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) + expect(storage_controllers).to receive(:get_primary_attachment) + .and_return(all_disks[0]) disk_metadata = subject.handle_configure_disk(machine, defined_disks[1], controller.name) expect(disk_metadata).to have_key(:controller) @@ -327,13 +342,10 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks 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]) + with(machine, defined_disks[1], all_disks).and_return(nil) - expect(subject).to receive(:compare_disk_size). - with(machine, defined_disks[1], all_disks[1]).and_return(false) - - expect(driver).to receive(:get_port_and_device).with("67890"). - and_return({}) + expect(storage_controllers).to receive(:get_primary_attachment) + .and_return(all_disks[0]) expect(driver).to receive(:attach_disk).with(controller.name, (disk_info[:port].to_i + 1).to_s, @@ -353,9 +365,6 @@ describe VagrantPlugins::ProviderVirtualBox::Cap::ConfigureDisks do expect(subject).to receive(:compare_disk_size). with(machine, defined_disks[1], all_disks[1]).and_return(false) - expect(driver).to receive(:get_port_and_device).with("67890"). - and_return(disk_info) - subject.handle_configure_disk(machine, defined_disks[1], controller.name) end end