Service discovery vibe redesign - #716
Conversation
b737cc3 to
d8f3ede
Compare
crimson11
left a comment
crimson11
left a comment
There was a problem hiding this comment.
Generally the "architecture" is unclear to me.
What shall
| import ScoreReq | ||
|
|
||
| ScoreReq.AssumedSystemReq ServiceDiscoveryTransportAvailable { | ||
| description = "The system shall provide process-local IPC transport suitable for communication with the service discovery daemon." |
There was a problem hiding this comment.
What does process-local IPC transport mean? Sounds like an anti-thesis? IPC is NOT process local. It is INTER-Process. I don't get it!
| } | ||
|
|
||
| ScoreReq.AssumedSystemReq ProcessIdentityProvidedByOS { | ||
| description = "The operating system shall provide process identity data (UID and PID) for each daemon session." |
There was a problem hiding this comment.
Do we need to be more explicit? IDK. I.e. explicitly state, that both parties (daemon and its client) need the process identity data of its "partner"?
|
|
||
| ScoreReq.AssumedSystemReq ProcessIdentityProvidedByOS { | ||
| description = "The operating system shall provide process identity data (UID and PID) for each daemon session." | ||
| rationale = "The daemon validates ownership and prevents spoofed unregister operations based on OS-provided identity." |
There was a problem hiding this comment.
Also the client needs to be sure to be talking to the "right" daemon! I guess from safety perspective this is essential. Maybe we could "relax" this, if we make sure, that the daemon-provided message-passing endpoint can ONLY be occupied/used by the correct/right daemon instance. In this case the client doesn't need "process identity" from the daemon. Your thoughts?
Bottomline: For the client, we simply have the requirement, that there is a mechanism to assure, that he connects to/talks to the correct daemon instance. Whether this is achieved by:
- clients connecting to an "endpoint", which can't be spoofed/taken over by a malicious daemon instance.
- clients checking the process identity of the daemon/communication partner after message-passing connect
we don't care.
| } | ||
|
|
||
| ScoreReq.AoU SingleDaemonAuthority { | ||
| description = "Exactly one service discovery daemon instance shall act as registry authority at a time." |
There was a problem hiding this comment.
In what context? I.e. if we have an interVM setup our current design says: Each VM has its own "score::mw::com communication domain" and each such domain has its own service-discovery -> daemon instance.
Thus, makes it sense to phrase it:
Exactly one service discovery daemon instance shall act as registry authority at a time in a given score::mw::com communication domain
or are we just opening up Pandoras box ... needing to come up with definition, what such a domain exactly is?
| } | ||
|
|
||
| ScoreReq.FeatReq PartitionedVisibilityForIntegrityLevels { | ||
| description = "The service discovery component shall enforce ASIL-B and ASIL-QM partitioned visibility and prevent lower-integrity claims from escalating service quality." |
There was a problem hiding this comment.
What does this exactly mean? I.e.: It is a valid use case, that an ASIL-B client does see QM-quality offerings.
It is also a good idea, that a QM client doesn't see an offering of a service-instance in ASIL-B quality.
(This req. we can then only support, when the SD daemon has a complete trustable list of uid->ASIL-level assignments? This we need to generate from a static deployment info ...)
Sidenote: A LoLa service-instance provider of an instance in ASIL-B quality will allways also offer the instance in QM initially, but he may withdraw the QM offering selectively, if he thinks, that QM clients did mess up.
| IntegrityLevel provider_integrity{IntegrityLevel::kAsilQm}; | ||
| std::uint32_t provider_uid{0U}; | ||
| std::uint32_t provider_pid{0U}; | ||
| std::uint64_t provider_session_id{0U}; |
There was a problem hiding this comment.
Seems redundant? An instance of Registration is send across a message-passing session? So the "session" if required will be implicitly deduced by the receiver/daemon?
| std::uint64_t provider_session_id{0U}; | ||
| }; | ||
|
|
||
| constexpr std::size_t kMaxRegistrationsPerService{32U}; |
There was a problem hiding this comment.
Where does this come from?
Is kMaxRegistrationsPerService basically a cap how many different instances can be registered/offered for a given service-instance-type?
I this a theoretical maximum (then 32 is too small) ... or is it some value, which is deduced from a static configuration model? I.e. for a given setup designed by the integrators? Then it shouldn't be a constant, but a config value read by the daemon from its startup config.
| #include <iostream> | ||
| #include <thread> | ||
|
|
||
| int main() |
There was a problem hiding this comment.
Unexpected, that this daemonm-application is in the tests-folder? I would have expected it to be part of the mw::com::service_discovery implementationn folder.
| return EXIT_FAILURE; | ||
| } | ||
|
|
||
| while (!stop_source.get_token().stop_requested()) |
There was a problem hiding this comment.
Really - a busy loop waking up every 50msec to check a variable?
Should be replaced by a cond_var.wait() ... and the signal-handler shall notify the cond_var.
Alternative: In case we introduce a worker/background-thread for the daemon, this thread could then also "supervise" the stop_source.
But currently there is obviously no need for such a "worker-thread"? I.e. all activities the daemon does are triggered by a message-reception and are then synchronously done in the context of the message-passing reception thread. This is good. Don't want to arteficially introduce a worker/background thread, when there is no need!
| namespace score::mw::com::impl | ||
| { | ||
|
|
||
| class ServiceDiscoveryCompat final : public IServiceDiscovery |
There was a problem hiding this comment.
What is Compat ?
... and I feel the whole class-hierarchy/interface tree is "broken"?
This class should rather implement IServiceDiscoveryClient!
Testing the ability to GitHub Copilot to be integrated into our infrastructure by vibe coding a replacement of filesystem-based service discovery.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If this PR is still relevant, please leave a comment or push new changes to keep it open. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
ClangTidy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
3 participants
Testing the ability to GitHub Copilot to be integrated into our infrastructure by vibe coding a replacement of filesystem-based service discovery.