-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: add autoware_node_death_monitor package for monitoring node crashes #1786
base: tier4/main
Are you sure you want to change the base?
Conversation
…shes Signed-off-by: Kyoichi Sugahara <kyoichi.sugahara@tier4.jp>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
実装の細かい部分は他の方に任せますが、ハイレベルな設計で気になったポイントを書きます。
・launch.logって必ず書き込まれるんだっけ?その保証をしておきたい。
・これってノードが死んだらDiagでエラーだす実装を追加する、で良いですよね?
・ファイルが見つからなかったときに、初回でエラーが出るだけなので「node_death_monitorが立ち上がっているからOKだと思っていたらファイル見れてなかった、実はノード死んでた」ケースがありそう。起動して10秒経ってもファイル見つからない場合は、それ自体でDiagエラー出した方が良い。
それ以外、全般的な方針は問題/違和感ありません。
cmake_minimum_required(VERSION 3.14) | ||
project(autoware_node_death_monitor) | ||
|
||
find_package(autoware_cmake REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about node_alive_monitor
or process_alive_monitor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed name to process_alive_monitor
in 572427d
@@ -0,0 +1,73 @@ | |||
# autoware_node_death_monitor | |||
|
|||
This package provides a monitoring node that detects ROS 2 node crashes by analyzing `launch.log` files, rather than subscribing to `/rosout` logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
launch.logにメッセージが出力されることって保証されてましたっけ?もしyesならREADMEに「ROS2の仕組みとして必ずlounch.logに出力される」とか、もしくは「ooオプションがONになっていることを確認すること」とかを記載したい。
std::streampos last_valid_pos = static_cast<std::streampos>(last_file_pos_); | ||
|
||
size_t iteration = 0; | ||
while (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while(true)
はNG, while(rclcpp::ok)
みたいなの無かったっけ。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 572427d
書き込まれることは保証されている認識ですが、どこに書き込まれるかは環境設定に依存しているので運用側とのコミュニケーションが必要と思っている点です。
です。
これは heartbeat topic を出して topic_state_monitor から確認する予定です。(異常系開発の議事録側には書いてたのですが、こちらに書いてないことで誤解招いてしまっているので、READMEに追記します 🙇 |
Signed-off-by: Kyoichi Sugahara <kyoichi.sugahara@tier4.jp>
…toring and diagnostic information Signed-off-by: Kyoichi Sugahara <kyoichi.sugahara@tier4.jp>
Signed-off-by: Kyoichi Sugahara <kyoichi.sugahara@tier4.jp>
|
@kyoichi-sugahara
|
@Naophis
ありがとうございます!折衷案の共有大変たすかります。 |
@kyoichi-sugahara 中間にlaunch.logを使った死活監視がいて、ROSとの界面を担うやつが標準入力を受けて鳴く という構成です。 この構成のメリットは、ECU内でautowareを分割起動する構成の場合でも、publishするnodeは1つで監視する非ROSの軽量なサービスを複数用意するだけで実現できる点です。 標準入力がセキュリティ的にもあまり受け入れられないかもな〜とか思ってたりもしますので、他のスマートな方法があれば是非という感じでした(ここが自分の中で悶々と悩んで作れなかったところです) |
@Naophis ちなみに細かい点で気になったのは call_diag で対象外のノードなどを指定する構造になっているかとおもいますが、ノードの指定は autoware_launch ではなく別リポジトリのyamlファイル等で指定する想定でしょうか? |
@kyoichi-sugahara このnodeってどのように起動し待機することを想定してますか?
どちらになります? |
アーキテクチャーのべき論でいえば、理解はできるのですがどれくらい怖いのかあまりイメージできていないです。
autoware_launch と一緒に起動です! |
問題が起きたとき、活躍してほしい時に限って道連れになるので、独立したほうが良いと思ってのコメントです。(ココらへんは理想形に対し近づき方・歩幅の問題かも?)
現在の手法だと、
が考えられます。
この部分については、なるほどです。 あとは、そもそも起動できてないやつって検出出来ないと思うのですが、それってありなんですかね・・・? 本当は、 ホワイトリストを持つ必要があるのですが、特定自動運行向けならありだと思います。 (pubsub_inspectorも似た方法でサーチしてます) |
たしかにですね。ECUごとに立ち上げればよいのではという感覚でいますが、その場合運用面の煩雑さが懸念ですかね。
ですね!ホワイトリストの作成の煩雑さを回避するべく、はじめはすべてのノードが立ち上がるという前提で今回のノード死検知でちゃんと起動していない Node を知りたいの要求を満たすイメージです。
並行して あれ、でもよく考えたら |
栗原さんに確認したところ、自動運転中は MRM に移行しないように
というアプローチ + ブラックリストの方がシンプルで筋が良いように感じてきました。 |
これはメモ程度の話なのですが 未知のnodeについては、ros2 topic echo/hz/list etc... をCLIで実行するとdiscovery protocolに負荷をかけて、たまに色々やらかすのですが、CLI実行時にもインスタントのnodeのエントリーは行われるので、”知らないNodeリスト"があると、別の用途もあるかもです。 |
Description
Draft PR mainly for reviewing changes
以下READMEに追記しますが、一旦ここにメモらせてください。→ b0dae02 で追記
Related links
Parent Issue:
How was this PR tested?
psim で以下の動作確認を実施済み
Notes for reviewers
None.
Interface changes
None.
Effects on system behavior
None.