From 466330e0293da4f1060feea799b3a9e4fe831741 Mon Sep 17 00:00:00 2001 From: Dhamo1107 Date: Sat, 19 Oct 2024 12:47:16 +0530 Subject: [PATCH 1/4] Add the volume to the root disk (vda) if the selected flavor has a root disk size of 0 during instance provisioning --- .../cloud_manager/provision/volume_attachment.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb b/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb index 42e9214ba..74fabeb01 100644 --- a/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb +++ b/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb @@ -1,10 +1,20 @@ module ManageIQ::Providers::Openstack::CloudManager::Provision::VolumeAttachment def create_requested_volumes(requested_volumes) - volumes_attrs_list = [default_volume_attributes] + volumes_attrs_list = instance_type.root_disk_size > 0 ? [default_volume_attributes] : [] connection_options = {:service => "volume", :tenant_name => cloud_tenant.try(:name)} source.ext_management_system.with_provider_connection(connection_options) do |service| - requested_volumes.each do |volume_attrs| + requested_volumes.each_with_index do |volume_attrs, index| + if instance_type.root_disk_size == 0 # Handle root disk logic only when root_disk_size is 0 + if index == 0 # The first volume should be the root disk, typically assigned to vda + volume_attrs[:imageRef] = source.ems_ref # Set the image reference for booting + volume_attrs[:bootable] = true + volume_attrs[:boot_index] = 0 + else # Subsequent volumes will be assigned to vdb, vdc, etc. + volume_attrs[:boot_index] = -1 + end + end + new_volume_id = service.volumes.create(volume_attrs).id new_volume_attrs = volume_attrs.clone new_volume_attrs[:uuid] = new_volume_id From d7e2dab8df8cb399ad09479a25175511fb0fe06c Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Thu, 24 Oct 2024 10:57:55 -0400 Subject: [PATCH 2/4] Cleanup volume_attachment_spec This spec made heavy use of instance vars rather than `let()` which makes it more difficult to override these in sub-contexts. --- .../provision/volume_attachment_spec.rb | 80 +++++++++---------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb index 6d2d924b4..c66e74e7d 100644 --- a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb +++ b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb @@ -1,82 +1,78 @@ describe ManageIQ::Providers::Openstack::CloudManager::Provision::VolumeAttachment do - before do - @ems = FactoryBot.create(:ems_openstack_with_authentication) - @template = FactoryBot.create(:template_openstack, :ext_management_system => @ems) - @flavor = FactoryBot.create(:flavor_openstack) - @volume = FactoryBot.create(:cloud_volume_openstack) + let(:ems) { FactoryBot.create(:ems_openstack_with_authentication) } + let(:flavor) { FactoryBot.create(:flavor_openstack) } + let(:template) { FactoryBot.create(:template_openstack, :ext_management_system => ems) } + let(:volume) { FactoryBot.create(:cloud_volume_openstack) } + let(:service) { double("volume service") } + let(:task_options) { {:instance_type => flavor, :src_vm_id => template.id, :volumes => task_volumes} } + let(:task_volumes) { [{:name => "custom_volume_1", :size => 2}] } + let(:task) { FactoryBot.create(:miq_provision_openstack, :source => template, :state => 'pending', :status => 'Ok', :options => task_options) } + before do # We're storing objects in the instance_type, so we must permit loading this class - ActiveRecord.yaml_column_permitted_classes = YamlPermittedClasses.app_yaml_permitted_classes | [@flavor.class] - - @task = FactoryBot.create(:miq_provision_openstack, - :source => @template, - :state => 'pending', - :status => 'Ok', - :options => { - :instance_type => @flavor, - :src_vm_id => @template.id, - :volumes => [{:name => "custom_volume_1", :size => 2}] - }) + ActiveRecord.yaml_column_permitted_classes = YamlPermittedClasses.app_yaml_permitted_classes | [flavor.class] end context "#configure_volumes" do - it "create volumes" do - service = double - allow(service).to receive_message_chain('volumes.create').and_return @volume - allow(@task.source.ext_management_system).to receive(:with_provider_connection)\ + before do + allow(service).to receive_message_chain(:volumes, :create).and_return(volume) + allow(task.source.ext_management_system).to receive(:with_provider_connection) .with({:service => 'volume', :tenant_name => nil}).and_yield(service) - allow(@task).to receive(:instance_type).and_return @flavor + allow(task).to receive(:instance_type).and_return(flavor) + end + it "create volumes" do default_volume = {:name => "root", :size => 1, :source_type => "image", :destination_type => "local", :boot_index => 0, :delete_on_termination => true, :uuid => nil} - requested_volume = {:name => "custom_volume_1", :size => 2, :uuid => @volume.id, :source_type => "volume", + requested_volume = {:name => "custom_volume_1", :size => 2, :uuid => volume.id, :source_type => "volume", :destination_type => "volume"} - expect(@task.create_requested_volumes(@task.options[:volumes])).to eq [default_volume, requested_volume] + expect(task.create_requested_volumes(task.options[:volumes])).to eq([default_volume, requested_volume]) end end context "#check_volumes" do it "status pending" do pending_volume_attrs = {:source_type => "volume"} - service = double - allow(service).to receive_message_chain('volumes.get').and_return FactoryBot.build(:cloud_volume_openstack, - :status => "pending") - allow(@task.source.ext_management_system).to receive(:with_provider_connection)\ + + allow(service).to receive_message_chain('volumes.get') + .and_return(FactoryBot.build(:cloud_volume_openstack, :status => "pending")) + allow(task.source.ext_management_system).to receive(:with_provider_connection) .with({:service => 'volume', :tenant_name => nil}).and_yield(service) - expect(@task.do_volume_creation_check([pending_volume_attrs])).to eq [false, "pending"] + expect(task.do_volume_creation_check([pending_volume_attrs])).to eq([false, "pending"]) end it "check creation status available" do pending_volume_attrs = {:source_type => "volume"} - service = double - allow(service).to receive_message_chain('volumes.get').and_return FactoryBot.build(:cloud_volume_openstack, - :status => "available") - allow(@task.source.ext_management_system).to receive(:with_provider_connection)\ + + allow(service).to receive_message_chain('volumes.get') + .and_return(FactoryBot.build(:cloud_volume_openstack, :status => "available")) + allow(task.source.ext_management_system).to receive(:with_provider_connection) .with({:service => 'volume', :tenant_name => nil}).and_yield(service) - expect(@task.do_volume_creation_check([pending_volume_attrs])).to eq true + expect(task.do_volume_creation_check([pending_volume_attrs])).to be_truthy end it "check creation status - not found" do pending_volume_attrs = {:source_type => "volume"} - service = double - allow(service).to receive_message_chain('volumes.get').and_return nil - allow(@task.source.ext_management_system).to receive(:with_provider_connection)\ + + allow(service).to receive_message_chain('volumes.get').and_return(nil) + allow(task.source.ext_management_system).to receive(:with_provider_connection) .with({:service => 'volume', :tenant_name => nil}).and_yield(service) - expect(@task.do_volume_creation_check([pending_volume_attrs])).to eq [false, nil] + expect(task.do_volume_creation_check([pending_volume_attrs])).to eq([false, nil]) end it "status error" do pending_volume_attrs = {:source_type => "volume"} - service = double - allow(service).to receive_message_chain('volumes.get').and_return FactoryBot.build(:cloud_volume_openstack, - :status => "error") - allow(@task.source.ext_management_system).to receive(:with_provider_connection)\ + + allow(service).to receive_message_chain('volumes.get') + .and_return(FactoryBot.build(:cloud_volume_openstack, :status => "error")) + allow(task.source.ext_management_system).to receive(:with_provider_connection) .with({:service => 'volume', :tenant_name => nil}).and_yield(service) - expect { @task.do_volume_creation_check([pending_volume_attrs]) }.to raise_error(MiqException::MiqProvisionError) + + expect { task.do_volume_creation_check([pending_volume_attrs]) }.to raise_error(MiqException::MiqProvisionError) end end end From d3da5e74c8fcd32049a9f74acac6988c71ef8d19 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Thu, 24 Oct 2024 11:08:26 -0400 Subject: [PATCH 3/4] Add specs for volume provision flavor no root disk If a flavor has no root disk we have to set up the bootable volume manually based on the first requested volume. --- .../provision/volume_attachment_spec.rb | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb index c66e74e7d..40d2bded8 100644 --- a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb +++ b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb @@ -29,6 +29,30 @@ expect(task.create_requested_volumes(task.options[:volumes])).to eq([default_volume, requested_volume]) end + + context "with a flavor that has no root disk" do + let(:flavor) { FactoryBot.create(:flavor_openstack, :root_disk_size => 0) } + + it "sets the requested volume as a boot disk" do + expected_volume = {:name => "custom_volume_1", :size => 2, :uuid => volume.id, :source_type => "volume", + :destination_type => "volume", :boot_index => 0, :bootable => true, :imageRef => template.ems_ref} + + expect(task.create_requested_volumes(task.options[:volumes])).to eq([expected_volume]) + end + + context "with multiple requested volumes" do + let(:task_volumes) { [{:name => "custom_volume_1", :size => 2}, {:name => "custom_volume_2", :size => 4}] } + + it "sets other volumes as boot_index -1" do + expected_volume_1 = {:name => "custom_volume_1", :size => 2, :uuid => volume.id, :source_type => "volume", + :destination_type => "volume", :boot_index => 0, :bootable => true, :imageRef => template.ems_ref} + expected_volume_2 = {:name => "custom_volume_2", :size => 4, :uuid => volume.id, :source_type => "volume", + :destination_type => "volume", :boot_index => -1} + + expect(task.create_requested_volumes(task.options[:volumes])).to eq([expected_volume_1, expected_volume_2]) + end + end + end end context "#check_volumes" do From b5f9b0f2099c007d39b9a7880df2f33e01710a28 Mon Sep 17 00:00:00 2001 From: Adam Grare Date: Tue, 10 Dec 2024 09:09:01 -0500 Subject: [PATCH 4/4] Drop boot_index -1 --- .../openstack/cloud_manager/provision/volume_attachment.rb | 2 -- .../cloud_manager/provision/volume_attachment_spec.rb | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb b/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb index 74fabeb01..50ac4695d 100644 --- a/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb +++ b/app/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment.rb @@ -10,8 +10,6 @@ def create_requested_volumes(requested_volumes) volume_attrs[:imageRef] = source.ems_ref # Set the image reference for booting volume_attrs[:bootable] = true volume_attrs[:boot_index] = 0 - else # Subsequent volumes will be assigned to vdb, vdc, etc. - volume_attrs[:boot_index] = -1 end end diff --git a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb index 40d2bded8..436b8d1e2 100644 --- a/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb +++ b/spec/models/manageiq/providers/openstack/cloud_manager/provision/volume_attachment_spec.rb @@ -43,11 +43,11 @@ context "with multiple requested volumes" do let(:task_volumes) { [{:name => "custom_volume_1", :size => 2}, {:name => "custom_volume_2", :size => 4}] } - it "sets other volumes as boot_index -1" do + it "only sets boot_index for first volumes" do expected_volume_1 = {:name => "custom_volume_1", :size => 2, :uuid => volume.id, :source_type => "volume", :destination_type => "volume", :boot_index => 0, :bootable => true, :imageRef => template.ems_ref} expected_volume_2 = {:name => "custom_volume_2", :size => 4, :uuid => volume.id, :source_type => "volume", - :destination_type => "volume", :boot_index => -1} + :destination_type => "volume"} expect(task.create_requested_volumes(task.options[:volumes])).to eq([expected_volume_1, expected_volume_2]) end