Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Eligible storages for placement methods #578

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Eligible storages for placement methods #578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
150229d
Eligible storages for ManageIQ/Infrastructure/VM/Provisioning/Placeme…
pkomanek Sep 10, 2019
3aa946d
Spec modifications to changed automate placement method.
pkomanek Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions ...tructure/VM/Provisioning/Placement.class/__methods__/microsoft_best_fit_least_utilized.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
# Description: This method is used to find all hosts, datastores that are the least utilized
#

def clear_host(prov)
prov.set_option(:placement_host_name, [nil, nil])
end

prov = $evm.root["miq_provision"]
vm = prov.vm_template
raise "VM not specified" if vm.nil?
Expand All @@ -19,8 +23,12 @@
next unless h.power_state == "on"
nvms = h.vms.length

# Setting the host to filter eligible storages
prov.set_host(h)

if min_registered_vms.nil? || nvms < min_registered_vms
storages = h.writable_storages.find_all { |s| s.free_space > vm.provisioned_storage } # Filter out storages that do not have enough free space for the Vm
# Filter out storages that do not have enough free space for the Vm
storages = prov.eligible_storages.find_all { |s| s.free_space > vm.provisioned_storage }
s = storages.max_by(&:free_space)
unless s.nil?
host = h
Expand All @@ -31,7 +39,7 @@
end

# Set host and storage
prov.set_host(host) if host
host ? prov.set_host(host) : clear_host(prov)
prov.set_storage(storage) if storage

$evm.log("info", "vm=[#{vm.name}] host=[#{host.try(:name)}] storage=[#{storage.try(:name)}]")
12 changes: 10 additions & 2 deletions ...rastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,20 @@ def storage_profile_id
end
end

def clear_host
request.set_option(:placement_host_name, [nil, nil])
end

def best_fit_least_utilized
host = storage = min_registered_vms = nil
request.eligible_hosts.select { |h| !h.maintenance && h.power_state == "on" }.each do |h|
next if min_registered_vms && h.vms.size >= min_registered_vms

storages = h.writable_storages.find_all { |s| s.free_space > vm.provisioned_storage } # Filter out storages that do not have enough free space for the Vm
# Setting the host to filter eligible storages
request.set_host(h)

# Filter out storages that do not have enough free space for the Vm
storages = request.eligible_storages.find_all { |s| s.free_space > vm.provisioned_storage }
storages.select! { |s| s.storage_profiles.pluck(:id).include?(storage_profile_id) } if storage_profile_id

s = storages.max_by(&:free_space)
Expand All @@ -53,7 +61,7 @@ def best_fit_least_utilized
end

# Set host and storage
request.set_host(host) if host
host ? request.set_host(host) : clear_host
request.set_storage(storage) if storage

