Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add requester prefix with timestamp to command operations #2381

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

Ruadhri17
Copy link
Contributor

Proposed changes

This PR fixes issue of #2251 with clearing log upload commands but not being a requester. It was also applied to config operations as the problem is basically the same. To make command ids more clearer and easier to debug, current nanoid was replaced with RFC3339 timestamp.

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

#2251

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

@reubenmiller
Copy link
Contributor

reubenmiller commented Oct 27, 2023

@Ruadhri17 Just a few questions/requests:

  1. Can you please add a system test?
  2. Does this PR apply to all of the tedge commands? e.g. what happens if a local restart command is issued, does the mapper still react to it?

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request October 27, 2023 14:47 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #2381 (4049e2f) into main (e11b64f) will decrease coverage by 0.1%.
The diff coverage is 92.7%.

❗ Current head 4049e2f differs from pull request most recent head 673135c. Consider uploading reports for the commit 673135c to get more accurate results

Additional details and impacted files
Files Coverage Δ
...re/c8y_api/src/smartrest/smartrest_deserializer.rs 93.2% <100.0%> (-0.1%) ⬇️
...tes/core/tedge_agent/src/software_manager/actor.rs 65.8% <100.0%> (ø)
...tes/core/tedge_agent/src/software_manager/tests.rs 92.7% <100.0%> (ø)
...tedge_agent/src/tedge_operation_converter/tests.rs 91.4% <100.0%> (-0.1%) ⬇️
crates/core/tedge_api/src/lib.rs 100.0% <100.0%> (ø)
crates/core/tedge_api/src/messages.rs 93.4% <100.0%> (-0.2%) ⬇️
crates/core/tedge_api/src/mqtt_topics.rs 88.3% <100.0%> (+0.4%) ⬆️
...extensions/c8y_mapper_ext/src/config_operations.rs 87.3% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/log_upload.rs 80.0% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/tests.rs 91.8% <100.0%> (ø)
... and 2 more

... and 5 files with indirect coverage changes

@Ruadhri17
Copy link
Contributor Author

  1. Can you please add a system test?

Sure

  1. Does this PR apply to all of the tedge commands? e.g. what happens if a local restart command is issued, does the mapper still react to it?

For now only for log and config operations. However, it is possible to apply it to e.g restart though the structure is a little bit different. I will continue working on that.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
355 0 3 355 100 1h5m30.609s

@didier-wenzek
Copy link
Contributor

  1. Does this PR apply to all of the tedge commands? e.g. what happens if a local restart command is issued, does the mapper still react to it?

For now only for log and config operations. However, it is possible to apply it to e.g restart though the structure is a little bit different. I will continue working on that.

Yes, this should not cause specific issues.

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request October 30, 2023 18:23 — with GitHub Actions Inactive
Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need the clarification to the behaviour when the command is not issued by the requester.

I really believe that the mapper that is not the command issuer should ignore the message. It means c8y-mapper should not publish any status change messages (e.g. EXECUTING, FAILED, SUCCESSFUL) to c8y if the command ID doesn't contain c8y-mapper, which means the command must be addressed by the other mapper.

@didier-wenzek
Copy link
Contributor

I really believe that the mapper that is not the command issuer should ignore the message. It means c8y-mapper should not publish any status change messages (e.g. EXECUTING, FAILED, SUCCESSFUL) to c8y if the command ID doesn't contain c8y-mapper, which means the command must be addressed by the other mapper.

Exactly. The mapper should only emit and process commands with an identifier matching its prefix.

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is correct and I will be happy to approve once the tests fixed.

The unit tests have to be updated with appropriate prefix:

-            &Topic::new_unchecked("te/device/child1///cmd/config_snapshot/1234"),
+            &Topic::new_unchecked("te/device/child1///cmd/config_snapshot/c8y-mapper-1234"),

The failing system test is flaky.

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request October 31, 2023 22:28 — with GitHub Actions Inactive
@didier-wenzek
Copy link
Contributor

I pushed a fix to use the same prefix for all commands forwarded to the mapper.

@reubenmiller
Copy link
Contributor

I pushed a fix to use the same prefix for all commands forwarded to the mapper.

There seems to be other build issues here

@didier-wenzek didier-wenzek temporarily deployed to Test Pull Request November 1, 2023 12:48 — with GitHub Actions Inactive
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Ruadhri17 and others added 4 commits November 1, 2023 14:13
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
- restart
- software_list
- software_update

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@Ruadhri17 Ruadhri17 changed the title Add requester prefix with timestamp to log and config operations' topics Add requester prefix with timestamp to command operations Nov 1, 2023
Signed-off-by: Reuben Miller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller temporarily deployed to Test Pull Request November 1, 2023 15:46 — with GitHub Actions Inactive
@reubenmiller reubenmiller added the theme:mqtt Theme: mqtt and mosquitto related topics label Nov 1, 2023
@reubenmiller reubenmiller merged commit 25efa8f into thin-edge:main Nov 1, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:mqtt Theme: mqtt and mosquitto related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants