Add skip_references option to Rbac/MiqReport #17469

NickLaMuro · 2018-05-23T16:05:31Z

This allows for the manual configuration of product reports to include an option to use skip_references to avoid LEFT JOIN bombs from happening when multiple includes are being referenced in Rbac.

This partially fixes the issue reported in https://bugzilla.redhat.com/show_bug.cgi?id=1580569, but will require an update to the manageiq-ui-classic to be fixed fully.

Background

The /ems_infra/:id?display=hosts route would pull up the product/views/Host.yaml from the manageiq-ui-classic report and run the following query:

-- Many rows here in the SELECT are omitted for space
SELECT "hosts"."id" AS t0_r0,
       "hosts"."name" AS t0_r1,
       -- ...
       "hosts"."physical_server_id" AS t0_r36,
       "ext_management_systems"."id" AS t1_r0,
       "ext_management_systems"."name" AS t1_r1,
       -- ...
       "ext_management_systems"."tenant_mapping_enabled" AS t1_r23,
       "compliances"."id" AS t2_r0,
       -- ...
       "compliances"."event_type" AS t2_r6,
       "tags"."id" AS t3_r0,
       "tags"."name" AS t3_r1
FROM "hosts"
LEFT OUTER JOIN "ext_management_systems" ON "ext_management_systems"."id" = "hosts"."ems_id"
LEFT OUTER JOIN "compliances" ON "compliances"."resource_id" = "hosts"."id" AND "compliances"."resource_type" = 'Host'
LEFT OUTER JOIN "taggings" ON "taggings"."taggable_id" = "hosts"."id" AND "taggings"."taggable_type" = 'Host'
LEFT OUTER JOIN "tags" ON "tags"."id" = "taggings"."tag_id"
ORDER BY LOWER("hosts"."name") ASC

With only 20 hosts, but around 14k compliances, the result set returned from above was over 2 million rows with 60+ columns per row. This caused ActiveRecord/pg gems to use Gigs of memory to process the query, and was taking over 25min for the query to be fully processed.

The change makes it so the query is done in a more SQL friendly fashion by executing the includes as individual SQL calls. Means more SQL calls over all, but only a increase of about 4, with drastically less data sent back over the wire (most of it previously was duplicate).

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1580569
UI PR (merge after this one): Use skip_references in product/views/Host.yaml manageiq-ui-classic#3984
Previous required PRs for this to work:
- Skip query references in Rbac when not needed #17141
- Fixes to Rbac::Filterer#skip_references #17429

Allows for manually triggering the `skip_references` option in Rbac for performance reasons. This can be used in specific situations where otherwise the autogenerated reference skipping would not be enabled.

Allows setting `skip_references` in report yaml files, but isn't something that we would commit to the DB. This is something that is an optimization that really won't make sense to an end user, so this fix simply allows reports in the `product/` folder to modified to be better optimized. No DB migration necessary.

Includes :skip_references into the options that are passed to Rbac.search in the MiqReport#paged_view_search method.

miq-bot · 2018-05-23T16:17:30Z

Checked commits NickLaMuro/manageiq@87be69d~...46be5a3 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

app/models/miq_report/search.rb

❗ - Line 97, Col 35 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the same line as the last argument when opening brace is on the same line as the first argument.

lib/rbac/filterer.rb

❗ - Line 314, Col 7 - Layout/MultilineOperationIndentation - Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

JPrause · 2018-05-24T13:32:52Z

@miq-bot add_label blocker

JPrause · 2018-05-24T13:34:08Z

@NickLaMuro if this can be backported, can you add the labels; gaprindashvili/yes and fine/yes

NickLaMuro · 2018-05-24T16:50:05Z

@JPrause I am actually going to remove the blocker label since we have a much lower risk fix for the BZ in the UI classic repo: ManageIQ/manageiq-ui-classic#3989

I will update the fine/yes and gaprindashvili/yes labels later when this becomes something we are focusing on later.

@miq-bot remove_label blocker

kbrock · 2018-10-18T18:13:50Z

@NickLaMuro do want to merge this to have in our future toolbox?

unless I missed something, it looks innocuous unless we pass the label. I'm thinking hammer/yes but all else no

NickLaMuro · 2018-11-13T23:57:51Z

@kbrock I realize this is a month later, but I think we can merge this and expect to use it as part of the ivanchuk release. We can hammer backport if a BZ comes up.

But should be completely harmless to merge, and yes, helpful as another thing to use in our bugfix toolbox.

