From e40772f7b769716ae2ef18f3e2ac52bace63ed22 Mon Sep 17 00:00:00 2001 From: Maarten Raaphorst Date: Mon, 15 May 2017 13:50:55 +0200 Subject: [PATCH 01/11] No longer raise exception when scheduler job fails to run after ten tries, but record the error instead. --- .../app/controllers/monitoring.coffee | 17 +++++++++++++++- .../javascripts/app/views/monitoring.jst.eco | 9 +++++++++ app/controllers/monitoring_controller.rb | 20 +++++++++++++++++++ app/models/scheduler.rb | 15 +++++++++++--- config/routes/monitoring.rb | 7 ++++--- db/migrate/20170515000001_scheduler_status.rb | 8 ++++++++ 6 files changed, 69 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20170515000001_scheduler_status.rb diff --git a/app/assets/javascripts/app/controllers/monitoring.coffee b/app/assets/javascripts/app/controllers/monitoring.coffee index c43b29142..2601628b8 100644 --- a/app/assets/javascripts/app/controllers/monitoring.coffee +++ b/app/assets/javascripts/app/controllers/monitoring.coffee @@ -4,6 +4,7 @@ class Index extends App.ControllerSubContent events: 'click .js-resetToken': 'resetToken' 'click .js-select': 'selectAll' + 'click .js-restartDeadJobs': 'restartDeadJobs' constructor: -> super @@ -29,7 +30,10 @@ class Index extends App.ControllerSubContent ) render: => - @html App.view('monitoring')(data: @data) + @html App.view('monitoring')( + data: @data + job_restart_count: @job_restart_count + ) resetToken: (e) => e.preventDefault() @@ -42,4 +46,15 @@ class Index extends App.ControllerSubContent @load() ) + restartDeadJobs: (e) => + e.preventDefault() + @ajax( + id: 'restart_dead_jobs_request' + type: 'POST' + url: "#{@apiPath}/monitoring/restart_dead_jobs" + success: (data) => + @job_restart_count = data.job_restart_count + @render() + ) + App.Config.set('Monitoring', { prio: 3600, name: 'Monitoring', parent: '#system', target: '#system/monitoring', controller: Index, permission: ['admin.monitoring'] }, 'NavBarAdmin') diff --git a/app/assets/javascripts/app/views/monitoring.jst.eco b/app/assets/javascripts/app/views/monitoring.jst.eco index 053c128ed..247513122 100644 --- a/app/assets/javascripts/app/views/monitoring.jst.eco +++ b/app/assets/javascripts/app/views/monitoring.jst.eco @@ -34,6 +34,15 @@ <% end %> <% end %> + <% if !_.isEmpty(@data.issues): %> + + <% if !_.isUndefined(@job_restart_count): %> +

+ <%- @T('Detected %s dead job(s) available for restart', @job_restart_count) %> + <%- ', restarting...' if @job_restart_count > 0 %> +

+ <% end %> + <% end %> \ No newline at end of file diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index ece7e4a5d..c29a0762e 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -81,6 +81,10 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX issues.push 'scheduler not running' end + Scheduler.where(status: 'error').each { |scheduler| + issues.push "Failed to run scheduled job \'#{scheduler.name}\'. Cause: #{scheduler.error_message}" + } + token = Setting.get('monitoring_token') if issues.empty? @@ -173,6 +177,22 @@ curl http://localhost/api/v1/monitoring/status?token=XXX render json: result, status: :created end + def restart_dead_jobs + access_check + + count = 0 + Scheduler.where(status: 'error').where(active: false).each { |scheduler| + scheduler.active = true + scheduler.save + count += 1 + } + + result = { + job_restart_count: count + } + render json: result + end + private def token_or_permission_check diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index 0ce63ea30..7311c40d9 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -169,8 +169,10 @@ class Scheduler < ApplicationModel end def self._start_job(job, try_count = 0, try_run_time = Time.zone.now) - job.last_run = Time.zone.now - job.pid = Thread.current.object_id + job.last_run = Time.zone.now + job.pid = Thread.current.object_id + job.status = 'ok' + job.error_message = '' job.save logger.info "execute #{job.method} (try_count #{try_count})..." eval job.method() # rubocop:disable Lint/Eval @@ -197,7 +199,14 @@ class Scheduler < ApplicationModel if try_run_max > try_count _start_job(job, try_count, try_run_time) else - raise "STOP thread for #{job.method} after #{try_count} tries (#{e.inspect})" + @@jobs_started[ job.id ] = false + error = "Failed to run #{job.method} after #{try_count} tries #{e.inspect}" + logger.error error + + job.error_message = error + job.status = 'error' + job.active = false + job.save end end diff --git a/config/routes/monitoring.rb b/config/routes/monitoring.rb index 18f2e6c93..bf7205054 100644 --- a/config/routes/monitoring.rb +++ b/config/routes/monitoring.rb @@ -1,8 +1,9 @@ Zammad::Application.routes.draw do api_path = Rails.configuration.api_path - match api_path + '/monitoring/health_check', to: 'monitoring#health_check', via: :get - match api_path + '/monitoring/status', to: 'monitoring#status', via: :get - match api_path + '/monitoring/token', to: 'monitoring#token', via: :post + match api_path + '/monitoring/health_check', to: 'monitoring#health_check', via: :get + match api_path + '/monitoring/status', to: 'monitoring#status', via: :get + match api_path + '/monitoring/token', to: 'monitoring#token', via: :post + match api_path + '/monitoring/restart_dead_jobs', to: 'monitoring#restart_dead_jobs', via: :post end diff --git a/db/migrate/20170515000001_scheduler_status.rb b/db/migrate/20170515000001_scheduler_status.rb new file mode 100644 index 000000000..c6be3481a --- /dev/null +++ b/db/migrate/20170515000001_scheduler_status.rb @@ -0,0 +1,8 @@ +class SchedulerStatus < ActiveRecord::Migration + def up + change_table :schedulers do |t| + t.string :error_message + t.string :status + end + end +end From 7b86174d26047bd6d16b682c43b185d06371f613 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 14:45:13 +0200 Subject: [PATCH 02/11] Changed wording from dead to failed in pull request 1118. --- app/assets/javascripts/app/controllers/monitoring.coffee | 8 ++++---- app/assets/javascripts/app/views/monitoring.jst.eco | 4 ++-- app/controllers/monitoring_controller.rb | 2 +- config/routes/monitoring.rb | 8 ++++---- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/app/controllers/monitoring.coffee b/app/assets/javascripts/app/controllers/monitoring.coffee index 2601628b8..b483057bd 100644 --- a/app/assets/javascripts/app/controllers/monitoring.coffee +++ b/app/assets/javascripts/app/controllers/monitoring.coffee @@ -4,7 +4,7 @@ class Index extends App.ControllerSubContent events: 'click .js-resetToken': 'resetToken' 'click .js-select': 'selectAll' - 'click .js-restartDeadJobs': 'restartDeadJobs' + 'click .js-restartFailedJobs': 'restartFailedJobs' constructor: -> super @@ -46,12 +46,12 @@ class Index extends App.ControllerSubContent @load() ) - restartDeadJobs: (e) => + restartFailedJobs: (e) => e.preventDefault() @ajax( - id: 'restart_dead_jobs_request' + id: 'restart_failed_jobs_request' type: 'POST' - url: "#{@apiPath}/monitoring/restart_dead_jobs" + url: "#{@apiPath}/monitoring/restart_failed_jobs" success: (data) => @job_restart_count = data.job_restart_count @render() diff --git a/app/assets/javascripts/app/views/monitoring.jst.eco b/app/assets/javascripts/app/views/monitoring.jst.eco index 247513122..5619b15b6 100644 --- a/app/assets/javascripts/app/views/monitoring.jst.eco +++ b/app/assets/javascripts/app/views/monitoring.jst.eco @@ -35,10 +35,10 @@ <% end %> <% if !_.isEmpty(@data.issues): %> - + <% if !_.isUndefined(@job_restart_count): %>

