fix(mqtt): Map mosquitto bridge service health status updates#2903
Conversation
| if self.config.bridge_in_mapper && entity_topic_id.is_bridge_health_topic() { | ||
| // Skip service creation for the mapper-inbuilt bridge, as it is part of the mapper service itself | ||
| return Ok(vec![]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Replaced this with a better logic higher up the stack on the auto-registration path, which was the only path where this logic was relevant.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Robot Results
|
didier-wenzek
left a comment
There was a problem hiding this comment.
The new system tests related to bridge health status are clear and extensive enough to provide the confidence that this PR fix the recently-discovered issues.
However, the code related to health status is still fragile because unstructured.
- There are two similar
HealthStatusstructs defined by c8y related crates (c8y_apiandc8y_mapper_ext). - The respective roles of
tedge_apiandc8y_apiis not clear. Several methods should be moved intotedge_apito encapsulate all the tedge conventions. - This PR introduces
UP_STATUSandDOWN_STATUSconstants, but"up"and"down"literals are used everywhere.
Hence, I'm okay to merge this PR if the point if to have an urgent fix. But to avoid other future regressions, it would be really better to take the time to clean-up health status handling.
| fn entity_is_mosquitto_bridge_service(entity_topic_id: &EntityTopicId) -> bool { | ||
| entity_topic_id | ||
| .default_service_name() | ||
| .filter(|name| { | ||
| name.starts_with(MOSQUITTO_BRIDGE_PREFIX) && name.ends_with(MOSQUITTO_BRIDGE_SUFFIX) | ||
| }) | ||
| .is_some() | ||
| } |
There was a problem hiding this comment.
Shouldn't this function be part of tedge_api?
| // If the auto-registration is done on a health status message, | ||
| // no need to map it to a C8y service creation message here, | ||
| // as the status message itself is mapped into a service creation message | ||
| // in try_convert_data_message called after this auto-registration. | ||
| // This avoids redundant service status creation/mapping |
There was a problem hiding this comment.
yeah. It seems that something can still be improved here.
| return vec![]; | ||
| } | ||
|
|
||
| if entity.topic_id.is_bridge_health_topic() { |
There was a problem hiding this comment.
Beware that the is_bridge_health_topic method is now unused and has to be removed.
| let status = match payload { | ||
| MQTT_BRIDGE_UP_PAYLOAD => UP_STATUS, | ||
| MQTT_BRIDGE_DOWN_PAYLOAD => DOWN_STATUS, | ||
| _ => UNKNOWN_STATUS, | ||
| }; |
There was a problem hiding this comment.
This logic should be moved into tedge_api. It would even be better to have a function taking both the topic and the payload. Indeed, with the current API (a bunch of constants), this is up to the caller to figure out that this is a bridge health topic and that the payload has to be decoded differently).
There was a problem hiding this comment.
Yes, a comprehensive cleanup of the currently fragmented health monitoring code will be done a follow-up PR as mentioned in this comment
rina23q
left a comment
There was a problem hiding this comment.
I confirmed that this PR fixes the reported issues. Thanks Albin.
I approve this state since it fixes the issues and robot tests coverage is good, although I agree with the comments from Didier about c8y_api vs c8y_mapper_ext.
I completely agree and this was the cleanup that I wanted to do as well. But, kept the minimal changes in this PR because of the urgency in fixing the bug. Will definitely do a follow-up PR with all that cleanup. |
gligorisaev
left a comment
There was a problem hiding this comment.
These test cases are well-designed for verifying the mapping of service statuses to Cumulocity managed objects.
They ensure that both valid and invalid inputs are handled appropriately, and the expected outcomes are clearly defined.
Proposed changes
upTypes of changes
Paste Link to the issue
#2902
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments