Convert ComplianceMixin to use has_one/virtual_delegate #17475

NickLaMuro · 2018-05-23T23:37:30Z

~~Built off of: #17473~~ Now Merged

This converts the functionality defined in the virtual_has_one, virtual_columns, and associated methods to a has_one and a pair of virtual_delegates.

This allows these columns to also be used in SQL, and removes some of the boilerplate code necessary for the methods that are defined.

This allows the changes in #17474 to then be used on the Compute -> Infrastructure -> Host page, among others, and circumvents needing all of the compliance records to be downloaded to get the last_compliance_status for those reports.

Metrics

An example set of runs I had with a 50 Host provider:

Before

ms	queries	query (ms)	rows
835	163	252.5	3810
782	163	164.7	3810
464	163	165.9	3810

After

ms	queries	query (ms)	rows
3061	113	172.5	3810
742	113	166.1	3810
815	114	155.0	3811

Links

For https://bugzilla.redhat.com/show_bug.cgi?id=1580569
Built off of Add has_one scope support to virtual_delegate #17473 (merged)
Related Allow MiqReport.paged_view_search to take advantage of Rbac :extra_cols #17474
Alternative to Add skip_references option to Rbac/MiqReport #17469 and Use skip_references in product/views/Host.yaml manageiq-ui-classic#3984

Steps for Testing/QA

Need to build up some seed data, but a before and after with all of these changes in place makes it so that when a large number of compliance reports exists for a small number of hosts (even when it is something like 20...), there isn't a LEFT JOIN bomb to build the report data (due to the .includes/.references calls in Rbac).

Here is some test data that I was using:

https://gist.github.com/NickLaMuro/225833358423723ed17ff294415fa6b4

For testing, I did the following:

