Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[review] delayed_cron_job を入れ替える rake タスク #33

Merged
merged 4 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions lib/sg_fargate_rails/delayed_cron_job_utility.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
module SgFargateRails
class DelayedCronJobUtility
class << self
def refresh_cron_jobs!
DelayedCronJobUtility.new.refresh_cron_jobs!
end

def cron_jobs
Delayed::Job.where.not(cron: nil)
end
end

def initialize
unless defined?(::DelayedCronJob)
raise 'DelayedCronJob not defined.'
end
end

def refresh_cron_jobs!
ActiveRecord::Base.transaction do
destroy_cron_jobs!
create_cron_jobs!
end
end

private

def destroy_cron_jobs!
DelayedCronJobUtility.cron_jobs.find_each do |delayed_job|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

この条件だけだと、現在進行系で動いてるジョブのレコードを消すことにならない?と思いました。(ちょうどいい属性があるかは未確認)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

実行中のジョブもキューデータが消されます。消えてもワーカーとしては問題なく動作することを確認してます。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

懸念があるとすると
処理が長引いて、本来はスキップされるはずの次のスケジュールに新しくジョブが追加されて、別のワーカーが部分的に並列実行しちゃうかも

でも、これは opsworks でも起きてるはずなので(そもそもスケジュールスキップされないはず)問題ではなさそう

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ジョブが成功する場合はいいとして、例外などで失敗した場合も問題ないですか?
ジョブが失敗したことを記録する先がなくなって、期待しない動作にならないかなーと。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

考えてなかった!
処理用の別のジョブを産んでくれればよいのにな delayed_cron_job ...

delayed_cron_job → rake task → delayed_job という処理順にしてもらうしかないのかな

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

エラー情報が紛失する?
=> bugsnag に残るはず

リトライを期待してたら困る?
=> 現状 (OpsWorks) の cron はリトライされないからその想定はないはず

Copy link
Contributor Author

@morikiyo morikiyo Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

異常系の実機検証してみた。

cron job でエラーが発生したら jobmon, bugsnag, cloudwatch logs にエラー情報が残る
DB (delayed_jobs) からは deploy でエラー情報が消えてしまうが、↑の3つのエラー情報があるから問題はなさそう。

cron job の処理実行中にデプロイによる refresh タスクが実行されたら
コンテナが停止され
jobmon => running のままで通知が来る
bugsnag => "rake aborted!"
cloudwatch logs => SIGTERM のログあり

[Worker(host:ip-10-1-0-74.ap-northeast-1.compute.internal pid:41)] Job Jobmon::TaskJob [030e6b50-6b26-410c-8287-76f61d6bb786] from DelayedJob(default) with arguments: [{"task"=>"cron:long_process_test", "estimate_time"=>1800, "_aj_ruby2_keywords"=>["task", "estimate_time"]}] (id=728) (queue=default) FAILED with SignalException: SIGTERM
E, [2023-12-01T16:39:25.166443 #41] ERROR -- : 2023-12-01T16:39:25+0900: [Worker(host:ip-10-1-0-74.ap-northeast-1.compute.internal pid:41)] Job Jobmon::TaskJob [030e6b50-6b26-410c-8287-76f61d6bb786] from DelayedJob(default) with arguments: [{"task"=>"cron:long_process_test", "estimate_time"=>1800, "_aj_ruby2_keywords"=>["task", "estimate_time"]}] (id=728) (queue=default) FAILED with SignalException: SIGTERM

# NOTE: 念のため cron が設定されていることを再チェック
if delayed_job.cron.present?
delayed_job.destroy!
end
end
end

def create_cron_jobs!
cron_settings.each do |_name, options|
job_class = options[:class]
job_class = options[:class].constantize unless job_class.is_a?(Class)

args = options[:args]
if args.blank?
job_class.set(cron: options[:cron]).perform_later
elsif args.is_a?(Array)
job_class.set(cron: options[:cron]).perform_later(*args)
elsif args.is_a?(Hash)
job_class.set(cron: options[:cron]).perform_later(**args)
else
raise 'invalid args option.'
end
end
end

def cron_settings
@cron_settings ||= Rails.application.config_for('delayed_cron_jobs')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delayed_cron_jobs.yamlのテンプレートは作成しない?

上記のテンプレートを作成するかどうかの設定化もやった方がよさそう。
そして、READMEにも追加したいね。

まだ検証フェーズということであればあとでやるでもよさそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

テンプレートジェネレートした方が良さそう。
追加しておきます!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end
end
end
1 change: 1 addition & 0 deletions lib/sg_fargate_rails/railtie.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'sg_fargate_rails/adjust_cloudfront_headers'
require 'sg_fargate_rails/delayed_cron_job_utility'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delayed_jobが有効な場合のみrequireするほうが良さそう。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ですよね。

defined?(::DelayedCronJob) は、このタイミングではクラスロードの関係か、まだ使えませんでした。
その場合どんな判定処理をするのがよさそうでしょう?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

必要になったタイミングでrequireするほうがエコだし、refresh_delayed_cron_jobsのrakeタスク内でrequireすれば良さそう!


https://github.com/SonicGarden/jobmon_ruby/blob/b8867774137fc5e1ddd58366bdd3aa4c23a9caab/lib/tasks/sidekiq_queue_monitor.rake#L9

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

そうか!ここで require するものと完全に思い込んでました!

require 'sg_fargate_rails/healthcheck'
require 'sg_fargate_rails/maintenance'
require 'sg_fargate_rails/remote_ip'
Expand Down
7 changes: 7 additions & 0 deletions lib/tasks/sg_fargate_rails.rake
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,11 @@ namespace :sg_fargate_rails do
)
end
end

desc 'Refresh Delayed Cron Jobs'
task refresh_delayed_cron_jobs: :environment do
Rails.logger.info('[refresh_delayed_cron_jobs] refresh begin...')
SgFargateRails::DelayedCronJobUtility.refresh_cron_jobs!
Rails.logger.info('[refresh_delayed_cron_jobs] refresh end.')
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delayed_jobが有効な場合のみrakeタスクを定義するほうが良さそう。

end