Launch Manager loads the new configuration file#248
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
|
Please lnk corresponding issue/bug. |
f13cffe to
eb807a1
Compare
|
The created documentation from the pull request is available at: docu-html |
| << std::string_view{f_exception_r.what()}; | ||
| const auto* comp = supervised_components_[idx]; | ||
| const std::string checkpointCfgName = comp->name + "_checkpoint"; | ||
| const uint32_t checkpointId = 1U; |
There was a problem hiding this comment.
Not sure if this is worth it, but the checkpointId has to match this
I wonder if we should make this a static constant in score/launch_manager/src/daemon/src/alive_monitor/details/factory/StaticConfig.hpp which is then used in both places.
The background is, when this code was still supporting Deadline and Logical supervisions, then different kinds of checkpoints were required.
But now only Alive supervision support is left, which only needs sending "Alive notificiations" - for which he just use checkpointId 1.
There was a problem hiding this comment.
We would need to reorganize the code a bit
Looking at:
score/launch_manager/src/daemon/src/alive_monitor/details/factory/StaticConfig.hpp
score/launch_manager/src/alive/src/alive.cpp
we might want to move StaticConfig.hpp into score/launch_manager/src/common or a similar location. We might want to create a ticket out if this comment to work in it later?
There was a problem hiding this comment.
Ticket for removal of this id already exists #251
| void logConfiguration() noexcept(true); | ||
|
|
||
| /// @brief Configured watchdog devices | ||
| /// By default, no watchdog device is configured |
There was a problem hiding this comment.
Maybe we should bring back some of this doxygen, thats seems rather useful
| // strdup() returns nullptr on OOM. On this embedded target, OOM during daemon | ||
| // startup is unrecoverable — the OS will terminate the process. | ||
| size_t arg_index = 0U; | ||
| startup.argv_[arg_index++] = strdup(startup.executable_path_.c_str()); |
There was a problem hiding this comment.
We could use comp.deployment_config.executable_path.c_str(), but this would create a mixed ownership between the configuration object in main and the adapter. The Adapter copies the configuration so that it owns it. Yes this costs a bit of resources but is the simplest and clearest way currently to bridge the old to new configuration.
| const auto& wd = *wd_opt; | ||
| watchdog::DeviceConfig wdConfig{}; | ||
| wdConfig.fileName = wd.device_file_path; | ||
| assert(wd.max_timeout_ms <= std::numeric_limits<std::uint16_t>::max()); |
There was a problem hiding this comment.
Should we make this a validation in score/launch_manager/src/daemon/src/configuration/details/flatbuffer_type_converters.cpp ?
| } | ||
| dep.target_process_id_ = IdentifierHash{dep_name}; | ||
| os_process.dependencies_.push_back(dep); | ||
| } |
| for (const auto& comp_name : fallback.depends_on) { | ||
| auto it = component_to_process_index.find(comp_name); | ||
| if (it != component_to_process_index.end()) { | ||
| fallback_state.process_indexes_.push_back(it->second); |
There was a problem hiding this comment.
I think here we also need to resolve the dependencies recursively.
E.g. fallback_run_target depends_on [CompA]
and CompA depends_on [CompB] then
fallback_run_target = [CompA, CompB]
I think it would be good to extend the UT with this case
| true) | ||
| { | ||
| return score::lcm::IdentifierHash{f_processPath_r}.data(); | ||
| std::optional<common::ProcessId> FlatCfgFactory::getProcessId( |
There was a problem hiding this comment.
std::optional no longer required, hashing cannot fail
| pg.off_state_ = IdentifierHash{"MainPG/Off"}; | ||
|
|
||
| IdentifierHash recovery_state{"Recovery"}; | ||
| for (const auto& rt : config.runTargets()) { |
There was a problem hiding this comment.
Could be changed to just
IdentifierHash recovery_state = IdentifierHash{std::string("MainPG/") + "fallback_run_target"};
without the for loop
| component_by_name[comp.name] = ∁ | ||
| } | ||
|
|
||
| std::map<std::string, uint32_t> component_to_process_index; |
There was a problem hiding this comment.
do we need a separate map just for the index or can we get the index from component_by_name?
| size_t arg_index = 0U; | ||
| startup.argv_[arg_index++] = strdup(startup.executable_path_.c_str()); | ||
| size_t max_args = std::min(props.process_arguments.size(), | ||
| static_cast<size_t>(score::lcm::internal::kMaxArg)); |
There was a problem hiding this comment.
should we assert that we have enough space for all configured process_arguments and env variables?
kMaxArg with 20 is rather small, can we bump this to 100?
| bool is_sup = (props.application_profile.application_type == ApplicationType::ReportingAndSupervised || | ||
| props.application_profile.application_type == ApplicationType::StateManager); | ||
| if (is_sup && env_index < static_cast<size_t>(score::lcm::internal::kMaxEnv)) { | ||
| std::string iface_path = "LCM_ALIVE_INTERFACE_PATH=" + score::lcm::internal::aliveInterfacePath(comp.name); |
There was a problem hiding this comment.
we could also add a constant for the env name
| ? score::lcm::ProcessState::kRunning | ||
| : score::lcm::ProcessState::kTerminated; | ||
| } | ||
| } |
There was a problem hiding this comment.
here is a possible error case. In case the configured dependency does not actually exist
| pgm.number_of_restart_attempts = deploy.ready_recovery_action->number_of_attempts; | ||
| } | ||
|
|
||
| for (const auto& dep_name : props.depends_on) { |
There was a problem hiding this comment.
depends_on might also contain a run_target and not only a component.
Special Case: This may be also the fallback_run_target
| } | ||
| } | ||
| auto rt_it = run_target_by_name.find(dep_name); | ||
| if (rt_it != run_target_by_name.end()) { |
There was a problem hiding this comment.
I think depends_on might also refer to fallback_run_target which is currently not in the run_target_by_name map
| run_target_by_name[rt.name] = &rt; | ||
| } | ||
|
|
||
| std::map<std::string, const ComponentConfig*> component_by_name; |
There was a problem hiding this comment.
component_by_name exists twice in this class
| std::set<std::string>& visited) { | ||
| if (!visited.insert(dep_name).second) return; | ||
| auto comp_it = component_to_process_index.find(dep_name); | ||
| if (comp_it != component_to_process_index.end()) { |
There was a problem hiding this comment.
There might be some possible error cases when component/runTarget names are not found because they have not been configured
There was a problem hiding this comment.
Maybe we keep the current validation in the python script and just put asserts in the code for these error cases
| component_by_name[comp.name] = ∁ | ||
| } | ||
|
|
||
| std::function<void(const std::string&, std::vector<uint32_t>&, std::set<std::string>&)> resolve_depends; |
There was a problem hiding this comment.
Maybe it could make sense to have a dedicated Visitor class for this
| } | ||
|
|
||
| void ConfigurationAdapter::resolveDependencyIndexes(std::vector<OsProcess>& processes) { | ||
| for (auto& proc : processes) { |
| component_by_name[comp.name] = ∁ | ||
| } | ||
|
|
||
| std::map<std::string, uint32_t> component_to_process_index; |
There was a problem hiding this comment.
should we build these maps once during initialize and store them as private member?
| if (pg) { | ||
| if (index < pg->processes_.size()) { | ||
| return pg; | ||
| } |
There was a problem hiding this comment.
does not make sense to validate the index here
|
|
||
| static const uint32_t kDefaultProcessExecutionError; | ||
| static uint32_t kDefaultProcessorAffinityMask(); | ||
| static const int32_t kDefaultSchedulingPolicy; |
There was a problem hiding this comment.
These defaults seem obsolete now

Closes: #209