Removing Config Adapter by MaciejKaszynski · Pull Request #455 · eclipse-score/lifecycle

MaciejKaszynski · 2026-08-12T08:17:24Z

Fixes #420

Currently we have a lot of code to bridge the old config and new. This PR removes the bridge and changes. Process Group Manager, Graph and Alive Monitor to use the new config.

This also partially addresses some multi-og code from #413. However our IPC still has a field for the pg_name and there is still a bunch of references to it in comments and method names.

This also creates a temporary CreateDependencyGraph function that takes the config and creates a DependencyGraph from it. This is intentionally left as not polished and un covered with UTs as it shall be more or less removed in #463.

Another point to address is that the AliveMonitor copies its config as the shape of the config doesn't fit the current implementation. This will be addressed in #477.

github-actions · 2026-08-13T07:14:54Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 7fb2072a-30a1-4de3-bc91-c686983947c3
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (33 packages loaded, 10 targets configured)

Analyzing: target //:license-check (88 packages loaded, 10 targets configured)

Analyzing: target //:license-check (146 packages loaded, 2945 targets configured)

Analyzing: target //:license-check (160 packages loaded, 5907 targets configured)

Analyzing: target //:license-check (167 packages loaded, 9402 targets configured)

Analyzing: target //:license-check (172 packages loaded, 9451 targets configured)

Analyzing: target //:license-check (172 packages loaded, 9451 targets configured)

Analyzing: target //:license-check (174 packages loaded, 9575 targets configured)

INFO: Analyzed target //:license-check (177 packages loaded, 11589 targets configured).
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 23.740s, Critical Path: 0.25s
INFO: 16 processes: 4 disk cache hit, 12 internal.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

github-actions · 2026-08-14T14:32:22Z

The created documentation from the pull request is available at: docu-html

danth

The graph creation function is a bit long, but since it will be improved anyway after #463, I don't think it is worth making any major changes in this pull request.

danth · 2026-08-14T15:21:22Z


-    LM_LOG_DEBUG() << "Process group index" << index << "(with name" << pg_name << ") has" << num_processes
-                   << "processes";
+    DependencyGraph<Graph::Component> graph(components.size() + run_targets.size() + 1);


Should we add a comment explaining why the + 1 is necessary?

danth · 2026-08-14T15:30:23Z

+        LM_LOG_DEBUG() << "HERER++++++++";
        // Now process the request
        switch (scc->request().request_or_response_)
        {
            case ControlClientCode::kSetStateRequest:
+                LM_LOG_DEBUG() << "HERER++++++++";


Remove print debugging.

danth · 2026-08-14T15:31:52Z

+            // TODO: Determine recovery state from configuration
+            recovery_state.pg_state_name_ = IdentifierHash("fallback");  // TODO: Get from configuration


Does this need to be done?

WilliamRoebuck

Looks good, I have mostly small comments, one or two suggestions for code changes

WilliamRoebuck · 2026-08-17T11:00:07Z

    EXPECT_THAT(target.recovery_action.run_target, Eq("SafeState"));
 }

+TEST_F(FlatbufferConfigLoaderTest, ConfiguredOffRunTargetIsLoadedVerbatim)


Can we add or extend a test to check that an "Off" target is created if none exists?

WilliamRoebuck · 2026-08-17T11:57:24Z

+            auto it = component_name_to_index.find(dep_name);
+            SCORE_LANGUAGE_FUTURECPP_PRECONDITION_MESSAGE(
+                it != component_name_to_index.end(), "Component dependency not found in component list");

-    SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE(states != nullptr, "Process group states not found for process group");
+            graph.addDependency(comp_dep_i, it->second);


This addDependency section is repeated 3 times, can we add a helper function/lambda? Or even better maybe we can iterate through all the dependencies in one loop somehow?

Maybe you can add everything to the graph and then iterate through graph items, accessing the config by index?

WilliamRoebuck · 2026-08-17T13:02:19Z

 /// @details The implementation should be async signal safe.
-OsalReturnType ProcessLauncher::setSchedulingAndSecurity(const OsalConfig& config)
+OsalReturnType ProcessLauncher::setSchedulingAndSecurity(
+    const score::mw::lifecycle::internal::configuration::ComponentConfig& config)


Can we pass just the deployment config here?

WilliamRoebuck · 2026-08-17T13:12:56Z

    alive_monitor_thread_->stop();
-    configuration_.deinitialize();
-    process_groups_.clear();
+    // No deinitialize needed - Config destructor handles cleanup


I don't think we need this comment

WilliamRoebuck · 2026-08-17T13:36:02Z

-    }
-    return result;
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_MESSAGE(bool(graph_), "Graph not initialized");
+    // Convert string_view to IdentifierHash


There's a constructor at IdentifierHash.cpp:66, doesn't it work?

WilliamRoebuck · 2026-08-17T13:39:16Z

+            // TODO: Determine recovery state from configuration
+            recovery_state.pg_state_name_ = IdentifierHash("fallback");  // TODO: Get from configuration


Double // TODO: 🙂

WilliamRoebuck · 2026-08-17T13:39:46Z

+            recovery_state.pg_state_name_ = IdentifierHash("fallback");  // TODO: Get from configuration

-            LM_LOG_WARN() << "Problem discovered in PG" << recovery_state.pg_name_ << "Activating Recovery state.";
+            LM_LOG_WARN() << "Problem discovered, activating recovery state: " << recovery_state.pg_state_name_;


Suggested change

LM_LOG_WARN() << "Problem discovered, activating recovery state: " << recovery_state.pg_state_name_;

LM_LOG_WARN() << "Problem discovered, activating recovery state:" << recovery_state.pg_state_name_;

Extra space

NicolasFussberger · 2026-08-18T06:37:25Z

-            return static_cast<int32_t>(index);
-        }
+        auto off_index = graph.emplace(std::in_place_type<RunTarget>, graph.size());
+        run_target_map.insert({IdentifierHash{"Off"}.data(), off_index});


The "Off" name exists in many locations as hardcoded string. Maybe we can move this to a single constant

NicolasFussberger · 2026-08-18T06:47:45Z

+    // on https://github.com/eclipse-score/lifecycle/issues/463
+    // making the dep_graph a hash map would make this much cleaner as we
+    // wouldn't have to keep track of stuff...
+    const std::vector<configuration::RunTargetConfig> run_targets = config.takeRunTargets();


process group manager still uses configuration_.runTargets() even though its been moved out here

NicolasFussberger · 2026-08-18T06:51:46Z

+    // Off can be configred by the user and would mean we need +1 but
+    // over-reserving is ok.
+    // fallback is always created.
+    const std::size_t graph_size = components.size() + run_targets.size() + 2U;


This calculation exists in a duplicate way also here in process_group_manager.cpp:207

NicolasFussberger · 2026-08-18T06:54:03Z

+        off_rt_defined |= bool(run_target.name == "Off");
+        component_name_to_index[run_target.name] = index;
+        run_target_map.insert({IdentifierHash{run_target.name}.data(), index});
+    };


Suggested change

};

}

NicolasFussberger · 2026-08-18T06:55:10Z


-        for (const auto process_index : state.process_indexes_)
+        off_rt_defined |= bool(run_target.name == "Off");
+        component_name_to_index[run_target.name] = index;


is it ensured that component and run target names are unique, otherwise this may silently overwrite a preexisting name

NicolasFussberger · 2026-08-18T06:55:45Z

+/// @param process_map Map for tracking process PIDs.
+/// @param run_target_map Map to keep the translation between IDHash to Index
+/// @return A populated dependency graph with all components and run targets.
+DependencyGraph<Graph::Component> CreateDependencyGraph(


Its a rather long private method with many parameters. I wonder if this can be refactored to make it simpler and more easily testable by having e.g. a separate GraphBuilder class or something like that

This method will already need to be rewritten as part of #463, so I think it could be left for a separate PR?

Will this ticket really dramatically change the scope of this method?

I suppose not from an external perspective, but I think the implementation inside will change a lot.

NicolasFussberger · 2026-08-18T07:00:52Z

-    return success;
+    graph_ = std::make_shared<Graph>(
+        // size is +2 for fallback + off
+        configuration_.components().size() + configuration_.runTargets().size() + 2,


I wonder if this size can be inferred from the configuration inside the Graph, so that this calculation does not need to exist multiple times

NicolasFussberger · 2026-08-18T07:03:29Z

-    // Single-graph assumption: PGM creates one ProcessMonitor bound to the first (only) graph, so
-    // every event always applies to it. Multi-graph routing is deferred to a future revision.
-    Graph& graph = *process_groups_.front();
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_MESSAGE(bool(graph_), "Graph not initialized");


I suppose we should probably use static_cast over c-style casts

NicolasFussberger · 2026-08-18T07:05:50Z

+    const IdentifierHash old_state = graph_->getProcessGroupState();
+    // the fallback state doesn't have a name in the config, so we use
+    // "fallback", it doesn't actually matter...
+    const IdentifierHash recovery_state("fallback");


This name exists in multiple places as hardcoded string and they need to be identical for things to work, so I would recommend to move this to a single constant.

github-project-automation Bot added this to LCM - Lifecycle & Health FT Aug 12, 2026

github-project-automation Bot moved this to Backlog in LCM - Lifecycle & Health FT Aug 12, 2026

MaciejKaszynski requested a deployment to workflow-approval August 12, 2026 08:17 — with GitHub Actions Waiting

MaciejKaszynski requested a deployment to workflow-approval August 13, 2026 07:13 — with GitHub Actions Waiting

MaciejKaszynski added 8 commits August 13, 2026 09:38

Inital changes

8118405

Rest is working

69ede3d

Fixing rebase

aa2482c

Graph converted

1b738e3

Fixing namespace

8420e6b

lm building

f2ea525

Fixing some tests

50ec974

Fix rebase

84721b9

MaciejKaszynski force-pushed the new-config branch from 48210c0 to 84721b9 Compare August 13, 2026 09:15

MaciejKaszynski requested a deployment to workflow-approval August 13, 2026 09:15 — with GitHub Actions Waiting

Fixing dep graph

7fff290

MaciejKaszynski requested a deployment to workflow-approval August 13, 2026 14:26 — with GitHub Actions Waiting

Some fixes

f961f8a

MaciejKaszynski requested a deployment to workflow-approval August 14, 2026 08:24 — with GitHub Actions Waiting

Adding bad fix

4bbbfed

MaciejKaszynski requested a deployment to workflow-approval August 14, 2026 08:28 — with GitHub Actions Waiting

Fixing last test

ee01564

MaciejKaszynski requested a deployment to workflow-approval August 14, 2026 10:51 — with GitHub Actions Waiting

Fixing UTs

7a58b19

MaciejKaszynski deployed to workflow-approval August 14, 2026 13:45 — with GitHub Actions Active

Merge branch 'main' into new-config

98b3963

MaciejKaszynski requested a deployment to workflow-approval August 14, 2026 14:24 — with GitHub Actions Waiting

MaciejKaszynski marked this pull request as ready for review August 14, 2026 14:24

MaciejKaszynski requested review from FScholPer, NicolasFussberger, anmittag, antonkri, pawelrutkaq and ramceb as code owners August 14, 2026 14:24

danth reviewed Aug 14, 2026

View reviewed changes

Addressing some review comments

7920df4

MaciejKaszynski requested a deployment to workflow-approval August 17, 2026 10:44 — with GitHub Actions Waiting

WilliamRoebuck suggested changes Aug 17, 2026

View reviewed changes

NicolasFussberger reviewed Aug 18, 2026

View reviewed changes

NicolasFussberger requested changes Aug 18, 2026

View reviewed changes

anmittag assigned MaciejKaszynski Aug 18, 2026

		// TODO: Determine recovery state from configuration
		recovery_state.pg_state_name_ = IdentifierHash("fallback"); // TODO: Get from configuration

	LM_LOG_WARN() << "Problem discovered, activating recovery state: " << recovery_state.pg_state_name_;
	LM_LOG_WARN() << "Problem discovered, activating recovery state:" << recovery_state.pg_state_name_;

Uh oh!

Conversation

MaciejKaszynski commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Aug 14, 2026

Uh oh!

danth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

WilliamRoebuck left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

MaciejKaszynski commented Aug 12, 2026 •

edited

Loading

github-actions Bot commented Aug 13, 2026 •

edited

Loading