Puppet » Puppet Dashboard

When we switch to Puppet’s new reporting REST interface, this problem should go away. In the meantime, I’m not sure of the best solution. I’m open to suggestions / patches :)

This is a bad bug because it’s losing data and can break the application if the data is truncated in a bad spot. I’m escalating this so we resolve this sooner.

For now, the best workaround I can think for regaining the use of the Dashboard is to delete these truncated reports from the database. You can do this by running ./script/dbconsole production from the puppet-dashboard directory and executing:

delete from reports where length(report) = 65535;

And then pres CTRL-D to close this console.

Once the underlying problem is fixed, users can reimport their original report files.

The attached github branch just increases the reports.report text field size up to 16mb. Perhaps not the ideal long term solution since databases could grow quickly, but enough for now to prevent data loss.

There was a test that we wrote to help validate that the migration did what we intended, but I didn’t think the test was useful in the long term. Igal, you may disagree, if so let me know and we can clean it up and add it back in. If not, I’ll probably work on cleaning up some portion of it for a more useful way to create test reports of variable length in the future.

Here’s the test just in case:

diff --git spec/models/report_spec.rb spec/models/report_spec.rb
index 723b0ea..8bc040d 100644
--- spec/models/report_spec.rb
+++ spec/models/report_spec.rb
@@ -15,6 +15,17 @@ describe Report do
         @report_data = YAML.load(@report_yaml).extend(ReportExtensions).extend(ReportExtensions)
       end
 
+      it "handles greater than 64k of report text" do
+        # can actually handle up to 16mb but that test is really slow
+        report = report_from_yaml
+        file_size = 65 * 1024
+        report.report.logs.first.instance_variable_set(:@message, "*" * file_size)
+        original_report_length = report.report.logs.first.message.length
+        report.save!
+        report.reload
+        report.report.logs.first.message.length.should == original_report_length
+      end
+
       it "sets success correctly based on whether the report contains failures" do
         report = report_model_from_yaml('failure.yml')
         report.save!

Forgot to assign to myself while working on it. Better late than never.

Matt:

Your fix works, but I’m switching this issue back to “Accepted”.

I’d like to have the truncation test be included in the code. It makes me feel better having it around because I know that this issue isn’t going to come back and we can remove that quirky test when we refactor reports.

Another thing that I’d like to see fixed is that existing reports may have been truncated in such a way that their YAML is invalid and can’t be deseriaized, which causes rendering errors.

One approach is to have another migration delete the reports whose lengths are exactly the maximum allowed length of the old field, e.g.:

SQL: delete from reports where length(report) = 65535;
Rails: Report.destroy_all(['length(reports.report) = ?', 65535])

Another approach is to have a migration find all the records whose reports of this exact length, try to parse them, delete the ones that fail to parse because they’re useless, and then display a listing of all the records deleted and those that are suspect so the user can choose to re-import them.

Thoughts?

Igal, I added the test and the migration to the branch on the ticket as a couple more commits.

I don’t think it’s worth checking that reports of exactly that length are valid. I played with doing that, and it looks like in theory we could just iterate through reports of length and check report.valid? and delete them if not. However, I’m not confident that the data I was using to test that is representative of the truncated reports we’re seeing in the wild. I’m fairly confident though that any report of that exact data length is a problem.

If you’re not comfortable with that then let me know and I’ll change it to only delete reports that detect as invalid.

Matt:

Thanks for the details and discussion.

Okay, lets remove all the reports of the suspicious length regardless of whether they’re valid. This way people could just run the migration and then re-run the reports:import task, and this would guarantee that the reports they have in the system then are okay. Otherwise, the user would need to run additional tasks and do manual inspection to decide which valid but suspicious records they want to keep, which seems to be more trouble than it’s worth.

PS: I don’t think you pushed the changes to your branch.

Good catch on the push. I did push it, but to a different branch name that I didn’t mean to use. The attached branch now has the changes.

Matt:

Please review my changes at: http://github.com/igal/puppet-dashboard/tree/bug-3298-increase_reports_report_field_size

[#3298] Add warnings explaining truncation bug and how to re-import reports.

Here is the new output for the migration:

==  ChangeReportReportFieldSizeToBeLarger: migrating ==========================
-- WARNING: Deleted 28 invalid reports that were truncated by the database due to an earlier bug. Please import your reports again (see README for details) if you wish to have these deleted reports re-added correctly.
-- Increasing column size of reports to avoid truncation problems:
-- change_column(:reports, :report, :mediumtext, {:limit=>16777216})
   -> 11.8885s
==  ChangeReportReportFieldSizeToBeLarger: migrated (14.4187s) ================

+1

Done. Merged into http://github.com/puppetlabs/puppet-dashboard/tree/pu