Add QoS to metadata (re-do #330)

[emersonknapp]

Part of #125
This is an update of #330, which was reverted

• This PR adds a field offered_qos_profiles to TopicMetadata. The field is not yet filled by anything, and is is not deserialized on loading.
• bumps the metadata version to 4 to reflect the change in data model

Next step will be to query and store the QoS profiles for recorded topics. The reason we are not deserializing is because the metadata version will need to be passed to deserialization, or the deserialization will need to just be tolerant to the missing field. We'll address that decision in its own PR.

rosbag2_transport constructs the TopicMetadata, so that's where the serialization/deserialization will need to occur (in recorder.cpp / player.cpp)

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

[Karsten1987]

lgtm with green CI ;-)

[emersonknapp]

• Linux
• Linux-aarch64
• macOS
• Windows

It was a symbol visibility issue for the test executable to link the qos utility functions. Added the explicit yaml dependency while I was poking around

[emersonknapp]

Sanity re-test for windows - the failures seem really unrelated and are potentially just flaky
Double sanity check on master without this change

[emersonknapp]

@Karsten1987 the buildfarm is showing the exact same test failures on windows on master as with this PR. do you think we're good to merge?

[emersonknapp]

@Karsten1987 this particular test is failing on the nightly build as well https://ci.ros2.org/view/nightly/job/nightly_win_deb/1573/testReport/junit/rosbag2_tests/RecordFixture/record_end_to_end_test_with_zstd_file_compression_compresses_files/ - in order to not block this feature due to unrelated issues, I'm going to merge now

[Karsten1987]

while debugging #329 I've noticed the following test error with cyclonedds:

[1.348s] 7: /home/karsten/ros2_ws/src/ros2/rosbag2/rosbag2_transport/test/rosbag2_transport/test_record.cpp:99: Failure
[1.349s] 7: Value of: serialized_profiles
[1.349s] 7: Expected: contains regular expression "- history: 3\n  depth: 0\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 2147483647\n    nsec: 4294967295\n  lifespan:\n    sec: 2147483647\n    nsec: 4294967295\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 2147483647\n    nsec: 4294967295\n  avoid_ros_namespace_conventions: false"
[1.349s] 7:   Actual: "- history: 1\n  depth: 10\n  reliability: 1\n  durability: 2\n  deadline:\n    sec: 9223372036\n    nsec: 854775807\n  lifespan:\n    sec: 9223372036\n    nsec: 854775807\n  liveliness: 1\n  liveliness_lease_duration:\n    sec: 9223372036\n    nsec: 854775807\n  avoid_ros_namespace_conventions: true"
[1.352s] 7: [  FAILED  ] RecordIntegrationTestFixture.qos_is_stored_in_metadata (172 ms)

[emersonknapp]

Thanks for pointing that out! I think we should call this a bug in rmw_cyclone.

See how the different values are the first two, around history.

• History/depth policy have no bearing on compatibility, so it is not useful to know what the endpoint uses
• The other RMW implementations return RMW_QOS_HISTORY_POLICY_UNKNOWN and depth=0, which makes all endpoint info consistent, which makes it much easier to respect the above point

EDIT: I now see that the "infinity" values for the rmw_time_t types are also different. This is problematic in a different way. FastRTPS and Connext were both returning the same values for infinity, but Cyclone seems to be using something different.

The values I was expecting here were sec = 0x7FFFFFFF (signed 32 bit int max) and nsec = 0xFFFFFFFF (unsigned 32 bit int max, or -1?).

The values Cyclone is returning are sec = 9223372036 == 0x225C17D04 and nsec = 854775807 == 0x32F2D7FF which my brain sees as totally random.

[Karsten1987]

do you mind reporting this on https://github.com/ros2/rmw_cyclonedds? I feel you have more insights on QoS than me to give a meaningful issue report ;-)

However, just for my understanding: If history/depth have no influence, why are we testing for it then? We rely on every rmw implementation to adhere to some sort of default then. Are these defaults documented?

[emersonknapp]

I think it's reasonable to remove the check for it in the test you saw failing. I'm worried about cross-rmw compatibility, seeing these values, though.

EDIT: see previous comment for more analysis on the values