Consistent node naming #158

AAlon · 2018-11-16T08:20:59Z

Summary

This is part of the improvements suggested here mainly intended for CLI tools' usability with security.

All ros2cli nodes will start with prefix ros2cli_node.

Related changes

Refer to

https://discourse.ros.org/t/ros2-security-cli-tools/6647/
ros2/sros2#69

ruffsl

I'm not sure a extending ROS2 CLI node names with a new longer hard coded prefix would be best, given that the length of our namespaces is finite from the restrictions in DDS. Like I mentioned elsewhere, exposing them as parameterizable arguments might be more flexible:
ros2/rosbag2#60 (comment)

ruffsl · 2018-11-16T19:08:29Z

ros2cli/ros2cli/node/direct.py

@@ -35,7 +33,7 @@ def timer_callback():

        node_name_suffix = getattr(
            args, 'node_name_suffix', '_%d' % os.getpid())
-        self.node = rclpy.create_node(HIDDEN_NODE_PREFIX + 'ros2cli_node' + node_name_suffix)
+        self.node = rclpy.create_node(CLI_NODE_NAME_PREFIX + node_name_suffix)


Wouldn't this remove this node's status as 'hidden'?

Yes, that's the intention. Some CLI nodes are hidden while others are not, and I haven't found a good reason for this discrepancy. Moreover, ROS1 CLI nodes all start up as visible. Since this hiding is just a CLI-level feature (rendered irrelevant by supplying the -a flag) I don't feel strongly either way but I feel like it should be consistent (either all hidden or all visible).

I feel all CLI tools used for debugging or status checking should be hidden by default and minimally intrusive, to avoid everyone else's workstations debugging the same platform/network from cluttering the shared data bus of everyone else, e.g. increasing the signal to noise ratio of parameters/topics/participant namespaces as more developers/cli-tools share the same network rather than solely from the number of robots. This is just my opinion from working on robotic competitions and hackathons during crowded events such as RoboCup, or shared research labs.

What CLI do you currently find to be inconsistent in being hidden?
Perhaps is worth fixing them as well to be hidden?

ros2 topic echo is hidden while ros2 topic pub is not. I can change all of them to be hidden, that's fine, as long as it's consistent :)

Updated: 5d2d22d

ruffsl · 2018-11-16T19:10:02Z

ros2cli/ros2cli/node/__init__.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+# TODO(mikaelarguedas) revisit this once it's specified
+HIDDEN_NODE_PREFIX = '_'


+1 in consolidating the declaration of HIDDEN_NODE_PREFIX

Imo the consolidation should not happen here in ros2cli (I assume that is why you kept the TODO comment) but go into rclpy. Similar to HIDDEN_TOPIC_PREFIX.

I believe ros2/rclpy#259 addresses this comment

AAlon · 2018-11-19T07:27:13Z

I'm not sure a extending ROS2 CLI node names with a new longer hard coded prefix would be best, given that the length of our namespaces is finite from the restrictions in DDS. Like I mentioned elsewhere, exposing them as parameterizable arguments might be more flexible:
ros2/rosbag2#60 (comment)

Some CLI nodes already starts with the ros2cli_node prefix, this is only about making it consistent across all CLI nodes.
Yes, we should allow the user to supply node and ns name. However, this is outside the scope of this change, which deals with making the default naming consistent. Most usage will be with the default names.
Since this applies only to the default naming (rather than user-supplied), correct me if I'm wrong but there should be no issues with name length. We know the expected length: "ros2cli_node_<requester/publisher/etc>[_<process-id>]". Can change the ros2cli_node to ros2cli if it's still a concern.

AAlon · 2018-12-10T20:37:05Z

Ping @ruffsl ^^

ruffsl · 2018-12-12T08:14:17Z

@AAlon , Could you rebase wrt current master?
There are some other commits in here that make sorting out the diff harder.

AAlon · 2018-12-12T22:04:46Z

@AAlon , Could you rebase wrt current master?
There are some other commits in here that make sorting out the diff harder.

Done.

ruffsl · 2018-12-13T19:21:32Z

Can change the ros2cli_node to ros2cli if it's still a concern.

I think that's a good idea, and would be my last nitpick.

AAlon · 2019-01-08T20:46:11Z

Are we ready to merge this? the rosbag build in ros2/rosbag2#60 wouldn't succeed unless it has ros2cli with this change.