@handle.log("info", "vm=[#{vm.name}] host=[#{host}] storage=[#{storage}]")
Expand Down
19 changes: 4 additions & 15 deletions spec/automation/unit/method_validation/scvmm_microsoft_best_fit_least_utilized_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
describe "SCVMM microsoft_best_fit_least_utilized" do
let(:user) { FactoryBot.create(:user_with_group) }
let(:user) { FactoryBot.create(:user_with_group, :settings => {:display => {:timezone => 'UTC'}}) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are actually doing eligible_resources worflow, it is required to get the user timezone. You can see the get_timezone method here: https://github.com/ManageIQ/manageiq/blob/master/app/models/user.rb#L231. I was choosing from 3 options there. 1) override this method with allow and return rspec methods 2) set the user timezone inside the config 3) set the server timezone. I picked the 2nd option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to where the timezone is required by eligible_resources? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the error output without setting the timezone. You can see the whole path here:

 NoMethodError:
   undefined method `server_timezone' for nil:NilClass
 # /home/pkomanek/manageiq/manageiq/app/models/mixins/timezone_mixin.rb:21:in `server_timezone'
 # /home/pkomanek/manageiq/manageiq/app/models/user.rb:219:in `get_timezone'
 # /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:220:in `search'
 # /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:146:in `search'
 # /home/pkomanek/manageiq/manageiq/lib/rbac.rb:3:in `search'
 # /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:441:in `filtered'
 # /home/pkomanek/manageiq/manageiq/lib/rbac/filterer.rb:150:in `filtered'
 # /home/pkomanek/manageiq/manageiq/lib/rbac.rb:11:in `filtered'
 # /home/pkomanek/manageiq/manageiq/app/models/miq_search.rb:43:in `filtered'
 # /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:815:in `process_filter'
 # /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:1056:in `allowed_hosts_obj'
 # /home/pkomanek/manageiq/manageiq/app/models/miq_provision_virt_workflow.rb:190:in `allowed_hosts_obj'
 # /home/pkomanek/manageiq/manageiq/app/models/miq_request_workflow.rb:1088:in `allowed_hosts'
 # /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_provision_mixin.rb:82:in `block in eligible_resources'
 # /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_request_mixin.rb:158:in `workflow'
 # /home/pkomanek/manageiq/manageiq/app/models/mixins/miq_provision_mixin.rb:73:in `eligible_resources'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:333:in `public_send'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:333:in `block in object_send'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:352:in `ar_method'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:362:in `ar_method'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:331:in `object_send'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:46:in `eligible_resources'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:9:in `block (2 levels) in expose_eligible_resources'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:352:in `ar_method'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_model_base.rb:362:in `ar_method'
 # /home/pkomanek/manageiq/manageiq-automation_engine/lib/miq_automation_engine/service_models/mixins/miq_ae_service_miq_provision_mixin.rb:8:in `block in expose_eligible_resources'
 # ./content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb:42:in `best_fit_least_utilized'
 # ./content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb:17:in `main'
 # ./spec/content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized_spec.rb:171:in `block (4 levels) in <top (required)>'

let(:ws) do
MiqAeEngine.instantiate("/System/Request/Call_Instance_With_Message?" \
"namespace=Infrastructure/VM/Provisioning&class=Placement" \
Expand Down Expand Up @@ -29,7 +29,7 @@
ws.root
miq_provision.reload

expect(miq_provision.options[:placement_host_name]).to be_nil
expect(miq_provision.options[:placement_host_name]).to eq([nil, nil])
expect(miq_provision.options[:placement_ds_name]).to be_nil
end

Expand All @@ -45,17 +45,13 @@
ws.root
miq_provision.reload

expect(miq_provision.options[:placement_host_name]).to be_nil
expect(miq_provision.options[:placement_host_name]).to eq([nil, nil])
expect(miq_provision.options[:placement_ds_name]).to be_nil
end

context "with storage" do
before do
host.storages << storage
storage_struct = MiqHashStruct.new(:id => storage.id,
:evm_object_class => storage.class.base_class.name.to_sym)
allow_any_instance_of(ManageIQ::Providers::Microsoft::InfraManager::ProvisionWorkflow)
.to receive(:allowed_storages).and_return([storage_struct])
end

it "will set placement values" do
Expand All @@ -80,18 +76,11 @@
before do
host.storages << [ro_storage, storage]
HostStorage.where(:host_id => host.id, :storage_id => ro_storage.id).update(:read_only => true)

evm_object_class = storage.class.base_class.name.to_sym
storage_struct = [MiqHashStruct.new(:id => ro_storage.id, :evm_object_class => evm_object_class),
MiqHashStruct.new(:id => storage.id, :evm_object_class => evm_object_class)]
allow_any_instance_of(ManageIQ::Providers::Microsoft::InfraManager::ProvisionWorkflow)
.to receive(:allowed_storages).and_return(storage_struct)
end

it "picks the largest writable datastore" do
ws.root
miq_provision.reload

expect(miq_provision.options[:placement_host_name]).to eq([host.id, host.name])
expect(miq_provision.options[:placement_ds_name]).to eq([storage.id, storage.name])
end
Expand All @@ -101,7 +90,7 @@

ws.root
miq_provision.reload
expect(miq_provision.options[:placement_host_name]).to be_nil
expect(miq_provision.options[:placement_host_name]).to eq([nil, nil])
expect(miq_provision.options[:placement_ds_name]).to be_nil
end
end
Expand Down
67 changes: 22 additions & 45 deletions ...ucture/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
:state => 'active',
:status => 'Ok')
end
let(:user) { FactoryBot.create(:user_with_group) }
let(:user) { FactoryBot.create(:user_with_group, :settings => {:display => {:timezone => 'UTC'}}) }
let(:vm_template) { FactoryBot.create(:template_vmware, :ext_management_system => ems) }

let(:svc_miq_provision) { MiqAeMethodService::MiqAeServiceMiqProvision.find(miq_provision.id) }
Expand Down Expand Up @@ -48,11 +48,7 @@
let(:host3) { FactoryBot.create(:host_vmware, :storages => [ro_storage, storages[2]], :vms => vms[2..4], :ext_management_system => ems) }
let(:host4) { FactoryBot.create(:host_vmware, :storages => storages[0..2], :vms => vms[2..4], :ext_management_system => ems) }
let(:host5) { FactoryBot.create(:host_vmware, :storages => [ro_storage, storages[2]], :vms => vms[2..4], :ext_management_system => ems) }

let(:host_struct) do
[MiqHashStruct.new(:id => host1.id, :evm_object_class => host1.class.base_class.name.to_sym),
MiqHashStruct.new(:id => host2.id, :evm_object_class => host2.class.base_class.name.to_sym)]
end
let(:host_struct) { [host1, host2, host4] }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

host_struct is changed from array of MiqHashStruct to array of AR objects. Is this by purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look into the previous spec state, there was a hook, where were the #eligible_storages and #eligible_hosts methods overridden to return array of specific service models. We changed the automate method to use eligible_storages instead of writable_storages method and the filtering of the storages is now executed by eligible_resources workflow. In this case I had to change the host_struct, since it is no more used as the result of hookedeligible_hosts, but in the #find_all_ems_of_type, which is used inside the #allowed_hosts as a part of the eligible workflow. With this change I am able to test the filtering.


let(:svc_host1) { MiqAeMethodService::MiqAeServiceHost.find(host1.id) }
let(:svc_host2) { MiqAeMethodService::MiqAeServiceHost.find(host2.id) }
Expand All @@ -70,14 +66,12 @@
host4.ems_cluster = ems_cluster
datacenter.with_relationship_type("ems_metadata") { datacenter.add_child(ems_cluster) }
HostStorage.where(:host_id => host3.id, :storage_id => ro_storage.id).update(:read_only => true)
allow_any_instance_of(ManageIQ::Providers::Vmware::InfraManager::ProvisionWorkflow)
.to receive(:find_all_ems_of_type).with(Host).and_return(host_struct)
end

it "selects a host with fewer vms and a storage with more free space" do
allow(svc_miq_provision).to receive(:eligible_hosts).and_return(svc_host_struct)
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_storages)

expect(svc_miq_provision).to receive(:set_host).with(svc_host1)
allow(svc_miq_provision).to receive(:set_storage) do |s|
expect(svc_miq_provision).to receive(:set_storage) do |s|
expect(s.id).to eq(svc_host1.storages[1].id)
expect(s.name).to eq(svc_host1.storages[1].name)
end
Expand All @@ -86,12 +80,10 @@
end

context "with all storages accessible" do
it "selects largest storage that is writable" do
allow(svc_miq_provision).to receive(:eligible_hosts).and_return([svc_host3])
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_host3.storages)
let(:host_struct) { [host3] }

expect(svc_miq_provision).to receive(:set_host).with(svc_host3)
allow(svc_miq_provision).to receive(:set_storage) do |s|
it "selects largest storage that is writable" do
expect(svc_miq_provision).to receive(:set_storage) do |s|
# ro_storage is larger but read-only, so it should select storages[2]
expect(s.id).to eq(storages[2].id)
expect(s.name).to eq(storages[2].name)
Expand All @@ -102,35 +94,30 @@
end

context "with no storages accessible" do
let(:host_struct) { [host5] }

before do
HostStorage.where(:host_id => host5.id, :storage_id => ro_storage.id).update(:accessible => false)
HostStorage.where(:host_id => host5.id, :storage_id => storages[2].id).update(:accessible => false)
end

it "selects nothing" do
allow(svc_miq_provision).to receive(:eligible_hosts).and_return([svc_host5])
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_host5.storages)

expect(svc_miq_provision).not_to receive(:set_host)
allow(svc_miq_provision).to receive(:set_storage) do |s|
# both ro_storage and storages[2] are inaccessible
expect(s.id).to eq(nil)
expect(s.name).to eq(nil)
end
# both ro_storage and storages[2] are inaccessible
expect(svc_miq_provision).to_not receive(:set_storage)

described_class.new(ae_service).main
end
end

context "with the writable storage inaccessible" do
let(:host_struct) { [host5] }

before do
HostStorage.where(:host_id => host5.id, :storage_id => storages[2].id).update(:accessible => false)
end
it "selects ro storage" do
allow(svc_miq_provision).to receive(:eligible_hosts).and_return([svc_host5])
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_host5.storages)

expect(svc_miq_provision).to receive(:set_host).with(svc_host5)
allow(svc_miq_provision).to receive(:set_storage) do |s|
it "selects ro storage" do
expect(svc_miq_provision).to receive(:set_storage) do |s|
# storages[2] is inaccessible so it should select the ro
expect(s.id).to eq(ro_storage.id)
expect(s.name).to eq(ro_storage.name)
Expand All @@ -145,11 +132,7 @@
miq_provision.update_attributes(:options => options)
storages[2].storage_profiles = [storage_profile]

allow(svc_miq_provision).to receive(:eligible_hosts).and_return(svc_host_struct)
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_storages)

expect(svc_miq_provision).to receive(:set_host).with(svc_host4)
allow(svc_miq_provision).to receive(:set_storage) do |s|
expect(svc_miq_provision).to receive(:set_storage) do |s|
expect(s.id).to eq(svc_host4.storages[2].id)
expect(s.name).to eq(svc_host4.storages[2].name)
end
Expand All @@ -164,14 +147,12 @@
datacenter.add_child(host1)
datacenter.add_child(host2)
end
allow_any_instance_of(MiqRequestWorkflow)
.to receive(:find_all_ems_of_type).with(Host).and_return(host_struct)
end

it "selects a host with fewer vms and a storage with more free space" do
allow(svc_miq_provision).to receive(:eligible_hosts).and_return(svc_host_struct)
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_storages)

expect(svc_miq_provision).to receive(:set_host).with(svc_host1)
allow(svc_miq_provision).to receive(:set_storage) do |s|
expect(svc_miq_provision).to receive(:set_storage) do |s|
expect(s.id).to eq(svc_host1.storages[1].id)
expect(s.name).to eq(svc_host1.storages[1].name)
end
Expand All @@ -182,11 +163,7 @@
it "selects a host not in maintenance" do
host1.update_attributes(:maintenance => true)

allow(svc_miq_provision).to receive(:eligible_hosts).and_return(svc_host_struct)
allow(svc_miq_provision).to receive(:eligible_storages).and_return(svc_storages)

expect(svc_miq_provision).to receive(:set_host).with(svc_host2)
allow(svc_miq_provision).to receive(:set_storage) do |s|
expect(svc_miq_provision).to receive(:set_storage) do |s|
expect(s.id).to eq(svc_host2.storages[1].id)
expect(s.name).to eq(svc_host2.storages[1].name)
end
Expand Down