Rework destroy domain for multiple box volumes (#1404)

When destroying domains were there are multiple box volumes to be
removed, need to perform a series of checks to establish that the
correct volumes are being removed.

- check if device aliases are in use
- process the box volume removals first
- detect if disks attached outside of vagrant-libvirt
- prefer aliases for additional disks
- avoid use of devices for multiple disks to detect as currently
  assigned incorrectly.
- fallback to detecting number of box volumes to determine
  point at which additional disks start

Attempts to flag a number of cases where behaviour might be incorrect to
help users spot when vagrant-libvirt may accidentally remove something
it shouldn't.

lib/vagrant-libvirt/action/destroy_domain.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'log4r'
+require 'rexml'
 
 module VagrantPlugins
   module ProviderLibvirt
@@ -48,37 +49,126 @@ module VagrantPlugins
             # cdroms are consider volumes, but cannot be destroyed
             domain.destroy(destroy_volumes: true)
           else
+            domain_xml = libvirt_domain.xml_desc(1)
+            xml_descr = REXML::Document.new(domain_xml)
+            disks_xml = REXML::XPath.match(xml_descr, '/domain/devices/disk[@device="disk"]')
+            have_aliases = !(REXML::XPath.match(disks_xml, './alias[@name="ua-box-volume-0"]').first).nil?
+            if !have_aliases
+              env[:ui].warn(I18n.t('vagrant_libvirt.destroy.obsolete_method'))
+            end
+
             domain.destroy(destroy_volumes: false)
 
-            env[:machine].provider_config.disks.each do |disk|
-              # shared disks remove only manually or ???
-              next if disk[:allow_existing]
-              diskname = libvirt_domain.name + '-' + disk[:device] + '.' + disk[:type].to_s
-              # diskname is unique
-              libvirt_disk = domain.volumes.select do |x|
-                x.name == diskname
-              end.first
-              if libvirt_disk
-                libvirt_disk.destroy
-              elsif disk[:path]
-                poolname = env[:machine].provider_config.storage_pool_name
-                libvirt_disk = domain.volumes.select do |x|
-                  # FIXME: can remove pool/target.img and pool/123/target.img
-                  x.path =~ /\/#{disk[:path]}$/ && x.pool_name == poolname
+            volumes = domain.volumes
+
+            # Remove root storage. If no aliases available, perform the removal by name and keep track
+            # of how many matches there are in the volumes. This will provide a fallback offset to where
+            # the additional storage devices are.
+            detected_box_volumes = 0
+            if have_aliases
+              REXML::XPath.match(disks_xml, './alias[contains(@name, "ua-box-volume-")]').each do |box_disk|
+                diskname = box_disk.parent.elements['source'].attributes['file'].rpartition('/').last
+                detected_box_volumes += 1
+
+                destroy_volume(volumes, diskname, env)
+              end
+            else
+              # fallback to try and infer which boxes are box images, as they are listed first
+              # as soon as there is no match, can exit
+              disks_xml.each_with_index do |box_disk, idx|
+                name = libvirt_domain.name + (idx == 0 ? '.img' : "_#{idx}.img")
+                diskname = box_disk.elements['source'].attributes['file'].rpartition('/').last
+
+                break if name != diskname
+                detected_box_volumes += 1
+
+                root_disk = volumes.select do |x|
+                  x.name == name if x
                 end.first
-                libvirt_disk.destroy if libvirt_disk
+                if root_disk
+                  root_disk.destroy
+                end
               end
             end
 
-            # remove root storage
-            root_disk = domain.volumes.select do |x|
-              x.name == libvirt_domain.name + '.img' if x
-            end.first
-            root_disk.destroy if root_disk
+            # work out if there are any custom disks attached that wasn't done by vagrant-libvirt,
+            # and warn there might be unexpected behaviour
+            total_disks = disks_xml.length
+            offset = total_disks - env[:machine].provider_config.disks.length
+            if offset != detected_box_volumes
+              env[:ui].warn(I18n.t('vagrant_libvirt.destroy.unexpected_volumes'))
+            end
+
+            if !have_aliases
+              # if no aliases found, see if it's possible to check the number of box disks
+              # otherwise the destroy could remove the wrong disk by accident.
+              if env[:machine].box != nil
+                box_disks = env[:machine].box.metadata.fetch('disks', [1])
+                offset = box_disks.length
+                if offset != detected_box_volumes
+                  env[:ui].warn(I18n.t('vagrant_libvirt.destroy.expected_removal_mismatch'))
+                end
+              else
+                env[:ui].warn(I18n.t('vagrant_libvirt.destroy.box_metadata_unavailable'))
+              end
+
+              # offset only used when no aliases available
+              offset = detected_box_volumes
+            end
+
+            env[:machine].provider_config.disks.each_with_index.each do |disk, index|
+              # shared disks remove only manually or ???
+              next if disk[:allow_existing]
+
+              # look for exact match using aliases which will be used
+              # for subsequent domain creations
+              if have_aliases
+                domain_disk = REXML::XPath.match(disks_xml, './alias[@name="ua-disk-volume-' + index.to_s + '"]').first
+                domain_disk = domain_disk.parent if !domain_disk.nil?
+              else
+                # otherwise fallback to find the disk by device if specified by user
+                # and finally index counting with offset and hope the match is correct
+                if offset > 1
+                  # Currently disk devices are resolved incorrectly if the box has more than one device
+                  # this should be solved subsequently by delaying the resolution, however for now
+                  # default to using offset when more than one box volume.
+                  domain_disk = disks_xml[offset + index]
+                elsif !disk[:device].nil?
+                  domain_disk = REXML::XPath.match(disks_xml, './target[@dev="' + disk[:device] + '"]').first
+                  domain_disk = domain_disk.parent if !domain_disk.nil?
+                else
+                  domain_disk = disks_xml[offset + index]
+                end
+              end
+
+              next if domain_disk.nil?
+
+              diskname = domain_disk.elements['source'].attributes['file'].rpartition('/').last
+              destroy_volume(volumes, diskname, env)
+            end
           end
 
           @app.call(env)
         end
+
+        protected
+
+        def destroy_volume(volumes, diskname, env)
+          # diskname is unique
+          libvirt_disk = volumes.select do |x|
+            x.name == diskname if x
+          end.first
+          if libvirt_disk
+            libvirt_disk.destroy
+          elsif disk[:path]
+            poolname = env[:machine].provider_config.storage_pool_name
+            libvirt_disk = volumes.select do |x|
+              # FIXME: can remove pool/target.img and pool/123/target.img
+              x.path =~ /\/#{disk[:path]}$/ && x.pool_name == poolname
+            end.first
+            libvirt_disk.destroy if libvirt_disk
+          end
+        end
       end
     end
   end