dirk-thomas · 2019-01-08T21:01:20Z

ros2cli/ros2cli/node/__init__.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+# TODO(mikaelarguedas) revisit this once it's specified
+HIDDEN_NODE_PREFIX = '_'


Imo the consolidation should not happen here in ros2cli (I assume that is why you kept the TODO comment) but go into rclpy. Similar to HIDDEN_TOPIC_PREFIX.

dirk-thomas · 2019-01-08T21:02:45Z

ros2cli/ros2cli/node/__init__.py

+# limitations under the License.
+# TODO(mikaelarguedas) revisit this once it's specified
+HIDDEN_NODE_PREFIX = '_'
+CLI_NODE_NAME_PREFIX = HIDDEN_NODE_PREFIX + 'ros2cli'


Nipick: why use CLI_ as the prefix for this variable? I would suggest either ROS2CLI_NODE_NAME_PREFIX (to match the package name) or just NODE_NAME_PREFIX (since the symbol is defined in the namespace of ros2cli).

Imo the consolidation should not happen here in ros2cli (I assume that is why you kept the TODO comment) but go into rclpy. Similar to HIDDEN_TOPIC_PREFIX.

Ok, I can expose it from rclpy/node.py. I kept the comment because I was moving the same piece of code to a different place, and wasn't sure what he meant by once it's specified :)

Actually, thinking some more, I'm not so sure about it.
I can make the change if you still think that's the way to go, but this feature is strictly at the ROS CLI level; the node is not inherently 'hidden' in any way. It's simply an interpretation given by the CLI based on a prefix. I would think exposing a variable named HIDDEN_NODE_PREFIX from rclpy might be confusing and incorrectly imply that there's an actual 'hidden node feature' implemented in ROS.

Thoughts?

Nipick: why use CLI_ as the prefix for this variable? I would suggest either ROS2CLI_NODE_NAME_PREFIX (to match the package name) or just NODE_NAME_PREFIX (since the symbol is defined in the namespace of ros2cli).

Sure, I can change that.

I would argue that the very same semanstic applies to HIDDEN_TOPIC_PREFIX and that is exposed by rclpy in order to establish a common understanding of what by convention is being considered "hidden".

I would argue that the very same semanstic applies to HIDDEN_TOPIC_PREFIX and that is exposed by rclpy in order to establish a common understanding of what by convention is being considered "hidden".

Yep, and I'd say HIDDEN_TOPIC_PREFIX could've been defined in ros2cli as well. I don't feel strongly though; made the changes and updated the PR.

I'd say HIDDEN_TOPIC_PREFIX could've been defined in ros2cli as well.

If the underscore prefix is supposed to be a generally agreed upon convention it isn't sufficient to define it in ros2cli. E.g. what if rviz want to show a drop down list of topics and ignore hidden ones? ros2cli isn't accessible for that. So it needs to be defined in the client library across all languages.

In the future the actual definition could be pushed down into a constant defined in a .msg / .idl file.

…CLI nodes with a consistent prefix.

…stent node naming for ros2cli.

…_ros2cli'

…X -> NODE_NAME_PREFIX.

AAlon · 2019-01-11T00:33:03Z

Corresponding rclpy change: ros2/rclpy#259

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette · 2019-01-25T20:10:56Z

It looks like all of the previous comments were fixed. The automated CI is going to fail here because ros2/rclpy#259 isn't in Crystal; that's fine. The code looks good to me, so I'm going to launch CI on it; assuming that is green, I'll approve.

@AAlon We just started requiring DCO on commits on this repository, so if you can rebase and add the Signed-off-by for all of the commits (and squash them while you are at it), we can then get this in. Thanks.

clalancette · 2019-01-25T20:13:16Z

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2019-01-25T20:17:31Z

We just started requiring DCO on commits on this repository, so if you can rebase and add the Signed-off-by for all of the commits (and squash them while you are at it), we can then get this in. Thanks.

We don't enforce the DCO check yet. So no need to update this PR which was opened long before.

cottsay · 2019-01-28T22:55:44Z

It looks like the CI jobs came back green, @clalancette. If there are no objections, I think this is ready to merge (without DCO sign-off).

fix deprecation warning related to collections.abc

