Support for software list & update on child devices #2307

didier-wenzek · 2023-09-29T14:21:23Z

Proposed changes

Implement software_list support as described by #2235

Use te MQTT scheme
Use commands instead of requests / responses for software list
Update the agent to publish software-list capability on start
Update the agent to respond to software-list requests
Update the c8y mapper to forward software list request to child devices when their agent starts.
Update the c8y mapper to request a software list request after a software-update operation (successful or not)

Implement software_update support as described by #2235

Use commands instead of requests / responses for software update
Update the agent to publish software-update capability on start
Update the agent to respond to software-update requests
Update the c8y mapper to to forward software update requests
Publish the list of supported software types along the capability message. See Use Softwaretype according to new Advanced Software Management in 10.14 #1352

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

crates/core/tedge_api/src/messages.rs

codecov · 2023-09-29T14:59:18Z

Codecov Report

Merging #2307 (352388d) into main (af49e4d) will decrease coverage by 0.4%.
The diff coverage is 79.3%.

Additional details and impacted files

Files	Coverage Δ
crates/core/c8y_api/src/json_c8y.rs	`87.0% <100.0%> (+0.2%)`	⬆️
crates/core/c8y_api/src/smartrest/error.rs	`0.0% <ø> (ø)`
...re/c8y_api/src/smartrest/smartrest_deserializer.rs	`93.3% <100.0%> (-0.2%)`	⬇️
...core/c8y_api/src/smartrest/smartrest_serializer.rs	`83.7% <ø> (-5.2%)`	⬇️
crates/core/c8y_api/src/smartrest/topic.rs	`92.1% <100.0%> (+4.1%)`	⬆️
...s/core/tedge_agent/src/software_manager/builder.rs	`75.0% <100.0%> (ø)`
...dge_agent/src/tedge_operation_converter/builder.rs	`91.3% <100.0%> (-0.7%)`	⬇️
crates/core/tedge_api/src/lib.rs	`100.0% <100.0%> (ø)`
crates/core/tedge_api/src/mqtt_topics.rs	`87.6% <100.0%> (-0.7%)`	⬇️
crates/core/tedge_api/src/topic.rs	`100.0% <ø> (+2.8%)`	⬆️
... and 15 more

... and 5 files with indirect coverage changes

github-actions · 2023-09-29T15:10:16Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
345	0	3	345	100	1h1m46.986999999s

crates/extensions/c8y_mapper_ext/src/converter.rs

albinsuresh

LGTM. Happy to approve after a test run once the merge conflicts are resolved.

crates/extensions/c8y_mapper_ext/src/converter.rs

albinsuresh · 2023-10-16T04:06:26Z

crates/extensions/c8y_mapper_ext/src/converter.rs

