Skip to content

feat: support retained-only flag in tedge mqtt sub #3494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 21, 2025
Merged

feat: support retained-only flag in tedge mqtt sub #3494

merged 2 commits into from
Mar 21, 2025

Conversation

reubenmiller
Copy link
Contributor

Proposed changes

Adding support for the --retained-only flag (same function as the mosquitto_sub flag of the same name), to allow users to only get the retained messages from the local MQTT broker.

Note: When subscribing to the retained messages, it will only return the messages with the retain flag set, and it will actively stop when receiving the first non-retained message, or if the user specifies a duration timeout.

Below shows the most common usage of the flag.

tedge mqtt sub '#' --retained-only --duration 1s

The change will also allow the remaining usage of mosquitto_sub in the system tests to be replaced with tedge mqtt sub and also providing a useful tool to users.

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


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check 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 reubenmiller temporarily deployed to Test Pull Request March 21, 2025 08:00 — with GitHub Actions Inactive
@reubenmiller reubenmiller added theme:mqtt Theme: mqtt and mosquitto related topics theme:cli Theme: cli related topics labels Mar 21, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/core/tedge/src/cli/mqtt/subscribe.rs 0.00% 4 Missing ⚠️
crates/core/tedge/src/cli/mqtt/cli.rs 0.00% 3 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

didier-wenzek
didier-wenzek approved these changes Mar 21, 2025
View reviewed changes
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.

Why not.

In practice, this command will behave as expected, but beware that retained messages are weird.

  • There is no guarantee that all the retained messages are sent first on connect
  • The retain flag is cleared for retained message updates.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 21, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
600 0 3 600 100 1h47m43.723689s

@reubenmiller
Copy link
Contributor Author

  • There is no guarantee that all the retained messages are sent first on connect

Yes, though it seems that it is common behaviour (though maybe not in the spec). Or at least when using tedge mqtt sub, we're using cleanSession=true, and a random client id, so in practice I believe it should work fine (at least when we still to mosquitto).

  • The retain flag is cleared for retained message updates.

Yes, this is the part which catches most new users out. The retained flag is only added to messages and sent to the client if the messages are retained on the MQTT broker prior to the client connecting. But if mosquitto_sub already offers it, then it seems to be more-or-less accepted.

@didier-wenzek
Copy link
Contributor

But if mosquitto_sub already offers it, then it seems to be more-or-less accepted.

Yes. And, as long as you're aware of the lack of guarantees, it's really convenient.

@reubenmiller
add retained-only flag to tedge mqtt sub
0aaf182 
Signed-off-by: reubenmiller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller force-pushed the feat-mqtt-sub-retained-only branch from 20cba40 to 407c4de Compare March 21, 2025 10:37
@reubenmiller reubenmiller temporarily deployed to Test Pull Request March 21, 2025 10:37 — with GitHub Actions Inactive
@reubenmiller
Copy link
Contributor Author

Seems like this added a flaky test. I'll review the test to see how it can be made more reliable.

@reubenmiller reubenmiller temporarily deployed to Test Pull Request March 21, 2025 11:58 — with GitHub Actions Inactive
@reubenmiller reubenmiller force-pushed the feat-mqtt-sub-retained-only branch from 79287b5 to 5cf240f Compare March 21, 2025 13:41
@reubenmiller reubenmiller temporarily deployed to Test Pull Request March 21, 2025 13:41 — with GitHub Actions Inactive
@reubenmiller
Copy link
Contributor Author

reubenmiller commented Mar 21, 2025 •
edited
Loading

I was able to reproduce the flaky test locally, and tweak the testings to be more reliable.

I suspect the root cause was the non-retained message was sometimes being published before the subscription was established. Now an additional sleep has been added before publishing the message.

Before

Overall: FAILED
Results: 25 iterations, 18 passed, 7 failed
Elapsed time: 0:15:32.402041
Failed iterations: [10, 17, 18, 21, 22, 23, 24]

After

Overall: PASSED
Results: 25 iterations, 25 passed, 0 failed
Elapsed time: 0:18:20.541304

@reubenmiller
add system test
75ac45f 
Signed-off-by: reubenmiller <reuben.d.miller@gmail.com>
@reubenmiller reubenmiller force-pushed the feat-mqtt-sub-retained-only branch from 5cf240f to 75ac45f Compare March 21, 2025 14:06
@reubenmiller reubenmiller temporarily deployed to Test Pull Request March 21, 2025 14:06 — with GitHub Actions Inactive
@reubenmiller reubenmiller enabled auto-merge March 21, 2025 14:15
@reubenmiller reubenmiller added this pull request to the merge queue Mar 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 21, 2025
@reubenmiller reubenmiller added this pull request to the merge queue Mar 21, 2025
Merged via the queue into thin-edge:main with commit 8a4f7da Mar 21, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@albinsuresh albinsuresh Awaiting requested review from albinsuresh albinsuresh is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

Assignees
No one assigned
Labels
theme:cli Theme: cli related topics theme:mqtt Theme: mqtt and mosquitto related topics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@reubenmiller @didier-wenzek