Add option to turn off calling Rbac in MiqRequestWorkflow.

[lfu]

During validate_values and create_request, MiqReqestWorkflow does not need to call Rbac which may take a while to get back with result for a large DB.

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

[chessbyte]

/cc @kbrock

[gmcculloug]

@lfu We can simplify this change since validation does not need to refresh any of the fields, not just ones that call into rbac.

Also, I found two places in the UI where we can reduce calls here app/controllers/application_controller/miq_request_methods.rb#L553
and here
app/controllers/application_controller/miq_request_methods.rb#L565

They can be reduced and take advantage of the new flag to skip refreshing values:

-      @edit[:wf].get_all_fields(d).keys.each do |f|
-        field = @edit[:wf].get_field(f, d)
+      @edit[:wf].get_all_fields(d, false).each do |f, field|

Let's get together Monday morning and discuss.

[kbrock]

I've been sitting on this a little to think about it.

From the comments, I read: "security is slow, just turn it off"

I'm not sure if the code actually has that same story.
I agree that this is too slow. and needs to be fixed.
Just hope we can fix it without the "ignore security here" flag

[kbrock]

One thing of note. It may simplify if we reduce the number of layers of abstraction.
What we are doing is simple, run a query and put some security around it. Would be nice if the code reflected that rather than calling helpers that are abstracting the problem.

To speed this up, is it possible not convert into ids? And ultimately, not download the objects anyway. I'll use this as an example, and while my suggestions may not make sense here, keep them in mind in general.

    MiqPreloader.preload(hosts, :storages) # fetch all the storages into memory

    storages = hosts.each_with_object({}) do |host, hash|
      host.writable_storages.each { |s| hash[s.id] = s }
    end.values # unique list of writable storages?

    # fetch storages (from storage id and rbac in query)
    allowed_storages_cache = process_filter(:ds_filter, Storage, storages).collect do |s|
      # I had thought rbac would have converted to ids. So I'm confused here.
      ci_to_hash_struct(s)
    end
    # ids coming out - is the future code going to use the ids to convert back into objects again?

Can the hosts not be loaded in the first place? And not all the hosts? Maybe a sql group by or maybe it is something more like a simple select:

Storages.where(:host_id => hosts.select(:ids))

note: there is no to_a and this is not actually run. Rbac will tack on the necessary logic and THEN it will fetch the objects from the database once. (vs 3+ times)

I'm not sure the exact circumstances here, but it seems like it is bringing back too many host then too many storage records and then filtering (sql is good at this) and then converting into ids to then pass to rbac to fetch again? But I may be reading this incorrectly

[gmcculloug]

@kbrock I think you are misreading what I was trying to say. During dialog field validation we are making sure the selected value meets the requirements of the field. For example, if it is required it cannot be empty or if you select PXE provisioning you also need to select a PXE server. These validation only need the currently value for the field and do not need to re-run the logic to what other values are available.

What we are suggesting in this PR is to remove unnecessary calls. I think your suggesting is perfectly valid but something I would address in a separate PR.

[gmcculloug]

@lfu This UI call can also be reduced like the ones mentioned above:

manageiq/app/controllers/miq_request_controller.rb

Line 294 in 822181d

@edit[:wf].get_all_fields(d).keys.each do |f| # Go thru all field

[gmcculloug]

@lfu Is the validate_values method still needed? I do not see anyone calling it.

[miq-bot]

Checked commits lfu/manageiq@880f717~...4e42e50 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 3 offenses detected

app/models/miq_request_workflow.rb

• 🔹 - Line 308, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for get_field is too high. [29.27/15]
• 🔹 - Line 308, Col 3 - Metrics/CyclomaticComplexity - Cyclomatic complexity for get_field is too high. [11/6]
• 🔹 - Line 308, Col 3 - Metrics/PerceivedComplexity - Perceived complexity for get_field is too high. [12/7]

[gmcculloug]

Looks good. @lfu Can you comment on the performance improvements you are seeing with these changes for a user with rbac?

[lfu]

It took about 10 minutes to create a service template provisioning request from rails console with 10 belongs_to filters set for a certain group that the user belongs to.
With this commit, the time went down to slightly over 4 minutes.