-            let smartrest_set_operation_status =
-                SmartRestSetOperationToExecuting::from_thin_edge_json(response)?.to_smartrest()?;
-            Ok(vec![Message::new(&topic, smartrest_set_operation_status)])
+    async fn register_software_list_operation(


Should we use this PR as an opportunity to refactor out all the software management related functions from this converter.rs module to a dedicated software_operation.rs, like log_upload.rs or config_operation.rs, so that this module is leaner?

Indeed, it would be good to have operation specific modules or even actor extensions. However, I would prefer to address that later once both software list and software update fully ported to support child devices.

crates/extensions/c8y_mapper_ext/src/tests.rs

albinsuresh

Happy to approve once the system test failures are sorted.

crates/extensions/c8y_mapper_ext/src/converter.rs

crates/extensions/c8y_mapper_ext/src/tests.rs

didier-wenzek · 2023-10-18T08:30:52Z

The failing system test is related to self-update: thin-edge used to update thin-edge on the device.

So issue is that this test doesn't update tedge-agent but only tedge-mapper (and other tedge executables unrelated to the issue). But the new tedge-mapper is not compatible with the old tedge-agent on how a software-list is requested. Hence, the software list is is not updated on c8y, failing the test.

reubenmiller · 2023-10-18T09:13:15Z

The failing system test is related to self-update: thin-edge used to update thin-edge on the device.

So issue is that this test doesn't update tedge-agent but only tedge-mapper (and other tedge executables unrelated to the issue). But the new tedge-mapper is not compatible with the old tedge-agent on how a software-list is requested. Hence, the software list is is not updated on c8y, failing the test.

So this is the first problem that has popped up due to the fact that the tedge-agent service is not restarted during the installation process, so the old instance continues running after the new version has been installed. We will have to solve this in the workflow, otherwise it will lead to very unexpected results for users.

albinsuresh

The core changes look good. Just some refactoring suggestions.

crates/core/tedge_agent/src/software_manager/actor.rs

albinsuresh · 2023-10-26T07:56:34Z

tests/RobotFramework/tests/cumulocity/software_management/software-update-child.robot

+    Execute Command    sudo tedge config set mqtt.topic_root te
+    Execute Command    sudo tedge config set mqtt.device_topic_id "device/${CHILD_SN}//"
+
+    # Install plugin after the default settings have been updated to prevent it from starting up as the main plugin


That's a huge UX issue that we'll have to solve somehow. Most customers wouldn't do it this way. They'd install everything first and then try to configure things, right? May be we should consider not starting tedge-agent by default, unless it was already up and running (during updates). It's probably not that easy considering self-update and other update paths.

Yes we still have some conceptual work to do here as a follow up, however I wouldn't look at the exact solution until we consolidate the number of services/processes.

However running thin-edge.io in a container is easier to change this as you can set an environment variable to affect the behaviour of each component. systemd does not support inheriting environment variables.

albinsuresh · 2023-10-26T08:07:25Z

crates/core/tedge_api/src/messages.rs

+    Payload: Default,
+{
+    /// Build a new command with a random id
+    pub fn new(target: &EntityTopicId) -> Self {


Suggested change

pub fn new(target: &EntityTopicId) -> Self {

pub fn new(schema: MqttSchema, target: &EntityTopicId) -> Self {

Seeing the topic(), capability_message(), command_message() and clearing_message() all taking the schema separately, it feels like the schema should have been a part of the constructor. It's not something that's gonna change dynamically between these function calls, right?

Having the schema attached to a message looks upside down.

I would prefer to address this in a more generic manner as proposed here #2357

crates/extensions/c8y_http_proxy/src/actor.rs

albinsuresh · 2023-10-26T09:02:21Z

crates/extensions/c8y_mapper_ext/src/config.rs

@@ -108,8 +107,15 @@ impl C8yMapperConfig {
        let mut topics = Self::default_internal_topic_filter(&config_dir)?;

        // Add feature topic filters
-        topics.add_all(mqtt_schema.topics(AnyEntity, Command(OperationType::Restart)));
-        topics.add_all(mqtt_schema.topics(AnyEntity, CommandMetadata(OperationType::Restart)));
+        for cmd in [


Minor:

Suggested change

for cmd in [

for cmd_type in [

tests/RobotFramework/tests/cumulocity/software_management/software-update-child.robot

albinsuresh · 2023-10-26T10:22:07Z

crates/core/tedge_api/src/lib.rs

+    /*
+
+        #[test]
+        fn using_a_software_update_response() {


Remove that test? Looks like an irrelevant test as software update response doesn't carry the software list anymore.

crates/extensions/c8y_mapper_ext/src/converter.rs

reubenmiller

Approved. Great effort!

albinsuresh

LGTM

albinsuresh · 2023-10-27T13:59:16Z

tests/RobotFramework/tests/cumulocity/self-update/tedge_self_update.robot

+    Device Should Have Installed Software    tedge,${NEW_VERSION_ESCAPED}::apt    tedge-mapper,${NEW_VERSION_ESCAPED}::apt    tedge-agent,${NEW_VERSION_ESCAPED}::apt    tedge-watchdog,${NEW_VERSION_ESCAPED}::apt    tedge-configuration-plugin,${NEW_VERSION_ESCAPED}::apt    tedge-log-plugin,${NEW_VERSION_ESCAPED}::apt    tedge-apt-plugin,${NEW_VERSION_ESCAPED}::apt
+
+    # Software list reported by the new agent
+    Restart Service    tedge-agent


Does this mean that end-user needs to explicitly restart the tedge-agent after every update? Or that has always been the case?

Yes. This is the case since the work on self-update. Restarting the agent during a software update operation leads to a fail operation status.

This is something we will need to fix, sure.

albinsuresh · 2023-10-27T14:53:12Z

crates/extensions/c8y_mapper_ext/src/compatibility_adapter.rs

+                return Ok(None);
+            }
+            request.insert("id".to_string(), Value::String(cmd_id.to_string()));
+            request.remove("status"); // as the old agent denies unknown fields


Minor:

Suggested change

request.remove("status"); // as the old agent denies unknown fields

request.remove("status"); // as the old agent denies unknown status values like `init`

albinsuresh · 2023-10-27T15:05:14Z

crates/extensions/c8y_mapper_ext/src/compatibility_adapter.rs

+        "Fail to extract command 'id' from agent {cmd_type} response: {}",
+        std::str::from_utf8(payload).unwrap_or("non utf8 payload")
+    ))
+}


I'd add some unit test coverage, if there's time, especially for the convert_to_old_agent_request function which has some deep nested logic.

This is first step toward their deprecation. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

A huge change but straightforward. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

The aim is to be able to handle the case where the mapper has been updated but not the agent. This will typically arises during a self update where the tedge packages are updated by the agent. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek requested review from albinsuresh and Bravo555 September 29, 2023 14:21

didier-wenzek commented Sep 29, 2023

View reviewed changes

crates/core/tedge_api/src/messages.rs Outdated Show resolved Hide resolved

crates/core/tedge_api/src/messages.rs Outdated Show resolved Hide resolved

didier-wenzek had a problem deploying to Test Pull Request September 29, 2023 14:32 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 372a181 to 40dc038 Compare September 29, 2023 14:41

didier-wenzek had a problem deploying to Test Pull Request September 29, 2023 14:52 — with GitHub Actions Failure

didier-wenzek mentioned this pull request Sep 29, 2023

Update the mappers to consume MEA from the new topics #2082

Closed

11 tasks

didier-wenzek had a problem deploying to Test Pull Request September 29, 2023 22:00 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request September 30, 2023 00:32 — with GitHub Actions Failure

Bravo555 approved these changes Oct 15, 2023

View reviewed changes

crates/extensions/c8y_mapper_ext/src/converter.rs Outdated Show resolved Hide resolved

albinsuresh reviewed Oct 16, 2023

View reviewed changes

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 31acc8f to 88d9d74 Compare October 17, 2023 07:30

didier-wenzek had a problem deploying to Test Pull Request October 17, 2023 07:42 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request October 17, 2023 08:29 — with GitHub Actions Failure

albinsuresh reviewed Oct 18, 2023

View reviewed changes

crates/extensions/c8y_mapper_ext/src/converter.rs Show resolved Hide resolved

crates/extensions/c8y_mapper_ext/src/tests.rs Show resolved Hide resolved

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 08:55 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 86d799f to d87ed9d Compare October 18, 2023 09:21

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 09:32 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from d87ed9d to 4f0655e Compare October 18, 2023 12:33

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 12:48 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 14:14 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 15:57 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch 2 times, most recently from 21d3e01 to 8372b7d Compare October 18, 2023 16:30

didier-wenzek had a problem deploying to Test Pull Request October 18, 2023 16:43 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 8372b7d to 2062b6c Compare October 18, 2023 17:03

albinsuresh reviewed Oct 26, 2023

View reviewed changes

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from e81959f to ddbe805 Compare October 26, 2023 11:21

didier-wenzek had a problem deploying to Test Pull Request October 26, 2023 11:40 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request October 26, 2023 18:09 — with GitHub Actions Failure

didier-wenzek had a problem deploying to Test Pull Request October 26, 2023 19:19 — with GitHub Actions Failure

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 92af9ad to 3e0466f Compare October 27, 2023 08:17

didier-wenzek had a problem deploying to Test Pull Request October 27, 2023 08:31 — with GitHub Actions Failure

reubenmiller added the theme:software Theme: Software management label Oct 27, 2023

didier-wenzek had a problem deploying to Test Pull Request October 27, 2023 10:09 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request October 27, 2023 10:32 — with GitHub Actions Inactive

didier-wenzek had a problem deploying to Test Pull Request October 27, 2023 11:38 — with GitHub Actions Failure

didier-wenzek temporarily deployed to Test Pull Request October 27, 2023 12:02 — with GitHub Actions Inactive

reubenmiller approved these changes Oct 27, 2023

View reviewed changes

reubenmiller mentioned this pull request Oct 27, 2023

Software management for child devices thin-edge/thin-edge.io-roadmap#38

Closed

albinsuresh approved these changes Oct 27, 2023

View reviewed changes

didier-wenzek added 8 commits October 27, 2023 17:48

Simplify definition of tedge/commands topics

cb37f65

This is first step toward their deprecation. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Replace Software Req/Res with a Command

c411d4f

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Simplify tedge Command message implementation

c8e2d50

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Add SoftwareUpdateCommand

25ae1e9

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Fix self-update test

078df2c

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Replace SoftwareUpdate Req/Res with SoftwareUpdateCommand

d39df67

A huge change but straightforward. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

Add system test for child-device software updates

481ba08

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>

didier-wenzek force-pushed the feat/tedge-agent-software-update-child-devices branch from 9555293 to 352388d Compare October 27, 2023 15:48

didier-wenzek temporarily deployed to Test Pull Request October 27, 2023 16:03 — with GitHub Actions Inactive

didier-wenzek merged commit 1fcce2c into thin-edge:main Oct 27, 2023
18 checks passed

didier-wenzek deleted the feat/tedge-agent-software-update-child-devices branch October 27, 2023 18:31

This was referenced Oct 30, 2023

tedge-agent documentation #1470

Closed

Update tedge-agent to work with new topics #2223

Closed

Support running tedge-agent on a child device #2018

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for software list & update on child devices #2307

Support for software list & update on child devices #2307

didier-wenzek commented Sep 29, 2023 •

edited

codecov bot commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023 •

edited

albinsuresh left a comment

albinsuresh Oct 16, 2023

didier-wenzek Oct 17, 2023

albinsuresh left a comment

didier-wenzek commented Oct 18, 2023

reubenmiller commented Oct 18, 2023

albinsuresh left a comment

albinsuresh Oct 26, 2023

reubenmiller Oct 27, 2023

reubenmiller Oct 27, 2023

albinsuresh Oct 26, 2023

didier-wenzek Oct 27, 2023

albinsuresh Oct 26, 2023

albinsuresh Oct 26, 2023

reubenmiller left a comment

albinsuresh left a comment

albinsuresh Oct 27, 2023

didier-wenzek Oct 27, 2023

albinsuresh Oct 27, 2023

albinsuresh Oct 27, 2023

	pub fn new(target: &EntityTopicId) -> Self {
	pub fn new(schema: MqttSchema, target: &EntityTopicId) -> Self {

	request.remove("status"); // as the old agent denies unknown fields
	request.remove("status"); // as the old agent denies unknown status values like `init`

Support for software list & update on child devices #2307

Support for software list & update on child devices #2307

Conversation

didier-wenzek commented Sep 29, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

codecov bot commented Sep 29, 2023 • edited

Codecov Report

github-actions bot commented Sep 29, 2023 • edited

Robot Results

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

didier-wenzek commented Oct 18, 2023

reubenmiller commented Oct 18, 2023

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reubenmiller left a comment

Choose a reason for hiding this comment

albinsuresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek commented Sep 29, 2023 •

edited

codecov bot commented Sep 29, 2023 •

edited

github-actions bot commented Sep 29, 2023 •

edited