iox-#2128 Draft design for named segments

[gpalmer-latai]

Design document outlining the intended implementation for #2128

Pre-Review Checklist for the PR Author

1. Add a second reviewer for complex new features or larger refactorings
2. Code follows the coding style of CONTRIBUTING.md
3. Tests follow the best practice for testing
4. Changelog updated in the unreleased section including API breaking changes
5. Branch follows the naming format (iox-123-this-is-a-branch)
6. Commits messages are according to this guideline
   • Commit messages have the issue ID (iox-#123 commit text)
   • Commit author matches Eclipse Contributor Agreement (and ECA is signed)
7. Update the PR title
   • Follow the same conventions as for commit messages
   • Link to the relevant issue
8. Relevant issues are linked
9. Add sensible notes for the reviewer
10. All checks have passed (except task-list-completed)
11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
12. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

• Commits are properly organized and messages are according to the guideline
• Code according to our coding style and naming conventions
• Unit tests have been written for new behavior
• Public API changes are documented via doxygen
• Copyright owner are updated in the changed files
• All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
• PR title describes the changes

Post-review Checklist for the PR Author

1. All open points are addressed and tracked via issues

References

• Closes TBD

[elBoberido]

Half way through it

[elBoberido]

When we create a new API, the user should explicitely tell what the behavior should be by either passing a list of segments or using a flag which indicates to get all segments with read or write access. The current API will keep its behavior but would be deprecated at the long term.

[gpalmer-latai]

This seems reasonable but it would be a somewhat breaking API change as users would need to include the flag, or explicitly pass an empty list that they are not currently passing.

This is in contrast to simply not providing a list, which can be interpreted as the deprecated behavior. I'm a bit ambivalent here. I'm open to a small initial breaking change followed by dropping support for the old behavior. Or we could simply start by printing a warning when no list/flag is provided.

[gpalmer-latai]

How about this (might be what you meant)?

• Users may provide a list. If they don't the default behavior is to map all shared memory segments the process has access to.
• Users may specify a flag indicating that they wish to map read-compatible, write-compatible, or only specified segments.
• If they say "only specified segments", and don't provide a list, it is an error.
• If they say "read-compatible" and provide a list, it should be an error because anything they specify will be redundant or invalid.
• If they say "write-compatible" and provide a list, that's fine. Maybe there should be a warning/error if they've specified a write-compatible segment because that is redundant. But this combination allows them to map all write-access segments as well as specific read-only-access segments
• If they don't specify the flag and don't specify a list, the behavior is the "deprecated" default of mapping all read (and write)-access segments, accompanied by a deprecation warning.
• If they don't specify the flag but do specify a list, then the default value of the flag is interpreted as `only specified segments'.

[elBoberido]

I think I need some pseudo code for this 😅

It also feels a bit complicated with the implicit behavior when something is omitted.

[elBoberido]

Here we have again the issue with the fixed size container. We need to find a sane limit without causing the data structures exploding in their memory requirements.

[gpalmer-latai]

Yes... 🙁

In my own experimentation I actually just made it a single string. I think in practice that is usually what would be used since the main benefit of this feature is communicating over specific isolated "channels". But I was trying to leave a bit flexibility for some hypothetical use cases like having multiple publishers publishing on different segments, but the same topic.

Another idea to consider which I do not recall whether I've mentioned before, but we could instead of introducing a new string field, add some coupling to the service description. For example - we could say that the "group" field is used to identify the shared memory segment to use, and all topics belonging to the same group share the same segment.

I'm very hesitant/skeptical of this approach because I prefer not to couple features to one another. This would also more likely be a breaking API change unless we were careful to add some flags like "useServiceGroupAsSegmentMapping" or whatever.

[gpalmer-latai]

I guess you could have an enum that allows to choose from one of the following:

enum class SegmentMatchingHeuristic
{
  matchByWriteAccess = 0,
  matchByServiceGroup = 1,
  matchByServiceInstance = 2,
  matchByServiceTopic = 3
};

I think how it would probably work is that segment names would need to be configured as one of

"group"
"group-instance"
"group-instance-topic"

And this would have to be enforced when parsing the RouDi config.

Which would also mean that service descriptions are not allowed to contain the - character. Or choose the same constraints with whatever other character you want.

[gpalmer-latai]

I think, in terms of the FFI implications here. In a system where multiple producers for a single topic are allowed, you would still want there to be a way for each producer to be isolated to writing to different shared memory segments and as such the "match by service description" approach would be insufficient.

[elBoberido]

I'm wondering whether it is necessary to have the segments in the SubscriberOptions at all. An alternative would be to leave it to the runtime, e.g.

for(const auto* name: {"foo", "bar"}) {
    runtime.mapReadOnlySegment(name);
}

The same could be done for the publisher. Since the whole process has read/write access to these segments and there might be multiple subscriber/publisher which access the same segments this could be done upfront at the start of the application.

Regarding the ServiceDescription. There is the idea to combine the three string into one. See also #1052. The three separate strings date back to a time where iceoryx was meant to be the base for an AUTOSAR Adaptiv middleware and some design decisions where made with that idea in mind. With iceoryx2 there is also just one string and with a new API we could also move into this direction. The old ServiceDescription would the just be concatenated to e.g. /service/event/instance.

For iceoryx2 we currently have a separate shm segment for each publisher. Since this can become an issue with many publisher, there is the idea to group some together and create a SharedMemoryDirectory which can be used by multiple publisher. I think this goes into the same direction of what you describe as "group". I need to check with @elfenpiff how exactly this is implemented with iceoryx2.

I think we can hit two birds with this refactoring and create a API to ease the transition from iceoryx1 to iceoryx2.

[gpalmer-latai]

The other tricky thing I've encountered is the fact that a subscriber may succeed in "connecting" with a publisher, but since the publisher publishes messages to a segment the subscriber has no access to, it doesn't actually receive any messages, which can be a really surprising silent error.

We'd probably want to add some sort of accounting in RouDi to make sure this doesn't happen.

[gpalmer-latai]

For example, if we added a list of segment ids to the subscriber port data, and a single segment id to the publisher port data, we could use this additional information in isCompatiblePubSub to determine if a publishers messages can actually reach a subscriber.

And we can then treat this error the same way we treat policy mismatches.

[elBoberido]

Also just thinking out loud. The question is whether this check should be done in RouDi or the subscriber. A subscriber might receive a sample from a segment which is not mapped but the process might have read access. The subscriber process would then print an error message if it could not map the segment. This would be more visible to the user than a warning from RouDi. We already had some bug reports because of RouDi printing an error message and the user just looking at the terminal with the application.

[gpalmer-latai]

So I'll need to take another look but IIRC one issue I ran into was that the subscriber didn't have direct access to that sort of information. The Runtime is what has access to a lot of the information and the only interaction Subscribers have with it is in the beginning via getMiddlewareSubscriber() to get the subscriber port.

The subscriber could certainly tell that the segment is not mapped, but the only way to get to the information identifying the segment to determine if the subscriber in fact has read access is via the segment manager, which the runtime currently only temporarily gains access to during construction (the identifying information is collected via the IPC connection, but the runtime doesn't hold onto it).

I must confess that in following your suggestion to subclass things, I managed to subclass the runtime and, after moving the ipcChannelInterface from private to protected, have now stored the relative pointer to the segment manager as a member of my derived class.

With this, you CAN easily ask questions like "Do I have read access to this segment?" And then decide "well, lets go map it now then!". But to do this - the subscriber needs to talk to the runtime. I suppose this is alright - it can always invoke the runtime via getInstance(). If we ever get to refactoring the runtime away from being a singleton we would instead need to reference it as a member of the subscriber, and maybe do some attorney pattern stuff for the API's touching the segment manager that only the subscriber should be using in only a specific way.

As for the runtime - I'd recommend taking a look at this shared_memory_user and considering what we need to change to expose the info we need, without exposing too much. It seems to try to only temporarily access the segment manager to just get the info it needs and map everything, and then relinquish it back. But if we want to be able to map segments later - we are going to need to access the segment manager again anyway so... either the runtime should just keep a hold of the segment manager or the shared_memory_user API also needs to be extended. I guess there is no reason that class can't privately reference the segment manager past the life of the constructor 🤔

[gpalmer-latai]

Oh another thing I wanted to mention. Having access to the segment manager seems to make accessing introspection information so much easier than setting up a whole other introspection client. Haha.

[gpalmer-latai]

@elBoberido I'm back from vacation whenever you want to continue discussing this. If you'd like, we can add an agenda item to one of the next two developer meetups. Though I understand the focus may be on introducing Iceoryx2.

[elBoberido]

I'm wondering whether it is necessary to have the segments in the SubscriberOptions at all. An alternative would be to leave it to the runtime, e.g.

for(const auto* name: {"foo", "bar"}) {
    runtime.mapReadOnlySegment(name);
}

The same could be done for the publisher. Since the whole process has read/write access to these segments and there might be multiple subscriber/publisher which access the same segments this could be done upfront at the start of the application.

Regarding the ServiceDescription. There is the idea to combine the three string into one. See also #1052. The three separate strings date back to a time where iceoryx was meant to be the base for an AUTOSAR Adaptiv middleware and some design decisions where made with that idea in mind. With iceoryx2 there is also just one string and with a new API we could also move into this direction. The old ServiceDescription would the just be concatenated to e.g. /service/event/instance.

For iceoryx2 we currently have a separate shm segment for each publisher. Since this can become an issue with many publisher, there is the idea to group some together and create a SharedMemoryDirectory which can be used by multiple publisher. I think this goes into the same direction of what you describe as "group". I need to check with @elfenpiff how exactly this is implemented with iceoryx2.

I think we can hit two birds with this refactoring and create a API to ease the transition from iceoryx1 to iceoryx2.

[elBoberido]

Although the boolean would also work in this case, in general I prefer enums since they are more future proof and have more than two options. It could be something like

enum class UnmappedSegmentBehavior {
    Ignore,
    LoadOnDemand,
    TerminateProcess,
    ReleaseSampleAndReturnError
};

UnmappedSegmentBehavior::Ignore could be the default and allows to create a subscriber and afterwards calling runtime.mapReadOnlySegments. Even if nothing is called, it might be interesting when the application is not interested in the data itself but might only forward the sample.

[gpalmer-latai]

Good point. An enum would work fine. Note that this value only applies to the creation of Publishers/Clients/Servers when they request a segment. I haven't considered the possibility of detecting we have a sample from an unregistered segment and suspect it would be non-trivial to add the ability in this scenario to map the segment on demand (the smart chunk data type almost certainly doesn't have access to the segment manager).

[elBoberido]

I think we need to add some hooks to the building blocks the be able to gracefully handle this situation. We should be able to get the information from the pointer repository and then inform someone that we cannot read the payload. This should be possible without reading the chunk header. Just dropping the sample and printing a warning would also be an option. After all, not being able to read the payload is most probably just a misconfiguration.

[gpalmer-latai]

Yeah I agree. take should probably return an informative error, at the very least.

Looking into the code, it is unclear to me so far where exactly things will fail. At a high level I know that the subscriber queue contains offset pointers. I don't know exactly if/when that pointer might be dereferenced and crash because the segment is not mapped. But presumably this happens somewhere during the take() and can be guarded against, propagating an error out.

Then the question is whether we want to just return the error to the user, or whether we want to allow some sort of hook in the subscriber to ask the runtime to map that segment.

[gpalmer-latai]

So... I THINK that all the relative pointer stuff and pointer repository stuff will just return nullptrs when referencing an unmapped segment id. Reasoning about taking and releasing chunks in the subscriber in this sense is okay. It can increment and decrement the reference in the management segment no problem, without actually having access to the chunk data itself.

The first potential problem I see is at this line of code: https://github.com/eclipse-iceoryx/iceoryx/blob/master/iceoryx_posh/include/iceoryx_posh/internal/popo/building_blocks/chunk_queue_popper.inl#L57

It looks like the subscriber gets a chunk, doesn't actually check if the chunk is a nullptr, and then plows into dereferencing the nullptr with:

        auto receivedChunkHeaderVersion = chunk.getChunkHeader()->chunkHeaderVersion();

I'd have to do some testing to confirm my suspicion. This is already a bug unto its own right. Outside of this design we should have better error handling for this scenario.

[elBoberido]

I would decouple the creation of the runtime from the mapping of the segments. This makes it easier to continue execution and react on unavailable segments than just aborting the creation of the runtime.

[gpalmer-latai]

How about adding a flag indicating whether none, all read-only, all write, or all read and write segments should be mapped upon creation? And then additionally the ability to map specific ones?

We can of course also expose those as separate functions as well.

[elBoberido]

I think that might work.

[gpalmer-latai]

I've made some changes to the design to remove the subscriber segment requesting feature and change how segments are requested to be mapped in the runtime. I'll go through again on Monday and see if I can clean up the design again some more.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (41ec0e7) 80.15% compared to head (085e8b6) 80.21%.
Report is 26 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2140      +/-   ##
==========================================
+ Coverage   80.15%   80.21%   +0.05%     
==========================================
  Files         418      419       +1     
  Lines       16248    16276      +28     
  Branches     2251     2252       +1     
==========================================
+ Hits        13024    13055      +31     
+ Misses       2425     2424       -1     
+ Partials      799      797       -2     

Flag	Coverage Δ
unittests	80.00% <ø> (+0.05%)	⬆️
unittests_timing	15.34% <ø> (+0.14%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 9 files with indirect coverage changes

[elBoberido]

I've made some changes to the design to remove the subscriber segment requesting feature and change how segments are requested to be mapped in the runtime. I'll go through again on Monday and see if I can clean up the design again some more.

I don't have time to look at it today and tomorrow might also be difficult. On Wednesday it shouldn't be a problem.

[elBoberido]

This would be mostly for cases where one would like to create a publisher with a new segment and does not want to update all the applications with the corresponding subscriber. In the end, only the publisher really need to know the segment for allocation reasons but the subscriber can do the mapping fully dynamically. Of course this would be more of a desktop application feature than one for a safety application, except maybe during development time.

[elBoberido]

I'm not sure I understand this correctly. Can you rephrase please

[gpalmer-latai]

Yeah I'll see if I can rephrase but what I mean is:

When you create a subscriber, you don't bother checking whether that subscriber will have appropriate access to what publishers publish. You just hope it does, and, as discussed in other threads, either capture an error or do some dynamic mapping on the fly of that segment (still checking for the error case that the subscriber just doesn't have read access).

The check I'm talking about here is just whether the subscriber has access to any segment at all, in the case where no dynamic mapping is allowed. This would mean that any take() will always fail and it would make sense to report this error early.

For publishers on the other hand, you can more specifically check whether they have access to the segment they specify.

[elBoberido]

Maybe it's worth noting somewhere that the access rights will be set to the user group RouDi is running if no reader or writer group is set. Also, if no name is set, the name of writer group will be used.

Unfortunately, the segment feature predates the open sourcing of iceoryx and we don't yet have a design document stating all of this but only a configuration guide.

[elBoberido]

Here it becomes a bit philosophical. Does the segment info belong in the options struct or should it be a separate parameter like the ServiceDescription. It could become an optional<string> or an optional<SegmentID>.

It could even be combined with mapSegmentForWriting, assuming it would not return an error if the segment is already mapped but the segment ID, one could do this call before creating the publisher/client/server.

[gpalmer-latai]

Well, one advantage to making it a default-able optional is that we might be able to get away with not having to change any code/break any downstream code. I did have to refactor a bunch of tests that initialize the full options struct in a brace initializer list.

It might have been less annoying if the options structs were instead builders for the publishers because then the way they would be used would be only explicitly changing the fields folks don't want to remain the defaults.

I actually think this might be the way to go in general - have publishers and subscribers created by builders. This way you can capture the miriad of errors that can occur instead of the current behavior which I believe is "error handler and crash".

[elBoberido]

Yes, this would be ideal and you might already guess what's coming :) ... alreadysolvediniceoryx2™

[elBoberido]

This might become confusing when the option to map all segments with read and/or write access is added ... or is the premappedSegments option meant for this?

Here it might be more straightforward to just specify whether segments should be mapped with the read/write access wildcard and just have subsequent calls on the runtime for additional segments.

[gpalmer-latai]

Yeah I suppose I agree. Map nothing, or some preset, then just explicitly map what you want in subsequent calls. Or you can go a step further and leave out the ability to do ANY mapping in the creation of the runtime. After all... creating the runtime and mapping segments are sort of separate concerns and the error cases could benefit from being disentangled.

And it's nice avoiding allocating static vectors. Less stack memory bloat.

[elBoberido]

Indeed. There is currently too much entangled in the ctor/initRuntime call

[elBoberido]

I think we need to become more resilient regarding user errors. In this case it would nice to decouple the creation of the runtime, which should never fail if the system is in a healthy state and the request of segments which is a user configuration and a user might decide to continue with only a subset of the segments as long as the runtime creation was successful. One use case for this would be to publish to a system health monitor that something went wrong while trying to map a specific segment.

This would also unify the behavior for UnmappedSegmentBehavior where the runtime creation is also independent of the segment mapping.

[gpalmer-latai]

Yep, agreed. How do you feel about no mapping happening in create and just exposing API's for "map specific segment" or "map all read/write segments"?

[elBoberido]

Fuck ack :)

