tedge-mapper-c8y sync with tedge-agent #2050
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Codecov Report
Additional details and impacted files
|
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
albinsuresh
reviewed
Jun 28, 2023
Comment on lines
111
to
117
| // Send the init messages | ||
| if !self.tedge_agent_status && check_tedge_agent_status(&message) { | ||
| self.tedge_agent_status = true; | ||
| self.create_tedge_agent_supported_ops().await?; | ||
| self.send_out_sw_list_request_to_agent().await?; | ||
| } | ||
|
|
There was a problem hiding this comment.
As I had expressed earlier, I still feel that this code better belongs in the converter for the following reasons, which I couldn't convey better in the other PR:
- All the other MQTT messages are already handled there. So it'd be better to keep everything at one place so that it's easier to mange.
- Since there's already a
process_health_status_messagefunction in the converter, this block would have been a better fit there, than here. - That
converteralready has code to handle the software list and stuff like that as well. So, you could reuse quite a lot of that directly from there than introducing newer functions likesend_out_sw_list_request_to_agenthere that callsself.converter.get_software_list_message()anyway. - Not to corrupt the actor further with state fields like
ops_dirandtedge_agent_statusfields as these are just details. Even for theops_dir, it could be read from theC8yMapperConfigthat's already passed toCumulocityConverter::new().
So, I'd still strongly recommend that we keep the actor clean, just with the message routing logic and push all the message processing details into the converter.
There was a problem hiding this comment.
Sure, moved the code into the converter.
| const TEDGE_HEALTH_UP_PAYLOAD: &str = r#""status":"up""#; | ||
| const TEDGE_HEALTH_DOWN_PAYLOAD: &str = r#""status":"down""#; | ||
|
|
||
| pub fn check_tedge_agent_status(message: &Message) -> bool { |
There was a problem hiding this comment.
[Minor] Not sure if this logic really deserved a module of its own. Could have been just a function in the converter.
There was a problem hiding this comment.
moved this also into converter
Comment on lines
11
to
12
| payload.contains(TEDGE_HEALTH_UP_PAYLOAD) | ||
| || payload.contains(TEDGE_HEALTH_DOWN_PAYLOAD) |
There was a problem hiding this comment.
If the point is to make sure that the payload parses well as well, why not just parse it using serde into a struct similar to HealthStatus as done in service_monitor.rs? This payload check is a bit fragile as even the addition of an extra space between the key and value would break it.
...tFramework/tests/cumulocity/supported_operations/mapper-publishing-agent-supported-ops.robot
Show resolved
Hide resolved
albinsuresh
reviewed
Jun 28, 2023
| @@ -50,6 +51,8 @@ pub struct C8yMapperActor { | |||
| messages: SimpleMessageBox<C8yMapperInput, C8yMapperOutput>, | |||
| mqtt_publisher: LoggingSender<MqttMessage>, | |||
| timer_sender: LoggingSender<SyncStart>, | |||
| ops_dir: PathBuf, | |||
| tedge_agent_status: bool, | |||
There was a problem hiding this comment.
I understand that you added this flag to prevent the mapper from repeatedly sending the software list request to the agent every time its status changes. But, may be it's a good thing. What if the agent restarted with a new set of sm-plugins loaded. In that case, it might be better to send the software list request to the agent every time it says it's "up".
There was a problem hiding this comment.
You mean send the software list request only when the status is up?
There was a problem hiding this comment.
Yeah, send the software list request every time we receive an up status. Then, we don't even need this flag.
There was a problem hiding this comment.
But the plan was to send if tedge-agent is up at least once. i.e up/down any message on health topic.
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
reviewed
Jun 28, 2023
Comment on lines
1051
to
1052
| pub pid: Option<u64>, | ||
| pub status: Option<String>, |
There was a problem hiding this comment.
Why Option? A health status should definitely have both these fields, right?
There was a problem hiding this comment.
These topics are open, anything can be published. So, to be safer, if there is no status in the health message then it will not fail.
There was a problem hiding this comment.
Yeah there is the possibility of anyone publishing anything on that topic. But, as the mapper, it should only parse the well-formed health status messages with a valid pid and status fields. Any other malformed messages should just be ignored, without crashing the mapper.
There was a problem hiding this comment.
Yes, right. I did make it String.
| Agent gets the software list request once it comes up | ||
| Execute Command sudo systemctl stop tedge-agent | ||
| # wait till health status down | ||
| Should Have MQTT Messages tedge/health/tedge-agent |
There was a problem hiding this comment.
I don't know if this API behaviour changed recently, but earlier this was checking the MQTT message trace right from the point where the device started up and will check for a message with the given topic anywhere in that trace. But, here I believe you wanted to check if the health status was generated after the tedge-agent was shutdown. For that, you need to pass the date_from parameter as well. You need to pass the message_contains or message_pattern args as well to make sure that the status value is down.
There was a problem hiding this comment.
Added timestamp to check the specific messages.
Comment on lines
41
to
45
| Should Have MQTT Messages tedge/health/tedge-agent | ||
| # now there should be a new list request | ||
| Should Have MQTT Messages tedge/commands/req/software/list | ||
| # agent must process the list request and send response | ||
| Should Have MQTT Messages tedge/commands/res/software/list |
There was a problem hiding this comment.
Same comment as above, regarding the usage of date_from and message_contains parameters.
There was a problem hiding this comment.
Refactored to add the timestamp and check for the specific message
Robot Results
Passed Tests
|
rina23q
reviewed
Jun 30, 2023
| # wait till list request is pushed out | ||
| Should Have MQTT Messages tedge/commands/req/software/list | ||
| # stop mapper and remove the supported operations | ||
| Execute Command sudo systemctl stop tedge-mapper-c8y |
There was a problem hiding this comment.
You can use Stop Service key instead of Execute Command. (Also for restart, start, ...)
Comment on lines
12
to
13
| # wait till list request is pushed out | ||
| Should Have MQTT Messages tedge/commands/req/software/list |
There was a problem hiding this comment.
Refactored. I was thinking just to make sure the list command is issued upon starting tedge-mapper-c8y. Now its moved to the second test and not required here anymore. This one tests the creation of operation files and publishing of operations.
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
albinsuresh
reviewed
Jul 4, 2023
| ThinEdgeIO.start Service tedge-mapper-c8y | ||
| Should Have MQTT Messages tedge/health/tedge-mapper-c8y message_contains=up date_from=${timestamp} | ||
| # After receiving the health status `up` from tege-agent, the mapper creates supported operations and will publish to c8y | ||
| Should Have MQTT Messages tedge/health/tedge-agent message_contains=up |
There was a problem hiding this comment.
Suggested change
| Should Have MQTT Messages tedge/health/tedge-agent message_contains=up | |
| Should Have MQTT Messages tedge/health/tedge-agent message_contains=up date_from=${timestamp} |
albinsuresh
approved these changes
Jul 4, 2023
Signed-off-by: Pradeep Kumar K J <pkj@softwareag.com>
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
force-pushed
the
mapper-check-agent-status
branch
from
445387e
to
ccdfa9b
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Problem:
When the
tedge-mapper-c8ystarts, it will publish thetedge-agentsupported operation to c8y and also sends asoftware listrequest totedge-agent. If thetedge-agentis not up, then thesoftware list requestwill be lost.Proposed solution:
tedge-agentstatus (either up or down)c8y_SoftwareUpdate&c8y_Restartsupported operations filesAdded an integration test as well
Types of changes
Paste Link to the issue
#1201
#1731
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments