From 70f427f8bfb2837c2354ffd06f6779bd11ec577f Mon Sep 17 00:00:00 2001 From: Boris Odnopozov Date: Sun, 10 Mar 2019 15:02:19 +0200 Subject: [PATCH] Fix deleting a disk from VM Removing a disk was not handled correctly Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666593 --- .../ovirt_services/strategies/v4.rb | 12 ++++- .../ovirt_services/strategies/v4_spec.rb | 45 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb b/app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb index a1117a509..edea4b3ba 100644 --- a/app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb +++ b/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 diff --git a/spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb b/spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb index 9584464d3..32053a289 100644 --- a/spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb +++ b/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 } }