Fix deleting a disk from VM

Removing a disk was not handled correctly

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666593

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb

@@ -824,8 +824,18 @@ def prepare_vm_disk_attachment(disk_spec, storage_spec)
     #
     def remove_vm_disks(vm_service, disk_specs)
       attachments_service = vm_service.disk_attachments_service
+
+      disk_attachment_by_disk_name_hash = attachments_service.list.collect do |attachment_service_obj|
+        attachment_service = attachments_service.attachment_service(attachment_service_obj.id)
+        [vm_service.connection.follow_link(attachment_service_obj.disk).name, attachment_service]
+      end.to_h
+
       disk_specs.each do |disk_spec|
-        attachment_service = attachments_service.attachment_service(disk_spec['disk_name'])
+        disk_spec = disk_spec.with_indifferent_access
+        attachment_service = disk_attachment_by_disk_name_hash[disk_spec['disk_name']]
+        unless attachment_service
+          raise "no disk with the name #{disk_spec['disk_name']} is attached to the vm: #{vm_service.get.name}"
+        end
         attachment_service.remove(:detach_only => !disk_spec['delete_backing'])
       end
     end

spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb

@@ -79,7 +79,7 @@
       @vm   = FactoryBot.create(:vm_redhat, :ext_management_system => @ems)
       @cores_per_socket = 2
       @num_of_sockets   = 3
-      @vm_proxy = double("OvirtSDK4::Vm.new")
+      @vm_proxy = double("OvirtSDK4::Vm.new", :name => "vm_name_1")
       @vm_service = double("OvirtSDK4::Vm")
       allow(@ems).to receive(:highest_supported_api_version).and_return(4)
       allow(@vm).to receive(:with_provider_object).and_yield(@vm_service)
@@ -104,6 +104,48 @@
       @ems.vm_reconfigure(@vm, :spec => spec)
     end
 
+    describe "remove disk" do
+      before do
+        @connection = double("OvirtSDK4::Connection")
+        @disk_1 = double("OvirtSDK4::Disk", :id => 'disk_id1', :name => 'disk1')
+        @disk_2 = double("OvirtSDK4::Disk", :id => 'disk_id2', :name => 'disk2')
+        @disk_attachment_1 = double("OvirtSDK4::DiskAttachment", :id => 'disk_id1', :disk => @disk_1)
+        @disk_attachment_2 = double("OvirtSDK4::DiskAttachment", :id => 'disk_id1', :disk => @disk_1)
+        @disk_attachments_service = double("OvirtSDK4::DiskAttachmentsService", :list => [@disk_attachment_1, @disk_attachment_2])
+        @disk_attachment_service_1 = double("OvirtSDK4::DiskAttachmentService", :id => 'disk_attachment_service_1', :get => @disk_attachment_1)
+        @disk_attachment_service_2 = double("OvirtSDK4::DiskAttachmentService", :id => 'disk_attachment_service_2', :get => @disk_attachment_2)
+        allow(@vm_service).to receive(:disk_attachments_service).and_return(@disk_attachments_service)
+        allow(@vm_service).to receive(:connection).and_return(@connection)
+        allow(@disk_attachments_service).to receive(:attachment_service).with(@disk_1.id).and_return(@disk_attachment_service_1)
+        allow(@disk_attachments_service).to receive(:attachment_service).with(@disk_2.id).and_return(@disk_attachment_service_2)
+        allow(@connection).to receive(:follow_link).with(@disk_attachment_1.disk).and_return(@disk_1)
+      end
+      let(:delete_backing) { true }
+      let(:spec) { { 'disksRemove' => [{ 'disk_name' => 'disk1', 'delete_backing' => delete_backing }] } }
+      subject(:reconfigure_vm) { @ems.vm_reconfigure(@vm, :spec => spec) }
+      context 'delete backing' do
+        it 'sends a remove command tp the appropriate disk attachment' do
+          expect(@disk_attachment_service_1).to receive(:remove).with(:detach_only => false)
+          subject
+        end
+      end
+
+      context 'detach without removing disk' do
+        let(:delete_backing) { false }
+        it 'sends a remove command tp the appropriate disk attachment' do
+          expect(@disk_attachment_service_1).to receive(:remove).with(:detach_only => true)
+          subject
+        end
+      end
+
+      context 'disk missing' do
+        let(:spec) { { 'disksRemove' => [{ 'disk_name' => 'disk3', 'delete_backing' => delete_backing }] } }
+        it 'raises an error with the vm and disk name' do
+          expect { subject }.to raise_error("no disk with the name disk3 is attached to the vm: vm_name_1")
+        end
+      end
+    end
+
     describe "memory" do
       before do
         @memory_policy = double("memory_policy")
@@ -113,7 +155,6 @@
         allow(@vm_proxy).to receive(:memory_policy).and_return(@memory_policy)
         allow(@vm_proxy).to receive(:name).and_return("vm_name")
         @memory_spec = { :memory => memory, :memory_policy => { :guaranteed => guaranteed } }
-
       end
       subject(:reconfigure_vm) { @ems.vm_reconfigure(@vm, :spec => spec) }
       let(:spec) { { 'memoryMB' => 8.gigabytes / 1.megabyte } }