Add tool to remove grouping from report results #17589

jrafanie · 2018-06-14T21:30:12Z

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

gtanzillo · 2018-06-14T21:37:46Z

tools/remove_grouping_from_report_results.rb

+    rr.report.extras = report.extras.except!(:grouping)
+    rr.report.save
+    rr.save
+    unless rr.reload.report.extras.key?(:grouping)


Is it really necessary to reload here? Perhaps you can do this instead?

unless rr.save ...

My 2 cents: I don't mind the verbosity of reloading this from the database to confirm it is changed there (since it being fixed there is what matters most).

Honestly, I was thinking of not doing reload due to the performance problem, but while developing this, I was concerned that I wasn't really saving the correct value back into the database. Therefore, I thought it was worthwhile to pay a little overhead to ensure we are actually doing the right thing.

NickLaMuro

Looks good. Have a few things of concern that you might want to look at, specifically the rr.report.save section.

Other than that, I don't really mind the verbose file name (the one that is currently in the PR description, not the current one). I think keeping it as a super crappy long name will give use a small push to remove it from the repo and make it a migration.

NickLaMuro · 2018-06-14T22:56:22Z

tools/remove_grouping_from_report_results.rb

+
+puts "Using options: #{opts.inspect}"
+
+# Load rails after trollop options are set.  No one wants to wait for -h.


You are a developer after my own heart. 😍

I waited twice and got so I changed it

NickLaMuro · 2018-06-14T22:58:31Z

tools/remove_grouping_from_report_results.rb

+
+  if report.extras.key?(:grouping)
+    rr.report.extras = report.extras.except!(:grouping)
+    rr.report.save


I don't think you want this. The id for this should be nil, and this should just be a copy of that record. Doing a .save here would be doing a CREATE basically, right?

Yeah, good point. I wrote this at first and didn't come back to it. Now that it's easy to test and use, I can verify each line is needed and ✂️ 👊 🚽 anything that doesn't make the cut.

NickLaMuro · 2018-06-14T22:59:35Z

tools/remove_grouping_from_report_results.rb

+require File.expand_path('../config/environment', __dir__)
+if opts[:dry_run]
+  puts "Running in dry-run, changes will be rolled back when complete."
+  ActiveRecord::Base.connection.begin_transaction(joinable: false)


Do we want to be doing this regardless if we are doing a dry run? Maybe we should be doing .save! as well so if it fails, we rollback and can do some investigation.

Ah, interesting point. What if you do the whole table in one invocation? Will that blow up the transaction log? I'm thinking there's thousands of rows typically, not tens or hundreds of thousands.

NickLaMuro · 2018-06-14T23:01:00Z

tools/remove_grouping_from_report_results.rb

+  next if report.nil? || report.extras.nil?
+
+  if report.extras.key?(:grouping)
+    rr.report.extras = report.extras.except!(:grouping)


Pendantic: If you wanted to use less code, you could just do a rr.report.extras.delete(:grouping).

Truth. I'll fix.

NickLaMuro · 2018-06-14T23:02:53Z

tools/remove_grouping_from_report_results.rb

@@ -0,0 +1,47 @@
+#!/usr/bin/env ruby
+require 'trollop'


require 'optparse' for lyfe!!1!

I'm waiting for @kbrock to rename trollor to optimal or whatever we decided. 🕐

I always favor stdlib things when possible, so that was what my comment was all about.

NickLaMuro · 2018-06-14T23:04:12Z

tools/remove_grouping_from_report_results.rb

+#!/usr/bin/env ruby
+require 'trollop'
+
+ARGV.shift if ARGV[0] == '--' # if invoked with rails runner


Uh, who would really want to deal with a slow -h to do this...

Also, can this work like that? I have had troubles in the past requiring config/environment twice in one script, but maybe this will be fine.

This is a thing we've done since the dawn of time for tools. I suppose it's fine to not do this and expect to be run with ruby.

NickLaMuro · 2018-06-14T23:05:27Z

tools/remove_grouping_from_report_results.rb

+fixed = 0
+MiqReportResult.find_each(batch_size: opts[:batch_size]).with_index do |rr, i|
+  break if opts[:count] > 0 && i == opts[:count]
+  report = rr.report


Pedantic: Is this really needed? You dance between using both rr.report and report below, and the omission of rr. isn't really saving you much on character count.

🕺 yeah. I wasn't in love with this either. I'll rework it.

NickLaMuro · 2018-06-14T23:06:42Z

tools/remove_grouping_from_report_results.rb

+    rr.report.extras = report.extras.except!(:grouping)
+    rr.report.save
+    rr.save
+    unless rr.reload.report.extras.key?(:grouping)


My 2 cents: I don't mind the verbosity of reloading this from the database to confirm it is changed there (since it being fixed there is what matters most).

NickLaMuro · 2018-06-14T23:22:55Z

@jrafanie tsk tsk. Using non-hashrocket syntax...

You monster...

jrafanie · 2018-06-15T11:50:54Z

@jrafanie tsk tsk. Using non-hashrocket syntax...

🙈

jrafanie · 2018-06-15T13:44:24Z

@jrafanie tsk tsk. Using non-hashrocket syntax...

You monster...

I don't know what you're saying, there's no 1.9 syntax here. 😉

jrafanie · 2018-06-15T14:10:02Z

tools/remove_grouping_from_report_results.rb

+
+if defined?(Rails)
+  puts "Warning: Rails is already loaded!  Please do not invoke using rails runner. Exiting with help text.\n\n"
+  Trollop.educate


@NickLaMuro changed the -- handling to just warn, print the help text and exit. Let me know if this is overkill.

Works for me. I didn't realize what you had was standard for the tools/ dir, so it was more of a "is this okay" out of naivety than anything else.

jrafanie · 2018-06-15T14:10:46Z

tools/remove_grouping_from_report_results.rb

+
+    if rr.report.extras.key?(:grouping)
+      rr.report.extras.delete(:grouping)
+      rr.save!


@NickLaMuro I am using save! now and rescuing on error.

jrafanie · 2018-06-15T14:16:01Z

tools/remove_grouping_from_report_results.rb

+opts = Trollop.options do
+  banner "Remove extras[:grouping] from MiqReportResult#report column.\n\nUsage: ruby #{$PROGRAM_NAME} [options]\n\nOptions:\n\t"
+  opt :dry_run,    "For testing, rollback any changes when the script exits.", :short => :none, :default => false
+  opt :batch_size, "Limit memory usage by process this number of report results at a time.",    :default => 50


Does 50 batch size make sense? Considering the size of these, I wanted extra queries instead of possibly bloating the memory of the process.

I think so. The deserialization is probably going to hit us harder than the queries time themselves. I don't see this as being an issue with N+1's.

jrafanie · 2018-06-15T14:33:58Z

⚠️ - Line 1, Col 1 - Lint/ScriptPermission - Script file remove_grouping_from_report_results.rb doesn't have execute permission.

You crazy bot

diff --git a/tools/remove_grouping_from_report_results.rb b/tools/remove_grouping_from_report_results.rb
new file mode 100755

Style/RescueStandardError - Avoid rescuing without specifying an error class

Bot, these aren't the complaints you're looking for.

NickLaMuro

This looks good on my end. I assume that everything is working as expected in testing it as well, so I think we can ship.

The comments that I made along with this are not worth holding up the review.

NickLaMuro · 2018-06-15T16:18:55Z

tools/remove_grouping_from_report_results.rb

+
+    total += 1
+  rescue => err
+    puts "\nWarning: Rolling back all changes since an error occurred on MiqReportResult with id: #{rr.try(:id)}: #{err.message}"


Pandantic: Should this be warn instead of puts?

yeah, I had warn but we don't warn elsewhere. 🤷‍♂️

NickLaMuro · 2018-06-15T16:19:18Z

tools/remove_grouping_from_report_results.rb

+    next if rr.report.nil? || rr.report.extras.nil?
+
+    if rr.report.extras.key?(:grouping)
+      rr.report.extras.except!(:grouping)


I may have told you to use the non-bang version of this... oops...

lol, i confirmed it wasn't working and could then make it work using the bang method

NickLaMuro · 2018-06-15T16:20:40Z

tools/remove_grouping_from_report_results.rb

+if opts[:dry_run]
+  puts "Running in dry-run, all changes will be rolled back when complete."
+
+  at_exit do


Just because I am not sure: I assume that at_exit will still fire after a exit 1, right?

(Nick could probably just go test this in a ruby script himself...)

yeah, good point. I'll test that

No need, I already did:

$ ruby -e "at_exit { puts 'foo' }; exit 42" foo $ echo $? 42

I did several tests of dry run so I'm pretty confident it works. It's a little messy how I'm doing it right now but I can't think of a better way to handle the begin/commit/rollback for dry-run/error/exit

jrafanie · 2018-06-18T16:07:29Z

@carbonin Can you review/merge? 🙇

carbonin

One question about a condition.