- <%- @T('Detected %s dead job(s) available for restart', @job_restart_count) %> + <%- @T('Detected %s failed job(s) available for restart', @job_restart_count) %> <%- ', restarting...' if @job_restart_count > 0 %>

<% end %> diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index c29a0762e..eab0c7829 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -177,7 +177,7 @@ curl http://localhost/api/v1/monitoring/status?token=XXX render json: result, status: :created end - def restart_dead_jobs + def restart_failed_jobs access_check count = 0 diff --git a/config/routes/monitoring.rb b/config/routes/monitoring.rb index bf7205054..637330fac 100644 --- a/config/routes/monitoring.rb +++ b/config/routes/monitoring.rb @@ -1,9 +1,9 @@ Zammad::Application.routes.draw do api_path = Rails.configuration.api_path - match api_path + '/monitoring/health_check', to: 'monitoring#health_check', via: :get - match api_path + '/monitoring/status', to: 'monitoring#status', via: :get - match api_path + '/monitoring/token', to: 'monitoring#token', via: :post - match api_path + '/monitoring/restart_dead_jobs', to: 'monitoring#restart_dead_jobs', via: :post + match api_path + '/monitoring/health_check', to: 'monitoring#health_check', via: :get + match api_path + '/monitoring/status', to: 'monitoring#status', via: :get + match api_path + '/monitoring/token', to: 'monitoring#token', via: :post + match api_path + '/monitoring/restart_failed_jobs', to: 'monitoring#restart_failed_jobs', via: :post end From d3e4577fa5b4c70d8ddcde90f75cd5981d7e5d57 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 15:00:12 +0200 Subject: [PATCH 03/11] Improved codestyle. --- app/controllers/monitoring_controller.rb | 14 ++++++-------- app/models/scheduler.rb | 21 ++++++++++++--------- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index eab0c7829..fa9867fd1 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -81,9 +81,9 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX issues.push 'scheduler not running' end - Scheduler.where(status: 'error').each { |scheduler| + Scheduler.where(status: 'error').each do |scheduler| issues.push "Failed to run scheduled job \'#{scheduler.name}\'. Cause: #{scheduler.error_message}" - } + end token = Setting.get('monitoring_token') @@ -181,16 +181,14 @@ curl http://localhost/api/v1/monitoring/status?token=XXX access_check count = 0 - Scheduler.where(status: 'error').where(active: false).each { |scheduler| - scheduler.active = true - scheduler.save + Scheduler.where(status: 'error', active: false).each do |scheduler| + scheduler.update(active: true) count += 1 - } + end - result = { + render json: { job_restart_count: count } - render json: result end private diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index 7311c40d9..f541ece10 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -169,11 +169,13 @@ class Scheduler < ApplicationModel end def self._start_job(job, try_count = 0, try_run_time = Time.zone.now) - job.last_run = Time.zone.now - job.pid = Thread.current.object_id - job.status = 'ok' - job.error_message = '' - job.save + job.update( + last_run: Time.zone.now, + pid: Thread.current.object_id, + status: 'ok', + error_message: '', + ) + logger.info "execute #{job.method} (try_count #{try_count})..." eval job.method() # rubocop:disable Lint/Eval rescue => e @@ -203,10 +205,11 @@ class Scheduler < ApplicationModel error = "Failed to run #{job.method} after #{try_count} tries #{e.inspect}" logger.error error - job.error_message = error - job.status = 'error' - job.active = false - job.save + job.update( + error_message: error, + status: 'error', + active: false, + ) end end From 1ce2b124382ad3bb82b1163258235c489298c9cb Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 15:06:51 +0200 Subject: [PATCH 04/11] Added version to migration. --- db/migrate/20170515000001_scheduler_status.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/migrate/20170515000001_scheduler_status.rb b/db/migrate/20170515000001_scheduler_status.rb index c6be3481a..ae9f0c46e 100644 --- a/db/migrate/20170515000001_scheduler_status.rb +++ b/db/migrate/20170515000001_scheduler_status.rb @@ -1,4 +1,4 @@ -class SchedulerStatus < ActiveRecord::Migration +class SchedulerStatus < ActiveRecord::Migration[4.2] def up change_table :schedulers do |t| t.string :error_message From 7248be908fca037e1e3f824ce064f5f968c6fd5e Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 15:50:46 +0200 Subject: [PATCH 05/11] Improved migration and changed wording. --- app/assets/javascripts/app/views/monitoring.jst.eco | 6 +++--- app/controllers/monitoring_controller.rb | 2 +- db/migrate/20120101000001_create_base.rb | 2 ++ db/migrate/20170515000001_scheduler_status.rb | 8 ++++++-- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/app/views/monitoring.jst.eco b/app/assets/javascripts/app/views/monitoring.jst.eco index 5619b15b6..d5650ee79 100644 --- a/app/assets/javascripts/app/views/monitoring.jst.eco +++ b/app/assets/javascripts/app/views/monitoring.jst.eco @@ -35,11 +35,11 @@ <% end %> <% if !_.isEmpty(@data.issues): %> - + <% if !_.isUndefined(@job_restart_count): %>

