Add options extensions to ros2launch and extensibility to the node action #216

gbiggs · 2021-01-26T01:18:19Z

This PR is a response to #180. In that PR, a request was made that extensibility be added to the launch system and the --secure option be added as an extension. This PR provides the extensibility support. The security extension is provided in a PR on a new repository.

It adds:

The ability to load extensions that provide additional options to the ros2 launch command. For example, an extension can add an option --secure. Aside from getting the chance to add as many options as desired to the command line flags, extensions are given the chance to execute code prior to the LaunchService starting, prior to the launch being executed, and after the launch process completes (i.e. on shutdown?).
The ability to load extensions that add additional command line flags to the command lines that launch nodes, and execute code just before the node is launched. This is a short-term solution to the need for extensibility. In the long term we plan to refactor all of launch to be more amenable to extensibility.

For an example, see this PR which adds a security extension.

Once both PRs are accepted, the ros2launch_security repository will need to be transferred to the security WG.

gbiggs · 2021-02-02T03:36:19Z

Ready for review.

kyrofa · 2021-02-02T23:41:41Z

@gbiggs thank you for doing this. I will leave a code review to more qualified people, but I'll say we do believe this covers the functionality we would need for what we're doing.

gbiggs · 2021-02-02T23:43:27Z

@kyrofa Let us know who you want to set as maintainer, and if the security WG wants to take the plugins one.

kyrofa · 2021-02-02T23:59:58Z

@gbiggs I can take it and add the necessary people if that works.

gbiggs · 2021-02-07T19:48:54Z

Great, thanks! I've added you as maintainer to the new package.

Arnatious

Looking great, only some minor nitpicks.

Arnatious · 2021-02-10T19:03:07Z

launch_ros/launch_ros/actions/node.py

@@ -14,8 +14,10 @@

 """Module for the Node action."""

+import importlib_metadata


Third party package, should be in the same block as yaml

Fixed in ded9e1d

Arnatious · 2021-02-10T19:03:11Z

launch_ros/launch_ros/actions/node.py

 import os
 import pathlib
+from packaging.version import Version


Third party package, should be in the same block as yaml

Fixed in ded9e1d

Arnatious · 2021-02-10T19:33:26Z

ros2launch/ros2launch/api/api.py

+        )
+        launch_description = result[0]
+        if len(result) > 1:
+            tails[name] = result[1:]


Is this variable used solely for debugging? It isn't ever read from.

Yes, it was. Fixed in 22d14af

ros-discourse · 2021-02-22T17:46:29Z

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-02-18/19102/1

hidmic

First pass!

hidmic · 2021-03-15T14:46:53Z

ros2launch/ros2launch/option/__init__.py

+    * `NAME` (will be set to the entry point name)
+
+    The following methods may be defined:
+    * `add_arguments` to add arguments to the argparse parser


@gbiggs nit: consider moving documentation to each method, and only listing which ones are required here.

Done in 1e18575

hidmic · 2021-03-15T14:47:19Z

ros2launch/ros2launch/command/launch.py

@@ -145,6 +150,7 @@ def main(self, *, parser, args):
        else:
            raise RuntimeError('unexpected mode')
        launch_arguments.extend(args.launch_arguments)
+


@gbiggs unrelated?

This vanished when I rebased.

hidmic · 2021-03-15T14:50:03Z

ros2launch/ros2launch/option/__init__.py

+    def prelaunch(self, launch_description, options, args):
+        return (launch_description,)
+
+    def postlaunch(self, launch_return_code, options, args):


@gbiggs hmm, what is this options argument for? BTW it doesn't seem like prelaunch and postlaunch are provided with it in any call site.

It looks like it was vestigial of an earlier iteration of the changes. I've removed the arguments in 40ee929.

hidmic · 2021-03-15T14:51:25Z

launch_ros/launch_ros/actions/node.py

+    * `NAME` (will be set to the entry point name)
+
+    The following methods may be defined:
+    * `command_extension` to extend the command used to launch the node's


@gbiggs nit: consider moving documentation to each method, and only listing which ones are required here.

Done in 6754e9a.

hidmic · 2021-03-15T14:53:58Z

launch_ros/launch_ros/actions/node.py

@@ -408,6 +453,10 @@ def execute(self, context: LaunchContext) -> Optional[List[Action]]:
            ros_specific_arguments['name'] = '__node:={}'.format(self.__expanded_node_name)
        if self.__expanded_node_namespace != '':
            ros_specific_arguments['ns'] = '__ns:={}'.format(self.__expanded_node_namespace)