Also, from a high level, would it make sense for the guts of this to be a method on MiqReportResult (#clear_report_grouping perhaps?) rather than embedding all the logic in an untested tool?

Then, in a follow-up, we could use this method after generating the html data so future users won't need this tool.

carbonin · 2018-06-18T17:16:44Z

tools/remove_grouping_from_report_results.rb

+    if rr.report.extras.key?(:grouping)
+      rr.report.extras.except!(:grouping)
+      rr.save!
+      if rr.reload.report.extras.key?(:grouping)


When would this happen?

This was here before I changed line 43 from .save to .save! and added the begin/rescue around it. It's unlikely to happen now but I left this because it seemed odd to leave out one of the three cases below:

"doesn't need fixing"
"fixed"
"could NOT be fixed"

NickLaMuro · 2018-06-18T17:29:03Z

Also, from a high level, would it make sense for the guts of this to be a method on MiqReportResult (#clear_report_grouping perhaps?) rather than embedding all the logic in an untested tool?

@carbonin Kind of solving that here:

#17598

NickLaMuro

Random thing I noticed while reading through some of the other review comments. Low priority.

NickLaMuro · 2018-06-18T18:27:58Z

tools/remove_grouping_from_report_results.rb

+MiqReportResult.find_each(:batch_size => opts[:batch_size]).with_index do |rr, i|
+  begin
+    break if opts[:count].positive? && i == opts[:count]
+    next if rr.report.nil? || rr.report.extras.nil?


Sorry, just noticed this, but doesn't this next break the total count accuracy?

yeah, good 👁. I'll fix that.

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 ```

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

miq-bot · 2018-06-18T18:53:35Z

Checked commits jrafanie/manageiq@8b4fe07~...7a54727 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 2 offenses detected

tools/remove_grouping_from_report_results.rb

⚠️ - Line 1, Col 1 - Lint/ScriptPermission - Script file remove_grouping_from_report_results.rb doesn't have execute permission.
❗ - Line 55, Col 3 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

NickLaMuro

I am @NickLaMuro, and I (still) approve of these changes.

jrafanie · 2018-06-18T19:42:00Z

@carbonin it's green now 👍

…esults Add tool to remove grouping from report results (cherry picked from commit c36370d) https://bugzilla.redhat.com/show_bug.cgi?id=1594387

simaishi · 2018-06-22T19:38:50Z

Fine backport details:

$ git log -1
commit 7a3b8d247c77be02fea51c622466aa457c081052
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Mon Jun 18 16:09:05 2018 -0400

    Merge pull request #17589 from jrafanie/remove_grouping_from_report_results
    
    Add tool to remove grouping from report results
    (cherry picked from commit c36370d2a19196d780c2bfc3ec632955473236e8)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594387

…esults Add tool to remove grouping from report results (cherry picked from commit c36370d) https://bugzilla.redhat.com/show_bug.cgi?id=1594386

simaishi · 2018-06-22T19:45:38Z

Gaprindashvili backport details:

$ git log -1
commit c98aecd9674a05a0afc9ee0987f60ff91d0acd6d
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Mon Jun 18 16:09:05 2018 -0400

    Merge pull request #17589 from jrafanie/remove_grouping_from_report_results
    
    Add tool to remove grouping from report results
    (cherry picked from commit c36370d2a19196d780c2bfc3ec632955473236e8)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1594386

jrafanie requested review from gtanzillo and NickLaMuro June 14, 2018 21:30

jrafanie added enhancement tools gaprindashvili/yes labels Jun 14, 2018

jrafanie force-pushed the remove_grouping_from_report_results branch from c0e3a2a to daf38a7 Compare June 14, 2018 21:34

gtanzillo reviewed Jun 14, 2018

View reviewed changes

NickLaMuro suggested changes Jun 14, 2018

View reviewed changes

jrafanie force-pushed the remove_grouping_from_report_results branch from daf38a7 to b343e61 Compare June 15, 2018 13:43

jrafanie force-pushed the remove_grouping_from_report_results branch from b343e61 to 382060f Compare June 15, 2018 14:08

jrafanie commented Jun 15, 2018

View reviewed changes

jrafanie force-pushed the remove_grouping_from_report_results branch from 382060f to fec179d Compare June 15, 2018 15:56

NickLaMuro approved these changes Jun 15, 2018

View reviewed changes

carbonin reviewed Jun 18, 2018

View reviewed changes

NickLaMuro reviewed Jun 18, 2018

View reviewed changes

carbonin self-assigned this Jun 18, 2018

jrafanie added 3 commits June 18, 2018 14:47

Wrap in a transaction, rollback on dry-run or error

51be173

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

Increment the total before moving to the next iteration

7a54727

jrafanie force-pushed the remove_grouping_from_report_results branch from fec179d to 7a54727 Compare June 18, 2018 18:48

NickLaMuro approved these changes Jun 18, 2018

View reviewed changes

carbonin approved these changes Jun 18, 2018

View reviewed changes

carbonin merged commit c36370d into ManageIQ:master Jun 18, 2018

carbonin added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 18, 2018

jrafanie deleted the remove_grouping_from_report_results branch June 18, 2018 20:22

jrafanie added the fine/yes label Jun 19, 2018

simaishi added fine/backported and removed fine/yes labels Jun 22, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jun 22, 2018


		puts "Using options: #{opts.inspect}"

		# Load rails after trollop options are set. No one wants to wait for -h.

Add tool to remove grouping from report results #17589

Add tool to remove grouping from report results #17589

Conversation

jrafanie commented Jun 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 14, 2018

jrafanie commented Jun 15, 2018

jrafanie commented Jun 15, 2018

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

jrafanie commented Jun 15, 2018

NickLaMuro left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Jun 18, 2018

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 18, 2018

NickLaMuro left a 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 Jun 18, 2018

NickLaMuro left a comment

Choose a reason for hiding this comment

jrafanie commented Jun 18, 2018

simaishi commented Jun 22, 2018

simaishi commented Jun 22, 2018

jrafanie commented Jun 14, 2018 •

edited