- <%- @T('Detected %s failed job(s) available for restart', @job_restart_count) %> - <%- ', restarting...' if @job_restart_count > 0 %> + <%- @T('%s failed job(s) marked for restart', @job_restart_count) %> + <%- @T(', restarting...') if @job_restart_count > 0 %>

<% end %> <% end %> diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index fa9867fd1..4da0addde 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -82,7 +82,7 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX end Scheduler.where(status: 'error').each do |scheduler| - issues.push "Failed to run scheduled job \'#{scheduler.name}\'. Cause: #{scheduler.error_message}" + issues.push "Failed to run scheduled job '#{scheduler.name}'. Cause: #{scheduler.error_message}" end token = Setting.get('monitoring_token') diff --git a/db/migrate/20120101000001_create_base.rb b/db/migrate/20120101000001_create_base.rb index 71547ebb9..78708a076 100644 --- a/db/migrate/20120101000001_create_base.rb +++ b/db/migrate/20120101000001_create_base.rb @@ -514,6 +514,8 @@ class CreateBase < ActiveRecord::Migration t.integer :prio, null: false t.string :pid, limit: 250, null: true t.string :note, limit: 250, null: true + t.string :error_message, null: true + t.string :status, null: true t.boolean :active, null: false, default: false t.integer :updated_by_id, null: false t.integer :created_by_id, null: false diff --git a/db/migrate/20170515000001_scheduler_status.rb b/db/migrate/20170515000001_scheduler_status.rb index ae9f0c46e..dcaa8ef87 100644 --- a/db/migrate/20170515000001_scheduler_status.rb +++ b/db/migrate/20170515000001_scheduler_status.rb @@ -1,8 +1,12 @@ class SchedulerStatus < ActiveRecord::Migration[4.2] def up + + # return if it's a new setup + return if !Setting.find_by(name: 'system_init_done') + change_table :schedulers do |t| - t.string :error_message - t.string :status + t.string :error_message, null: true + t.string :status, null: true end end end From 63aa9142af30144235e5c9edeef0792dc042abec Mon Sep 17 00:00:00 2001 From: Jens Pfeifer Date: Thu, 7 Sep 2017 14:33:09 +0000 Subject: [PATCH 06/11] Improved scheduler restart button handling in monitoring view. --- .../javascripts/app/controllers/monitoring.coffee | 8 ++------ .../javascripts/app/views/monitoring.jst.eco | 10 ++-------- app/controllers/monitoring_controller.rb | 15 +++++++-------- db/migrate/20170515000001_scheduler_status.rb | 2 +- 4 files changed, 12 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/app/controllers/monitoring.coffee b/app/assets/javascripts/app/controllers/monitoring.coffee index b483057bd..2b6900248 100644 --- a/app/assets/javascripts/app/controllers/monitoring.coffee +++ b/app/assets/javascripts/app/controllers/monitoring.coffee @@ -30,10 +30,7 @@ class Index extends App.ControllerSubContent ) render: => - @html App.view('monitoring')( - data: @data - job_restart_count: @job_restart_count - ) + @html App.view('monitoring')(data: @data) resetToken: (e) => e.preventDefault() @@ -53,8 +50,7 @@ class Index extends App.ControllerSubContent type: 'POST' url: "#{@apiPath}/monitoring/restart_failed_jobs" success: (data) => - @job_restart_count = data.job_restart_count - @render() + @load() ) App.Config.set('Monitoring', { prio: 3600, name: 'Monitoring', parent: '#system', target: '#system/monitoring', controller: Index, permission: ['admin.monitoring'] }, 'NavBarAdmin') diff --git a/app/assets/javascripts/app/views/monitoring.jst.eco b/app/assets/javascripts/app/views/monitoring.jst.eco index d5650ee79..ceea50003 100644 --- a/app/assets/javascripts/app/views/monitoring.jst.eco +++ b/app/assets/javascripts/app/views/monitoring.jst.eco @@ -34,15 +34,9 @@ <% end %> <% end %> - <% if !_.isEmpty(@data.issues): %> + <% if _.contains(@data.actions, 'restart_failed_jobs'): %> - <% if !_.isUndefined(@job_restart_count): %> -

- <%- @T('%s failed job(s) marked for restart', @job_restart_count) %> - <%- @T(', restarting...') if @job_restart_count > 0 %> -

- <% end %> <% end %> - \ No newline at end of file + diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index 4da0addde..82980318c 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -30,6 +30,7 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX token_or_permission_check issues = [] + actions = Set.new # channel check last_run_tolerance = Time.zone.now - 1.hour @@ -81,8 +82,9 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX issues.push 'scheduler not running' end - Scheduler.where(status: 'error').each do |scheduler| + Scheduler.where(status: 'error', active: false).each do |scheduler| issues.push "Failed to run scheduled job '#{scheduler.name}'. Cause: #{scheduler.error_message}" + actions.add(:restart_failed_jobs) end token = Setting.get('monitoring_token') @@ -100,8 +102,9 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX result = { healthy: false, message: issues.join(';'), - issues: issues, - token: token, + issues: issues, + actions: actions, + token: token, } render json: result end @@ -180,15 +183,11 @@ curl http://localhost/api/v1/monitoring/status?token=XXX def restart_failed_jobs access_check - count = 0 Scheduler.where(status: 'error', active: false).each do |scheduler| scheduler.update(active: true) - count += 1 end - render json: { - job_restart_count: count - } + render json: {}, status: :ok end private diff --git a/db/migrate/20170515000001_scheduler_status.rb b/db/migrate/20170515000001_scheduler_status.rb index dcaa8ef87..6c55b2ff7 100644 --- a/db/migrate/20170515000001_scheduler_status.rb +++ b/db/migrate/20170515000001_scheduler_status.rb @@ -1,4 +1,4 @@ -class SchedulerStatus < ActiveRecord::Migration[4.2] +class SchedulerStatus < ActiveRecord::Migration def up # return if it's a new setup From 80199a61b535984c574cdb26c5c08bb1123c7b0d Mon Sep 17 00:00:00 2001 From: Jens Pfeifer Date: Thu, 7 Sep 2017 15:17:06 +0000 Subject: [PATCH 07/11] Added tests for scheduler fields error_message and status. --- spec/factories/scheduler.rb | 25 +++++++++++++++++++++++++ spec/models/scheduler_spec.rb | 18 ++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 spec/factories/scheduler.rb diff --git a/spec/factories/scheduler.rb b/spec/factories/scheduler.rb new file mode 100644 index 000000000..5b484c9a1 --- /dev/null +++ b/spec/factories/scheduler.rb @@ -0,0 +1,25 @@ +FactoryGirl.define do + sequence :test_scheduler_name do |n| + "Testscheduler#{n}" + end +end + +FactoryGirl.define do + + factory :scheduler do + name { generate(:test_scheduler_name) } + last_run { Time.zone.now } + pid 1337 + prio 1 + status 'ok' + active true + period { 10.minutes } + running false + note 'test' + updated_by_id 1 + created_by_id 1 + created_at 1 + updated_at 1 + add_attribute(:method) { 'test' } + end +end diff --git a/spec/models/scheduler_spec.rb b/spec/models/scheduler_spec.rb index 7432186d0..44e0df6b0 100644 --- a/spec/models/scheduler_spec.rb +++ b/spec/models/scheduler_spec.rb @@ -26,6 +26,24 @@ RSpec.describe Scheduler do SpecSpace.send(:remove_const, :DelayedJobBackend) end + describe '._start_job' do + + it 'sets error status/message for failed jobs' do + job = create(:scheduler) + described_class._start_job(job) + expect(job.status).to eq 'error' + expect(job.active).to be false + expect(job.error_message).to be_present + end + + it 'executes job that is expected to succeed' do + expect(Setting).to receive(:reload) + job = create(:scheduler, method: 'Setting.reload') + described_class._start_job(job) + expect(job.status).to eq 'ok' + end + end + describe '.cleanup' do it 'gets called by .threads' do From 60e7d990ea063e8f34615d0c0f9b62fc0896af08 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 17:34:32 +0200 Subject: [PATCH 08/11] Moved .restart_failed_jobs to backend and added rspec test. --- app/controllers/monitoring_controller.rb | 4 +--- app/models/scheduler.rb | 14 ++++++++++++++ spec/models/scheduler_spec.rb | 11 +++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index 82980318c..aab7f5ccb 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -183,9 +183,7 @@ curl http://localhost/api/v1/monitoring/status?token=XXX def restart_failed_jobs access_check - Scheduler.where(status: 'error', active: false).each do |scheduler| - scheduler.update(active: true) - end + Scheduler.restart_failed_jobs render json: {}, status: :ok end diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index f541ece10..a94a777a2 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -267,4 +267,18 @@ class Scheduler < ApplicationModel end + # This function restarts failed jobs to retry them + # + # @example + # Scheduler.restart_failed_jobs + # + # return [true] + def self.restart_failed_jobs + Scheduler.where(status: 'error', active: false).each do |scheduler| + scheduler.update(active: true) + end + + true + end + end diff --git a/spec/models/scheduler_spec.rb b/spec/models/scheduler_spec.rb index 44e0df6b0..087501aca 100644 --- a/spec/models/scheduler_spec.rb +++ b/spec/models/scheduler_spec.rb @@ -26,6 +26,17 @@ RSpec.describe Scheduler do SpecSpace.send(:remove_const, :DelayedJobBackend) end + describe '.restart_failed_jobs' do + + it 'does restart failed jobs' do + job = create(:scheduler, status: 'error', active: false) + described_class.restart_failed_jobs + job.reload + expect(job.active).to be true + end + + end + describe '._start_job' do it 'sets error status/message for failed jobs' do From 68ba6b308745baa1d694ecc11d785e5beb72cab7 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 17:43:57 +0200 Subject: [PATCH 09/11] Added basic controller test for restart_failed_jobs route. --- test/controllers/monitoring_controller_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/controllers/monitoring_controller_test.rb b/test/controllers/monitoring_controller_test.rb index 7914b9522..56d2d7958 100644 --- a/test/controllers/monitoring_controller_test.rb +++ b/test/controllers/monitoring_controller_test.rb @@ -391,4 +391,10 @@ class MonitoringControllerTest < ActionDispatch::IntegrationTest end + test '09 check restart_failed_jobs' do + credentials = ActionController::HttpAuthentication::Basic.encode_credentials('monitoring-admin@example.com', 'adminpw') + post "/api/v1/monitoring/restart_failed_jobs", {}, @headers.merge('Authorization' => credentials) + assert_response(200) + end + end From 70ce1be8c994817034ad0a86fb021baedb527ce2 Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Thu, 7 Sep 2017 17:44:32 +0200 Subject: [PATCH 10/11] Tidied. --- test/controllers/monitoring_controller_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/monitoring_controller_test.rb b/test/controllers/monitoring_controller_test.rb index 56d2d7958..fcd066e62 100644 --- a/test/controllers/monitoring_controller_test.rb +++ b/test/controllers/monitoring_controller_test.rb @@ -393,7 +393,7 @@ class MonitoringControllerTest < ActionDispatch::IntegrationTest test '09 check restart_failed_jobs' do credentials = ActionController::HttpAuthentication::Basic.encode_credentials('monitoring-admin@example.com', 'adminpw') - post "/api/v1/monitoring/restart_failed_jobs", {}, @headers.merge('Authorization' => credentials) + post '/api/v1/monitoring/restart_failed_jobs', {}, @headers.merge('Authorization' => credentials) assert_response(200) end From be730c08d9c78436f395d9f63c3d22d212c818c2 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Thu, 7 Sep 2017 18:07:48 +0200 Subject: [PATCH 11/11] Moved listing of failed jobs to own method and added tests. --- app/controllers/monitoring_controller.rb | 4 ++-- app/models/scheduler.rb | 14 ++++++++++++-- spec/models/scheduler_spec.rb | 12 +++++++++++- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/app/controllers/monitoring_controller.rb b/app/controllers/monitoring_controller.rb index aab7f5ccb..248e7b1e8 100644 --- a/app/controllers/monitoring_controller.rb +++ b/app/controllers/monitoring_controller.rb @@ -82,8 +82,8 @@ curl http://localhost/api/v1/monitoring/health_check?token=XXX issues.push 'scheduler not running' end - Scheduler.where(status: 'error', active: false).each do |scheduler| - issues.push "Failed to run scheduled job '#{scheduler.name}'. Cause: #{scheduler.error_message}" + Scheduler.failed_jobs.each do |job| + issues.push "Failed to run scheduled job '#{job.name}'. Cause: #{job.error_message}" actions.add(:restart_failed_jobs) end diff --git a/app/models/scheduler.rb b/app/models/scheduler.rb index a94a777a2..22581ba9c 100644 --- a/app/models/scheduler.rb +++ b/app/models/scheduler.rb @@ -267,6 +267,16 @@ class Scheduler < ApplicationModel end + # This function returns a list of failed jobs + # + # @example + # Scheduler.failed_jobs + # + # return [Array] + def self.failed_jobs + where(status: 'error', active: false) + end + # This function restarts failed jobs to retry them # # @example @@ -274,8 +284,8 @@ class Scheduler < ApplicationModel # # return [true] def self.restart_failed_jobs - Scheduler.where(status: 'error', active: false).each do |scheduler| - scheduler.update(active: true) + failed_jobs.each do |job| + job.update(active: true) end true diff --git a/spec/models/scheduler_spec.rb b/spec/models/scheduler_spec.rb index 087501aca..67412c29d 100644 --- a/spec/models/scheduler_spec.rb +++ b/spec/models/scheduler_spec.rb @@ -26,6 +26,17 @@ RSpec.describe Scheduler do SpecSpace.send(:remove_const, :DelayedJobBackend) end + describe '.failed_jobs' do + + it 'does list failed jobs' do + job = create(:scheduler, status: 'error', active: false) + failed_list = described_class.failed_jobs + expect(failed_list).to be_present + expect(failed_list).to include(job) + end + + end + describe '.restart_failed_jobs' do it 'does restart failed jobs' do @@ -34,7 +45,6 @@ RSpec.describe Scheduler do job.reload expect(job.active).to be true end - end describe '._start_job' do