...though, had to remind myself how this worked again and how it was helpful in the first place 😅

NickLaMuro · 2018-11-14T17:32:09Z

@miq-bot assign @kbrock

NickLaMuro · 2018-11-14T17:32:36Z

@miq-bot add_label enhancement, hammer/no

kbrock

Looking good. just the one question

Do we need specs for this?

kbrock · 2018-11-15T16:56:37Z

lib/rbac/filterer.rb

@@ -310,8 +310,9 @@ def is_sti?(klass)
    # as done previously.
    def skip_references?(scope, options, attrs, exp_sql, exp_includes)
      return false if scope.singleton_class.included_modules.include?(ActiveRecord::NullRelation)
-      options[:extra_cols].blank? &&
-        (!attrs[:apply_limit_in_sql] && (exp_sql.nil? || exp_includes.nil?))
+      options[:skip_references] ||


do we want this to be true or false? or were you thinking only true

Suggested change

options[:skip_references] ||

options.key?(:skip_references) ? options[:skip_references] :

@kbrock well, to say:

...or were you thinking only true

Is slightly misleading, because you can pass true or false/nil with this currently, just false/nil will just defer to the other logic. But I do get what you are getting at.

I think for now, I would like to keep it as is just to keep the fail safe check, just to make sure that anyone that uses this doesn't remove references when they are actually needed in the where. I guess that could lead to it being confusing, but I guess for the most part, I assume that (as you suggested) the true will be the only value that is used.

I guess this is a round-about way of saying I don't have a real strong feeling on this one, so what do you think?

NickLaMuro added 3 commits May 23, 2018 10:58

Add skip_references option to Rbac

87be69d

Allows for manually triggering the `skip_references` option in Rbac for performance reasons. This can be used in specific situations where otherwise the autogenerated reference skipping would not be enabled.

Add skip_references to paged_view_search

46be5a3

Includes :skip_references into the options that are passed to Rbac.search in the MiqReport#paged_view_search method.

NickLaMuro mentioned this pull request May 23, 2018

Use skip_references in product/views/Host.yaml ManageIQ/manageiq-ui-classic#3984

Closed

This was referenced May 23, 2018

Add has_one scope support to virtual_delegate #17473

Merged

Convert ComplianceMixin to use has_one/virtual_delegate #17475

Merged

miq-bot added the blocker label May 24, 2018

miq-bot removed the blocker label May 24, 2018

miq-bot assigned kbrock Nov 14, 2018

miq-bot added enhancement hammer/no labels Nov 14, 2018

kbrock reviewed Nov 15, 2018

View reviewed changes

kbrock merged commit 99dca1f into ManageIQ:master Nov 21, 2018

mfeifer added this to the Sprint 100 Ending Dec 3, 2018 milestone Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skip_references option to Rbac/MiqReport #17469

Add skip_references option to Rbac/MiqReport #17469

NickLaMuro commented May 23, 2018 •

edited

miq-bot commented May 23, 2018

JPrause commented May 24, 2018

JPrause commented May 24, 2018

NickLaMuro commented May 24, 2018

kbrock commented Oct 18, 2018 •

edited

NickLaMuro commented Nov 13, 2018

NickLaMuro commented Nov 14, 2018

NickLaMuro commented Nov 14, 2018

kbrock left a comment

kbrock Nov 15, 2018

NickLaMuro Nov 19, 2018 •

edited

	options[:skip_references] \|\|
	options.key?(:skip_references) ? options[:skip_references] :

Add skip_references option to Rbac/MiqReport #17469

Add skip_references option to Rbac/MiqReport #17469

Conversation

NickLaMuro commented May 23, 2018 • edited

Background

Links

miq-bot commented May 23, 2018

JPrause commented May 24, 2018

JPrause commented May 24, 2018

NickLaMuro commented May 24, 2018

kbrock commented Oct 18, 2018 • edited

NickLaMuro commented Nov 13, 2018

NickLaMuro commented Nov 14, 2018

NickLaMuro commented Nov 14, 2018

kbrock left a comment

Choose a reason for hiding this comment

kbrock Nov 15, 2018

Choose a reason for hiding this comment

NickLaMuro Nov 19, 2018 • edited

Choose a reason for hiding this comment

NickLaMuro commented May 23, 2018 •

edited

kbrock commented Oct 18, 2018 •

edited

NickLaMuro Nov 19, 2018 •

edited