…ame scope (ros2#158) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

tfoote added the in review Waiting for review (Kanban column) label Nov 16, 2018

This was referenced Nov 16, 2018

Consistent node naming across ros2cli tools ros2/rosbag2#60

Merged

Security directory lookup improvements ros2/rcl#332

Merged

ruffsl reviewed Nov 16, 2018

View reviewed changes

AAlon force-pushed the ros2sec branch from e54cecb to 3bccd8a Compare December 12, 2018 22:04

ruffsl approved these changes Dec 14, 2018

View reviewed changes

clalancette self-requested a review December 20, 2018 18:18

dirk-thomas reviewed Jan 8, 2019

View reviewed changes

AAlon added 7 commits January 11, 2019 00:14

Support for easy integration with ros2 security features by starting …

e706cb1

…CLI nodes with a consistent prefix.

Removing unneeded comment

122490d

Making DirectNode visible (removing hidden node prefix) to have consi…

2c493e3

…stent node naming for ros2cli.

Start all CLI nodes as hidden.

2d5892f

Shortening the default CLI node name prefix from '_ros2cli_node' to '…

0747f45

…_ros2cli'

Importing HIDDEN_NODE_PREFIX from rclpy, renaming CLI_NODE_NAME_PREFI…

19ec2fd

…X -> NODE_NAME_PREFIX.

ros2node - Importing HIDDEN_NODE_PREFIX from rclpy

5fd1232

AAlon force-pushed the ros2sec branch from 6e56075 to 5fd1232 Compare January 11, 2019 00:18

AAlon mentioned this pull request Jan 11, 2019

Adding HIDDEN_NODE_PREFIX definition to node.py ros2/rclpy#259

Merged

Linter fixes.

10c9614

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

clalancette merged commit 9186400 into ros2:master Jan 29, 2019

clalancette removed the in review Waiting for review (Kanban column) label Jan 29, 2019

chapulina mentioned this pull request Jan 29, 2019

Crystal Patch Release 2 ros2/ros2#647

Closed

Karsten1987 mentioned this pull request Feb 28, 2019

Failed to load entry point 'play': cannot import name 'NODE_NAME_PREFIX' ros2/rosbag2#92

Closed

esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022

Merge pull request ros2#158 from ros2/fix_deprecation_warning

135420e

fix deprecation warning related to collections.abc

esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022

Add a SetParameter action that sets a parameter to all nodes in the s…

bd54a04

…ame scope (ros2#158) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent node naming #158

Consistent node naming #158

AAlon commented Nov 16, 2018 •

edited

Loading

ruffsl left a comment

ruffsl Nov 16, 2018

AAlon Nov 19, 2018

ruffsl Dec 13, 2018

AAlon Dec 13, 2018

AAlon Dec 13, 2018

ruffsl Nov 16, 2018

dirk-thomas Jan 8, 2019

chapulina Jan 11, 2019

AAlon commented Nov 19, 2018

AAlon commented Dec 10, 2018

ruffsl commented Dec 12, 2018

AAlon commented Dec 12, 2018

ruffsl commented Dec 13, 2018

AAlon commented Jan 8, 2019

dirk-thomas Jan 8, 2019

dirk-thomas Jan 8, 2019

AAlon Jan 8, 2019

AAlon Jan 8, 2019 •

edited

Loading

dirk-thomas Jan 10, 2019

AAlon Jan 11, 2019

dirk-thomas Jan 11, 2019

AAlon commented Jan 11, 2019

clalancette commented Jan 25, 2019

clalancette commented Jan 25, 2019 •

edited

Loading

dirk-thomas commented Jan 25, 2019

cottsay commented Jan 28, 2019

Consistent node naming #158

Consistent node naming #158

Conversation

AAlon commented Nov 16, 2018 • edited Loading

Summary

Related changes

Refer to

ruffsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAlon commented Nov 19, 2018

AAlon commented Dec 10, 2018

ruffsl commented Dec 12, 2018

AAlon commented Dec 12, 2018

ruffsl commented Dec 13, 2018

AAlon commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAlon Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AAlon commented Jan 11, 2019

clalancette commented Jan 25, 2019

clalancette commented Jan 25, 2019 • edited Loading

dirk-thomas commented Jan 25, 2019

cottsay commented Jan 28, 2019

AAlon commented Nov 16, 2018 •

edited

Loading

AAlon Jan 8, 2019 •

edited

Loading

clalancette commented Jan 25, 2019 •

edited

Loading