Configuration description for Launch Manager#54
Configuration description for Launch Manager#54SimonKozik wants to merge 6 commits intoeclipse-score:mainfrom
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
pawelrutkaq
left a comment
There was a problem hiding this comment.
General questions: Since we discuss JSON, we will need again JSON to Flatbuffer tooling? Will this tool be generic or custom for LaunchDaemon ?
So as i remeber, you can for free convert Json to flatbuffers but this requires that it is compatible with fbs schema. Will it be ?
There was a problem hiding this comment.
general: please use rst in top ./doc folder
There was a problem hiding this comment.
Do you want an extra rst file in the doc folder, that will includes this md file?
Or to translate this md file into a rst file?
There was a problem hiding this comment.
Keep doc in doc folder so doc-as-code works, so this file shall be rst too.
| "gid" : 1000, | ||
| "supplementary_group_ids": [500, 600, 700] | ||
| } | ||
| } |
There was a problem hiding this comment.
if we combine deployment configuration with component description in single JSON file, it's not reusable so that separation is rather only for json handling (this contradicts the above explanations).
There was a problem hiding this comment.
True. I think the rough workflow that was discussed early January was that the app developer will somehow provide the component-related config + maybe partially the deployment config.
However, since this workflow is currently rather unclear and splitup into multiple files also raised more open points. Therefore, in the last discussion we concluded that as a first step everything shall to into a single file.
There was a problem hiding this comment.
But then maybe even in a single json the object should be separated so later splitting it to multiple files does not change schema really.
There was a problem hiding this comment.
@pawelrutkaq we were talking internally about this separation as well, but as Nicolas said, a lot of things is unclear.
Just for clarification, are you talking about something like this?
{
"component_configs": {
"test_app1": {
"is_native_application": false,
"is_supervised": true,
"is_self_terminating": false,
"is_state_manager": false
}
},
"deployment_configs": {
"test_app1": {
"startup_timeout": 0.5,
"shutdown_timeout": 0.5,
"uid" : 1000,
"gid" : 1000,
"supplementary_group_ids": [500, 600, 700],
"scheduling_policy": "SCHED_OTHER",
"scheduling_priority": "0"
}
}
}| }, | ||
| "Full": { | ||
| "description": "Everything running", | ||
| "includes": ["Minimal", "test_app1"], |
There was a problem hiding this comment.
here you lost info who the Minimal or test_app1 is. How you will handle different json object under it in code ?
There was a problem hiding this comment.
Do you mean it should be like this?
{
"includes": [
{
"run_target": "Minimal"
},
{
"component": "test_app1"
}
]
}
There was a problem hiding this comment.
Maybe the run_target or component shall have requires "type" keyword, it would be easier to use here as "includes": ["Minimal", "test_app1"],. Then logic can simply relay on querying this type. (still 2 places to lookup needed but that maybe not a problem)
There was a problem hiding this comment.
If I understand you correctly you are proposing this:
{
"includes": [
{
"type": "run_target",
"name": "Minimal"
},
{
"type": "component",
"name": "test_app1"
}
]
}As far as I can tell both versions contain the same information, but version in this comment is more verbose (i.e. more unnecessary typing).
If I misunderstood, could you provide some json example of what you are suggesting pls?
There was a problem hiding this comment.
I was thinking about something else but maybe the best would be simply:
{
"includes": {
"components": [
"Minimal"
],
"run_targets": [
"test_app1"
]
}
}
This way is super clear what is what and parsing is also easy and unambiguous.
FYI: I meant that we keep includes flat "includes":["run", "app"] but this is not really possible, would require that ie no components have the same name as run target etc.
| } | ||
| }, | ||
| "initial_run_target": "Minimal", | ||
| "health_monitoring" : { |
There was a problem hiding this comment.
there is no health_monitoring at LaunchDaemon so shall be removed. is you meant here app supervision, tthe cycle comes via IPC.
There was a problem hiding this comment.
This setting is about the evaluation of alive notifications in the launch manager daemon.
Currently, the evaluation is performed in a cycle. E.g. every 50ms launch_manager wakes up and is evaluating the checkpoints that have been received in the last 50ms. It would make sense to have this configurable. Depending on the system and the required detection time of failures, this number might be small (maybe 10ms) or big (maybe 100ms).
Otherwise, if the launch_manager would wake up on every received alive notification and do the evaluation immediately - the performance would be too bad if many applications report alive notifications
There was a problem hiding this comment.
Yes I get it. But first of all, change the name - highly confusing with other components. Second of all, you mean you use single thread cycled by this value to check alive notifications presence for all processes ?
There was a problem hiding this comment.
Current implementation does use a single thread. I guess that could be extended in the future if this is required.
Do you have a suggestion for another name? Both the library and the daemon are doing some kind of health monitoring. But yeah, having the same name is confusing at the moment.
There was a problem hiding this comment.
since we said once the api betwen daemon and users app is SupervisorAPI, maybe it shall be supervisor_cycle_time (app_supervision_cycle_time) or something in this manner ?
You will need compiler, but this is part of FlatBuffers (https://flatbuffers.dev/flatc/). |
I am asking after transition, its still unclear since as i said to use flatcc, this config what we do here must be |
Current plan is that after transition, user visible json format will be fbs compatible. Or in other words, during transition we plan to change fbs format, so it becomes compatible with json format we create here. Finger cross this explain things. |
df9301a to
4c4610a
Compare
| "is_supervised": false, | ||
| "is_self_terminating": true | ||
| }, | ||
| "deployment_config": { |
There was a problem hiding this comment.
In my opinion it would make sense if - as intermediate solution - we also add here the expectation of alive notifications for the launch manager.
Until we have the new runtime registration mechanism implemented both in the launch_manager and in the health_monitoring_library we need this information from the configuration to keep the current functionality working. Without this information, launch_manager will not know the expected frequency of alive notifications.
Once the runtime registration mechanism is implemented, we can remove it from the configuration again.
Proposal for Launch Manager configuration, that is based on discussion we had so far.
Related to #38