Architecture overview for Rialto server manager - #566
Open
balasaraswathy-n wants to merge 6 commits into
Open
Conversation
|
Pull request must be merged with a description containing the required fields, Summary: If there is no jira releated to this change, please put 'Jira: NO-JIRA'. Description can be changed by editing the top comment on your pull request and making a new commit. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds architecture/operations documentation intended to serve as implementation-verified briefs for Rialto overall, ServerManager, and logging, plus an internal prompt for generating similar briefs.
Changes:
- Added ServerManager SME operational notes (deployment, monitoring, common issues, incident learnings).
- Added a detailed ServerManager architecture brief including diagrams, state/healthcheck matrices, and simulator interface.
- Added top-level Rialto architecture brief and logging module architecture brief, plus a reusable “catalyst” prompt for generating briefs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
serverManager/SME-notes.md |
Operational runbook-style notes for deploying/monitoring/troubleshooting ServerManager. |
serverManager/architecture-brief.md |
Detailed ServerManager architecture write-up with diagrams and state/healthcheck behavior. |
logging/architecture.md |
Logging subsystem architecture overview (APIs, sinks, configuration, concurrency). |
docs/architecture-brief.md |
Top-level Rialto architecture brief linking to module briefs and documenting APIs/diagrams. |
.github/prompts/catalyst-architecture-brief.prompt.md |
Prompt template/checklist for generating validated architecture briefs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SmLib -->|session orchestration| SmCommon | ||
| SmCommon -->|setConfiguration setState ping| SmIpc | ||
| SmIpc -->|protobuf IPC| IpcClient | ||
| SmSim -->|HTTP localhost 9008| SmLib |
Comment on lines
+256
to
+257
| ## Key Decisions | ||
| Architectural decisions and rationale for `RialtoServerManager`: |
|
Coverage statistics of your commit: |
| - Primary Users and Use Cases | ||
| - Non Functional Requirements (availability, performance, security, scalability) | ||
| - Integration Points with external systems | ||
| 4. **C4 System Context Diagram** - Visually appealing system context using Mermaid graph TD format with color-coded categories, emojis, and descriptive connection labels |
Contributor
There was a problem hiding this comment.
The generated diagram contains errors.
|
|
||
| Detailed initialization behavior from current code paths: | ||
| - When config-file support is enabled, config is read in precedence order: base (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`), then overrides (`RIALTO_CONFIG_OVERRIDES_PATH`). | ||
| - `envVariables` from a more important file replace lower-priority values; `extraEnvVariables` are merged last and can overwrite keys. |
Contributor
There was a problem hiding this comment.
I think, that it's important to add one thing here.
The values of both envVariables and/or extraEnvVariables from the config files from /etc are wiped, if envVariables and/or extraEnvVariables exist in the overrides file.
| - For active sessions, shared memory setup is part of readiness and must be mapped by the app client. | ||
|
|
||
| Connection-info contract detail: | ||
| - After transition out of not-running, app manager is expected to query `getAppConnectionInfo()` promptly so the socket name can be passed into container/app launch context. |
Contributor
There was a problem hiding this comment.
out of Uninitialized. Preloaded servers do not have socket name assigned yet.
|
|
||
| ### Matrix Notes | ||
| - Observer contract: each completed transition edge should result in manager-to-host `stateChanged(appId, state)` notification. | ||
| - Connection info usage: after leaving not-running, app manager should query `getAppConnectionInfo(appId)` promptly unless socket name was pre-supplied. |
Contributor
There was a problem hiding this comment.
After leaving Uninitialized :)
| - Healthcheck and startup timers are treated as first-class failure detectors with policy-driven recovery. | ||
| - Testing approaches: | ||
| - Unit tests in `tests/unittests/` validate lifecycle logic, configuration behavior, and edge-case handling. | ||
| - Component/integration tests in `tests/componenttests/` validate multi-process and IPC behavior. |
Contributor
There was a problem hiding this comment.
Server manager doesn't have CTs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
docs/architecture-brief.md:187
- The edge label says the simulator talks over "HTTP localhost 9008", but the simulator binds to
0.0.0.0:9008(seeserverManager/serverManagerSim/TestService.cpp:28). For accuracy, consider wording this as "HTTP port 9008" (or explicitly0.0.0.0:9008).
SmSim -->|HTTP localhost 9008| SmLib
serverManager/architecture-brief.md:259
- This section has two adjacent headings that differ only by wording ("Key Design Decisions" then "Key Decisions"), which reads like duplication and makes navigation/linking ambiguous. Consider having a single
## Key Decisionssection, and make the explanatory part a subsection (e.g.,### Rationale).
## Key Design Decisions
- Single event-thread serialization in `SessionServerAppManager` prevents state races around process and IPC lifecycle.
- Per-server IPC clients are guarded by a mutexed controller map.
- Preloaded server pool reduces startup latency for first-use apps.
- Recovery is policy-based (`numOfFailedPingsBeforeRecovery`) rather than immediate hard reset on first signal.
- Socket path strategy supports autogenerated, absolute-path, and name-only input forms.
## Key Decisions
Architectural decisions and rationale for `RialtoServerManager`:
Comment on lines
+104
to
+115
| subgraph RialtoSystem ["🎬 Rialto Media Platform"] | ||
| ClientApi["📦 RialtoClient API\nmedia/public include"] | ||
| SessionServer["🖥️ RialtoServer Runtime\nmedia server modules"] | ||
| ServerManager["🧭 RialtoServerManager\nsession orchestration"] | ||
| end | ||
|
|
||
| subgraph ExternalSystems ["🌐 External and Platform Systems"] | ||
| GStreamer["🎞️ GStreamer Runtime"] | ||
| OpenCdm["🔐 OpenCDM"] | ||
| Wpe["🧩 WPEFramework Core and COM"] | ||
| LocalOs["🐧 Linux Kernel Services\nunix sockets and shared memory"] | ||
| end |
Comment on lines
+140
to
+160
| graph TD | ||
| App["📱 Application Process\nuses libRialtoClient"] | ||
|
|
||
| subgraph ClientContainer ["Client Container Unit C++17"] | ||
| ClientLib["RialtoClient shared library\nmedia/client/main/source/MediaPipeline.cpp\nmedia/client/main/source/MediaKeys.cpp"] | ||
| ClientIpc["Client IPC adapters\nmedia/client/ipc/source/MediaPipelineIpc.cpp\nmedia/client/ipc/source/MediaKeysIpc.cpp"] | ||
| end | ||
|
|
||
| subgraph IpcTransport ["IPC Transport Libraries C++17"] | ||
| IpcClient["RialtoIpcClient\nipc/client/source/IpcChannelImpl.cpp"] | ||
| IpcCommon["RialtoIpcCommon\nipc/common/source/NamedSocket.cpp"] | ||
| IpcServer["RialtoIpcServer\nipc/server/source/IpcServerImpl.cpp"] | ||
| end | ||
|
|
||
| subgraph SessionServerContainer ["Session Server Unit C++17"] | ||
| ServerExe["RialtoServer executable\nmedia/server/service/source/main.cpp"] | ||
| ServerIpc["Server IPC modules\nmedia/server/ipc/source/MediaPipelineModuleService.cpp"] | ||
| ServerMain["Server main domain\nmedia/server/main/source/MediaPipelineServerInternal.cpp"] | ||
| GstPlayer["GStreamer player tasks\nmedia/server/gstplayer/source/GstGenericPlayer.cpp"] | ||
| end | ||
|
|
Comment on lines
+161
to
+174
| subgraph ServerManagerContainer ["Server Manager Unit C++17"] | ||
| SmLib["RialtoServerManager shared library\nserverManager/service/source/ServerManagerService.cpp"] | ||
| SmCommon["Session app manager\nserverManager/common/source/SessionServerAppManager.cpp"] | ||
| SmIpc["Server manager IPC client\nserverManager/ipc/source/Controller.cpp"] | ||
| SmSim["RialtoServerManagerSim HTTP utility\nserverManager/serverManagerSim/TestService.cpp"] | ||
| end | ||
|
|
||
| subgraph PlatformDeps ["Platform Dependencies"] | ||
| Proto["protobuf runtime and generated stubs\nproto/*.proto"] | ||
| Gst["gstreamer 1.0 modules"] | ||
| Ocdm["ocdm"] | ||
| WpeCore["WPEFrameworkCore and COM"] | ||
| Mongoose["mongoose HTTP library"] | ||
| end |
| Detailed initialization behavior from current code paths: | ||
| - When config-file support is enabled, config is read in precedence order: base (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`), then overrides (`RIALTO_CONFIG_OVERRIDES_PATH`). | ||
| - `envVariables` from a more important file replace lower-priority values; `extraEnvVariables` are merged last and can overwrite keys. | ||
| - The values of both envVariables and/or extraEnvVariables from the config files from (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`) are wiped, if envVariables and/or extraEnvVariables exist in the overrides file (`RIALTO_CONFIG_OVERRIDES_PATH`) |
Comment on lines
+53
to
+58
| CallSite[Caller macro or API] --> Filter{Component valid?\nLevel enabled?} | ||
| Filter -- no --> Drop[Drop message] | ||
| Filter -- yes --> Format[Format message\ntrim newline\nappend strerror text if sys variant] | ||
| Format --> SinkSelect{Custom handler set?} | ||
| SinkSelect -- yes --> Handler[Invoke component handler] | ||
| SinkSelect -- no --> FileCheck{File logging enabled\nand fd open?} |
Comment on lines
+241
to
+248
| #### System Context Diagram (Enhanced Mermaid Graph TD) | ||
| - **Use Mermaid graph TD format** instead of C4Context for better visual appeal | ||
| - **Color-code by component type**: users (orange), core system (blue), external services (green) | ||
| - **Add relevant emojis/icons** to make components instantly recognizable | ||
| - **Group related services** in subgraphs with clear boundaries | ||
| - **Use descriptive connection labels** with real examples and specific protocols | ||
| - **Show actual user interactions** with example queries/actions | ||
|
|
Comment on lines
+39
to
+57
| graph TD | ||
| Integrator["Platform Integrator / Host Process"] | ||
| SessionServer["RialtoSessionServer Processes"] | ||
| Kernel["Linux OS: process, sockets, permissions"] | ||
| Proto["Protobuf Contracts"] | ||
|
|
||
| subgraph ServerManagerSystem ["ServerManager"] | ||
| PublicApi["Public C++ API\nIServerManagerService"] | ||
| ServiceCore["Service + App Manager\nRialtoServerManager"] | ||
| Sim["HTTP Simulator\nRialtoServerManagerSim"] | ||
| end | ||
|
|
||
| Integrator -->|C++ calls| PublicApi | ||
| PublicApi --> ServiceCore | ||
| Sim -->|calls same API| PublicApi | ||
| ServiceCore -->|ServerManagerModule RPC| SessionServer | ||
| ServiceCore -->|spawn/kill, socketpair, timer| Kernel | ||
| ServiceCore --> Proto | ||
| SessionServer --> Proto |
Comment on lines
+25
to
+28
| - Verify basic app lifecycle path via integration host or simulator: | ||
| - initiate app to `Inactive` | ||
| - transition to `Active` | ||
| - transition to `NotRunning` |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
serverManager/architecture-brief.md:113
- This bullet is a bit hard to parse and is missing a terminating period. Consider rephrasing to make the override semantics explicit (discard vs merge).
- When config-file support is enabled, config is read in precedence order: base (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`), then overrides (`RIALTO_CONFIG_OVERRIDES_PATH`).
- `envVariables` from a more important file replace lower-priority values; `extraEnvVariables` are merged last and can overwrite keys.
- The values of both envVariables and/or extraEnvVariables from the config files from (`RIALTO_CONFIG_PATH`), SoC (`RIALTO_CONFIG_SOC_PATH`) are wiped, if envVariables and/or extraEnvVariables exist in the overrides file (`RIALTO_CONFIG_OVERRIDES_PATH`)
- Effective config includes: number of preloaded servers, session server binary path, startup timeout, and healthcheck interval.
serverManager/architecture-brief.md:255
- There are two adjacent top-level sections named almost the same ("Key Design Decisions" and "Key Decisions"), which makes the outline harder to navigate. Consider collapsing this into a single heading and making the second one a subheading (or vice versa).
## Key Design Decisions
- Single event-thread serialization in `SessionServerAppManager` prevents state races around process and IPC lifecycle.
- Per-server IPC clients are guarded by a mutexed controller map.
- Preloaded server pool reduces startup latency for first-use apps.
- Recovery is policy-based (`numOfFailedPingsBeforeRecovery`) rather than immediate hard reset on first signal.
Comment on lines
+7
to
+9
| `serverManager` is the session lifecycle orchestration component of Rialto. It is responsible for: | ||
| - Spawning and configuring `RialtoSessionServer` processes. | ||
| - Tracking and driving per-application session state transitions. |
Comment on lines
+216
to
+219
| - After transition out of not-running, app manager is expected to query `getAppConnectionInfo()` promptly so the socket name can be passed into container/app launch context. | ||
| - If app manager provided socket name up front, returned connection info acts as confirmation rather than discovery. | ||
| - out of Uninitialized. Preloaded servers do not have socket name assigned yet. | ||
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
serverManager/architecture-brief.md:258
- This section has two adjacent top-level headings (“Key Design Decisions” and “Key Decisions”) that cover the same topic, which makes the outline harder to follow. Consider demoting the second heading to a subsection (or merging the content) to avoid duplicate TOC entries.
## Key Decisions
serverManager/architecture-brief.md:218
- This bullet is a sentence fragment (“out of Uninitialized…”) and reads like an editing leftover. Rewording it as a complete, explicit statement improves readability.
- out of Uninitialized. Preloaded servers do not have socket name assigned yet.
.github/prompts/catalyst-architecture-brief.prompt.md:487
- The prompt is internally inconsistent: earlier it mandates a “System Context Diagram (Enhanced Mermaid Graph TD)”, but here it says “Use C4 syntax for system context diagrams”. Aligning these instructions reduces confusion and prevents mixed syntax output.
**Use C4 syntax for system context diagrams:**
- Standardized notation for high-level business view
- Clear stakeholder communication
- Good for external system relationships
| - Healthcheck uses one periodic timer with outstanding-ack tracking, not one timer per server. | ||
| - Late ack handling is strict by ping id; stale ack does not clear current outstanding ping. | ||
| - Recovery may use hard kill (`SIGKILL`) for session-server restart paths; this is robust but not graceful. | ||
| - `getAppConnectionInfo` returns empty string if app is not known; this is a normal failure form, not an exception. |
| @@ -0,0 +1,605 @@ | |||
| # Rialto Architecture Brief | |||
|
|
|||
| Status: Validation: Complete ✅ | |||
|
|
||
| ## ⚠️ CRITICAL: READ-ONLY ANALYSIS AND DOCUMENTATION | ||
|
|
||
| **Your ONLY output is a comprehensive architecture brief document.** |
| 6. Skip any of the 9 steps or 3 validation rounds | ||
| 7. Make ANY changes to the application itself | ||
|
|
||
| **All output must be in a single architecture brief document only.** |
|
Coverage statistics of your commit: |
2 similar comments
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
serverManager/architecture-brief.md:113
- This bullet is inaccurate vs
serverManager/service/source/ConfigHelper.cpp:extraEnvVariablesare only cleared when a more important config file provides non-emptyenvVariables(which clearsm_extraEnvVarsFromConfigFile). An overrides file with an emptyextraEnvVariableslist does not clear previously loaded extra env vars by itself.
- The more important file (`RIALTO_CONFIG_OVERRIDES_PATH`) must clearly specify the extraEnvVariables , if they are empty any previously loaded `extraEnvVariables` will be cleared
serverManager/architecture-brief.md:219
- This line is grammatically unclear and reads like a sentence fragment; it’s also inconsistent with the surrounding capitalization of
UNINITIALIZED.
- out of Uninitialized. Preloaded servers do not have socket name assigned yet.
serverManager/architecture-brief.md:8
- The doc uses both
RialtoSessionServer(here) andRialtoServer(the actual executable target inmedia/server/CMakeLists.txt) to refer to the same process. Consider clarifying the naming to avoid confusion for operators and integrators.
- Spawning and configuring `RialtoSessionServer` processes.
serverManager/architecture-brief.md:260
- There are two adjacent sections titled "Key Design Decisions" and "Key Decisions", which makes navigation ambiguous (especially since both contain decision-oriented content). Renaming one of the headers would make the structure clearer.
## Key Design Decisions
- Single event-thread serialization in `SessionServerAppManager` prevents state races around process and IPC lifecycle.
- Per-server IPC clients are guarded by a mutexed controller map.
- Preloaded server pool reduces startup latency for first-use apps.
- Recovery is policy-based (`numOfFailedPingsBeforeRecovery`) rather than immediate hard reset on first signal.
- Socket path strategy supports autogenerated, absolute-path, and name-only input forms.
## Key Decisions
Architectural decisions and rationale for `RialtoServerManager`:
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.