Seed with the above script: bin/rails r bz_1580569_db_replication_script.rb
Spin up a server: bin/rails s
Do the following in a browser:
- Login
- Navigate to Compute -> Infrastructure -> Providers in the side menu
- Up the per_page to 1k (just to load all of the records in a single page
- Select a provider (one with the most Host records, ideally)
- Select the Hosts button

Between master, and this change, you should see the number of queries per request for the last portion in the above drop by the number of Host records for that provider. The Metrics section above used this process.

kbrock

This is very promising / sweet.

Let me know when the tests are in.

kbrock · 2018-05-24T17:13:55Z

app/models/mixins/compliance_mixin.rb

+
+    virtual_delegate :last_compliance_status, 
+                     :to        => "last_compliance.compliant",
+                     :type      => :boolean,


pretty sure :type not required (for either of these)

kbrock · 2018-08-01T19:55:00Z

@NickLaMuro could you rebase to get rid of the first commit?

Possibly drop the :type values too?

NickLaMuro · 2018-08-01T20:14:25Z

When this becomes a priority or I have free time, sure.

Update: Added the [WIP] flag to this PR while I wait to get some spare cycles to update it (currently working on a branch that I don't want to "un-checkout" to do). Will remove the flag when I am ready for another look. Thanks.

kbrock · 2018-11-14T13:49:14Z

@NickLaMuro is this still a WIP? (it is good stuff)

This converts the functionality defined in the virtual_has_one, virtual_columns, and associated methods to a has_one and a pair of virtual_delegates. This allows these columns to also be used in SQL, and removes some of the boilerplate code necessary for the methods that are defined.

NickLaMuro · 2018-11-14T22:23:38Z

@NickLaMuro is this still a WIP?

@kbrock yes, it was. Was getting some specs together (via some blatant plagiarism of your specs from #18198), as well as putting together some metrics and reproduction steps in place.

But now it is not.

miq-bot · 2018-11-14T22:25:51Z

Checked commit NickLaMuro@f6a75b0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

kbrock

This looks great. I kicked one test - looked like a sporadic failure. I will merge once it goes green.

NickLaMuro · 2018-11-14T23:50:09Z

I kicked one test - looked like a sporadic failure. I will merge once it goes green.

Thanks for catching that!

himdel · 2018-11-16T13:52:10Z

This PR causes ui-classic travis to go red:

  1) ComplianceSummaryHelper when @explorer is set #textual_compliance_status
     Failure/Error: {:time => time_ago_in_words(date.in_time_zone(Time.zone)).titleize}
     NoMethodError:
       undefined method `in_time_zone' for nil:NilClass
     # ./app/helpers/compliance_summary_helper.rb:16:in `textual_compliance_status'
     # ./spec/helpers/compliance_summary_helper_spec.rb:22:in `block (3 levels) in <top (required)>'
  2) ComplianceSummaryHelper for classic screens when @explorer is not set #textual_compliance_status
     Failure/Error: {:time => time_ago_in_words(date.in_time_zone(Time.zone)).titleize}
     NoMethodError:
       undefined method `in_time_zone' for nil:NilClass
     # ./app/helpers/compliance_summary_helper.rb:16:in `textual_compliance_status'
     # ./spec/helpers/compliance_summary_helper_spec.rb:50:in `block (3 levels) in <top (required)>'

seems that..

@record.number_of(:compliances)
1

@record.compliances.first.compliant
true

@record.last_compliance_status
nil

(mentioning ManageIQ/manageiq-ui-classic#4921)

Over time, the `compliances` table will get quiet large, and when paired with the rest of the tables in this report, you end up getting a query executed that looks like this: SELECT "vms"."id" AS t0_r0, ..., "vms"."memory_hot_add_increment" AS t0_r75, "hosts"."id" AS t1_r0, ..., "hosts"."physical_server_id" AS t1_r36, "storages"."id" AS t2_r0, ..., "storages"."storage_domain_type" AS t2_r18, "ext_management_systems"."id" AS t3_r0, ..., "ext_management_systems"."tenant_mapping_enabled" AS t3_r23, "snapshots"."id" AS t4_r0, ..., "snapshots"."ems_ref" AS t4_r16, "compliances"."id" AS t5_r0, ..., "compliances"."event_type" AS t5_r6, "operating_systems"."id" AS t6_r0, ..., "operating_systems"."kernel_version" AS t6_r25, "hardwares"."id" AS t7_r0, ..., "hardwares"."provision_state" AS t7_r34, "tags"."id" AS t8_r0, "tags"."name" AS t8_r1 FROM "vms" LEFT OUTER JOIN "hosts" ON "hosts"."id" = "vms"."host_id" LEFT OUTER JOIN "storages" ON "storages"."id" = "vms"."storage_id" LEFT OUTER JOIN "ext_management_systems" ON "ext_management_systems"."id" = "vms"."ems_id" LEFT OUTER JOIN "snapshots" ON "snapshots"."vm_or_template_id" = "vms"."id" LEFT OUTER JOIN "compliances" ON "compliances"."resource_id" = "vms"."id" AND "compliances"."resource_type" = $1 LEFT OUTER JOIN "operating_systems" ON "operating_systems"."vm_or_template_id" = "vms"."id" LEFT OUTER JOIN "hardwares" ON "hardwares"."vm_or_template_id" = "vms"."id" LEFT OUTER JOIN "taggings" ON "taggings"."taggable_id" = "vms"."id" AND "taggings"."taggable_type" = $2 LEFT OUTER JOIN "tags" ON "tags"."id" = "taggings"."tag_id" WHERE "vms"."type" IN (...) AND "vms"."template" = $3 AND (lower(vms.name) like '%win2k%' escape '`') AND "vms"."id" IN (...) ORDER BY LOWER("vms"."name") ASC This ends up creating 243 columns per record, and the number of rows returned has been observed to be over 80k with just 20 VMs being targeted in the `"vms"."id" IN (...)` portion of the query. Since some fixes have been made to avoid the N+1 that results from doing this: - ManageIQ/manageiq#17473 - ManageIQ/manageiq#17474 - ManageIQ/manageiq#17475 It is much better to do use those facilities. Even with the N+1, it is much better making extra round trips than the gigs of data that would be returned as a result.

kbrock · 2019-07-31T23:26:04Z

related to https://bugzilla.redhat.com/show_bug.cgi?id=1733351

but please don't merge yet until we verify this

kbrock · 2019-08-01T17:28:43Z

@simaishi I have verified that this PR does indeed removed the join to compliances in Hammer

simaishi · 2019-08-01T17:52:14Z

@kbrock Please add a comment regarding this PR in the BZ. I don't look for hammer/yes PRs, but look at BZs to determine what PR is needed and if that's hammer/yes. So unless a BZ mentions a PR, it won't be picked up for backport...

…iance_to_virtual_delegate Convert ComplianceMixin to use has_one/virtual_delegate (cherry picked from commit 391cf5d) https://bugzilla.redhat.com/show_bug.cgi?id=1738266

simaishi · 2019-08-14T16:27:31Z

Hammer backport details:

$ git log -1
commit b449f18cb2d3cabca81faba5503aeed583cc8ec0
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Thu Nov 15 11:53:56 2018 -0500

    Merge pull request #17475 from NickLaMuro/compliance_mixin_last_compliance_to_virtual_delegate
    
    Convert ComplianceMixin to use has_one/virtual_delegate
    
    (cherry picked from commit 391cf5d3c214f517086ab2cc5df1a2ea074ef472)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1738266

This was referenced May 23, 2018

Add has_one scope support to virtual_delegate #17473

Merged

Remove compliances include from Host.yaml ManageIQ/manageiq-ui-classic#3989

Merged

kbrock reviewed May 24, 2018

View reviewed changes

NickLaMuro changed the title ~~Convert ComplianceMixin to use has_one/virtual_delegate~~ [WIP] Convert ComplianceMixin to use has_one/virtual_delegate Aug 1, 2018

miq-bot added the wip label Aug 1, 2018

NickLaMuro force-pushed the compliance_mixin_last_compliance_to_virtual_delegate branch from adfbfb5 to 890dcd7 Compare November 14, 2018 00:37

NickLaMuro mentioned this pull request Nov 14, 2018

[AuthenticationMixin] authentication_status to virtual_delegate #18196

Merged

NickLaMuro force-pushed the compliance_mixin_last_compliance_to_virtual_delegate branch from 890dcd7 to c1a57a2 Compare November 14, 2018 22:10

NickLaMuro force-pushed the compliance_mixin_last_compliance_to_virtual_delegate branch from c1a57a2 to f6a75b0 Compare November 14, 2018 22:21

NickLaMuro changed the title ~~[WIP] Convert ComplianceMixin to use has_one/virtual_delegate~~ Convert ComplianceMixin to use has_one/virtual_delegate Nov 14, 2018

miq-bot removed the wip label Nov 14, 2018

kbrock approved these changes Nov 14, 2018

View reviewed changes

NickLaMuro mentioned this pull request Nov 15, 2018

Use v_total_vms for HostDecorator ManageIQ/manageiq-ui-classic#4923

Merged

kbrock self-assigned this Nov 15, 2018

kbrock added this to the Sprint 99 Ending Nov 19, 2018 milestone Nov 15, 2018

kbrock added performance core/reporting labels Nov 15, 2018

kbrock merged commit 391cf5d into ManageIQ:master Nov 15, 2018

himdel mentioned this pull request Nov 16, 2018

Broken travis from other repos ManageIQ/manageiq-ui-classic#4921

Closed

himdel mentioned this pull request Nov 16, 2018

Fix compliance travis - save records after changing compliances ManageIQ/manageiq-ui-classic#4933

Merged

NickLaMuro mentioned this pull request Feb 27, 2019

[Performance] Remove compliance from VmOrTemplate includes ManageIQ/manageiq-ui-classic#5283

Merged

kbrock added the hammer/yes label Jul 31, 2019

simaishi added hammer/backported and removed hammer/yes labels Aug 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert ComplianceMixin to use has_one/virtual_delegate #17475

Convert ComplianceMixin to use has_one/virtual_delegate #17475

NickLaMuro commented May 23, 2018 •

edited

Loading

kbrock left a comment

kbrock May 24, 2018 •

edited

Loading

kbrock commented Aug 1, 2018

NickLaMuro commented Aug 1, 2018 •

edited

Loading

kbrock commented Nov 14, 2018

NickLaMuro commented Nov 14, 2018

miq-bot commented Nov 14, 2018

kbrock left a comment

NickLaMuro commented Nov 14, 2018

himdel commented Nov 16, 2018

kbrock commented Jul 31, 2019 •

edited

Loading

kbrock commented Aug 1, 2019

simaishi commented Aug 1, 2019

simaishi commented Aug 14, 2019

Convert ComplianceMixin to use has_one/virtual_delegate #17475

Convert ComplianceMixin to use has_one/virtual_delegate #17475

Conversation

NickLaMuro commented May 23, 2018 • edited Loading

Metrics

Links

Steps for Testing/QA

kbrock left a comment

Choose a reason for hiding this comment

kbrock May 24, 2018 • edited Loading

Choose a reason for hiding this comment

kbrock commented Aug 1, 2018

NickLaMuro commented Aug 1, 2018 • edited Loading

kbrock commented Nov 14, 2018

NickLaMuro commented Nov 14, 2018

miq-bot commented Nov 14, 2018

kbrock left a comment

Choose a reason for hiding this comment

NickLaMuro commented Nov 14, 2018

himdel commented Nov 16, 2018

kbrock commented Jul 31, 2019 • edited Loading

kbrock commented Aug 1, 2019

simaishi commented Aug 1, 2019

simaishi commented Aug 14, 2019

NickLaMuro commented May 23, 2018 •

edited

Loading

kbrock May 24, 2018 •

edited

Loading

NickLaMuro commented Aug 1, 2018 •

edited

Loading

kbrock commented Jul 31, 2019 •

edited

Loading