[elBoberido]

Assuming we would decouple the mapping of the segments from the creation of the producer, then all of this could be handled in the runtime and RouDi could be left out and kept more simple.

[gpalmer-latai]

Yeah... The reasons I mention RouDi doing this is because the logic is already there today: https://github.com/eclipse-iceoryx/iceoryx/blob/master/iceoryx_posh/source/roudi/process_manager.cpp#L493

The runtime sends an IPC request with publisher info and it is RouDi that checks the segment manager and figures out where that publisher should publish.

It is an interesting thought to move some of this to the Runtime... I think you'd need to expose that API through the SharedMemoryUser class. I'm not sure that the runtime could go so far as to acquire the port data itself (though this moves more towards the decentralized architecture of iox2 and is a nice thought). It could however identify the segment it wishes to publish into and make a more specific request to roudi along the lines of "hey, I want a publisher port and I'm going to publish into THIS segment. Feel free to check on your end that I'm able to, though I already know I'm able to".

[elBoberido]

Like already mentioned above. I have the feeling this becomes confusing due to the parameter influencing each other too much.

It might also break applications by just changing the segment config in a compatible way. Let's assume one want's to map all segments with write access with the premappedSegments options and one segment with read access from segmentFilter, let's call the segment alice. Now the config is changed so that the process has also write access to alice and suddenly the application does run anymore although it is not a breaking change in the configuration.

[gpalmer-latai]

Hm yeah. It could be pretty annoying even though you could guard against it.

Another option is to simply ignore, but log a warning, about the case where the segment is already mapped. Most users wouldn't care anyway.

[elBoberido]

On this we already agree :)

[elBoberido]

I think here some further discussion are needed :)

[gpalmer-latai]

Yeah. It sounds like most of the existential questions revolve around refactoring the runtime. I wonder if it would make sense to move all this optional section to a different design draft and just focus on allowing publishers to specify segments in this design. The feature doesn't actually require any changes to the runtime API - just slight tweaks to the segment matching algorithms for publishers.

[gpalmer-latai]

I might be biased due to the fact that, the segment naming is the concrete feature I need to deliver to my company to achieve our goals. But more generally I find it good software engineering practice to separate concerns as much as possible and deliver features in bite-sized pieces 😉

[elBoberido]

Sure. Go ahead. It basically all boils down to create an API which does not impose a high burden on maintenance. As you know, the community is not that big and getting a lot of bug reports because of Hyrum's Law is something we want to avoid.