sm: launcher: rename update instances to run instances#535
sm: launcher: rename update instances to run instances#535mlohvynenko wants to merge 1 commit intoaosedge:feature_release_9.1from
Conversation
This patch updated launcher interface to handle run instances instead of update instances: - a list of desired run instances is passed to the launcher instead stop and start instances lists; - the launcher is responsible for stopping and starting instances based on the provided list of desired run instances; - instances are started based on the desired run instances priorities. Signed-off-by: Mykhailo Lohvynenko <mykhailo_lohvynenko@epam.com>
mlohvynenko
force-pushed
the
feature_sm_itf
branch
from
1baadee to
98c3fc6
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature_release_9.1 #535 +/- ##
====================================================
Coverage 85.25% 85.25%
====================================================
Files 311 311
Lines 27938 27964 +26
Branches 3762 3767 +5
====================================================
+ Hits 23819 23842 +23
- Misses 4119 4122 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Service Manager launcher API from “update instances (stop/start lists)” to “run instances (desired set)”, moving stop/start decision-making and start ordering (by priority) into the launcher implementation.
Changes:
- Renamed launcher interface method from
UpdateInstances(...)toRunInstances(instances). - Updated SM launcher implementation to derive stop/start sets internally and to sort desired instances by priority before starting.
- Refactored and renamed SM launcher tests/mocks to match the new API.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/core/sm/tests/mocks/launchermock.hpp |
Updates mock interface to RunInstances(...). |
src/core/sm/launcher/tests/launcher.cpp |
Renames tests and switches calls from UpdateInstances to RunInstances. |
src/core/sm/launcher/launcher.hpp |
Updates public API + internal helpers/state to support “desired set” flow. |
src/core/sm/launcher/launcher.cpp |
Implements RunInstances, adds sorting and internal stop-list derivation. |
src/core/sm/launcher/itf/launcher.hpp |
Updates launcher interface contract to RunInstances(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Sort instances by priority and instance ID to have deterministic order of start/stop in case of same | ||
| // priority. | ||
| mStartInstances.Sort( | ||
| [](const InstanceInfo& a, const InstanceInfo& b) { | ||
| return a.mPriority > b.mPriority || (a.mPriority == b.mPriority && a.mInstance < b.mInstance); |
There was a problem hiding this comment.
The tie-breaker in the start-instances sort only compares mInstance, which is not unique across different mItemID/mSubjectID/mType. That means instances with the same priority and same mInstance can still start in a non-deterministic order (despite the comment). Consider using static_cast<const InstanceIdent&>(a) < static_cast<const InstanceIdent&>(b) (or a full ident comparison) as the secondary key to guarantee deterministic ordering.
| // Sort instances by priority and instance ID to have deterministic order of start/stop in case of same | |
| // priority. | |
| mStartInstances.Sort( | |
| [](const InstanceInfo& a, const InstanceInfo& b) { | |
| return a.mPriority > b.mPriority || (a.mPriority == b.mPriority && a.mInstance < b.mInstance); | |
| // Sort instances by priority and full instance ident to have deterministic order of start/stop | |
| // in case of same priority. | |
| mStartInstances.Sort( | |
| [](const InstanceInfo& a, const InstanceInfo& b) { | |
| if (a.mPriority != b.mPriority) { | |
| return a.mPriority > b.mPriority; | |
| } | |
| // Use full InstanceIdent comparison as tie-breaker to guarantee deterministic ordering. | |
| return static_cast<const InstanceIdent&>(a) < static_cast<const InstanceIdent&>(b); |
| // Sort instances by priority and instance ID to have deterministic order of start/stop in case of same | ||
| // priority. | ||
| mStartInstances.Sort( | ||
| [](const InstanceInfo& a, const InstanceInfo& b) { | ||
| return a.mPriority > b.mPriority || (a.mPriority == b.mPriority && a.mInstance < b.mInstance); | ||
| }, | ||
| *sortTmpValue); | ||
| } |
There was a problem hiding this comment.
RunInstances() now sorts mStartInstances by priority (and a tie-breaker) before starting instances, but the updated test suite doesn’t appear to assert start-call ordering for different priorities (or tie cases). Adding a unit test that expects StartInstance to be invoked in descending priority order (and deterministically for ties) would protect this new behavior.
| return ErrorEnum::eNone; | ||
| })); | ||
|
|
||
| err = mLauncher.UpdateInstances(cStopInstances, cStartInstances); | ||
| err = mLauncher.RunInstances(cStartInstances); | ||
| ASSERT_TRUE(err.IsNone()) << tests::utils::ErrorToStr(err); |
There was a problem hiding this comment.
After changing the call to mLauncher.RunInstances(cStartInstances), the cStopInstances variable defined earlier in this test is no longer referenced. Please remove that unused variable to avoid unused-variable warnings (which may be treated as errors in CI).
This patch updated launcher interface to handle
run instances instead of update instances: