Eligible storages for placement methods #578

pkomanek · 2019-09-10T13:51:04Z

Changing the host.writable_storages to request.eligible_storages for ManageIQ placement methods to avoid getting the RBAC error: "not an eligible resource for this provisioning instance".

This PR is based on https://bugzilla.redhat.com/show_bug.cgi?id=1698183.

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

coveralls · 2019-09-10T14:05:26Z

Pull Request Test Coverage Report for Build 3671

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+5.6%) to 96.797%

Totals
Change from base Build 3644:	5.6%
Covered Lines:	2750
Relevant Lines:	2841

💛 - Coveralls

tinaafitz · 2019-10-04T17:26:08Z

@gmcculloug Please review

lfu · 2019-10-07T14:44:56Z

spec/automation/unit/method_validation/scvmm_microsoft_best_fit_least_utilized_spec.rb

@@ -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'}}) }


What is the reason for this change?

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.

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

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)>'

lfu · 2019-10-07T14:47:22Z

...structure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized_spec.rb

-      [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] }


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

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.

gmcculloug · 2019-10-08T15:23:40Z

...rastructure/VM/Provisioning/Placement.class/__methods__/microsoft_best_fit_least_utilized.rb

@@ -31,7 +35,7 @@
 end

 # Set host and storage
-prov.set_host(host) if host
+host ? prov.set_host(host) : prov.set_option(:placement_host_name, [nil, nil])


Minor, but I suggest you move the prov.set_option(:placement_host_name, [nil, nil]) logic into a method named clear_host which would make this easier to read.

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

gmcculloug · 2019-10-08T15:24:10Z

...Infrastructure/VM/Provisioning/Placement.class/__methods__/vmware_best_fit_least_utilized.rb

@@ -53,7 +57,7 @@ def best_fit_least_utilized
                end

                # Set host and storage
-                request.set_host(host) if host
+                host ? request.set_host(host) : request.set_option(:placement_host_name, [nil, nil])


Use the same clear_host technique here.

…nt methods.

miq-bot · 2019-10-10T12:31:10Z

Some comments on commits pkomanek/manageiq-content@150229d~...3aa946d

spec/content/automate/ManageIQ/Infrastructure/VM/Provisioning/Placement.class/methods/vmware_best_fit_least_utilized_spec.rb

⚠️ - 150 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 69 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2019-10-10T12:31:25Z

Checked commits pkomanek/manageiq-content@150229d~...3aa946d with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🍪

simaishi · 2020-02-21T14:52:24Z

@pkomanek @gmcculloug can this be ivanchuk/yes?

tinaafitz · 2020-02-21T18:36:26Z

Hi @simaishi, yes, it can be ivanchuk/yes.

…st_fit_least_utilized Eligible storages for placement methods (cherry picked from commit 957bbd9) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805979

simaishi · 2020-02-21T21:29:05Z

Ivanchuk backport details:

$ git log -1
commit 267b4786c5231afd17a445ba4aafa0f0670de38f
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Oct 10 10:08:17 2019 -0400

    Merge pull request #578 from pkomanek/eligible_storages_for_vmware_best_fit_least_utilized

    Eligible storages for placement methods

    (cherry picked from commit 957bbd973e3d0856c426004b8adb99eeb3d82659)

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

pkomanek force-pushed the eligible_storages_for_vmware_best_fit_least_utilized branch from fb09ed8 to dc73260 Compare September 10, 2019 13:53

miq-bot added the wip label Sep 10, 2019

pkomanek force-pushed the eligible_storages_for_vmware_best_fit_least_utilized branch 2 times, most recently from 6b986e3 to 748742b Compare September 18, 2019 17:21

pkomanek force-pushed the eligible_storages_for_vmware_best_fit_least_utilized branch 2 times, most recently from bb575da to 6e8ae11 Compare September 25, 2019 14:49

pkomanek force-pushed the eligible_storages_for_vmware_best_fit_least_utilized branch 2 times, most recently from e7cbf09 to c3f4b2a Compare October 3, 2019 12:50

pkomanek changed the title ~~[WIP] Eligible storages for placement methods~~ Eligible storages for placement methods Oct 3, 2019

miq-bot removed the wip label Oct 3, 2019

tinaafitz requested a review from lfu October 4, 2019 17:25

lfu reviewed Oct 7, 2019

View reviewed changes

tinaafitz self-assigned this Oct 7, 2019

lfu approved these changes Oct 8, 2019

View reviewed changes

tinaafitz approved these changes Oct 8, 2019

View reviewed changes

gmcculloug reviewed Oct 8, 2019

View reviewed changes

pkomanek added 2 commits October 10, 2019 14:26

Eligible storages for ManageIQ/Infrastructure/VM/Provisioning/Placeme…

150229d

…nt methods.

Spec modifications to changed automate placement method.

3aa946d

pkomanek force-pushed the eligible_storages_for_vmware_best_fit_least_utilized branch from c3f4b2a to 3aa946d Compare October 10, 2019 12:26

gmcculloug added changelog/yes enhancement providers/provisioning providers/virtual labels Oct 10, 2019

gmcculloug merged commit 957bbd9 into ManageIQ:master Oct 10, 2019

gmcculloug added this to the Sprint 122 Ending Oct 14, 2019 milestone Oct 10, 2019

pkomanek deleted the eligible_storages_for_vmware_best_fit_least_utilized branch October 21, 2019 13:07

simaishi added the ivanchuk/yes label Feb 21, 2020

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eligible storages for placement methods #578

Eligible storages for placement methods #578

pkomanek commented Sep 10, 2019 •

edited by simaishi

Loading

coveralls commented Sep 10, 2019 •

edited

Loading

tinaafitz commented Oct 4, 2019

lfu Oct 7, 2019

pkomanek Oct 8, 2019

lfu Oct 8, 2019

pkomanek Oct 8, 2019

lfu Oct 7, 2019

pkomanek Oct 8, 2019

gmcculloug Oct 8, 2019

gmcculloug Oct 8, 2019

miq-bot commented Oct 10, 2019

miq-bot commented Oct 10, 2019

simaishi commented Feb 21, 2020

tinaafitz commented Feb 21, 2020

simaishi commented Feb 21, 2020

Eligible storages for placement methods #578

Eligible storages for placement methods #578

Conversation

pkomanek commented Sep 10, 2019 • edited by simaishi Loading

coveralls commented Sep 10, 2019 • edited Loading

Pull Request Test Coverage Report for Build 3671

💛 - Coveralls

tinaafitz commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 10, 2019

miq-bot commented Oct 10, 2019

simaishi commented Feb 21, 2020

tinaafitz commented Feb 21, 2020

simaishi commented Feb 21, 2020

pkomanek commented Sep 10, 2019 •

edited by simaishi

Loading

coveralls commented Sep 10, 2019 •

edited

Loading