Description of problem: The UI is unable to render a large report in browser Version-Release number of selected component (if applicable): 5.9.2 or 5.8.2.3-8 (hotfix release) How reproducible: Always Steps to Reproduce: 1. Click on large report 2. 3. Actual results: UI spins forever never loading it Expected results: UI generates report successfully Additional info: When rendering, top command shows a ruby process taking 100% of one core. It doesn't seem to branch out to another process.
https://github.com/ManageIQ/manageiq/pull/17589
I have add two patches, when both are merged, should significantly improve the speed on these pages: * https://github.com/ManageIQ/manageiq/pull/17590 * https://github.com/ManageIQ/manageiq-ui-classic/pull/4143 When combined with Joe's PR from above: https://github.com/ManageIQ/manageiq/pull/17589 We hope should fix most of the slowness being experienced on these pages. However, we are going to do some further testing to confirm.
https://github.com/ManageIQ/manageiq/pull/17598
New commits detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/6b2e57d1af83e7dcd1229a451b2d11110a988def commit 6b2e57d1af83e7dcd1229a451b2d11110a988def Author: Nick LaMuro <nicklamuro> AuthorDate: Thu Jun 14 17:26:37 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Thu Jun 14 17:26:37 2018 -0400 Add MiqReportResult#valid_report_column? https://bugzilla.redhat.com/show_bug.cgi?id=1590908 A quicker check than having to use `MiqReportResult#report_results`, since binary_blob doesn't need to be loaded into memory. On top of that, generally the binary_blob.data isn't even used in the controller at the end of the day, and this version of the MiqReport record ends up being what is used when generating the page for viewing. app/models/miq_report_result.rb | 7 + 1 file changed, 7 insertions(+) https://github.com/ManageIQ/manageiq/commit/3a26db0c3dbf44411a0f03899721b5049c388650 commit 3a26db0c3dbf44411a0f03899721b5049c388650 Author: Nick LaMuro <nicklamuro> AuthorDate: Thu Jun 14 17:28:46 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Thu Jun 14 17:28:46 2018 -0400 Add MiqReportResult#contains_records? https://bugzilla.redhat.com/show_bug.cgi?id=1590908 Like `MiqReportResult#has_report?`, this is a faster version of checking if the report_result has rows. Like `MiqReport#contains_records?`, this will check against the `extras` attribute on the deserialized version of the `report` column, but instead of using the `table` value (which is not saved into the `report` column), just do a light SQL existence check on the `html_details` to confirm there are generated rows for this report. The SQL query most likely won't be needed, and it is super light weight, only returning a single digit, or an empty result set if there are no records. app/models/miq_report_result.rb | 7 + 1 file changed, 7 insertions(+)
New commits detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/8b4fe076e39917f2d7f418c58e1b2354af5a78d9 commit 8b4fe076e39917f2d7f418c58e1b2354af5a78d9 Author: Joe Rafaniello <jrafanie> AuthorDate: Thu Jun 14 17:15:41 2018 -0400 Commit: Joe Rafaniello <jrafanie> CommitDate: Thu Jun 14 17:15:41 2018 -0400 Add tool to remove grouping from report results https://bugzilla.redhat.com/show_bug.cgi?id=1590908 Some aggregation information was persisted into the miq_report_results table in the report column's `extras[:grouping]` section. We don't need the grouping after we've generated the html data. This tool removes this from each row in the table. The default options will process the whole table in batches of 50 at a time and will persist all changes. If you run it with -h, you'll receive a help banner: ``` Remove extras[:grouping] from MiqReportResult#report column. Usage: ruby tools/remove_grouping_from_report_results.rb [options] Options: --dry-run For testing, rollback any changes when the script exits. -b, --batch-size=<i> Limit memory usage by process this number of report results at a time. (Default: 50) -c, --count=<i> Stop checking after this number of report results. (Default: 0) -h, --help Show this message ``` Example output looks like this: ``` ruby tools/remove_grouping_from_report_results.rb -c 11 Using options: {:dry_run=>false, :batch_size=>50, :count=>11, :help=>false, :count_given=>true} ** Using session_store: ActionDispatch::Session::MemCacheStore MiqReportResult: 22000000000001 doesn't need fixing MiqReportResult: 22000000000002 doesn't need fixing MiqReportResult: 22000000000003 fixed MiqReportResult: 22000000000004 doesn't need fixing MiqReportResult: 22000000000005 doesn't need fixing MiqReportResult: 22000000000006 fixed MiqReportResult: 22000000000007 doesn't need fixing MiqReportResult: 22000000000008 doesn't need fixing MiqReportResult: 22000000000009 doesn't need fixing MiqReportResult: 22000000000010 doesn't need fixing MiqReportResult: 22000000000011 fixed Processed 11 rows. 1 were fixed. 13.170998 seconds ``` tools/remove_grouping_from_report_results.rb | 52 + 1 file changed, 52 insertions(+) https://github.com/ManageIQ/manageiq/commit/51be17337f3be50af8810a38e3b143186f4940c5 commit 51be17337f3be50af8810a38e3b143186f4940c5 Author: Joe Rafaniello <jrafanie> AuthorDate: Fri Jun 15 09:55:46 2018 -0400 Commit: Joe Rafaniello <jrafanie> CommitDate: Fri Jun 15 09:55:46 2018 -0400 Wrap in a transaction, rollback on dry-run or error https://bugzilla.redhat.com/show_bug.cgi?id=1590908 tools/remove_grouping_from_report_results.rb | 43 +- 1 file changed, 27 insertions(+), 16 deletions(-)
New commit detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/253cf7a55f89891c0e0b34be25c699e56e2f0854 commit 253cf7a55f89891c0e0b34be25c699e56e2f0854 Author: Nick LaMuro <nicklamuro> AuthorDate: Fri Jun 15 17:47:47 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Fri Jun 15 17:47:47 2018 -0400 Add save hooks on MiqReportResult to remove groupings https://bugzilla.redhat.com/show_bug.cgi?id=1590908 The `report.extras[:groupings]` on MiqReportResult were being serialized to the `report` column, and on certain chargeback reports, can get quite large. This data is not needed, so we remove it prior to saving. The `after_commit` exists in case there is a use for the saved data after it has been saved and still used within the original instance of the MiqReportResult for building the result. Most likely this is overkill, but for now it is in place as a safety measure. Also, `after_commit` is used because a `after_save` will cause an "stack level too deep" error since editing the report value causes the record to be put into a `dirty` state, and the ActiveRecord internals don't like that. app/models/miq_report_result.rb | 14 + spec/models/miq_report_result_spec.rb | 17 + 2 files changed, 31 insertions(+)
New commit detected on ManageIQ/manageiq/master: https://github.com/ManageIQ/manageiq/commit/880f2ba4f8579e15882d204a4cad48e37d0844e3 commit 880f2ba4f8579e15882d204a4cad48e37d0844e3 Author: Nick LaMuro <nicklamuro> AuthorDate: Mon Jun 18 21:25:36 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Mon Jun 18 21:25:36 2018 -0400 Add `nil` check for report.extras on save hooks https://bugzilla.redhat.com/show_bug.cgi?id=1590908 app/models/miq_report_result.rb | 4 +- 1 file changed, 2 insertions(+), 2 deletions(-)
New commits detected on ManageIQ/manageiq-ui-classic/master: https://github.com/ManageIQ/manageiq-ui-classic/commit/ff7ee24f4d50b38e96d5c0eccd8ca7398d6d310e commit ff7ee24f4d50b38e96d5c0eccd8ca7398d6d310e Author: Nick LaMuro <nicklamuro> AuthorDate: Thu Jun 14 17:43:10 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Thu Jun 14 17:43:10 2018 -0400 Prefer valid_report_column? and contains_records? from MiqReportResult https://bugzilla.redhat.com/show_bug.cgi?id=1590908 These are new methods added in: https://github.com/ManageIQ/manageiq/pull/17590 And are far more efficient for the use cases of ChargebackController and ReportController::SavedReports. app/controllers/chargeback_controller.rb | 5 +- app/controllers/report_controller/saved_reports.rb | 5 +- spec/controllers/chargeback_controller_spec.rb | 3 +- spec/controllers/miq_report_controller/trees_spec.rb | 16 +- spec/controllers/report_controller_spec.rb | 10 +- 5 files changed, 24 insertions(+), 15 deletions(-) https://github.com/ManageIQ/manageiq-ui-classic/commit/b5260fd3dff13c9aa0cf7593518bfb557c07c420 commit b5260fd3dff13c9aa0cf7593518bfb557c07c420 Author: Nick LaMuro <nicklamuro> AuthorDate: Thu Jun 14 18:34:52 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Thu Jun 14 18:34:52 2018 -0400 Prefer `rr.report` over `rr.report_results` in SavedReportPaging mixin https://bugzilla.redhat.com/show_bug.cgi?id=1590908 This not only makes it so we are consistent between the main controller and the mixin, but this is also much faster to use this over `MiqReportResult#report_results` when there is a large `binary_blob` associated with the `MiqReportResult` record. app/controllers/mixins/saved_report_paging.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) https://github.com/ManageIQ/manageiq-ui-classic/commit/cb118f0c34462eca296e460d7b01cee29b51066c commit cb118f0c34462eca296e460d7b01cee29b51066c Author: Nick LaMuro <nicklamuro> AuthorDate: Thu Jun 14 17:52:53 2018 -0400 Commit: Nick LaMuro <nicklamuro> CommitDate: Thu Jun 14 17:52:53 2018 -0400 Prefer `.exists?` over `.length?` in report button helpers https://bugzilla.redhat.com/show_bug.cgi?id=1590908 Calls to `.length` will fetch the entire record set or rows for the MiqReportResult, and in the current case of these button helpers, throw them all away without using any of the data fetched. In most cases, the `html_details` relation is probably what was is being used, so even caching these ahead of time is wasteful. By using `.exists?` as an alternative, we basically do a: SELECT 1 as one FROM miq_report_result_details WHERE "miq_report_result_details"."miq_report_result_id" = ? LIMIT 1 Which only returns a single digit from the database if it has at least one record and nothing if it doesn't. This will also be cached in a controller action by the ActiveRecord query cache, so it will also require zero queries the second time around. app/helpers/application_helper/button/report_download_choice.rb | 2 +- app/helpers/application_helper/button/report_only.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
Moving to POST since alls PRs are merged: https://github.com/ManageIQ/manageiq/pull/17589 (tool to cleanup miq_report_results table) https://github.com/ManageIQ/manageiq/pull/17590 (new faster methods added to core for use by the UI) https://github.com/ManageIQ/manageiq-ui-classic/pull/4143 (faster index pages for reports results using prior PR's methods) https://github.com/ManageIQ/manageiq/pull/17598 (add save hooks to ensure we don't save the unneeded chargeback aggregation into new miq_report_results rows) https://github.com/ManageIQ/manageiq/pull/17605 (fix for prior save hooks PR)
Hi David, is a hotfix needed for 5.9 or 5.8? It's unclear by the description, which version the customer will be using. If 5.8, we need to add backports for them.
5.8
Verified that the code is present in 5.10.0.4