From d4463944f4894e043c53aa4efd9adc2142775e97 Mon Sep 17 00:00:00 2001 From: Jeremy Stanley Date: Tue, 6 Feb 2024 16:58:06 -0700 Subject: [PATCH] stop parallelized reports on error the code that aborts "in progress" runners currently doesn't abort runners that haven't started yet, but should also the scope used to check whether all runners have completed should look for failed/aborted runners (in practice this hasn't mattered in the past because the report itself was put in "error" state, but it feels like a recipe for incomplete data to be returned to customers if something changes) also make sure a report runner that fails remains in "error" state; currently `update_parallel_progress`, called in the `ensure` clause in `run_account_report_runner`, changes it to "complete" test plan: - specs - smoke test parallelized reports like grade export and/or student activity flag=none refs FOO-4281 fixes FOO-4289 Change-Id: I8f64743bff6ffd2fe9515338de4c27572d213924 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/339890 Tested-by: Service Cloud Jenkins Reviewed-by: August Thornton QA-Review: Jeremy Stanley Product-Review: Jeremy Stanley --- app/models/account_report.rb | 16 +++--- app/models/account_report_runner.rb | 2 +- .../lib/account_reports/report_helper.rb | 13 ++--- .../spec_canvas/report_helper_spec.rb | 53 ++++++++++++++++++- spec/models/account_report_spec.rb | 30 +++++++++++ 5 files changed, 98 insertions(+), 16 deletions(-) diff --git a/app/models/account_report.rb b/app/models/account_report.rb index 4c15005d3bd..6ecd9d1072a 100644 --- a/app/models/account_report.rb +++ b/app/models/account_report.rb @@ -29,6 +29,8 @@ class AccountReport < ActiveRecord::Base has_many :account_report_runners, inverse_of: :account_report, autosave: false has_many :account_report_rows, inverse_of: :account_report, autosave: false + after_save :abort_incomplete_runners_if_needed + validates :account_id, :user_id, :workflow_state, presence: true serialize :parameters, type: Hash @@ -70,12 +72,7 @@ class AccountReport < ActiveRecord::Base alias_method :destroy_permanently!, :destroy def destroy self.workflow_state = "deleted" - result = save! - if saved_change_to_workflow_state? - abort_incomplete_runners - delay.delete_account_report_rows - end - result + save! end def self.delete_old_rows_and_runners @@ -143,6 +140,13 @@ class AccountReport < ActiveRecord::Base AccountReports.available_reports end + def abort_incomplete_runners_if_needed + if saved_change_to_workflow_state? && (deleted? || aborted? || error?) + abort_incomplete_runners + delay(priority: Delayed::LOWER_PRIORITY).delete_account_report_rows + end + end + def abort_incomplete_runners account_report_runners.incomplete.in_batches.update_all(workflow_state: "aborted", updated_at: Time.now.utc) end diff --git a/app/models/account_report_runner.rb b/app/models/account_report_runner.rb index a5b282fd181..23dd0d71a7c 100644 --- a/app/models/account_report_runner.rb +++ b/app/models/account_report_runner.rb @@ -71,9 +71,9 @@ class AccountReportRunner < ActiveRecord::Base update!(workflow_state: "error", ended_at: Time.now.utc) end - scope :in_progress, -> { where(workflow_state: %w[running]) } scope :completed, -> { where(workflow_state: %w[completed]) } scope :incomplete, -> { where(workflow_state: %w[created running]) } + scope :incomplete_or_failed, -> { where.not(workflow_state: "completed") } def delete_account_report_rows account_report_rows.in_batches(of: 10_000).delete_all diff --git a/gems/plugins/account_reports/lib/account_reports/report_helper.rb b/gems/plugins/account_reports/lib/account_reports/report_helper.rb index 20adaf3a3c8..d0079c5e8f5 100644 --- a/gems/plugins/account_reports/lib/account_reports/report_helper.rb +++ b/gems/plugins/account_reports/lib/account_reports/report_helper.rb @@ -428,7 +428,7 @@ module AccountReports::ReportHelper @account_report = report_runner.account_report begin - if @account_report.aborted? || @account_report.deleted? + if @account_report.aborted? || @account_report.deleted? || @account_report.error? report_runner.abort return end @@ -442,11 +442,11 @@ module AccountReports::ReportHelper fail_with_error(e) ensure update_parallel_progress(account_report: @account_report, report_runner:) - compile_parallel_report(headers, files:) if last_account_report_runner?(@account_report) + compile_parallel_report(report_runner, headers, files:) if last_account_report_runner?(@account_report) end end - def compile_parallel_report(headers, files: nil) + def compile_parallel_report(report_runner, headers, files: nil) GuardRail.activate(:primary) { @account_report.update(total_lines: @account_report.account_report_rows.count + 1) } xlog_location = AccountReport.current_xlog_location # wait 2 minutes for report db to catch up, if it does not catch up, use the @@ -499,9 +499,6 @@ module AccountReports::ReportHelper def fail_with_error(error) GuardRail.activate(:primary) do - # this should leave the runner that caused a failure to be in running or error state. - @account_report.account_report_runners.in_progress.update_all(workflow_state: "aborted") - @account_report.delete_account_report_rows Canvas::Errors.capture_exception(:account_report, error) @account_report.workflow_state = "error" @account_report.save! @@ -519,7 +516,7 @@ module AccountReports::ReportHelper end def update_parallel_progress(account_report: @account_report, report_runner:) - return if runner_aborted?(report_runner) + return if runner_aborted?(report_runner) || report_runner.error? report_runner.complete # let the regular report process update progress to 100 percent, cap at 99. @@ -538,7 +535,7 @@ module AccountReports::ReportHelper end def last_account_report_runner?(account_report) - return false if account_report.account_report_runners.incomplete.exists? + return false if account_report.account_report_runners.incomplete_or_failed.exists? AccountReport.transaction do @account_report.reload(lock: true) diff --git a/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb b/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb index 725e9f349fa..cd434601356 100644 --- a/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb +++ b/gems/plugins/account_reports/spec_canvas/report_helper_spec.rb @@ -18,12 +18,38 @@ # with this program. If not, see . # +require_relative "report_spec_helper" + module AccountReports class TestReport include ReportHelper - def initialize(account_report) + def initialize(account_report, items = []) @account_report = account_report + @items = items + end + + def test_report + create_report_runners(@items, @items.size, min: 1) + write_report_in_batches(["item"]) + end + + def test_report_runner(runner) + runner.batch_items.each_with_index do |item, i| + raise "fail" if item == "fail" + + add_report_row(row: [item], row_number: i, report_runner: runner) + end + end + end + + module Default + def self.test_report(account_report) + TestReport.new(account_report, account_report.parameters[:items]).test_report + end + + def self.parallel_test_report(account_report, runner) + TestReport.new(account_report).test_report_runner(runner) end end end @@ -71,6 +97,31 @@ describe "report helper" do expect(account_report.parameters["extra_text"]).to eq "Failed, the report failed to generate a file. Please try again." end + describe "parallel run" do + include ReportSpecHelper + + before :once do + AccountReports.configure_account_report "Default", { + "test_report" => { + title: -> { "Test Report" }, + } + } + @account = Account.default + end + + it "assembles rows from each runner" do + result = read_report("test_report", params: { items: (1..6).to_a }, order: "skip") + expect(result).to match_array([["1"], ["2"], ["3"], ["4"], ["5"], ["6"]]) + end + + it "handles errors appropriately" do + ar = run_report("test_report", params: { items: [1, 2, "fail", 4, 5, 6] }) + expect(ar).to be_error + expect(ar.account_report_runners.group(:workflow_state).count).to eq("completed" => 2, "error" => 1, "aborted" => 3) + expect(ErrorReport.last.message).to eq "fail" + end + end + describe "load pseudonyms" do before(:once) do @user = user_with_pseudonym(active_all: true, account:, user:) diff --git a/spec/models/account_report_spec.rb b/spec/models/account_report_spec.rb index 08961617407..ff6e5eb199a 100644 --- a/spec/models/account_report_spec.rb +++ b/spec/models/account_report_spec.rb @@ -43,4 +43,34 @@ describe AccountReport do expect(AccountReportRow.where(id: a_row_2.id).count).to eq(0) end end + + describe "stopping parallelized reports" do + before :once do + @account = account_model + @report = AccountReport.create!(account: @account, user: account_admin_user, workflow_state: "running") + @runners = [@report.account_report_runners.create!(workflow_state: "completed"), + @report.account_report_runners.create!(workflow_state: "running"), + @report.account_report_runners.create!(workflow_state: "created")] + end + + it "aborts runners when errored" do + @report.mark_as_errored + expect(@runners.map { |r| r.reload.workflow_state }).to eq %w[completed aborted aborted] + end + + it "aborts runners when deleted" do + @report.destroy + expect(@runners.map { |r| r.reload.workflow_state }).to eq %w[completed aborted aborted] + end + + it "aborts runners when aborted" do + @report.update! workflow_state: "aborted" + expect(@runners.map { |r| r.reload.workflow_state }).to eq %w[completed aborted aborted] + end + + it "leaves runners alone when completed" do + @report.update! workflow_state: "completed" + expect(@runners.map { |r| r.reload.workflow_state }).to eq %w[completed running created] + end + end end