+        node_info = NodeActionExtension.NodeInfo(self.__package, self.__node_executable, self.node_name)
+        for extension in self.__extensions.values():
+            ros_specific_arguments = extension.execute(context, ros_specific_arguments, node_info)


@gbiggs why not passing the node action itself instead of this ad-hoc data structure?

It seemed cleaner to pass in a minimal data structure rather than have extensions need to know about the entire Node action class and its internal variables.

I see. I will point out that this data in particular (e.g. the package name) could be exposed via public API. You wouldn't have to access internal implementations details from the extension in that case.

I've added public API to expose the package and executable, and removed the node_info structure in 2d9f3d1.

hidmic · 2021-03-15T14:56:03Z

launch_ros/launch_ros/actions/node.py

@@ -342,6 +383,10 @@ def _perform_substitutions(self, context: LaunchContext) -> None:
            raise
        self.__final_node_name = prefix_namespace(
            self.__expanded_node_namespace, self.__expanded_node_name)
+
+        for extension in self.__extensions.values():
+            self.cmd.extend(extension.command_extension(context))


@gbiggs why a method to specifically extend the command? Isn't it possible to do so (even resolving substitutions) upon extension execution?

I can't remember specifically, but I suspect I was following the existing pattern of the Node class, which splits up building the command and executing it. Possibly it does things that way for debug output?

Hmm, neither NodeExtension.command_extension nor NodeExtension.execute deal with execution. Both configure the command line, in direct and indirect ways respectively.

SecurityNodeActionExtension.execute could fetch an enclave and extend the command line w/o the extra step.

It's not just fetching an enclave, it's doing stuff that has side effects. In the case of the security extension, it's actually creating and using files on disc. There's also a need to add an option to the ros2 launch command line for the user to use, which is why there is the separate command_extension method. The function documentation for that was wrong; I've fixed it in c2b2b7f.

Hmm, I'm a bit confused. From what I can see in this patch, NodeActionExtension.command_extension is not used by ros2 launch directly, but within Node._perform_substitutions to extend Node.cmd. In which case, my point is that both NodeActionExtension.command and NodeActionExtension.pre_execute happen on Action.execute. There's no configuration stage in between. An indirection through context locals would make sense if there was a context global to override (or not).

Anyhow, I won't block on this. But we do need that documentation sorted out.

OK, I was the one confused. Sorry about that. I was getting it mixed up with the ros2 launch command line flag adding, which is handled by the options extension.

I looked over the code and I agree that it is possible to combine the two functions. I can make an argument against combining them that the current way follows the structure of the Node action in that it partitions preparing the command line from preparing ROS-specific arguments and executing the command, but it's a weak argument. So I went ahead and changed it to match your suggestion in da09c67.

hidmic · 2021-03-15T14:57:15Z

launch_ros/launch_ros/actions/node.py

@@ -432,3 +481,86 @@ def expanded_node_namespace(self):
    def expanded_remapping_rules(self):
        """Getter for expanded_remappings."""
        return self.__expanded_remappings
+
+    def _get_extensions(self):


@gbiggs nit: consider making this a free function ~~or staticmethod~~ (better not, too much support logic inside the Node class) that returns the list of extensions.

This function accesses the node's logger to output an error message that is specific to the function's internal implementation. I could refactor that out into returning an optional error message, but I'm wary of going to far for something that's meant to be temporary. How strongly do you feel about this?

How strongly do you feel about this?

It's a bit too much unrelated code. Also, as far as I can see the logger is fixed for the entire module. It should be straightforward to fetch it again if need be.

for something that's meant to be temporary.

Until we get around to replace it, it is permanent.

I've made the two functions free in 0d9cb67.

Until we get around to replace it, it is permanent

Fair point! :)

hidmic · 2021-03-15T14:58:30Z

launch_ros/launch_ros/actions/node.py

+            "supports '%s'" % (extension_point_version, extension_version))
+
+
+def get_upper_bound_caret_version(version):


@gbiggs consider moving these functions to another file.

Done in 6754e9a.

hidmic · 2021-03-15T15:00:01Z

Also, this patch needs conflicts resolved.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2021-03-17T08:25:04Z

Also, this patch needs conflicts resolved.

Rebased to fix this. I'll start addressing your other comments now.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2021-03-18T01:24:21Z

@hidmic Thanks for the comments. This is ready for another pass.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic

Overall LGTM but for a few comments.

Once we settle those we need CI.

hidmic · 2021-03-22T14:52:33Z

launch_ros/launch_ros/actions/node.py

@@ -432,3 +481,86 @@ def expanded_node_namespace(self):
    def expanded_remapping_rules(self):
        """Getter for expanded_remappings."""
        return self.__expanded_remappings
+
+    def _get_extensions(self):


How strongly do you feel about this?

It's a bit too much unrelated code. Also, as far as I can see the logger is fixed for the entire module. It should be straightforward to fetch it again if need be.

for something that's meant to be temporary.

Until we get around to replace it, it is permanent.

hidmic · 2021-03-22T15:01:10Z

launch_ros/launch_ros/actions/node.py

@@ -408,6 +453,10 @@ def execute(self, context: LaunchContext) -> Optional[List[Action]]:
            ros_specific_arguments['name'] = '__node:={}'.format(self.__expanded_node_name)
        if self.__expanded_node_namespace != '':
            ros_specific_arguments['ns'] = '__ns:={}'.format(self.__expanded_node_namespace)
+        node_info = NodeActionExtension.NodeInfo(self.__package, self.__node_executable, self.node_name)
+        for extension in self.__extensions.values():
+            ros_specific_arguments = extension.execute(context, ros_specific_arguments, node_info)


I see. I will point out that this data in particular (e.g. the package name) could be exposed via public API. You wouldn't have to access internal implementations details from the extension in that case.

hidmic · 2021-03-22T15:04:21Z

launch_ros/launch_ros/actions/node.py

@@ -342,6 +383,10 @@ def _perform_substitutions(self, context: LaunchContext) -> None:
            raise
        self.__final_node_name = prefix_namespace(
            self.__expanded_node_namespace, self.__expanded_node_name)
+
+        for extension in self.__extensions.values():
+            self.cmd.extend(extension.command_extension(context))


Hmm, neither NodeExtension.command_extension nor NodeExtension.execute deal with execution. Both configure the command line, in direct and indirect ways respectively.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic · 2021-03-29T13:29:16Z

launch_ros/launch_ros/actions/node.py

@@ -342,6 +383,10 @@ def _perform_substitutions(self, context: LaunchContext) -> None:
            raise
        self.__final_node_name = prefix_namespace(
            self.__expanded_node_namespace, self.__expanded_node_name)
+
+        for extension in self.__extensions.values():
+            self.cmd.extend(extension.command_extension(context))


Hmm, I'm a bit confused. From what I can see in this patch, NodeActionExtension.command_extension is not used by ros2 launch directly, but within Node._perform_substitutions to extend Node.cmd. In which case, my point is that both NodeActionExtension.command and NodeActionExtension.pre_execute happen on Action.execute. There's no configuration stage in between. An indirection through context locals would make sense if there was a context global to override (or not).

Anyhow, I won't block on this. But we do need that documentation sorted out.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic

CI up to launch_ros, test_launch_ros, and ros2launch:

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-03-30T20:53:15Z

@gbiggs flake8 isn't happy, mind to check?

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs · 2021-04-01T01:21:59Z

@hidmic I fixed the two flake8 errors I could see in the output.

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

clalancette · 2021-04-01T17:52:56Z

I fixed a few more flake8 fixes that I saw locally (and in CI).

However, I am still seeing this warning:

launch_ros/actions/test_extension.py:18
  /home/ubuntu/ros2_ws/src/ros2/launch_ros/launch_ros/launch_ros/actions/test_extension.py:18: PytestCollectionWarning: cannot collect test class 'TestNodeActionExtension' because it has a __init__ constructor (from: launch_ros/actions/test_extension.py)
    class TestNodeActionExtension(NodeActionExtension):

I think what is happening here is that pytest is getting confused because the class starts with the name Test. Looking around, I don't see this class or file being used anywhere. Is it?

Arnatious · 2021-04-01T18:52:14Z

I think what is happening here is that pytest is getting confused because the class starts with the name Test. Looking around, I don't see this class or file being used anywhere. Is it?

For reference setting the class atrtibute __test__ = False will squelch that error

gbiggs · 2021-04-01T23:48:25Z

Sorry about that. That's a class I was using to do some quick tests. I thought I had deleted it, turns out I had committed it instead by mistake. I've pushed the removal of them.

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

clalancette · 2021-04-05T12:31:20Z

All right, tests are green locally. Let's run them in CI again and see what happens:

Linux
Linux-aarch64
macOS
Windows

clalancette · 2021-04-05T14:41:14Z

All right, this is approved by @hidmic, and CI is clean (other than the Windows failure, but that is unrelated to this PR). I'm going to go ahead and merge this, thanks for all of the work @gbiggs , and thanks for the reviews @hidmic.

hidmic · 2021-04-05T14:42:46Z

@clalancette shall we run a combined CI with osrf/ros2launch_security#1?

clalancette · 2021-04-05T14:45:40Z

Good idea, I'll go ahead and do that over on that pull request.

This was referenced Jan 26, 2021

Add security extensions osrf/ros2launch_security#1

Merged

add --secure option to launch with encryption #180

Closed

gbiggs changed the title ~~WIP: Add options extensions to ros2launch and extensibility to the node action~~ Add options extensions to ros2launch and extensibility to the node action Feb 2, 2021

Arnatious reviewed Feb 10, 2021

View reviewed changes

hidmic reviewed Mar 15, 2021

View reviewed changes

gbiggs added 4 commits March 17, 2021 17:24

Add extensible options

dc1a506

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Add extensibility to the node action

28b2970

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix imports ordering

5f56404

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Remove unused variable

24d5bbe

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs force-pushed the extensions-support branch from 22d14af to 24d5bbe Compare March 17, 2021 08:24

gbiggs added 7 commits March 17, 2021 17:44

Move documentation to each method

1e18575

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Remove extraneous files

95778e4

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Remove vestigial argument

40ee929

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Move utility functions to another file

6754e9a

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix syntax error introduced by rebase

a7e603b

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix missing module name

1fed4c5

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Remove vestigial test entry points

416f72e

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix flake8 errors

1958ed7

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic reviewed Mar 22, 2021

View reviewed changes

gbiggs added 2 commits March 29, 2021 15:44

Remove node_info structure and add public API

2d9f3d1

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Rename pre-execute method to better match its function

e8ecbcd

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

gbiggs added 5 commits March 29, 2021 16:34

Fix method documentation

c2b2b7f

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Make functions for getting extensions free functions

0d9cb67

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix flake8 errors

e3a2bd6

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix syntax errors

c7cbad4

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

Fix flake8 errors

72b3821

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic approved these changes Mar 29, 2021

View reviewed changes

Change extension API to use one function

da09c67

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

hidmic approved these changes Mar 30, 2021

View reviewed changes

Fix import ordering

172a638

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

More flake8 fixes.

e68baf8

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

Remove test extensions

94f5b52

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

clalancette added this to In progress in Galactic via automation Apr 5, 2021

clalancette merged commit cedfedf into ros2:master Apr 5, 2021

clalancette moved this from In progress to Needs Release in Galactic Apr 5, 2021

clalancette moved this from Needs Release to Done in Galactic May 23, 2021

		@@ -14,8 +14,10 @@

		"""Module for the Node action."""

		import importlib_metadata

		"supports '%s'" % (extension_point_version, extension_version))


		def get_upper_bound_caret_version(version):

Add options extensions to ros2launch and extensibility to the node action #216

Add options extensions to ros2launch and extensibility to the node action #216

Conversation

gbiggs commented Jan 26, 2021 • edited Loading

gbiggs commented Feb 2, 2021

kyrofa commented Feb 2, 2021

gbiggs commented Feb 2, 2021

kyrofa commented Feb 2, 2021

gbiggs commented Feb 7, 2021

Arnatious 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

ros-discourse commented Feb 22, 2021

hidmic 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

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

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

hidmic commented Mar 15, 2021

gbiggs commented Mar 17, 2021

gbiggs commented Mar 18, 2021

hidmic 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

hidmic left a comment

Choose a reason for hiding this comment

hidmic commented Mar 30, 2021

gbiggs commented Apr 1, 2021

clalancette commented Apr 1, 2021

Arnatious commented Apr 1, 2021

gbiggs commented Apr 1, 2021

clalancette commented Apr 5, 2021

clalancette commented Apr 5, 2021

hidmic commented Apr 5, 2021

clalancette commented Apr 5, 2021

gbiggs commented Jan 26, 2021 •

edited

Loading