search

xqms / rosmon

ROS node launcher & monitoring daemon
Other
180 stars 47 forks source link

Resolves #145 by providing a number of restarts threshold per node vi… #146

Closed MCFurry closed 3 years ago

MCFurry commented 3 years ago

Resolves #145 by providing a number of restarts threshold per node via ROS-launch arguments

I also included a small test-launch-file for a manual test.

xqms commented 3 years ago

Thanks for the PR, also for providing the test launch file 👍

The changes look good to me. I'm on vacation for the next week, so it'll be a few days before I can properly test this.

MCFurry commented 3 years ago

Alright, enjoy your holiday!

MCFurry commented 3 years ago

One issue though we found in our pipelines, is that the extra rosmon-* node-arguments in a launch-file are not roslaunch 'compatible'. I.e. when testing my test launchfile here using: mon launch rosmon_core respawns.launch No-one complains, but when doing the same with: roslaunch rosmon_core respawns.launch We get a: WARNING: [~/ros/melodic/system/src/rosmon/rosmon_core/test/respawns.launch] unknown <node> attribute 'rosmon-restart-warn-threshold'

But the same holds of course for the already present rosmon-cpu-limit and rosmon-memory-limit arguments.

Of course, we are in fact creating mon-launch files here, not ROS-launch files and except for the warning the actual launching with roslaunch still succeeds. What doesn't, is the standard roslaunch check our pipelines execute: rosrun roslaunch roslaunch-check launch

Would it make sense to create a rosmon specific monlaunch-check script, that knows about these arguments? So it would largely be identical to roslaunch-check but with some rosmon specific tweaks?

xqms commented 3 years ago

Just a quick comment: You can already use mon launch --benchmark my_launch_file.launch (maybe it's not a good name) to check if a launch file is loadable by rosmon.

Regarding roslaunch issuing a warning: I cannot think of a way to prevent that. But at least it's only a warning ;)

MCFurry commented 3 years ago

Ahh that's good to know! We might switch our pipeline checker to use that instead when using rosmon in a project. Thx!

xqms commented 3 years ago

Merged in a8b25058027e6ee9ca47a3ddf6f3473c29a46237 with some minor additions. I also added the new attribute to the documentation at https://wiki.ros.org/rosmon/XML.

Thanks for your contribution :+1:

MCFurry commented 3 years ago

Thank you for the review and merge! Are you planning on a new release of rosmon as well? (We might even make the next sync?)

xqms commented 3 years ago

Since I just merged #148 which is a larger technical change with potential side effects, I'd like to leave things here in master for a while to ensure proper testing before doing a new release.

MCFurry commented 3 years ago

Ahh I see, then we'll go from source for the time being instead of the apt-package. Since we're using rosmon pretty much daily, anything we can help with to speed up testing the current master?