Pick smallest available participant ID for new paricipants by sloretz · Pull Request #3437 · eProsima/Fast-DDS

sloretz · 2023-04-05T20:29:42Z

Picks the smallest available participant ID.

Description

This picks the smallest available participant ID when creating new participants.
This is necessary for the out of box discovery improvements in ROS 2: ros2/rmw_fastrtps#653

The issue with picking one greater than the maximum participant ID is that it's possible for the ID to increase well beyond the number of alive participants. For example, Participant A is created with 0, and B is created with 1. A is destroyed, and C is created. C takes ID 2 even though 0 is available and there are only two participants. When using unicast discovery this can cause participants to no longer be discoverable after the maximum ID climbs past maxInititialPeersRange. This is currently causing the test_launch_ros tests to fail with the change to unicast discovery by default.

Assuming the change looks ok, would it be possible to backport this to the 2.10 branch (assuming that's the branch that will be used for ROS Iron)?

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
Any new/modified methods have been properly documented using Doxygen.
Changes are ABI compatible.
Changes are API compatible.
N/A New feature has been added to the versions.md file (if applicable).
N/A New feature has been documented/Current behavior is correctly described in the documentation.
N/A Applicable backports have been included in the description.

Reviewer Checklist

The PR has a milestone assigned.
Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

MiguelCompany · 2023-04-05T21:40:43Z

@richiprosima please test this

MiguelCompany · 2023-04-06T11:57:16Z

@sloretz Reusing a participant ID within the same process is a bad idea, since it will allow for two participants to have the same GUID.

I am developing a different way of achieving the expected behavior, where the ports used do not depend on the participantID, and we always try with the lowest possible port.

sloretz · 2023-04-06T13:53:07Z

I am developing a different way of achieving the expected behavior, where the ports used do not depend on the participantID, and we always try with the lowest possible port.

@MiguelCompany I thought the ports being calculated from the participant ID was part of the RTPS standard.

https://fast-dds.docs.eprosima.com/en/latest/fastdds/xml_configuration/domainparticipant.html#port-configuration

and

https://www.omg.org/spec/DDSI-RTPS/2.3/PDF

I see in 9.6.1.1 it says

The domainId and participantId identifiers are used to avoid port conflicts among Participants on the same
node. Each Participant on the same node and in the same domain must use a unique participantId. In the case
of multicast, all Participants in the same domain share the same port number, so the participantId identifier is
not used in the port number expression.

It seems like it would be OK to reuse a participant ID as no two participants would have the same ID at the same time. If I understand correctly, wouldn't the same thing happen if participants were in different processes and someone were to stop and restart the last participant on a machine?

If the GUID being reused is a problem, maybe a solution is to change how the participant GUID is generated? It looks like the standard is more flexible about that.

8.2.4.2 The GUIDs of RTPS Participants
Every Participant has GUID <prefix, ENTITYID_PARTICIPANT>, where the constant
ENTITYID_PARTICIPANT is a special value defined by the RTPS protocol. Its actual value depends on the
PSM.

The implementation is free to choose the prefix, as long as every Participant in the Domain has a unique GUID.

MiguelCompany · 2023-04-06T17:11:45Z

@richiprosima Please test this

MiguelCompany · 2023-04-06T17:16:53Z

@sloretz On f6f9840c28ed4282a70b95bd32a052ab98a6d46a I added a small change that makes the GUID be different when creating the same participant ID twice.

The idea is to use a counter on the high part of the ID that is used to generate the GUID.

MiguelCompany · 2023-04-06T18:04:58Z

@richiprosima Please test this

wjwwood

lgtm

Signed-off-by: Shane Loretz <sloretz@google.com>

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany · 2023-04-07T14:52:35Z

@richiprosima Please test this

MiguelCompany · 2023-04-07T14:57:04Z

@sloretz @wjwwood I rebased this and added a mechanism to keep track of participant ids that are reserved when calling create_participant_guid from here and used later on participant creation here

This should solve the tests that were failing yesterday.

MiguelCompany · 2023-04-07T14:57:25Z

@EduPonz Would you mind reviewing this?

EduPonz

This LGTM. I think the added members (data & functions) would benefit from some hpp documentation. The same goes for some of the not so straight forward code both around the reserved/used pattern (clever way of going around the full locking BTW), and around the get_id_for_prefix function, which shows some nice out-of-the-box thinking 🤯 .

jepemi · 2024-03-13T11:32:55Z

@Mergifyio backport 2.6.x

mergify · 2024-03-13T11:33:05Z

backport 2.6.x

✅ Backports have been created

Details

#4555 Pick smallest available participant ID for new paricipants (backport #3437) has been created for branch 2.6.x but encountered conflicts

* Pick smallest available participant ID for new paricipants Signed-off-by: Shane Loretz <sloretz@google.com> * Fix style issues Signed-off-by: Shane Loretz <sloretz@google.com> * Eliminate m_maxRTPSParticipantId Signed-off-by: Shane Loretz <sloretz@google.com> * Clearer comments on rationale behind return value Signed-off-by: Shane Loretz <sloretz@google.com> * static_cast to avoid warning on WIN32 Signed-off-by: Shane Loretz <sloretz@google.com> * Use special participant id value for prefix creation. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * New reservation mechanism Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Shane Loretz <sloretz@google.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Miguel Company <MiguelCompany@eprosima.com> (cherry picked from commit e46e875) # Conflicts: # src/cpp/rtps/RTPSDomain.cpp # src/cpp/rtps/RTPSDomainImpl.hpp

…4555) * Pick smallest available participant ID for new paricipants (#3437) * Pick smallest available participant ID for new paricipants Signed-off-by: Shane Loretz <sloretz@google.com> * Fix style issues Signed-off-by: Shane Loretz <sloretz@google.com> * Eliminate m_maxRTPSParticipantId Signed-off-by: Shane Loretz <sloretz@google.com> * Clearer comments on rationale behind return value Signed-off-by: Shane Loretz <sloretz@google.com> * static_cast to avoid warning on WIN32 Signed-off-by: Shane Loretz <sloretz@google.com> * Use special participant id value for prefix creation. Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> * New reservation mechanism Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> --------- Signed-off-by: Shane Loretz <sloretz@google.com> Signed-off-by: Miguel Company <MiguelCompany@eprosima.com> Co-authored-by: Miguel Company <MiguelCompany@eprosima.com> (cherry picked from commit e46e875) # Conflicts: # src/cpp/rtps/RTPSDomain.cpp # src/cpp/rtps/RTPSDomainImpl.hpp * Refs #20120: Solve conflicts Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #20120: Fix wrong deleted return Signed-off-by: Jesus Perez <jesusperez@eprosima.com> * Refs #20120: Uncrustify Signed-off-by: Jesus Perez <jesusperez@eprosima.com> --------- Signed-off-by: Jesus Perez <jesusperez@eprosima.com> Co-authored-by: Shane Loretz <sloretz@google.com> Co-authored-by: Jesus Perez <jesusperez@eprosima.com>

sloretz mentioned this pull request Apr 5, 2023

[rmw_fastrtps] Improve handling of dynamic discovery ros2/rmw_fastrtps#653

Merged

MiguelCompany added this to the v2.10.2 milestone Apr 5, 2023

sloretz force-pushed the sloretz__smallest_participant_id branch from 2dc0abf to f446ceb Compare April 5, 2023 23:55

sloretz mentioned this pull request Apr 6, 2023

Improve handling of dynamic discovery ros2/ros2#1359

Closed

8 tasks

wjwwood reviewed Apr 7, 2023

View reviewed changes

sloretz and others added 6 commits April 7, 2023 16:40

Pick smallest available participant ID for new paricipants

bdb77a6

Signed-off-by: Shane Loretz <sloretz@google.com>

Fix style issues

b03d022

Signed-off-by: Shane Loretz <sloretz@google.com>

Eliminate m_maxRTPSParticipantId

1f9f9a8

Signed-off-by: Shane Loretz <sloretz@google.com>

Clearer comments on rationale behind return value

af9ad6a

Signed-off-by: Shane Loretz <sloretz@google.com>

static_cast to avoid warning on WIN32

6e0980c

Signed-off-by: Shane Loretz <sloretz@google.com>

Use special participant id value for prefix creation.

3df4fb1

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the sloretz__smallest_participant_id branch from 7160972 to e4f0c60 Compare April 7, 2023 14:44

New reservation mechanism

8850078

Signed-off-by: Miguel Company <MiguelCompany@eprosima.com>

MiguelCompany force-pushed the sloretz__smallest_participant_id branch from e4f0c60 to 8850078 Compare April 7, 2023 14:52

EduPonz approved these changes Apr 7, 2023

View reviewed changes

EduPonz merged commit e46e875 into eProsima:master Apr 7, 2023

sloretz deleted the sloretz__smallest_participant_id branch April 20, 2023 22:13

EduPonz mentioned this pull request Aug 17, 2023

Unable to have a successful test with the "Connecting Nodes over an External Network" example eProsima/vulcanexus#148

Closed

mergify bot mentioned this pull request Mar 13, 2024

Pick smallest available participant ID for new paricipants (backport #3437) #4555

Merged

10 tasks

Conversation

sloretz commented Apr 5, 2023 • edited by EduPonz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contributor Checklist

Reviewer Checklist

Uh oh!

MiguelCompany commented Apr 5, 2023

Uh oh!

MiguelCompany commented Apr 6, 2023

Uh oh!

sloretz commented Apr 6, 2023

Uh oh!

MiguelCompany commented Apr 6, 2023

Uh oh!

MiguelCompany commented Apr 6, 2023

Uh oh!

MiguelCompany commented Apr 6, 2023

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

MiguelCompany commented Apr 7, 2023

Uh oh!

MiguelCompany commented Apr 7, 2023

Uh oh!

MiguelCompany commented Apr 7, 2023

Uh oh!

EduPonz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jepemi commented Mar 13, 2024

Uh oh!

mergify bot commented Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sloretz commented Apr 5, 2023 •

edited by EduPonz

Loading

EduPonz left a comment •

edited

Loading

mergify bot commented Mar 13, 2024 •

edited

Loading