Implement a health-check actor for c8y-device-management plugin #1815
Conversation
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
crates/core/tedge_actors/Cargo.toml
Outdated
| anyhow = "1.0.69" | ||
| async-trait = "0.1" | ||
| futures = { version = "0.3" } | ||
| log = "0.4" | ||
| mqtt_channel = { path = "../../common/mqtt_channel" } |
There was a problem hiding this comment.
Please remove these two extra dependencies on anyhow and mqtt_channel
|
|
||
| #[error(transparent)] | ||
| MqttConnectionError(#[from] mqtt_channel::MqttError), |
There was a problem hiding this comment.
This error is too specific to an error defined at this level.
| #[error(transparent)] | |
| MqttConnectionError(#[from] mqtt_channel::MqttError), |
There was a problem hiding this comment.
Not needed anymore, removed.
| @@ -17,8 +17,10 @@ c8y_log_manager = { path = "../../extensions/c8y_log_manager" } | |||
| env_logger = "0.10" | |||
| log = "0.4" | |||
| tedge_actors = { path = "../../core/tedge_actors" } | |||
| tedge_api = { path = "../../core/tedge_api" } | |||
There was a problem hiding this comment.
It would be better to avoid a direct dependency to tedge_api.
A simple option, to fix that, is to re-export health_status_down_message in tedge_health_ex.
A better option could be to add a set_init_and_last_will method` to the health actor builder to update the MQTT config with the init and last-will messages.
There was a problem hiding this comment.
Added a function set_init_and_last_will. I tried re-exporting those health status helpers functions from tedge_api. But could not avoid the Cargo.toml entry.
| repository = { workspace = true } | ||
|
|
||
| [dependencies] | ||
| anyhow = "1.0" |
There was a problem hiding this comment.
A library should not depend on anyhow. See anyhow comparison to thiserror.
| anyhow = "1.0" |
| #[derive(Error, Debug, Clone, PartialEq, Eq)] | ||
| #[derive(Error, Debug)] |
There was a problem hiding this comment.
There is no reasons to remove these trait implementations.
There was a problem hiding this comment.
This HealthMonitorActor actor is correctly implemented.
| mqtt_config | ||
| .clone() | ||
| .with_session_name(PLUGIN_NAME) | ||
| .with_last_will_message(health_status_down_message(PLUGIN_NAME)), |
There was a problem hiding this comment.
The init message is missing and must be added.
One way, to avoid this kind of miss, is to add a set_init_and_last_will method to the health actor builder to update the MQTT config with the init and last-will messages.
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| @@ -68,6 +69,11 @@ async fn main() -> anyhow::Result<()> { | |||
| log_actor.with_c8y_http_proxy(&mut c8y_http_proxy_actor)?; | |||
| log_actor.with_mqtt_connection(&mut mqtt_actor)?; | |||
|
|
|||
| //Instantiate health monitor actor | |||
| let health_actor = HealthMonitorBuilder::new(PLUGIN_NAME); | |||
| mqtt_actor.mqtt_config = health_actor.set_init_and_last_will(mqtt_actor.mqtt_config); | |||
There was a problem hiding this comment.
Although there's nothing wrong with this approach, I wish this init and last-will messages were also exchanged with the MQTT actor via the Config object of the ServiceConsumer to make it similar to how the TopicFilters are exchanged. But, since the init and last-will messages can only be registered once with the MQTT actor, this is probably fine.
There was a problem hiding this comment.
Good point. It would be indeed nice to have this done when the health actor is connected to the mqtt actor. Regarding the issue that at most one last will message can be set, one can simply raise a LinkError if two actors attempt to set a last-will. This can be done as a follow-up task.
| @@ -32,6 +32,9 @@ pub enum RuntimeError { | |||
|
|
|||
| #[error(transparent)] | |||
| LinkError(#[from] LinkError), | |||
|
|
|||
| #[error(transparent)] | |||
| HealthMonitorError(#[from] anyhow::Error), | |||
There was a problem hiding this comment.
Why are we adding an actor specific error to the core actor crate? If a health-check related error really needs to be a first class citizen in the actor crate (I doubt that's the case), then better to define that as a concrete type rather than as a wrapper over anyhow:Error.
crates/core/tedge_actors/Cargo.toml
Outdated
| @@ -10,6 +10,7 @@ homepage = { workspace = true } | |||
| repository = { workspace = true } | |||
|
|
|||
| [dependencies] | |||
| anyhow = "1.0.69" | |||
There was a problem hiding this comment.
That looks wrong. We've been trying to remove this dependency so far.
There was a problem hiding this comment.
This was removed, forgot to push the changes.
crates/core/tedge_api/src/lib.rs
Outdated
| pub use health::health_status_down_message; | ||
| pub use health::health_status_up_message; |
There was a problem hiding this comment.
It really feels like these 2 functions better belong in the tedge_health_ext crate itself as every plugin wanting the health-check functionality is expected to depend on this crate. But, since all the older plugins are also relying on the same, we can consider moving these in future.
There was a problem hiding this comment.
These pub use are useless here.
| use tedge_api::health_status_up_message; | ||
|
|
||
| pub struct HealthMonitorActor { | ||
| daemon_to_be_monitored: String, |
There was a problem hiding this comment.
| daemon_to_be_monitored: String, | |
| daemon_name: String, |
Optional
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| use tedge_api::health_status_down_message; | ||
| use tedge_api::health_status_up_message; |
There was a problem hiding this comment.
Note. These two use could be pub.
This is no more required with the HealthMonitorBuilder::set_init_and_last_will()
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
| dbg!(message.payload_str().unwrap()); | ||
| assert!(message.payload_str()?.contains("up")) |
There was a problem hiding this comment.
This check could be a bit more strict. What about the topic for instance?
- [minor] please remove the
dbg!
| dbg!(message.payload_str().unwrap()); | ||
| assert!(message.payload_str()?.contains("up")) |
There was a problem hiding this comment.
This check could be a bit more strict. What about the topic for instance?
- [minor] please remove the
dbg!
| let runtime_events_logger = None; | ||
| let mut runtime = Runtime::try_new(runtime_events_logger).await?; |
There was a problem hiding this comment.
It's okay. However, technically, you don't need a runtime to test the actor.
This would even be better because, here you do not control when the runtime is finalized.
| let runtime_events_logger = None; | |
| let mut runtime = Runtime::try_new(runtime_events_logger).await?; |
There was a problem hiding this comment.
refactored and its not required anymore
| let health_actor = HealthMonitorBuilder::new(service_to_be_monitored); | ||
|
|
||
| let health_actor = health_actor.with_connection(&mut health_mqtt_builder); | ||
| runtime.spawn(health_actor).await?; |
There was a problem hiding this comment.
This is okay. However, using a runtime is not required.
| runtime.spawn(health_actor).await?; | |
| let (actor, message_box) = health_mqtt_builder.build(); | |
| tokio::spawn(actor.run(message_box)); |
There was a problem hiding this comment.
Updated the code as suggested above.
PradeepKiruvale
force-pushed
the
health-actor
branch
2 times, most recently
from
7bc2a74
to
15fefe9
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
force-pushed
the
health-actor
branch
from
15fefe9
to
b670f52
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
Proposed changes
Code changes for adding a
health-checkactor.This actor responds to the mqtt requests on
tedge/health-check/#topics about the status of the service ontedge/health/<service-name>topic.Types of changes
Paste Link to the issue
#1768
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments