Health status messages on startup and shutdown of tedge daemons #1650

PradeepKiruvale · 2022-12-15T12:07:09Z

Proposed changes

The code changes here include sending the health status of the tedge daemon by itself.
When the daemon is ready, it will send
[tedge/health/<tedge-daemon-name>] {"pid":249234,"status":"up","time":1670535886}
When the daemon stops, it will send
[tedge/health/<tedge-daemon-name>] {"pid":249510,"status":"down"}

This has been implemented for all the tedge daemons including tedge-mapper, tedge-agent, tedge-watchdog, c8y-log/configuration-plugin

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

#1503

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

didier-wenzek

The API of mqtt_channel::Config::with_last_will_message() must be fixed.

The test checking last will message must be fixed too.

crates/common/mqtt_channel/Cargo.toml

crates/common/mqtt_channel/src/config.rs

crates/common/mqtt_channel/src/tests.rs

crates/core/tedge_agent/src/agent.rs

didier-wenzek

This will be perfect with a tedge_api::health::get_last_will_message function.

crates/core/tedge_watchdog/src/systemd_watchdog.rs

didier-wenzek

It would be good to fix the test. Or at least, to have some evidence that rumqttd is not supporting last will messages. The failing test is not clear enough to be sure of that.

crates/core/tedge_api/src/health.rs

albinsuresh · 2022-12-17T08:32:12Z

crates/core/tedge_api/src/health.rs

@@ -27,3 +27,16 @@ pub async fn send_health_status(responses: &mut impl PubChannel, daemon_name: &s
    let health_message = Message::new(&response_topic_health, health_status);
    let _ = responses.send(health_message).await;
 }
+
+pub fn get_last_will_message(daemon_name: String) -> Message {


Suggested change

pub fn get_last_will_message(daemon_name: String) -> Message {

pub fn health_status_down_message(daemon_name: String) -> Message {

That's what this message is. We're just using it as a last will message.

Minor: I prefer health_status_down_message as proposed by @albinsuresh rather than get_health_status_down_message as currently proposed.

albinsuresh · 2022-12-17T08:55:25Z

crates/common/mqtt_channel/src/config.rs

@@ -140,6 +155,16 @@ impl Config {

        mqtt_options.set_max_packet_size(self.max_packet_size, self.max_packet_size);

+        if let Some(lwp) = self.last_will_message.clone() {


Suggested change

if let Some(lwp) = self.last_will_message.clone() {

if let Some(lwp) = self.last_will_message {

Since the last_will_message is behind a shared reference, we have to clone it here. Another way is to take the ref &self.last_will_message but the problem is when the struct members (topic, message) are initialized, they have to be cloned because they are behind the shared ref.

albinsuresh · 2022-12-21T06:45:27Z

crates/tests/mqtt_tests/src/test_mqtt_server.rs

-                    let payload = match std::str::from_utf8(bytes.as_ref()) {
+            let notification = match rx.recv().unwrap() {
+                Some(v) => v,
+                None => continue,


Is it a good idea to continue on None? A None is typically returned when the broker closes the channel from the other end, right? Then what's the point in continuing? If it's still valid, better add a comment mentioning why.

From the rumqttd source code comments, I can understand that there can be a situation when there are no messages in the queue, then it returns None. But after some time there can be a newer message as well. So, I feel it's better to continue.

This is coming from this example but the contract of the recv() method is not clear about that.

albinsuresh · 2022-12-21T06:49:54Z

crates/tests/mqtt_tests/src/test_mqtt_server.rs

                    );
                }
+
+                v => {


What are these Notification variants and is it worth printing those? If we're gonna keep it, just add a comment mentioning what these vs are.

Sure, I will add the comment there. These notifications are from the broker for the notifications like `Forward, acks, etc.

Not very useful though: e.g. DeviceAck(SubAck(SubAck { pkid: 0, return_codes: [QoS0] }))

---- tests::tests::ensure_that_last_will_message_is_delivered stdout ---- MQTT-TEST INFO: start test MQTT broker (port = 55555) DeviceAck(SubAck(SubAck { pkid: 0, return_codes: [QoS0] })) MQTT-TEST MSG: topic = b"test/lwp", payload = "hello 1" MQTT-TEST MSG: topic = b"test/lwp", payload = "hello 2" MQTT-TEST MSG: topic = b"test/lwp", payload = "hello 3"

What would be great is to trace new connection as well as disconnection. I would ignore all other rumqttd internal messages.

Sure, I will not print anyother notifications.

albinsuresh · 2022-12-21T06:50:57Z

crates/tests/mqtt_tests/src/test_mqtt_server.rs

        throttle_delay_ms: 0,
        max_payload_size: 268435455,
        max_inflight_count: 200,
-        max_inflight_size: 1024,
-        login_credentials: None,
+        max_inflight_size: 10204,


Intentionally changed that max size?

typo, fixed

albinsuresh · 2022-12-21T06:52:08Z

crates/tests/mqtt_tests/src/test_mqtt_server.rs

    };

    let server_config = ServerSettings {
        listen: SocketAddr::V4(SocketAddrV4::new(Ipv4Addr::new(0, 0, 0, 0), port)),
-        cert: None,
-        next_connection_delay_ms: 1,
+        next_connection_delay_ms: 0,


Why was it zero earlier and why change that to 1 now? What's the impact?

reverted back

crates/tests/mqtt_tests/Cargo.toml

crates/common/mqtt_channel/Cargo.toml

crates/tests/mqtt_tests/src/test_mqtt_server.rs

Tedge daemons will send 'up' message when starts and a lastwill message when it stops Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>

didier-wenzek

Approved

didier-wenzek · 2023-01-04T21:20:35Z

crates/common/mqtt_channel/Cargo.toml

 thiserror = "1.0"
 tokio = { version = "1.12", features = ["rt", "time"] }

 [dev-dependencies]
 anyhow = "1.0"
+maplit = "1.0"


I don't see where this is used.

didier-wenzek · 2023-01-04T21:22:34Z

crates/core/tedge_api/src/health.rs

@@ -27,3 +27,16 @@ pub async fn send_health_status(responses: &mut impl PubChannel, daemon_name: &s
    let health_message = Message::new(&response_topic_health, health_status);
    let _ = responses.send(health_message).await;
 }
+
+pub fn get_last_will_message(daemon_name: String) -> Message {


Minor: I prefer health_status_down_message as proposed by @albinsuresh rather than get_health_status_down_message as currently proposed.

albinsuresh · 2023-01-05T05:37:06Z

crates/common/mqtt_channel/src/connection.rs

@@ -200,8 +199,7 @@ impl Connection {
                    }
                }

-                Ok(Event::Incoming(Incoming::Disconnect))
-                | Ok(Event::Outgoing(Outgoing::Disconnect)) => {
+                Ok(Event::Incoming(Incoming::Disconnect)) => {


Could you explain this change? Why is Outgoing::Disconnect ignored?

Exiting on Event::Outgoing(Outgoing::Disconnect) prevents a client.disconnect() to be effective - since not forwarded by the rumqttc state machine. The impacts were mostly on the tests related to last will messages: this is the only places with explicit client.disconnect() calls.

albinsuresh · 2023-01-05T05:38:32Z

crates/core/tedge_agent/src/agent.rs

@@ -179,7 +180,8 @@ impl SmAgentConfig {
        let mqtt_config = mqtt_channel::Config::default()
            .with_host(tedge_config.query(MqttBindAddressSetting)?.to_string())
            .with_port(tedge_config.query(MqttPortSetting)?.into())
-            .with_max_packet_size(10 * 1024 * 1024);
+            .with_max_packet_size(10 * 1024 * 1024)
+            .with_last_will_message(get_health_status_down_message("tedge-agent"));


Minor: The string literal "tedge-agent" is reused here and in the send_health_status function. Better to move it to a const.

Good point. It's also strange that no session_name is set. The same const should be used for both.

albinsuresh · 2023-01-05T05:39:07Z

crates/core/tedge_watchdog/src/systemd_watchdog.rs

@@ -121,13 +123,17 @@ async fn monitor_tedge_service(
 ) -> Result<(), WatchdogError> {
    let client_id: &str = &format!("{}_{}", name, nanoid!());
    let mqtt_config = get_mqtt_config(tedge_config_location, client_id)?
-        .with_subscriptions(res_topic.try_into()?);
+        .with_subscriptions(res_topic.try_into()?)
+        .with_last_will_message(get_health_status_down_message("tedge-watchdog"));


Minor: Same comment as above: move to a const.

albinsuresh · 2023-01-05T05:39:17Z

plugins/c8y_configuration_plugin/src/config_manager.rs

@@ -132,6 +133,10 @@ impl ConfigManager {

    pub async fn run(&mut self) -> Result<(), anyhow::Error> {
        self.get_pending_operations_from_cloud().await?;
+
+        // Now the configuration plugin is done with the initialization and ready for processing the messages
+        send_health_status(&mut self.mqtt_client.published, "c8y-configuration-plugin").await;


Minor: Same comment as above: move to a const.

Good point.

albinsuresh · 2023-01-05T05:39:22Z

plugins/c8y_log_plugin/src/main.rs

@@ -87,7 +88,8 @@ async fn create_mqtt_client(
    let mqtt_config = mqtt_channel::Config::default()
        .with_session_name("c8y-log-plugin")
        .with_port(mqtt_port)
-        .with_subscriptions(topics);
+        .with_subscriptions(topics)
+        .with_last_will_message(get_health_status_down_message("c8y-log-plugin"));


Minor: Same comment as above: move to a const.

PradeepKiruvale requested review from albinsuresh and rina23q December 15, 2022 12:07

didier-wenzek self-requested a review December 15, 2022 13:52

didier-wenzek requested changes Dec 15, 2022

View reviewed changes

didier-wenzek reviewed Dec 16, 2022

View reviewed changes

crates/core/tedge_watchdog/src/systemd_watchdog.rs Outdated Show resolved Hide resolved

didier-wenzek self-requested a review December 16, 2022 09:11

didier-wenzek reviewed Dec 16, 2022

View reviewed changes

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

reubenmiller added theme:mqtt Theme: mqtt and mosquitto related topics theme:troubleshooting Theme: Troubleshooting and remote control labels Dec 16, 2022

albinsuresh reviewed Dec 17, 2022

View reviewed changes

PradeepKiruvale force-pushed the tedge-daemon-status branch from 88415bc to 85c40a3 Compare December 20, 2022 12:22

albinsuresh reviewed Dec 21, 2022

View reviewed changes

didier-wenzek reviewed Dec 21, 2022

View reviewed changes

crates/tests/mqtt_tests/Cargo.toml Outdated Show resolved Hide resolved

crates/common/mqtt_channel/Cargo.toml Outdated Show resolved Hide resolved

crates/tests/mqtt_tests/src/test_mqtt_server.rs Outdated Show resolved Hide resolved

PradeepKiruvale added 4 commits December 22, 2022 05:37

Add health check messages when tedge daemons start and stop

a4d1fca

Tedge daemons will send 'up' message when starts and a lastwill message when it stops Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>

Revert rumqttd to older version

2980aa9

revert irrelavent changes

3c4a199

unused dependancy

68575e4

PradeepKiruvale force-pushed the tedge-daemon-status branch 3 times, most recently from ef71650 to a4d1fca Compare January 4, 2023 10:59

didier-wenzek approved these changes Jan 4, 2023

View reviewed changes

albinsuresh approved these changes Jan 5, 2023

View reviewed changes

PradeepKiruvale added 2 commits January 5, 2023 11:21

Move all daemon names to const

6abb812

remove unwanted entry

0653dd8

PradeepKiruvale merged commit 47847d6 into thin-edge:main Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Health status messages on startup and shutdown of tedge daemons #1650

Health status messages on startup and shutdown of tedge daemons #1650

PradeepKiruvale commented Dec 15, 2022

didier-wenzek left a comment

didier-wenzek left a comment

didier-wenzek left a comment

albinsuresh Dec 17, 2022

PradeepKiruvale Dec 20, 2022

didier-wenzek Jan 4, 2023

albinsuresh Dec 17, 2022

PradeepKiruvale Dec 20, 2022

albinsuresh Dec 21, 2022

PradeepKiruvale Dec 21, 2022

didier-wenzek Dec 21, 2022 •

edited

albinsuresh Dec 21, 2022

PradeepKiruvale Dec 21, 2022

didier-wenzek Dec 21, 2022 •

edited

PradeepKiruvale Dec 21, 2022

albinsuresh Dec 21, 2022

PradeepKiruvale Dec 21, 2022

albinsuresh Dec 21, 2022

PradeepKiruvale Dec 21, 2022

didier-wenzek left a comment

didier-wenzek Jan 4, 2023

PradeepKiruvale Jan 5, 2023

didier-wenzek Jan 4, 2023

albinsuresh Jan 5, 2023

didier-wenzek Jan 5, 2023

albinsuresh Jan 5, 2023

didier-wenzek Jan 5, 2023

PradeepKiruvale Jan 5, 2023

albinsuresh Jan 5, 2023

albinsuresh Jan 5, 2023

didier-wenzek Jan 5, 2023

PradeepKiruvale Jan 5, 2023

albinsuresh Jan 5, 2023

PradeepKiruvale Jan 5, 2023

	pub fn get_last_will_message(daemon_name: String) -> Message {
	pub fn health_status_down_message(daemon_name: String) -> Message {

		@@ -140,6 +155,16 @@ impl Config {

		mqtt_options.set_max_packet_size(self.max_packet_size, self.max_packet_size);

		if let Some(lwp) = self.last_will_message.clone() {

                                   );
                               }
+                              v => {

Health status messages on startup and shutdown of tedge daemons #1650

Health status messages on startup and shutdown of tedge daemons #1650

Conversation

PradeepKiruvale commented Dec 15, 2022

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

didier-wenzek left a 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

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

didier-wenzek Dec 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didier-wenzek Dec 21, 2022 • edited

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

didier-wenzek 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

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

didier-wenzek Dec 21, 2022 •

edited

didier-wenzek Dec 21, 2022 •

edited