Add `mqtt.topic_root` and `mqtt.device_topic_id` options #2231

Bravo555 · 2023-09-07T14:10:40Z

Proposed changes

Settings mqtt.topic_root have been mqtt.device_topic_id added to tedge-config and used inside tedge-agent, enabling the agent to be run on a child device and connect to a parent's MQTT broker.
Also --mqtt-topic-root and --device-topic-id flags have been added to tedge-agent, which can be used to override these values if taken from a configuration file.
tedge-log-manager was updated to use new mqtt.topic_root and mqtt.device_topic_id config options, still allowing them to be overridden by cli flags. The naming of the flags was made consistent with tedge-agent and some code was cleaned up.

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

#2018

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

This PR completes the Make tedge-agent running on any device section of this TODO list

Bravo555 · 2023-09-07T14:17:54Z

crates/core/tedge_agent/src/agent.rs

@@ -38,22 +40,35 @@ pub struct AgentConfig {
    pub use_lock: bool,
    pub log_dir: Utf8PathBuf,
    pub data_dir: Utf8PathBuf,
+    pub mqtt_device_topic_id: Arc<str>,


Using Arc<str> here because it's better for immutable data, as it doesn't allocate more memory when cloning, and also tells the reader that it's immutable. More information in this video.
I think most of the data we're working with in thin-edge is immutable, so I'm experimenting with using these immutable types for new code.

Thank you. Indeed a nice hint, that can be used for all the Strings we use as names or identifiers.

This is really nice! It's a great help to distinguish if a member is immutable or mutable by just looking at the struct data type.

Bravo555 · 2023-09-07T14:22:42Z

crates/core/tedge_agent/src/agent.rs

 }

 impl AgentConfig {
-    pub fn from_tedge_config(
+    pub fn from_config_and_cliopts(


We should try to find a more systematic solution to merging CLI arguments into a common config, that can be applied across all of our binaries. I think that's what figment does and we already use it to handle environment variables, but for now I wanted to push this ASAP so I haven't looked into it yet.

Yes this would be nice to have.

Bravo555 · 2023-09-07T14:25:26Z

plugins/tedge_log_plugin/src/main.rs

-    #[clap(long, default_value = "te")]
-    root: String,
+    #[clap(long)]
+    mqtt_topic_root: Option<Arc<str>>,


Renamed flags here to be consistent with tedge-agent, also because I think --root or --device do not give much context to the user of what these options are for, although probably this should be accompanied by help text.

Understood. Consistency is most important!

codecov · 2023-09-07T14:28:06Z

Codecov Report

Merging #2231 (73723ae) into main (41a412c) will decrease coverage by 0.1%.
The diff coverage is 13.3%.

❗ Current head 73723ae differs from pull request most recent head bb1ee36. Consider uploading reports for the commit bb1ee36 to get more accurate results

Additional details and impacted files

Files Changed	Coverage Δ
crates/core/tedge_agent/src/agent.rs	`0.0% <0.0%> (ø)`
crates/core/tedge_agent/src/main.rs	`0.0% <0.0%> (ø)`
crates/extensions/tedge_log_manager/src/config.rs	`0.0% <0.0%> (ø)`
plugins/tedge_log_plugin/src/main.rs	`0.0% <0.0%> (ø)`
.../tedge_config/src/tedge_config_cli/tedge_config.rs	`89.1% <75.0%> (-0.3%)`	⬇️
crates/extensions/tedge_log_manager/src/tests.rs	`92.2% <100.0%> (ø)`

... and 17 files with indirect coverage changes

github-actions · 2023-09-07T15:08:40Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
270	0	5	270	100	55m17.121s

didier-wenzek · 2023-09-07T15:34:38Z

crates/core/tedge_agent/src/agent.rs

-        let health_actor = HealthMonitorBuilder::new(TEDGE_AGENT, &mut mqtt_actor_builder);
+        let health_actor = HealthMonitorBuilder::new("health-actor", &mut mqtt_actor_builder);


No, the name which is expected here is the name of the service.

This is one of the reasons why the system tests are failing. They expect the "tedge-agent" to be up, not some unknown "health-actor".

However, using "tedge-agent" here can only be a short term solution. To have the agent running concurrently on several devices, one needs instance specific health status. The better would be to provide here the service topic id. Ignoring the issue with custom device topic id, the service topic id can be forged after the parent topic id. For instance device/child001/service/tedge-agent.

I wanted to write a proof of concept for health checks under a new topic scheme and tweaked the converter to convert both ways from old topics to new topics and vice versa, but I didn't know that when subscribing and publishing on the same topic, the client will be sent messages it itself published, resulting in an infinite loop.

This is fixed in MQTT 5, where the subscription option No Local is used to ask the broker not to echo messages back to the client which published it, but according to @reubenmiller it will be some time before we can use MQTT 5.

To detect whether or not the message is something we published just echoed back to us, we'd have to include some marker data or add other non-trivial logic, so for now I opted to just remain with TEDGE_AGENT for health checks.

Ruadhri17

Looks good (Apart from the health actor service name)

rina23q

Looks good in general!

rina23q · 2023-09-08T13:00:57Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

@@ -435,6 +435,14 @@ define_tedge_config! {
    },

    mqtt: {
+        /// MQTT topic root


This comment goes directly to the description showed by tedge config --doc. So I want to have a bit better description. (Another suggestion is welcomed!)

Suggested change

/// MQTT topic root

/// The MQTT root/base topic prefix

I vote for MQTT root topic prefix as the word base is used nowhere in the docs.

rina23q · 2023-09-08T13:03:34Z

crates/core/tedge_agent/src/agent.rs

@@ -38,22 +40,35 @@ pub struct AgentConfig {
    pub use_lock: bool,
    pub log_dir: Utf8PathBuf,
    pub data_dir: Utf8PathBuf,
+    pub mqtt_device_topic_id: Arc<str>,


This is really nice! It's a great help to distinguish if a member is immutable or mutable by just looking at the struct data type.

rina23q · 2023-09-08T13:10:02Z

plugins/tedge_log_plugin/src/main.rs

-    #[clap(long, default_value = "te")]
-    root: String,
+    #[clap(long)]
+    mqtt_topic_root: Option<Arc<str>>,


Understood. Consistency is most important!

Settings `mqtt.topic_root` have been `mqtt.device_topic_id` added to tedge-config and used inside tedge-agent, enabling the agent to be run on a child device and connect to a parent's MQTT broker. Also `--mqtt-topic-root` and `--device-topic-id` flags have been added to `tedge-agent` Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

`tedge-log-manager` was updated to use new `mqtt.topic_root` and `mqtt.device_topic_id` config options, still allowing them to be overridden by cli flags. The naming of the flags was made consistent with `tedge-agent` and some code was cleaned up. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>

didier-wenzek

I'm okay to approve this PR. I just need a confirmation from @reubenmiller about the names for the two new config settings.

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

didier-wenzek

Approved

Align with the change made by thin-edge#2231 * --device => --mqtt-device-topic-id * --root => --mqtt-topic-root Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com>

Bravo555 requested review from didier-wenzek and Ruadhri17 September 7, 2023 14:11

Bravo555 had a problem deploying to Test Pull Request September 7, 2023 14:23 — with GitHub Actions Failure

Bravo555 commented Sep 7, 2023

View reviewed changes

Bravo555 added theme:cli Theme: cli related topics theme:mqtt Theme: mqtt and mosquitto related topics labels Sep 7, 2023

didier-wenzek requested changes Sep 7, 2023

View reviewed changes

Ruadhri17 reviewed Sep 8, 2023

View reviewed changes

rina23q mentioned this pull request Sep 8, 2023

feat: tedge-log-plugin packaging and tests #2213

Merged

12 tasks

rina23q reviewed Sep 8, 2023

View reviewed changes

Bravo555 force-pushed the agent-on-child branch from 73723ae to c8b632e Compare September 11, 2023 15:51

Bravo555 added 2 commits September 11, 2023 15:57

Bravo555 force-pushed the agent-on-child branch from c8b632e to bb1ee36 Compare September 11, 2023 15:57

Bravo555 temporarily deployed to Test Pull Request September 11, 2023 16:08 — with GitHub Actions Inactive

didier-wenzek reviewed Sep 11, 2023

View reviewed changes

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs Show resolved Hide resolved

didier-wenzek approved these changes Sep 12, 2023

View reviewed changes

Bravo555 merged commit d42b117 into thin-edge:main Sep 12, 2023
16 checks passed

Bravo555 deleted the agent-on-child branch September 12, 2023 10:17

Bravo555 mentioned this pull request Sep 12, 2023

Support running tedge-agent on a child device #2018

Closed

rina23q mentioned this pull request Sep 19, 2023

Create generic tedge configuration plugin #2238

Merged

13 tasks

rina23q mentioned this pull request Sep 27, 2023

Move cloud specific logic out of plugins #2017

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `mqtt.topic_root` and `mqtt.device_topic_id` options #2231

Add `mqtt.topic_root` and `mqtt.device_topic_id` options #2231

Bravo555 commented Sep 7, 2023 •

edited

Bravo555 Sep 7, 2023

didier-wenzek Sep 7, 2023

rina23q Sep 8, 2023

Bravo555 Sep 7, 2023

didier-wenzek Sep 7, 2023

Bravo555 Sep 7, 2023

rina23q Sep 8, 2023

codecov bot commented Sep 7, 2023 •

edited

github-actions bot commented Sep 7, 2023 •

edited

didier-wenzek Sep 7, 2023

Bravo555 Sep 11, 2023 •

edited

Ruadhri17 left a comment

rina23q left a comment

rina23q Sep 8, 2023

didier-wenzek Sep 8, 2023

rina23q Sep 8, 2023

rina23q Sep 8, 2023

didier-wenzek left a comment

didier-wenzek left a comment

		let health_actor = HealthMonitorBuilder::new(TEDGE_AGENT, &mut mqtt_actor_builder);
		let health_actor = HealthMonitorBuilder::new("health-actor", &mut mqtt_actor_builder);

Add mqtt.topic_root and mqtt.device_topic_id options #2231

Add mqtt.topic_root and mqtt.device_topic_id options #2231

Conversation

Bravo555 commented Sep 7, 2023 • edited

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

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

codecov bot commented Sep 7, 2023 • edited

Codecov Report

github-actions bot commented Sep 7, 2023 • edited

Robot Results

Choose a reason for hiding this comment

Bravo555 Sep 11, 2023 • edited

Choose a reason for hiding this comment

Ruadhri17 left a comment

Choose a reason for hiding this comment

rina23q 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 left a comment

Choose a reason for hiding this comment

didier-wenzek left a comment

Choose a reason for hiding this comment

Add `mqtt.topic_root` and `mqtt.device_topic_id` options #2231

Add `mqtt.topic_root` and `mqtt.device_topic_id` options #2231

Bravo555 commented Sep 7, 2023 •

edited

codecov bot commented Sep 7, 2023 •

edited

github-actions bot commented Sep 7, 2023 •

edited

Bravo555 Sep 11, 2023 •

edited