Actions proposal

[sloretz]

This PR is contains proposed changes and additions to the actions design doc started by @gbiggs in PR #183. It is targeted at gh-pages instead of gbiggs:gh-pages so the PR appears on this repo and the waffle board.

[mikeferguson]

In addition to my individual comments on code above, I want to further talk about the states Accepted/Executing. To me, it seems that we need both?

Let's assume I'm porting something that was previously implemented with a SimpleActionServer:

• I presume that I would set a thread pool of 1 to process the execute() callback. This will make sure we only process one goal at a time.
• I presume there are some other thread(s) involved that call handle_goal(). Thus, multiple goals may be "accepted", each is added to some internal queue.
• The single thread calls execute() with each of my accepted goals. Just before the execute() is called, the caller marks the goal as having state "Executing". The execute() callback then eventually returns a result at which point the goal is either Succeeded, Cancelled, or Aborted.

Thus anytime I have more goals than execute threads, I need both "Executing" and "Accepted" as valid states, so that someone can determine which goals are actually being executed, versus which are queued up.

Is that logic correct? Or am I missing something?

I'd suggest we might want to add some additional information on the assumed threading model in this document, since that was a common headache with ROS1 actionlib.

[jacobperron]

Let's assume I'm porting something that was previously implemented with a SimpleActionServer:

• I presume that I would set a thread pool of 1 to process the execute() callback. This will make sure we only process one goal at a time.
• I presume there are some other thread(s) involved that call handle_goal(). Thus, multiple goals may be "accepted", each is added to some internal queue.
• The single thread calls execute() with each of my accepted goals. Just before the execute() is called, the caller marks the goal as having state "Executing". The execute() callback then eventually returns a result at which point the goal is either Succeeded, Cancelled, or Aborted.

Thus anytime I have more goals than execute threads, I need both "Executing" and "Accepted" as valid states, so that someone can determine which goals are actually being executed, versus which are queued up.

I think this logic is correct, and there is value in an additional state, "ACCEPTED". Another way about it is to have handle_goal() defer the result (which should be possible with ROS 2 services). In this scenario, the goal service requests could be queued and responded to when ready, but I guess this leaves some state tracking to the client. Originally, I was trying to avoid a preliminary state to EXECUTING to reduce complexity of the state machine. Adding an ACCEPTED state means that it is possible for a goal to be "recalled" before it transitions to EXECUTING. This starts to become more like the original ROS 1 Action Server state machine, where ACCEPTED is analogous to PENDING:

But even if we add an ACCEPTED (or PENDING) state, we still might be able to avoid the extra states implemented in ROS 1:

• REJECTED does not have to be explicitly modeled since a service call must be completed to accept a goal before a client is allowed to cancel.
• Unless people feel strongly that a RECALLING state is valuable, we could instead make a transition directly from ACCEPTED to CANCELLING (or CANCELED) when a client makes a cancel request.

In summary, I think adding a new state is a good idea, but I would call it PENDING.

---

I'd suggest we might want to add some additional information on the assumed threading model in this document, since that was a common headache with ROS1 actionlib.

Agreed. We were thinking that the user could have control over the threading model (e.g. use some predefined policies or implement their own). Maybe a section on this topic and some common use cases we imagine is warranted.

[mikeferguson]

@jacobperron I concur that ACCEPTED is useful, I also concur that we should try to avoid RECALLING/RECALLED and just go straight to CANCELLED.

[paulbovbel]

https://github.com/ros2/design/pull/193#discussion_r225316311 makes me wonder:

_action/fibonacci/<client_id>/feedback
_action/fibonacci/<client_id>/status

those look like 'internal' topics of some sort. Will actions play nicely with ROS2 bagging? In ROS1 actions were bagged (allowing introspection, logging, etc.), but ROS2 actions will likely involve services. Can services be bagged natively in ROS2?

[wjwwood]

those look like 'internal' topics of some sort.

The _ at the beginning of a "token" implies it is hidden by default. All this means in practice is that command line tools will not show it unless you ask them to do so (e.g. ros2 topic --include-hidden-topics ...), but otherwise they are just normal topics and can be subscribed to, echoed, or recorded. See:

https://design.ros2.org/articles/topic_and_service_names.html#hidden-topic-or-service-names

In ROS1 actions were bagged (allowing introspection, logging, etc.), but ROS2 actions will likely involve services. Can services be bagged natively in ROS2?

The plan is to have some way of recording service calls and responses in rosbag's, but there are some additional constraints we'd have to put on services in ROS 2 to make that happen (currently it's not required to use pub/sub to implement services, but something like that would be required in order to have a recording service snoop on the traffic between requester and replied). If you want to push on that issue, I'd bring it up on the rosbag discussions (though they may be too busy to discuss it in detail atm).

Also, we could have rosbag be aware of actions as a communication primitive and record the right things depending on your preferences (feedback and status only, requests and replies, etc...).

[sloretz]

I'd suggest we might want to add some additional information on the assumed threading model in this document, since that was a common headache with ROS1 actionlib.

@mikeferguson the threading model is controlled by the executor, which means a compose-able node with an action server doesn't have control over it. If an action server blocks in execute, then goal requests will be blocked too if a user runs it in a single threaded executor.

I'm not sure if the execute callback is a good idea for most action servers. Instead compose-able nodes should avoid blocking to be reusable in more executors. An action server author would need to use some form of a asynchronous programming with timers or guard condition callbacks to provide feedback and return a result. On the other hand, an execute callback that blocks forces an action server implementation to eventually return a result. The current plan is to offer both an execute callback and a way to provide a result later.

Thus anytime I have more goals than execute threads, I need both "Executing" and "Accepted" as valid states, so that someone can determine which goals are actually being executed, versus which are queued up.

With just Executing the only way to tell what goals are actually in progress is by the presence of feedback. Accepting before Executing would make it clearer when an action server is queuing goals. Adding Accepting also means handle_goal() needs a way to indicate that a goal is accepted but should not be executed yet.

[ruffsl]

#193 (comment) makes me wonder:

_action/fibonacci/<client_id>/feedback
_action/fibonacci/<client_id>/status

That same comment makes me alarmed! :skull_and_crossbones: :radioactive: :biohazard:

I would like to formally petition against the use of dynamic namespacing, or use of namespaces for ROS2 subsystems that are not deterministically defined before runtime and outside of user control; due in part that doing otherwise would significantly complicate security, specifically both the Policy Decision Point (PDP) and Policy Enforcement Point (PEP) implemented in the transport security.

Let's play with a toy example:
Say we want to amend a policy profile to account for a simple action server and client pair. From my understanding of the current proposal, such a profile might look vaguely something like this:

#### fibonacci server ``` xml CN=fibonacci_server ... ra/fibonacciFeedback ra/fibonacciStatus rr/fibonacciCancleReply rr/fibonacciGoalReply rr/fibonacciResultReply rq/fibonacciCancleRequest rq/fibonacciGoalRequest rq/fibonacciResultRequest DENY ... ``` #### fibonacci client ``` xml CN=fibonacci_client ... rq/fibonacciCancleRequest rq/fibonacciGoalRequest rq/fibonacciResultRequest ra/fibonacciFeedback ra/fibonacciStatus rr/fibonacciCancleReply rr/fibonacciGoalReply rr/fibonacciResultReply DENY ... ```

However, should the client_id be used in namespacing the action, then users would then already have to start resorting to permissive wildcards, like * in fnmatch, for basic first class ROS2 functionally; a much more dangerous prospect than static non-interpritided string matching.

This sounds like an ideal case for the "keyed data" feature in DDS which we're hopefully going to expose once we support IDL. The idea is that in your message definition you annotate that one of the fields is a key and then when subscribing each different value for the key produce a separate "instance" which essentially means a separate message queue.

As @wjwwood mentioned in his comment, keys are perhaps much more suited for this, and by incorporating such meta information into the message itself, one could then think of more fine-grained application oriented security features.

E.g. limiting client influence over other peer client requests; nodes are provisioned access to certain specified client ID ranges should be only be able to modify server request from other clients within their own range, like with GIDs in *nix. For example, an e-stop client could cancel all move-it goals for the assembly line, but robot-arm-1 should only be able to cancel goals that pertain to robot-arm-1 platform.

Summarizing:
To keep the security implementation for actions well defined, I'd discourage such dynamic elements, e.g. client/session/goal IDs and etc, from being inserted into the resolvable action substem namespacings.

[jacobperron]

Thanks for the review, everyone!

Updates to the design doc:

• Added ACCEPTED state to the goal state machine.
  • This allows users to queue/list goals and know which ones have begun executing.
• Removed transition EXECUTING -> CANCELED. This lets the core handle transitions after an accepted cancel request and the implementer confirm with 'set_canceled'.
• Minor typo, grammar, consistency corrections.
• Updated the dishwasher example to alleviate any confusion with IDs.
• Client is now the sole entity responsible for generating goal IDs as UUIDs.
• Added section in "Alternatives" regarding using multiple topics for feedback/status (instead of the proposed single topic per action server).

TODO:

• Add section summarizing differences between ROS 1 and ROS 2
  • Any differences in terminology
  • API differences side-by-side
  • Example from MoveIt!
• More sequence diagram examples

[gbiggs]

Also, we could have rosbag be aware of actions as a communication primitive and record the right things depending on your preferences (feedback and status only, requests and replies, etc...).

I think this is a requirement of making actions a first-class citizen in ROS2. All the tools, not just ros2 action, should be action-aware.

[sloretz]

Would this need an additional topic, or would the client side set up background thread that calls get_result() to wait for completion, then calls the callback as its final action?

@gbiggs If I understand correctly I don't think a topic or a background thread are needed. At the rcl layer the client would send the "get result" service request as soon as it learns the goal is accepted. When the "get result" service response is received the service becomes ready in the waitset. The executor in rclcpp or rclpy would call a callback in spin() to handle the result.

[jacobperron]

I'm going to approve, but there is one long outstanding discussion that is not resolved: #193 (comment) . @jacobperron I'd appreciate a comment to close out that thread, a new issue if there is further to discuss, or a clarification to the document before merge. Thanks.

Thanks, @clalancette! I made brief comment pointing to the continuation of the discussion in #203. Although it has been closed, I think we can postpone opening up another ticket until the issue is raised again or we decide it should be addressed.